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

Subsystems: the rest

STALE3743d

7 messages, 2 authors, 2016-06-15 · open the first message on its own page

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

From: Junio C Hamano <hidden>
Date: 2016-06-15 23:02:47

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 <redacted>
---

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

From: Jonathan Nieder <hidden>
Date: 2016-06-15 23:02:47

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 <redacted>
Reviewed-by: Jonathan Nieder <redacted>
 * ... 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 v2 1/4] builtin/merge.c: drop a parameter that is never used

From: Junio C Hamano <hidden>
Date: 2016-06-15 23:02:48

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 <redacted>
---
 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 0/4] Turning Conflicts: hint into comment

From: Junio C Hamano <hidden>
Date: 2016-06-15 23:02:48

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 2/4] merge & sequencer: unify codepaths that write "Conflicts:" hint

From: Junio C Hamano <hidden>
Date: 2016-06-15 23:02:48

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 <redacted>
---
 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

From: Junio C Hamano <hidden>
Date: 2016-06-15 23:02:48

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 <redacted>
---
 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

From: Junio C Hamano <hidden>
Date: 2016-06-15 23:02:48

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 <redacted>
---
 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
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help