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

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 6, 2024, 16:21 UTC
Message-ID: <xmqqy144wy60.fsf@gitster.g>
In-Reply-To: <20240906145743.2059405-1-brianmlyles@gmail.com>

Brian Lyles <brianmlyles@gmail.com> writes:

Show 15 quoted lines

> 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.

Show 8 quoted lines

> 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.

Show 53 quoted lines

> 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