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

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

[Stefan Beller]

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

[PATCHv3 1/9] 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..beee0bb 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.583.gd6329be.dirty

Re: [PATCHv3 1/9] t7408: modernize style

[Eric Sunshine]

On Tue, Aug 9, 2016 at 12:08 AM, Stefan Beller <sbeller@google.com> wrote:
> t7408: modernize style
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
> diff --git a/t/t7408-submodule-reference.sh b/t/t7408-submodule-reference.sh
> @@ -8,74 +8,76 @@ test_description='test clone --reference'
> +test_expect_success 'that reference gets used with add' '
> +       (
> +               cd super/sub &&
> +               echo "0 objects, 0 kilobytes" > expected &&

Since you're modernizing the style, perhaps drop the space after '>'
here and elsewhere.

> +               git count-objects > current &&
> +               diff expected current

Modern practice is to call this 'actual' rather than 'current', but
perhaps that's outside the scope of this patch(?).

> +       )
> +'

Re: [PATCHv3 1/9] t7408: modernize style

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> On Tue, Aug 9, 2016 at 12:08 AM, Stefan Beller <sbeller@google.com> wrote:
>> t7408: modernize style
>>
>> Signed-off-by: Stefan Beller <sbeller@google.com>
>> ---
>> diff --git a/t/t7408-submodule-reference.sh b/t/t7408-submodule-reference.sh
>> @@ -8,74 +8,76 @@ test_description='test clone --reference'
>> +test_expect_success 'that reference gets used with add' '
>> +       (
>> +               cd super/sub &&
>> +               echo "0 objects, 0 kilobytes" > expected &&
>
> Since you're modernizing the style, perhaps drop the space after '>'
> here and elsewhere.
>
>> +               git count-objects > current &&
>> +               diff expected current
>
> Modern practice is to call this 'actual' rather than 'current', but
> perhaps that's outside the scope of this patch(?).

Not just that, modern practice is to use "test_cmp" not "diff".

Re: [PATCHv3 1/9] t7408: modernize style

[Junio C Hamano]

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

> Eric Sunshine <sunshine@sunshineco.com> writes:
>
>> On Tue, Aug 9, 2016 at 12:08 AM, Stefan Beller <sbeller@google.com> wrote:
>>> t7408: modernize style
>>>
>>> Signed-off-by: Stefan Beller <sbeller@google.com>
>>> ---
>>> diff --git a/t/t7408-submodule-reference.sh b/t/t7408-submodule-reference.sh
>>> @@ -8,74 +8,76 @@ test_description='test clone --reference'
>>> +test_expect_success 'that reference gets used with add' '
>>> +       (
>>> +               cd super/sub &&
>>> +               echo "0 objects, 0 kilobytes" > expected &&
>>
>> Since you're modernizing the style, perhaps drop the space after '>'
>> here and elsewhere.
>>
>>> +               git count-objects > current &&
>>> +               diff expected current
>>
>> Modern practice is to call this 'actual' rather than 'current', but
>> perhaps that's outside the scope of this patch(?).
>
> Not just that, modern practice is to use "test_cmp" not "diff".

Sorry, hit the "send" too soon, before writing "But all of that
comes in the next step."

Re: [PATCHv3 1/9] t7408: modernize style

[Stefan Beller]

On Tue, Aug 9, 2016 at 8:51 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> Eric Sunshine <sunshine@sunshineco.com> writes:
>>

I originally thought that it may be easier to have 2 patches.
This first one is very gentle and "obviously correct" as it only changes
formatting and drops the directory changes.

The second patch goes for renaming ans subtle style issues,
combining some tests, so it is more likely to break.

After this review, I consider using just one patch and do it all
at once to not confuse the readers as otherwise I should reword
the commit message with the rationale of doing it in two patches.

Re: [PATCHv3 1/9] t7408: modernize style

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> On Tue, Aug 9, 2016 at 8:51 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> Junio C Hamano <gitster@pobox.com> writes:
>>
>>> Eric Sunshine <sunshine@sunshineco.com> writes:
>>>
>
> I originally thought that it may be easier to have 2 patches.
> This first one is very gentle and "obviously correct" as it only changes
> formatting and drops the directory changes.
>
> The second patch goes for renaming ans subtle style issues,
> combining some tests, so it is more likely to break.
>
> After this review, I consider using just one patch and do it all
> at once to not confuse the readers as otherwise I should reword
> the commit message with the rationale of doing it in two patches.

FWIW, I would think your split between 1/ and 2/ were very sensible,
and have a slight preference for keeping them separate.

If you already have squashed, I do not insist you to split it again;
it is not a big deal either way.

Re: [PATCHv3 1/9] t7408: modernize style

[Eric Sunshine]

On Tue, Aug 9, 2016 at 1:39 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>> I originally thought that it may be easier to have 2 patches.
>> This first one is very gentle and "obviously correct" as it only changes
>> formatting and drops the directory changes.
>>
>> The second patch goes for renaming ans subtle style issues,
>> combining some tests, so it is more likely to break.
>>
>> After this review, I consider using just one patch and do it all
>> at once to not confuse the readers as otherwise I should reword
>> the commit message with the rationale of doing it in two patches.
>
> FWIW, I would think your split between 1/ and 2/ were very sensible,
> and have a slight preference for keeping them separate.

The review comment about renaming "current" to "actual" was made
without having looked yet at patch 2. Having now seen patch 2, I agree
with Junio that the existing split is preferable.

So, only the review comment about dropping space after '>' remains relevant.

[PATCHv3 2/9] 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 | 57 ++++++++++++++++++------------------------
 1 file changed, 25 insertions(+), 32 deletions(-)

diff --git a/t/t7408-submodule-reference.sh b/t/t7408-submodule-reference.sh
index beee0bb..4a1b8f0 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,44 +49,28 @@ 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' '
-	(
-		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
-'
+# The tests up to this point, and repositories created by them
+# (A, B, super and super/sub), are about setting up the stage
+# forsubsequent 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.583.gd6329be.dirty

Re: [PATCHv3 2/9] t7408: merge short tests, factor out testing method

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> +# The tests up to this point, and repositories created by them
> +# (A, B, super and super/sub), are about setting up the stage
> +# forsubsequent tests and meant to be kept throughout the

s/forsub/for sub/;

Re: [PATCHv3 2/9] t7408: merge short tests, factor out testing method

[Stefan Beller]

On Tue, Aug 9, 2016 at 9:41 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> +# The tests up to this point, and repositories created by them
>> +# (A, B, super and super/sub), are about setting up the stage
>> +# forsubsequent tests and meant to be kept throughout the
>
> s/forsub/for sub/;

ok.

[PATCHv3 3/9] 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.583.gd6329be.dirty

[PATCHv3 4/9] 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.583.gd6329be.dirty

[PATCHv3 5/9] 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 f044a8c..052a769 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")),
@@ -325,7 +325,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,
@@ -977,7 +977,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.583.gd6329be.dirty

[PATCHv2 5/6] submodule update: add super-reference flag

[Stefan Beller]

When we have a another clone of a superproject, we may want to
mirror the submodules using alternates. Introduce an option
`--super-reference` that let's a user point to another superproject,
which is assumed to have the same structure as the one they are
running the "submodule update" command from and has all submodules
checked out to borrow the submodule objects from within the other
superprojects git directory.

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

diff --git a/Documentation/git-submodule.txt b/Documentation/git-submodule.txt
index bf3bb37..6f2f873 100644
--- a/Documentation/git-submodule.txt
+++ b/Documentation/git-submodule.txt
@@ -16,7 +16,7 @@ SYNOPSIS
 'git submodule' [--quiet] deinit [-f|--force] (--all|[--] <path>...)
 'git submodule' [--quiet] update [--init] [--remote] [-N|--no-fetch]
 	      [--[no-]recommend-shallow] [-f|--force] [--rebase|--merge]
-	      [--reference <repository>] [--depth <depth>] [--recursive]
+	      [--[super-]reference <repository>] [--depth <depth>] [--recursive]
 	      [--jobs <n>] [--] [<path>...]
 'git submodule' [--quiet] summary [--cached|--files] [(-n|--summary-limit) <n>]
 	      [commit] [--] [<path>...]
@@ -370,6 +370,12 @@ the submodule itself.
 	This option is only valid for add and update commands.  These
 	commands sometimes need to clone a remote repository. In this case,
 	this option will be passed to the linkgit:git-clone[1] command.
+
+--super-reference <superproject repository>::
+	This option is only valid for the update command. When update needs
+	to clone a repository, a reference will be passed to the clone command
+	that points at the submodule path inside the reference superproject.
+
 +
 *NOTE*: Do *not* use this option unless you have read the note
 for linkgit:git-clone[1]'s `--reference` and `--shared` options carefully.
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index b7710a7..ea6b27c 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -584,6 +584,7 @@ struct submodule_update_clone {
 	int quiet;
 	int recommend_shallow;
 	struct string_list references;
+	struct string_list super_references;
 	const char *depth;
 	const char *recursive_prefix;
 	const char *prefix;
@@ -600,7 +601,7 @@ struct submodule_update_clone {
 };
 #define SUBMODULE_UPDATE_CLONE_INIT {0, MODULE_LIST_INIT, 0, \
 	SUBMODULE_UPDATE_STRATEGY_INIT, 0, -1, STRING_LIST_INIT_DUP, \
-	NULL, NULL, NULL, \
+	STRING_LIST_INIT_DUP, NULL, NULL, NULL, \
 	STRING_LIST_INIT_DUP, 0, NULL, 0, 0}
 
 
@@ -715,6 +716,15 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce,
 		for_each_string_list_item(item, &suc->references)
 			argv_array_pushl(&child->args, "--reference", item->string, NULL);
 	}
+	if (suc->super_references.nr) {
+		struct string_list_item *item;
+		for_each_string_list_item(item, &suc->super_references) {
+			strbuf_reset(&sb);
+			argv_array_pushf(&child->args, "--reference=%s/%s",
+					 relative_path(item->string, suc->prefix, &sb),
+					 sub->path);
+		}
+	}
 	if (suc->depth)
 		argv_array_push(&child->args, suc->depth);
 
@@ -835,6 +845,8 @@ static int update_clone(int argc, const char **argv, const char *prefix)
 			   N_("rebase, merge, checkout or none")),
 		OPT_STRING_LIST(0, "reference", &suc.references, N_("repo"),
 			   N_("reference repository")),
+		OPT_STRING_LIST(0, "super-reference", &suc.super_references, N_("repo"),
+			   N_("superproject of a reference repository")),
 		OPT_STRING(0, "depth", &suc.depth, "<depth>",
 			   N_("Create a shallow clone truncated to the "
 			      "specified number of revisions")),
diff --git a/git-submodule.sh b/git-submodule.sh
index 3b412f5..17f4ace 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -34,6 +34,7 @@ command=
 branch=
 force=
 reference=
+superreference=
 cached=
 recursive=
 init=
@@ -520,6 +521,14 @@ cmd_update()
 		--reference=*)
 			reference="$1"
 			;;
