[PATCH 0/1] warn about auto fast-forwarded submodules during merges

[PATCH 0/1] warn about auto fast-forwarded submodules during merges

[Leif Middelschulte]

From: Leif Middelschulte <Leif.Middelschulte@gmail.com>

Warn the user during merges about automatically fast-forwarded submodules.
This is just informational and does *not* change behavior otherwise.

It is a follow up to Elijah Newren's suggestion[0] to provide the attached patch.

[0] https://marc.info/?l=git&m=152544498723355&w=2

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

 submodule.c | 2 ++
 1 file changed, 2 insertions(+)

-- 
2.15.1 (Apple Git-101)

[PATCH 1/1] Warn about fast-forwarding of submodules during merge

[Leif Middelschulte]

From: Leif Middelschulte <Leif.Middelschulte@gmail.com>

Warn 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)).

Signed-off-by: Leif Middelschulte <Leif.Middelschulte@gmail.com>
---
 submodule.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/submodule.c b/submodule.c
index 74d35b257..0198a72e6 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1817,10 +1817,12 @@ int merge_submodule(struct object_id *result, const char *path,
 	/* Case #1: a is contained in b or vice versa */
 	if (in_merge_bases(commit_a, commit_b)) {
 		oidcpy(result, b);
+		warning("Fast-forwarding submodule %s", path);
 		return 1;
 	}
 	if (in_merge_bases(commit_b, commit_a)) {
 		oidcpy(result, a);
+		warning("Fast-forwarding submodule %s", path);
 		return 1;
 	}
 
-- 
2.15.1 (Apple Git-101)

Re: [PATCH 1/1] Warn about fast-forwarding of submodules during merge

[Stefan Beller]

On Thu, May 10, 2018 at 11:26 AM, Leif Middelschulte
<leif.middelschulte@gmail.com> wrote:
> From: Leif Middelschulte <Leif.Middelschulte@gmail.com>

Hi Leif!

thanks for following up with a patch!

> Warn 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)).
>
> Signed-off-by: Leif Middelschulte <Leif.Middelschulte@gmail.com>
> ---
>  submodule.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/submodule.c b/submodule.c
> index 74d35b257..0198a72e6 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -1817,10 +1817,12 @@ int merge_submodule(struct object_id *result, const char *path,
>         /* Case #1: a is contained in b or vice versa */
>         if (in_merge_bases(commit_a, commit_b)) {
>                 oidcpy(result, b);
> +               warning("Fast-forwarding submodule %s", path);
>                 return 1;
>         }
>         if (in_merge_bases(commit_b, commit_a)) {
>                 oidcpy(result, a);
> +               warning("Fast-forwarding submodule %s", path);
>                 return 1;
>         }

The code looks correct, however I think we can improve it.
(Originally I was just wondering if stderr is the right output,
which lead me to the thoughts below:)

Looking through the code of merge-recursive.c,
all the other merge outputs are done via 'output()'
that is able to buffer up the output as well as handles
the output for different verbosity settings.

So I would think we should make the output() function available
outside of merge-recursive.c. (and rename it to a be more concise
and descriptive in the global namespace) and make use of it.

Funnily we already have MERGE_WARNING in submodule.c
which outputs information for all the other cases. I would think
we ought to convert those to the output(), too.

Thanks,
Stefan

Re: [PATCH 1/1] Warn about fast-forwarding of submodules during merge

[Leif Middelschulte]

Hi Stefan,


Am 10. Mai 2018 um 20:49:39, Stefan Beller
(sbeller@google.com(mailto:sbeller@google.com)) schrieb:

> On Thu, May 10, 2018 at 11:26 AM, Leif Middelschulte
> wrote:
> > From: Leif Middelschulte
>
> Hi Leif!
>
> thanks for following up with a patch!
sure, thanks for the quick review.
>
> > Warn 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)).
> >
> > Signed-off-by: Leif Middelschulte
> > ---
> > submodule.c | 2 ++
> > 1 file changed, 2 insertions(+)
> >
> > diff --git a/submodule.c b/submodule.c
> > index 74d35b257..0198a72e6 100644
> > --- a/submodule.c
> > +++ b/submodule.c
> > @@ -1817,10 +1817,12 @@ int merge_submodule(struct object_id *result, const char *path,
> > /* Case #1: a is contained in b or vice versa */
> > if (in_merge_bases(commit_a, commit_b)) {
> > oidcpy(result, b);
> > + warning("Fast-forwarding submodule %s", path);
> > return 1;
> > }
> > if (in_merge_bases(commit_b, commit_a)) {
> > oidcpy(result, a);
> > + warning("Fast-forwarding submodule %s", path);
> > return 1;
> > }
>
> The code looks correct, however I think we can improve it.
> (Originally I was just wondering if stderr is the right output,
> which lead me to the thoughts below:)
I’ve had the same thoughts about stderr. However, I thought that using a
log function named `warning` to warn the user would be the right choice.
If anything, I thought, the warning function might need refactoring.

> Looking through the code of merge-recursive.c,
> all the other merge outputs are done via 'output()'
> that is able to buffer up the output as well as handles
> the output for different verbosity settings.
>
> So I would think we should make the output() function available
> outside of merge-recursive.c. (and rename it to a be more concise
> and descriptive in the global namespace) and make use of it.
Sure, let me know what to use instead and I’ll update and resubmit the patch.

>
> Funnily we already have MERGE_WARNING in submodule.c
> which outputs information for all the other cases. I would think
> we ought to convert those to the output(), too.
Sure, but `MERGE_WARNING` prefixes all the messages with "Failed to
merge submodule“.
>
> Thanks,
> Stefan

Thank you,
Leif

[PATCH 0/2] Submodule merging: i18n, verbosity

[Stefan Beller]

Leif wrote:
> Sure, let me know what to use instead and I’ll update and resubmit the patch.
> Sure, but `MERGE_WARNING` prefixes all the messages with "Failed to
> merge submodule“.

I thought about replying and coming up with good reasons, but I wrote some
patches instead.

They can also be found at https://github.com/stefanbeller/git/tree/submodule_i18n_verbose

I think these would be a good foundation for your patch as well, as you can use the
output() function for the desired cases.

Feel free to take these patches as part of your series or adapt
(or be inspired by) as needed.

Thanks,
Stefan


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

 merge-recursive.c | 169 +++++++++++++++++++++++++++++++++++++++++++++-
 submodule.c       | 168 +--------------------------------------------
 submodule.h       |   6 +-
 3 files changed, 170 insertions(+), 173 deletions(-)

-- 
2.17.0.255.g8bfb7c0704

[PATCH 1/2] submodule.c: move submodule merging to merge-recursive.c

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

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 merge-recursive.c | 166 +++++++++++++++++++++++++++++++++++++++++++++
 submodule.c       | 168 +---------------------------------------------
 submodule.h       |   6 +-
 3 files changed, 170 insertions(+), 170 deletions(-)

diff --git a/merge-recursive.c b/merge-recursive.c
index 0c0d48624da..700ba15bf88 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -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,
diff --git a/submodule.c b/submodule.c
index 74d35b25779..654089b3647 100644
--- a/submodule.c
+++ b/submodule.c
@@ -153,7 +153,8 @@ void stage_updated_gitmodules(struct index_state *istate)
 		die(_("staging updated .gitmodules failed"));
 }
 
