# [PATCH v2 0/5] Small "git interpret-trailers" fixes 9 messages from 2014-11-09 to 2014-11-10. Participants: Christian Couder, Junio C Hamano. Thread: https://gitlist.dev/t/37912 ## Christian Couder, 2014-11-09 09:23 Subject: [PATCH v2 0/5] Small "git interpret-trailers" fixes Message-ID: <20141109092313.4864.54933.chriscool@tuxfamily.org> URL: https://gitlist.dev/e/20141109092313.4864.54933.chriscool%40tuxfamily.org ``` Here is a series to fix a few small bugs in "git interpret-trailers". Patch 1/5 and patch 2/5 are independent from the last 3 patches, and can be applied to master. Patches 3/5, 4/5 and 5/5 depend on this series by Junio: jc/conflict-hint (2014-10-28) 4 commits (merged to 'next' on 2014-10-29 at 693250f) Changes since the previous version are: * use "%.*s" instead of a strbuf, in patch 2/5 * add new find_trailer_end() function and a call to strbuf_release(), in patch 4/5 Thanks to Junio and Peff. Christian Couder (5): trailer: ignore comment lines inside the trailers trailer: display a trailer without its trailing newline commit: make ignore_non_trailer() non static trailer: reuse ignore_non_trailer() to ignore conflict lines trailer: add test with an old style conflict block builtin/commit.c | 46 ------------------------------------------- commit.c | 46 +++++++++++++++++++++++++++++++++++++++++++ commit.h | 3 +++ t/t7513-interpret-trailers.sh | 34 +++++++++++++++++++++++++++++++- trailer.c | 46 ++++++++++++++++++++++++++++++++----------- 5 files changed, 117 insertions(+), 58 deletions(-) -- 2.1.2.555.gfbecd99 ``` ## Christian Couder, 2014-11-09 09:23 Subject: [PATCH v2 1/5] trailer: ignore comment lines inside the trailers Message-ID: <20141109092344.4864.68904.chriscool@tuxfamily.org> URL: https://gitlist.dev/e/20141109092344.4864.68904.chriscool%40tuxfamily.org In-Reply-To: <20141109092313.4864.54933.chriscool@tuxfamily.org> ``` Otherwise trailers that are commented out might be processed. We would also error out if the comment line char is also a separator. This means that comments inside a trailer block will disappear, but that was already the case anyway. Signed-off-by: Christian Couder --- trailer.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/trailer.c b/trailer.c index 8514566..761b763 100644 --- a/trailer.c +++ b/trailer.c @@ -804,8 +804,10 @@ static int process_input_file(struct strbuf **lines, /* Parse trailer lines */ for (i = trailer_start; i < patch_start; i++) { - struct trailer_item *new = create_trailer_item(lines[i]->buf); - add_trailer_item(in_tok_first, in_tok_last, new); + if (lines[i]->buf[0] != comment_line_char) { + struct trailer_item *new = create_trailer_item(lines[i]->buf); + add_trailer_item(in_tok_first, in_tok_last, new); + } } return patch_start; -- 2.1.2.555.gfbecd99 ``` ## Christian Couder, 2014-11-09 09:23 Subject: [PATCH v2 2/5] trailer: display a trailer without its trailing newline Message-ID: <20141109092344.4864.55173.chriscool@tuxfamily.org> URL: https://gitlist.dev/e/20141109092344.4864.55173.chriscool%40tuxfamily.org In-Reply-To: <20141109092313.4864.54933.chriscool@tuxfamily.org> ``` Trailers passed to the parse_trailer() function often have a trailing newline. When erroring out, we should display the invalid trailer properly, that means without any trailing newline. Helped-by: Junio C Hamano Helped-by: Jeff King Signed-off-by: Christian Couder --- trailer.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/trailer.c b/trailer.c index 761b763..219a5a2 100644 --- a/trailer.c +++ b/trailer.c @@ -583,8 +583,12 @@ static int parse_trailer(struct strbuf *tok, struct strbuf *val, const char *tra strbuf_addch(&seps, '='); len = strcspn(trailer, seps.buf); strbuf_release(&seps); - if (len == 0) - return error(_("empty trailer token in trailer '%s'"), trailer); + if (len == 0) { + int l = strlen(trailer); + while (l > 0 && isspace(trailer[l - 1])) + l--; + return error(_("empty trailer token in trailer '%.*s'"), l, trailer); + } if (len < strlen(trailer)) { strbuf_add(tok, trailer, len); strbuf_trim(tok); -- 2.1.2.555.gfbecd99 ``` ## Christian Couder, 2014-11-09 09:23 Subject: [PATCH v2 3/5] commit: make ignore_non_trailer() non static Message-ID: <20141109092344.4864.789.chriscool@tuxfamily.org> URL: https://gitlist.dev/e/20141109092344.4864.789.chriscool%40tuxfamily.org In-Reply-To: <20141109092313.4864.54933.chriscool@tuxfamily.org> ``` Signed-off-by: Christian Couder --- builtin/commit.c | 46 ---------------------------------------------- commit.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++ commit.h | 3 +++ 3 files changed, 49 insertions(+), 46 deletions(-) diff --git a/builtin/commit.c b/builtin/commit.c index e3c60dd..cda74e9 100644 --- a/builtin/commit.c +++ b/builtin/commit.c @@ -677,52 +677,6 @@ static void adjust_comment_line_char(const struct strbuf *sb) comment_line_char = *p; } -/* - * Inspect sb and determine the true "end" of the log message, in - * order to find where to put a new Signed-off-by: line. Ignored are - * trailing comment lines and blank lines, and also the traditional - * "Conflicts:" block that is not commented out, so that we can use - * "git commit -s --amend" on an existing commit that forgot to remove - * it. - * - * Returns the number of bytes from the tail to ignore, to be fed as - * the second parameter to append_signoff(). - */ -static int ignore_non_trailer(struct strbuf *sb) -{ - int boc = 0; - int bol = 0; - int in_old_conflicts_block = 0; - - while (bol < sb->len) { - char *next_line; - - if (!(next_line = memchr(sb->buf + bol, '\n', sb->len - bol))) - next_line = sb->buf + sb->len; - else - next_line++; - - if (sb->buf[bol] == comment_line_char || sb->buf[bol] == '\n') { - /* is this the first of the run of comments? */ - if (!boc) - boc = bol; - /* otherwise, it is just continuing */ - } else if (starts_with(sb->buf + bol, "Conflicts:\n")) { - in_old_conflicts_block = 1; - if (!boc) - boc = bol; - } else if (in_old_conflicts_block && sb->buf[bol] == '\t') { - ; /* a pathname in the conflicts block */ - } else if (boc) { - /* the previous was not trailing comment */ - boc = 0; - in_old_conflicts_block = 0; - } - bol = next_line - sb->buf; - } - return boc ? sb->len - boc : 0; -} - static int prepare_to_commit(const char *index_file, const char *prefix, struct commit *current_head, struct wt_status *s, diff --git a/commit.c b/commit.c index 19cf8f9..a54cb9a 100644 --- a/commit.c +++ b/commit.c @@ -1640,3 +1640,49 @@ const char *find_commit_header(const char *msg, const char *key, size_t *out_len } return NULL; } + +/* + * Inspect sb and determine the true "end" of the log message, in + * order to find where to put a new Signed-off-by: line. Ignored are + * trailing comment lines and blank lines, and also the traditional + * "Conflicts:" block that is not commented out, so that we can use + * "git commit -s --amend" on an existing commit that forgot to remove + * it. + * + * Returns the number of bytes from the tail to ignore, to be fed as + * the second parameter to append_signoff(). + */ +int ignore_non_trailer(struct strbuf *sb) +{ + int boc = 0; + int bol = 0; + int in_old_conflicts_block = 0; + + while (bol < sb->len) { + char *next_line; + + if (!(next_line = memchr(sb->buf + bol, '\n', sb->len - bol))) + next_line = sb->buf + sb->len; + else + next_line++; + + if (sb->buf[bol] == comment_line_char || sb->buf[bol] == '\n') { + /* is this the first of the run of comments? */ + if (!boc) + boc = bol; + /* otherwise, it is just continuing */ + } else if (starts_with(sb->buf + bol, "Conflicts:\n")) { + in_old_conflicts_block = 1; + if (!boc) + boc = bol; + } else if (in_old_conflicts_block && sb->buf[bol] == '\t') { + ; /* a pathname in the conflicts block */ + } else if (boc) { + /* the previous was not trailing comment */ + boc = 0; + in_old_conflicts_block = 0; + } + bol = next_line - sb->buf; + } + return boc ? sb->len - boc : 0; +} diff --git a/commit.h b/commit.h index bc68ccb..cd35ac1 100644 --- a/commit.h +++ b/commit.h @@ -337,6 +337,9 @@ extern void free_commit_extra_headers(struct commit_extra_header *extra); extern const char *find_commit_header(const char *msg, const char *key, size_t *out_len); +/* Find the end of the log message, the right place for a new trailer. */ +extern int ignore_non_trailer(struct strbuf *sb); + typedef void (*each_mergetag_fn)(struct commit *commit, struct commit_extra_header *extra, void *cb_data); -- 2.1.2.555.gfbecd99 ``` ## Christian Couder, 2014-11-09 09:23 Subject: [PATCH v2 4/5] trailer: reuse ignore_non_trailer() to ignore conflict lines Message-ID: <20141109092344.4864.88180.chriscool@tuxfamily.org> URL: https://gitlist.dev/e/20141109092344.4864.88180.chriscool%40tuxfamily.org In-Reply-To: <20141109092313.4864.54933.chriscool@tuxfamily.org> ``` Make sure we look for trailers before any conflict line by reusing the ignore_non_trailer() function. Helped-by: Junio C Hamano Signed-off-by: Christian Couder --- t/t7513-interpret-trailers.sh | 2 ++ trailer.c | 32 +++++++++++++++++++++++++------- 2 files changed, 27 insertions(+), 7 deletions(-) diff --git a/t/t7513-interpret-trailers.sh b/t/t7513-interpret-trailers.sh index 1efb880..fed053a 100755 --- a/t/t7513-interpret-trailers.sh +++ b/t/t7513-interpret-trailers.sh @@ -232,6 +232,8 @@ test_expect_success 'with message that has comments' ' Reviewed-by: Johan Cc: Peff + # last comment + EOF cat basic_patch >>expected && git interpret-trailers --trim-empty --trailer "Cc: Peff" message_with_comments >actual && diff --git a/trailer.c b/trailer.c index 219a5a2..30636d2 100644 --- a/trailer.c +++ b/trailer.c @@ -2,6 +2,7 @@ #include "string-list.h" #include "run-command.h" #include "string-list.h" +#include "commit.h" #include "trailer.h" /* * Copyright (c) 2013, 2014 Christian Couder @@ -769,6 +770,22 @@ static int find_trailer_start(struct strbuf **lines, int count) return only_spaces ? count : 0; } +/* Get the index of the end of the trailers */ +static int find_trailer_end(struct strbuf **lines, int patch_start) +{ + struct strbuf sb = STRBUF_INIT; + int i, ignore_bytes; + + for (i = 0; i < patch_start; i++) + strbuf_addbuf(&sb, lines[i]); + ignore_bytes = ignore_non_trailer(&sb); + strbuf_release(&sb); + for (i = patch_start - 1; i >= 0 && ignore_bytes > 0; i--) + ignore_bytes -= lines[i]->len; + + return i + 1; +} + static int has_blank_line_before(struct strbuf **lines, int start) { for (;start >= 0; start--) { @@ -791,14 +808,15 @@ static int process_input_file(struct strbuf **lines, struct trailer_item **in_tok_last) { int count = 0; - int patch_start, trailer_start, i; + int patch_start, trailer_start, trailer_end, i; /* Get the line count */ while (lines[count]) count++; patch_start = find_patch_start(lines, count); - trailer_start = find_trailer_start(lines, patch_start); + trailer_end = find_trailer_end(lines, patch_start); + trailer_start = find_trailer_start(lines, trailer_end); /* Print lines before the trailers as is */ print_lines(lines, 0, trailer_start); @@ -807,14 +825,14 @@ static int process_input_file(struct strbuf **lines, printf("\n"); /* Parse trailer lines */ - for (i = trailer_start; i < patch_start; i++) { + for (i = trailer_start; i < trailer_end; i++) { if (lines[i]->buf[0] != comment_line_char) { struct trailer_item *new = create_trailer_item(lines[i]->buf); add_trailer_item(in_tok_first, in_tok_last, new); } } - return patch_start; + return trailer_end; } static void free_all(struct trailer_item **first) @@ -831,7 +849,7 @@ void process_trailers(const char *file, int trim_empty, struct string_list *trai struct trailer_item *in_tok_last = NULL; struct trailer_item *arg_tok_first; struct strbuf **lines; - int patch_start; + int trailer_end; /* Default config must be setup first */ git_config(git_trailer_default_config, NULL); @@ -840,7 +858,7 @@ void process_trailers(const char *file, int trim_empty, struct string_list *trai lines = read_input_file(file); /* Print the lines before the trailers */ - patch_start = process_input_file(lines, &in_tok_first, &in_tok_last); + trailer_end = process_input_file(lines, &in_tok_first, &in_tok_last); arg_tok_first = process_command_line_args(trailers); @@ -851,7 +869,7 @@ void process_trailers(const char *file, int trim_empty, struct string_list *trai free_all(&in_tok_first); /* Print the lines after the trailers as is */ - print_lines(lines, patch_start, INT_MAX); + print_lines(lines, trailer_end, INT_MAX); strbuf_list_free(lines); } -- 2.1.2.555.gfbecd99 ``` ## Christian Couder, 2014-11-09 09:23 Subject: [PATCH v2 5/5] trailer: add test with an old style conflict block Message-ID: <20141109092344.4864.23949.chriscool@tuxfamily.org> URL: https://gitlist.dev/e/20141109092344.4864.23949.chriscool%40tuxfamily.org In-Reply-To: <20141109092313.4864.54933.chriscool@tuxfamily.org> ``` Signed-off-by: Christian Couder --- t/t7513-interpret-trailers.sh | 32 +++++++++++++++++++++++++++++++- 1 file changed, 31 insertions(+), 1 deletion(-) diff --git a/t/t7513-interpret-trailers.sh b/t/t7513-interpret-trailers.sh index fed053a..bd0ab46 100755 --- a/t/t7513-interpret-trailers.sh +++ b/t/t7513-interpret-trailers.sh @@ -213,7 +213,7 @@ test_expect_success 'with 2 files arguments' ' ' test_expect_success 'with message that has comments' ' - cat basic_message >>message_with_comments && + cat basic_message >message_with_comments && sed -e "s/ Z\$/ /" >>message_with_comments <<-\EOF && # comment @@ -240,6 +240,36 @@ test_expect_success 'with message that has comments' ' test_cmp expected actual ' +test_expect_success 'with message that has an old style conflict block' ' + cat basic_message >message_with_comments && + sed -e "s/ Z\$/ /" >>message_with_comments <<-\EOF && + # comment + + # other comment + Cc: Z + # yet another comment + Reviewed-by: Johan + Reviewed-by: Z + # last comment + + Conflicts: + + EOF + cat basic_message >expected && + cat >>expected <<-\EOF && + # comment + + Reviewed-by: Johan + Cc: Peff + # last comment + + Conflicts: + + EOF + git interpret-trailers --trim-empty --trailer "Cc: Peff" message_with_comments >actual && + test_cmp expected actual +' + test_expect_success 'with commit complex message and trailer args' ' cat complex_message_body >expected && sed -e "s/ Z\$/ /" >>expected <<-\EOF && -- 2.1.2.555.gfbecd99 ``` ## Junio C Hamano, 2014-11-10 17:49 Subject: Re: [PATCH v2 4/5] trailer: reuse ignore_non_trailer() to ignore conflict lines Message-ID: URL: https://gitlist.dev/e/xmqqbnof9c9d.fsf%40gitster.dls.corp.google.com In-Reply-To: <20141109092344.4864.88180.chriscool@tuxfamily.org> ``` Christian Couder writes: > Make sure we look for trailers before any conflict line > by reusing the ignore_non_trailer() function. > > Helped-by: Junio C Hamano > Signed-off-by: Christian Couder > --- It makes sense to unify the logic to decide where the trailer block is, I would think. I however don't think I helped this change in any way, not more than "maintained the codebase as a solid foundation to build new features on", but at that point it would apply to any other change and not worth mentioning ;-). ``` ## Christian Couder, 2014-11-10 19:05 Subject: Re: [PATCH v2 4/5] trailer: reuse ignore_non_trailer() to ignore conflict lines Message-ID: <20141110.200502.1295000903783079007.chriscool@tuxfamily.org> URL: https://gitlist.dev/e/20141110.200502.1295000903783079007.chriscool%40tuxfamily.org In-Reply-To: ``` From: Junio C Hamano Subject: Re: [PATCH v2 4/5] trailer: reuse ignore_non_trailer() to ignore conflict lines Date: Mon, 10 Nov 2014 09:49:34 -0800 > Christian Couder writes: > >> Make sure we look for trailers before any conflict line >> by reusing the ignore_non_trailer() function. >> >> Helped-by: Junio C Hamano >> Signed-off-by: Christian Couder >> --- > > It makes sense to unify the logic to decide where the trailer block > is, I would think. I however don't think I helped this change in > any way, not more than "maintained the codebase as a solid > foundation to build new features on", but at that point it would > apply to any other change and not worth mentioning ;-). Do you want me to resend the series with only the Helped-by removed? ``` ## Junio C Hamano, 2014-11-10 19:58 Subject: Re: [PATCH v2 4/5] trailer: reuse ignore_non_trailer() to ignore conflict lines Message-ID: URL: https://gitlist.dev/e/xmqqy4ri969y.fsf%40gitster.dls.corp.google.com In-Reply-To: <20141110.200502.1295000903783079007.chriscool@tuxfamily.org> ``` Christian Couder writes: > From: Junio C Hamano > Subject: Re: [PATCH v2 4/5] trailer: reuse ignore_non_trailer() to ignore conflict lines > Date: Mon, 10 Nov 2014 09:49:34 -0800 > >> Christian Couder writes: >> >>> Make sure we look for trailers before any conflict line >>> by reusing the ignore_non_trailer() function. >>> >>> Helped-by: Junio C Hamano >>> Signed-off-by: Christian Couder >>> --- >> >> It makes sense to unify the logic to decide where the trailer block >> is, I would think. I however don't think I helped this change in >> any way, not more than "maintained the codebase as a solid >> foundation to build new features on", but at that point it would >> apply to any other change and not worth mentioning ;-). > > Do you want me to resend the series with only the Helped-by removed? Nah, I've already removed it. You have better things to spend your time on ;-) ```