fix(git-node): handle multi-line trailers#1062
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1062 +/- ##
==========================================
+ Coverage 71.74% 71.78% +0.04%
==========================================
Files 41 41
Lines 5878 5887 +9
==========================================
+ Hits 4217 4226 +9
Misses 1661 1661 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Renegade334
left a comment
Renegade334
left a comment
There was a problem hiding this comment.
Just as a courtesy, I changed the "Fixes" in the PR description as GH was threatening to cross-close Matteo's announcement issue on the core repo.
| const amended = original.trim().split('\n'); | ||
| const stillInTrailers = () => { | ||
| const result = interpretTrailers(amended.join('\n')); | ||
| return result.length && originalTrailers.startsWith(result.trim()); | ||
| }; | ||
| for (let i = amended.length - 1; amended[i] === '' || stillInTrailers(); i--) { | ||
| // Remove last line until git no longer detects any trailers | ||
| amended.pop(); | ||
| } |
There was a problem hiding this comment.
Presumably we could skip this step if originalTrailers is empty?
| const amended = original.trim().split('\n'); | ||
| const stillInTrailers = () => { | ||
| const result = interpretTrailers(amended.join('\n')); | ||
| return result.length && originalTrailers.startsWith(result.trim()); | ||
| }; | ||
| for (let i = amended.length - 1; amended[i] === '' || stillInTrailers(); i--) { | ||
| // Remove last line until git no longer detects any trailers | ||
| amended.pop(); | ||
| } |
There was a problem hiding this comment.
I wonder if there's a better way than spawning a git process per line until the trailers are completely identified. Doesn't git interpret-trailers require a double-newline before a trailer block – could we look backwards for this instead?
There was a problem hiding this comment.
That's fair, I've added a shortcut so it skips all the lines until it finds an empty string, so most (all?) commit messages only need 2 spawn of git interpret-trailers
2 participants
The current code assumes trailers will always be single-line, having a multi-line trailer currently results in non-sense output. This PR switch to relying on calling
git interpret-trailersmultiple times to filter out the trailers consistently.Refs: nodejs/node#62577 (comment)