-static int add_submodule_odb(const char *path)
+/* TODO: remove this function, use repo_submodule_init instead. */
+int add_submodule_odb(const char *path)
 {
 	struct strbuf objects_directory = STRBUF_INIT;
 	int ret = 0;
@@ -1701,171 +1702,6 @@ int submodule_move_head(const char *path,
 	return ret;
 }
 
-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);
-
-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;
-}
-
 /*
  * Embeds a single submodules git directory into the superprojects git dir,
  * non recursively.
diff --git a/submodule.h b/submodule.h
index e5526f6aaab..b96689ac0db 100644
--- a/submodule.h
+++ b/submodule.h
@@ -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,
-- 
2.17.0.255.g8bfb7c0704

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

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

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 merge-recursive.c | 33 +++++++++++++++------------------
 1 file changed, 15 insertions(+), 18 deletions(-)

diff --git a/merge-recursive.c b/merge-recursive.c
index 700ba15bf88..a4b91d17f87 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -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, 1, _("Found a possible merge resolution for the submodule:\n"));
 		print_commit((struct commit *) merges.objects[0].item);
-		fprintf(stderr,
+		output(o, 1, _(
 			"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:
-- 
2.17.0.255.g8bfb7c0704

Re: [PATCH 0/2] Submodule merging: i18n, verbosity

[Elijah Newren]

On Thu, May 10, 2018 at 2:19 PM, Stefan Beller <sbeller@google.com> wrote:
> Leif wrote:
>> Sure, let me know what to use instead and I’ll update and resubmit the patch.
>> Sure, but `MERGE_WARNING` prefixes all the messages with "Failed to
>> merge submodule“.
>
> I thought about replying and coming up with good reasons, but I wrote some
> patches instead.
>
> They can also be found at https://github.com/stefanbeller/git/tree/submodule_i18n_verbose
>
> I think these would be a good foundation for your patch as well, as you can use the
> output() function for the desired cases.
>
> Feel free to take these patches as part of your series or adapt
> (or be inspired by) as needed.

This is awesome.  In addition to the good reasons you gave, switching
merge_submodule() to use output() was one of several things on my todo
list since I think it'd be needed for remerge-diffs
(https://bugs.chromium.org/p/git/issues/detail?id=12) and might be
useful for merges in bare repos; thanks for tackling it.

Patches look good to me.  Having Leif's patch on top of these two
would be great.

Elijah

Re: [PATCH 0/2] Submodule merging: i18n, verbosity

[Stefan Beller]

Hi Elijah,

On Thu, May 10, 2018 at 5:04 PM, Elijah Newren <newren@gmail.com> wrote:
> On Thu, May 10, 2018 at 2:19 PM, Stefan Beller <sbeller@google.com> wrote:
>> Leif wrote:
>>> Sure, let me know what to use instead and I’ll update and resubmit the patch.
>>> Sure, but `MERGE_WARNING` prefixes all the messages with "Failed to
>>> merge submodule“.
>>
>> I thought about replying and coming up with good reasons, but I wrote some
>> patches instead.
>>
>> They can also be found at https://github.com/stefanbeller/git/tree/submodule_i18n_verbose
>>
>> I think these would be a good foundation for your patch as well, as you can use the
>> output() function for the desired cases.
>>
>> Feel free to take these patches as part of your series or adapt
>> (or be inspired by) as needed.
>
> This is awesome.  In addition to the good reasons you gave, switching
> merge_submodule() to use output() was one of several things on my todo
> list since I think it'd be needed for remerge-diffs
> (https://bugs.chromium.org/p/git/issues/detail?id=12) and might be
> useful for merges in bare repos; thanks for tackling it.

Thanks for the encouraging words!
The one nit I find on that series is that we need to rely on and export
the add_submodule_odb function as I want to get rid of that function
once the object store series has progressed far enough.

>
> Patches look good to me.  Having Leif's patch on top of these two
> would be great.

ok, Let's go with that.

Stefan

[PATCH 0/1] rebased: inform about auto submodule ff during merge

[Leif Middelschulte]

From: Leif Middelschulte <Leif.Middelschulte@gmail.com>

This patch is in response to Stefan Beller's Commit 0357af480
("merge-recursive: i18n submodule merge output and respect verbosity",
2018-05-10) and is based on the changes it provided.

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

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

-- 
2.15.1 (Apple Git-101)

[PATCH 1/1] Inform about fast-forwarding of submodules during merge

[Leif Middelschulte]

From: Leif Middelschulte <Leif.Middelschulte@gmail.com>

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

Signed-off-by: Leif Middelschulte <Leif.Middelschulte@gmail.com>
---
 merge-recursive.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/merge-recursive.c b/merge-recursive.c
index a4b91d17f..4a03044d1 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -1093,10 +1093,14 @@ 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);
+		output(o, 1, _("Note: Fast-forwarding submodule %s to the following commit"), path);
+		output_commit_title(o, commit_b);
 		return 1;
 	}
 	if (in_merge_bases(commit_b, commit_a)) {
 		oidcpy(result, a);
+		output(o, 1, _("Note: Fast-forwarding submodule %s to the following commit:"), path);
+		output_commit_title(o, commit_a);
 		return 1;
 	}
 
-- 
2.15.1 (Apple Git-101)

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

[Stefan Beller]

On Mon, May 14, 2018 at 1:57 PM, Leif Middelschulte
<leif.middelschulte@gmail.com> wrote:
> From: Leif Middelschulte <Leif.Middelschulte@gmail.com>
>
> 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)).
>
> Signed-off-by: Leif Middelschulte <Leif.Middelschulte@gmail.com>

Thanks for following up with a patch.
This looks good to me!

Thanks,
Stefan

> ---
>  merge-recursive.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/merge-recursive.c b/merge-recursive.c
> index a4b91d17f..4a03044d1 100644
> --- a/merge-recursive.c
> +++ b/merge-recursive.c
> @@ -1093,10 +1093,14 @@ 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);
> +               output(o, 1, _("Note: Fast-forwarding submodule %s to the following commit"), path);
> +               output_commit_title(o, commit_b);
>                 return 1;
>         }
>         if (in_merge_bases(commit_b, commit_a)) {
>                 oidcpy(result, a);
> +               output(o, 1, _("Note: Fast-forwarding submodule %s to the following commit:"), path);
> +               output_commit_title(o, commit_a);
>                 return 1;
>         }
>
> --
> 2.15.1 (Apple Git-101)
>

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

[Elijah Newren]

Hi Leif,

On Mon, May 14, 2018 at 1:57 PM, Leif Middelschulte
<leif.middelschulte@gmail.com> wrote:

Thanks for updating the patch on top of Stefan's series.  :-)

>         /* Case #1: a is contained in b or vice versa */
>         if (in_merge_bases(commit_a, commit_b)) {
>                 oidcpy(result, b);
> +               output(o, 1, _("Note: Fast-forwarding submodule %s to the following commit"), path);
> +               output_commit_title(o, commit_b);

Level 1 is for conflicts; I don't think this message should have
higher priority than "Auto-merging $PATH" for normal files, so it
needs to be 2 (or maybe 3, see below) rather than 1.  (The default
output level is 2, so it'd still be shown, but we do allow people to
remove informational message and just get conflicts by setting
GIT_MERGE_VERBOSITY to 1, or request extra information by setting it
higher)

Also, this two-line message seems somewhat verbose compared to the
other messages in merge_submdoule(), and when compared to the simple
"Auto-merging $PATH" we do for normal files.  The multi-line nature of
it particularly strikes me; the merge-recursive code has generally
avoided multi-line messages even for conflicts.

In comparison, your original patch just had ("Fast-forwarding
submodule %s", path).

Maybe you could "if (show(o, 3)) { output your current message } else
{ output the simpler message }" ?  Or is this verbosity warranted for
submodules at the default print level?

I'm not a heavy user of submodules, so I may need to get others to
weigh in on the verbosity and multi-line aspects, but I wanted to at
least flag this as somewhat surprising to me.


Elijah

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

[Elijah Newren]

I know I said the patches looked okay earlier, but I just noticed something...

On Thu, May 10, 2018 at 2:19 PM, Stefan Beller <sbeller@google.com> wrote:

>         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);

We allow folks to set GIT_MERGE_VERBOSITY to change how much output
they get.  A setting of 1 should only show conflicts or major
warnings.  2 is the default and adds a few more messages (e.g.
"Auto-merging $PATH", "Adding $PATH" for one-sided adds, etc.), higher
levels show even more.

Anyway this output message is correct to use level 1 since this is a
conflict, but...

> +               output(o, 1, _("Found a possible merge resolution for the submodule:\n"));

I think this should use level 2.

>                 print_commit((struct commit *) merges.objects[0].item);
> -               fprintf(stderr,
> +               output(o, 1, _(
>                        "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);

and so should this one (in fact, I'm tempted to say these last two
should use level 3, but since it looks like a command users may have
difficulty finding on their own, I'm okay with going with 2).

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

[Junio C Hamano]

Elijah Newren <newren@gmail.com> writes:

> Hi Leif,
>
> On Mon, May 14, 2018 at 1:57 PM, Leif Middelschulte
> <leif.middelschulte@gmail.com> wrote:
>
> Thanks for updating the patch on top of Stefan's series.  :-)
>
>>         /* Case #1: a is contained in b or vice versa */
>>         if (in_merge_bases(commit_a, commit_b)) {
>>                 oidcpy(result, b);
>> +               output(o, 1, _("Note: Fast-forwarding submodule %s to the following commit"), path);
>> +               output_commit_title(o, commit_b);
>
> Level 1 is for conflicts; I don't think this message should have
> higher priority than "Auto-merging $PATH" for normal files, so it
> needs to be 2 (or maybe 3, see below) rather than 1.  (The default
> output level is 2, so it'd still be shown, but we do allow people to
> remove informational message and just get conflicts by setting
> GIT_MERGE_VERBOSITY to 1, or request extra information by setting it
> higher)
>
> Also, this two-line message seems somewhat verbose compared to the
> other messages in merge_submdoule(), and when compared to the simple
> "Auto-merging $PATH" we do for normal files.  The multi-line nature of
> it particularly strikes me; the merge-recursive code has generally
> avoided multi-line messages even for conflicts.
>
> In comparison, your original patch just had ("Fast-forwarding
> submodule %s", path).

FWIW, I share both of your surprises.  Between level 2 and 3, after
skimming merge-recursive.c for existing use of output levels, I
think the situation for non-submodule merges that is closest to
these two cases the patch covers for submodules is probably the
message given when content merge happened to end up with what we
already had.  It is part of a normal merge operation that is not a
singificant event in the larger picture, yet it is rather rare and
interesting when you are curious on events that occur infrequently.

So a one-liner message as everybody else emitted at level 3 or more
verbose would probably be a good balance with the remainder of the
system, I would think.

Thanks for a review.

[PATCH 1/1] Inform about fast-forwarding of submodules during merge

[Leif Middelschulte]

From: Leif Middelschulte <Leif.Middelschulte@gmail.com>

Silent fast-forwarding might lead to inconveniences in cases where
submodules are expected to have a certain revision, because 'more recent'
(therefore fast-forwardable) versions might break behavior/contain regressions.

A use-case is the integration (merge) phase as part of the feature-centric
'git-flow' workflow [0]. I.e. a feature might be well-tested with a certain
submodule revision, but break because of regressions (or changes in general)
within an updated version of the sourced submodule.

This change tries to support the integrator by telling her about another possible
source of unexpected behavior (differing submodule versions) she might see
during integration tests.

[0] http://nvie.com/posts/a-successful-git-branching-model/

Signed-off-by: Leif Middelschulte <Leif.Middelschulte@gmail.com>
---
 merge-recursive.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/merge-recursive.c b/merge-recursive.c
index a4b91d17f..e2c99924d 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -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, 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));
+		else
+			; /* no output */
+
 		return 1;
 	}
 	if (in_merge_bases(commit_b, commit_a)) {
 		oidcpy(result, a);
+		if (show(o, 3)) {
+			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));
+		else
+			; /* no output */
+
 		return 1;
 	}
 
-- 
2.15.1 (Apple Git-101)

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

[Elijah Newren]

Hi Leif,

On Fri, May 18, 2018 at 12:48 PM, Leif Middelschulte
<leif.middelschulte@gmail.com> wrote:
> From: Leif Middelschulte <Leif.Middelschulte@gmail.com>
>
> Silent fast-forwarding might lead to inconveniences in cases where
> submodules are expected to have a certain revision, because 'more recent'
> (therefore fast-forwardable) versions might break behavior/contain regressions.
>
> A use-case is the integration (merge) phase as part of the feature-centric
> 'git-flow' workflow [0]. I.e. a feature might be well-tested with a certain
> submodule revision, but break because of regressions (or changes in general)
> within an updated version of the sourced submodule.
>
> This change tries to support the integrator by telling her about another possible
> source of unexpected behavior (differing submodule versions) she might see
> during integration tests.

Thanks for continuing to push on this.  This looks good so far (to
me), but I was also hoping to see the analogy between these messages
and "Auto-merging $FILE" for regular files mentioned.  Both Junio[1]
and I[2] pointed out this similarity, and I think this
similarity/analogy is useful additional motivation for making this
change.

[1] https://public-inbox.org/git/xmqqo9hg7554.fsf@gitster-ct.c.googlers.com/
[2] https://public-inbox.org/git/CABPp-BGaibCPWuCnaX5Af=sv-2zvyhNcupT+-PkxHDfJBg_Vbw@mail.gmail.com/


> +               } else if (show(o, 2))
> +                       output(o, 2, _("Fast-forwarding submodule %s to %s"), path, oid_to_hex(b));
...
> +               } else if (show(o, 2))
> +                       output(o, 2, _("Fast-forwarding submodule %s to %s"), path, oid_to_hex(a));

Also, by analogy to the "Auto-merging $FILE" comparison, the "to %s"
on these two lines feels out of place.  Users can just look at the
submodule to see what it was updated to.  In a sea of output from
merging, this extra detail feels like noise for the standard use-case,
unless I'm misunderstanding how submodules are special.  Junio also
commented on this in the same email referenced above (at [1]).  Is
there a reason this is an important piece of the message for you to be
shown at the standard merge verbosity?

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

[Junio C Hamano]

Elijah Newren <newren@gmail.com> writes:

> Thanks for continuing to push on this.  This looks good so far (to
> me), but I was also hoping to see the analogy between these messages
> and "Auto-merging $FILE" for regular files mentioned.  Both Junio[1]
> and I[2] pointed out this similarity, and I think this
> similarity/analogy is useful additional motivation for making this
> change.

... meaning that it should be discussed and named as the primary
reason why this change is a good idea?

Re-reading what Leif wrote in the first paragraph, I tend to think
that "the more recent version may break us" Leif gives is not a
particularly convincing one.  After all, if we did not change the
commit bound at a submodule since we forked, while they changed it
to something else (either old or new), even though our changes may
have been fully tested with the version of the submodule we have
been testing with, it may break with the version the merged branch
has been using.  Such an update is cleanly and silently resolved at
the tree-level three-way merge, but the risk of breakage is no
different to the case this patch adds new notices to.

More importantly, the same "the changes we made may get broken by
changes in areas that are textually unrelated they made" will happen
without submodules.  Content-level three-way merges that resolves
cleanly at the textual level may need to get semantic adjustment.
Do we treat clean 3-way content merges as suspicious and give a
similar warning?  That smells like madness.

But as you said, we give "Auto-merging $FILE" notice to clean 3-way
merge at the content-level for normal files, and there is no good
reason why we should not do the same for submodules when one
fast-forwards to the other, which is an analogue to the
content-level 3-way merge where one branch's version is a superset
of the other ones.  And that is quite a convincing reason why a new
"Auto-merging $SUBMODULE" notice is a good idea.

> ...
> Also, by analogy to the "Auto-merging $FILE" comparison, the "to %s"
> on these two lines feels out of place.  Users can just look at the
> submodule to see what it was updated to.  In a sea of output from
> merging, this extra detail feels like noise for the standard use-case,
> unless I'm misunderstanding how submodules are special.

Now you meantion it, that part of the message does look more like a
debugging aid than a feature that helps actual end-users.  After
all, if our side did not change the commit recorded for the
submodule while their side changed, we do not report the result of
such a tree-level three-way merge that takes what commit they had at
their tip.