+		--super-reference)
+			case "$2" in '') usage ;; esac
+			superreference="--super-reference=$2"
+			shift
+			;;
+		--super-reference=*)
+			superreference="$1"
+			;;
 		-m|--merge)
 			update="merge"
 			;;
@@ -576,6 +585,7 @@ cmd_update()
 		${prefix:+--recursive-prefix "$prefix"} \
 		${update:+--update "$update"} \
 		${reference:+"$reference"} \
+		${superreference:+"$superreference"} \
 		${depth:+--depth "$depth"} \
 		${recommend_shallow:+"$recommend_shallow"} \
 		${jobs:+$jobs} \
-- 
2.9.2.572.g9d9644e.dirty

[PATCHv3 6/9] clone: implement optional references

[Stefan Beller]

In a later patch we want to try to create alternates for
all 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             | 32 ++++++++++++++++++++++++++------
 2 files changed, 30 insertions(+), 7 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 052a769..8f3c4d4 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,11 +286,22 @@ 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;
+	const char *repo, *ref_git_s;
+	int *required = cb_data;
 	struct strbuf alternate = STRBUF_INIT;
 
-	/* Beware: read_gitfile(), real_path() and mkpath() return static buffer */
-	ref_git = xstrdup(real_path(item->string));
+	ref_git_s = *required ?
+			real_path(item->string) :
+			real_path_if_valid(item->string);
+	if (!ref_git_s) {
+		warning(_("Not using proposed alternate %s"), item->string);
+		return 0;
+	} else
+		/*
+		 * Beware: read_gitfile(), real_path() and mkpath()
+		 * return static buffer
+		 */
+		ref_git = xstrdup(ref_git_s);
 
 	repo = read_gitfile(ref_git);
 	if (!repo)
