[PATCHv4 0/6] git clone: Marry --recursive and --reference

[PATCHv4 0/6] git clone: Marry --recursive and --reference

[Stefan Beller]

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

[PATCHv4 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.737.g4a14654

[PATCHv4 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.737.g4a14654

[PATCHv4 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 b09632e..db9270e 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.737.g4a14654

[PATCHv4 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 db9270e..a10a777 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 b57f87d..a1cc71b 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -576,7 +576,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.737.g4a14654

[PATCHv4 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 | 42 ++++++--------------------------
 cache.h         |  1 +
 sha1_file.c     | 74 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 82 insertions(+), 35 deletions(-)

diff --git a/builtin/clone.c b/builtin/clone.c
index f044a8c..24b17539 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -282,44 +282,16 @@ 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 sb = STRBUF_INIT;
+	char *ref_git = compute_alternate_path(item->string, &sb);
 
-	if (!access(mkpath("%s/shallow", ref_git), F_OK))
-		die(_("reference repository '%s' is shallow"), item->string);
+	if (!ref_git)
+		die("%s", sb.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(&sb);
 	return 0;
 }
 
diff --git a/cache.h b/cache.h
index 95a0bd3..35f41f7 100644
--- a/cache.h
+++ b/cache.h
@@ -1344,6 +1344,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 02940f1..7351d8c 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -418,6 +418,80 @@ 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;
+	struct strbuf err_buf = STRBUF_INIT;
+
+	ref_git_s = real_path_if_valid(path);
+	if (!ref_git_s) {
+		strbuf_addf(&err_buf, _("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;
+		if (get_common_dir(&sb, ref_git)) {
+			strbuf_addf(&err_buf,
+				    _("reference repository '%s' as a linked "
+				      "checkout is not supported yet."),
+				    path);
+			goto out;
+		}
+
+		strbuf_addf(&err_buf, _("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"),
+			    path);
+		goto out;
+	}
+
+	if (!access(mkpath("%s/info/grafts", ref_git), F_OK)) {
+		strbuf_addf(&err_buf,
+			    _("reference repository '%s' is grafted"),
+			    path);
+		goto out;
+	}
+
+out:
+	if (err_buf.len) {
+		strbuf_addbuf(err, &err_buf);
+		free(ref_git);
+		ref_git = NULL;
+	}
+
+	strbuf_release(&err_buf);
+	return ref_git;
+}
+
 int foreach_alt_odb(alt_odb_fn fn, void *cb)
 {
 	struct alternate_object_database *ent;
-- 
2.9.2.737.g4a14654

[PATCHv4 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 24b17539..7ccbdef 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")),
@@ -297,7 +297,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,
@@ -949,7 +949,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.737.g4a14654

[PATCHv4 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             | 29 ++++++++++++++++++++++-------
 2 files changed, 26 insertions(+), 8 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 7ccbdef..fdb2ca9 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,13 +286,20 @@ 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;
+	int *required = cb_data;
 	char *ref_git = compute_alternate_path(item->string, &sb);
 
-	if (!ref_git)
-		die("%s", sb.buf);
-
-	strbuf_addf(&sb, "%s/objects", ref_git);
-	add_to_alternates_file(sb.buf);
+	if (!ref_git) {
+		if (*required)
+			die("%s", sb.buf);
+		else
+			fprintf(stderr,
+				_("info: Could not add alternate for '%s': %s\n"),
+				item->string, sb.buf);
+	} else {
+		strbuf_addf(&sb, "%s/objects", ref_git);
+		add_to_alternates_file(sb.buf);
+	}
 
 	strbuf_release(&sb);
 	return 0;
@@ -297,7 +307,12 @@ static int add_one_reference(struct string_list_item *item, void *cb_data)
 
 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,
@@ -949,7 +964,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.737.g4a14654

[PATCHv4 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 0bcb679..e09d32c 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2847,6 +2847,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 fdb2ca9..0593aee 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -944,6 +944,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 a10a777..a71c903 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 }
+
+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.737.g4a14654

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

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> +	struct strbuf sb = STRBUF_INIT;
> +	char *ref_git = compute_alternate_path(item->string, &sb);

Who owns the memory for ref_git?

> -	if (!access(mkpath("%s/shallow", ref_git), F_OK))
> -		die(_("reference repository '%s' is shallow"), item->string);
> +	if (!ref_git)
> +		die("%s", sb.buf);

Presumably the second argument to compute_alternate_path() is a
strbuf to receive the error message?  It is unfortunate that the
variable used for this purpose is a bland "sb", but perhaps that
cannot be helped as you would reuse that strbuf for a different
purpose (i.e. not to store the error message, but to formulate a
pathname).

> -	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(&sb);

I am wondering about the loss of free() here in the first comment.

> +/*
> + * 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;
> +	struct strbuf err_buf = STRBUF_INIT;

Why do you need "err_buf", instead of directly writing the error to
"err", especially if "err" is not optional?

> + ...
> +out:
> +	if (err_buf.len) {
> +		strbuf_addbuf(err, &err_buf);
> +		free(ref_git);
> +		ref_git = NULL;
> +	}
> +
> +	strbuf_release(&err_buf);
> +	return ref_git;
> +}

So ref_git is a piece of memory on heap, and the caller is
responsible for not leaking it.

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

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> +int add_possible_reference_from_superproject(
> +		struct alternate_object_database *alt, void *sas_cb)
> +{
> +	struct submodule_alternate_setup *sas = sas_cb;
> +
> + ...
> +	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);

OK.  Thanks to the refactoring in the earlier step, this has become
quite straight-forward and robust; this code will always decide if
the other directory can be used as an alternate the same way the
"git clone" we will call would decide.

Good.

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

[Stefan Beller]

On Fri, Aug 12, 2016 at 3:25 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> +     struct strbuf sb = STRBUF_INIT;
>> +     char *ref_git = compute_alternate_path(item->string, &sb);
>
> Who owns the memory for ref_git?

The caller of compute_alternate_path(..), which makes
add_one_reference faulty as of this patch.

>
>> -     if (!access(mkpath("%s/shallow", ref_git), F_OK))
>> -             die(_("reference repository '%s' is shallow"), item->string);
>> +     if (!ref_git)
>> +             die("%s", sb.buf);
>
> Presumably the second argument to compute_alternate_path() is a
> strbuf to receive the error message?  It is unfortunate that the
> variable used for this purpose is a bland "sb", but perhaps that
> cannot be helped as you would reuse that strbuf for a different
> purpose (i.e. not to store the error message, but to formulate a
> pathname).

Ok. I had an intermediate version with 2 strbufs but for some reason I
decided one is better. We'll have 2 again. (err and sb; sb will have a
smaller scope only in the else part.)

>
>> -     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(&sb);
>
> I am wondering about the loss of free() here in the first comment.

fixed in a reroll.

>
>> +/*
>> + * 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;
>> +     struct strbuf err_buf = STRBUF_INIT;
>
> Why do you need "err_buf", instead of directly writing the error to
> "err", especially if "err" is not optional?
>
>> + ...
>> +out:
>> +     if (err_buf.len) {

If we were directly writing to err, we would have checked
err.len here. Then you open up a subtle way of saying "dry run"
by giving a non empty error buffer.

I contemplated doing that actually instead of splitting up into 2 functions,
but I considered that bad taste as it would require documentation.

>> +             strbuf_addbuf(err, &err_buf);
>> +             free(ref_git);
>> +             ref_git = NULL;
>> +     }
>> +
>> +     strbuf_release(&err_buf);
>> +     return ref_git;
>> +}
>
> So ref_git is a piece of memory on heap, and the caller is
> responsible for not leaking it.

Correct.

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

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> On Fri, Aug 12, 2016 at 3:25 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Stefan Beller <sbeller@google.com> writes:
>>
>>> +     struct strbuf sb = STRBUF_INIT;
>>> +     char *ref_git = compute_alternate_path(item->string, &sb);
>>
>> Who owns the memory for ref_git?
>
> The caller of compute_alternate_path(..), which makes
> add_one_reference faulty as of this patch.
>
>>
>>> -     if (!access(mkpath("%s/shallow", ref_git), F_OK))
>>> -             die(_("reference repository '%s' is shallow"), item->string);
>>> +     if (!ref_git)
>>> +             die("%s", sb.buf);
>>
>> Presumably the second argument to compute_alternate_path() is a
>> strbuf to receive the error message?  It is unfortunate that the
>> variable used for this purpose is a bland "sb", but perhaps that
>> cannot be helped as you would reuse that strbuf for a different
>> purpose (i.e. not to store the error message, but to formulate a
>> pathname).
>
> Ok. I had an intermediate version with 2 strbufs but for some reason I
> decided one is better. We'll have 2 again. (err and sb; sb will have a
> smaller scope only in the else part.)

My "unfortunate" was meant to say "it is unfortunate that this is
the best, adding one extra variable is not worth the cost".

>> Why do you need "err_buf", instead of directly writing the error to
>> "err", especially if "err" is not optional?
>>
>>> + ...
>>> +out:
>>> +     if (err_buf.len) {
>
> If we were directly writing to err, we would have checked
> err.len here. Then you open up a subtle way of saying "dry run"
> by giving a non empty error buffer.
>
> I contemplated doing that actually instead of splitting up into 2 functions,
> but I considered that bad taste as it would require documentation.

Sorry, but I do not understand this response at all.  I am still
talking about keeping one function "compute_alternate_path()".  The
suggestion was just to drop "struct strbuf err_buf" local variable,
and instead add the error messages the patch adds to err_buf with
code like:

> +	if (!ref_git_s) {
> +		strbuf_addf(&err_buf, _("path '%s' does not exist"), path);
> +		goto out;

to directly do

		strbuf_addf(err, _("path '%s' does not exist"), path);

instead.  That way you do not have to move the bytes around from one
buffer to the other, like this:

>>> +             strbuf_addbuf(err, &err_buf);
>>> +             free(ref_git);
>>> +             ref_git = NULL;
>>> +     }

If you allow err->len to be non-zero upon entry, you'd need a way to
remember if you noticed an error, so that you can free and clear
ref_git after "out:" label, and doing so with a separate variable
would make the code more readable, I would think, either by (1)
recording the original err->len upon entry, and comparing it with
err->len at "out:" label, or (2) using a new bool "seen_error = 0"
and setting it to true when you diagnose an error.  The latter would
make the code a bit more verbose but I suspect the result would be
easier to read than the former.

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

[Stefan Beller]

On Mon, Aug 15, 2016 at 1:36 PM, Junio C Hamano <gitster@pobox.com> wrote:
>
>>> Why do you need "err_buf", instead of directly writing the error to
>>> "err", especially if "err" is not optional?
>>>
>>>> + ...
>>>> +out:
>>>> +     if (err_buf.len) {
>>
>> If we were directly writing to err, we would have checked
>> err.len here. Then you open up a subtle way of saying "dry run"
>> by giving a non empty error buffer.
>>
>> I contemplated doing that actually instead of splitting up into 2 functions,
>> but I considered that bad taste as it would require documentation.
>
> Sorry, but I do not understand this response at all.

Me neither. I think I elided the interesting part.

> I am still
> talking about keeping one function "compute_alternate_path()".  The
> suggestion was just to drop "struct strbuf err_buf" local variable,
> and instead add the error messages the patch adds to err_buf with
> code like:
>
>> +     if (!ref_git_s) {
>> +             strbuf_addf(&err_buf, _("path '%s' does not exist"), path);
>> +             goto out;
>
> to directly do
>
>                 strbuf_addf(err, _("path '%s' does not exist"), path);

done.

> err->len at "out:" label, or (2) using a new bool "seen_error = 0"
> and setting it to true when you diagnose an error.  The latter would
> make the code a bit more verbose but I suspect the result would be
> easier to read than the former.
>

(2) is very readable, will reroll with that.

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

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

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

I have to admit that I need to follow the codepath in config.c every
time to check, but I _think_ git_config_get_string() gives you your
own copy of the value.  As this function does not give ownership of
sm_alternate or error_strategy to something else, they are leaked
every time this function is called, I think.

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

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

[Stefan Beller]

On Wed, Aug 17, 2016 at 1:02 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> +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);
>
> I have to admit that I need to follow the codepath in config.c every
> time to check, but I _think_ git_config_get_string() gives you your
> own copy of the value.  As this function does not give ownership of
> sm_alternate or error_strategy to something else, they are leaked
> every time this function is called, I think.

There are quite a few more occurrences of git_config_get_string
in the submodule helper, I'll also look at those.

Thanks,
Stefan

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

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Stefan Beller <sbeller@google.com> writes:
>
>> +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);
>
> I have to admit that I need to follow the codepath in config.c every
> time to check, but I _think_ git_config_get_string() gives you your
> own copy of the value.  As this function does not give ownership of
> sm_alternate or error_strategy to something else, they are leaked
> every time this function is called, I think.
>
>> +	sas.submodule_name = sm_name;
>> +	sas.reference = reference;
>> +	if (!strcmp(error_strategy, "die"))
>> +		sas.error_mode = SUBMODULE_ALTERNATE_ERROR_DIE;

Another thing I noticed but forgot to mention.  Can error_strategy
be NULL here?  We are assuming sm_alternate can be, so I presume
that it is sensible to protect against dereferencing a NULL here,
too?

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

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

[Stefan Beller]

On Wed, Aug 17, 2016 at 2:31 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> Stefan Beller <sbeller@google.com> writes:
>>
>>> +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);
>>
>> I have to admit that I need to follow the codepath in config.c every
>> time to check, but I _think_ git_config_get_string() gives you your
>> own copy of the value.  As this function does not give ownership of
>> sm_alternate or error_strategy to something else, they are leaked
>> every time this function is called, I think.
>>
>>> +    sas.submodule_name = sm_name;
>>> +    sas.reference = reference;
>>> +    if (!strcmp(error_strategy, "die"))
>>> +            sas.error_mode = SUBMODULE_ALTERNATE_ERROR_DIE;
>
> Another thing I noticed but forgot to mention.  Can error_strategy
> be NULL here?  We are assuming sm_alternate can be, so I presume
> that it is sensible to protect against dereferencing a NULL here,
> too?

Oh!

Of course we need to fix that too. That slipped in because I assumed that those
two variables go in tandem (if sm_alternate is set, then
error_strategy is set as well),
but that is not the case of course.

Which leads to another thing: we need to have a default for error_strategy.
I think "die" is reasonable here. That also needs a documentation update.

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