Skip to contentSkip to conversation

Reroll of sb/submodule-merge-in-merge-recursive

PATCH In review Stefan Beller 4 participants 19 messages May 15, 2018 – Jun 11, 2018

lore
Share & tools
lore
#0 lore raw

v2 cover Reroll of sb/submodule-merge-in-merge-recursive

This rerolls the two commits found at [1] with the feedback of Eliah and puts Leifs patch[2] on top, that I edited according to Eliahs feedback, but kept Leifs ownership.

This has addressed all of Eliahs feedback AFAICT. You'll find a branch-diff below[3], which lacks
the new patch of Leif in that series, but is part of the reroll?

Leif, what do you think?

Thanks,
Stefan

[1] https://public-inbox.org/git/[email protected]/
[2] https://public-inbox.org/git/[email protected]/
[3] git branch-diff origin/master..origin/sb/submodule-merge-in-merge-recursive origin/master..HEAD >>0000-cover-letter.patch

Leif Middelschulte (1):
Inform about fast-forwarding of submodules during merge

Stefan Beller (2):
submodule.c: move submodule merging to merge-recursive.c
merge-recursive: i18n submodule merge output and respect verbosity

 merge-recursive.c | 185 +++++++++++++++++++++++++++++++++++++++++++++-
 submodule.c       | 168 +----------------------------------------
 submodule.h       |   6 +-
 3 files changed, 186 insertions(+), 173 deletions(-)
Signature · 36 lines
2.17.0.582.gccdcbd54c44.dirty



1:  e022c7976ae ! 1:  3b638ccac64 submodule.c: move submodule merging to merge-recursive.c
    @@ -20,7 +20,6 @@
         This commit is best viewed with --color-moved.
     
         Signed-off-by: Stefan Beller <[email protected]>
    -    Signed-off-by: Junio C Hamano <[email protected]>
     
     diff --git a/merge-recursive.c b/merge-recursive.c
     --- a/merge-recursive.c