@@ -304,7 +318,8 @@ static int add_one_reference(struct string_list_item *item, void *cb_data)
 	} 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."),
+			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);
@@ -325,7 +340,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,
@@ -977,7 +997,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.583.gd6329be.dirty

Re: [PATCHv3 6/9] clone: implement optional references

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> @@ -283,11 +286,22 @@ 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;
> +	const char *repo, *ref_git_s;
> +	int *required = cb_data;
>  	struct strbuf alternate = STRBUF_INIT;
>  
> -	/* Beware: read_gitfile(), real_path() and mkpath() return static buffer */
> -	ref_git = xstrdup(real_path(item->string));
> +	ref_git_s = *required ?
> +			real_path(item->string) :
> +			real_path_if_valid(item->string);
> +	if (!ref_git_s) {
> +		warning(_("Not using proposed alternate %s"), item->string);

If I am reading the implementation of real_path_internal()
correctly, the most relevant reason that an "if-able" repository
cannot be used causes real_path_if_valid() to return NULL.

Let's say your superproject borrows from ~/w/super, whose submodule
repositories (if cloned) are directly in "~/w/super/.git/modules/".
When you clone a submodule X for your superproject, you allow it to
borrow from "~/w/super/.git/modules/X" if there is one available.

I think both real_path() and real_path_if_valid() happily returns
the real path to "~/w/super/.git/modules/" with "X" concatenated at
the end, as long as that 'modules' directory exists, even when there
is no X inside.

Using if_valid() is still necessary to avoid a totally bogus path to
cause real_path() to barf.  I am just saying that this warning is
not so interesting, and a real check, "do we have a repository
there?  If not, don't add it as an alternate" must be somewhere
else.

> +		return 0;
> +	} else
> +		/*
> +		 * Beware: read_gitfile(), real_path() and mkpath()
> +		 * return static buffer
> +		 */
> +		ref_git = xstrdup(ref_git_s);
>  
>  	repo = read_gitfile(ref_git);
>  	if (!repo)
> @@ -304,7 +318,8 @@ static int add_one_reference(struct string_list_item *item, void *cb_data)
>  	} 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."),
> +			die(_("reference repository '%s' as a "
> +			      "linked checkout is not supported yet."),
>  			    item->string);

And curiously, this is not such a check.  This is an unrelated change.

>  		die(_("reference repository '%s' is not a local repository."),
>  		    item->string);

You need to arrange this die() not to trigger when you are asked to
borrow from there if there is one to borrow from.  I see a few other
die() following this check to avoid using shallow repositories and
repositories with grafts, but I think they all should turn into a
"Nah, this is unusable, and I was asked to borrow _only_ if the
repository is usable, so I won't use it."

Re: [PATCHv3 6/9] clone: implement optional references

[Junio C Hamano]

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

> If I am reading the implementation of real_path_internal()
> correctly, the most relevant reason that an "if-able" repository
> cannot be used causes real_path_if_valid() to return NULL.

Oops, too many proofreading and rephrasing.  Please insert "I do not
think that" before "the most relevant".

Re: [PATCHv3 6/9] clone: implement optional references

[Stefan Beller]

On Tue, Aug 9, 2016 at 10:54 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> If I am reading the implementation of real_path_internal()
>> correctly, the most relevant reason that an "if-able" repository
>> cannot be used causes real_path_if_valid() to return NULL.
>
> Oops, too many proofreading and rephrasing.  Please insert "I do not
> think that" before "the most relevant".

Side note:
  It took me a while to mentally process and reread this.
  It would have been easier with just
  s/the most relevant reason/I do not think that the most relevant reason/
  but that is just me being used to s/ expressions by now.

I think our emails nearly crossed. In the other thread I wondered if
you had different expectations on the -if-able part than I do.

For the expected use case I do expect that the missing path is the
most relevant thing (the submodule is not checked out).

Of course the submodule may be cloned shallow as it was recommended
in the .gitmodules file, so we should also care about that use case.


> Let's say your superproject borrows from ~/w/super, whose submodule
> repositories (if cloned) are directly in "~/w/super/.git/modules/".
> When you clone a submodule X for your superproject, you allow it to
> borrow from "~/w/super/.git/modules/X" if there is one available.

which is why we need to pass in ..../X/ with an ending slash.
See the comment in 8/9.

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

Thinking about that we may want to not rely on such a hack,
but make it clearer.

Thanks,
Stefan

Re: [PATCHv3 6/9] clone: implement optional references

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> Thinking about that we may want to not rely on such a hack,
> but make it clearer.

But that is far from sufficient, isn't it?

You'd need to bypass "not a local repository", "shallow" and "is
grafted" anyway, so in that sense, that hack is not doing much.
If modules/X (or modules/X/) does not exist, it won't pass these
checks, so burdening the readers with slash trickery is not really
worth their time.

[PATCHv2 6/6] clone: reference flag is used for submodules as well

[Stefan Beller]

When giving a --reference while also giving --recurse, the alternates
for the submodules are assumed to be in the superproject as well.

In case they are not, we error out when cloning the submodule.
However we error out completely, we did not record the alternates yet,
so a following update command succeeds as usual (with no alternates).

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 Documentation/git-clone.txt    |  5 +++++
 builtin/clone.c                | 22 ++++++++++++++++++----
 t/t7408-submodule-reference.sh | 31 ++++++++++++++++++++++++++++++-
 3 files changed, 53 insertions(+), 5 deletions(-)

diff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt
index ec41d3d..2c6dea6 100644
--- a/Documentation/git-clone.txt
+++ b/Documentation/git-clone.txt
@@ -100,6 +100,11 @@ objects from the source repository into a pack in the cloned repository.
 +
 *NOTE*: see the NOTE for the `--shared` option, and also the
 `--dissociate` option.
++
+When `--reference` is given together with `--recursive`,
+the reference repository is assumed to contain the submodules
+as well and the submodules are setup as alternates of the
+submodules in the given reference project.
 
 --dissociate::
 	Borrow the objects from reference repositories specified
