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

[Phillip Wood <phillip.wood123@gmail.com>]

Hi Brian

On 06/09/2024 05:08, 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.
> 
> Detect when a message exists but does not end with a newline character,
> and add an extra newline before appending the new trailer.

Thanks for the comprehensive commit message. If the problem only affects
"git interpret-trailers" I wonder if it would be simpler to do

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,

So that we feed the trailer machinery a message with a trailing new line.
Thanks for adding some tests, I've left one small comment below.

> diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c
> index 1d969494cf..9d8f94341d 100644
> --- a/builtin/interpret-trailers.c
> +++ b/builtin/interpret-trailers.c
> @@ -153,13 +153,15 @@ static void interpret_trailers(const struct process_trailer_options *opts,
> 
>   	info = parse_trailers(opts, sb.buf, &head);
> 
> -	/* Print the lines before the trailers */
> -	if (!opts->only_trailers)
> +	if (!opts->only_trailers) {
> +		/* Print the lines before the trailers */
>   		fwrite(sb.buf, 1, trailer_block_start(info), outfile);
> 
> -	if (!opts->only_trailers && !blank_line_before_trailer_block(info))
> -		fprintf(outfile, "\n");
> -
> +		if (message_without_trailing_newline_before_trailer_block(info))
> +			fprintf(outfile, "\n\n");
> +		else if (!blank_line_before_trailer_block(info))
> +			fprintf(outfile, "\n");
> +	}
> 
>   	if (!opts->only_input) {
>   		LIST_HEAD(config_head);
> diff --git a/t/t7513-interpret-trailers.sh b/t/t7513-interpret-trailers.sh
> index 3d3e13ccf8..d5303c3f74 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" | \

There is no need to add a backslash after the pipe here or in the other tests.

Best Wishes

Phillip

> +	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
> diff --git a/trailer.c b/trailer.c
> index 72e5136c73..178adae6ad 100644
> --- a/trailer.c
> +++ b/trailer.c
> @@ -18,6 +18,12 @@ struct trailer_info {
>   	 */
>   	int blank_line_before_trailer;
> 
> +	/*
> +	 * True if there is a message before the trailer block and it does not
> +	 * end with a trailing newline character.
> +	 */
> +	int message_without_trailing_newline_before_trailer;
> +
>   	/*
>   	 * Offsets to the trailer block start and end positions in the input
>   	 * string. If no trailer block is found, these are both set to the
> @@ -946,6 +952,11 @@ static int ends_with_blank_line(const char *buf, size_t len)
>   	return is_blank_line(buf + ll);
>   }
> 
> +static int has_message_without_trailing_newline_char(const char *buf, size_t len)
> +{
> +	return len > 0 && buf[len - 1] != '\n';
> +}
> +
>   static void unfold_value(struct strbuf *val)
>   {
>   	struct strbuf out = STRBUF_INIT;
> @@ -1017,6 +1028,8 @@ static struct trailer_info *trailer_info_get(const struct process_trailer_option
> 
>   	info->blank_line_before_trailer = ends_with_blank_line(str,
>   							       trailer_block_start);
> +	info->message_without_trailing_newline_before_trailer
> +		= has_message_without_trailing_newline_char(str, trailer_block_start);
>   	info->trailer_block_start = trailer_block_start;
>   	info->trailer_block_end = end_of_log_message;
>   	info->trailers = trailer_strings;
> @@ -1090,6 +1103,11 @@ int blank_line_before_trailer_block(struct trailer_info *info)
>   	return info->blank_line_before_trailer;
>   }
> 
> +int message_without_trailing_newline_before_trailer_block(struct trailer_info *info)
> +{
> +	return info->message_without_trailing_newline_before_trailer;
> +}
> +
>   void trailer_info_release(struct trailer_info *info)
>   {
>   	size_t i;
> diff --git a/trailer.h b/trailer.h
> index 6eb53df155..81449151c7 100644
> --- a/trailer.h
> +++ b/trailer.h
> @@ -125,6 +125,12 @@ size_t trailer_block_end(struct trailer_info *);
>    */
>   int blank_line_before_trailer_block(struct trailer_info *);
> 
> +/*
> + * Return true if there is a message before the trailer block and it does not
> + * end with a trailing newline character.
> + */
> +int message_without_trailing_newline_before_trailer_block(struct trailer_info *);
> +
>   /*
>    * Free trailer_info struct.
>    */
> --
> 2.45.2
>