{"thread":{"id":"37802","subject":"[PATCH] builtin/merge.c: drop a parameter that is never used","startedAt":"2014-10-24T18:27:22Z","lastAt":"2014-10-28T22:22:38Z","messageCount":11,"participants":["Junio C Hamano","Jonathan Nieder","Jeff King","Christian Couder"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"251025","messageId":"xmqq1tpxgw6t.fsf@gitster.dls.corp.google.com","threadId":"37802","inReplyTo":null,"subject":"[PATCH] builtin/merge.c: drop a parameter that is never used","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-10-24T18:27:22Z","receivedAt":"2014-10-24T18:27:22Z","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\n * ... or is there any \"renormalization\" the said commit meant to\n   but forgot to do?\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-595-g1568c45\n"},{"id":"251026","messageId":"20141024193723.GF5500@google.com","threadId":"37802","inReplyTo":"xmqq1tpxgw6t.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] builtin/merge.c: drop a parameter that is never used","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-10-24T19:37:23Z","receivedAt":"2014-10-24T19:37:23Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Junio C Hamano wrote:\n\n> Since the very beginning when we added the \"renormalizing\" parameter\n> to this function with 7610fa57 (merge-recursive --renormalize,\n> 2010-08-05), nobody seems to have ever referenced it.\n>\n> Signed-off-by: Junio C Hamano <gitster@pobox.com>\n\nReviewed-by: Jonathan Nieder <jrnieder@gmail.com>\n\n>  * ... or is there any \"renormalization\" the said commit meant to\n>    but forgot to do?\n\nI suspect it's related to this TODO from rerere.c::handle_cache:\n\n\t/*\n\t * NEEDSWORK: handle conflicts from merges with\n\t * merge.renormalize set, too\n\t */\n\tll_merge(&result, path, &mmfile[0], NULL,\n\t\t &mmfile[1], \"ours\",\n\t\t &mmfile[2], \"theirs\", NULL);\n\nBut if someone has time for it, rather than plumbing in a useless\nparameter that goes nowhere, it would be better to add tests as a\nreminder of what is unimplemented. :)\n\nThanks,\nJonathan\n"},{"id":"251027","messageId":"xmqqvbn9f9ig.fsf@gitster.dls.corp.google.com","threadId":"37802","inReplyTo":"xmqq1tpxgw6t.fsf@gitster.dls.corp.google.com","subject":"[PATCH] merge & sequencer: unify codepaths that write \"Conflicts:\" hint","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-10-24T21:22:31Z","receivedAt":"2014-10-24T21:22:31Z","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\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-595-g1568c45\n"},{"id":"251028","messageId":"xmqqr3xxf9ey.fsf_-_@gitster.dls.corp.google.com","threadId":"37802","inReplyTo":"xmqqvbn9f9ig.fsf@gitster.dls.corp.google.com","subject":"[PATCH] merge & sequencer: turn \"Conflicts:\" hint into a comment","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-10-24T21:24:37Z","receivedAt":"2014-10-24T21:24:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Just like other hints such as \"Change to be committed\" we show in\nthe editor to remind the committer what paths were involved in the\nresulting commit to improve their log message, this section is\nmerely a reminder.  Traditionally, it was not made into comments\nprimarily because it has to be generated outside wt-status\ninfrastructure, and secondary it was meant as a bit stronger\nreminder than the rest (i.e. explaining how you resolved conflicts\nis much more important than mentioning what you did to every paths\ninvolved in the commit), but that still does not make this hint a\nhint, which should be commented out by default.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n builtin/commit.c                | 42 ++++++++++++++++++++---------------------\n sequencer.c                     |  7 +++----\n t/t3507-cherry-pick-conflict.sh | 22 +++++++++++++--------\n 3 files changed, 38 insertions(+), 33 deletions(-)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex fedb45a..3a1d1a8 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -719,30 +719,30 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\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/* Ignore comments and blanks after the trailer */\n+\t\tint boc = 0;\n+\t\tint bol = 0;\n+\n+\t\twhile (bol < sb.len) {\n+\t\t\tchar *next_line;\n+\n+\t\t\tif (!(next_line = memchr(sb.buf + bol, '\\n', sb.len - bol)))\n+\t\t\t\tnext_line = sb.buf + sb.len;\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\tnext_line++;\n+\n+\t\t\tif (sb.buf[bol] == comment_line_char || sb.buf[bol] == '\\n') {\n+\t\t\t\t/* is this the first of the run of comments? */\n+\t\t\t\tif (!boc)\n+\t\t\t\t\tboc = bol;\n+\t\t\t\t/* otherwise, it is just continuing */\n+\t\t\t} else if (boc) {\n+\t\t\t\t/* the previous was not trailing comment */\n+\t\t\t\tboc = 0;\n \t\t\t}\n-\t\t\twhile (i < eol)\n-\t\t\t\ti++;\n-\t\t\tprevious = eol;\n+\t\t\tbol = next_line - sb.buf;\n \t\t}\n-\n-\t\tappend_signoff(&sb, ignore_footer, 0);\n+\t\tappend_signoff(&sb, boc ? sb.len - boc : 0, 0);\n \t}\n \n \tif (fwrite(sb.buf, 1, sb.len, s->fp) < sb.len)\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..fa04226 100755\n--- a/t/t3507-cherry-pick-conflict.sh\n+++ b/t/t3507-cherry-pick-conflict.sh\n@@ -351,18 +351,24 @@ 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-- \n2.1.2-595-g1568c45\n"},{"id":"251071","messageId":"20141026185909.GC18144@peff.net","threadId":"37802","inReplyTo":"xmqqr3xxf9ey.fsf_-_@gitster.dls.corp.google.com","subject":"Re: [PATCH] merge & sequencer: turn \"Conflicts:\" hint into a comment","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-10-26T18:59:10Z","receivedAt":"2014-10-26T18:59:10Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Oct 24, 2014 at 02:24:37PM -0700, Junio C Hamano wrote:\n\n> Just like other hints such as \"Change to be committed\" we show in\n> the editor to remind the committer what paths were involved in the\n> resulting commit to improve their log message, this section is\n> merely a reminder.  Traditionally, it was not made into comments\n> primarily because it has to be generated outside wt-status\n> infrastructure, and secondary it was meant as a bit stronger\n> reminder than the rest (i.e. explaining how you resolved conflicts\n> is much more important than mentioning what you did to every paths\n> involved in the commit), but that still does not make this hint a\n> hint, which should be commented out by default.\n\nYay. I like this new behavior much better.\n\nJust to play devil's advocate for a moment, though, are we hurting\npeople who find it useful to record that information in the commit\nmessage?\n\nFor the most part, combined-diff (and --cc) will show the interesting\ncases anyway. But if you take a whole file from one side of the merge,\nthen there is nothing interesting for diff to show. Do people still want\nto get that more complete list of potentially interesting files? And if\nso, how do they do it?  I think there really isn't a great way besides\nrepeating the merge.\n\nIf that is the only casualty, I think it is probably a net-win. We may\nwant better tooling around viewing the merge later, but that can wait\nuntil somebody steps up with a real use case (because even that conflict\nlist may not be completely what they want; they may also want the list\nof files that were auto-merged successfully, for example). And I think\nthere was work recently on a diff view for merge commits that involved\nrecreating the merge (I do not remember the details, though).\n\n> diff --git a/sequencer.c b/sequencer.c\n> index 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\nThis ends up adding a space followed by a tab. Besides being redundant,\nit makes my editor highlight it as a whitespace error. I realize this is\na pretty minor nit, though.\n\n-Peff\n"},{"id":"251121","messageId":"xmqqegttfmg0.fsf@gitster.dls.corp.google.com","threadId":"37802","inReplyTo":"20141026185909.GC18144@peff.net","subject":"Re: [PATCH] merge & sequencer: turn \"Conflicts:\" hint into a comment","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-10-27T17:32:15Z","receivedAt":"2014-10-27T17:32:15Z","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> Just to play devil's advocate for a moment, though, are we hurting\n> people who find it useful to record that information in the commit\n> message?\n\nI thought about it, but ultimately, they are using it wrong if they\ndid depend on them.  As you said, the paths themselves are not that\ninteresting, unless they are accompanied by description in the log\nmessage explaining what caused conflicts and how they were resolved\nat the semantic level.  The hints are to remind them what conflicts\nin which paths to describe.\n\nI do not mind \"merge.commentConflictsHint = no\" as a backward\ncompatibility toggle, if somebody cares deeply about it, though.\n\n> If that is the only casualty, I think it is probably a net-win. We may\n> want better tooling around viewing the merge later, but that can wait\n> until somebody steps up with a real use case (because even that conflict\n> list may not be completely what they want; they may also want the list\n> of files that were auto-merged successfully, for example).\n\nYup.\n\nAlso Christian's \"trailer\" series may want to learn the same trick\nwe did to builtin/commit.c in this series, if it does not already\nknow about possible trailing comment and blank lines.\n\n>> diff --git a/sequencer.c b/sequencer.c\n>> index 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>\n> This ends up adding a space followed by a tab. Besides being redundant,\n> it makes my editor highlight it as a whitespace error. I realize this is\n> a pretty minor nit, though.\n\nInteresting ;-)\n\nI do not think it is too hard to teach strbuf_commented_addf() about\nthe leading HT, but that would be a separate topic; if squashing the\nSP-HT to HT is worth doing for this codepath, doing it at that helper\nwould benefit all callers.\n"},{"id":"251132","messageId":"xmqqsii9qleh.fsf@gitster.dls.corp.google.com","threadId":"37802","inReplyTo":"xmqqegttfmg0.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] merge & sequencer: turn \"Conflicts:\" hint into a comment","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-10-27T20:59:18Z","receivedAt":"2014-10-27T20:59:18Z","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>>> diff --git a/sequencer.c b/sequencer.c\n>>> index 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>>\n>> This ends up adding a space followed by a tab. Besides being redundant,\n>> it makes my editor highlight it as a whitespace error. I realize this is\n>> a pretty minor nit, though.\n>\n> Interesting ;-)\n>\n> I do not think it is too hard to teach strbuf_commented_addf() about\n> the leading HT, but that would be a separate topic; if squashing the\n> SP-HT to HT is worth doing for this codepath, doing it at that helper\n> would benefit all callers.\n\n-- >8 --\nSubject: [PATCH] strbuf_add_lines(): avoid SP-HT sequence\n\nThe strbuf_add_commented_lines() function passes a pair of prefixes,\none for a line that has some meat on it, and the other for an empty\nline.  The former is set to a comment char followed by a SP, while\nthe latter is set to just the comment char.  This is to give a SP\nafter the comment character, e.g. \"# <user text>\\n\" and to avoid\nemitting an unsightly \"# \\n\" in its output.\n\nTeach the machinery to also use the latter space-less prefix when\nthe payload line begins with a tab; otherwise we will end up showing\n\"# \\t<user text>\\n\" which is similarly unsightly.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n strbuf.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/strbuf.c b/strbuf.c\nindex 0346e74..88cafd4 100644\n--- a/strbuf.c\n+++ b/strbuf.c\n@@ -229,7 +229,8 @@ static void add_lines(struct strbuf *out,\n \t\tconst char *next = memchr(buf, '\\n', size);\n \t\tnext = next ? (next + 1) : (buf + size);\n \n-\t\tprefix = (prefix2 && buf[0] == '\\n') ? prefix2 : prefix1;\n+\t\tprefix = ((prefix2 && (buf[0] == '\\n' || buf[0] == '\\t'))\n+\t\t\t  ? prefix2 : prefix1);\n \t\tstrbuf_addstr(out, prefix);\n \t\tstrbuf_add(out, buf, next - buf);\n \t\tsize -= next - buf;\n"},{"id":"251133","messageId":"20141027211441.GH5500@google.com","threadId":"37802","inReplyTo":"20141026185909.GC18144@peff.net","subject":"Re: [PATCH] merge & sequencer: turn \"Conflicts:\" hint into a comment","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-10-27T21:14:42Z","receivedAt":"2014-10-27T21:14:42Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n\n> For the most part, combined-diff (and --cc) will show the interesting\n> cases anyway. But if you take a whole file from one side of the merge,\n> then there is nothing interesting for diff to show. Do people still want\n> to get that more complete list of potentially interesting files? And if\n> so, how do they do it?  I think there really isn't a great way besides\n> repeating the merge.\n\nIf you have time to experiment with tr/remerge-diff from pu[1], that\nwould be welcome.\n\nMaybe some day it can be the default behavior for 'log -p'.\n\n> If that is the only casualty, I think it is probably a net-win.\n\nYes.\n\nThanks,\nJonathan\n\n[1] http://thread.gmane.org/gmane.comp.version-control.git/256591\n"},{"id":"251141","messageId":"CAP8UFD28mXm2nKRbah=Nv+2Tcuvvax+NdHdp7niiekbfk-i+9w@mail.gmail.com","threadId":"37802","inReplyTo":"xmqqegttfmg0.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] merge & sequencer: turn \"Conflicts:\" hint into a comment","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2014-10-28T06:51:20Z","receivedAt":"2014-10-28T06:51:20Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Mon, Oct 27, 2014 at 6:32 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Jeff King <peff@peff.net> writes:\n>\n>> If that is the only casualty, I think it is probably a net-win. We may\n>> want better tooling around viewing the merge later, but that can wait\n>> until somebody steps up with a real use case (because even that conflict\n>> list may not be completely what they want; they may also want the list\n>> of files that were auto-merged successfully, for example).\n>\n> Yup.\n>\n> Also Christian's \"trailer\" series may want to learn the same trick\n> we did to builtin/commit.c in this series, if it does not already\n> know about possible trailing comment and blank lines.\n\nThe trailer series already tries to ignore comments and blank lines.\nThis is the relevant function:\n\n/*\n * Return the (0 based) index of the first trailer line or count if\n * there are no trailers. Trailers are searched only in the lines from\n * index (count - 1) down to index 0.\n */\nstatic int find_trailer_start(struct strbuf **lines, int count)\n{\n    int start, only_spaces = 1;\n\n    /*\n     * Get the start of the trailers by looking starting from the end\n     * for a line with only spaces before lines with one separator.\n     */\n    for (start = count - 1; start >= 0; start--) {\n        if (lines[start]->buf[0] == comment_line_char)\n            continue;\n        if (contains_only_spaces(lines[start]->buf)) {\n            if (only_spaces)\n                continue;\n            return start + 1;\n        }\n        if (strcspn(lines[start]->buf, separators) < lines[start]->len) {\n            if (only_spaces)\n                only_spaces = 0;\n            continue;\n        }\n        return count;\n    }\n\n    return only_spaces ? count : 0;\n}\n\nBut I am not sure sure that it does all of what you do to\nbuiltin/commit.c in the above patch. I will have a look.\nAnyway I would be happy to use an existing function or to refactor\nsome existing code into a shared function if possible.\n\nThanks,\nChristian.\n"},{"id":"251169","messageId":"20141028222108.GB20722@peff.net","threadId":"37802","inReplyTo":"xmqqsii9qleh.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] merge & sequencer: turn \"Conflicts:\" hint into a comment","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-10-28T22:21:08Z","receivedAt":"2014-10-28T22:21:08Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Oct 27, 2014 at 01:59:18PM -0700, Junio C Hamano wrote:\n\n> > I do not think it is too hard to teach strbuf_commented_addf() about\n> > the leading HT, but that would be a separate topic; if squashing the\n> > SP-HT to HT is worth doing for this codepath, doing it at that helper\n> > would benefit all callers.\n> \n> -- >8 --\n> Subject: [PATCH] strbuf_add_lines(): avoid SP-HT sequence\n> [...]\n\nThanks for doing this. I agree that this is the right place to put the\nmagic, and the patch looks obviously correct. I also double-checked it\nwith the \"# Conflicts\" patches and it addresses the problem I saw.\n\n-Peff\n"},{"id":"251170","messageId":"20141028222238.GC20722@peff.net","threadId":"37802","inReplyTo":"20141027211441.GH5500@google.com","subject":"Re: [PATCH] merge & sequencer: turn \"Conflicts:\" hint into a comment","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-10-28T22:22:38Z","receivedAt":"2014-10-28T22:22:38Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Oct 27, 2014 at 02:14:42PM -0700, Jonathan Nieder wrote:\n\n> Jeff King wrote:\n> \n> > For the most part, combined-diff (and --cc) will show the interesting\n> > cases anyway. But if you take a whole file from one side of the merge,\n> > then there is nothing interesting for diff to show. Do people still want\n> > to get that more complete list of potentially interesting files? And if\n> > so, how do they do it?  I think there really isn't a great way besides\n> > repeating the merge.\n> \n> If you have time to experiment with tr/remerge-diff from pu[1], that\n> would be welcome.\n\nThanks, that was the topic I was thinking of.\n\nIt's not very often that I want to carefully investigate merge commits\n(usually it is when I am trying to help somebody track the addition or\ndeletion of content that came as part of an evil merge), but I'll give\nit a try next time it comes up.\n\n-Peff\n"}]}