diff --git a/builtin/clone.c b/builtin/clone.c
index f044a8c..e8272b5 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_reference = STRING_LIST_INIT_NODUP;
+static struct string_list super_references = STRING_LIST_INIT_DUP;
 static int option_dissociate;
 static int max_jobs = -1;
 
@@ -280,10 +281,11 @@ static void strip_trailing_slashes(char *dir)
 	*end = '\0';
 }
 
-static int add_one_reference(struct string_list_item *item, void *cb_data)
+static int add_one_reference(struct string_list_item *item, void *cb_dir)
 {
 	char *ref_git;
 	const char *repo;
+	const char *dir = cb_dir;
 	struct strbuf alternate = STRBUF_INIT;
 
 	/* Beware: read_gitfile(), real_path() and mkpath() return static buffer */
@@ -316,6 +318,13 @@ static int add_one_reference(struct string_list_item *item, void *cb_data)
 	if (!access(mkpath("%s/info/grafts", ref_git), F_OK))
 		die(_("reference repository '%s' is grafted"), item->string);
 
+	if (option_recursive) {
+		struct strbuf sb = STRBUF_INIT;
+		char *rel = xstrdup(relative_path(item->string, dir, &sb));
+		string_list_append(&super_references, rel);
+		strbuf_reset(&sb);
+	}
+
 	strbuf_addf(&alternate, "%s/objects", ref_git);
 	add_to_alternates_file(alternate.buf);
 	strbuf_release(&alternate);
@@ -323,9 +332,9 @@ static int add_one_reference(struct string_list_item *item, void *cb_data)
 	return 0;
 }
 
-static void setup_reference(void)
+static void setup_reference(char *dir)
 {
-	for_each_string_list(&option_reference, add_one_reference, NULL);
+	for_each_string_list(&option_reference, add_one_reference, dir);
 }
 
 static void copy_alternates(struct strbuf *src, struct strbuf *dst,
@@ -736,6 +745,7 @@ static int checkout(void)
 
 	if (!err && option_recursive) {
 		struct argv_array args = ARGV_ARRAY_INIT;
+		static struct string_list_item *item;
 		argv_array_pushl(&args, "submodule", "update", "--init", "--recursive", NULL);
 
 		if (option_shallow_submodules == 1)
@@ -744,6 +754,10 @@ static int checkout(void)
 		if (max_jobs != -1)
 			argv_array_pushf(&args, "--jobs=%d", max_jobs);
 
+		if (super_references.nr)
+			for_each_string_list_item(item, &super_references)
+				argv_array_pushf(&args, "--super-reference=%s", item->string);
+
 		err = run_command_v_opt(args.argv, RUN_GIT_CMD);
 		argv_array_clear(&args);
 	}
@@ -978,7 +992,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 	strbuf_reset(&key);
 
 	if (option_reference.nr)
-		setup_reference();
+		setup_reference(dir);
 
 	fetch_pattern = value.buf;
 	refspec = parse_fetch_refspec(1, &fetch_pattern);
diff --git a/t/t7408-submodule-reference.sh b/t/t7408-submodule-reference.sh
index 1d9326e..588d53e 100755
--- a/t/t7408-submodule-reference.sh
+++ b/t/t7408-submodule-reference.sh
@@ -53,7 +53,8 @@ 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
+		git commit -m B-super-added &&
+		git repack -ad
 	) &&
 	test_alternate_usage super/.git/modules/sub/objects/info/alternates super/sub
 '
@@ -65,4 +66,32 @@ test_expect_success 'updating superproject keeps alternates' '
 	test_alternate_usage 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_usage .git/objects/info/alternates . &&
+		# test submodule has correct setup
+		test_alternate_usage .git/modules/sub/objects/info/alternates sub
+	)
+'
+
+test_expect_success 'cloning superproject, missing submodule alternates' '
+	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_usage .git/objects/info/alternates . &&
+		# update of the submodule succeeds
+		git submodule update --init &&
+		# and we have no alternates:
+		test_must_fail test_alternate_usage .git/modules/sub/objects/info/alternates sub &&
+		test_path_is_file sub/file1
+	)
+'
+
 test_done
-- 
2.9.2.572.g9d9644e.dirty

[PATCHv3 7/9] submodule helper: pass through --reference-if-able

[Stefan Beller]

In a later patch we need to pass optional references
through to git clone, so add the capability for it.

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

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 7096848..f360473 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -442,7 +442,8 @@ 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, struct string_list *reference, int quiet)
+			   const char *depth, struct string_list *reference,
+			   struct string_list *reference_if_able, int quiet)
 {
 	struct child_process cp;
 	child_process_init(&cp);
@@ -459,6 +460,12 @@ static int clone_submodule(const char *path, const char *gitdir, const char *url
 			argv_array_pushl(&cp.args, "--reference",
 					 item->string, NULL);
 	}
+	if (reference_if_able->nr) {
+		struct string_list_item *item;
+		for_each_string_list_item(item, reference_if_able)
+			argv_array_pushl(&cp.args, "--reference-if-able",
+					 item->string, NULL);
+	}
 	if (gitdir && *gitdir)
 		argv_array_pushl(&cp.args, "--separate-git-dir", gitdir, NULL);
 
@@ -481,6 +488,7 @@ static int module_clone(int argc, const char **argv, const char *prefix)
 	struct strbuf rel_path = STRBUF_INIT;
 	struct strbuf sb = STRBUF_INIT;
 	struct string_list reference = STRING_LIST_INIT_NODUP;
+	struct string_list reference_if_able = STRING_LIST_INIT_NODUP;
 
 	struct option module_clone_options[] = {
 		OPT_STRING(0, "prefix", &prefix,
@@ -498,6 +506,9 @@ static int module_clone(int argc, const char **argv, const char *prefix)
 		OPT_STRING_LIST(0, "reference", &reference,
 			   N_("repo"),
 			   N_("reference repository")),
+		OPT_STRING_LIST(0, "reference-if-able", &reference_if_able,
+			   N_("repo"),
+			   N_("reference repository")),
 		OPT_STRING(0, "depth", &depth,
 			   N_("string"),
 			   N_("depth for shallow clones")),
@@ -507,8 +518,8 @@ static int module_clone(int argc, const char **argv, const char *prefix)
 
 	const char *const git_submodule_helper_usage[] = {
 		N_("git submodule--helper clone [--prefix=<path>] [--quiet] "
-		   "[--reference <repository>] [--name <name>] [--depth <depth>] "
-		   "--url <url> --path <path>"),
+		   "[--reference[-if-able] <repository>] [--name <name>] "
+		   "[--depth <depth>] --url <url> --path <path>"),
 		NULL
 	};
 
@@ -532,7 +543,8 @@ 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,
+		    &reference_if_able, quiet))
 			die(_("clone of '%s' into submodule path '%s' failed"),
 			    url, path);
 	} else {
-- 
2.9.2.583.gd6329be.dirty

[PATCHv3 8/9] submodule: try alternates when superproject has an alternate

[Stefan Beller]

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 Documentation/git-clone.txt    |  4 ++++
 builtin/submodule--helper.c    | 47 ++++++++++++++++++++++++++++++++++++++++++
 t/t7408-submodule-reference.sh | 29 +++++++++++++++++++++++++-
 3 files changed, 79 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt
index e316c4b..cadf138 100644
--- a/Documentation/git-clone.txt
+++ b/Documentation/git-clone.txt
@@ -103,6 +103,10 @@ objects from the source repository into a pack in the cloned repository.
 +
 *NOTE*: see the NOTE for the `--shared` option, and also the
 `--dissociate` option.
++
+When using --reference any submodule that is cloned
+sets up a corresponding alternate at $GIT_DIR/modules if such a
+an alternate exists.
 
 --dissociate::
 	Borrow the objects from reference repositories specified
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index f360473..fc14843 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -635,6 +635,45 @@ static void next_submodule_warn_missing(struct submodule_update_clone *suc,
 	}
 }
 
