[PATCHv5 0/8] git clone: Marry --recursive and --reference

[PATCHv5 0/8] git clone: Marry --recursive and --reference

[Stefan Beller]

v5:

Thanks Junio, Ramsay for comments.

As the comments only address programming work as opposed to design, the 
interdiff is rather small:

diff --git a/builtin/clone.c b/builtin/clone.c
index 0593aee..404c5e8 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -285,23 +285,26 @@ static void strip_trailing_slashes(char *dir)
 
 static int add_one_reference(struct string_list_item *item, void *cb_data)
 {
-	struct strbuf sb = STRBUF_INIT;
+	struct strbuf err = STRBUF_INIT;
 	int *required = cb_data;
-	char *ref_git = compute_alternate_path(item->string, &sb);
+	char *ref_git = compute_alternate_path(item->string, &err);
 
 	if (!ref_git) {
 		if (*required)
-			die("%s", sb.buf);
+			die("%s", err.buf);
 		else
 			fprintf(stderr,
 				_("info: Could not add alternate for '%s': %s\n"),
-				item->string, sb.buf);
+				item->string, err.buf);
 	} else {
+		struct strbuf sb = STRBUF_INIT;
 		strbuf_addf(&sb, "%s/objects", ref_git);
 		add_to_alternates_file(sb.buf);
+		strbuf_release(&sb);
 	}
 
-	strbuf_release(&sb);
+	strbuf_release(&err);
+	free(ref_git);
 	return 0;
 }
 
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 472b1d9..681b91d 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -484,7 +484,7 @@ struct submodule_alternate_setup {
 #define SUBMODULE_ALTERNATE_SETUP_INIT { NULL, \
 	SUBMODULE_ALTERNATE_ERROR_IGNORE, NULL }
 
