[PATCH] builtin/merge.c: drop a parameter that is never used

[PATCH] builtin/merge.c: drop a parameter that is never used

[Junio C Hamano]

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 <gitster@pobox.com>
---

 * ... 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

Re: [PATCH] builtin/merge.c: drop a parameter that is never used

[Jonathan Nieder]

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 <gitster@pobox.com>

Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

>  * ... 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

[PATCH] merge & sequencer: unify codepaths that write "Conflicts:" hint

[Junio C Hamano]

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 <gitster@pobox.com>
---

 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

[PATCH] merge & sequencer: turn "Conflicts:" hint into a comment

[Junio C Hamano]

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 <gitster@pobox.com>
---
 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 <<EOF > expected &&
-picked
 
-Signed-off-by: C O Mitter <committer@example.com>
+	# Do S-o-b and Conflicts appear in the right order?
+	cat <<-\EOF >expect &&
+	Signed-off-by: C O Mitter <committer@example.com>
+	# 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 <committer@example.com>
+	EOF
 
-	git show -s --pretty=format:%B > actual &&
+	git show -s --pretty=format:%B >actual &&
 	test_cmp expected actual
 '
 
-- 
2.1.2-595-g1568c45

Re: [PATCH] merge & sequencer: turn "Conflicts:" hint into a comment

[Jeff King]

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

Re: [PATCH] merge & sequencer: turn "Conflicts:" hint into a comment

[Junio C Hamano]

Jeff King <peff@peff.net> 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.

Re: [PATCH] merge & sequencer: turn "Conflicts:" hint into a comment

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> 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. "# <user text>\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<user text>\n" which is similarly unsightly.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 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;

Re: [PATCH] merge & sequencer: turn "Conflicts:" hint into a comment

[Jonathan Nieder]

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

Re: [PATCH] merge & sequencer: turn "Conflicts:" hint into a comment

[Christian Couder]

On Mon, Oct 27, 2014 at 6:32 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Jeff King <peff@peff.net> 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.

[PATCH v2 0/4] Turning Conflicts: hint into comment

[Junio C Hamano]

So here is a reroll to hopefully bring this topic closer to
perfection.

The first two patches haven't changed, except that they are now part
of a numbered series.

The third patch is a refactoring that helps clarify what happens in
the last step, which is new.

The endgame is similar to what was posted before, except that we pay
attention to "Conflicts:" and HT indented pathnames in addition to
comments and blanks when determining the true end of the log
message.  This is so that running "git commit --amend -s" on a
commit that was created with older versions of Git by a careless
user who left the old conflicts hint around will insert the new
sign-off at the right place.

The series is designed to apply on v1.8.5.x series, even though I do
not anticpiate that we would need to backport this to maintenance
branches.  !prefixcmp() will need to turn into starts_with() when
merging to newer codebase, which I can let rerere take care of.

Junio C Hamano (4):
  builtin/merge.c: drop a parameter that is never used
  merge & sequencer: unify codepaths that write "Conflicts:" hint
  builtin/commit.c: extract ignore_non_trailer() helper function
  merge & sequencer: turn "Conflicts:" hint into a comment

 builtin/commit.c                | 74 ++++++++++++++++++++++++++---------------
 builtin/merge.c                 | 22 ++++--------
 sequencer.c                     | 34 ++++++++++---------
 sequencer.h                     |  1 +
 t/t3507-cherry-pick-conflict.sh | 42 ++++++++++++++++++-----
 5 files changed, 109 insertions(+), 64 deletions(-)

-- 
2.1.2-620-g33c52cb

[PATCH v2 1/4] builtin/merge.c: drop a parameter that is never used

[Junio C Hamano]

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 <gitster@pobox.com>
---
 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-620-g33c52cb

[PATCH v2 2/4] merge & sequencer: unify codepaths that write "Conflicts:" hint

[Junio C Hamano]

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 <gitster@pobox.com>
---
 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-620-g33c52cb

[PATCH v2 3/4] builtin/commit.c: extract ignore_non_trailer() helper function

[Junio C Hamano]

Extract a helper function from prepare_to_commit() to determine
where to place a new Signed-off-by: line, which is essentially the
true "end" of the log message, ignoring the trailing "Conflicts:"
line and everything below it.

The detection _should_ make sure the "Conflicts:" line it finds is
truly the conflict hint block by checking everything that follows is
a HT indented pathname to avoid false positive, but this logic will
be revamped in a later patch to ignore comments and blanks anyway,
so it is left as-is in this step.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/commit.c | 59 +++++++++++++++++++++++++++++++-------------------------
 1 file changed, 33 insertions(+), 26 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index fedb45a..cd455aa 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -593,6 +593,37 @@ static char *cut_ident_timestamp_part(char *string)
 	return ket;
 }
 
