git@vger.kernel.org mailing list mirror (one of many)
 help / Atom feed
* [PATCHv2 0/3] Reroll of sb/submodule-merge-in-merge-recursive
@ 2018-05-15 20:00 Stefan Beller
  2018-05-15 20:00 ` [PATCH] git-submodule.sh: try harder to fetch a submodule Stefan Beller
                   ` (6 more replies)
  0 siblings, 7 replies; 26+ messages in thread
From: Stefan Beller @ 2018-05-15 20:00 UTC (permalink / raw)
  To: git, leif.middelschulte; +Cc: gitster, newren, 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?

Thanks,
Stefan

[1] https://public-inbox.org/git/20180510211917.138518-1-sbeller@google.com/
[2] https://public-inbox.org/git/20180514205737.21313-2-leif.middelschulte@gmail.com/
[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
    @@ -20,7 +20,6 @@
         This commit is best viewed with --color-moved.
     
         Signed-off-by: Stefan Beller <sbeller@google.com>
    -    Signed-off-by: Junio C Hamano <gitster@pobox.com>
     
     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 <sbeller@google.com>
    -    Signed-off-by: Junio C Hamano <gitster@pobox.com>
     
     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

^ permalink raw reply	[flat|nested] 26+ messages in thread

* [PATCH] git-submodule.sh: try harder to fetch a submodule
  2018-05-15 20:00 [PATCHv2 0/3] Reroll of sb/submodule-merge-in-merge-recursive Stefan Beller
@ 2018-05-15 20:00 ` Stefan Beller
  2018-05-15 20:00 ` [PATCH] grep: handle corrupt index files early Stefan Beller
                   ` (5 subsequent siblings)
  6 siblings, 0 replies; 26+ messages in thread
From: Stefan Beller @ 2018-05-15 20:00 UTC (permalink / raw)
  To: git, leif.middelschulte; +Cc: gitster, newren, Stefan Beller

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

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 git-submodule.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/git-submodule.sh b/git-submodule.sh
index 24914963ca2..00fcd69138f 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -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
-- 
2.17.0.582.gccdcbd54c44.dirty


^ permalink raw reply	[flat|nested] 26+ messages in thread

* [PATCH] grep: handle corrupt index files early
  2018-05-15 20:00 [PATCHv2 0/3] Reroll of sb/submodule-merge-in-merge-recursive Stefan Beller
  2018-05-15 20:00 ` [PATCH] git-submodule.sh: try harder to fetch a submodule Stefan Beller
@ 2018-05-15 20:00 ` Stefan Beller
  2018-05-15 20:00 ` [PATCH 1/3] submodule.c: move submodule merging to merge-recursive.c Stefan Beller
                   ` (4 subsequent siblings)
  6 siblings, 0 replies; 26+ messages in thread
From: Stefan Beller @ 2018-05-15 20:00 UTC (permalink / raw)
  To: git, leif.middelschulte; +Cc: gitster, newren, Stefan Beller

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

Signed-off-by: Stefan Beller <sbeller@google.com>
---

Found while reviewing the series
https://public-inbox.org/git/20180514105823.8378-1-ao2@ao2.it/

 builtin/grep.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/builtin/grep.c b/builtin/grep.c
index 6e7bc76785a..69f0743619f 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -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];
-- 
2.17.0.582.gccdcbd54c44.dirty


^ permalink raw reply	[flat|nested] 26+ messages in thread

* [PATCH 1/3] submodule.c: move submodule merging to merge-recursive.c
  2018-05-15 20:00 [PATCHv2 0/3] Reroll of sb/submodule-merge-in-merge-recursive Stefan Beller
  2018-05-15 20:00 ` [PATCH] git-submodule.sh: try harder to fetch a submodule Stefan Beller
  2018-05-15 20:00 ` [PATCH] grep: handle corrupt index files early Stefan Beller
@ 2018-05-15 20:00 ` Stefan Beller
  2018-05-15 20:00 ` [PATCH 2/3] merge-recursive: i18n submodule merge output and respect verbosity Stefan Beller
                   ` (3 subsequent siblings)
  6 siblings, 0 replies; 26+ messages in thread
From: Stefan Beller @ 2018-05-15 20:00 UTC (permalink / raw)
  To: git, leif.middelschulte; +Cc: gitster, newren, 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.582.gccdcbd54c44.dirty


^ permalink raw reply	[flat|nested] 26+ messages in thread

* [PATCH 2/3] merge-recursive: i18n submodule merge output and respect verbosity
  2018-05-15 20:00 [PATCHv2 0/3] Reroll of sb/submodule-merge-in-merge-recursive Stefan Beller
                   ` (2 preceding siblings ...)
  2018-05-15 20:00 ` [PATCH 1/3] submodule.c: move submodule merging to merge-recursive.c Stefan Beller
@ 2018-05-15 20:00 ` Stefan Beller
  2018-05-16  1:17   ` Junio C Hamano
  2018-05-15 20:00 ` [PATCH 3/3] Inform about fast-forwarding of submodules during merge Stefan Beller
                   ` (2 subsequent siblings)
  6 siblings, 1 reply; 26+ messages in thread
From: Stefan Beller @ 2018-05-15 20:00 UTC (permalink / raw)
  To: git, leif.middelschulte; +Cc: gitster, newren, 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..0571919ee0a 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, 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:
-- 
2.17.0.582.gccdcbd54c44.dirty


^ permalink raw reply	[flat|nested] 26+ messages in thread

* [PATCH 3/3] Inform about fast-forwarding of submodules during merge
  2018-05-15 20:00 [PATCHv2 0/3] Reroll of sb/submodule-merge-in-merge-recursive Stefan Beller
                   ` (3 preceding siblings ...)
  2018-05-15 20:00 ` [PATCH 2/3] merge-recursive: i18n submodule merge output and respect verbosity Stefan Beller
@ 2018-05-15 20:00 ` Stefan Beller
  2018-05-16  1:36   ` Elijah Newren
                     ` (2 more replies)
  2018-05-15 20:02 ` [PATCHv2 0/3] Reroll of sb/submodule-merge-in-merge-recursive Stefan Beller
  2018-05-15 20:15 ` Leif Middelschulte
  6 siblings, 3 replies; 26+ messages in thread
From: Stefan Beller @ 2018-05-15 20:00 UTC (permalink / raw)
  To: git, leif.middelschulte
  Cc: gitster, newren, Leif Middelschulte, Stefan Beller

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>
Signed-off-by: Stefan Beller <sbeller@google.com>
---
 merge-recursive.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/merge-recursive.c b/merge-recursive.c
index 0571919ee0a..29a430c418a 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, 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;
 	}
 
-- 
2.17.0.582.gccdcbd54c44.dirty


^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCHv2 0/3] Reroll of sb/submodule-merge-in-merge-recursive
  2018-05-15 20:00 [PATCHv2 0/3] Reroll of sb/submodule-merge-in-merge-recursive Stefan Beller
                   ` (4 preceding siblings ...)
  2018-05-15 20:00 ` [PATCH 3/3] Inform about fast-forwarding of submodules during merge Stefan Beller
@ 2018-05-15 20:02 ` Stefan Beller
  2018-05-15 20:15 ` Leif Middelschulte
  6 siblings, 0 replies; 26+ messages in thread
From: Stefan Beller @ 2018-05-15 20:02 UTC (permalink / raw)
  To: git, Leif Middelschulte; +Cc: Junio C Hamano, Elijah Newren, 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

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCHv2 0/3] Reroll of sb/submodule-merge-in-merge-recursive
  2018-05-15 20:00 [PATCHv2 0/3] Reroll of sb/submodule-merge-in-merge-recursive Stefan Beller
                   ` (5 preceding siblings ...)
  2018-05-15 20:02 ` [PATCHv2 0/3] Reroll of sb/submodule-merge-in-merge-recursive Stefan Beller
@ 2018-05-15 20:15 ` Leif Middelschulte
  2018-05-15 20:49   ` Stefan Beller
  6 siblings, 1 reply; 26+ messages in thread
From: Leif Middelschulte @ 2018-05-15 20:15 UTC (permalink / raw)
  To: Stefan Beller, git; +Cc: newren, gitster

Hello Stefan,

thank you once again for your effort.

Am 15. Mai 2018 um 22:00:34, Stefan Beller
(sbeller@google.com(mailto:sbeller@google.com)) schrieb:

> 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

>
> Thanks,
> Stefan
>
> [1] https://public-inbox.org/git/20180510211917.138518-1-sbeller@google.com/
> [2] https://public-inbox.org/git/20180514205737.21313-2-leif.middelschulte@gmail.com/
> [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
> @@ -20,7 +20,6 @@
> This commit is best viewed with --color-moved.
>
> Signed-off-by: Stefan Beller
> - Signed-off-by: Junio C Hamano
>
> 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
> - Signed-off-by: Junio C Hamano
>
> 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

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCHv2 0/3] Reroll of sb/submodule-merge-in-merge-recursive
  2018-05-15 20:15 ` Leif Middelschulte
@ 2018-05-15 20:49   ` Stefan Beller
  0 siblings, 0 replies; 26+ messages in thread
From: Stefan Beller @ 2018-05-15 20:49 UTC (permalink / raw)
  To: Leif Middelschulte; +Cc: git, Elijah Newren, Junio C Hamano

On Tue, May 15, 2018 at 1:15 PM, Leif Middelschulte
<leif.middelschulte@gmail.com> wrote:
> Hello Stefan,
>
> thank you once again for your effort.
>
> Am 15. Mai 2018 um 22:00:34, Stefan Beller
> (sbeller@google.com(mailto:sbeller@google.com)) schrieb:
>
>> 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?

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

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 2/3] merge-recursive: i18n submodule merge output and respect verbosity
  2018-05-15 20:00 ` [PATCH 2/3] merge-recursive: i18n submodule merge output and respect verbosity Stefan Beller
@ 2018-05-16  1:17   ` Junio C Hamano
  0 siblings, 0 replies; 26+ messages in thread
From: Junio C Hamano @ 2018-05-16  1:17 UTC (permalink / raw)
  To: Stefan Beller; +Cc: git, leif.middelschulte, newren

Stefan Beller <sbeller@google.com> writes:

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

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 3/3] Inform about fast-forwarding of submodules during merge
  2018-05-15 20:00 ` [PATCH 3/3] Inform about fast-forwarding of submodules during merge Stefan Beller
@ 2018-05-16  1:36   ` Elijah Newren
  2018-05-16  1:36   ` Junio C Hamano
  2018-05-16  1:42   ` Junio C Hamano
  2 siblings, 0 replies; 26+ messages in thread
From: Elijah Newren @ 2018-05-16  1:36 UTC (permalink / raw)
  To: Stefan Beller; +Cc: Git Mailing List, Leif Middelschulte, Junio C Hamano

On Tue, May 15, 2018 at 1:00 PM, Stefan Beller <sbeller@google.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>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>  merge-recursive.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
>
> diff --git a/merge-recursive.c b/merge-recursive.c
> index 0571919ee0a..29a430c418a 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, 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).

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

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

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 3/3] Inform about fast-forwarding of submodules during merge
  2018-05-15 20:00 ` [PATCH 3/3] Inform about fast-forwarding of submodules during merge Stefan Beller
  2018-05-16  1:36   ` Elijah Newren
@ 2018-05-16  1:36   ` Junio C Hamano
  2018-05-16  1:42   ` Junio C Hamano
  2 siblings, 0 replies; 26+ messages in thread
From: Junio C Hamano @ 2018-05-16  1:36 UTC (permalink / raw)
  To: Stefan Beller; +Cc: git, leif.middelschulte, newren

Stefan Beller <sbeller@google.com> writes:

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

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

perhaps?

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

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 3/3] Inform about fast-forwarding of submodules during merge
  2018-05-15 20:00 ` [PATCH 3/3] Inform about fast-forwarding of submodules during merge Stefan Beller
  2018-05-16  1:36   ` Elijah Newren
  2018-05-16  1:36   ` Junio C Hamano
@ 2018-05-16  1:42   ` Junio C Hamano
  2018-06-04 18:48     ` [PATCH v4 0/1] merge-recursive: give notice when submodule commit gets fast-forwarded Leif Middelschulte
  2 siblings, 1 reply; 26+ messages in thread
From: Junio C Hamano @ 2018-05-16  1:42 UTC (permalink / raw)
  To: Stefan Beller; +Cc: git, leif.middelschulte, newren

Stefan Beller <sbeller@google.com> writes:

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

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.

^ permalink raw reply	[flat|nested] 26+ messages in thread

* [PATCH v4 0/1] merge-recursive: give notice when submodule commit gets fast-forwarded
  2018-05-16  1:42   ` Junio C Hamano
@ 2018-06-04 18:48     ` Leif Middelschulte
  2018-06-04 18:48       ` [PATCH 1/1] " Leif Middelschulte
  0 siblings, 1 reply; 26+ messages in thread
From: Leif Middelschulte @ 2018-06-04 18:48 UTC (permalink / raw)
  To: gitster; +Cc: git, leif.middelschulte, newren, sbeller, Leif Middelschulte

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

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/xmqqo9hg7554.fsf@gitster-ct.c.googlers.com/#t
[1] https://public-inbox.org/git/xmqqzi0t1waf.fsf@gitster-ct.c.googlers.com/

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

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

-- 
2.15.1 (Apple Git-101)


^ permalink raw reply	[flat|nested] 26+ messages in thread

* [PATCH 1/1] merge-recursive: give notice when submodule commit gets fast-forwarded
  2018-06-04 18:48     ` [PATCH v4 0/1] merge-recursive: give notice when submodule commit gets fast-forwarded Leif Middelschulte
@ 2018-06-04 18:48       ` " Leif Middelschulte
  2018-06-07  5:22         ` Elijah Newren
  0 siblings, 1 reply; 26+ messages in thread
From: Leif Middelschulte @ 2018-06-04 18:48 UTC (permalink / raw)
  To: gitster; +Cc: git, leif.middelschulte, newren, sbeller, Leif Middelschulte

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

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.

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

diff --git a/merge-recursive.c b/merge-recursive.c
index a4b91d17f..0990a135b 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -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;
 	}
 
-- 
2.15.1 (Apple Git-101)


^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 1/1] merge-recursive: give notice when submodule commit gets fast-forwarded
  2018-06-04 18:48       ` [PATCH 1/1] " Leif Middelschulte
@ 2018-06-07  5:22         ` Elijah Newren
  2018-06-11 17:31           ` [PATCH] merge-submodule: reduce output verbosity Leif Middelschulte
  0 siblings, 1 reply; 26+ messages in thread
From: Elijah Newren @ 2018-06-07  5:22 UTC (permalink / raw)
  To: Leif Middelschulte; +Cc: Junio C Hamano, Git Mailing List, Stefan Beller

Hi Leif,

On Mon, Jun 4, 2018 at 11:48 AM, Leif Middelschulte
<leif.middelschulte@gmail.com> wrote:
> From: Leif Middelschulte <Leif.Middelschulte@gmail.com>
>
> 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/xmqqwove4pzo.fsf@gitster-ct.c.googlers.com/


> +                       output(o, 2, _("Auto-merging %s"), path);
...
> +                       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?

^ permalink raw reply	[flat|nested] 26+ messages in thread

* [PATCH] merge-submodule: reduce output verbosity
  2018-06-07  5:22         ` Elijah Newren
@ 2018-06-11 17:31           ` Leif Middelschulte
  2018-06-11 18:04             ` Junio C Hamano
  0 siblings, 1 reply; 26+ messages in thread
From: Leif Middelschulte @ 2018-06-11 17:31 UTC (permalink / raw)
  To: newren; +Cc: git, gitster, leif.middelschulte, sbeller, Leif Middelschulte

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

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

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

diff --git a/merge-recursive.c b/merge-recursive.c
index ac27abbd4..5eb907f46 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -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 */
 
-- 
2.15.1 (Apple Git-101)


^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH] merge-submodule: reduce output verbosity
  2018-06-11 17:31           ` [PATCH] merge-submodule: reduce output verbosity Leif Middelschulte
@ 2018-06-11 18:04             ` Junio C Hamano
  2018-06-11 19:07               ` Leif Middelschulte
  0 siblings, 1 reply; 26+ messages in thread
From: Junio C Hamano @ 2018-06-11 18:04 UTC (permalink / raw)
  To: Leif Middelschulte; +Cc: newren, git, sbeller

Leif Middelschulte <leif.middelschulte@gmail.com> writes:

> From: Leif Middelschulte <Leif.Middelschulte@gmail.com>
>
> The output shall behave more similar to ordinary file merges' output to provide
> a more consistent user experience.
>
> Signed-off-by: Leif Middelschulte <Leif.Middelschulte@gmail.com>
> ---
>  merge-recursive.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Thanks, both.  Very much appreciated.

>
> diff --git a/merge-recursive.c b/merge-recursive.c
> index ac27abbd4..5eb907f46 100644
> --- a/merge-recursive.c
> +++ b/merge-recursive.c
> @@ -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 */

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH] merge-submodule: reduce output verbosity
  2018-06-11 18:04             ` Junio C Hamano
@ 2018-06-11 19:07               ` Leif Middelschulte
  0 siblings, 0 replies; 26+ messages in thread
From: Leif Middelschulte @ 2018-06-11 19:07 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: newren, sbeller, git

Hello,

Am 11. Juni 2018 um 20:04:19, Junio C Hamano
(gitster@pobox.com(mailto:gitster@pobox.com)) schrieb:

> Leif Middelschulte writes:
>
> > From: Leif Middelschulte
> >
> > The output shall behave more similar to ordinary file merges' output to provide
> > a more consistent user experience.
> >
> > 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
>
> >
> > diff --git a/merge-recursive.c b/merge-recursive.c
> > index ac27abbd4..5eb907f46 100644
> > --- a/merge-recursive.c
> > +++ b/merge-recursive.c
> > @@ -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 */

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH] git-submodule.sh: try harder to fetch a submodule
  2018-05-15 19:07     ` Stefan Beller
  2018-05-15 19:40       ` Stefan Beller
@ 2018-05-15 20:04       ` Jonathan Nieder
  1 sibling, 0 replies; 26+ messages in thread
From: Jonathan Nieder @ 2018-05-15 20:04 UTC (permalink / raw)
  To: Stefan Beller; +Cc: Junio C Hamano, git

Stefan Beller wrote:

> I'll resend it with a warning (using say()).

Thanks, makes sense.

> I think we have 2 bugs and this is merely fixing the second bug.

I'm fearing that there are more than two.

[...]
>   $ git init confused-head
>   $ (cd confused-head && git branch test \
>         $(git commit-tree $(git write-tree) -m test))
>   $ git clone --no-checkout  --depth=1 \
>         --separate-git-dir=test.git confused-head/.git test
> Cloning into 'test'...
> warning: --depth is ignored in local clones; use file:// instead.
> done.
>
>   $ git -C test.git config remote.origin.fetch
>   $ echo $?
> 1
>
> (A) Despite the warning of --depth having no impact, the
>   omission thereof changes the repository state.
> (B) There is no remote.origin.fetch configuration, which
>   is weird. See builtin/clone.c:830, that states for this case:

I can reproduce the issue without submodules and without --local,
as follows:

	git init --bare empty.git
	git init --bare almost-empty.git
	git -C ~/src/git push $(pwd)/almost-empty HEAD:refs/heads/upstream

	git clone --single-branch file://$(pwd)/empty.git
	git clone --single-branch file://$(pwd)/almost-empty.git

	git -C almost-empty.git branch -D upstream

	git -C empty fetch
	git -C almost-empty fetch

Expected result:
Both fetches succeed.

Actual result:
First fetch succeeds, second produces
"fatal: Couldn't find remote ref HEAD".

Note that empty.git and almost-empty.git are basically identical.
The difference instead lies in the clones' .git/config files:

diff --git 1/empty/.git/config 2/almost-empty/.git/config
index b51bb0d..ee21198 100644
--- 1/empty/.git/config
+++ 2/almost-empty/.git/config
@@ -4,7 +4,4 @@
        bare = false
        logallrefupdates = true
 [remote "origin"]
-       url = file:///tmp/t/empty.git
-[branch "master"]
-       remote = origin
-       merge = refs/heads/master
+       url = file:///tmp/t/almost-empty.git

Thanks,
Jonathan

^ permalink raw reply	[flat|nested] 26+ messages in thread

* [PATCH] git-submodule.sh: try harder to fetch a submodule
  2018-05-15 19:07     ` Stefan Beller
@ 2018-05-15 19:40       ` Stefan Beller
  2018-05-15 20:04       ` Jonathan Nieder
  1 sibling, 0 replies; 26+ messages in thread
From: Stefan Beller @ 2018-05-15 19:40 UTC (permalink / raw)
  To: sbeller; +Cc: git, gitster, jrnieder

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

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 git-submodule.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/git-submodule.sh b/git-submodule.sh
index 24914963ca2..00fcd69138f 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -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
-- 
2.17.0.582.gccdcbd54c44.dirty


^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH] git-submodule.sh: try harder to fetch a submodule
  2018-05-12  0:03   ` Junio C Hamano
@ 2018-05-15 19:07     ` Stefan Beller
  2018-05-15 19:40       ` Stefan Beller
  2018-05-15 20:04       ` Jonathan Nieder
  0 siblings, 2 replies; 26+ messages in thread
From: Stefan Beller @ 2018-05-15 19:07 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Jonathan Nieder, git

On Fri, May 11, 2018 at 5:03 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> A more typical example would be if the ref simply doesn't exist (i.e.,
>> is a branch yet to be born).
>
> Indeed this is interesting.  At first glance I thought this was
> about underlying "git clone" failing to grab things from a
> repository with unborn HEAD, but that part works perfectly OK.

ok.

> So it probably should be more like
>
>         guard1 || action1 || warn
>         guard2 || action2 || die
>
> so that no matter what the outcome of the action1 is, the second set
> gets executed.
>

I'll resend it with a warning (using say()).

I think we have 2 bugs and this is merely fixing the second bug.

The first bug:
We had a call chain as
  git clone --recurse-submodules=<path spec>
    -> git submodule update --init --recursive $(pathspec)
      -> git submodule--helper update-clone # will clone
        -> git submodule helper clone
          -> git clone --no-checkout --separate-git-dir ...

The call to the "git clone" produces an interesting
submodule state:

  $ git init confused-head
  $ (cd confused-head && git branch test \
        $(git commit-tree $(git write-tree) -m test))
  $ git clone --no-checkout  --depth=1 \
        --separate-git-dir=test.git confused-head/.git test
Cloning into 'test'...
warning: --depth is ignored in local clones; use file:// instead.
done.

  $ git -C test.git config remote.origin.fetch
  $ echo $?
1

(A) Despite the warning of --depth having no impact, the
  omission thereof changes the repository state.
(B) There is no remote.origin.fetch configuration, which
  is weird. See builtin/clone.c:830, that states for this case:

    /*
     * otherwise, the next "git fetch" will
     * simply fetch from HEAD without updating
     * any remote-tracking branch, which is what
     * we want.
     */

I disagree as the next fetch will be confused
(HEAD is not advertised on the next ls-remote)

The patch that is under discussion here is merely
papering over the effect of having no fetch spec,
by allowing the second fetch (fetching the sha1 directly)
to run, which ignores the configuration as a refspec is
given.

However it is still a bug, as such repositories are out there,
which is why I said there are 2 bugs initially. It's just that
the first bug enables the second bug.

Thanks,
Stefan

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH] git-submodule.sh: try harder to fetch a submodule
  2018-05-11 23:28 ` Jonathan Nieder
  2018-05-11 23:42   ` Stefan Beller
@ 2018-05-12  0:03   ` Junio C Hamano
  2018-05-15 19:07     ` Stefan Beller
  1 sibling, 1 reply; 26+ messages in thread
From: Junio C Hamano @ 2018-05-12  0:03 UTC (permalink / raw)
  To: Jonathan Nieder; +Cc: Stefan Beller, git

Jonathan Nieder <jrnieder@gmail.com> writes:

>> HEAD is allowed by the protocol spec and would happen, if HEAD points at a
>> ref, that this user cannot see (due to ACLs for example).
>
> A more typical example would be if the ref simply doesn't exist (i.e.,
> is a branch yet to be born).

Indeed this is interesting.  At first glance I thought this was
about underlying "git clone" failing to grab things from a
repository with unborn HEAD, but that part works perfectly OK.  And
if this failed clone were a full-repository clone that tried to grab
even HEAD, then it is likely that we got the tip we need to populate
the submodule's working tree (or the remote is useless for that in
the first place).  So the "allow to try even harder" is probably a
good direction to go in.

>>  				# is not reachable from a ref.
>>  				is_tip_reachable "$sm_path" "$sha1" ||
>>  				fetch_in_submodule "$sm_path" $depth ||
>
> Is keeping the '||' at the end of this line intended?

Good question.  It used to be

	guard1 || action1 || die
	guard2 || action2 || die

Even after a successful exit from "action1", the code used to try
the second attempt, and the patch is removing the first die, making
the whole thing into

	guard1 || action1 ||
	guard2 || action2 || die

which suggests a grave regression, doesn't it?  "action1" (a whole
repository fetch) may not pull down the needed commit even the fetch
operation itself may succeed, in which case "guard2" notices that
the tip is still not here and "action2" (an exact SHA-1 fetch) tries
to pull down the exact thing as the last resort.

So it probably should be more like

	guard1 || action1 || warn
	guard2 || action2 || die

so that no matter what the outcome of the action1 is, the second set
gets executed.



^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH] git-submodule.sh: try harder to fetch a submodule
  2018-05-11 23:28 ` Jonathan Nieder
@ 2018-05-11 23:42   ` Stefan Beller
  2018-05-12  0:03   ` Junio C Hamano
  1 sibling, 0 replies; 26+ messages in thread
From: Stefan Beller @ 2018-05-11 23:42 UTC (permalink / raw)
  To: Jonathan Nieder; +Cc: Junio C Hamano, git

On Fri, May 11, 2018 at 4:28 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Hi,
>
> Stefan Beller wrote:
>
>> 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.
>
> Interesting.
>
> I think what would help most is an example set of commands I can use
> to reproduce this (bonus points if in the form of a test).

I tried coming up with a test in git-core that produces a remote similar to
Gerrit, and let me tell you, it's not Git that is weird here. ;)

>> > If $sha1 was not part of the default fetch ... fail ourselves here
>> assumes that the fetch_in_submodule only fails when the serverside does
>
> I'm having some trouble with the formatting here.  Is the part
> preceded by '>' a quote, and if so a quote from what?

The quote is from fb43e31f2b4.

>> 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. Not advertising
>
> nit: it can be useful to have a blank line before and after such
> example output to help both my eyes and tools like "git log
> --format='%w(100)%b'" to understand the formatting.

Why would you use this formatting?

>
>> HEAD is allowed by the protocol spec and would happen, if HEAD points at a
>> ref, that this user cannot see (due to ACLs for example).
>
> A more typical example would be if the ref simply doesn't exist (i.e.,
> is a branch yet to be born).

Oh, I checked that but not for the submodule case, let me check that.

>> diff --git a/git-submodule.sh b/git-submodule.sh
>> index 24914963ca2..13b378a6c8f 100755
>> --- a/git-submodule.sh
>> +++ b/git-submodule.sh
>> @@ -614,7 +614,6 @@ cmd_update()
>>                               # is not reachable from a ref.
>>                               is_tip_reachable "$sm_path" "$sha1" ||
>>                               fetch_in_submodule "$sm_path" $depth ||
>
> Is keeping the '||' at the end of this line intended?

yes, as we only want to run the code below if there was some error.

>> -                             die "$(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
>>                               is_tip_reachable "$sm_path" "$sha1" ||
>>                               fetch_in_submodule "$sm_path" $depth "$sha1" ||
>>                               die "$(eval_gettext "Fetched in submodule path '\$displaypath', but it did not contain \$sha1. Direct fetching of that commit failed.")"
>
> Should this error message be changed?

I don't think so?

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH] git-submodule.sh: try harder to fetch a submodule
  2018-05-11 23:17 [PATCH] git-submodule.sh: try harder to fetch a submodule Stefan Beller
@ 2018-05-11 23:28 ` Jonathan Nieder
  2018-05-11 23:42   ` Stefan Beller
  2018-05-12  0:03   ` Junio C Hamano
  0 siblings, 2 replies; 26+ messages in thread
From: Jonathan Nieder @ 2018-05-11 23:28 UTC (permalink / raw)
  To: Stefan Beller; +Cc: gitster, git

Hi,

Stefan Beller wrote:

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

Interesting.

I think what would help most is an example set of commands I can use
to reproduce this (bonus points if in the form of a test).

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

I'm having some trouble with the formatting here.  Is the part
preceded by '>' a quote, and if so a quote from what?

> 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. Not advertising

nit: it can be useful to have a blank line before and after such
example output to help both my eyes and tools like "git log
--format='%w(100)%b'" to understand the formatting.

> HEAD is allowed by the protocol spec and would happen, if HEAD points at a
> ref, that this user cannot see (due to ACLs for example).

A more typical example would be if the ref simply doesn't exist (i.e.,
is a branch yet to be born).

> 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.
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>  git-submodule.sh | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/git-submodule.sh b/git-submodule.sh
> index 24914963ca2..13b378a6c8f 100755
> --- a/git-submodule.sh
> +++ b/git-submodule.sh
> @@ -614,7 +614,6 @@ cmd_update()
>  				# is not reachable from a ref.
>  				is_tip_reachable "$sm_path" "$sha1" ||
>  				fetch_in_submodule "$sm_path" $depth ||

Is keeping the '||' at the end of this line intended?

> -				die "$(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
> 				is_tip_reachable "$sm_path" "$sha1" ||
> 				fetch_in_submodule "$sm_path" $depth "$sha1" ||
> 				die "$(eval_gettext "Fetched in submodule path '\$displaypath', but it did not contain \$sha1. Direct fetching of that commit failed.")"

Should this error message be changed?

Thanks,
Jonathan

^ permalink raw reply	[flat|nested] 26+ messages in thread

* [PATCH] git-submodule.sh: try harder to fetch a submodule
@ 2018-05-11 23:17 Stefan Beller
  2018-05-11 23:28 ` Jonathan Nieder
  0 siblings, 1 reply; 26+ messages in thread
From: Stefan Beller @ 2018-05-11 23:17 UTC (permalink / raw)
  To: gitster; +Cc: git, Stefan Beller

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.

> 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. Not advertising
HEAD is allowed by the protocol spec and would happen, if HEAD points at a
ref, that this user cannot see (due to ACLs for example).

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.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 git-submodule.sh | 1 -
 1 file changed, 1 deletion(-)

diff --git a/git-submodule.sh b/git-submodule.sh
index 24914963ca2..13b378a6c8f 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -614,7 +614,6 @@ 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'")"
 
 				# Now we tried the usual fetch, but $sha1 may
 				# not be reachable from any of the refs
-- 
2.17.0.582.gccdcbd54c44.dirty


^ permalink raw reply	[flat|nested] 26+ messages in thread

end of thread, back to index

Thread overview: 26+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-05-15 20:00 [PATCHv2 0/3] Reroll of sb/submodule-merge-in-merge-recursive Stefan Beller
2018-05-15 20:00 ` [PATCH] git-submodule.sh: try harder to fetch a submodule Stefan Beller
2018-05-15 20:00 ` [PATCH] grep: handle corrupt index files early Stefan Beller
2018-05-15 20:00 ` [PATCH 1/3] submodule.c: move submodule merging to merge-recursive.c Stefan Beller
2018-05-15 20:00 ` [PATCH 2/3] merge-recursive: i18n submodule merge output and respect verbosity Stefan Beller
2018-05-16  1:17   ` Junio C Hamano
2018-05-15 20:00 ` [PATCH 3/3] Inform about fast-forwarding of submodules during merge Stefan Beller
2018-05-16  1:36   ` Elijah Newren
2018-05-16  1:36   ` Junio C Hamano
2018-05-16  1:42   ` Junio C Hamano
2018-06-04 18:48     ` [PATCH v4 0/1] merge-recursive: give notice when submodule commit gets fast-forwarded Leif Middelschulte
2018-06-04 18:48       ` [PATCH 1/1] " Leif Middelschulte
2018-06-07  5:22         ` Elijah Newren
2018-06-11 17:31           ` [PATCH] merge-submodule: reduce output verbosity Leif Middelschulte
2018-06-11 18:04             ` Junio C Hamano
2018-06-11 19:07               ` Leif Middelschulte
2018-05-15 20:02 ` [PATCHv2 0/3] Reroll of sb/submodule-merge-in-merge-recursive Stefan Beller
2018-05-15 20:15 ` Leif Middelschulte
2018-05-15 20:49   ` Stefan Beller
  -- strict thread matches above, loose matches on Subject: below --
2018-05-11 23:17 [PATCH] git-submodule.sh: try harder to fetch a submodule Stefan Beller
2018-05-11 23:28 ` Jonathan Nieder
2018-05-11 23:42   ` Stefan Beller
2018-05-12  0:03   ` Junio C Hamano
2018-05-15 19:07     ` Stefan Beller
2018-05-15 19:40       ` Stefan Beller
2018-05-15 20:04       ` Jonathan Nieder

git@vger.kernel.org mailing list mirror (one of many)

Archives are clonable:
	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.org/gmane.comp.version-control.git

 note: .onion URLs require Tor: https://www.torproject.org/
       or Tor2web: https://www.tor2web.org/

AGPL code for this site: git clone https://public-inbox.org/ public-inbox