-int add_possible_reference_from_superproject(
+static int add_possible_reference_from_superproject(
 		struct alternate_object_database *alt, void *sas_cb)
 {
 	struct submodule_alternate_setup *sas = sas_cb;
diff --git a/sha1_file.c b/sha1_file.c
index 2489653..0fe5aa3 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -435,11 +435,12 @@ char *compute_alternate_path(const char *path, struct strbuf *err)
 {
 	char *ref_git = NULL;
 	const char *repo, *ref_git_s;
-	struct strbuf err_buf = STRBUF_INIT;
+	int seen_error = 0;
 
 	ref_git_s = real_path_if_valid(path);
 	if (!ref_git_s) {
-		strbuf_addf(&err_buf, _("path '%s' does not exist"), path);
+		seen_error = 1;
+		strbuf_addf(err, _("path '%s' does not exist"), path);
 		goto out;
 	} else
 		/*
@@ -462,40 +463,41 @@ char *compute_alternate_path(const char *path, struct strbuf *err)
 		ref_git = ref_git_git;
 	} else if (!is_directory(mkpath("%s/objects", ref_git))) {
 		struct strbuf sb = STRBUF_INIT;
+		seen_error = 1;
 		if (get_common_dir(&sb, ref_git)) {
-			strbuf_addf(&err_buf,
+			strbuf_addf(err,
 				    _("reference repository '%s' as a linked "
 				      "checkout is not supported yet."),
 				    path);
 			goto out;
 		}
 
-		strbuf_addf(&err_buf, _("reference repository '%s' is not a "
+		strbuf_addf(err, _("reference repository '%s' is not a "
 					"local repository."), path);
 		goto out;
 	}
 
 	if (!access(mkpath("%s/shallow", ref_git), F_OK)) {
-		strbuf_addf(&err_buf, _("reference repository '%s' is shallow"),
+		strbuf_addf(err, _("reference repository '%s' is shallow"),
 			    path);
+		seen_error = 1;
 		goto out;
 	}
 
 	if (!access(mkpath("%s/info/grafts", ref_git), F_OK)) {
-		strbuf_addf(&err_buf,
+		strbuf_addf(err,
 			    _("reference repository '%s' is grafted"),
 			    path);
+		seen_error = 1;
 		goto out;
 	}
 
 out:
-	if (err_buf.len) {
-		strbuf_addbuf(err, &err_buf);
+	if (seen_error) {
 		free(ref_git);
 		ref_git = NULL;
 	}
 
-	strbuf_release(&err_buf);
 	return ref_git;
 }
 
Thanks,
Stefan


v4:
Thanks to Junios critial questions regarding the design, I took a step back
to look at the bigger picture, again.

new patches:
  clone: factor out checking for an alternate path
  clone: recursive and reference option triggers submodule alternates
  
The last patch redesigns completely how we approach the problem.
Now there are no new command line options (that relate to the problem
of marrying --recursive and --reference), but instead we communicate
everything via configuration options to have a lasting effect (i.e.
submodule update remembers the decision of the initial setup)

Thanks,
Stefan

v3:

Thanks to Junios critial questions regarding the design, I took a step back
to look at the bigger picture. 

--super-reference sounds confusing. (what is the super referring to?)
So drop that approach.

Instead we'll compute where the reference might be in the superproject scope
and ask the submodule clone operation to consider an optional reference.
If the referenced alternate is not there, we'll just warn about it and
carry on.


* fixed the style in patch 2.

* fixed another bug in the last patch, that is unrelated, but would have helped
  me a lot.

Thanks,
Stefan

 Documentation/git-clone.txt    |   9 ++++++++-
 builtin/clone.c                |  36 ++++++++++++++++++++++++++++--------
 builtin/submodule--helper.c    | 105 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-----------------
 git-submodule.sh               |   2 +-
 t/t7408-submodule-reference.sh | 162 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++----------------------------------------------------------------------
 5 files changed, 217 insertions(+), 97 deletions(-)

v2:
 * fixed the p1,2 cleanups
 * added documentation to patches 5,6
 * improved commit message in v4
 
 Thanks,
 Stefan
 
v1:
 
 Currently when cloning a superproject with --recursive and --reference
 only the superproject learns about its alternates. The submodules are
 cloned independently, which may incur lots of network costs.
 
 Assume that the reference repository has the submodules at the same
 paths as the to-be-cloned submodule and try to setup alternates from
 there.
 
 Some submodules in the referenced superproject may not be there, 
 (they are just not initialized/cloned/checked out), which yields
 an error for now. In future work we may want to soften the alternate
 check and not die in the clone when one of the given alternates doesn't
 exist.
 
 patch 1,2 are modernizing style of t7408, 
 patches 3,4 are not strictly necessary, but I think it is a good thing
 to not leave the submodule related C code in a crippled state (i.e.
 allowing only one reference). The shell code would also need this update,
 but it looked ugly to me, so I postpone it until more of the submodule code
 is written in C. 
 
 Thanks,
 Stefan 

Stefan Beller (6):
  t7408: modernize style
  t7408: merge short tests, factor out testing method
  submodule--helper module-clone: allow multiple references
  submodule--helper update-clone: allow multiple references
  submodule update: add super-reference flag
  clone: reference flag is used for submodules as well

 builtin/clone.c                |  22 ++++--
 builtin/submodule--helper.c    |  45 ++++++++----
 git-submodule.sh               |  12 +++-
 t/t7408-submodule-reference.sh | 153 +++++++++++++++++++++++------------------
 4 files changed, 147 insertions(+), 85 deletions(-)

-- 
2.9.2.572.g9d9644e.dirty

[PATCHv5 1/8] t7408: modernize style

[Stefan Beller]

No functional change intended. This commit only changes formatting
to the style we recently use, e.g. starting the body of a test with a
single quote on the same line as the header, and then having the test
indented in the following lines.

Whenever we change directories, we do that in subshells.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 t/t7408-submodule-reference.sh | 140 +++++++++++++++++++++--------------------
 1 file changed, 71 insertions(+), 69 deletions(-)

diff --git a/t/t7408-submodule-reference.sh b/t/t7408-submodule-reference.sh
index eaea19b..b84c6748 100755
--- a/t/t7408-submodule-reference.sh
+++ b/t/t7408-submodule-reference.sh
@@ -8,74 +8,76 @@ test_description='test clone --reference'
 
 base_dir=$(pwd)
 
-U=$base_dir/UPLOAD_LOG
-
-test_expect_success 'preparing first repository' \
-'test_create_repo A && cd A &&
-echo first > file1 &&
-git add file1 &&
-git commit -m A-initial'
-
-cd "$base_dir"
-
-test_expect_success 'preparing second repository' \
-'git clone A B && cd B &&
-echo second > file2 &&
-git add file2 &&
-git commit -m B-addition &&
-git repack -a -d &&
-git prune'
-
-cd "$base_dir"
-
-test_expect_success 'preparing superproject' \
-'test_create_repo super && cd super &&
-echo file > file &&
-git add file &&
-git commit -m B-super-initial'
-
-cd "$base_dir"
-
-test_expect_success 'submodule add --reference' \
-'cd super && git submodule add --reference ../B "file://$base_dir/A" sub &&
-git commit -m B-super-added'
-
-cd "$base_dir"
-
-test_expect_success 'after add: existence of info/alternates' \
-'test_line_count = 1 super/.git/modules/sub/objects/info/alternates'
-
-cd "$base_dir"
-
-test_expect_success 'that reference gets used with add' \
-'cd super/sub &&
-echo "0 objects, 0 kilobytes" > expected &&
-git count-objects > current &&
-diff expected current'
-
-cd "$base_dir"
-
-test_expect_success 'cloning superproject' \
-'git clone super super-clone'
-
-cd "$base_dir"
-
-test_expect_success 'update with reference' \
-'cd super-clone && git submodule update --init --reference ../B'
-
-cd "$base_dir"
-
-test_expect_success 'after update: existence of info/alternates' \
-'test_line_count = 1 super-clone/.git/modules/sub/objects/info/alternates'
-
-cd "$base_dir"
-
-test_expect_success 'that reference gets used with update' \
-'cd super-clone/sub &&
-echo "0 objects, 0 kilobytes" > expected &&
-git count-objects > current &&
-diff expected current'
-
-cd "$base_dir"
+test_expect_success 'preparing first repository' '
+	test_create_repo A &&
+	(
+		cd A &&
+		echo first >file1 &&
+		git add file1 &&
+		git commit -m A-initial
+	)
+'
+
+test_expect_success 'preparing second repository' '
+	git clone A B &&
+	(
+		cd B &&
+		echo second >file2 &&
+		git add file2 &&
+		git commit -m B-addition &&
+		git repack -a -d &&
+		git prune
+	)
+'
+
+test_expect_success 'preparing superproject' '
+	test_create_repo super &&
+	(
+		cd super &&
+		echo file >file &&
+		git add file &&
+		git commit -m B-super-initial
+	)
+'
+
+test_expect_success 'submodule add --reference' '
+	(
+		cd super &&
+		git submodule add --reference ../B "file://$base_dir/A" sub &&
+		git commit -m B-super-added
+	)
+'
+
+test_expect_success 'after add: existence of info/alternates' '
+	test_line_count = 1 super/.git/modules/sub/objects/info/alternates
+'
+
+test_expect_success 'that reference gets used with add' '
+	(
+		cd super/sub &&
+		echo "0 objects, 0 kilobytes" >expected &&
+		git count-objects >current &&
+		diff expected current
+	)
+'
+
+test_expect_success 'cloning superproject' '
+	git clone super super-clone
+'
+
+test_expect_success 'update with reference' '
+	cd super-clone && git submodule update --init --reference ../B
+'
+
+test_expect_success 'after update: existence of info/alternates' '
+	test_line_count = 1 super-clone/.git/modules/sub/objects/info/alternates
+'
+
+test_expect_success 'that reference gets used with update' '
+	cd super-clone/sub &&
+	echo "0 objects, 0 kilobytes" >expected &&
+	git count-objects >current &&
+	diff expected current
+'
 
 test_done
-- 
2.9.2.730.g525ad04.dirty

[PATCHv5 2/8] t7408: merge short tests, factor out testing method

[Stefan Beller]

Tests consisting of one line each can be consolidated to have fewer tests
to run as well as fewer lines of code.

When having just a few git commands, do not create a new shell but
use the -C flag in Git to execute in the correct directory.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 t/t7408-submodule-reference.sh | 48 ++++++++++++++++++++++--------------------
 1 file changed, 25 insertions(+), 23 deletions(-)

diff --git a/t/t7408-submodule-reference.sh b/t/t7408-submodule-reference.sh
index b84c6748..dff47af 100755
--- a/t/t7408-submodule-reference.sh
+++ b/t/t7408-submodule-reference.sh
@@ -8,6 +8,15 @@ test_description='test clone --reference'
 
 base_dir=$(pwd)
 
+test_alternate_is_used () {
+	alternates_file="$1" &&
+	working_dir="$2" &&
+	test_line_count = 1 "$alternates_file" &&
+	echo "0 objects, 0 kilobytes" >expect &&
+	git -C "$working_dir" count-objects >actual &&
+	test_cmp expect actual
+}
+
 test_expect_success 'preparing first repository' '
 	test_create_repo A &&
 	(
@@ -40,16 +49,14 @@ test_expect_success 'preparing superproject' '
 	)
 '
 
-test_expect_success 'submodule add --reference' '
+test_expect_success 'submodule add --reference uses alternates' '
 	(
 		cd super &&
 		git submodule add --reference ../B "file://$base_dir/A" sub &&
-		git commit -m B-super-added
-	)
-'
-
-test_expect_success 'after add: existence of info/alternates' '
-	test_line_count = 1 super/.git/modules/sub/objects/info/alternates
+		git commit -m B-super-added &&
+		git repack -ad
+	) &&
+	test_alternate_is_used super/.git/modules/sub/objects/info/alternates super/sub
 '
 
 test_expect_success 'that reference gets used with add' '
@@ -61,23 +68,18 @@ test_expect_success 'that reference gets used with add' '
 	)
 '
 
-test_expect_success 'cloning superproject' '
-	git clone super super-clone
-'
-
-test_expect_success 'update with reference' '
-	cd super-clone && git submodule update --init --reference ../B
-'
-
-test_expect_success 'after update: existence of info/alternates' '
-	test_line_count = 1 super-clone/.git/modules/sub/objects/info/alternates
-'
+# The tests up to this point, and repositories created by them
+# (A, B, super and super/sub), are about setting up the stage
+# for subsequent tests and meant to be kept throughout the
+# remainder of the test.
+# Tests from here on, if they create their own test repository,
+# are expected to clean after themselves.
 
-test_expect_success 'that reference gets used with update' '
-	cd super-clone/sub &&
-	echo "0 objects, 0 kilobytes" >expected &&
-	git count-objects >current &&
-	diff expected current
+test_expect_success 'updating superproject keeps alternates' '
+	test_when_finished "rm -rf super-clone" &&
+	git clone super super-clone &&
+	git -C super-clone submodule update --init --reference ../B &&
+	test_alternate_is_used super-clone/.git/modules/sub/objects/info/alternates super-clone/sub
 '
 
 test_done
-- 
2.9.2.730.g525ad04.dirty

[PATCHv5 3/8] submodule--helper module-clone: allow multiple references

[Stefan Beller]

Allow users to pass in multiple references, just as clone accepts multiple
references as well.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 builtin/submodule--helper.c | 20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 6f6d67a..ce9b3e9 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -442,7 +442,7 @@ static int module_name(int argc, const char **argv, const char *prefix)
 }
 
 static int clone_submodule(const char *path, const char *gitdir, const char *url,
-			   const char *depth, const char *reference, int quiet)
+			   const char *depth, struct string_list *reference, int quiet)
 {
 	struct child_process cp;
 	child_process_init(&cp);
@@ -453,8 +453,12 @@ static int clone_submodule(const char *path, const char *gitdir, const char *url
 		argv_array_push(&cp.args, "--quiet");
 	if (depth && *depth)
 		argv_array_pushl(&cp.args, "--depth", depth, NULL);
-	if (reference && *reference)
-		argv_array_pushl(&cp.args, "--reference", reference, NULL);
+	if (reference->nr) {
+		struct string_list_item *item;
+		for_each_string_list_item(item, reference)
+			argv_array_pushl(&cp.args, "--reference",
+					 item->string, NULL);
+	}
 	if (gitdir && *gitdir)
 		argv_array_pushl(&cp.args, "--separate-git-dir", gitdir, NULL);
 
@@ -470,13 +474,13 @@ static int clone_submodule(const char *path, const char *gitdir, const char *url
 
 static int module_clone(int argc, const char **argv, const char *prefix)
 {
-	const char *name = NULL, *url = NULL;
-	const char *reference = NULL, *depth = NULL;
+	const char *name = NULL, *url = NULL, *depth = NULL;
 	int quiet = 0;
 	FILE *submodule_dot_git;
 	char *p, *path = NULL, *sm_gitdir;
 	struct strbuf rel_path = STRBUF_INIT;
 	struct strbuf sb = STRBUF_INIT;
+	struct string_list reference = STRING_LIST_INIT_NODUP;
 
 	struct option module_clone_options[] = {
 		OPT_STRING(0, "prefix", &prefix,
@@ -491,8 +495,8 @@ static int module_clone(int argc, const char **argv, const char *prefix)
 		OPT_STRING(0, "url", &url,
 			   N_("string"),
 			   N_("url where to clone the submodule from")),
-		OPT_STRING(0, "reference", &reference,
-			   N_("string"),
+		OPT_STRING_LIST(0, "reference", &reference,
+			   N_("repo"),
 			   N_("reference repository")),
 		OPT_STRING(0, "depth", &depth,
 			   N_("string"),
@@ -528,7 +532,7 @@ static int module_clone(int argc, const char **argv, const char *prefix)
 	if (!file_exists(sm_gitdir)) {
 		if (safe_create_leading_directories_const(sm_gitdir) < 0)
 			die(_("could not create directory '%s'"), sm_gitdir);
-		if (clone_submodule(path, sm_gitdir, url, depth, reference, quiet))
+		if (clone_submodule(path, sm_gitdir, url, depth, &reference, quiet))
 			die(_("clone of '%s' into submodule path '%s' failed"),
 			    url, path);
 	} else {
-- 
2.9.2.730.g525ad04.dirty

[PATCHv5 4/8] submodule--helper update-clone: allow multiple references

[Stefan Beller]

Allow the user to pass in multiple references to update_clone.
Currently this is only internal API, but once the shell script is
replaced by a C version, this is needed.

This fixes an API bug between the shell script and the helper.
Currently the helper accepts "--reference" "--reference=foo"
as a OPT_STRING whose value happens to be "--reference=foo", and
then uses

        if (suc->reference)
                argv_array_push(&child->args, suc->reference)

where suc->reference _is_ "--reference=foo" when invoking the
underlying "git clone", it cancels out.

With this change we omit one of the "--reference" arguments when
passing references from the shell script to the helper.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 builtin/submodule--helper.c | 14 +++++++++-----
 git-submodule.sh            |  2 +-
 2 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index ce9b3e9..7096848 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -584,7 +584,7 @@ struct submodule_update_clone {
 	/* configuration parameters which are passed on to the children */
 	int quiet;
 	int recommend_shallow;
-	const char *reference;
+	struct string_list references;
 	const char *depth;
 	const char *recursive_prefix;
 	const char *prefix;
@@ -600,7 +600,8 @@ struct submodule_update_clone {
 	int failed_clones_nr, failed_clones_alloc;
 };
 #define SUBMODULE_UPDATE_CLONE_INIT {0, MODULE_LIST_INIT, 0, \
-	SUBMODULE_UPDATE_STRATEGY_INIT, 0, -1, NULL, NULL, NULL, NULL, \
+	SUBMODULE_UPDATE_STRATEGY_INIT, 0, -1, STRING_LIST_INIT_DUP, \
+	NULL, NULL, NULL, \
 	STRING_LIST_INIT_DUP, 0, NULL, 0, 0}
 
 
@@ -710,8 +711,11 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce,
 	argv_array_pushl(&child->args, "--path", sub->path, NULL);
 	argv_array_pushl(&child->args, "--name", sub->name, NULL);
 	argv_array_pushl(&child->args, "--url", url, NULL);
-	if (suc->reference)
-		argv_array_push(&child->args, suc->reference);
+	if (suc->references.nr) {
+		struct string_list_item *item;
+		for_each_string_list_item(item, &suc->references)
+			argv_array_pushl(&child->args, "--reference", item->string, NULL);
+	}
 	if (suc->depth)
 		argv_array_push(&child->args, suc->depth);
 
@@ -830,7 +834,7 @@ static int update_clone(int argc, const char **argv, const char *prefix)
 		OPT_STRING(0, "update", &update,
 			   N_("string"),
 			   N_("rebase, merge, checkout or none")),
-		OPT_STRING(0, "reference", &suc.reference, N_("repo"),
+		OPT_STRING_LIST(0, "reference", &suc.references, N_("repo"),
 			   N_("reference repository")),
 		OPT_STRING(0, "depth", &suc.depth, "<depth>",
 			   N_("Create a shallow clone truncated to the "
diff --git a/git-submodule.sh b/git-submodule.sh
index c90dc33..3b412f5 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -575,7 +575,7 @@ cmd_update()
 		${wt_prefix:+--prefix "$wt_prefix"} \
 		${prefix:+--recursive-prefix "$prefix"} \
 		${update:+--update "$update"} \
-		${reference:+--reference "$reference"} \
+		${reference:+"$reference"} \
 		${depth:+--depth "$depth"} \
 		${recommend_shallow:+"$recommend_shallow"} \
 		${jobs:+$jobs} \
-- 
2.9.2.730.g525ad04.dirty

[PATCHv5 5/8] clone: factor out checking for an alternate path

[Stefan Beller]

In a later patch we want to determine if a path is suitable as an
alternate from other commands than builtin/clone. Move the checking
functionality of `add_one_reference` to `compute_alternate_path` that is
defined in cache.h.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 builtin/clone.c | 43 +++++++-------------------------
 cache.h         |  1 +
 sha1_file.c     | 76 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 86 insertions(+), 34 deletions(-)

diff --git a/builtin/clone.c b/builtin/clone.c
index f044a8c..4cd3a66 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -282,44 +282,19 @@ static void strip_trailing_slashes(char *dir)
 
 static int add_one_reference(struct string_list_item *item, void *cb_data)
 {
-	char *ref_git;
-	const char *repo;
-	struct strbuf alternate = STRBUF_INIT;
-
-	/* Beware: read_gitfile(), real_path() and mkpath() return static buffer */
-	ref_git = xstrdup(real_path(item->string));
-
-	repo = read_gitfile(ref_git);
-	if (!repo)
-		repo = read_gitfile(mkpath("%s/.git", ref_git));
-	if (repo) {
-		free(ref_git);
-		ref_git = xstrdup(repo);
-	}
-
-	if (!repo && is_directory(mkpath("%s/.git/objects", ref_git))) {
-		char *ref_git_git = mkpathdup("%s/.git", ref_git);
-		free(ref_git);
-		ref_git = ref_git_git;
-	} else if (!is_directory(mkpath("%s/objects", ref_git))) {
-		struct strbuf sb = STRBUF_INIT;
-		if (get_common_dir(&sb, ref_git))
-			die(_("reference repository '%s' as a linked checkout is not supported yet."),
-			    item->string);
-		die(_("reference repository '%s' is not a local repository."),
-		    item->string);
-	}
+	struct strbuf err = STRBUF_INIT;
+	struct strbuf sb = STRBUF_INIT;
+	char *ref_git = compute_alternate_path(item->string, &err);
 
-	if (!access(mkpath("%s/shallow", ref_git), F_OK))
-		die(_("reference repository '%s' is shallow"), item->string);
+	if (!ref_git)
+		die("%s", err.buf);
 
-	if (!access(mkpath("%s/info/grafts", ref_git), F_OK))
-		die(_("reference repository '%s' is grafted"), item->string);
+	strbuf_addf(&sb, "%s/objects", ref_git);
+	add_to_alternates_file(sb.buf);
 
-	strbuf_addf(&alternate, "%s/objects", ref_git);
-	add_to_alternates_file(alternate.buf);
-	strbuf_release(&alternate);
 	free(ref_git);
+	strbuf_release(&err);
+	strbuf_release(&sb);
 	return 0;
 }
 
diff --git a/cache.h b/cache.h
index b5f76a4..1087dca 100644
--- a/cache.h
+++ b/cache.h
@@ -1342,6 +1342,7 @@ extern struct alternate_object_database {
 } *alt_odb_list;
 extern void prepare_alt_odb(void);
 extern void read_info_alternates(const char * relative_base, int depth);
+extern char *compute_alternate_path(const char *path, struct strbuf *err);
 extern void add_to_alternates_file(const char *reference);
 typedef int alt_odb_fn(struct alternate_object_database *, void *);
 extern int foreach_alt_odb(alt_odb_fn, void*);
diff --git a/sha1_file.c b/sha1_file.c
index cb571ac..0fe5aa3 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -425,6 +425,82 @@ void add_to_alternates_file(const char *reference)
 	free(alts);
 }
 
+/*
+ * Compute the exact path an alternate is at and returns it. In case of
+ * error NULL is returned and the human readable error is added to `err`
+ * `path` may be relative and should point to $GITDIR.
+ * `err` must not be null.
+ */
+char *compute_alternate_path(const char *path, struct strbuf *err)
+{
+	char *ref_git = NULL;
+	const char *repo, *ref_git_s;
+	int seen_error = 0;
+
+	ref_git_s = real_path_if_valid(path);
+	if (!ref_git_s) {
+		seen_error = 1;
+		strbuf_addf(err, _("path '%s' does not exist"), path);
+		goto out;
+	} else
+		/*
+		 * Beware: read_gitfile(), real_path() and mkpath()
+		 * return static buffer
+		 */
+		ref_git = xstrdup(ref_git_s);
+
+	repo = read_gitfile(ref_git);
+	if (!repo)
+		repo = read_gitfile(mkpath("%s/.git", ref_git));
+	if (repo) {
+		free(ref_git);
+		ref_git = xstrdup(repo);
+	}
+
+	if (!repo && is_directory(mkpath("%s/.git/objects", ref_git))) {
+		char *ref_git_git = mkpathdup("%s/.git", ref_git);
+		free(ref_git);
+		ref_git = ref_git_git;
+	} else if (!is_directory(mkpath("%s/objects", ref_git))) {
+		struct strbuf sb = STRBUF_INIT;
+		seen_error = 1;
+		if (get_common_dir(&sb, ref_git)) {
+			strbuf_addf(err,
+				    _("reference repository '%s' as a linked "
+				      "checkout is not supported yet."),
+				    path);
+			goto out;
+		}
+
+		strbuf_addf(err, _("reference repository '%s' is not a "
+					"local repository."), path);
+		goto out;
+	}
+
+	if (!access(mkpath("%s/shallow", ref_git), F_OK)) {
+		strbuf_addf(err, _("reference repository '%s' is shallow"),
+			    path);
+		seen_error = 1;
+		goto out;
+	}
+
+	if (!access(mkpath("%s/info/grafts", ref_git), F_OK)) {
+		strbuf_addf(err,
+			    _("reference repository '%s' is grafted"),
+			    path);
+		seen_error = 1;
+		goto out;
+	}
+
+out:
+	if (seen_error) {
+		free(ref_git);
+		ref_git = NULL;
+	}
+
+	return ref_git;
+}
+
 int foreach_alt_odb(alt_odb_fn fn, void *cb)
 {
 	struct alternate_object_database *ent;
-- 
2.9.2.730.g525ad04.dirty

[PATCHv5 6/8] clone: clarify option_reference as required

[Stefan Beller]

In the next patch we introduce optional references; To better distinguish
between optional and required references we rename the variable.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 builtin/clone.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/builtin/clone.c b/builtin/clone.c
index 4cd3a66..ef5db7c 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -50,7 +50,7 @@ static int option_verbosity;
 static int option_progress = -1;
 static enum transport_family family;
 static struct string_list option_config = STRING_LIST_INIT_NODUP;
-static struct string_list option_reference = STRING_LIST_INIT_NODUP;
+static struct string_list option_required_reference = STRING_LIST_INIT_NODUP;
 static int option_dissociate;
 static int max_jobs = -1;
 
@@ -79,7 +79,7 @@ static struct option builtin_clone_options[] = {
 		    N_("number of submodules cloned in parallel")),
 	OPT_STRING(0, "template", &option_template, N_("template-directory"),
 		   N_("directory from which templates will be used")),
-	OPT_STRING_LIST(0, "reference", &option_reference, N_("repo"),
+	OPT_STRING_LIST(0, "reference", &option_required_reference, N_("repo"),
 			N_("reference repository")),
 	OPT_BOOL(0, "dissociate", &option_dissociate,
 		 N_("use --reference only while cloning")),
@@ -300,7 +300,7 @@ static int add_one_reference(struct string_list_item *item, void *cb_data)
 
 static void setup_reference(void)
 {
-	for_each_string_list(&option_reference, add_one_reference, NULL);
+	for_each_string_list(&option_required_reference, add_one_reference, NULL);
 }
 
 static void copy_alternates(struct strbuf *src, struct strbuf *dst,
@@ -952,7 +952,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 	git_config_set(key.buf, repo);
 	strbuf_reset(&key);
 
-	if (option_reference.nr)
+	if (option_required_reference.nr)
 		setup_reference();
 
 	fetch_pattern = value.buf;
-- 
2.9.2.730.g525ad04.dirty

[PATCHv5 7/8] clone: implement optional references

[Stefan Beller]

In a later patch we want to try to create alternates for submodules,
but they might not exist in the referenced superproject. So add a way
to skip the non existing references and report them.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 Documentation/git-clone.txt |  5 ++++-
 builtin/clone.c             | 35 +++++++++++++++++++++++++----------
 2 files changed, 29 insertions(+), 11 deletions(-)

diff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt
index ec41d3d..e316c4b 100644
--- a/Documentation/git-clone.txt
+++ b/Documentation/git-clone.txt
@@ -90,13 +90,16 @@ If you want to break the dependency of a repository cloned with `-s` on
 its source repository, you can simply run `git repack -a` to copy all
 objects from the source repository into a pack in the cloned repository.
 
---reference <repository>::
+--reference[-if-able] <repository>::
 	If the reference repository is on the local machine,
 	automatically setup `.git/objects/info/alternates` to
 	obtain objects from the reference repository.  Using
 	an already existing repository as an alternate will
 	require fewer objects to be copied from the repository
 	being cloned, reducing network and local storage costs.
+	When using the `--reference-if-able`, a non existing
+	directory is skipped with a warning instead of aborting
+	the clone.
 +
 *NOTE*: see the NOTE for the `--shared` option, and also the
 `--dissociate` option.
diff --git a/builtin/clone.c b/builtin/clone.c
index ef5db7c..0182665 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -51,6 +51,7 @@ static int option_progress = -1;
 static enum transport_family family;
 static struct string_list option_config = STRING_LIST_INIT_NODUP;
 static struct string_list option_required_reference = STRING_LIST_INIT_NODUP;
+static struct string_list option_optional_reference = STRING_LIST_INIT_NODUP;
 static int option_dissociate;
 static int max_jobs = -1;
 
@@ -81,6 +82,8 @@ static struct option builtin_clone_options[] = {
 		   N_("directory from which templates will be used")),
 	OPT_STRING_LIST(0, "reference", &option_required_reference, N_("repo"),
 			N_("reference repository")),
+	OPT_STRING_LIST(0, "reference-if-able", &option_optional_reference,
+			N_("repo"), N_("reference repository")),
 	OPT_BOOL(0, "dissociate", &option_dissociate,
 		 N_("use --reference only while cloning")),
 	OPT_STRING('o', "origin", &option_origin, N_("name"),
@@ -283,24 +286,36 @@ static void strip_trailing_slashes(char *dir)
 static int add_one_reference(struct string_list_item *item, void *cb_data)
 {
 	struct strbuf err = STRBUF_INIT;
-	struct strbuf sb = STRBUF_INIT;
+	int *required = cb_data;
 	char *ref_git = compute_alternate_path(item->string, &err);
 
-	if (!ref_git)
-		die("%s", err.buf);
-
-	strbuf_addf(&sb, "%s/objects", ref_git);
-	add_to_alternates_file(sb.buf);
+	if (!ref_git) {
+		if (*required)
+			die("%s", err.buf);
+		else
+			fprintf(stderr,
+				_("info: Could not add alternate for '%s': %s\n"),
+				item->string, err.buf);
+	} else {
+		struct strbuf sb = STRBUF_INIT;
+		strbuf_addf(&sb, "%s/objects", ref_git);
+		add_to_alternates_file(sb.buf);
+		strbuf_release(&sb);
+	}
 
-	free(ref_git);
 	strbuf_release(&err);
-	strbuf_release(&sb);
+	free(ref_git);
 	return 0;
 }
 
 static void setup_reference(void)
 {
-	for_each_string_list(&option_required_reference, add_one_reference, NULL);
+	int required = 1;
+	for_each_string_list(&option_required_reference,
+			     add_one_reference, &required);
+	required = 0;
+	for_each_string_list(&option_optional_reference,
+			     add_one_reference, &required);
 }
 
 static void copy_alternates(struct strbuf *src, struct strbuf *dst,
@@ -952,7 +967,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 	git_config_set(key.buf, repo);
 	strbuf_reset(&key);
 
-	if (option_required_reference.nr)
+	if (option_required_reference.nr || option_optional_reference.nr)
 		setup_reference();
 
 	fetch_pattern = value.buf;
-- 
2.9.2.730.g525ad04.dirty

[PATCHv5 8/8] clone: recursive and reference option triggers submodule alternates

[Stefan Beller]

When `--recursive` and `--reference` is given, it is reasonable to
expect that the submodules are created with references to the submodules
of the given alternate for the superproject.

  An initial attempt to do this was presented to the mailing list, which
  used flags that are passed around ("--super-reference") that instructed
  the submodule clone to look for a reference in the submodules of the
  referenced superproject. This is not well thought out, as any further
  `submodule update` should also respect the initial setup.

  When a new  submodule is added to the superproject and the alternate
  of the superproject does not know about that submodule yet, we rather
  error out informing the user instead of being unclear if we did or did
  not use a submodules alternate.

To solve this problem introduce new options that store the configuration
for what the user wanted originally.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 Documentation/config.txt       | 12 ++++++
 builtin/clone.c                | 19 +++++++++
 builtin/submodule--helper.c    | 87 ++++++++++++++++++++++++++++++++++++++++++
 t/t7408-submodule-reference.sh | 43 +++++++++++++++++++++
 4 files changed, 161 insertions(+)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index bc1c433..602f43a 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2837,6 +2837,18 @@ submodule.fetchJobs::
 	in parallel. A value of 0 will give some reasonable default.
 	If unset, it defaults to 1.
 
+submodule.alternateLocation::
+	Specifies how the submodules obtain alternates when submodules are
+	cloned. Possible values are `no`, `superproject`.
+	By default `no` is assumed, which doesn't add references. When the
+	value is set to `superproject` the submodule to be cloned computes
+	its alternates location relative to the superprojects alternate.
+
+submodule.alternateErrorStrategy
+	Specifies how to treat errors with the alternates for a submodule
+	as computed via `submodule.alternateLocation`. Possible values are
+	`ignore`, `info`, `die`.
+
 tag.forceSignAnnotated::
 	A boolean to specify whether annotated tags created should be GPG signed.
 	If `--annotate` is specified on the command line, it takes
diff --git a/builtin/clone.c b/builtin/clone.c
index 0182665..404c5e8 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -947,6 +947,25 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 		else
 			fprintf(stderr, _("Cloning into '%s'...\n"), dir);
 	}
+
+	if (option_recursive) {
+		if (option_required_reference.nr &&
+		    option_optional_reference.nr)
+			die(_("clone --recursive is not compatible with "
+			      "both --reference and --reference-if-able"));
+		else if (option_required_reference.nr) {
+			string_list_append(&option_config,
+				"submodule.alternateLocation=superproject");
+			string_list_append(&option_config,
+				"submodule.alternateErrorStrategy=die");
+		} else if (option_optional_reference.nr) {
+			string_list_append(&option_config,
+				"submodule.alternateLocation=superproject");
+			string_list_append(&option_config,
+				"submodule.alternateErrorStrategy=info");
+		}
+	}
+
 	init_db(option_template, INIT_DB_QUIET);
 	write_config(&option_config);
 
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 7096848..681b91d 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -472,6 +472,90 @@ static int clone_submodule(const char *path, const char *gitdir, const char *url
 	return run_command(&cp);
 }
 
+struct submodule_alternate_setup {
+	const char *submodule_name;
+	enum SUBMODULE_ALTERNATE_ERROR_MODE {
+		SUBMODULE_ALTERNATE_ERROR_DIE,
+		SUBMODULE_ALTERNATE_ERROR_INFO,
+		SUBMODULE_ALTERNATE_ERROR_IGNORE
+	} error_mode;
+	struct string_list *reference;
+};
+#define SUBMODULE_ALTERNATE_SETUP_INIT { NULL, \
+	SUBMODULE_ALTERNATE_ERROR_IGNORE, NULL }
+
+static int add_possible_reference_from_superproject(
+		struct alternate_object_database *alt, void *sas_cb)
+{
+	struct submodule_alternate_setup *sas = sas_cb;
+
+	/* directory name, minus trailing slash */
+	size_t namelen = alt->name - alt->base - 1;
+	struct strbuf name = STRBUF_INIT;
+	strbuf_add(&name, alt->base, namelen);
+
+	/*
+	 * If the alternate object store is another repository, try the
+	 * standard layout with .git/modules/<name>/objects
+	 */
+	if (ends_with(name.buf, ".git/objects")) {
+		char *sm_alternate;
+		struct strbuf sb = STRBUF_INIT;
+		struct strbuf err = STRBUF_INIT;
+		strbuf_add(&sb, name.buf, name.len - strlen("objects"));
+		/*
+		 * We need to end the new path with '/' to mark it as a dir,
+		 * otherwise a submodule name containing '/' will be broken
+		 * as the last part of a missing submodule reference would
+		 * be taken as a file name.
+		 */
+		strbuf_addf(&sb, "modules/%s/", sas->submodule_name);
+
+		sm_alternate = compute_alternate_path(sb.buf, &err);
+		if (sm_alternate) {
+			string_list_append(sas->reference, xstrdup(sb.buf));
+			free(sm_alternate);
+		} else {
+			switch (sas->error_mode) {
+			case SUBMODULE_ALTERNATE_ERROR_DIE:
+				die(_("submodule '%s' cannot add alternate: %s"),
+				    sas->submodule_name, err.buf);
+			case SUBMODULE_ALTERNATE_ERROR_INFO:
+				fprintf(stderr, _("submodule '%s' cannot add alternate: %s"),
+					sas->submodule_name, err.buf);
+			case SUBMODULE_ALTERNATE_ERROR_IGNORE:
+				; /* nothing */
+			}
+		}
+		strbuf_release(&sb);
+	}
+
+	strbuf_release(&name);
+	return 0;
+}
+
+static void prepare_possible_alternates(const char *sm_name,
+		struct string_list *reference)
+{
+	char *sm_alternate = NULL, *error_strategy = NULL;
+	struct submodule_alternate_setup sas = SUBMODULE_ALTERNATE_SETUP_INIT;
+
+	git_config_get_string("submodule.alternateLocation", &sm_alternate);
+	if (!sm_alternate)
+		return;
+
+	git_config_get_string("submodule.alternateErrorStrategy", &error_strategy);
+
+	sas.submodule_name = sm_name;
+	sas.reference = reference;
+	if (!strcmp(error_strategy, "die"))
+		sas.error_mode = SUBMODULE_ALTERNATE_ERROR_DIE;
+	if (!strcmp(error_strategy, "info"))
+		sas.error_mode = SUBMODULE_ALTERNATE_ERROR_INFO;
+	if (!strcmp(sm_alternate, "superproject"))
+		foreach_alt_odb(add_possible_reference_from_superproject, &sas);
+}
+
 static int module_clone(int argc, const char **argv, const char *prefix)
 {
 	const char *name = NULL, *url = NULL, *depth = NULL;
@@ -532,6 +616,9 @@ static int module_clone(int argc, const char **argv, const char *prefix)
 	if (!file_exists(sm_gitdir)) {
 		if (safe_create_leading_directories_const(sm_gitdir) < 0)
 			die(_("could not create directory '%s'"), sm_gitdir);
+
+		prepare_possible_alternates(name, &reference);
+
 		if (clone_submodule(path, sm_gitdir, url, depth, &reference, quiet))
 			die(_("clone of '%s' into submodule path '%s' failed"),
 			    url, path);
diff --git a/t/t7408-submodule-reference.sh b/t/t7408-submodule-reference.sh
index dff47af..1c1e289 100755
--- a/t/t7408-submodule-reference.sh
+++ b/t/t7408-submodule-reference.sh
@@ -82,4 +82,47 @@ test_expect_success 'updating superproject keeps alternates' '
 	test_alternate_is_used super-clone/.git/modules/sub/objects/info/alternates super-clone/sub
 '
 
+test_expect_success 'submodules use alternates when cloning a superproject' '
+	test_when_finished "rm -rf super-clone" &&
+	git clone --reference super --recursive super super-clone &&
+	(
+		cd super-clone &&
+		# test superproject has alternates setup correctly
+		test_alternate_is_used .git/objects/info/alternates . &&
+		# test submodule has correct setup
+		test_alternate_is_used .git/modules/sub/objects/info/alternates sub
+	)
+'
+
+test_expect_success 'missing submodule alternate fails clone and submodule update' '
+	test_when_finished "rm -rf super-clone" &&
+	git clone super super2 &&
+	test_must_fail git clone --recursive --reference super2 super2 super-clone &&
+	(
+		cd super-clone &&
+		# test superproject has alternates setup correctly
+		test_alternate_is_used .git/objects/info/alternates . &&
+		# update of the submodule succeeds
+		test_must_fail git submodule update --init &&
+		# and we have no alternates:
+		test_must_fail test_alternate_is_used .git/modules/sub/objects/info/alternates sub &&
+		test_must_fail test_path_is_file sub/file1
+	)
+'
+
+test_expect_success 'ignoring missing submodule alternates passes clone and submodule update' '
+	test_when_finished "rm -rf super-clone" &&
+	git clone --reference-if-able super2 --recursive super2 super-clone &&
+	(
+		cd super-clone &&
+		# test superproject has alternates setup correctly
+		test_alternate_is_used .git/objects/info/alternates . &&
+		# update of the submodule succeeds
+		git submodule update --init &&
+		# and we have no alternates:
+		test_must_fail test_alternate_is_used .git/modules/sub/objects/info/alternates sub &&
+		test_path_is_file sub/file1
+	)
+'
+
 test_done
-- 
2.9.2.730.g525ad04.dirty

Re: [PATCHv5 0/8] git clone: Marry --recursive and --reference

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> As the comments only address programming work as opposed to design, the 
> interdiff is rather small:

Thanks.  Will queue.  I think we do not need that extra err strbuf
in add_one_reference(), but we can go either way and it is not worth
rerolling.

Re: [PATCHv5 1/8] t7408: modernize style

[Jacob Keller]

On Mon, Aug 15, 2016 at 2:53 PM, Stefan Beller <sbeller@google.com> wrote:
> No functional change intended. This commit only changes formatting
> to the style we recently use, e.g. starting the body of a test with a
> single quote on the same line as the header, and then having the test
> indented in the following lines.
>
> Whenever we change directories, we do that in subshells.
>

I looked this over using -w to ignore whitespace changes, and it
appears to have no functional changes. Much cleaner style overall, and
easier to read the new file as it is now. The tests pass fine both
before and after this commit, and I don't see anything that should
functionally change the results.

Thanks,
Jake

Re: [PATCHv5 2/8] t7408: merge short tests, factor out testing method

[Jacob Keller]

On Mon, Aug 15, 2016 at 2:53 PM, Stefan Beller <sbeller@google.com> wrote:
> Tests consisting of one line each can be consolidated to have fewer tests
> to run as well as fewer lines of code.
>
> When having just a few git commands, do not create a new shell but
> use the -C flag in Git to execute in the correct directory.
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---

Looks good. The resulting test file is easier to read, and you created
a common function to perform checking if we used alternates instead of
duplicating that part multiple times.

>  t/t7408-submodule-reference.sh | 48 ++++++++++++++++++++++--------------------
>  1 file changed, 25 insertions(+), 23 deletions(-)
>
> diff --git a/t/t7408-submodule-reference.sh b/t/t7408-submodule-reference.sh
> index b84c6748..dff47af 100755
> --- a/t/t7408-submodule-reference.sh
> +++ b/t/t7408-submodule-reference.sh
> @@ -8,6 +8,15 @@ test_description='test clone --reference'
>
>  base_dir=$(pwd)
>
> +test_alternate_is_used () {
> +       alternates_file="$1" &&
> +       working_dir="$2" &&
> +       test_line_count = 1 "$alternates_file" &&
> +       echo "0 objects, 0 kilobytes" >expect &&
> +       git -C "$working_dir" count-objects >actual &&
> +       test_cmp expect actual
> +}
> +

This change wasn't mentioned in the description. You updated the tests
to use a stronger check for when alternates is in use. This is good. I
might have mentioned it in the description but it's not worth a
re-roll.

> -test_expect_success 'that reference gets used with update' '
> -       cd super-clone/sub &&
> -       echo "0 objects, 0 kilobytes" >expected &&
> -       git count-objects >current &&
> -       diff expected current

The stronger variant was used once here, but you use it in several
locations. It's good to have extracted this as the new tests are more
readable.

Thanks,
Jake

Re: [PATCHv5 3/8] submodule--helper module-clone: allow multiple references

[Jacob Keller]

On Mon, Aug 15, 2016 at 2:53 PM, Stefan Beller <sbeller@google.com> wrote:
> Allow users to pass in multiple references, just as clone accepts multiple
> references as well.
>

Straight forward and reasonable change.

Regards,
Jake

Re: [PATCHv5 4/8] submodule--helper update-clone: allow multiple references

[Jacob Keller]

On Mon, Aug 15, 2016 at 2:53 PM, Stefan Beller <sbeller@google.com> wrote:
> Allow the user to pass in multiple references to update_clone.
> Currently this is only internal API, but once the shell script is
> replaced by a C version, this is needed.
>
> This fixes an API bug between the shell script and the helper.
> Currently the helper accepts "--reference" "--reference=foo"
> as a OPT_STRING whose value happens to be "--reference=foo", and
> then uses
>
>         if (suc->reference)
>                 argv_array_push(&child->args, suc->reference)
>
> where suc->reference _is_ "--reference=foo" when invoking the
> underlying "git clone", it cancels out.
>
> With this change we omit one of the "--reference" arguments when
> passing references from the shell script to the helper.
>

Yep, I see the API fix, and it looks correct. Makes use of the helper
easier and more likely to be done correctly.

> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>  builtin/submodule--helper.c | 14 +++++++++-----
>  git-submodule.sh            |  2 +-
>  2 files changed, 10 insertions(+), 6 deletions(-)
>
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index ce9b3e9..7096848 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -584,7 +584,7 @@ struct submodule_update_clone {
>         /* configuration parameters which are passed on to the children */
>         int quiet;
>         int recommend_shallow;
> -       const char *reference;
> +       struct string_list references;
>         const char *depth;
>         const char *recursive_prefix;
>         const char *prefix;
> @@ -600,7 +600,8 @@ struct submodule_update_clone {
>         int failed_clones_nr, failed_clones_alloc;
>  };
>  #define SUBMODULE_UPDATE_CLONE_INIT {0, MODULE_LIST_INIT, 0, \
> -       SUBMODULE_UPDATE_STRATEGY_INIT, 0, -1, NULL, NULL, NULL, NULL, \
> +       SUBMODULE_UPDATE_STRATEGY_INIT, 0, -1, STRING_LIST_INIT_DUP, \
> +       NULL, NULL, NULL, \
>         STRING_LIST_INIT_DUP, 0, NULL, 0, 0}
>
>
> @@ -710,8 +711,11 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce,
>         argv_array_pushl(&child->args, "--path", sub->path, NULL);
>         argv_array_pushl(&child->args, "--name", sub->name, NULL);
>         argv_array_pushl(&child->args, "--url", url, NULL);
> -       if (suc->reference)
> -               argv_array_push(&child->args, suc->reference);

Here, you now no longer pass in the reference as a whole, but assume
that it is actually just the value to pass to --reference. And below
you correctly replace the extra --reference. Ok so I see how you fixed
the API bug.

> diff --git a/git-submodule.sh b/git-submodule.sh
> index c90dc33..3b412f5 100755
> --- a/git-submodule.sh
> +++ b/git-submodule.sh
> @@ -575,7 +575,7 @@ cmd_update()
>                 ${wt_prefix:+--prefix "$wt_prefix"} \
>                 ${prefix:+--recursive-prefix "$prefix"} \
>                 ${update:+--update "$update"} \
> -               ${reference:+--reference "$reference"} \
> +               ${reference:+"$reference"} \

From above, I see how you fixed this to assume (as above) that
$reference is "--reference <value>" and pass it directly. It worked
before but was definitely a bit "brittle" in that direct calling of
the helper function would not behave as expected. Nice!

Regards,
Jake

Re: [PATCHv5 5/8] clone: factor out checking for an alternate path

[Jacob Keller]

On Mon, Aug 15, 2016 at 2:53 PM, Stefan Beller <sbeller@google.com> wrote:
> +       if (!repo && is_directory(mkpath("%s/.git/objects", ref_git))) {
> +               char *ref_git_git = mkpathdup("%s/.git", ref_git);
> +               free(ref_git);
> +               ref_git = ref_git_git;
> +       } else if (!is_directory(mkpath("%s/objects", ref_git))) {

I realize this was directly copied but the use of !repo to determine
what file path here was definitely not easy to process, because it
made me think we were adding .git twice to the path. Not sure if there
is an easier way to expand this and avoid that confusion?

Also, what happens if repo is false, so we run the first block above,
but is_directory fails? We just continue along even though it appears
like we should fail? I'm not 100% sure I follow the logic here.
Regardless this appears to be a pretty direct copy of what was there
before so I don't think it's worse.

Thanks,
Jake

Re: [PATCHv5 6/8] clone: clarify option_reference as required

[Jacob Keller]

On Mon, Aug 15, 2016 at 2:53 PM, Stefan Beller <sbeller@google.com> wrote:
> In the next patch we introduce optional references; To better distinguish
> between optional and required references we rename the variable.
>

Makes sense. It's a bit weird to process "option_required_reference"
but I think it is reasonable.

Regards,
Jake

Re: [PATCHv5 7/8] clone: implement optional references

[Jacob Keller]

On Mon, Aug 15, 2016 at 2:53 PM, Stefan Beller <sbeller@google.com> wrote:
> In a later patch we want to try to create alternates for submodules,
> but they might not exist in the referenced superproject. So add a way
> to skip the non existing references and report them.
>

Seems pretty straight forward to me.

> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>  Documentation/git-clone.txt |  5 ++++-
>  builtin/clone.c             | 35 +++++++++++++++++++++++++----------
>  2 files changed, 29 insertions(+), 11 deletions(-)
>
> diff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt
> index ec41d3d..e316c4b 100644
> --- a/Documentation/git-clone.txt
> +++ b/Documentation/git-clone.txt
> @@ -90,13 +90,16 @@ If you want to break the dependency of a repository cloned with `-s` on
>  its source repository, you can simply run `git repack -a` to copy all
>  objects from the source repository into a pack in the cloned repository.
>
> ---reference <repository>::
> +--reference[-if-able] <repository>::
>         If the reference repository is on the local machine,
>         automatically setup `.git/objects/info/alternates` to
>         obtain objects from the reference repository.  Using
>         an already existing repository as an alternate will
>         require fewer objects to be copied from the repository
>         being cloned, reducing network and local storage costs.
> +       When using the `--reference-if-able`, a non existing
> +       directory is skipped with a warning instead of aborting
> +       the clone.
>  +
>  *NOTE*: see the NOTE for the `--shared` option, and also the
>  `--dissociate` option.
> diff --git a/builtin/clone.c b/builtin/clone.c
> index ef5db7c..0182665 100644
> --- a/builtin/clone.c
> +++ b/builtin/clone.c
> @@ -51,6 +51,7 @@ static int option_progress = -1;
>  static enum transport_family family;
>  static struct string_list option_config = STRING_LIST_INIT_NODUP;
>  static struct string_list option_required_reference = STRING_LIST_INIT_NODUP;
> +static struct string_list option_optional_reference = STRING_LIST_INIT_NODUP;
>  static int option_dissociate;
>  static int max_jobs = -1;
>
> @@ -81,6 +82,8 @@ static struct option builtin_clone_options[] = {
>                    N_("directory from which templates will be used")),
>         OPT_STRING_LIST(0, "reference", &option_required_reference, N_("repo"),
>                         N_("reference repository")),
> +       OPT_STRING_LIST(0, "reference-if-able", &option_optional_reference,
> +                       N_("repo"), N_("reference repository")),
>         OPT_BOOL(0, "dissociate", &option_dissociate,
>                  N_("use --reference only while cloning")),
>         OPT_STRING('o', "origin", &option_origin, N_("name"),
> @@ -283,24 +286,36 @@ static void strip_trailing_slashes(char *dir)
>  static int add_one_reference(struct string_list_item *item, void *cb_data)
>  {
>         struct strbuf err = STRBUF_INIT;
> -       struct strbuf sb = STRBUF_INIT;
> +       int *required = cb_data;
>         char *ref_git = compute_alternate_path(item->string, &err);
>
> -       if (!ref_git)
> -               die("%s", err.buf);
> -
> -       strbuf_addf(&sb, "%s/objects", ref_git);
> -       add_to_alternates_file(sb.buf);
> +       if (!ref_git) {
> +               if (*required)
> +                       die("%s", err.buf);
> +               else
> +                       fprintf(stderr,
> +                               _("info: Could not add alternate for '%s': %s\n"),
> +                               item->string, err.buf);
> +       } else {
> +               struct strbuf sb = STRBUF_INIT;
> +               strbuf_addf(&sb, "%s/objects", ref_git);
> +               add_to_alternates_file(sb.buf);
> +               strbuf_release(&sb);
> +       }

I might have done this with a "goto out" instead of the else block,
but this is reasonable as well.

Regards,
Jake

Re: [PATCHv5 8/8] clone: recursive and reference option triggers submodule alternates

[Jacob Keller]

On Mon, Aug 15, 2016 at 2:53 PM, Stefan Beller <sbeller@google.com> wrote:
> When `--recursive` and `--reference` is given, it is reasonable to
> expect that the submodules are created with references to the submodules
> of the given alternate for the superproject.
>
>   An initial attempt to do this was presented to the mailing list, which
>   used flags that are passed around ("--super-reference") that instructed
>   the submodule clone to look for a reference in the submodules of the
>   referenced superproject. This is not well thought out, as any further
>   `submodule update` should also respect the initial setup.
>
>   When a new  submodule is added to the superproject and the alternate
>   of the superproject does not know about that submodule yet, we rather
>   error out informing the user instead of being unclear if we did or did
>   not use a submodules alternate.
>
> To solve this problem introduce new options that store the configuration
> for what the user wanted originally.
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>  Documentation/config.txt       | 12 ++++++
>  builtin/clone.c                | 19 +++++++++
>  builtin/submodule--helper.c    | 87 ++++++++++++++++++++++++++++++++++++++++++
>  t/t7408-submodule-reference.sh | 43 +++++++++++++++++++++
>  4 files changed, 161 insertions(+)
>
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index bc1c433..602f43a 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -2837,6 +2837,18 @@ submodule.fetchJobs::
>         in parallel. A value of 0 will give some reasonable default.
>         If unset, it defaults to 1.
>
> +submodule.alternateLocation::
> +       Specifies how the submodules obtain alternates when submodules are
> +       cloned. Possible values are `no`, `superproject`.
> +       By default `no` is assumed, which doesn't add references. When the
> +       value is set to `superproject` the submodule to be cloned computes
> +       its alternates location relative to the superprojects alternate.
> +
> +submodule.alternateErrorStrategy
> +       Specifies how to treat errors with the alternates for a submodule
> +       as computed via `submodule.alternateLocation`. Possible values are
> +       `ignore`, `info`, `die`.
> +
>  tag.forceSignAnnotated::
>         A boolean to specify whether annotated tags created should be GPG signed.
>         If `--annotate` is specified on the command line, it takes
> diff --git a/builtin/clone.c b/builtin/clone.c
> index 0182665..404c5e8 100644
> --- a/builtin/clone.c
> +++ b/builtin/clone.c
> @@ -947,6 +947,25 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
>                 else
>                         fprintf(stderr, _("Cloning into '%s'...\n"), dir);
>         }
> +
> +       if (option_recursive) {
> +               if (option_required_reference.nr &&
> +                   option_optional_reference.nr)
> +                       die(_("clone --recursive is not compatible with "
> +                             "both --reference and --reference-if-able"));

So if you have multiple references that don't all match we basically
just refuse to allow recursive?

Would it be better to simply assume that we want to die on missing
references instead of failing the clone here? That is, treat it so
that multiple reference and reference-if-able will die, and only info
if we got only reference-if-able?

Probably what's here is fine, and mixing reference and
reference-if-able doesn't make much sense.

Thanks,
Jake

Re: [PATCHv5 8/8] clone: recursive and reference option triggers submodule alternates

[Stefan Beller]

>> +
>> +       if (option_recursive) {
>> +               if (option_required_reference.nr &&
>> +                   option_optional_reference.nr)
>> +                       die(_("clone --recursive is not compatible with "
>> +                             "both --reference and --reference-if-able"));
>
> So if you have multiple references that don't all match we basically
> just refuse to allow recursive?
>
> Would it be better to simply assume that we want to die on missing
> references instead of failing the clone here?

The new config options are per repo (or even set globally), and not
per alternate. And as we communicate the [if-able] part via the config
options to the submodules it is not feasible to transport both
kinds of (reference-or-die and reference-but-ignore-misses).

That is why I introduced this check in the first place. If we'd go back
to the drawing board and come up with a solution that is on a
"per alternate" basis we could allow such things.

> That is, treat it so
> that multiple reference and reference-if-able will die, and only info
> if we got only reference-if-able?
>
> Probably what's here is fine, and mixing reference and
> reference-if-able doesn't make much sense.

I think the reference-if-able doesn't make sense for one project alone
as you can easily script around that, but is only useful if you have
submodules in a partially checked out superproject that you want
to reference to.

Thanks,
Stefan

Re: [PATCHv5 8/8] clone: recursive and reference option triggers submodule alternates

[Jacob Keller]

On Tue, Aug 23, 2016 at 4:03 PM, Stefan Beller <sbeller@google.com> wrote:
>>> +
>>> +       if (option_recursive) {
>>> +               if (option_required_reference.nr &&
>>> +                   option_optional_reference.nr)
>>> +                       die(_("clone --recursive is not compatible with "
>>> +                             "both --reference and --reference-if-able"));
>>
>> So if you have multiple references that don't all match we basically
>> just refuse to allow recursive?
>>
>> Would it be better to simply assume that we want to die on missing
>> references instead of failing the clone here?
>
> The new config options are per repo (or even set globally), and not
> per alternate. And as we communicate the [if-able] part via the config
> options to the submodules it is not feasible to transport both
> kinds of (reference-or-die and reference-but-ignore-misses).
>
> That is why I introduced this check in the first place. If we'd go back
> to the drawing board and come up with a solution that is on a
> "per alternate" basis we could allow such things.
>
>> That is, treat it so
>> that multiple reference and reference-if-able will die, and only info
>> if we got only reference-if-able?
>>
>> Probably what's here is fine, and mixing reference and
>> reference-if-able doesn't make much sense.
>
> I think the reference-if-able doesn't make sense for one project alone
> as you can easily script around that, but is only useful if you have
> submodules in a partially checked out superproject that you want
> to reference to.
>
> Thanks,
> Stefan

I'm not sure there is a better design.  How are alternates stored? In
a config section? Or is there some way we can store the is-able per
alternate and look it up when adding them to submodule?

Thanks,
Jake

Re: [PATCHv5 8/8] clone: recursive and reference option triggers submodule alternates

[Stefan Beller]

On Tue, Aug 23, 2016 at 11:29 PM, Jacob Keller <jacob.keller@gmail.com> wrote:
> On Tue, Aug 23, 2016 at 4:03 PM, Stefan Beller <sbeller@google.com> wrote:
>>>> +
>>>> +       if (option_recursive) {
>>>> +               if (option_required_reference.nr &&
>>>> +                   option_optional_reference.nr)
>>>> +                       die(_("clone --recursive is not compatible with "
>>>> +                             "both --reference and --reference-if-able"));
>>>
>>> So if you have multiple references that don't all match we basically
>>> just refuse to allow recursive?
>>>
>>> Would it be better to simply assume that we want to die on missing
>>> references instead of failing the clone here?
>>
>> The new config options are per repo (or even set globally), and not
>> per alternate. And as we communicate the [if-able] part via the config
>> options to the submodules it is not feasible to transport both
>> kinds of (reference-or-die and reference-but-ignore-misses).
>>
>> That is why I introduced this check in the first place. If we'd go back
>> to the drawing board and come up with a solution that is on a
>> "per alternate" basis we could allow such things.
>>
>>> That is, treat it so
>>> that multiple reference and reference-if-able will die, and only info
>>> if we got only reference-if-able?
>>>
>>> Probably what's here is fine, and mixing reference and
>>> reference-if-able doesn't make much sense.
>>
>> I think the reference-if-able doesn't make sense for one project alone
>> as you can easily script around that, but is only useful if you have
>> submodules in a partially checked out superproject that you want
>> to reference to.
>>
>> Thanks,
>> Stefan
>
> I'm not sure there is a better design.  How are alternates stored? In
> a config section?

Alternates are stored in .git/objects/info/alternates
with each alternate in a new line. On that file (from
(man gitrepository-layout):

objects/info/alternates

This file records paths to alternate object stores that this object store
borrows objects from, one pathname per line. Note that not only native
Git tools use it locally, but the HTTP fetcher also tries to use it remotely;
this will usually work if you have relative paths (relative to the object
database, not to the repository!) in your alternates file, but it will not work
if you use absolute paths unless the absolute path in filesystem and web
URL is the same. See also objects/info/http-alternates.

So changing that file is out of question.
Ideally we would have a flag for each path here, though.

> Or is there some way we can store the is-able per
> alternate and look it up when adding them to submodule?

I guess we could invent a file as alternate-flags that is matches
line by line to the alternates file.

I don't think we'd want to go that way for now as it would really only
help in an edge case?

If we later find out we need the flag on a per-alternate basis we can
still come up with a solution and just not set these config variables,
so I think we'll be fine for now with this approach.

Thanks,
Stefan

Re: [PATCHv5 8/8] clone: recursive and reference option triggers submodule alternates

[Jacob Keller]

On Wed, Aug 24, 2016 at 3:52 PM, Stefan Beller <sbeller@google.com> wrote:
> On Tue, Aug 23, 2016 at 11:29 PM, Jacob Keller <jacob.keller@gmail.com> wrote:
>> On Tue, Aug 23, 2016 at 4:03 PM, Stefan Beller <sbeller@google.com> wrote:
>>>>> +
>>>>> +       if (option_recursive) {
>>>>> +               if (option_required_reference.nr &&
>>>>> +                   option_optional_reference.nr)
>>>>> +                       die(_("clone --recursive is not compatible with "
>>>>> +                             "both --reference and --reference-if-able"));
>>>>
>>>> So if you have multiple references that don't all match we basically
>>>> just refuse to allow recursive?
>>>>
>>>> Would it be better to simply assume that we want to die on missing
>>>> references instead of failing the clone here?
>>>
>>> The new config options are per repo (or even set globally), and not
>>> per alternate. And as we communicate the [if-able] part via the config
>>> options to the submodules it is not feasible to transport both
>>> kinds of (reference-or-die and reference-but-ignore-misses).
>>>
>>> That is why I introduced this check in the first place. If we'd go back
>>> to the drawing board and come up with a solution that is on a
>>> "per alternate" basis we could allow such things.
>>>
>>>> That is, treat it so
>>>> that multiple reference and reference-if-able will die, and only info
>>>> if we got only reference-if-able?
>>>>
>>>> Probably what's here is fine, and mixing reference and
>>>> reference-if-able doesn't make much sense.
>>>
>>> I think the reference-if-able doesn't make sense for one project alone
>>> as you can easily script around that, but is only useful if you have
>>> submodules in a partially checked out superproject that you want
>>> to reference to.
>>>
>>> Thanks,
>>> Stefan
>>
>> I'm not sure there is a better design.  How are alternates stored? In
>> a config section?
>
> Alternates are stored in .git/objects/info/alternates
> with each alternate in a new line. On that file (from
> (man gitrepository-layout):
>
> objects/info/alternates
>
> This file records paths to alternate object stores that this object store
> borrows objects from, one pathname per line. Note that not only native
> Git tools use it locally, but the HTTP fetcher also tries to use it remotely;
> this will usually work if you have relative paths (relative to the object
> database, not to the repository!) in your alternates file, but it will not work
> if you use absolute paths unless the absolute path in filesystem and web
> URL is the same. See also objects/info/http-alternates.
>
> So changing that file is out of question.
> Ideally we would have a flag for each path here, though.
>
>> Or is there some way we can store the is-able per
>> alternate and look it up when adding them to submodule?
>
> I guess we could invent a file as alternate-flags that is matches
> line by line to the alternates file.
>
> I don't think we'd want to go that way for now as it would really only
> help in an edge case?
>
> If we later find out we need the flag on a per-alternate basis we can
> still come up with a solution and just not set these config variables,
> so I think we'll be fine for now with this approach.
>

Yes that seems reasonable.

Thanks,
Jake

Re: [PATCHv5 8/8] clone: recursive and reference option triggers submodule alternates

[Stefan Beller]

On Wed, Aug 24, 2016 at 4:37 PM, Jacob Keller <jacob.keller@gmail.com> wrote:

> Yes that seems reasonable.
>
> Thanks,
> Jake

I reviewed all your comments and you seem to be ok with including this
series as it is queued currently?

Thanks,
Stefan

Re: [PATCHv5 8/8] clone: recursive and reference option triggers submodule alternates

[Jacob Keller]

On Tue, Aug 30, 2016 at 10:04 PM, Stefan Beller <sbeller@google.com> wrote:
> On Wed, Aug 24, 2016 at 4:37 PM, Jacob Keller <jacob.keller@gmail.com> wrote:
>
>> Yes that seems reasonable.
>>
>> Thanks,
>> Jake
>
> I reviewed all your comments and you seem to be ok with including this
> series as it is queued currently?
>
> Thanks,
> Stefan

Yea based on what's in Junio's tree, I think we've squashed in all the
suggested changes, unless there have been more suggestions I missed.

Regards,
Jake