| Message ID | 20240906041326.1684570-1-brianmlyles@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] interpret-trailers: handle message without trailing newline | expand |
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 >
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.
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. */
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(-) -- 2.45.2