+/*
+ * Inspect sb and determine the true "end" of the log message, in
+ * order to find where to put a new Signed-off-by: line.  Ignored are
+ * trailing "Conflict:" block.
+ *
+ * Returns the number of bytes from the tail to ignore, to be fed as
+ * the second parameter to append_signoff().
+ */
+static int ignore_non_trailer(struct strbuf *sb)
+{
+	int 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;
+		else
+			eol = sb->len;
+		if (!prefixcmp(sb->buf + previous, "\nConflicts:\n")) {
+			ignore_footer = sb->len - previous;
+			break;
+		}
+		while (i < eol)
+			i++;
+		previous = eol;
+	}
+	return ignore_footer;
+}
+
 static int prepare_to_commit(const char *index_file, const char *prefix,
 			     struct commit *current_head,
 			     struct wt_status *s,
@@ -718,32 +749,8 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 	if (clean_message_contents)
 		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;
-			else
-				eol = sb.len;
-			if (!prefixcmp(sb.buf + previous, "\nConflicts:\n")) {
-				ignore_footer = sb.len - previous;
-				break;
-			}
-			while (i < eol)
-				i++;
-			previous = eol;
-		}
-
-		append_signoff(&sb, ignore_footer, 0);
-	}
+	if (signoff)
+		append_signoff(&sb, ignore_non_trailer(&sb), 0);
 
 	if (fwrite(sb.buf, 1, sb.len, s->fp) < sb.len)
 		die_errno(_("could not write commit template"));
-- 
2.1.2-620-g33c52cb

[PATCH v2 4/4] merge & sequencer: turn "Conflicts:" hint into a comment

[Junio C Hamano]

Just like other hints such as "Changes to be committed" we show in
the editor to remind the committer what paths were involved in the
resulting commit to help improving their log message, this section
is merely a reminder.

Traditionally, it was not made into comments primarily because it
has to be generated outside the wt-status infrastructure, and also
because it was meant as a bit stronger reminder than the others
(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 part of the log message
proper, and not showing it as a comment is inviting mistakes.

Note that we still notice "Conflicts:" followed by list of indented
pathnames as an old-style cruft and insert a new Signed-off-by:
before it.  This is so that "commit --amend -s" adds the new S-o-b
at the right place when used on an older commit.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/commit.c                | 47 +++++++++++++++++++++++++++--------------
 sequencer.c                     |  7 +++---
 t/t3507-cherry-pick-conflict.sh | 42 +++++++++++++++++++++++++++++-------
 3 files changed, 68 insertions(+), 28 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index cd455aa..0a78e76 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -596,32 +596,47 @@ static char *cut_ident_timestamp_part(char *string)
 /*
  * Inspect sb and determine the true "end" of the log message, in
  * order to find where to put a new Signed-off-by: line.  Ignored are
- * trailing "Conflict:" block.
+ * trailing comment lines and blank lines, and also the traditional
+ * "Conflicts:" block that is not commented out, so that we can use
+ * "git commit -s --amend" on an existing commit that forgot to remove
+ * it.
  *
  * Returns the number of bytes from the tail to ignore, to be fed as
  * the second parameter to append_signoff().
  */
 static int ignore_non_trailer(struct strbuf *sb)
 {
-	int ignore_footer = 0;
-	int i, eol, previous = 0;
-	const char *nl;
+	int boc = 0;
+	int bol = 0;
+	int in_old_conflicts_block = 0;
 
-	for (i = 0; i < sb->len; i++) {
-		nl = memchr(sb->buf + i, '\n', sb->len - i);
-		if (nl)
-			eol = nl - sb->buf;
+	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 (!prefixcmp(sb->buf + bol, "Conflicts:\n")) {
+			in_old_conflicts_block = 1;
+			if (!boc)
+				boc = bol;
+		} else if (in_old_conflicts_block && sb->buf[bol] == '\t') {
+			; /* a pathname in the conflicts block */
+		} else if (boc) {
+			/* the previous was not trailing comment */
+			boc = 0;
+			in_old_conflicts_block = 0;
 		}
-		while (i < eol)
-			i++;
-		previous = eol;
+		bol = next_line - sb->buf;
 	}
-	return ignore_footer;
+	return boc ? sb->len - boc : 0;
 }
 
 static int prepare_to_commit(const char *index_file, const char *prefix,
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..7c5ad08 100755
--- a/t/t3507-cherry-pick-conflict.sh
+++ b/t/t3507-cherry-pick-conflict.sh
@@ -351,19 +351,45 @@ 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 <<EOF > expected &&
-picked
 
-Signed-off-by: C O Mitter <committer@example.com>
+	# Do S-o-b and Conflicts appear in the right order?
+	cat <<-\EOF >expect &&
+	Signed-off-by: C O Mitter <committer@example.com>
+	# 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 <committer@example.com>
+	EOF
 
-	git show -s --pretty=format:%B > actual &&
+	git show -s --pretty=format:%B >actual &&
 	test_cmp expected actual
 '
 
+test_expect_success 'commit --amend -s places the sign-off at the right place' '
+	pristine_detach initial &&
+	test_must_fail git cherry-pick picked &&
+
+	# emulate old-style conflicts block
+	mv .git/MERGE_MSG .git/MERGE_MSG+ &&
+	sed -e "/^# Conflicts:/,\$s/^# *//" <.git/MERGE_MSG+ >.git/MERGE_MSG &&
+
+	git commit -a &&
+	git commit --amend -s &&
+
+	# Do S-o-b and Conflicts appear in the right order?
+	cat <<-\EOF >expect &&
+	Signed-off-by: C O Mitter <committer@example.com>
+	Conflicts:
+	EOF
+	grep -e "^Conflicts:" -e '^Signed-off-by' <.git/COMMIT_EDITMSG >actual &&
+	test_cmp expect actual
+'
+
 test_done
-- 
2.1.2-620-g33c52cb

Re: [PATCH] merge & sequencer: turn "Conflicts:" hint into a comment

[Jeff King]

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

Re: [PATCH] merge & sequencer: turn "Conflicts:" hint into a comment

[Jeff King]

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