{"thread":{"id":"37827","subject":"[PATCH v2 1/4] builtin/merge.c: drop a parameter that is never used","startedAt":"2014-10-28T21:36:48Z","lastAt":"2014-10-28T21:36:52Z","messageCount":5,"participants":["Junio C Hamano"],"isPatch":true,"patchVersion":2,"patchTotal":4},"messages":[{"id":"251161","messageId":"1414532212-9016-1-git-send-email-gitster@pobox.com","threadId":"37827","inReplyTo":"xmqq1tpxgw6t.fsf@gitster.dls.corp.google.com","subject":"[PATCH v2 0/4] Turning Conflicts: hint into comment","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-10-28T21:36:48Z","receivedAt":"2014-10-28T21:36:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"So here is a reroll to hopefully bring this topic closer to\nperfection.\n\nThe first two patches haven't changed, except that they are now part\nof a numbered series.\n\nThe third patch is a refactoring that helps clarify what happens in\nthe last step, which is new.\n\nThe endgame is similar to what was posted before, except that we pay\nattention to \"Conflicts:\" and HT indented pathnames in addition to\ncomments and blanks when determining the true end of the log\nmessage.  This is so that running \"git commit --amend -s\" on a\ncommit that was created with older versions of Git by a careless\nuser who left the old conflicts hint around will insert the new\nsign-off at the right place.\n\nThe series is designed to apply on v1.8.5.x series, even though I do\nnot anticpiate that we would need to backport this to maintenance\nbranches.  !prefixcmp() will need to turn into starts_with() when\nmerging to newer codebase, which I can let rerere take care of.\n\nJunio C Hamano (4):\n  builtin/merge.c: drop a parameter that is never used\n  merge & sequencer: unify codepaths that write \"Conflicts:\" hint\n  builtin/commit.c: extract ignore_non_trailer() helper function\n  merge & sequencer: turn \"Conflicts:\" hint into a comment\n\n builtin/commit.c                | 74 ++++++++++++++++++++++++++---------------\n builtin/merge.c                 | 22 ++++--------\n sequencer.c                     | 34 ++++++++++---------\n sequencer.h                     |  1 +\n t/t3507-cherry-pick-conflict.sh | 42 ++++++++++++++++++-----\n 5 files changed, 109 insertions(+), 64 deletions(-)\n\n-- \n2.1.2-620-g33c52cb\n"},{"id":"251160","messageId":"1414532212-9016-2-git-send-email-gitster@pobox.com","threadId":"37827","inReplyTo":"1414532212-9016-1-git-send-email-gitster@pobox.com","subject":"[PATCH v2 1/4] builtin/merge.c: drop a parameter that is never used","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-10-28T21:36:49Z","receivedAt":"2014-10-28T21:36:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Since the very beginning when we added the \"renormalizing\" parameter\nto this function with 7610fa57 (merge-recursive --renormalize,\n2010-08-05), nobody seems to have ever referenced it.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n builtin/merge.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex 41fb66d..f6894c7 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -884,7 +884,7 @@ static int finish_automerge(struct commit *head,\n \treturn 0;\n }\n \n-static int suggest_conflicts(int renormalizing)\n+static int suggest_conflicts(void)\n {\n \tconst char *filename;\n \tFILE *fp;\n@@ -1557,7 +1557,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \t\tfprintf(stderr, _(\"Automatic merge went well; \"\n \t\t\t\"stopped before committing as requested\\n\"));\n \telse\n-\t\tret = suggest_conflicts(option_renormalize);\n+\t\tret = suggest_conflicts();\n \n done:\n \tfree(branch_to_free);\n-- \n2.1.2-620-g33c52cb\n"},{"id":"251162","messageId":"1414532212-9016-3-git-send-email-gitster@pobox.com","threadId":"37827","inReplyTo":"1414532212-9016-1-git-send-email-gitster@pobox.com","subject":"[PATCH v2 2/4] merge & sequencer: unify codepaths that write \"Conflicts:\" hint","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-10-28T21:36:50Z","receivedAt":"2014-10-28T21:36:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Two identical loops in suggest_conflicts() in merge, and\ndo_recursive_merge() in sequencer, can use a single helper function\nextracted from the latter that prepares the \"Conflicts:\" hint that\nis meant to remind the user the paths for which merge conflicts had\nto be resolved to write a better commit log message.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n builtin/merge.c | 18 +++++-------------\n sequencer.c     | 35 ++++++++++++++++++++---------------\n sequencer.h     |  1 +\n 3 files changed, 26 insertions(+), 28 deletions(-)\n\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex f6894c7..d30cb60 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -28,6 +28,7 @@\n #include \"remote.h\"\n #include \"fmt-merge-msg.h\"\n #include \"gpg-interface.h\"\n+#include \"sequencer.h\"\n \n #define DEFAULT_TWOHEAD (1<<0)\n #define DEFAULT_OCTOPUS (1<<1)\n@@ -888,24 +889,15 @@ static int suggest_conflicts(void)\n {\n \tconst char *filename;\n \tFILE *fp;\n-\tint pos;\n+\tstruct strbuf msgbuf = STRBUF_INIT;\n \n \tfilename = git_path(\"MERGE_MSG\");\n \tfp = fopen(filename, \"a\");\n \tif (!fp)\n \t\tdie_errno(_(\"Could not open '%s' for writing\"), filename);\n-\tfprintf(fp, \"\\nConflicts:\\n\");\n-\tfor (pos = 0; pos < active_nr; pos++) {\n-\t\tconst struct cache_entry *ce = active_cache[pos];\n-\n-\t\tif (ce_stage(ce)) {\n-\t\t\tfprintf(fp, \"\\t%s\\n\", ce->name);\n-\t\t\twhile (pos + 1 < active_nr &&\n-\t\t\t\t\t!strcmp(ce->name,\n-\t\t\t\t\t\tactive_cache[pos + 1]->name))\n-\t\t\t\tpos++;\n-\t\t}\n-\t}\n+\n+\tappend_conflicts_hint(&msgbuf);\n+\tfputs(msgbuf.buf, fp);\n \tfclose(fp);\n \trerere(allow_rerere_auto);\n \tprintf(_(\"Automatic merge failed; \"\ndiff --git a/sequencer.c b/sequencer.c\nindex 06e52b4..0f84bbe 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -287,6 +287,24 @@ static int fast_forward_to(const unsigned char *to, const unsigned char *from,\n \treturn ret;\n }\n \n+void append_conflicts_hint(struct strbuf *msgbuf)\n+{\n+\tint i;\n+\n+\tstrbuf_addstr(msgbuf, \"\\nConflicts:\\n\");\n+\tfor (i = 0; i < active_nr;) {\n+\t\tconst struct cache_entry *ce = active_cache[i++];\n+\t\tif (ce_stage(ce)) {\n+\t\t\tstrbuf_addch(msgbuf, '\\t');\n+\t\t\tstrbuf_addstr(msgbuf, ce->name);\n+\t\t\tstrbuf_addch(msgbuf, '\\n');\n+\t\t\twhile (i < active_nr && !strcmp(ce->name,\n+\t\t\t\t\t\t\tactive_cache[i]->name))\n+\t\t\t\ti++;\n+\t\t}\n+\t}\n+}\n+\n static int do_recursive_merge(struct commit *base, struct commit *next,\n \t\t\t      const char *base_label, const char *next_label,\n \t\t\t      unsigned char *head, struct strbuf *msgbuf,\n@@ -328,21 +346,8 @@ static int do_recursive_merge(struct commit *base, struct commit *next,\n \tif (opts->signoff)\n \t\tappend_signoff(msgbuf, 0, 0);\n \n-\tif (!clean) {\n-\t\tint i;\n-\t\tstrbuf_addstr(msgbuf, \"\\nConflicts:\\n\");\n-\t\tfor (i = 0; i < active_nr;) {\n-\t\t\tconst struct cache_entry *ce = active_cache[i++];\n-\t\t\tif (ce_stage(ce)) {\n-\t\t\t\tstrbuf_addch(msgbuf, '\\t');\n-\t\t\t\tstrbuf_addstr(msgbuf, ce->name);\n-\t\t\t\tstrbuf_addch(msgbuf, '\\n');\n-\t\t\t\twhile (i < active_nr && !strcmp(ce->name,\n-\t\t\t\t\t\tactive_cache[i]->name))\n-\t\t\t\t\ti++;\n-\t\t\t}\n-\t\t}\n-\t}\n+\tif (!clean)\n+\t\tappend_conflicts_hint(msgbuf);\n \n \treturn !clean;\n }\ndiff --git a/sequencer.h b/sequencer.h\nindex 1fc22dc..c53519d 100644\n--- a/sequencer.h\n+++ b/sequencer.h\n@@ -51,5 +51,6 @@ int sequencer_pick_revisions(struct replay_opts *opts);\n extern const char sign_off_header[];\n \n void append_signoff(struct strbuf *msgbuf, int ignore_footer, unsigned flag);\n+void append_conflicts_hint(struct strbuf *msgbuf);\n \n #endif\n-- \n2.1.2-620-g33c52cb\n"},{"id":"251163","messageId":"1414532212-9016-4-git-send-email-gitster@pobox.com","threadId":"37827","inReplyTo":"1414532212-9016-1-git-send-email-gitster@pobox.com","subject":"[PATCH v2 3/4] builtin/commit.c: extract ignore_non_trailer() helper function","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-10-28T21:36:51Z","receivedAt":"2014-10-28T21:36:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Extract a helper function from prepare_to_commit() to determine\nwhere to place a new Signed-off-by: line, which is essentially the\ntrue \"end\" of the log message, ignoring the trailing \"Conflicts:\"\nline and everything below it.\n\nThe detection _should_ make sure the \"Conflicts:\" line it finds is\ntruly the conflict hint block by checking everything that follows is\na HT indented pathname to avoid false positive, but this logic will\nbe revamped in a later patch to ignore comments and blanks anyway,\nso it is left as-is in this step.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n builtin/commit.c | 59 +++++++++++++++++++++++++++++++-------------------------\n 1 file changed, 33 insertions(+), 26 deletions(-)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex fedb45a..cd455aa 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -593,6 +593,37 @@ static char *cut_ident_timestamp_part(char *string)\n \treturn ket;\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 \"Conflict:\" block.\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 ignore_footer = 0;\n+\tint i, eol, previous = 0;\n+\tconst char *nl;\n+\n+\tfor (i = 0; i < sb->len; i++) {\n+\t\tnl = memchr(sb->buf + i, '\\n', sb->len - i);\n+\t\tif (nl)\n+\t\t\teol = nl - sb->buf;\n+\t\telse\n+\t\t\teol = sb->len;\n+\t\tif (!prefixcmp(sb->buf + previous, \"\\nConflicts:\\n\")) {\n+\t\t\tignore_footer = sb->len - previous;\n+\t\t\tbreak;\n+\t\t}\n+\t\twhile (i < eol)\n+\t\t\ti++;\n+\t\tprevious = eol;\n+\t}\n+\treturn ignore_footer;\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,\n@@ -718,32 +749,8 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n \tif (clean_message_contents)\n \t\tstripspace(&sb, 0);\n \n-\tif (signoff) {\n-\t\t/*\n-\t\t * See if we have a Conflicts: block at the end. If yes, count\n-\t\t * its size, so we can ignore it.\n-\t\t */\n-\t\tint ignore_footer = 0;\n-\t\tint i, eol, previous = 0;\n-\t\tconst char *nl;\n-\n-\t\tfor (i = 0; i < sb.len; i++) {\n-\t\t\tnl = memchr(sb.buf + i, '\\n', sb.len - i);\n-\t\t\tif (nl)\n-\t\t\t\teol = nl - sb.buf;\n-\t\t\telse\n-\t\t\t\teol = sb.len;\n-\t\t\tif (!prefixcmp(sb.buf + previous, \"\\nConflicts:\\n\")) {\n-\t\t\t\tignore_footer = sb.len - previous;\n-\t\t\t\tbreak;\n-\t\t\t}\n-\t\t\twhile (i < eol)\n-\t\t\t\ti++;\n-\t\t\tprevious = eol;\n-\t\t}\n-\n-\t\tappend_signoff(&sb, ignore_footer, 0);\n-\t}\n+\tif (signoff)\n+\t\tappend_signoff(&sb, ignore_non_trailer(&sb), 0);\n \n \tif (fwrite(sb.buf, 1, sb.len, s->fp) < sb.len)\n \t\tdie_errno(_(\"could not write commit template\"));\n-- \n2.1.2-620-g33c52cb\n"},{"id":"251164","messageId":"1414532212-9016-5-git-send-email-gitster@pobox.com","threadId":"37827","inReplyTo":"1414532212-9016-1-git-send-email-gitster@pobox.com","subject":"[PATCH v2 4/4] merge & sequencer: turn \"Conflicts:\" hint into a comment","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-10-28T21:36:52Z","receivedAt":"2014-10-28T21:36:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Just like other hints such as \"Changes to be committed\" we show in\nthe editor to remind the committer what paths were involved in the\nresulting commit to help improving their log message, this section\nis merely a reminder.\n\nTraditionally, it was not made into comments primarily because it\nhas to be generated outside the wt-status infrastructure, and also\nbecause it was meant as a bit stronger reminder than the others\n(i.e. explaining how you resolved conflicts is much more important\nthan mentioning what you did to every paths involved in the commit).\n\nBut that still does not make this hint a part of the log message\nproper, and not showing it as a comment is inviting mistakes.\n\nNote that we still notice \"Conflicts:\" followed by list of indented\npathnames as an old-style cruft and insert a new Signed-off-by:\nbefore it.  This is so that \"commit --amend -s\" adds the new S-o-b\nat the right place when used on an older commit.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n builtin/commit.c                | 47 +++++++++++++++++++++++++++--------------\n sequencer.c                     |  7 +++---\n t/t3507-cherry-pick-conflict.sh | 42 +++++++++++++++++++++++++++++-------\n 3 files changed, 68 insertions(+), 28 deletions(-)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex cd455aa..0a78e76 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -596,32 +596,47 @@ static char *cut_ident_timestamp_part(char *string)\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 \"Conflict:\" block.\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 ignore_footer = 0;\n-\tint i, eol, previous = 0;\n-\tconst char *nl;\n+\tint boc = 0;\n+\tint bol = 0;\n+\tint in_old_conflicts_block = 0;\n \n-\tfor (i = 0; i < sb->len; i++) {\n-\t\tnl = memchr(sb->buf + i, '\\n', sb->len - i);\n-\t\tif (nl)\n-\t\t\teol = nl - sb->buf;\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\teol = sb->len;\n-\t\tif (!prefixcmp(sb->buf + previous, \"\\nConflicts:\\n\")) {\n-\t\t\tignore_footer = sb->len - previous;\n-\t\t\tbreak;\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 (!prefixcmp(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\twhile (i < eol)\n-\t\t\ti++;\n-\t\tprevious = eol;\n+\t\tbol = next_line - sb->buf;\n \t}\n-\treturn ignore_footer;\n+\treturn boc ? sb->len - boc : 0;\n }\n \n static int prepare_to_commit(const char *index_file, const char *prefix,\ndiff --git a/sequencer.c b/sequencer.c\nindex 0f84bbe..1d97da3 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -291,13 +291,12 @@ void append_conflicts_hint(struct strbuf *msgbuf)\n {\n \tint i;\n \n-\tstrbuf_addstr(msgbuf, \"\\nConflicts:\\n\");\n+\tstrbuf_addch(msgbuf, '\\n');\n+\tstrbuf_commented_addf(msgbuf, \"Conflicts:\\n\");\n \tfor (i = 0; i < active_nr;) {\n \t\tconst struct cache_entry *ce = active_cache[i++];\n \t\tif (ce_stage(ce)) {\n-\t\t\tstrbuf_addch(msgbuf, '\\t');\n-\t\t\tstrbuf_addstr(msgbuf, ce->name);\n-\t\t\tstrbuf_addch(msgbuf, '\\n');\n+\t\t\tstrbuf_commented_addf(msgbuf, \"\\t%s\\n\", ce->name);\n \t\t\twhile (i < active_nr && !strcmp(ce->name,\n \t\t\t\t\t\t\tactive_cache[i]->name))\n \t\t\t\ti++;\ndiff --git a/t/t3507-cherry-pick-conflict.sh b/t/t3507-cherry-pick-conflict.sh\nindex 223b984..7c5ad08 100755\n--- a/t/t3507-cherry-pick-conflict.sh\n+++ b/t/t3507-cherry-pick-conflict.sh\n@@ -351,19 +351,45 @@ test_expect_success 'commit after failed cherry-pick does not add duplicated -s'\n test_expect_success 'commit after failed cherry-pick adds -s at the right place' '\n \tpristine_detach initial &&\n \ttest_must_fail git cherry-pick picked &&\n+\n \tgit commit -a -s &&\n-\tpwd &&\n-\tcat <<EOF > expected &&\n-picked\n \n-Signed-off-by: C O Mitter <committer@example.com>\n+\t# Do S-o-b and Conflicts appear in the right order?\n+\tcat <<-\\EOF >expect &&\n+\tSigned-off-by: C O Mitter <committer@example.com>\n+\t# Conflicts:\n+\tEOF\n+\tgrep -e \"^# Conflicts:\" -e '^Signed-off-by' <.git/COMMIT_EDITMSG >actual &&\n+\ttest_cmp expect actual &&\n+\n+\tcat <<-\\EOF >expected &&\n+\tpicked\n \n-Conflicts:\n-\tfoo\n-EOF\n+\tSigned-off-by: C O Mitter <committer@example.com>\n+\tEOF\n \n-\tgit show -s --pretty=format:%B > actual &&\n+\tgit show -s --pretty=format:%B >actual &&\n \ttest_cmp expected actual\n '\n \n+test_expect_success 'commit --amend -s places the sign-off at the right place' '\n+\tpristine_detach initial &&\n+\ttest_must_fail git cherry-pick picked &&\n+\n+\t# emulate old-style conflicts block\n+\tmv .git/MERGE_MSG .git/MERGE_MSG+ &&\n+\tsed -e \"/^# Conflicts:/,\\$s/^# *//\" <.git/MERGE_MSG+ >.git/MERGE_MSG &&\n+\n+\tgit commit -a &&\n+\tgit commit --amend -s &&\n+\n+\t# Do S-o-b and Conflicts appear in the right order?\n+\tcat <<-\\EOF >expect &&\n+\tSigned-off-by: C O Mitter <committer@example.com>\n+\tConflicts:\n+\tEOF\n+\tgrep -e \"^Conflicts:\" -e '^Signed-off-by' <.git/COMMIT_EDITMSG >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.1.2-620-g33c52cb\n"}]}