# [PATCH] builtin/merge.c: drop a parameter that is never used 11 messages from 2014-10-24 to 2014-10-28. Participants: Junio C Hamano, Jonathan Nieder, Jeff King, Christian Couder. Thread: https://gitlist.dev/t/37802 ## Junio C Hamano, 2014-10-24 18:27 Subject: [PATCH] builtin/merge.c: drop a parameter that is never used Message-ID: URL: https://gitlist.dev/e/xmqq1tpxgw6t.fsf%40gitster.dls.corp.google.com ``` Since the very beginning when we added the "renormalizing" parameter to this function with 7610fa57 (merge-recursive --renormalize, 2010-08-05), nobody seems to have ever referenced it. Signed-off-by: Junio C Hamano --- * ... or is there any "renormalization" the said commit meant to but forgot to do? builtin/merge.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/builtin/merge.c b/builtin/merge.c index 41fb66d..f6894c7 100644 --- a/builtin/merge.c +++ b/builtin/merge.c @@ -884,7 +884,7 @@ static int finish_automerge(struct commit *head, return 0; } -static int suggest_conflicts(int renormalizing) +static int suggest_conflicts(void) { const char *filename; FILE *fp; @@ -1557,7 +1557,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix) fprintf(stderr, _("Automatic merge went well; " "stopped before committing as requested\n")); else - ret = suggest_conflicts(option_renormalize); + ret = suggest_conflicts(); done: free(branch_to_free); -- 2.1.2-595-g1568c45 ``` ## Jonathan Nieder, 2014-10-24 19:37 Subject: Re: [PATCH] builtin/merge.c: drop a parameter that is never used Message-ID: <20141024193723.GF5500@google.com> URL: https://gitlist.dev/e/20141024193723.GF5500%40google.com In-Reply-To: ``` Junio C Hamano wrote: > Since the very beginning when we added the "renormalizing" parameter > to this function with 7610fa57 (merge-recursive --renormalize, > 2010-08-05), nobody seems to have ever referenced it. > > Signed-off-by: Junio C Hamano Reviewed-by: Jonathan Nieder > * ... or is there any "renormalization" the said commit meant to > but forgot to do? I suspect it's related to this TODO from rerere.c::handle_cache: /* * NEEDSWORK: handle conflicts from merges with * merge.renormalize set, too */ ll_merge(&result, path, &mmfile[0], NULL, &mmfile[1], "ours", &mmfile[2], "theirs", NULL); But if someone has time for it, rather than plumbing in a useless parameter that goes nowhere, it would be better to add tests as a reminder of what is unimplemented. :) Thanks, Jonathan ``` ## Junio C Hamano, 2014-10-24 21:22 Subject: [PATCH] merge & sequencer: unify codepaths that write "Conflicts:" hint Message-ID: URL: https://gitlist.dev/e/xmqqvbn9f9ig.fsf%40gitster.dls.corp.google.com In-Reply-To: ``` Two identical loops in suggest_conflicts() in merge, and do_recursive_merge() in sequencer, can use a single helper function extracted from the latter that prepares the "Conflicts:" hint that is meant to remind the user the paths for which merge conflicts had to be resolved to write a better commit log message. Signed-off-by: Junio C Hamano --- builtin/merge.c | 18 +++++------------- sequencer.c | 35 ++++++++++++++++++++--------------- sequencer.h | 1 + 3 files changed, 26 insertions(+), 28 deletions(-) diff --git a/builtin/merge.c b/builtin/merge.c index f6894c7..d30cb60 100644 --- a/builtin/merge.c +++ b/builtin/merge.c @@ -28,6 +28,7 @@ #include "remote.h" #include "fmt-merge-msg.h" #include "gpg-interface.h" +#include "sequencer.h" #define DEFAULT_TWOHEAD (1<<0) #define DEFAULT_OCTOPUS (1<<1) @@ -888,24 +889,15 @@ static int suggest_conflicts(void) { const char *filename; FILE *fp; - int pos; + struct strbuf msgbuf = STRBUF_INIT; filename = git_path("MERGE_MSG"); fp = fopen(filename, "a"); if (!fp) die_errno(_("Could not open '%s' for writing"), filename); - fprintf(fp, "\nConflicts:\n"); - for (pos = 0; pos < active_nr; pos++) { - const struct cache_entry *ce = active_cache[pos]; - - if (ce_stage(ce)) { - fprintf(fp, "\t%s\n", ce->name); - while (pos + 1 < active_nr && - !strcmp(ce->name, - active_cache[pos + 1]->name)) - pos++; - } - } + + append_conflicts_hint(&msgbuf); + fputs(msgbuf.buf, fp); fclose(fp); rerere(allow_rerere_auto); printf(_("Automatic merge failed; " diff --git a/sequencer.c b/sequencer.c index 06e52b4..0f84bbe 100644 --- a/sequencer.c +++ b/sequencer.c @@ -287,6 +287,24 @@ static int fast_forward_to(const unsigned char *to, const unsigned char *from, return ret; } +void append_conflicts_hint(struct strbuf *msgbuf) +{ + int i; + + strbuf_addstr(msgbuf, "\nConflicts:\n"); + for (i = 0; i < active_nr;) { + const struct cache_entry *ce = active_cache[i++]; + if (ce_stage(ce)) { + strbuf_addch(msgbuf, '\t'); + strbuf_addstr(msgbuf, ce->name); + strbuf_addch(msgbuf, '\n'); + while (i < active_nr && !strcmp(ce->name, + active_cache[i]->name)) + i++; + } + } +} + static int do_recursive_merge(struct commit *base, struct commit *next, const char *base_label, const char *next_label, unsigned char *head, struct strbuf *msgbuf, @@ -328,21 +346,8 @@ static int do_recursive_merge(struct commit *base, struct commit *next, if (opts->signoff) append_signoff(msgbuf, 0, 0); - if (!clean) { - int i; - strbuf_addstr(msgbuf, "\nConflicts:\n"); - for (i = 0; i < active_nr;) { - const struct cache_entry *ce = active_cache[i++]; - if (ce_stage(ce)) { - strbuf_addch(msgbuf, '\t'); - strbuf_addstr(msgbuf, ce->name); - strbuf_addch(msgbuf, '\n'); - while (i < active_nr && !strcmp(ce->name, - active_cache[i]->name)) - i++; - } - } - } + if (!clean) + append_conflicts_hint(msgbuf); return !clean; } diff --git a/sequencer.h b/sequencer.h index 1fc22dc..c53519d 100644 --- a/sequencer.h +++ b/sequencer.h @@ -51,5 +51,6 @@ int sequencer_pick_revisions(struct replay_opts *opts); extern const char sign_off_header[]; void append_signoff(struct strbuf *msgbuf, int ignore_footer, unsigned flag); +void append_conflicts_hint(struct strbuf *msgbuf); #endif -- 2.1.2-595-g1568c45 ``` ## Junio C Hamano, 2014-10-24 21:24 Subject: [PATCH] merge & sequencer: turn "Conflicts:" hint into a comment Message-ID: URL: https://gitlist.dev/e/xmqqr3xxf9ey.fsf_-_%40gitster.dls.corp.google.com In-Reply-To: ``` Just like other hints such as "Change to be committed" we show in the editor to remind the committer what paths were involved in the resulting commit to improve their log message, this section is merely a reminder. Traditionally, it was not made into comments primarily because it has to be generated outside wt-status infrastructure, and secondary it was meant as a bit stronger reminder than the rest (i.e. explaining how you resolved conflicts is much more important than mentioning what you did to every paths involved in the commit), but that still does not make this hint a hint, which should be commented out by default. Signed-off-by: Junio C Hamano --- builtin/commit.c | 42 ++++++++++++++++++++--------------------- sequencer.c | 7 +++---- t/t3507-cherry-pick-conflict.sh | 22 +++++++++++++-------- 3 files changed, 38 insertions(+), 33 deletions(-) diff --git a/builtin/commit.c b/builtin/commit.c index fedb45a..3a1d1a8 100644 --- a/builtin/commit.c +++ b/builtin/commit.c @@ -719,30 +719,30 @@ static int prepare_to_commit(const char *index_file, const char *prefix, stripspace(&sb, 0); if (signoff) { - /* - * See if we have a Conflicts: block at the end. If yes, count - * its size, so we can ignore it. - */ - int ignore_footer = 0; - int i, eol, previous = 0; - const char *nl; - - for (i = 0; i < sb.len; i++) { - nl = memchr(sb.buf + i, '\n', sb.len - i); - if (nl) - eol = nl - sb.buf; + /* Ignore comments and blanks after the trailer */ + int boc = 0; + int bol = 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 - eol = sb.len; - if (!prefixcmp(sb.buf + previous, "\nConflicts:\n")) { - ignore_footer = sb.len - previous; - break; + 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 (boc) { + /* the previous was not trailing comment */ + boc = 0; } - while (i < eol) - i++; - previous = eol; + bol = next_line - sb.buf; } - - append_signoff(&sb, ignore_footer, 0); + append_signoff(&sb, boc ? sb.len - boc : 0, 0); } if (fwrite(sb.buf, 1, sb.len, s->fp) < sb.len) diff --git a/sequencer.c b/sequencer.c index 0f84bbe..1d97da3 100644 --- a/sequencer.c +++ b/sequencer.c @@ -291,13 +291,12 @@ void append_conflicts_hint(struct strbuf *msgbuf) { int i; - strbuf_addstr(msgbuf, "\nConflicts:\n"); + strbuf_addch(msgbuf, '\n'); + strbuf_commented_addf(msgbuf, "Conflicts:\n"); for (i = 0; i < active_nr;) { const struct cache_entry *ce = active_cache[i++]; if (ce_stage(ce)) { - strbuf_addch(msgbuf, '\t'); - strbuf_addstr(msgbuf, ce->name); - strbuf_addch(msgbuf, '\n'); + strbuf_commented_addf(msgbuf, "\t%s\n", ce->name); while (i < active_nr && !strcmp(ce->name, active_cache[i]->name)) i++; diff --git a/t/t3507-cherry-pick-conflict.sh b/t/t3507-cherry-pick-conflict.sh index 223b984..fa04226 100755 --- a/t/t3507-cherry-pick-conflict.sh +++ b/t/t3507-cherry-pick-conflict.sh @@ -351,18 +351,24 @@ test_expect_success 'commit after failed cherry-pick does not add duplicated -s' test_expect_success 'commit after failed cherry-pick adds -s at the right place' ' pristine_detach initial && test_must_fail git cherry-pick picked && + git commit -a -s && - pwd && - cat < expected && -picked -Signed-off-by: C O Mitter + # Do S-o-b and Conflicts appear in the right order? + cat <<-\EOF >expect && + Signed-off-by: C O Mitter + # Conflicts: + EOF + grep -e "^# Conflicts:" -e '^Signed-off-by' <.git/COMMIT_EDITMSG >actual && + test_cmp expect actual && + + cat <<-\EOF >expected && + picked -Conflicts: - foo -EOF + Signed-off-by: C O Mitter + EOF - git show -s --pretty=format:%B > actual && + git show -s --pretty=format:%B >actual && test_cmp expected actual ' -- 2.1.2-595-g1568c45 ``` ## Jeff King, 2014-10-26 18:59 Subject: Re: [PATCH] merge & sequencer: turn "Conflicts:" hint into a comment Message-ID: <20141026185909.GC18144@peff.net> URL: https://gitlist.dev/e/20141026185909.GC18144%40peff.net In-Reply-To: ``` On Fri, Oct 24, 2014 at 02:24:37PM -0700, Junio C Hamano wrote: > Just like other hints such as "Change to be committed" we show in > the editor to remind the committer what paths were involved in the > resulting commit to improve their log message, this section is > merely a reminder. Traditionally, it was not made into comments > primarily because it has to be generated outside wt-status > infrastructure, and secondary it was meant as a bit stronger > reminder than the rest (i.e. explaining how you resolved conflicts > is much more important than mentioning what you did to every paths > involved in the commit), but that still does not make this hint a > hint, which should be commented out by default. Yay. I like this new behavior much better. Just to play devil's advocate for a moment, though, are we hurting people who find it useful to record that information in the commit message? For the most part, combined-diff (and --cc) will show the interesting cases anyway. But if you take a whole file from one side of the merge, then there is nothing interesting for diff to show. Do people still want to get that more complete list of potentially interesting files? And if so, how do they do it? I think there really isn't a great way besides repeating the merge. If that is the only casualty, I think it is probably a net-win. We may want better tooling around viewing the merge later, but that can wait until somebody steps up with a real use case (because even that conflict list may not be completely what they want; they may also want the list of files that were auto-merged successfully, for example). And I think there was work recently on a diff view for merge commits that involved recreating the merge (I do not remember the details, though). > diff --git a/sequencer.c b/sequencer.c > index 0f84bbe..1d97da3 100644 > --- a/sequencer.c > +++ b/sequencer.c > @@ -291,13 +291,12 @@ void append_conflicts_hint(struct strbuf *msgbuf) > { > int i; > > - strbuf_addstr(msgbuf, "\nConflicts:\n"); > + strbuf_addch(msgbuf, '\n'); > + strbuf_commented_addf(msgbuf, "Conflicts:\n"); > for (i = 0; i < active_nr;) { > const struct cache_entry *ce = active_cache[i++]; > if (ce_stage(ce)) { > - strbuf_addch(msgbuf, '\t'); > - strbuf_addstr(msgbuf, ce->name); > - strbuf_addch(msgbuf, '\n'); > + strbuf_commented_addf(msgbuf, "\t%s\n", ce->name); This ends up adding a space followed by a tab. Besides being redundant, it makes my editor highlight it as a whitespace error. I realize this is a pretty minor nit, though. -Peff ``` ## Junio C Hamano, 2014-10-27 17:32 Subject: Re: [PATCH] merge & sequencer: turn "Conflicts:" hint into a comment Message-ID: URL: https://gitlist.dev/e/xmqqegttfmg0.fsf%40gitster.dls.corp.google.com In-Reply-To: <20141026185909.GC18144@peff.net> ``` Jeff King writes: > Just to play devil's advocate for a moment, though, are we hurting > people who find it useful to record that information in the commit > message? I thought about it, but ultimately, they are using it wrong if they did depend on them. As you said, the paths themselves are not that interesting, unless they are accompanied by description in the log message explaining what caused conflicts and how they were resolved at the semantic level. The hints are to remind them what conflicts in which paths to describe. I do not mind "merge.commentConflictsHint = no" as a backward compatibility toggle, if somebody cares deeply about it, though. > If that is the only casualty, I think it is probably a net-win. We may > want better tooling around viewing the merge later, but that can wait > until somebody steps up with a real use case (because even that conflict > list may not be completely what they want; they may also want the list > of files that were auto-merged successfully, for example). Yup. Also Christian's "trailer" series may want to learn the same trick we did to builtin/commit.c in this series, if it does not already know about possible trailing comment and blank lines. >> diff --git a/sequencer.c b/sequencer.c >> index 0f84bbe..1d97da3 100644 >> --- a/sequencer.c >> +++ b/sequencer.c >> @@ -291,13 +291,12 @@ void append_conflicts_hint(struct strbuf *msgbuf) >> { >> int i; >> >> - strbuf_addstr(msgbuf, "\nConflicts:\n"); >> + strbuf_addch(msgbuf, '\n'); >> + strbuf_commented_addf(msgbuf, "Conflicts:\n"); >> for (i = 0; i < active_nr;) { >> const struct cache_entry *ce = active_cache[i++]; >> if (ce_stage(ce)) { >> - strbuf_addch(msgbuf, '\t'); >> - strbuf_addstr(msgbuf, ce->name); >> - strbuf_addch(msgbuf, '\n'); >> + strbuf_commented_addf(msgbuf, "\t%s\n", ce->name); > > This ends up adding a space followed by a tab. Besides being redundant, > it makes my editor highlight it as a whitespace error. I realize this is > a pretty minor nit, though. Interesting ;-) I do not think it is too hard to teach strbuf_commented_addf() about the leading HT, but that would be a separate topic; if squashing the SP-HT to HT is worth doing for this codepath, doing it at that helper would benefit all callers. ``` ## Junio C Hamano, 2014-10-27 20:59 Subject: Re: [PATCH] merge & sequencer: turn "Conflicts:" hint into a comment Message-ID: URL: https://gitlist.dev/e/xmqqsii9qleh.fsf%40gitster.dls.corp.google.com In-Reply-To: ``` Junio C Hamano writes: >>> diff --git a/sequencer.c b/sequencer.c >>> index 0f84bbe..1d97da3 100644 >>> --- a/sequencer.c >>> +++ b/sequencer.c >>> @@ -291,13 +291,12 @@ void append_conflicts_hint(struct strbuf *msgbuf) >>> { >>> int i; >>> >>> - strbuf_addstr(msgbuf, "\nConflicts:\n"); >>> + strbuf_addch(msgbuf, '\n'); >>> + strbuf_commented_addf(msgbuf, "Conflicts:\n"); >>> for (i = 0; i < active_nr;) { >>> const struct cache_entry *ce = active_cache[i++]; >>> if (ce_stage(ce)) { >>> - strbuf_addch(msgbuf, '\t'); >>> - strbuf_addstr(msgbuf, ce->name); >>> - strbuf_addch(msgbuf, '\n'); >>> + strbuf_commented_addf(msgbuf, "\t%s\n", ce->name); >> >> This ends up adding a space followed by a tab. Besides being redundant, >> it makes my editor highlight it as a whitespace error. I realize this is >> a pretty minor nit, though. > > Interesting ;-) > > I do not think it is too hard to teach strbuf_commented_addf() about > the leading HT, but that would be a separate topic; if squashing the > SP-HT to HT is worth doing for this codepath, doing it at that helper > would benefit all callers. -- >8 -- Subject: [PATCH] strbuf_add_lines(): avoid SP-HT sequence The strbuf_add_commented_lines() function passes a pair of prefixes, one for a line that has some meat on it, and the other for an empty line. The former is set to a comment char followed by a SP, while the latter is set to just the comment char. This is to give a SP after the comment character, e.g. "# \n" and to avoid emitting an unsightly "# \n" in its output. Teach the machinery to also use the latter space-less prefix when the payload line begins with a tab; otherwise we will end up showing "# \t\n" which is similarly unsightly. Signed-off-by: Junio C Hamano --- strbuf.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/strbuf.c b/strbuf.c index 0346e74..88cafd4 100644 --- a/strbuf.c +++ b/strbuf.c @@ -229,7 +229,8 @@ static void add_lines(struct strbuf *out, const char *next = memchr(buf, '\n', size); next = next ? (next + 1) : (buf + size); - prefix = (prefix2 && buf[0] == '\n') ? prefix2 : prefix1; + prefix = ((prefix2 && (buf[0] == '\n' || buf[0] == '\t')) + ? prefix2 : prefix1); strbuf_addstr(out, prefix); strbuf_add(out, buf, next - buf); size -= next - buf; ``` ## Jonathan Nieder, 2014-10-27 21:14 Subject: Re: [PATCH] merge & sequencer: turn "Conflicts:" hint into a comment Message-ID: <20141027211441.GH5500@google.com> URL: https://gitlist.dev/e/20141027211441.GH5500%40google.com In-Reply-To: <20141026185909.GC18144@peff.net> ``` Jeff King wrote: > For the most part, combined-diff (and --cc) will show the interesting > cases anyway. But if you take a whole file from one side of the merge, > then there is nothing interesting for diff to show. Do people still want > to get that more complete list of potentially interesting files? And if > so, how do they do it? I think there really isn't a great way besides > repeating the merge. If you have time to experiment with tr/remerge-diff from pu[1], that would be welcome. Maybe some day it can be the default behavior for 'log -p'. > If that is the only casualty, I think it is probably a net-win. Yes. Thanks, Jonathan [1] http://thread.gmane.org/gmane.comp.version-control.git/256591 ``` ## Christian Couder, 2014-10-28 06:51 Subject: Re: [PATCH] merge & sequencer: turn "Conflicts:" hint into a comment Message-ID: URL: https://gitlist.dev/e/CAP8UFD28mXm2nKRbah%3DNv%2B2Tcuvvax%2BNdHdp7niiekbfk-i%2B9w%40mail.gmail.com In-Reply-To: ``` On Mon, Oct 27, 2014 at 6:32 PM, Junio C Hamano wrote: > Jeff King writes: > >> If that is the only casualty, I think it is probably a net-win. We may >> want better tooling around viewing the merge later, but that can wait >> until somebody steps up with a real use case (because even that conflict >> list may not be completely what they want; they may also want the list >> of files that were auto-merged successfully, for example). > > Yup. > > Also Christian's "trailer" series may want to learn the same trick > we did to builtin/commit.c in this series, if it does not already > know about possible trailing comment and blank lines. The trailer series already tries to ignore comments and blank lines. This is the relevant function: /* * Return the (0 based) index of the first trailer line or count if * there are no trailers. Trailers are searched only in the lines from * index (count - 1) down to index 0. */ static int find_trailer_start(struct strbuf **lines, int count) { int start, only_spaces = 1; /* * Get the start of the trailers by looking starting from the end * for a line with only spaces before lines with one separator. */ for (start = count - 1; start >= 0; start--) { if (lines[start]->buf[0] == comment_line_char) continue; if (contains_only_spaces(lines[start]->buf)) { if (only_spaces) continue; return start + 1; } if (strcspn(lines[start]->buf, separators) < lines[start]->len) { if (only_spaces) only_spaces = 0; continue; } return count; } return only_spaces ? count : 0; } But I am not sure sure that it does all of what you do to builtin/commit.c in the above patch. I will have a look. Anyway I would be happy to use an existing function or to refactor some existing code into a shared function if possible. Thanks, Christian. ``` ## Jeff King, 2014-10-28 22:21 Subject: Re: [PATCH] merge & sequencer: turn "Conflicts:" hint into a comment Message-ID: <20141028222108.GB20722@peff.net> URL: https://gitlist.dev/e/20141028222108.GB20722%40peff.net In-Reply-To: ``` On Mon, Oct 27, 2014 at 01:59:18PM -0700, Junio C Hamano wrote: > > I do not think it is too hard to teach strbuf_commented_addf() about > > the leading HT, but that would be a separate topic; if squashing the > > SP-HT to HT is worth doing for this codepath, doing it at that helper > > would benefit all callers. > > -- >8 -- > Subject: [PATCH] strbuf_add_lines(): avoid SP-HT sequence > [...] Thanks for doing this. I agree that this is the right place to put the magic, and the patch looks obviously correct. I also double-checked it with the "# Conflicts" patches and it addresses the problem I saw. -Peff ``` ## Jeff King, 2014-10-28 22:22 Subject: Re: [PATCH] merge & sequencer: turn "Conflicts:" hint into a comment Message-ID: <20141028222238.GC20722@peff.net> URL: https://gitlist.dev/e/20141028222238.GC20722%40peff.net In-Reply-To: <20141027211441.GH5500@google.com> ``` On Mon, Oct 27, 2014 at 02:14:42PM -0700, Jonathan Nieder wrote: > Jeff King wrote: > > > For the most part, combined-diff (and --cc) will show the interesting > > cases anyway. But if you take a whole file from one side of the merge, > > then there is nothing interesting for diff to show. Do people still want > > to get that more complete list of potentially interesting files? And if > > so, how do they do it? I think there really isn't a great way besides > > repeating the merge. > > If you have time to experiment with tr/remerge-diff from pu[1], that > would be welcome. Thanks, that was the topic I was thinking of. It's not very often that I want to carefully investigate merge commits (usually it is when I am trying to help somebody track the addition or deletion of content that came as part of an evil merge), but I'll give it a try next time it comes up. -Peff ```