+struct submodule_alternate_setup {
+	struct submodule_update_clone *suc;
+	const char *submodule_name;
+	struct child_process *child;
+	struct strbuf *out;
+};
+
+int add_possible_reference(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")) {
+		struct strbuf sb = 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);
+		argv_array_pushf(&sas->child->args,
+				 "--reference-if-able=%s", sb.buf);
+		strbuf_release(&sb);
+	}
+
+	strbuf_release(&name);
+	return 0;
+}
+
 /**
  * Determine whether 'ce' needs to be cloned. If so, prepare the 'child' to
  * run the clone. Returns 1 if 'ce' needs to be cloned, 0 otherwise.
@@ -650,6 +689,7 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce,
 	const char *displaypath = NULL;
 	char *url = NULL;
 	int needs_cloning = 0;
+	struct submodule_alternate_setup sas;
 
 	if (ce_stage(ce)) {
 		if (suc->recursive_prefix)
@@ -728,6 +768,13 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce,
 		for_each_string_list_item(item, &suc->references)
 			argv_array_pushl(&child->args, "--reference", item->string, NULL);
 	}
+
+	sas.submodule_name = sub->name;
+	sas.suc = suc;
+	sas.child = child;
+	sas.out = out;
+	foreach_alt_odb(add_possible_reference, &sas);
+
 	if (suc->depth)
 		argv_array_push(&child->args, suc->depth);
 
diff --git a/t/t7408-submodule-reference.sh b/t/t7408-submodule-reference.sh
index 4a1b8f0..a9b89a3 100755
--- a/t/t7408-submodule-reference.sh
+++ b/t/t7408-submodule-reference.sh
@@ -7,7 +7,6 @@ test_description='test clone --reference'
 . ./test-lib.sh
 
 base_dir=$(pwd)
-
 test_alternate_is_used () {
 	alternates_file="$1" &&
 	working_dir="$2" &&
@@ -73,4 +72,32 @@ 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 'cloning superproject, missing submodule alternates' '
+	test_when_finished "rm -rf super-clone" &&
+	git clone super super2 &&
+	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
+		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.583.gd6329be.dirty

[PATCHv3 9/9] submodule--helper: use parallel processor correctly.

[Stefan Beller]

When developing the prior patches I had a temporary state in which
git-clone would segfault, when prepared the call in
prepare_to_clone_next_submodule. This lead to the call failing, i.e. in
`update_clone_task_finished` the task was scheduled to be tried again.
The second call to prepare_to_clone_next_submodule would return 0, as the
segfaulted clone did create the .git file already, such that was not
considered to need to be cloned again. I was seeing the "BUG: ce was
a submodule before?\n" message, which was the correct behavior at the
time as my local code was buggy. When trying to debug this failure, I
tried to use printing messages into the strbuf that is passed around,
but these messages were never printed as the die(..) doesn't
flush the `err` strbuf.

When implementing the die() in 665b35ecc (2016-06-09, "submodule--helper:
initial clone learns retry logic"), I considered this condition to be
a severe condition, which should lead to an immediate abort as we do not
trust ourselves any more. However the queued messages in `err` are valuable
so let's not toss them out by immediately dieing, but a graceful return.

Another thing to note: The error message itself was missleading. A return
value of 0 doesn't indicate the passed in `ce` is not a submodule any more,
but just that we do not consider cloning it any more.

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

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index fc14843..3e40f99 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -815,8 +815,12 @@ static int update_clone_get_next_task(struct child_process *child,
 	if (index < suc->failed_clones_nr) {
 		int *p;
 		ce = suc->failed_clones[index];
-		if (!prepare_to_clone_next_submodule(ce, child, suc, err))
-			die("BUG: ce was a submodule before?");
+		if (!prepare_to_clone_next_submodule(ce, child, suc, err)) {
+			suc->current ++;
+			strbuf_addf(err, "BUG: submodule considered for cloning,"
+				    "doesn't need cloning any more?\n");
+			return 0;
+		}
 		p = xmalloc(sizeof(*p));
 		*p = suc->current;
 		*idx_task_cb = p;
-- 
2.9.2.583.gd6329be.dirty

Re: [PATCHv3 9/9] submodule--helper: use parallel processor correctly.

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> When implementing the die() in 665b35ecc (2016-06-09, "submodule--helper:
> initial clone learns retry logic"), I considered this condition to be
> a severe condition, which should lead to an immediate abort as we do not
> trust ourselves any more. However the queued messages in `err` are valuable
> so let's not toss them out by immediately dieing, but a graceful return.

I think you'll be rerolling this series at least once (if only to
correct for 6/9), so perhaps split this fix into a preparatory fix
that can go earlier to 'next' and further that the remainder of the
series depend on?

>
> Another thing to note: The error message itself was missleading. A return
> value of 0 doesn't indicate the passed in `ce` is not a submodule any more,
> but just that we do not consider cloning it any more.
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>  builtin/submodule--helper.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index fc14843..3e40f99 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -815,8 +815,12 @@ static int update_clone_get_next_task(struct child_process *child,
>  	if (index < suc->failed_clones_nr) {
>  		int *p;
>  		ce = suc->failed_clones[index];
> -		if (!prepare_to_clone_next_submodule(ce, child, suc, err))
> -			die("BUG: ce was a submodule before?");
> +		if (!prepare_to_clone_next_submodule(ce, child, suc, err)) {
> +			suc->current ++;

s/current /current/;

> +			strbuf_addf(err, "BUG: submodule considered for cloning,"
> +				    "doesn't need cloning any more?\n");
> +			return 0;
> +		}
>  		p = xmalloc(sizeof(*p));
>  		*p = suc->current;
>  		*idx_task_cb = p;

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

[Jacob Keller]

On Mon, Aug 8, 2016 at 9:08 PM, Stefan Beller <sbeller@google.com> wrote:
>
> 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.
>

This makes a lot more sense to me, and being able to use
"reference-if-able" for regular clones may be a useful addition as
well.

I looked over the other patches and didn't see anything obviously
wrong, but take that with a grain of salt as I didn't spend a lot of
time at it.

Thanks,
Jake

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

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

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

I like the general direction, but I hope that the warning comes only
when the user said "--reference" on the command line (i.e. "you
asked me to use reference, but for this submodule I couldn't find a
usable one").  If the implementation allows the same mechanism to
help later "submodule init && submodule update" borrow from the
submodule repositories of the superproject the current superproject
borrows from (i.e. no explicit "--reference" on the command line
when doing init/update), I would think the case that needs warning
is "you didn't explicitly ask me to borrow objects, but I found one
we could, so I did it anyway without being asked", and it is not a
warning-worthy condition if we didn't find a cloned submodule in the
repository our superproject borrows from.

Thanks.

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

[Stefan Beller]

On Tue, Aug 9, 2016 at 8:49 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> 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.
>
> I like the general direction, but I hope that the warning comes only
> when the user said "--reference" on the command line (i.e. "you
> asked me to use reference, but for this submodule I couldn't find a
> usable one").

No it comes all the time now, as there is no difference between
"clone --recursive" and later running "submodule update" manually.

Assume a new submodule is added and you forget to update your
main alternate all your other clones are borrowing from. If there is
no warning you create a real clone for the new submodule all the time,
which is what you wanted to avoid in the first place?

>  If the implementation allows the same mechanism to
> help later "submodule init && submodule update" borrow from the
> submodule repositories of the superproject the current superproject
> borrows from (i.e. no explicit "--reference" on the command line
> when doing init/update), I would think the case that needs warning
> is "you didn't explicitly ask me to borrow objects, but I found one
> we could, so I did it anyway without being asked", and it is not a
> warning-worthy condition if we didn't find a cloned submodule in the
> repository our superproject borrows from.

"you did ask me to use alternates once and for all when setting up the
superproject: now for this added submodule I don't find the alternate;
That is strange?"

Thanks,
Stefan

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

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> "you did ask me to use alternates once and for all when setting up the
> superproject: now for this added submodule I don't find the alternate;
> That is strange?"

Absolutely.  I do not think you should expect a user to remember if
s/he used alternates when getting a copy of the superproject long
time ago.  Giving "info: using from ..." is good; giving "warning:
not using from ..." is probably irritating, I would think.

Stepping back a bit, when the user says --reference-if-able, does it
warrant any warning either way?  I.e. "we made it borrow from there,
so be careful not to trash that one" may be just as warning-worthy
(if not even more) as "you said we can borrow from there if there is
anything to borrow, but it turns out there isn't any, so the result
is complete stand-alone."  It feels as if we can go without any
warning at least from "git clone --reference-if-able", unless "-v"
option is given.

I have a very strong opinion what should happen when the end-user
facing command is "git submodule", but if I have to choose, I would
prefer to see "git submodule update" tell what it did with "info:"
either case, i.e. "info: borrowing from ... because the superproject
borrows from there", and "info: not borrowing from ... even though
the superproject borrows from there".

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

[Junio C Hamano]

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

> Stefan Beller <sbeller@google.com> writes:
>
>> "you did ask me to use alternates once and for all when setting up the
>> superproject: now for this added submodule I don't find the alternate;
>> That is strange?"
>
> Absolutely.  I do not think you should expect a user to remember if
> s/he used alternates when getting a copy of the superproject long
> time ago.  Giving "info: using from ..." is good; giving "warning:
> not using from ..." is probably irritating, I would think.
>
> Stepping back a bit, when the user says --reference-if-able, does it
> warrant any warning either way?  I.e. "we made it borrow from there,
> so be careful not to trash that one" may be just as warning-worthy
> (if not even more) as "you said we can borrow from there if there is
> anything to borrow, but it turns out there isn't any, so the result
> is complete stand-alone."  It feels as if we can go without any
> warning at least from "git clone --reference-if-able", unless "-v"
> option is given.
>
> I have a very strong opinion what should happen when the end-user

Too many proof-reading and rephrasing.  "I DON'T have a very strong
opinion" is what I wanted to say.  Sorry for the lack of last-time
proof-reading.

> facing command is "git submodule", but if I have to choose, I would
> prefer to see "git submodule update" tell what it did with "info:"
> either case, i.e. "info: borrowing from ... because the superproject
> borrows from there", and "info: not borrowing from ... even though
> the superproject borrows from there".

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

[Stefan Beller]

On Tue, Aug 9, 2016 at 10:47 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> "you did ask me to use alternates once and for all when setting up the
>> superproject: now for this added submodule I don't find the alternate;
>> That is strange?"
>
> Absolutely.  I do not think you should expect a user to remember if
> s/he used alternates when getting a copy of the superproject long
> time ago.  Giving "info: using from ..." is good; giving "warning:
> not using from ..." is probably irritating, I would think.
>
> Stepping back a bit, when the user says --reference-if-able, does it
> warrant any warning either way?

Stepping another step back, let's discuss the expectations of the
new option "--reference-if-able".

The way I understood and implemented it is

    here is a path, try to use it as an alternate; if that is not
    an alternate, it's fine too; maybe warn about it, but carry
    on with the operation.

With such a semantics you can just add the --reference-if-able
to the submodules that try to borrow from the other superprojects
submodule.

    I expected the "--reference-if-able" not necessarily be
    used by the end user. It is a convenient way for our scripted
    use, as the -if-able is just a check if the path exists and nothing
    else.

    We could check if the git dir exists inthe helper for the
    submodule command, such that we only pass --reference
    as we are certain the alternate exists; we could have a switch
    in the helper --on-missing-submodule-alternate=[die,info,silent]

The way you write this response I think you have a slightly
different understanding of what the -if-able ought to do?

    When --reference is given, only the superproject should
    borrow and the -if-able, may allow submodules to also borrow?

> I.e. "we made it borrow from there,
> so be careful not to trash that one" may be just as warning-worthy
> (if not even more) as "you said we can borrow from there if there is
> anything to borrow, but it turns out there isn't any, so the result
> is complete stand-alone."  It feels as if we can go without any
> warning at least from "git clone --reference-if-able", unless "-v"
> option is given.

But when git clone is not issueing a warning/info, who is responsible for
that? As you noted the superproject may be setup some time ago and
the user forgot they used references and want to use references for this
new submodule. So the helper would need to do that?

>
> I have a very strong opinion what should happen when the end-user
> facing command is "git submodule", but if I have to choose, I would
> prefer to see "git submodule update" tell what it did with "info:"
> either case, i.e. "info: borrowing from ... because the superproject
> borrows from there", and "info: not borrowing from ... even though
> the superproject borrows from there".

I see. This way the user is most informed. The current implementation
of "submodule update --init" for start from scratch yields for example:

Submodule 'foo' (<url>) registered for path 'foo'
Submodule 'hooks' (<url>) registered for path 'hooks'
Cloning into '/home/sbeller/super/foo'...
Cloning into '/home/sbeller/super/hooks'...
warning: Not using proposed alternate
/home/sbeller/super-reference/.git/modules/hooks/
Submodule path 'foo': checked out '7b41f3a413b46140b050ae5324cbbcdd467d2b3a'
Submodule path 'hooks': checked out '3acc14d10d26678eae6489038fe0d4dad644a9b4'

so before this series we had 3 lines per submodule, and with this series
we get a 4th line for the unusual case.

You would prefer to see always 4 lines per submodule?
Is one extra line (25% more output) a reasonable tradeoff for the
reference feature?

I dunno; I guess we could argue either way.

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

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> The way I understood and implemented it is
>
>     here is a path, try to use it as an alternate; if that is not
>     an alternate, it's fine too; maybe warn about it, but carry
>     on with the operation.

My expectation is without "maybe warn about it".  And not adding it
as an alternate (because it is not usable) is just "doing as I was
told to do", nothing noteworthy.  Because "do it only if you can" is
an explicit instruction to the doer that the caller does not care
about the outcome, I'd think there shouldn't be any warning, whether
it is used or not.  At the same time, if the caller wants to know
the outcome, and using if-able as a way to say "I prefer to see it
happen, but you do not have to panic if you can't", I'd think it is
OK to give "info:" to report which of the two possible paths was
taken in either case.  Throwing a "warning:" only when it didn't do
so does not make much sense to me.

> The way you write this response I think you have a slightly
> different understanding of what the -if-able ought to do?
>
>     When --reference is given, only the superproject should
>     borrow and the -if-able, may allow submodules to also borrow?

I have no idea what this sentence means.

>> I have a very strong opinion what should happen when the end-user
>> facing command is "git submodule", but if I have to choose, I would
>> prefer to see "git submodule update" tell what it did with "info:"
>> either case, i.e. "info: borrowing from ... because the superproject
>> borrows from there", and "info: not borrowing from ... even though
>> the superproject borrows from there".
>
> I see. This way the user is most informed.

Yes, and if we were to go that route, "clone" without "-v" should
not report which of the two it took.  It is OK for "submodule" to
look at what "clone" left as the result and do more intelligent
reporting that is better suited for the context of the command.

> The current implementation
> of "submodule update --init" for start from scratch yields for example:
>
> Submodule 'foo' (<url>) registered for path 'foo'
> Submodule 'hooks' (<url>) registered for path 'hooks'
> Cloning into '/home/sbeller/super/foo'...
> Cloning into '/home/sbeller/super/hooks'...
> warning: Not using proposed alternate
> /home/sbeller/super-reference/.git/modules/hooks/
> Submodule path 'foo': checked out '7b41f3a413b46140b050ae5324cbbcdd467d2b3a'
> Submodule path 'hooks': checked out '3acc14d10d26678eae6489038fe0d4dad644a9b4'
>
> so before this series we had 3 lines per submodule, and with this series
> we get a 4th line for the unusual case.
>
> You would prefer to see always 4 lines per submodule?

If the user says "--recursive --reference", then the user probably
deserves to be notified.  As I said (eh, rather, as I wanted to say
but failed to say so), I do not have a strong preference either way.

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

[Stefan Beller]

On Tue, Aug 9, 2016 at 11:44 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> The way I understood and implemented it is
>>
>>     here is a path, try to use it as an alternate; if that is not
>>     an alternate, it's fine too; maybe warn about it, but carry
>>     on with the operation.
>
> My expectation is without "maybe warn about it".  And not adding it
> as an alternate (because it is not usable) is just "doing as I was
> told to do", nothing noteworthy.  Because "do it only if you can" is
> an explicit instruction to the doer that the caller does not care
> about the outcome, I'd think there shouldn't be any warning, whether
> it is used or not.  At the same time, if the caller wants to know
> the outcome, and using if-able as a way to say "I prefer to see it
> happen, but you do not have to panic if you can't", I'd think it is
> OK to give "info:" to report which of the two possible paths was
> taken in either case.  Throwing a "warning:" only when it didn't do
> so does not make much sense to me.

I think this whole thing needs a new config variable to be set.

At the time of cloning you may run

  git clone --recursive --reference <other-super-project-location>
or
  git clone --recursive --reference-if-able <other-super-project-location>
or
  git clone --recursive

The first two should trickle down to the submodules, i.e.
in the first case we maybe want to run

    git config submodule.alternate=required-superproject &&
    git submodule update --init --recurse

In the second case
    git config submodule.alternate=optional-superproject &&
    git submodule update --init --recurse

And in the third case we maybe only want to record
    git config submodule.alternate=none # implicit by not setting it as well
    git submodule update --init --recurse


Then later when we run
    git submodule update
we have this option to know if a submodule alternate should be treated
as optional or required referenced as the existence
of the superprojects alternate (as a boolean indicator) is not enough of
an indicator what the user later wants.


>>
>> I see. This way the user is most informed.
>
> Yes, and if we were to go that route, "clone" without "-v" should
> not report which of the two it took.  It is OK for "submodule" to
> look at what "clone" left as the result and do more intelligent
> reporting that is better suited for the context of the command.
>
>> The current implementation
>> of "submodule update --init" for start from scratch yields for example:
>>
>> Submodule 'foo' (<url>) registered for path 'foo'
>> Submodule 'hooks' (<url>) registered for path 'hooks'
>> Cloning into '/home/sbeller/super/foo'...
>> Cloning into '/home/sbeller/super/hooks'...
>> warning: Not using proposed alternate
>> /home/sbeller/super-reference/.git/modules/hooks/
>> Submodule path 'foo': checked out '7b41f3a413b46140b050ae5324cbbcdd467d2b3a'
>> Submodule path 'hooks': checked out '3acc14d10d26678eae6489038fe0d4dad644a9b4'
>>
>> so before this series we had 3 lines per submodule, and with this series
>> we get a 4th line for the unusual case.
>>
>> You would prefer to see always 4 lines per submodule?
>
> If the user says "--recursive --reference", then the user probably
> deserves to be notified.  As I said (eh, rather, as I wanted to say
> but failed to say so), I do not have a strong preference either way.

With a new config option we can trigger the notifications based on that as well.

"required superproject" would die when the submodule alternate is not there,
and the "optional superproject" would always state if it found an alternate.

Thanks,
Stefan

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

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> At the time of cloning you may run
>
>   git clone --recursive --reference <other-super-project-location>
> or
>   git clone --recursive --reference-if-able <other-super-project-location>
> or
>   git clone --recursive

That's an interesting tangent.  I never meant "if-able" to be an
end-user visible option [*1*], but now you mention it, I do not see
a reason why "clone --reference-if-able" of the top-level project
cannot be used together with "--recursive".

> Then later when we run
>     git submodule update
> we have this option to know if a submodule alternate should be treated
> as optional or required referenced as the existence
> of the superprojects alternate (as a boolean indicator) is not enough of
> an indicator what the user later wants.

A tangent that comes to my mind after reading this is if letting
"if-able" just skip (with or without warning) and forget about it
once a clone is made is what we really want.

Suppose the "other-super-project-location" repository did not have a
clone of a submodule when you create a new clone of the superproject
using it as a reference.  The submodule will be made a full clone,
but after that happens, other-super-project-location can get
interested in the submodule and can acquire its own clone.

At that point, our clone of the submodule _could_ add the submodule
in the other-super-project-location as its alternate and lose the
duplicate objects that it could borrow from there by repacking, but
the suggested "clone with if-able, and forget about it after a clone
is made" would not allow us to do this.  I do not know if a
real-world use of submodules want the ability to do so, or it is
unnecessary.  I suspect with the recording of "you were told that
borrowing from the same location as the superproject is OK", this
becomes easily doable (i.e. subsequent "submodule update" can realize
that the submodule does not have alternates but it could borrow from
the submodule in the other-super-project-location).


[Footnote]

*1* Rather, I meant: clone has a very intimate knowledge on what and
    what cannot be borrowed from and it is not just "is there a
    directory?", so "git submodule update --init" is not in a good
    position to decide if it wants to add --reference to the
    invocation of "git clone" it makes internally, and introducing
    an "if-able" variant to "clone" is one way to relieve it from
    having to make that decision.

    I could have suggested an alternative: because the submodule
    machinery is gettting moved to C the "update --init" code that
    drives the internal invocation of "git clone" could share the
    the logic in "git clone --reference" that determines if a local
    repository can be used as an alternate by small refactoring of
    builtin/clone.c::add_one_reference().

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

[Stefan Beller]

On Tue, Aug 9, 2016 at 2:45 PM, Junio C Hamano <gitster@pobox.com> wrote:
> becomes easily doable (i.e. subsequent "submodule update" can realize
> that the submodule does not have alternates but it could borrow from
> the submodule in the other-super-project-location).

I would suggest to postpone this to a later time once the need arises.

I rather imagine that once you clone from "other-super-project-location"
and get the warning about no alternates borrowing, you can decide if you want
to set it up and link the submodule manually to that alternate, or if you just
don't care about this one repository being duplicated.

For now I plan to introduce 2 options, as these are 2 different things, that
can be extended independently of each other:

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

> *1* Rather, I meant: clone has a very intimate knowledge on what and
>     what cannot be borrowed from and it is not just "is there a
>     directory?", so "git submodule update --init" is not in a good
>     position to decide if it wants to add --reference to the
>     invocation of "git clone" it makes internally, and introducing
>     an "if-able" variant to "clone" is one way to relieve it from
>     having to make that decision.

That is how I first understood the design as well.

>
>     I could have suggested an alternative: because the submodule
>     machinery is gettting moved to C the "update --init" code that
>     drives the internal invocation of "git clone" could share the
>     the logic in "git clone --reference" that determines if a local
>     repository can be used as an alternate by small refactoring of
>     builtin/clone.c::add_one_reference().

I see. I might actually need to do this anyway as the helper is a place
to act on the submodule.alternateErrorStrategy.

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

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> On Tue, Aug 9, 2016 at 2:45 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> becomes easily doable (i.e. subsequent "submodule update" can realize
>> that the submodule does not have alternates but it could borrow from
>> the submodule in the other-super-project-location).
>
> I would suggest to postpone this to a later time once the need arises.

Oh, no question about that (did I even sound to be suggesting to do
it now?).  It is just one of the yardsticks I use when gauging if a
proposed solution is sound to see how it will naturally scale and/or
extend to other possible scenarios, and I was pointing out that this
seems to pass the test.