2:  2c02ece7e01 ! 2:  eb43110df9d merge-recursive: i18n submodule merge output and respect verbosity
    @@ -7,7 +7,6 @@
         internationalisation as well as the verbosity setting.
     
         Signed-off-by: Stefan Beller <[email protected]>
    -    Signed-off-by: Junio C Hamano <[email protected]>
     
     diff --git a/merge-recursive.c b/merge-recursive.c
     --- a/merge-recursive.c
    @@ -73,10 +72,10 @@
     -		fprintf(stderr, "Found a possible merge resolution "
     -				"for the submodule:\n");
     +		output(o, 1, _("Failed to merge submodule %s (not fast-forward)"), path);
    -+		output(o, 1, _("Found a possible merge resolution for the submodule:\n"));
    ++		output(o, 2, _("Found a possible merge resolution for the submodule:\n"));
      		print_commit((struct commit *) merges.objects[0].item);
     -		fprintf(stderr,
    -+		output(o, 1, _(
    ++		output(o, 2, _(
      			"If this is correct simply add it to the index "
      			"for example\n"
      			"by using:\n\n"
-:  ----------- > 3:  4a3bc435023 Inform about fast-forwarding of submodules during merge
#1 lore raw

PATCH 1/1 git-submodule.sh: try harder to fetch a submodule

This is the logical continuum of fb43e31f2b4 (submodule: try harder to fetch needed sha1 by direct fetching sha1, 2016-02-23) and fixes it as some assumptions were not correct.

The commit states:

Quoted 3 lines

If $sha1 was not part of the default fetch ... fail ourselves here assumes that the fetch_in_submodule only fails when the serverside does not support fetching by sha1.

There are other failures, why such a fetch may fail, such as
fatal: Couldn't find remote ref HEAD
which can happen if the remote side doesn't advertise HEAD and we do not have a local fetch refspec.

Not advertising HEAD is allowed by the protocol spec and would happen, if HEAD points at an unborn branch for example.

Not having a local fetch refspec can happen when submodules are fetched shallowly, as then git-clone doesn't setup a fetch refspec.

So do try even harder for a submodule by ignoring the exit code of the first fetch and rather relying on the following is_tip_reachable to see if we try fetching again.

---
 git-submodule.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
git-submodule.sh +1 -1
index 24914963ca2..00fcd69138f 100755
@@ -614,7 +614,7 @@ cmd_update()
 				# is not reachable from a ref.
 				is_tip_reachable "$sm_path" "$sha1" ||
 				fetch_in_submodule "$sm_path" $depth ||
-				die "$(eval_gettext "Unable to fetch in submodule path '\$displaypath'")"
+				say "$(eval_gettext "Unable to fetch in submodule path '\$displaypath'")"
 
 				# Now we tried the usual fetch, but $sha1 may
 				# not be reachable from any of the refs
Signature · 1 line
2.17.0.582.gccdcbd54c44.dirty
#2 lore raw

PATCH v2 1/3 submodule.c: move submodule merging to merge-recursive.c

In reply to Stefan Beller

In a later patch we want to improve submodule merging by using the output() function in merge-recursive.c for submodule merges to deliver a consistent UI to users.

To do so we could either make the output() function globally available so we can use it in submodule.c#merge_submodule(), or we could integrate the submodule merging into the merging code. Choose the later as we generally want to move submodules closer into the core.

Therefore we move any function related to merging submodules (merge_submodule(), find_first_merges() and print_commit) to merge-recursive.c. We'll keep add_submodule_odb() in submodule.c as it is used by other submodule functions. While at it, add a TODO note that we do not really like the function add_submodule_odb().

This commit is best viewed with --color-moved.

---
 merge-recursive.c | 166 +++++++++++++++++++++++++++++++++++++++++++++
 submodule.c       | 168 +---------------------------------------------
 submodule.h       |   6 +-
 3 files changed, 170 insertions(+), 170 deletions(-)
merge-recursive.c +166 -0
index 0c0d48624da..700ba15bf88 100644
@@ -23,6 +23,7 @@
 #include "merge-recursive.h"
 #include "dir.h"
 #include "submodule.h"
+#include "revision.h"
 
 struct path_hashmap_entry {
 	struct hashmap_entry e;
@@ -977,6 +978,171 @@ static int merge_3way(struct merge_options *o,
 	return merge_status;
 }
 
+static int find_first_merges(struct object_array *result, const char *path,
+		struct commit *a, struct commit *b)
+{
+	int i, j;
+	struct object_array merges = OBJECT_ARRAY_INIT;
+	struct commit *commit;
+	int contains_another;
+
+	char merged_revision[42];
+	const char *rev_args[] = { "rev-list", "--merges", "--ancestry-path",
+				   "--all", merged_revision, NULL };
+	struct rev_info revs;
+	struct setup_revision_opt rev_opts;
+
+	memset(result, 0, sizeof(struct object_array));
+	memset(&rev_opts, 0, sizeof(rev_opts));
+
+	/* get all revisions that merge commit a */
+	xsnprintf(merged_revision, sizeof(merged_revision), "^%s",
+			oid_to_hex(&a->object.oid));
+	init_revisions(&revs, NULL);
+	rev_opts.submodule = path;
+	/* FIXME: can't handle linked worktrees in submodules yet */
+	revs.single_worktree = path != NULL;
+	setup_revisions(ARRAY_SIZE(rev_args)-1, rev_args, &revs, &rev_opts);
+
+	/* save all revisions from the above list that contain b */
+	if (prepare_revision_walk(&revs))
+		die("revision walk setup failed");
+	while ((commit = get_revision(&revs)) != NULL) {
+		struct object *o = &(commit->object);
+		if (in_merge_bases(b, commit))
+			add_object_array(o, NULL, &merges);
+	}
+	reset_revision_walk();
+
+	/* Now we've got all merges that contain a and b. Prune all
+	 * merges that contain another found merge and save them in
+	 * result.
+	 */
+	for (i = 0; i < merges.nr; i++) {
+		struct commit *m1 = (struct commit *) merges.objects[i].item;
+
+		contains_another = 0;
+		for (j = 0; j < merges.nr; j++) {
+			struct commit *m2 = (struct commit *) merges.objects[j].item;
+			if (i != j && in_merge_bases(m2, m1)) {
+				contains_another = 1;
+				break;
+			}
+		}
+
+		if (!contains_another)
+			add_object_array(merges.objects[i].item, NULL, result);
+	}
+
+	object_array_clear(&merges);
+	return result->nr;
+}
+
+static void print_commit(struct commit *commit)
+{
+	struct strbuf sb = STRBUF_INIT;
+	struct pretty_print_context ctx = {0};
+	ctx.date_mode.type = DATE_NORMAL;
+	format_commit_message(commit, " %h: %m %s", &sb, &ctx);
+	fprintf(stderr, "%s\n", sb.buf);
+	strbuf_release(&sb);
+}
+
+#define MERGE_WARNING(path, msg) \
+	warning("Failed to merge submodule %s (%s)", path, msg);
+
+static int merge_submodule(struct object_id *result, const char *path,
+			   const struct object_id *base, const struct object_id *a,
+			   const struct object_id *b, int search)
+{
+	struct commit *commit_base, *commit_a, *commit_b;
+	int parent_count;
+	struct object_array merges;
+
+	int i;
+
+	/* store a in result in case we fail */
+	oidcpy(result, a);
+
+	/* we can not handle deletion conflicts */
+	if (is_null_oid(base))
+		return 0;
+	if (is_null_oid(a))
+		return 0;
+	if (is_null_oid(b))
+		return 0;
+
+	if (add_submodule_odb(path)) {
+		MERGE_WARNING(path, "not checked out");
+		return 0;
+	}
+
+	if (!(commit_base = lookup_commit_reference(base)) ||
+	    !(commit_a = lookup_commit_reference(a)) ||
+	    !(commit_b = lookup_commit_reference(b))) {
+		MERGE_WARNING(path, "commits not present");
+		return 0;
+	}
+
+	/* check whether both changes are forward */
+	if (!in_merge_bases(commit_base, commit_a) ||
+	    !in_merge_bases(commit_base, commit_b)) {
+		MERGE_WARNING(path, "commits don't follow merge-base");
+		return 0;
+	}
+
+	/* Case #1: a is contained in b or vice versa */
+	if (in_merge_bases(commit_a, commit_b)) {
+		oidcpy(result, b);
+		return 1;
+	}
+	if (in_merge_bases(commit_b, commit_a)) {
+		oidcpy(result, a);
+		return 1;
+	}
+
+	/*
+	 * Case #2: There are one or more merges that contain a and b in
+	 * the submodule. If there is only one, then present it as a
+	 * suggestion to the user, but leave it marked unmerged so the
+	 * user needs to confirm the resolution.
+	 */
+
+	/* Skip the search if makes no sense to the calling context.  */
+	if (!search)
+		return 0;
+
+	/* find commit which merges them */
+	parent_count = find_first_merges(&merges, path, commit_a, commit_b);
+	switch (parent_count) {
+	case 0:
+		MERGE_WARNING(path, "merge following commits not found");
+		break;
+
+	case 1:
+		MERGE_WARNING(path, "not fast-forward");
+		fprintf(stderr, "Found a possible merge resolution "
+				"for the submodule:\n");
+		print_commit((struct commit *) merges.objects[0].item);
+		fprintf(stderr,
+			"If this is correct simply add it to the index "
+			"for example\n"
+			"by using:\n\n"
+			"  git update-index --cacheinfo 160000 %s \"%s\"\n\n"
+			"which will accept this suggestion.\n",
+			oid_to_hex(&merges.objects[0].item->oid), path);
+		break;
+
+	default:
+		MERGE_WARNING(path, "multiple merges found");
+		for (i = 0; i < merges.nr; i++)
+			print_commit((struct commit *) merges.objects[i].item);
+	}
+
+	object_array_clear(&merges);
+	return 0;
+}
+
 static int merge_file_1(struct merge_options *o,
 					   const struct diff_filespec *one,
 					   const struct diff_filespec *a,
submodule.c +2 -166

180 lines not on this page · raw message

submodule.h +2 -4
index e5526f6aaab..b96689ac0db 100644
@@ -89,10 +89,8 @@ extern int submodule_uses_gitfile(const char *path);
 #define SUBMODULE_REMOVAL_IGNORE_UNTRACKED (1<<1)
 #define SUBMODULE_REMOVAL_IGNORE_IGNORED_UNTRACKED (1<<2)
 extern int bad_to_remove_submodule(const char *path, unsigned flags);
-extern int merge_submodule(struct object_id *result, const char *path,
-			   const struct object_id *base,
-			   const struct object_id *a,
-			   const struct object_id *b, int search);
+
+int add_submodule_odb(const char *path);
 
 /* Checks if there are submodule changes in a..b. */
 extern int submodule_touches_in_range(struct object_id *a,
Signature · 1 line
2.17.0.582.gccdcbd54c44.dirty
#3 lore raw

PATCH v2 2/3 merge-recursive: i18n submodule merge output and respect verbosity

In reply to Stefan Beller

The submodule merge code now uses the output() function that is used by all the rest of the merge-recursive-code. This allows for respecting internationalisation as well as the verbosity setting.

---
 merge-recursive.c | 33 +++++++++++++++------------------
 1 file changed, 15 insertions(+), 18 deletions(-)
merge-recursive.c +15 -18
index 700ba15bf88..0571919ee0a 100644
@@ -1048,18 +1048,17 @@ static void print_commit(struct commit *commit)
 	strbuf_release(&sb);
 }
 
-#define MERGE_WARNING(path, msg) \
-	warning("Failed to merge submodule %s (%s)", path, msg);
-
-static int merge_submodule(struct object_id *result, const char *path,
+static int merge_submodule(struct merge_options *o,
+			   struct object_id *result, const char *path,
 			   const struct object_id *base, const struct object_id *a,
-			   const struct object_id *b, int search)
+			   const struct object_id *b)
 {
 	struct commit *commit_base, *commit_a, *commit_b;
 	int parent_count;
 	struct object_array merges;
 
 	int i;
+	int search = !o->call_depth;
 
 	/* store a in result in case we fail */
 	oidcpy(result, a);
@@ -1073,21 +1072,21 @@ static int merge_submodule(struct object_id *result, const char *path,
 		return 0;
 
 	if (add_submodule_odb(path)) {
-		MERGE_WARNING(path, "not checked out");
+		output(o, 1, _("Failed to merge submodule %s (not checked out)"), path);
 		return 0;
 	}
 
 	if (!(commit_base = lookup_commit_reference(base)) ||
 	    !(commit_a = lookup_commit_reference(a)) ||
 	    !(commit_b = lookup_commit_reference(b))) {
-		MERGE_WARNING(path, "commits not present");
+		output(o, 1, _("Failed to merge submodule %s (commits not present)"), path);
 		return 0;
 	}
 
 	/* check whether both changes are forward */
 	if (!in_merge_bases(commit_base, commit_a) ||
 	    !in_merge_bases(commit_base, commit_b)) {
-		MERGE_WARNING(path, "commits don't follow merge-base");
+		output(o, 1, _("Failed to merge submodule %s (commits don't follow merge-base)"), path);
 		return 0;
 	}
 
@@ -1116,25 +1115,24 @@ static int merge_submodule(struct object_id *result, const char *path,
 	parent_count = find_first_merges(&merges, path, commit_a, commit_b);
 	switch (parent_count) {
 	case 0:
-		MERGE_WARNING(path, "merge following commits not found");
+		output(o, 1, _("Failed to merge submodule %s (merge following commits not found)"), path);
 		break;
 
 	case 1:
-		MERGE_WARNING(path, "not fast-forward");
-		fprintf(stderr, "Found a possible merge resolution "
-				"for the submodule:\n");
+		output(o, 1, _("Failed to merge submodule %s (not fast-forward)"), path);
+		output(o, 2, _("Found a possible merge resolution for the submodule:\n"));
 		print_commit((struct commit *) merges.objects[0].item);
-		fprintf(stderr,
+		output(o, 2, _(
 			"If this is correct simply add it to the index "
 			"for example\n"
 			"by using:\n\n"
 			"  git update-index --cacheinfo 160000 %s \"%s\"\n\n"
-			"which will accept this suggestion.\n",
+			"which will accept this suggestion.\n"),
 			oid_to_hex(&merges.objects[0].item->oid), path);
 		break;
 
 	default:
-		MERGE_WARNING(path, "multiple merges found");
+		output(o, 1, _("Failed to merge submodule %s (multiple merges found)"), path);
 		for (i = 0; i < merges.nr; i++)
 			print_commit((struct commit *) merges.objects[i].item);
 	}
@@ -1205,12 +1203,11 @@ static int merge_file_1(struct merge_options *o,
 				return ret;
 			result->clean = (merge_status == 0);
 		} else if (S_ISGITLINK(a->mode)) {
-			result->clean = merge_submodule(&result->oid,
+			result->clean = merge_submodule(o, &result->oid,
 						       one->path,
 						       &one->oid,
 						       &a->oid,
-						       &b->oid,
-						       !o->call_depth);
+						       &b->oid);
 		} else if (S_ISLNK(a->mode)) {
 			switch (o->recursive_variant) {
 			case MERGE_RECURSIVE_NORMAL:
Signature · 1 line
2.17.0.582.gccdcbd54c44.dirty
#4 lore raw

Re: [PATCH 2/3] merge-recursive: i18n submodule merge output and respect verbosity

merge-recursive.c static void print_commit(struct commit *commit) · line 1061

Quoted 12 lines · Stefan Beller
merge-recursive.c +4 -1
+static int merge_submodule(struct merge_options *o,
+			   struct object_id *result, const char *path,
 			   const struct object_id *base, const struct object_id *a,
-			   const struct object_id *b, int search)
+			   const struct object_id *b)
 {
 	struct commit *commit_base, *commit_a, *commit_b;
 	int parent_count;
 	struct object_array merges;
 
 	int i;
+	int search = !o->call_depth;

I kind of like this "while at it" change in this patch ;-)

#5 lore raw

PATCH 1/1 grep: handle corrupt index files early

In reply to Stefan Beller

Any other caller of 'repo_read_index' dies upon a negative return of it, so grep should, too.

---

Found while reviewing the series
https://public-inbox.org/git/[email protected]/

 builtin/grep.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
builtin/grep.c +2 -1
index 6e7bc76785a..69f0743619f 100644
@@ -488,7 +488,8 @@ static int grep_cache(struct grep_opt *opt, struct repository *repo,
 		strbuf_addstr(&name, repo->submodule_prefix);
 	}
 
-	repo_read_index(repo);
+	if (repo_read_index(repo) < 0)
+		die("index file corrupt");
 
 	for (nr = 0; nr < repo->index->cache_nr; nr++) {
 		const struct cache_entry *ce = repo->index->cache[nr];
Signature · 1 line
2.17.0.582.gccdcbd54c44.dirty
#6 lore raw

PATCH v2 3/3 Inform about fast-forwarding of submodules during merge

In reply to Stefan Beller

From: Leif Middelschulte <[email protected]>

Inform the user about an automatically fast-forwarded submodule. The silent merge behavior was introduced by commit 68d03e4a6e44 ("Implement automatic fast-forward merge for submodules", 2010-07-07)).

---
 merge-recursive.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)
merge-recursive.c +16 -0
index 0571919ee0a..29a430c418a 100644
@@ -1093,10 +1093,26 @@ static int merge_submodule(struct merge_options *o,
 	/* Case #1: a is contained in b or vice versa */
 	if (in_merge_bases(commit_a, commit_b)) {
 		oidcpy(result, b);
+		if (show(o, 3)) {
+			output(o, 1, _("Fast-forwarding submodule %s to the following commit:"), path);
+			output_commit_title(o, commit_b);
+		} else if (show(o, 2))
+			output(o, 2, _("Fast-forwarding submodule %s to %s"), path, oid_to_hex(b));
+		else
+			; /* no output */
+
 		return 1;
 	}
 	if (in_merge_bases(commit_b, commit_a)) {
 		oidcpy(result, a);
+		if (show(o, 3)) {
+			output(o, 1, _("Fast-forwarding submodule %s to the following commit:"), path);
+			output_commit_title(o, commit_a);
+		} else if (show(o, 2))
+			output(o, 2, _("Fast-forwarding submodule %s to %s"), path, oid_to_hex(a));
+		else
+			; /* no output */
+
 		return 1;
 	}
 
Signature · 1 line
2.17.0.582.gccdcbd54c44.dirty
#7 lore raw

Re: [PATCH 3/3] Inform about fast-forwarding of submodules during merge

Quoted 17 lines · Stefan Beller

From: Leif Middelschulte <[email protected]>

Inform the user about an automatically fast-forwarded submodule. The silent merge behavior was introduced by commit 68d03e4a6e44 ("Implement automatic fast-forward merge for submodules", 2010-07-07)).

---
 merge-recursive.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)
merge-recursive.c +0 -0
index 0571919ee0a..29a430c418a 100644

merge-recursive.c static int merge_submodule(struct merge_options *o, · line 1097

Quoted 5 lines
merge-recursive.c +2 -0
@@ -1093,10 +1093,26 @@ static int merge_submodule(struct merge_options *o,
        /* Case #1: a is contained in b or vice versa */
        if (in_merge_bases(commit_a, commit_b)) {
                oidcpy(result, b);
+               if (show(o, 3)) {
+                       output(o, 1, _("Fast-forwarding submodule %s to the following commit:"), path);

Seems slightly odd to mix 3 and 1 here; although it'll work just fine, I would have expected use of 3 in both places (much like you did with the 2 and 2 below).

merge-recursive.c static int merge_submodule(struct merge_options *o, · line 1109

Quoted 12 lines
merge-recursive.c +8 -0
+                       output_commit_title(o, commit_b);
+               } else if (show(o, 2))
+                       output(o, 2, _("Fast-forwarding submodule %s to %s"), path, oid_to_hex(b));
+               else
+                       ; /* no output */
+
                return 1;
        }
        if (in_merge_bases(commit_b, commit_a)) {
                oidcpy(result, a);
+               if (show(o, 3)) {
+                       output(o, 1, _("Fast-forwarding submodule %s to the following commit:"), path);

Same.

Quoted 11 lines
merge-recursive.c +6 -1
+                       output_commit_title(o, commit_a);
+               } else if (show(o, 2))
+                       output(o, 2, _("Fast-forwarding submodule %s to %s"), path, oid_to_hex(a));
+               else
+                       ; /* no output */
+
                return 1;
        }
 
--

2.17.0.582.gccdcbd54c44.dirty

Other than that nit-pick, looks good to me.

#8 lore raw

Re: [PATCH 3/3] Inform about fast-forwarding of submodules during merge

In reply to Stefan Beller
Quoted 2 lines · Stefan Beller

From: Leif Middelschulte <[email protected]>

Subject: merge-recursive: give notice when submodule commit gets fast-forwarded

perhaps?

merge-recursive.c static int merge_submodule(struct merge_options *o, · line 1103

Quoted 11 lines
merge-recursive.c +8 -0
 	/* Case #1: a is contained in b or vice versa */
 	if (in_merge_bases(commit_a, commit_b)) {
 		oidcpy(result, b);
+		if (show(o, 3)) {
+			output(o, 1, _("Fast-forwarding submodule %s to the following commit:"), path);
+			output_commit_title(o, commit_b);
+		} else if (show(o, 2))
+			output(o, 2, _("Fast-forwarding submodule %s to %s"), path, oid_to_hex(b));
+		else
+			; /* no output */
+

merge.verbosity::
Controls the amount of output shown by the recursive merge strategy. Level 0 outputs nothing except a final error message if conflicts were detected. Level 1 outputs only conflicts, 2 outputs conflicts and file changes. Level 5 and above outputs debugging information. The default is level 2. Can be overridden by the GIT_MERGE_VERBOSITY environment variable.

So, by default, we report the fact that we update submodule to a particular commit, which is quite similar to how we report auto merged paths using the content level 3-way merge; when you squint your eyes, the "fast-forward" of submodules look somewhat like a content-level 3-way merge anyway ;-)

And at level 3, which currently is used to report a non-event that does not change the result of the merge from what was naturally expected, we give a bit more detail by citing the commit the submodule gets fast-forwarded to [*1*].

Sort of makes sense.

[Footnote]

*1* I wonder if that is really necessary, though---we do not give "here is a diff" or "this is the new contents" after a path gets merged for normal files. And if it is needed perhaps because submodules are so special, I wonder if we also need to give the commit the submodule gets fast-forwarded from, i.e. the original one, the same way.

#9 lore raw

Re: [PATCH 3/3] Inform about fast-forwarding of submodules during merge

In reply to Stefan Beller
Quoted 5 lines · Stefan Beller

From: Leif Middelschulte <[email protected]>

Inform the user about an automatically fast-forwarded submodule. The silent merge behavior was introduced by commit 68d03e4a6e44 ("Implement automatic fast-forward merge for submodules", 2010-07-07)).

Oh, another thing I forgot to mention.

These three lines do not convey much useful information. The first sentence can be read from the patch text, and the rest can be read from "git blame" and "git log" output.

It is correct that the silent behaviour was introduced long time ago. The proposed log message does not even say if that silent behaviour is bad in any way, let alone why it is bad and need to be changed.

Perhaps Leif can elaborate why this change is a good idea in the first place?

Thanks.

#10 lore raw

v4 cover merge-recursive: give notice when submodule commit gets fast-forwarded

From: Leif Middelschulte <[email protected]>

The provided patch is in response to Elijah Newren's [0] and Junio Hamano's [1] comments on my prior patch regarding the reasoning and implementation of a user notification during (clean) merges of submodules.

[0] https://public-inbox.org/git/[email protected]/#t
[1] https://public-inbox.org/git/[email protected]/

Leif Middelschulte (1):
Inform about Auto-merging of submodules during merge

 merge-recursive.c | 10 ++++++++++
 1 file changed, 10 insertions(+)
Signature · 1 line
2.15.1 (Apple Git-101)
#11 lore raw

PATCH v4 1/1 merge-recursive: give notice when submodule commit gets fast-forwarded

From: Leif Middelschulte <[email protected]>

Since submodules are treated similarly to ordinary files (i.e. not as 'dumb' pointers), an automatic merge should be mentioned if the user asks for it. Just as it is mentioned for oridnary files.

---
 merge-recursive.c | 10 ++++++++++
 1 file changed, 10 insertions(+)
merge-recursive.c +10 -0
index a4b91d17f..0990a135b 100644
@@ -1093,10 +1093,20 @@ static int merge_submodule(struct merge_options *o,
 	/* Case #1: a is contained in b or vice versa */
 	if (in_merge_bases(commit_a, commit_b)) {
 		oidcpy(result, b);
+		if (show(o, 2))
+			output(o, 2, _("Auto-merging %s"), path);
+		else
+			; /* no output */
+
 		return 1;
 	}
 	if (in_merge_bases(commit_b, commit_a)) {
 		oidcpy(result, a);
+		if (show(o, 2))
+			output(o, 2, _("Auto-merging %s"), path);
+		else
+			; /* no output */
+
 		return 1;
 	}
 
Signature · 1 line
2.15.1 (Apple Git-101)
#12 lore raw

Re: [PATCH 1/1] merge-recursive: give notice when submodule commit gets fast-forwarded

Hi Leif,

Quoted 5 lines · Leif Middelschulte

From: Leif Middelschulte <[email protected]>

Since submodules are treated similarly to ordinary files (i.e. not as 'dumb' pointers), an automatic merge should be mentioned if the user asks for it. Just as it is mentioned for oridnary files.

Thanks for following up; sorry it took me a few days to respond. However, it looks like Junio merged the sb/submodule-merge-in-merge-recursive topic, including your patch, to master back on May 30. As such, instead of re-rolling your patch, we'd need a patch on top of the other existing change.

Also, take a look at the preliminary release announcement -- you show up as a new contributor to git! See it at
https://public-inbox.org/git/[email protected]/

Quoted 1 line

+ output(o, 2, _("Auto-merging %s"), path);

...

Quoted 1 line

+ output(o, 2, _("Auto-merging %s"), path);

I preferred your old initial wording here, "Fast-forwarding submodule %s" (I just wanted the "to %s" part at the end removed). I'm afraid that users who saw "Auto-merging $submodule" would assume that we descended into the submodule and ran a full merge there.

Could you submit a patch that just removed that "to %s" part?

#13 lore raw

PATCH 1/1 merge-submodule: reduce output verbosity

From: Leif Middelschulte <[email protected]>

The output shall behave more similar to ordinary file merges' output to provide a more consistent user experience.

---
 merge-recursive.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
merge-recursive.c +2 -2
index ac27abbd4..5eb907f46 100644
@@ -1208,7 +1208,7 @@ static int merge_submodule(struct merge_options *o,
 			output(o, 3, _("Fast-forwarding submodule %s to the following commit:"), path);
 			output_commit_title(o, commit_b);
 		} else if (show(o, 2))
-			output(o, 2, _("Fast-forwarding submodule %s to %s"), path, oid_to_hex(b));
+			output(o, 2, _("Fast-forwarding submodule %s"), path);
 		else
 			; /* no output */
 
@@ -1220,7 +1220,7 @@ static int merge_submodule(struct merge_options *o,
 			output(o, 3, _("Fast-forwarding submodule %s to the following commit:"), path);
 			output_commit_title(o, commit_a);
 		} else if (show(o, 2))
-			output(o, 2, _("Fast-forwarding submodule %s to %s"), path, oid_to_hex(a));
+			output(o, 2, _("Fast-forwarding submodule %s"), path);
 		else
 			; /* no output */
 
Signature · 1 line
2.15.1 (Apple Git-101)
#14 lore raw

Re: [PATCH] merge-submodule: reduce output verbosity

Quoted 9 lines · Leif Middelschulte

From: Leif Middelschulte <[email protected]>

The output shall behave more similar to ordinary file merges' output to provide a more consistent user experience.

---
 merge-recursive.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Thanks, both. Very much appreciated.

Quoted 22 lines
merge-recursive.c +2 -2
index ac27abbd4..5eb907f46 100644
@@ -1208,7 +1208,7 @@ static int merge_submodule(struct merge_options *o,
 			output(o, 3, _("Fast-forwarding submodule %s to the following commit:"), path);
 			output_commit_title(o, commit_b);
 		} else if (show(o, 2))
-			output(o, 2, _("Fast-forwarding submodule %s to %s"), path, oid_to_hex(b));
+			output(o, 2, _("Fast-forwarding submodule %s"), path);
 		else
 			; /* no output */
 
@@ -1220,7 +1220,7 @@ static int merge_submodule(struct merge_options *o,
 			output(o, 3, _("Fast-forwarding submodule %s to the following commit:"), path);
 			output_commit_title(o, commit_a);
 		} else if (show(o, 2))
-			output(o, 2, _("Fast-forwarding submodule %s to %s"), path, oid_to_hex(a));
+			output(o, 2, _("Fast-forwarding submodule %s"), path);
 		else
 			; /* no output */
#15 lore raw

Re: [PATCH] merge-submodule: reduce output verbosity

Hello,

Quoted 7 lines · Am 11. Juni 2018 um 20:04:19, Junio C Hamano…
Quoted 5 lines · Leif Middelschulte

From: Leif Middelschulte

The output shall behave more similar to ordinary file merges' output to provide a more consistent user experience.

Quoted 6 lines
Quoted 4 lines
Signed-off-by Leif Middelschulte

---
merge-recursive.c | 4 ++--

1 file changed, 2 insertions(+), 2 deletions(-)

Thanks, both. Very much appreciated.

You are welcome. Thank all of you, who participated in the discussions about this topic, for your patience, advice, and guidance. I am sorry it took me so long to reply and provide the patches.

Cheers,

Leif

Quoted 23 lines
Quoted 23 lines
merge-recursive.c +1 -1
index ac27abbd4..5eb907f46 100644
@@ -1208,7 +1208,7 @@ static int merge_submodule(struct merge_options *o,
 output(o, 3, _("Fast-forwarding submodule %s to the following commit:"), path);
 output_commit_title(o, commit_b);
 } else if (show(o, 2))
- output(o, 2, _("Fast-forwarding submodule %s to %s"), path, oid_to_hex(b));
+ output(o, 2, _("Fast-forwarding submodule %s"), path);

else
; /* no output */

merge-recursive.c +1 -1
@@ -1220,7 +1220,7 @@ static int merge_submodule(struct merge_options *o,
 output(o, 3, _("Fast-forwarding submodule %s to the following commit:"), path);
 output_commit_title(o, commit_a);
 } else if (show(o, 2))
- output(o, 2, _("Fast-forwarding submodule %s to %s"), path, oid_to_hex(a));
+ output(o, 2, _("Fast-forwarding submodule %s"), path);

else
; /* no output */

#16 lore raw
In reply to Stefan Beller

And I resent two of my earlier patches, please ignore those (0001-grep-handle-corrupt-index-files-early.patch and 0001-git-submodule.sh-try-harder-to-fetch-a-submodule.patch)

Stefan

#17 lore raw
In reply to Stefan Beller

Hello Stefan,

thank you once again for your effort.

Quoted 9 lines · Am 15. Mai 2018 um 22:00:34, Stefan Beller…

This rerolls the two commits found at [1] with the feedback of Eliah and puts Leifs patch[2] on top, that I edited according to Eliahs feedback, but kept Leifs ownership.

This has addressed all of Eliahs feedback AFAICT. You'll find a branch-diff below[3], which lacks
the new patch of Leif in that series, but is part of the reroll?

Leif, what do you think?

Seems great to me. Thank you for picking up and improving my changes :) One Question though: Shouldn’t an enum (like NOTES_MERGE_VERBOSITY_DEFAULT) be used instead of numbers?

Cheers,

Leif

Quoted 57 lines

Thanks,
Stefan

[1] https://public-inbox.org/git/[email protected]/
[2] https://public-inbox.org/git/[email protected]/
[3] git branch-diff origin/master..origin/sb/submodule-merge-in-merge-recursive origin/master..HEAD >>0000-cover-letter.patch

Leif Middelschulte (1):
Inform about fast-forwarding of submodules during merge

Stefan Beller (2):
submodule.c: move submodule merging to merge-recursive.c
merge-recursive: i18n submodule merge output and respect verbosity

merge-recursive.c | 185 +++++++++++++++++++++++++++++++++++++++++++++- submodule.c | 168 +---------------------------------------- submodule.h | 6 +-

3 files changed, 186 insertions(+), 173 deletions(-)

--
2.17.0.582.gccdcbd54c44.dirty

1: e022c7976ae ! 1: 3b638ccac64 submodule.c: move submodule merging to merge-recursive.c

(quoted diff) +0 -0
@@ -20,7 +20,6 @@

This commit is best viewed with --color-moved.

Signed-off-by: Stefan Beller
- Signed-off-by: Junio C Hamano

merge-recursive.c +0 -1
--- a/merge-recursive.c

2: 2c02ece7e01 ! 2: eb43110df9d merge-recursive: i18n submodule merge output and respect verbosity

merge-recursive.c +0 -0
@@ -7,7 +7,6 @@

internationalisation as well as the verbosity setting.

Signed-off-by: Stefan Beller
- Signed-off-by: Junio C Hamano

merge-recursive.c +3 -6
--- a/merge-recursive.c
@@ -73,10 +72,10 @@
- fprintf(stderr, "Found a possible merge resolution "
- "for the submodule:\n");
+ output(o, 1, _("Failed to merge submodule %s (not fast-forward)"), path);
-+ output(o, 1, _("Found a possible merge resolution for the submodule:\n"));
++ output(o, 2, _("Found a possible merge resolution for the submodule:\n"));
 print_commit((struct commit *) merges.objects[0].item);
- fprintf(stderr,
-+ output(o, 1, _(
++ output(o, 2, _(

"If this is correct simply add it to the index "
"for example\n"
"by using:\n\n"
-: ----------- > 3: 4a3bc435023 Inform about fast-forwarding of submodules during merge

#18 lore raw
Quoted 15 lines · Leif Middelschulte

Hello Stefan,

thank you once again for your effort.

Quoted 8 lines · Am 15. Mai 2018 um 22:00:34, Stefan Beller…

This rerolls the two commits found at [1] with the feedback of Eliah and puts Leifs patch[2] on top, that I edited according to Eliahs feedback, but kept Leifs ownership.

This has addressed all of Eliahs feedback AFAICT. You'll find a branch-diff below[3], which lacks
the new patch of Leif in that series, but is part of the reroll?

Quoted 5 lines
Quoted 1 line

Leif, what do you think?

Seems great to me. Thank you for picking up and improving my changes :) One Question though: Shouldn’t an enum (like NOTES_MERGE_VERBOSITY_DEFAULT) be used instead of numbers?

Hah! I did not know that existed.

$ git grep NOTES_MERGE_VERBOSITY_DEFAULT
builtin/notes.c:810:    o.verbosity = verbosity + NOTES_MERGE_VERBOSITY_DEFAULT;
notes-merge.c:22:       o->verbosity = NOTES_MERGE_VERBOSITY_DEFAULT;
notes-merge.h:9:        NOTES_MERGE_VERBOSITY_DEFAULT = 2,

It doesn't seem to be used much, as opposed to numbers:

$ git grep show -- merge-recursive.c
merge-recursive.c:201:static int show(struct merge_options *o, int v)
merge-recursive.c:211:  if (!show(o, v))
merge-recursive.c:570:  opts.show_rename_progress = o->show_rename_progress;
merge-recursive.c:1096:         if (show(o, 3)) {
merge-recursive.c:1099:         } else if (show(o, 2))
merge-recursive.c:1108:         if (show(o, 3)) {
merge-recursive.c:1111:         } else if (show(o, 2))
merge-recursive.c:2178:         if (show(o, 4) || o->call_depth)
merge-recursive.c:2275: if (show(o, 4)) {
merge-recursive.c:2286: if (show(o, 5)) {
merge-recursive.c:2351: if (show(o, 2))

(The first two are the implementation of show/output, third is somewhat unrelated to show() and all the rest is numbers).

If we'd want to use NOTES_MERGE_VERBOSITY_DEFAULT, I would suggest to send a followup series on top of this?

I would think numbers are fine for now.

Thanks,
Stefan