fix: respect existing trailers in commit messages #632
Conversation
This comment was marked as off-topic.
This comment was marked as off-topic.
| const metadata = this.metadata.trim().split('\n'); | ||
| const amended = original.split('\n'); | ||
| if (amended[amended.length - 1] !== '') { |
There was a problem hiding this comment.
Comment: This condition appears to have always been false because the code above calls trim() on the commit message before splitting it at \n.
| } else { | ||
| cli.error('Git found no trailers in the original commit message, ' + | ||
| `but '${line}' is present and should be a trailer.`); | ||
| process.exit(1); // make it work with git rebase -x |
There was a problem hiding this comment.
Comment: It seems reasonable to abort in this case, but if anyone thinks it could be problematic, I'd be happy to revert this particular check.
tniessen
force-pushed
the
respect-existing-trailers
branch
from
ff6ba13
to
ccb8d96
Compare
tniessen
changed the title
|
I'm going to try it with some backports and V8 update |
|
@targos Have you had a chance to test this yet? Looks like it'll be difficult to write tests for this part. Actually, I am not sure what happens with |
It's still on my todo list. |
There was a problem hiding this comment.
Just tried that with nodejs/node#44048 and I confirm this PR fixes the issue.
Use
git interpret-trailers(probably requires a fairly recent git version) to see if the original commit message already contains trailers and do not add an empty line before adding metadata if it does (as suggested in #602 (comment)). This should fix howgit node landtreatsCo-authored-bylines.No reordering happens. The generated metadata is appended to the existing trailer. My understanding is that the order of trailers is irrelevant, but if that's not true, we might want to revisit that part.
This particular part of the code base does not seem to be covered by any tests, so please review carefully.
Fixes: #602