[v3] interpret-trailers: handle message without trailing newline

Commit Message

Brian Lyles Sept. 6, 2024, 2:50 p.m. UTC

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 <[e-mail]>
---

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

Comments

Junio C Hamano Sept. 6, 2024, 4:21 p.m. UTC | #1

Brian Lyles <[e-mail]> 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 <[e-mail]>
> ---
>
> 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 Sept. 9, 2024, 9:13 a.m. UTC | #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 <[e-mail]>
> ---
> 
> 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
>

Phillip Wood Sept. 9, 2024, 9:13 a.m. UTC | #3

On 06/09/2024 17:21, Junio C Hamano wrote:
> Brian Lyles <[e-mail]> 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 <[e-mail]>
>> ---
>>
>> 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
>

Junio C Hamano Sept. 9, 2024, 3:58 p.m. UTC | #4

Phillip Wood <[e-mail]> 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.

Patch

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