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

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

[Brian Lyles]

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

For example, if a message's text was "The subject" with no trailing
newline at all, `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.

Signed-off-by: Brian Lyles <brianmlyles@gmail.com>
---
 builtin/interpret-trailers.c  | 12 ++++++-----
 t/t7513-interpret-trailers.sh | 40 +++++++++++++++++++++++++++++++++++
 trailer.c                     | 18 ++++++++++++++++
 trailer.h                     |  5 +++++
 4 files changed, 70 insertions(+), 5 deletions(-)

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" | \
+	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..9c19632b6d 100644
--- a/trailer.c
+++ b/trailer.c
@@ -18,6 +18,12 @@ struct trailer_info {
 	 */
 	int blank_line_before_trailer;

+	/*
+	 * True if the last character before the location pointed to be
+	 * trailer_block_start is a 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..04148be432 100644
--- a/trailer.h
+++ b/trailer.h
@@ -125,6 +125,11 @@ size_t trailer_block_end(struct trailer_info *);
  */
 int blank_line_before_trailer_block(struct trailer_info *);

+/*
+ * Return 1 if the trailer block had a newline character
+ */
+int message_without_trailing_newline_before_trailer_block(struct trailer_info *);
+
 /*
  * Free trailer_info struct.
  */
--
2.45.2

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

[Brian Lyles]

On 05/09/2024 12:34, Brian Lyles wrote:
> +	/*
> +	 * True if the last character before the location pointed to be
> +	 * trailer_block_start is a newline character.
> +	 */
> +	int message_without_trailing_newline_before_trailer;

I just noticed that this comment is incorrect, reflecting an older
local version of this patch. In v2 this will be:

    True if there is a message before the trailer block and it does not
	end with a trailing newline character.


> +/*
> + * Return 1 if the trailer block had a newline character
> + */
> +int message_without_trailing_newline_before_trailer_block(struct trailer_info *);

Same thing here. In v2 this will be:

	Return true if there is a message before the trailer block and it
	does not end with a trailing newline character.

I will wait to send out a v2 until later tonight to avoid spamming
folks, and give others a chance to provide feedback.

-- 
Thank you,
Brian Lyles

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

[Brian Lyles]

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.

Signed-off-by: Brian Lyles <brianmlyles@gmail.com>
---

Differences from v1:
- Minor tweak to commit message wording
- Updated stale documentation from initial prototype to accurately
  reflect the actual state of the code.

Range-diff vs v1:

1:  7f67f06a08 ! 1:  af02465f86 interpret-trailers: handle message without trailing newline
    @@ Commit message
         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 newline character.
    +    separated from the message by a blank line.

    -    For example, if a message's text was "The subject" with no trailing
    -    newline at all, `git interpret-trailers --trailer my-trailer=true` will
    -    result in the following malformed commit message:
    +    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
    @@ trailer.c: struct trailer_info {
         int blank_line_before_trailer;

     +   /*
    -+    * True if the last character before the location pointed to be
    -+    * trailer_block_start is a newline character.
    ++    * 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;
     +
    @@ trailer.h: size_t trailer_block_end(struct trailer_info *);
      int blank_line_before_trailer_block(struct trailer_info *);

     +/*
    -+ * Return 1 if the trailer block had a newline character
    ++ * 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 *);
     +

 builtin/interpret-trailers.c  | 12 ++++++-----
 t/t7513-interpret-trailers.sh | 40 +++++++++++++++++++++++++++++++++++
 trailer.c                     | 18 ++++++++++++++++
 trailer.h                     |  6 ++++++
 4 files changed, 71 insertions(+), 5 deletions(-)

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" | \
+	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

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

[Phillip Wood]

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
> 

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

[Brian Lyles]

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

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

[Junio C Hamano]

Phillip Wood <phillip.wood123@gmail.com> writes:

> 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);
>  }

It is much simpler, and if we are to require a message that the user
uses interpret-trailers on not to end in an incomplete line (which I
do not have any objection to), it is absolutely the right approach.

With a devil's advocate hat on, though, if the trailer operation is
to find the trailer on the incomplete line at the end, and insert a
trailer _before_ that one, would it be more faithful to the command
given by the end-user, if we inserted the new trailer without
touching the existing trailer line (including its lack of
terminating EOL)?  Which would mean that we'd need to remember the
fact that we added a LF here, and then before writing the result out
make the buffer to end with an incomplete line.  Which I personally
think is crazy, compared to the approach to declare that a message
that you subject to interpret-trailers command MUST BE a proper
text, not ending with an incomplete line.

So, yeah, I like that idea.

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

[Junio C Hamano]

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

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

[Phillip Wood]

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
> 

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

[Phillip Wood]

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
> 

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

[Junio C Hamano]

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.