diff mbox series

interpret-trailers: handle message without trailing newline

Message ID 20240905173445.1677704-1-brianmlyles@gmail.com (mailing list archive)
State Superseded
Headers show
Series interpret-trailers: handle message without trailing newline | expand

Commit Message

Brian Lyles Sept. 5, 2024, 5:34 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 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(-)

--
2.45.2

Comments

Brian Lyles Sept. 5, 2024, 6:24 p.m. UTC | #1
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.
diff mbox series

Patch

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