Message ID | 20240906145743.2059405-1-brianmlyles@gmail.com (mailing list archive) |
---|---|
State | New |
Headers | show |
Series | [v3] interpret-trailers: handle message without trailing newline | expand |
Brian Lyles <brianmlyles@gmail.com> writes: > When git-interpret-trailers is used to add a trailer to a message that > does not end in a trailing newline, the new trailer is added on the line > immediately following the message instead of as a trailer block > separated from the message by a blank line. > > For example, if a message's text was exactly "The subject" with no > trailing newline present, `git interpret-trailers --trailer > my-trailer=true` will result in the following malformed commit message: > > The subject > my-trailer: true > > While it is generally expected that a commit message should end with a > newline character, git-interpret-trailers should not be returning an > invalid message in this case. I am not sure if the above example resulted in "an invalid message", though ;-) As far as Git is concerned, a commit log can contain any sequence of bytes. But of course, various tools to manipulate the messages (e.g. "commit --amend" and your editor that gets invoked by it, "interpret-trailers") may not be prepared to see any arbitrary bytes. I would have written While a commit message can contain arbitrary byte sequence, the fact that the user invoked the interpret-trailers command on it means that the contents is expected to be a proper text, which should not end in an incomplete line. Instead of detecting and erroring out upon seeing such a log message, complete the last line if it lacks the terminating LF. or something like that, if I were working on this change. > Use `strbuf_complete_line` to ensure that the message ends with a > newline character when reading the input. > > Signed-off-by: Brian Lyles <brianmlyles@gmail.com> > --- > > The range-diff from v2 is not included since the patch is so different > that range-diff is not able to provide anything meaningful. Very sensible. Will queue. Thanks. > diff --git a/t/t7513-interpret-trailers.sh b/t/t7513-interpret-trailers.sh > index 3d3e13ccf8..d78cae3e04 100755 > --- a/t/t7513-interpret-trailers.sh > +++ b/t/t7513-interpret-trailers.sh > @@ -175,6 +175,46 @@ test_expect_success 'with only a title in the message' ' > test_cmp expected actual > ' > > +test_expect_success 'with a bodiless message that lacks a trailing newline after the subject' ' > + cat >expected <<-\EOF && > + area: change > + > + Reviewed-by: Peff > + Acked-by: Johan > + EOF > + printf "area: change" | > + git interpret-trailers --trailer "Reviewed-by: Peff" \ > + --trailer "Acked-by: Johan" >actual && > + test_cmp expected actual > +' > + > +test_expect_success 'with a bodied message that lacks a trailing newline after the body' ' > + cat >expected <<-\EOF && > + area: change > + > + details about the change. > + > + Reviewed-by: Peff > + Acked-by: Johan > + EOF > + printf "area: change\n\ndetails about the change." | > + git interpret-trailers --trailer "Reviewed-by: Peff" \ > + --trailer "Acked-by: Johan" >actual && > + test_cmp expected actual > +' > + > +test_expect_success 'with a message that lacks a trailing newline after the trailers' ' > + cat >expected <<-\EOF && > + area: change > + > + Reviewed-by: Peff > + Acked-by: Johan > + EOF > + printf "area: change\n\nReviewed-by: Peff" | > + git interpret-trailers --trailer "Acked-by: Johan" >actual && > + test_cmp expected actual > +' > + > test_expect_success 'with multiline title in the message' ' > cat >expected <<-\EOF && > place of > -- > 2.45.2
Hi Brian This version looks good to me Thanks Phillip On 06/09/2024 15:50, Brian Lyles wrote: > When git-interpret-trailers is used to add a trailer to a message that > does not end in a trailing newline, the new trailer is added on the line > immediately following the message instead of as a trailer block > separated from the message by a blank line. > > For example, if a message's text was exactly "The subject" with no > trailing newline present, `git interpret-trailers --trailer > my-trailer=true` will result in the following malformed commit message: > > The subject > my-trailer: true > > While it is generally expected that a commit message should end with a > newline character, git-interpret-trailers should not be returning an > invalid message in this case. > > Use `strbuf_complete_line` to ensure that the message ends with a > newline character when reading the input. > > Signed-off-by: Brian Lyles <brianmlyles@gmail.com> > --- > > Differences from v2: > - We now use `strbuf_complete_line` when reading the input file instead > of handling the lack of a newline when constructing the output, which > drastically simplifies the patch. Thanks to Phillip for this > suggestion. > - Removed some unnecessary `\` in the new tests. > > The range-diff from v2 is not included since the patch is so different > that range-diff is not able to provide anything meaningful. > > > builtin/interpret-trailers.c | 1 + > t/t7513-interpret-trailers.sh | 40 +++++++++++++++++++++++++++++++++++ > 2 files changed, 41 insertions(+) > > diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c > index 1d969494cf..e6f22459f1 100644 > --- a/builtin/interpret-trailers.c > +++ b/builtin/interpret-trailers.c > @@ -132,6 +132,7 @@ static void read_input_file(struct strbuf *sb, const char *file) > if (strbuf_read(sb, fileno(stdin), 0) < 0) > die_errno(_("could not read from stdin")); > } > + strbuf_complete_line(sb); > } > > static void interpret_trailers(const struct process_trailer_options *opts, > diff --git a/t/t7513-interpret-trailers.sh b/t/t7513-interpret-trailers.sh > index 3d3e13ccf8..d78cae3e04 100755 > --- a/t/t7513-interpret-trailers.sh > +++ b/t/t7513-interpret-trailers.sh > @@ -175,6 +175,46 @@ test_expect_success 'with only a title in the message' ' > test_cmp expected actual > ' > > +test_expect_success 'with a bodiless message that lacks a trailing newline after the subject' ' > + cat >expected <<-\EOF && > + area: change > + > + Reviewed-by: Peff > + Acked-by: Johan > + EOF > + printf "area: change" | > + git interpret-trailers --trailer "Reviewed-by: Peff" \ > + --trailer "Acked-by: Johan" >actual && > + test_cmp expected actual > +' > + > +test_expect_success 'with a bodied message that lacks a trailing newline after the body' ' > + cat >expected <<-\EOF && > + area: change > + > + details about the change. > + > + Reviewed-by: Peff > + Acked-by: Johan > + EOF > + printf "area: change\n\ndetails about the change." | > + git interpret-trailers --trailer "Reviewed-by: Peff" \ > + --trailer "Acked-by: Johan" >actual && > + test_cmp expected actual > +' > + > +test_expect_success 'with a message that lacks a trailing newline after the trailers' ' > + cat >expected <<-\EOF && > + area: change > + > + Reviewed-by: Peff > + Acked-by: Johan > + EOF > + printf "area: change\n\nReviewed-by: Peff" | > + git interpret-trailers --trailer "Acked-by: Johan" >actual && > + test_cmp expected actual > +' > + > test_expect_success 'with multiline title in the message' ' > cat >expected <<-\EOF && > place of > -- > 2.45.2 >
On 06/09/2024 17:21, Junio C Hamano wrote: > Brian Lyles <brianmlyles@gmail.com> writes: > >> When git-interpret-trailers is used to add a trailer to a message that >> does not end in a trailing newline, the new trailer is added on the line >> immediately following the message instead of as a trailer block >> separated from the message by a blank line. >> >> For example, if a message's text was exactly "The subject" with no >> trailing newline present, `git interpret-trailers --trailer >> my-trailer=true` will result in the following malformed commit message: >> >> The subject >> my-trailer: true >> >> While it is generally expected that a commit message should end with a >> newline character, git-interpret-trailers should not be returning an >> invalid message in this case. > > I am not sure if the above example resulted in "an invalid message", > though ;-) As far as Git is concerned, a commit log can contain any > sequence of bytes. I assume it means invalid in the sense that the trailers are not separated from the rest of the message by a blank line, not in the sense that the resulting commit object is invalid. Best Wishes Phillip > But of course, various tools to manipulate the messages (e.g. > "commit --amend" and your editor that gets invoked by it, > "interpret-trailers") may not be prepared to see any arbitrary > bytes. I would have written > > While a commit message can contain arbitrary byte sequence, the > fact that the user invoked the interpret-trailers command on it > means that the contents is expected to be a proper text, which > should not end in an incomplete line. Instead of detecting and > erroring out upon seeing such a log message, complete the last > line if it lacks the terminating LF. > > or something like that, if I were working on this change. > >> Use `strbuf_complete_line` to ensure that the message ends with a >> newline character when reading the input. >> >> Signed-off-by: Brian Lyles <brianmlyles@gmail.com> >> --- >> >> The range-diff from v2 is not included since the patch is so different >> that range-diff is not able to provide anything meaningful. > > Very sensible. > > Will queue. Thanks. > >> diff --git a/t/t7513-interpret-trailers.sh b/t/t7513-interpret-trailers.sh >> index 3d3e13ccf8..d78cae3e04 100755 >> --- a/t/t7513-interpret-trailers.sh >> +++ b/t/t7513-interpret-trailers.sh >> @@ -175,6 +175,46 @@ test_expect_success 'with only a title in the message' ' >> test_cmp expected actual >> ' >> >> +test_expect_success 'with a bodiless message that lacks a trailing newline after the subject' ' >> + cat >expected <<-\EOF && >> + area: change >> + >> + Reviewed-by: Peff >> + Acked-by: Johan >> + EOF >> + printf "area: change" | >> + git interpret-trailers --trailer "Reviewed-by: Peff" \ >> + --trailer "Acked-by: Johan" >actual && >> + test_cmp expected actual >> +' >> + >> +test_expect_success 'with a bodied message that lacks a trailing newline after the body' ' >> + cat >expected <<-\EOF && >> + area: change >> + >> + details about the change. >> + >> + Reviewed-by: Peff >> + Acked-by: Johan >> + EOF >> + printf "area: change\n\ndetails about the change." | >> + git interpret-trailers --trailer "Reviewed-by: Peff" \ >> + --trailer "Acked-by: Johan" >actual && >> + test_cmp expected actual >> +' >> + >> +test_expect_success 'with a message that lacks a trailing newline after the trailers' ' >> + cat >expected <<-\EOF && >> + area: change >> + >> + Reviewed-by: Peff >> + Acked-by: Johan >> + EOF >> + printf "area: change\n\nReviewed-by: Peff" | >> + git interpret-trailers --trailer "Acked-by: Johan" >actual && >> + test_cmp expected actual >> +' >> + >> test_expect_success 'with multiline title in the message' ' >> cat >expected <<-\EOF && >> place of >> -- >> 2.45.2 >
Phillip Wood <phillip.wood123@gmail.com> writes: > I assume it means invalid in the sense that the trailers are not > separated from the rest of the message by a blank line, not in the > sense that the resulting commit object is invalid. OK, then "invalid message" -> "message with invalid trailer lines", perhaps. >> But of course, various tools to manipulate the messages (e.g. >> "commit --amend" and your editor that gets invoked by it, >> "interpret-trailers") may not be prepared to see any arbitrary >> bytes. I would have written >> While a commit message can contain arbitrary byte sequence, the >> fact that the user invoked the interpret-trailers command on it >> means that the contents is expected to be a proper text, which >> should not end in an incomplete line. Instead of detecting and >> erroring out upon seeing such a log message, complete the last >> line if it lacks the terminating LF. >> or something like that, if I were working on this change. Thanks.
diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c index 1d969494cf..e6f22459f1 100644 --- a/builtin/interpret-trailers.c +++ b/builtin/interpret-trailers.c @@ -132,6 +132,7 @@ static void read_input_file(struct strbuf *sb, const char *file) if (strbuf_read(sb, fileno(stdin), 0) < 0) die_errno(_("could not read from stdin")); } + strbuf_complete_line(sb); } static void interpret_trailers(const struct process_trailer_options *opts, diff --git a/t/t7513-interpret-trailers.sh b/t/t7513-interpret-trailers.sh index 3d3e13ccf8..d78cae3e04 100755 --- a/t/t7513-interpret-trailers.sh +++ b/t/t7513-interpret-trailers.sh @@ -175,6 +175,46 @@ test_expect_success 'with only a title in the message' ' test_cmp expected actual ' +test_expect_success 'with a bodiless message that lacks a trailing newline after the subject' ' + cat >expected <<-\EOF && + area: change + + Reviewed-by: Peff + Acked-by: Johan + EOF + printf "area: change" | + git interpret-trailers --trailer "Reviewed-by: Peff" \ + --trailer "Acked-by: Johan" >actual && + test_cmp expected actual +' + +test_expect_success 'with a bodied message that lacks a trailing newline after the body' ' + cat >expected <<-\EOF && + area: change + + details about the change. + + Reviewed-by: Peff + Acked-by: Johan + EOF + printf "area: change\n\ndetails about the change." | + git interpret-trailers --trailer "Reviewed-by: Peff" \ + --trailer "Acked-by: Johan" >actual && + test_cmp expected actual +' + +test_expect_success 'with a message that lacks a trailing newline after the trailers' ' + cat >expected <<-\EOF && + area: change + + Reviewed-by: Peff + Acked-by: Johan + EOF + printf "area: change\n\nReviewed-by: Peff" | + git interpret-trailers --trailer "Acked-by: Johan" >actual && + test_cmp expected actual +' + test_expect_success 'with multiline title in the message' ' cat >expected <<-\EOF && place of
When git-interpret-trailers is used to add a trailer to a message that does not end in a trailing newline, the new trailer is added on the line immediately following the message instead of as a trailer block separated from the message by a blank line. For example, if a message's text was exactly "The subject" with no trailing newline present, `git interpret-trailers --trailer my-trailer=true` will result in the following malformed commit message: The subject my-trailer: true While it is generally expected that a commit message should end with a newline character, git-interpret-trailers should not be returning an invalid message in this case. Use `strbuf_complete_line` to ensure that the message ends with a newline character when reading the input. Signed-off-by: Brian Lyles <brianmlyles@gmail.com> --- Differences from v2: - We now use `strbuf_complete_line` when reading the input file instead of handling the lack of a newline when constructing the output, which drastically simplifies the patch. Thanks to Phillip for this suggestion. - Removed some unnecessary `\` in the new tests. The range-diff from v2 is not included since the patch is so different that range-diff is not able to provide anything meaningful. builtin/interpret-trailers.c | 1 + t/t7513-interpret-trailers.sh | 40 +++++++++++++++++++++++++++++++++++ 2 files changed, 41 insertions(+) -- 2.45.2