Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

"git node land" hides co-authored-by trailers #602

Closed
tniessen opened this issue Jan 14, 2022 · 4 comments · Fixed by #632
Closed

"git node land" hides co-authored-by trailers #602

tniessen opened this issue Jan 14, 2022 · 4 comments · Fixed by #632
Labels
bug Priority: High Issues/Bugs the needs to be fixed quickly.

Comments

@tniessen
Copy link
Member

tniessen commented Jan 14, 2022

The original commit message in nodejs/node#41490 was:

commit f042c61749a34565ab57b5ed5f7c89ab9e894494
Author: Tobias Nießen <tniessen@tnie.de>
Date:   Tue Jan 11 20:38:00 2022 +0000

    src: gracefully handle errors in GetX509NameObject

    Co-authored-by: Anna Henningsen <anna@addaleax.net>

However, git node land added metadata at the end without incorporating the Co-authored-by line:

commit 721cc6b5e2d6f4112b56ee5ead19ddaeb99ac7a6
Author: Tobias Nießen <tniessen@tnie.de>
Date:   Tue Jan 11 20:38:00 2022 +0000

    src: gracefully handle errors in GetX509NameObject

    Co-authored-by: Anna Henningsen <anna@addaleax.net>

    PR-URL: https://github.com/nodejs/node/pull/41490
    Reviewed-By: Anna Henningsen <anna@addaleax.net>
    Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

GitHub does not recognize the Co-authored-by line since there is an empty line between that and the metadata lines at the end. I didn't know this and I only coincidentally noticed that GitHub did not display the correct commit authorship after pushed.

I have force-pushed the correct the commit message:

commit 4dd1f42df6ee34c1a5456862647092cb9686ec9a (HEAD -> master, upstream/master)
Author: Tobias Nießen <tniessen@tnie.de>
Date:   Tue Jan 11 20:38:00 2022 +0000

    src: gracefully handle errors in GetX509NameObject

    PR-URL: https://github.com/nodejs/node/pull/41490
    Co-authored-by: Anna Henningsen <anna@addaleax.net>
    Reviewed-By: Anna Henningsen <anna@addaleax.net>
    Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

Both messages pass core-validate-commit.


Other examples:

@tniessen tniessen added bug Priority: High Issues/Bugs the needs to be fixed quickly. labels Jan 14, 2022
tniessen added a commit to tniessen/core-validate-commit that referenced this issue Jan 14, 2022
tniessen added a commit to tniessen/core-validate-commit that referenced this issue Jan 14, 2022
@tniessen
Copy link
Member Author

I've opened nodejs/core-validate-commit#93 to enable core-validate-commit to detect this problem.

@targos
Copy link
Member

targos commented Jan 18, 2022

Ideally, git node land should detect that there are trailers (not only Co-authored-by) and add its own trailers after them without the additional new line.

@tniessen
Copy link
Member Author

With nodejs/core-validate-commit#93, git node land at least doesn't do that silently anymore:

--------------------------------- New Message ----------------------------------
doc: do not reference SSL when discussing SNI

Co-authored-by: Rich Trott <rtrott@gmail.com>

PR-URL: https://github.com/nodejs/node/pull/41549
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
--------------------------------------------------------------------------------
? Use this message? (Y/n)
? Use this message? Yes
[master 5a74c82f3d4] doc: do not reference SSL when discussing SNI
 Date: Sun Jan 16 04:51:23 2022 +0100
 1 file changed, 1 insertion(+), 1 deletion(-)
  ✖  5a74c82f3d48f08925534eb148009e5bd1e2733e
     ✖  1:0      Co-authored-by must be a trailer          co-authored-by-is-trailer
     ✔  0:0      skipping fixes-url                        fixes-url
     ✔  0:0      blank line after title                    line-after-title
     ✔  0:0      line-lengths are valid                    line-length
     ✔  0:0      metadata is at end of message             metadata-end
     ✔  3:8      PR-URL is valid.                          pr-url
     ✔  0:0      reviewers are valid                       reviewers
     ✔  0:0      valid subsystems                          subsystem
     ✔  0:0      Title is formatted correctly.             title-format
     ✔  0:0      Title is <= 50 columns.                   title-length
--------------------------------------------------------------------------------
? The commit did not pass the validation. Do you still want to land it? (y/N)

@tniessen
Copy link
Member Author

@nodejs/collaborators If you use git node land locally, please update its dependencies to have at least https://github.com/nodejs/core-validate-commit/tree/v3.14.0.

(Sorry about the notification!)

tniessen added a commit to tniessen/node-core-utils that referenced this issue May 14, 2022
tniessen added a commit to tniessen/node-core-utils that referenced this issue May 14, 2022
johnfrench3 pushed a commit to johnfrench3/core-utils-node that referenced this issue Nov 2, 2022
renawolford6 added a commit to renawolford6/node-dev-build-core-utils that referenced this issue Nov 10, 2022
Developerarif2 pushed a commit to Developerarif2/node-core-utils that referenced this issue Jan 27, 2023
gerkai added a commit to gerkai/node-core-utils-project-build that referenced this issue Jan 27, 2023
shovon58 pushed a commit to shovon58/node-core-utils that referenced this issue Jun 9, 2023
patrickm68 added a commit to patrickm68/NodeJS-core-utils that referenced this issue Sep 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Priority: High Issues/Bugs the needs to be fixed quickly.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants