{"thread":{"id":"37898","subject":"[PATCH 1/5] trailer: ignore comment lines inside the trailers","startedAt":"2014-11-07T18:50:47Z","lastAt":"2014-11-09T22:40:09Z","messageCount":13,"participants":["Christian Couder","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":5},"messages":[{"id":"251467","messageId":"20141107184148.16854.63825.chriscool@tuxfamily.org","threadId":"37898","inReplyTo":null,"subject":"[PATCH 0/5] Small \"git interpret-trailers\" fixes","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2014-11-07T18:50:47Z","receivedAt":"2014-11-07T18:50:47Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Here is a series to fix a few small bugs in \"git interpret-trailers\".\n\nPatch 1/5 and patch 2/5 are independent from the last 3 patches,\nand can be applied to master.\n\nPatches 3/5, 4/5 and 5/5 depend on this series by Junio:\n\n\tjc/conflict-hint (2014-10-28) 4 commits\n\t  (merged to 'next' on 2014-10-29 at 693250f)\n\nChristian Couder (5):\n  trailer: ignore comment lines inside the trailers\n  trailer: display a trailer without its trailing newline\n  commit: make ignore_non_trailer() non static\n  trailer: reuse ignore_non_trailer() to ignore conflict lines\n  trailer: add test with an old style conflict block\n\n builtin/commit.c              | 46 -------------------------------------------\n commit.c                      | 46 +++++++++++++++++++++++++++++++++++++++++++\n commit.h                      |  3 +++\n t/t7513-interpret-trailers.sh | 34 +++++++++++++++++++++++++++++++-\n trailer.c                     | 39 +++++++++++++++++++++++++-----------\n 5 files changed, 110 insertions(+), 58 deletions(-)\n\n-- \n2.1.2.555.gfbecd99\n"},{"id":"251465","messageId":"20141107185053.16854.34105.chriscool@tuxfamily.org","threadId":"37898","inReplyTo":"20141107184148.16854.63825.chriscool@tuxfamily.org","subject":"[PATCH 1/5] trailer: ignore comment lines inside the trailers","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2014-11-07T18:50:48Z","receivedAt":"2014-11-07T18:50:48Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Otherwise trailers that are commented out might be\nprocessed. We would also error out if the comment line\nchar is also a separator.\n\nThis means that comments inside a trailer block will\ndisappear, but that was already the case anyway.\n\nSigned-off-by: Christian Couder <chriscool@tuxfamily.org>\n---\n trailer.c | 6 ++++--\n 1 file changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git a/trailer.c b/trailer.c\nindex 8514566..761b763 100644\n--- a/trailer.c\n+++ b/trailer.c\n@@ -804,8 +804,10 @@ static int process_input_file(struct strbuf **lines,\n \n \t/* Parse trailer lines */\n \tfor (i = trailer_start; i < patch_start; i++) {\n-\t\tstruct trailer_item *new = create_trailer_item(lines[i]->buf);\n-\t\tadd_trailer_item(in_tok_first, in_tok_last, new);\n+\t\tif (lines[i]->buf[0] != comment_line_char) {\n+\t\t\tstruct trailer_item *new = create_trailer_item(lines[i]->buf);\n+\t\t\tadd_trailer_item(in_tok_first, in_tok_last, new);\n+\t\t}\n \t}\n \n \treturn patch_start;\n-- \n2.1.2.555.gfbecd99\n"},{"id":"251470","messageId":"20141107185053.16854.84253.chriscool@tuxfamily.org","threadId":"37898","inReplyTo":"20141107184148.16854.63825.chriscool@tuxfamily.org","subject":"[PATCH 2/5] trailer: display a trailer without its trailing newline","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2014-11-07T18:50:49Z","receivedAt":"2014-11-07T18:50:49Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Trailers passed to the parse_trailer() function have\na trailing newline. When erroring out, we should display\nthe invalid trailer properly, that means without any\ntrailing newline.\n\nSigned-off-by: Christian Couder <chriscool@tuxfamily.org>\n---\n trailer.c | 8 ++++++--\n 1 file changed, 6 insertions(+), 2 deletions(-)\n\ndiff --git a/trailer.c b/trailer.c\nindex 761b763..f4d51ba 100644\n--- a/trailer.c\n+++ b/trailer.c\n@@ -583,8 +583,12 @@ static int parse_trailer(struct strbuf *tok, struct strbuf *val, const char *tra\n \tstrbuf_addch(&seps, '=');\n \tlen = strcspn(trailer, seps.buf);\n \tstrbuf_release(&seps);\n-\tif (len == 0)\n-\t\treturn error(_(\"empty trailer token in trailer '%s'\"), trailer);\n+\tif (len == 0) {\n+\t\tstruct strbuf sb = STRBUF_INIT;\n+\t\tstrbuf_addstr(&sb, trailer);\n+\t\tstrbuf_rtrim(&sb);\n+\t\treturn error(_(\"empty trailer token in trailer '%s'\"), sb.buf);\n+\t}\n \tif (len < strlen(trailer)) {\n \t\tstrbuf_add(tok, trailer, len);\n \t\tstrbuf_trim(tok);\n-- \n2.1.2.555.gfbecd99\n"},{"id":"251466","messageId":"20141107185053.16854.5866.chriscool@tuxfamily.org","threadId":"37898","inReplyTo":"20141107184148.16854.63825.chriscool@tuxfamily.org","subject":"[PATCH 3/5] commit: make ignore_non_trailer() non static","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2014-11-07T18:50:50Z","receivedAt":"2014-11-07T18:50:50Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Signed-off-by: Christian Couder <chriscool@tuxfamily.org>\n---\n builtin/commit.c | 46 ----------------------------------------------\n commit.c         | 46 ++++++++++++++++++++++++++++++++++++++++++++++\n commit.h         |  3 +++\n 3 files changed, 49 insertions(+), 46 deletions(-)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex e3c60dd..cda74e9 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -677,52 +677,6 @@ static void adjust_comment_line_char(const struct strbuf *sb)\n \tcomment_line_char = *p;\n }\n \n-/*\n- * Inspect sb and determine the true \"end\" of the log message, in\n- * order to find where to put a new Signed-off-by: line.  Ignored are\n- * trailing comment lines and blank lines, and also the traditional\n- * \"Conflicts:\" block that is not commented out, so that we can use\n- * \"git commit -s --amend\" on an existing commit that forgot to remove\n- * it.\n- *\n- * Returns the number of bytes from the tail to ignore, to be fed as\n- * the second parameter to append_signoff().\n- */\n-static int ignore_non_trailer(struct strbuf *sb)\n-{\n-\tint boc = 0;\n-\tint bol = 0;\n-\tint in_old_conflicts_block = 0;\n-\n-\twhile (bol < sb->len) {\n-\t\tchar *next_line;\n-\n-\t\tif (!(next_line = memchr(sb->buf + bol, '\\n', sb->len - bol)))\n-\t\t\tnext_line = sb->buf + sb->len;\n-\t\telse\n-\t\t\tnext_line++;\n-\n-\t\tif (sb->buf[bol] == comment_line_char || sb->buf[bol] == '\\n') {\n-\t\t\t/* is this the first of the run of comments? */\n-\t\t\tif (!boc)\n-\t\t\t\tboc = bol;\n-\t\t\t/* otherwise, it is just continuing */\n-\t\t} else if (starts_with(sb->buf + bol, \"Conflicts:\\n\")) {\n-\t\t\tin_old_conflicts_block = 1;\n-\t\t\tif (!boc)\n-\t\t\t\tboc = bol;\n-\t\t} else if (in_old_conflicts_block && sb->buf[bol] == '\\t') {\n-\t\t\t; /* a pathname in the conflicts block */\n-\t\t} else if (boc) {\n-\t\t\t/* the previous was not trailing comment */\n-\t\t\tboc = 0;\n-\t\t\tin_old_conflicts_block = 0;\n-\t\t}\n-\t\tbol = next_line - sb->buf;\n-\t}\n-\treturn boc ? sb->len - boc : 0;\n-}\n-\n static int prepare_to_commit(const char *index_file, const char *prefix,\n \t\t\t     struct commit *current_head,\n \t\t\t     struct wt_status *s,\ndiff --git a/commit.c b/commit.c\nindex 19cf8f9..a54cb9a 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -1640,3 +1640,49 @@ const char *find_commit_header(const char *msg, const char *key, size_t *out_len\n \t}\n \treturn NULL;\n }\n+\n+/*\n+ * Inspect sb and determine the true \"end\" of the log message, in\n+ * order to find where to put a new Signed-off-by: line.  Ignored are\n+ * trailing comment lines and blank lines, and also the traditional\n+ * \"Conflicts:\" block that is not commented out, so that we can use\n+ * \"git commit -s --amend\" on an existing commit that forgot to remove\n+ * it.\n+ *\n+ * Returns the number of bytes from the tail to ignore, to be fed as\n+ * the second parameter to append_signoff().\n+ */\n+int ignore_non_trailer(struct strbuf *sb)\n+{\n+\tint boc = 0;\n+\tint bol = 0;\n+\tint in_old_conflicts_block = 0;\n+\n+\twhile (bol < sb->len) {\n+\t\tchar *next_line;\n+\n+\t\tif (!(next_line = memchr(sb->buf + bol, '\\n', sb->len - bol)))\n+\t\t\tnext_line = sb->buf + sb->len;\n+\t\telse\n+\t\t\tnext_line++;\n+\n+\t\tif (sb->buf[bol] == comment_line_char || sb->buf[bol] == '\\n') {\n+\t\t\t/* is this the first of the run of comments? */\n+\t\t\tif (!boc)\n+\t\t\t\tboc = bol;\n+\t\t\t/* otherwise, it is just continuing */\n+\t\t} else if (starts_with(sb->buf + bol, \"Conflicts:\\n\")) {\n+\t\t\tin_old_conflicts_block = 1;\n+\t\t\tif (!boc)\n+\t\t\t\tboc = bol;\n+\t\t} else if (in_old_conflicts_block && sb->buf[bol] == '\\t') {\n+\t\t\t; /* a pathname in the conflicts block */\n+\t\t} else if (boc) {\n+\t\t\t/* the previous was not trailing comment */\n+\t\t\tboc = 0;\n+\t\t\tin_old_conflicts_block = 0;\n+\t\t}\n+\t\tbol = next_line - sb->buf;\n+\t}\n+\treturn boc ? sb->len - boc : 0;\n+}\ndiff --git a/commit.h b/commit.h\nindex bc68ccb..cd35ac1 100644\n--- a/commit.h\n+++ b/commit.h\n@@ -337,6 +337,9 @@ extern void free_commit_extra_headers(struct commit_extra_header *extra);\n extern const char *find_commit_header(const char *msg, const char *key,\n \t\t\t\t      size_t *out_len);\n \n+/* Find the end of the log message, the right place for a new trailer. */\n+extern int ignore_non_trailer(struct strbuf *sb);\n+\n typedef void (*each_mergetag_fn)(struct commit *commit, struct commit_extra_header *extra,\n \t\t\t\t void *cb_data);\n \n-- \n2.1.2.555.gfbecd99\n"},{"id":"251469","messageId":"20141107185053.16854.5660.chriscool@tuxfamily.org","threadId":"37898","inReplyTo":"20141107184148.16854.63825.chriscool@tuxfamily.org","subject":"[PATCH 4/5] trailer: reuse ignore_non_trailer() to ignore conflict lines","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2014-11-07T18:50:51Z","receivedAt":"2014-11-07T18:50:51Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Make sure we look for trailers before any conflict line\nby reusing the ignore_non_trailer() function.\n\nSigned-off-by: Christian Couder <chriscool@tuxfamily.org>\n---\n t/t7513-interpret-trailers.sh |  2 ++\n trailer.c                     | 25 ++++++++++++++++++-------\n 2 files changed, 20 insertions(+), 7 deletions(-)\n\ndiff --git a/t/t7513-interpret-trailers.sh b/t/t7513-interpret-trailers.sh\nindex 1efb880..fed053a 100755\n--- a/t/t7513-interpret-trailers.sh\n+++ b/t/t7513-interpret-trailers.sh\n@@ -232,6 +232,8 @@ test_expect_success 'with message that has comments' '\n \n \t\tReviewed-by: Johan\n \t\tCc: Peff\n+\t\t# last comment\n+\n \tEOF\n \tcat basic_patch >>expected &&\n \tgit interpret-trailers --trim-empty --trailer \"Cc: Peff\" message_with_comments >actual &&\ndiff --git a/trailer.c b/trailer.c\nindex f4d51ba..f6aa181 100644\n--- a/trailer.c\n+++ b/trailer.c\n@@ -2,6 +2,7 @@\n #include \"string-list.h\"\n #include \"run-command.h\"\n #include \"string-list.h\"\n+#include \"commit.h\"\n #include \"trailer.h\"\n /*\n  * Copyright (c) 2013, 2014 Christian Couder <chriscool@tuxfamily.org>\n@@ -791,14 +792,24 @@ static int process_input_file(struct strbuf **lines,\n \t\t\t      struct trailer_item **in_tok_last)\n {\n \tint count = 0;\n-\tint patch_start, trailer_start, i;\n+\tint trailer_start, trailer_end, patch_start, ignore_bytes, i;\n+\tstruct strbuf sb;\n \n \t/* Get the line count */\n \twhile (lines[count])\n \t\tcount++;\n \n \tpatch_start = find_patch_start(lines, count);\n-\ttrailer_start = find_trailer_start(lines, patch_start);\n+\n+\t/* Get the index of the end of the trailers */\n+\tfor (i = 0; i < patch_start; i++)\n+\t\tstrbuf_addbuf(&sb, lines[i]);\n+\tignore_bytes = ignore_non_trailer(&sb);\n+\tfor (i = patch_start - 1; i >= 0 && ignore_bytes > 0; i--)\n+\t\tignore_bytes -= lines[i]->len;\n+\ttrailer_end = i + 1;\n+\n+\ttrailer_start = find_trailer_start(lines, trailer_end);\n \n \t/* Print lines before the trailers as is */\n \tprint_lines(lines, 0, trailer_start);\n@@ -807,14 +818,14 @@ static int process_input_file(struct strbuf **lines,\n \t\tprintf(\"\\n\");\n \n \t/* Parse trailer lines */\n-\tfor (i = trailer_start; i < patch_start; i++) {\n+\tfor (i = trailer_start; i < trailer_end; i++) {\n \t\tif (lines[i]->buf[0] != comment_line_char) {\n \t\t\tstruct trailer_item *new = create_trailer_item(lines[i]->buf);\n \t\t\tadd_trailer_item(in_tok_first, in_tok_last, new);\n \t\t}\n \t}\n \n-\treturn patch_start;\n+\treturn trailer_end;\n }\n \n static void free_all(struct trailer_item **first)\n@@ -831,7 +842,7 @@ void process_trailers(const char *file, int trim_empty, struct string_list *trai\n \tstruct trailer_item *in_tok_last = NULL;\n \tstruct trailer_item *arg_tok_first;\n \tstruct strbuf **lines;\n-\tint patch_start;\n+\tint trailer_end;\n \n \t/* Default config must be setup first */\n \tgit_config(git_trailer_default_config, NULL);\n@@ -840,7 +851,7 @@ void process_trailers(const char *file, int trim_empty, struct string_list *trai\n \tlines = read_input_file(file);\n \n \t/* Print the lines before the trailers */\n-\tpatch_start = process_input_file(lines, &in_tok_first, &in_tok_last);\n+\ttrailer_end = process_input_file(lines, &in_tok_first, &in_tok_last);\n \n \targ_tok_first = process_command_line_args(trailers);\n \n@@ -851,7 +862,7 @@ void process_trailers(const char *file, int trim_empty, struct string_list *trai\n \tfree_all(&in_tok_first);\n \n \t/* Print the lines after the trailers as is */\n-\tprint_lines(lines, patch_start, INT_MAX);\n+\tprint_lines(lines, trailer_end, INT_MAX);\n \n \tstrbuf_list_free(lines);\n }\n-- \n2.1.2.555.gfbecd99\n"},{"id":"251468","messageId":"20141107185053.16854.69782.chriscool@tuxfamily.org","threadId":"37898","inReplyTo":"20141107184148.16854.63825.chriscool@tuxfamily.org","subject":"[PATCH 5/5] trailer: add test with an old style conflict block","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2014-11-07T18:50:52Z","receivedAt":"2014-11-07T18:50:52Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Signed-off-by: Christian Couder <chriscool@tuxfamily.org>\n---\n t/t7513-interpret-trailers.sh | 32 +++++++++++++++++++++++++++++++-\n 1 file changed, 31 insertions(+), 1 deletion(-)\n\ndiff --git a/t/t7513-interpret-trailers.sh b/t/t7513-interpret-trailers.sh\nindex fed053a..bd0ab46 100755\n--- a/t/t7513-interpret-trailers.sh\n+++ b/t/t7513-interpret-trailers.sh\n@@ -213,7 +213,7 @@ test_expect_success 'with 2 files arguments' '\n '\n \n test_expect_success 'with message that has comments' '\n-\tcat basic_message >>message_with_comments &&\n+\tcat basic_message >message_with_comments &&\n \tsed -e \"s/ Z\\$/ /\" >>message_with_comments <<-\\EOF &&\n \t\t# comment\n \n@@ -240,6 +240,36 @@ test_expect_success 'with message that has comments' '\n \ttest_cmp expected actual\n '\n \n+test_expect_success 'with message that has an old style conflict block' '\n+\tcat basic_message >message_with_comments &&\n+\tsed -e \"s/ Z\\$/ /\" >>message_with_comments <<-\\EOF &&\n+\t\t# comment\n+\n+\t\t# other comment\n+\t\tCc: Z\n+\t\t# yet another comment\n+\t\tReviewed-by: Johan\n+\t\tReviewed-by: Z\n+\t\t# last comment\n+\n+\t\tConflicts:\n+\n+\tEOF\n+\tcat basic_message >expected &&\n+\tcat >>expected <<-\\EOF &&\n+\t\t# comment\n+\n+\t\tReviewed-by: Johan\n+\t\tCc: Peff\n+\t\t# last comment\n+\n+\t\tConflicts:\n+\n+\tEOF\n+\tgit interpret-trailers --trim-empty --trailer \"Cc: Peff\" message_with_comments >actual &&\n+\ttest_cmp expected actual\n+'\n+\n test_expect_success 'with commit complex message and trailer args' '\n \tcat complex_message_body >expected &&\n \tsed -e \"s/ Z\\$/ /\" >>expected <<-\\EOF &&\n-- \n2.1.2.555.gfbecd99\n"},{"id":"251478","messageId":"20141107192239.GC5695@peff.net","threadId":"37898","inReplyTo":"20141107185053.16854.84253.chriscool@tuxfamily.org","subject":"Re: [PATCH 2/5] trailer: display a trailer without its trailing newline","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-11-07T19:22:39Z","receivedAt":"2014-11-07T19:22:39Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Nov 07, 2014 at 07:50:49PM +0100, Christian Couder wrote:\n\n> diff --git a/trailer.c b/trailer.c\n> index 761b763..f4d51ba 100644\n> --- a/trailer.c\n> +++ b/trailer.c\n> @@ -583,8 +583,12 @@ static int parse_trailer(struct strbuf *tok, struct strbuf *val, const char *tra\n>  \tstrbuf_addch(&seps, '=');\n>  \tlen = strcspn(trailer, seps.buf);\n>  \tstrbuf_release(&seps);\n> -\tif (len == 0)\n> -\t\treturn error(_(\"empty trailer token in trailer '%s'\"), trailer);\n> +\tif (len == 0) {\n> +\t\tstruct strbuf sb = STRBUF_INIT;\n> +\t\tstrbuf_addstr(&sb, trailer);\n> +\t\tstrbuf_rtrim(&sb);\n> +\t\treturn error(_(\"empty trailer token in trailer '%s'\"), sb.buf);\n> +\t}\n\nDoesn't this leak sb.buf?\n\n-Peff\n"},{"id":"251479","messageId":"xmqqd28yg6cz.fsf@gitster.dls.corp.google.com","threadId":"37898","inReplyTo":"20141107192239.GC5695@peff.net","subject":"Re: [PATCH 2/5] trailer: display a trailer without its trailing newline","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-11-07T19:26:04Z","receivedAt":"2014-11-07T19:26:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Fri, Nov 07, 2014 at 07:50:49PM +0100, Christian Couder wrote:\n>\n>> diff --git a/trailer.c b/trailer.c\n>> index 761b763..f4d51ba 100644\n>> --- a/trailer.c\n>> +++ b/trailer.c\n>> @@ -583,8 +583,12 @@ static int parse_trailer(struct strbuf *tok,\n>> struct strbuf *val, const char *tra\n>>  \tstrbuf_addch(&seps, '=');\n>>  \tlen = strcspn(trailer, seps.buf);\n>>  \tstrbuf_release(&seps);\n>> -\tif (len == 0)\n>> -\t\treturn error(_(\"empty trailer token in trailer '%s'\"), trailer);\n>> +\tif (len == 0) {\n>> +\t\tstruct strbuf sb = STRBUF_INIT;\n>> +\t\tstrbuf_addstr(&sb, trailer);\n>> +\t\tstrbuf_rtrim(&sb);\n>> +\t\treturn error(_(\"empty trailer token in trailer '%s'\"), sb.buf);\n>> +\t}\n>\n> Doesn't this leak sb.buf?\n\nYes.  \"%.*s\" might be your friend.\n"},{"id":"251516","messageId":"xmqqzjc2eoyw.fsf@gitster.dls.corp.google.com","threadId":"37898","inReplyTo":"20141107185053.16854.5660.chriscool@tuxfamily.org","subject":"Re: [PATCH 4/5] trailer: reuse ignore_non_trailer() to ignore conflict lines","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-11-07T20:27:03Z","receivedAt":"2014-11-07T20:27:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder <chriscool@tuxfamily.org> writes:\n\n>   * Copyright (c) 2013, 2014 Christian Couder <chriscool@tuxfamily.org>\n> @@ -791,14 +792,24 @@ static int process_input_file(struct strbuf **lines,\n>  \t\t\t      struct trailer_item **in_tok_last)\n>  {\n>  \tint count = 0;\n> -\tint patch_start, trailer_start, i;\n> +\tint trailer_start, trailer_end, patch_start, ignore_bytes, i;\n> +\tstruct strbuf sb;\n>  \n>  \t/* Get the line count */\n>  \twhile (lines[count])\n>  \t\tcount++;\n>  \n>  \tpatch_start = find_patch_start(lines, count);\n> -\ttrailer_start = find_trailer_start(lines, patch_start);\n> +\n> +\t/* Get the index of the end of the trailers */\n> +\tfor (i = 0; i < patch_start; i++)\n> +\t\tstrbuf_addbuf(&sb, lines[i]);\n> +\tignore_bytes = ignore_non_trailer(&sb);\n> +\tfor (i = patch_start - 1; i >= 0 && ignore_bytes > 0; i--)\n> +\t\tignore_bytes -= lines[i]->len;\n> +\ttrailer_end = i + 1;\n\nLooks like there is an impedance mismatch between the caller and the\ncallee here.  I can sort of understand why you might want to have an\narray of trailer items, one element per line in the trailer block,\nas that would make it easier on your brain when you have to reorder\nthem, insert a new one or a remove an existing one, but you seem to\nbe keeping _everything_ in that format, an array of strbuf with one\nstrbuf per line, which sounds really wasteful.  The data structure\nmight need to be rethoughtbefore this code becomes ready for\nproduction.\n\nI would have expected it to be more like (1) slurp everything in a\nsingle strbuf, (2) find the trailer block inside that single strbuf,\nsplitting what you read in the previous step into one strbuf for\nstuff before the trailer block, another strbuf for stuff after the\ntrailer block and an array of lines in the tailer block, (3) do\nwhatever your trailer flipping logic inside that array without\nhaving to worry about stuff before or after the trailer block and\nthen finally (4) spit the whole thing out by concatenating the stuff\nbefore the trailer block, the stuff in the trailer block and the\nstuff after the trailer block.\n\nOh well...\n"},{"id":"251592","messageId":"20141109.113509.1363113947439404678.chriscool@tuxfamily.org","threadId":"37898","inReplyTo":"xmqqzjc2eoyw.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 4/5] trailer: reuse ignore_non_trailer() to ignore conflict lines","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2014-11-09T10:35:09Z","receivedAt":"2014-11-09T10:35:09Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"From: Junio C Hamano <gitster@pobox.com>\nSubject: Re: [PATCH 4/5] trailer: reuse ignore_non_trailer() to ignore conflict lines\nDate: Fri, 07 Nov 2014 12:27:03 -0800\n\n> Christian Couder <chriscool@tuxfamily.org> writes:\n> \n>>   * Copyright (c) 2013, 2014 Christian Couder <chriscool@tuxfamily.org>\n>> @@ -791,14 +792,24 @@ static int process_input_file(struct strbuf **lines,\n>>  \t\t\t      struct trailer_item **in_tok_last)\n>>  {\n>>  \tint count = 0;\n>> -\tint patch_start, trailer_start, i;\n>> +\tint trailer_start, trailer_end, patch_start, ignore_bytes, i;\n>> +\tstruct strbuf sb;\n>>  \n>>  \t/* Get the line count */\n>>  \twhile (lines[count])\n>>  \t\tcount++;\n>>  \n>>  \tpatch_start = find_patch_start(lines, count);\n>> -\ttrailer_start = find_trailer_start(lines, patch_start);\n>> +\n>> +\t/* Get the index of the end of the trailers */\n>> +\tfor (i = 0; i < patch_start; i++)\n>> +\t\tstrbuf_addbuf(&sb, lines[i]);\n>> +\tignore_bytes = ignore_non_trailer(&sb);\n>> +\tfor (i = patch_start - 1; i >= 0 && ignore_bytes > 0; i--)\n>> +\t\tignore_bytes -= lines[i]->len;\n>> +\ttrailer_end = i + 1;\n> \n> Looks like there is an impedance mismatch between the caller and the\n> callee here.  I can sort of understand why you might want to have an\n> array of trailer items, one element per line in the trailer block,\n> as that would make it easier on your brain when you have to reorder\n> them, insert a new one or a remove an existing one, but you seem to\n> be keeping _everything_ in that format, an array of strbuf with one\n> strbuf per line, which sounds really wasteful.  The data structure\n> might need to be rethoughtbefore this code becomes ready for\n> production.\n> \n> I would have expected it to be more like (1) slurp everything in a\n> single strbuf, (2) find the trailer block inside that single strbuf,\n> splitting what you read in the previous step into one strbuf for\n> stuff before the trailer block, another strbuf for stuff after the\n> trailer block and an array of lines in the tailer block, (3) do\n> whatever your trailer flipping logic inside that array without\n> having to worry about stuff before or after the trailer block and\n> then finally (4) spit the whole thing out by concatenating the stuff\n> before the trailer block, the stuff in the trailer block and the\n> stuff after the trailer block.\n> \n> Oh well...\n\nI hope it is better now, as I encapsulated the call to\nignore_non_trailer() into a new find_trailer_end() function. There\nwere already find_trailer_start() and find_patch_start(), and I think\nthis way we could have a nice high level line oriented API.\n\nYeah, it won't be as efficient as using only one strbuf and only byte\noriented functions, and it looks much less manly too :-) But over time in\nGit we have developed a number of less efficient but quite clean\nabstractions like strbuf, argv_array, sha1_array and so on, that we\nare quite happy with.\n\nSo yeah, with old age we might have lost some manliness and lament the\ntime when men were really men, and ... Well, let's wait for the\nhopefully big party^W conference next year to do that together :-)\n\nThanks,\nChristian.\n"},{"id":"251619","messageId":"xmqqfvdse308.fsf@gitster.dls.corp.google.com","threadId":"37898","inReplyTo":"20141109.113509.1363113947439404678.chriscool@tuxfamily.org","subject":"Re: [PATCH 4/5] trailer: reuse ignore_non_trailer() to ignore conflict lines","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-11-09T16:45:59Z","receivedAt":"2014-11-09T16:45:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder <chriscool@tuxfamily.org> writes:\n\n> Yeah, it won't be as efficient as using only one strbuf and only byte\n> oriented functions, and it looks much less manly too :-) But over time in\n> Git we have developed a number of less efficient but quite clean\n> abstractions like strbuf, argv_array, sha1_array and so on, that we\n> are quite happy with.\n\nActually, all these examples you gave are fairly efficient and clean\nabstractions.  I find it insulting to pretend that the \"one line per\nstrbuf\" is in the same league.  It isn't.\n\nAnd it is not about manliness.\n"},{"id":"251631","messageId":"xmqqlhnkb5bf.fsf@gitster.dls.corp.google.com","threadId":"37898","inReplyTo":"xmqqfvdse308.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 4/5] trailer: reuse ignore_non_trailer() to ignore conflict lines","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-11-09T18:24:20Z","receivedAt":"2014-11-09T18:24:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Christian Couder <chriscool@tuxfamily.org> writes:\n>\n>> Yeah, it won't be as efficient as using only one strbuf and only byte\n>> oriented functions, and it looks much less manly too :-) But over time in\n>> Git we have developed a number of less efficient but quite clean\n>> abstractions like strbuf, argv_array, sha1_array and so on, that we\n>> are quite happy with.\n>\n> Actually, all these examples you gave are fairly efficient and clean\n> abstractions.  I find it insulting to pretend that the \"one line per\n> strbuf\" is in the same league.  It isn't.\n>\n> And it is not about manliness.\n\nBy the way, I do not mean to say that all of strbuf (which is a\nrather full API) is uniformly efficient and cleanly abstracted.\n\nFor example, I see strbuf_split() is a handy but a lousy invitation\nto stupid programmers to do stupid things and needs to be used\ncarefully (rather, you need to carefully consider if a possible\nsolution that uses it is really a good solution).  If you start with\na single strbuf (e.g. perhaps read from a file or a blob in bulk in\nan I/O efficient way), pass it around and then have to manipulate\neach and every line in it to massage the information on each line\ninto other useful form (e.g. perhaps each line is a [+]src:dst\nrefspec element and you are parsing it into an array of \"struct\nrefspec\"), it may be tempting to write a parser from a single strbuf\ninto a single refspec, use strbuf_split() to break the single strbuf\ninput into multiple and feed each of them to your parser.\n\nBut it does not have to be the only way to create such a caller.\nUnless the final \"other useful form\" is an array of strbuf, and you\nhave to remember that the whole point of using strbuf is because it\nis easy to edit its contents efficiently in place, that needs to\nfurther be modified in various ways, the splitting of N lines into N\nstrbuf, only to parse each line one by one, is a huge waste.  A\nright approach may involve introducing a foreach_strbuf_line\niterator function that takes a callback, or a macro of the same name\nthat puts a control structure.\n\nGoing back to the trailer processing, if you are likely to be\nediting each and every line in the input before producing the final\nresult, an array of strbuf would be a perfectly sensible data\nstructure to use.  I just didn't get the impression that that is\nwhat is being done.  You would be presented a single text with\nmultiple lines (whose natural representation would be a <ptr,len>\nthat fits in a single strbuf), you would be asked to inspect and\nmanipulate one single section in the middle (namely, the runs of\nlines that begin with some \"keyword: \"), and return the\nconcatenation of the part before that one section (intact), the\nsection you have manipulated (i.e. the trailer) and possibly the\nremainder (intact).  Outside that single section you will be\ntouching, I do not see a good reason for each of their lines to be\nfirst stored in a strbuf (while moving bunch of pointers in the\narray using ARRAY_GROW() and friends), only to be later concatenated\nback into a single string.\n"},{"id":"251642","messageId":"xmqq8ujkath2.fsf@gitster.dls.corp.google.com","threadId":"37898","inReplyTo":"xmqqlhnkb5bf.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 4/5] trailer: reuse ignore_non_trailer() to ignore conflict lines","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-11-09T22:40:09Z","receivedAt":"2014-11-09T22:40:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Junio C Hamano <gitster@pobox.com> writes:\n>\n>> Christian Couder <chriscool@tuxfamily.org> writes:\n>>\n>>> Yeah, it won't be as efficient as using only one strbuf and only byte\n>>> oriented functions, and it looks much less manly too :-) But over time in\n>>> Git we have developed a number of less efficient but quite clean\n>>> abstractions like strbuf, argv_array, sha1_array and so on, that we\n>>> are quite happy with.\n>>\n>> Actually, all these examples you gave are fairly efficient and clean\n>> abstractions.  I find it insulting to pretend that the \"one line per\n>> strbuf\" is in the same league.  It isn't.\n>>\n>> And it is not about manliness.\n>\n> By the way, I do not mean to say that all of strbuf (which is a\n> rather full API) is uniformly efficient and cleanly abstracted.\n> ...\n\nHaving said all that, please notice that I said \"might need to be\nrethought before the code becomes ready for production.\"\n\nAfter all, it would have perfectly been OK to experiment with this\nnew topic in a script; e.g. this topic being text processing, it\nmight have happened as a Perl script while we get the semantics and\ndesirable features nailed down before rewriting the real thing in C.\n\nAnd if that were what happened here, I wouldn't have been talking\nabout data structures and unnecessary complexity having to have one\nstrbuf per line only to join them into one string (or printf() out\nto a stream one-after-another) later.\n\nIn other words, I did not say \"this code is too ugly to live and we\nshould clean it up as soon as possible or we should revert it from\n'master'---it was a mistake to merge it\".\n\nI consider the \"trailer\" code in 'master' today as experimental (in\nother words, something I might have written in a scripting language\nas a mock-up if I were doing it) that needs to be worked on further\nto still get the features and semantics right.  And the patch series\nin this thread are hopefully taking us in the right direction to get\nthem right [*1*].\n\nYou could have just said \"Yeah, I realize that the code is way\nsuboptimal, but lets get it feature complete first and then clean it\nup\", or something, instead of getting defensive with unnecessary\nexcuses.\n\n\n[Footnote]\n\n*1* ... even though this step exposes why the approach taken,\nsplitting the input lines first into individual lines without even\nlooking at them, is a wrong one---if you started from a single\nbuffer that is not yet split, it would have been much easier to find\nwhere the trailer block is, and that is why you are re-joining the\nbuffer you have already split into lines (i.e. what I called\n\"impedance mismatch\"), which shows that the initial splitting is\ndone prematurely.  Hopefully you would realize that by now ;-).\n"}]}