From: Stefan Beller <hidden> Date: 2016-08-09 04:15:44
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
From: Stefan Beller <hidden> Date: 2016-08-09 04:08:30
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 <redacted>
---
t/t7408-submodule-reference.sh | 140 +++++++++++++++++++++--------------------
1 file changed, 71 insertions(+), 69 deletions(-)
@@ -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_repoA&&cdA&&-echofirst>file1&&-gitaddfile1&&-gitcommit-mA-initial'--cd"$base_dir"--test_expect_success'preparing second repository'\-'gitcloneAB&&cdB&&-echosecond>file2&&-gitaddfile2&&-gitcommit-mB-addition&&-gitrepack-a-d&&-gitprune'--cd"$base_dir"--test_expect_success'preparing superproject'\-'test_create_reposuper&&cdsuper&&-echofile>file&&-gitaddfile&&-gitcommit-mB-super-initial'--cd"$base_dir"--test_expect_success'submodule add --reference'\-'cdsuper&&gitsubmoduleadd--reference../B"file://$base_dir/A"sub&&-gitcommit-mB-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'\-'cdsuper/sub&&-echo"0 objects, 0 kilobytes">expected&&-gitcount-objects>current&&-diffexpectedcurrent'--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'\-'cdsuper-clone/sub&&-echo"0 objects, 0 kilobytes">expected&&-gitcount-objects>current&&-diffexpectedcurrent'--cd"$base_dir"+test_expect_success'preparing first repository''+test_create_repoA&&+(+cdA&&+echofirst>file1&&+gitaddfile1&&+gitcommit-mA-initial+)+'++test_expect_success'preparing second repository''+gitcloneAB&&+(+cdB&&+echosecond>file2&&+gitaddfile2&&+gitcommit-mB-addition&&+gitrepack-a-d&&+gitprune+)+'++test_expect_success'preparing superproject''+test_create_reposuper&&+(+cdsuper&&+echofile>file&&+gitaddfile&&+gitcommit-mB-super-initial+)+'++test_expect_success'submodule add --reference''+(+cdsuper&&+gitsubmoduleadd--reference../B"file://$base_dir/A"sub&&+gitcommit-mB-super-added+)+'++test_expect_success'after add: existence of info/alternates''+test_line_count=1super/.git/modules/sub/objects/info/alternates+'++test_expect_success'that reference gets used with add''+(+cdsuper/sub&&+echo"0 objects, 0 kilobytes">expected&&+gitcount-objects>current&&+diffexpectedcurrent+)+'++test_expect_success'cloning superproject''+gitclonesupersuper-clone+'++test_expect_success'update with reference''+cdsuper-clone&&gitsubmoduleupdate--init--reference../B+'++test_expect_success'after update: existence of info/alternates''+test_line_count=1super-clone/.git/modules/sub/objects/info/alternates+'++test_expect_success'that reference gets used with update''+cdsuper-clone/sub&&+echo"0 objects, 0 kilobytes">expected&&+gitcount-objects>current&&+diffexpectedcurrent+' test_done
From: Stefan Beller <hidden> Date: 2016-08-09 04:08:36
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 <redacted>
---
t/t7408-submodule-reference.sh | 57 ++++++++++++++++++------------------------
1 file changed, 25 insertions(+), 32 deletions(-)
@@ -40,44 +49,28 @@ test_expect_success 'preparing superproject' ')'-test_expect_success'submodule add --reference''+test_expect_success'submodule add --reference uses alternates''(cdsuper&&gitsubmoduleadd--reference../B"file://$base_dir/A"sub&&-gitcommit-mB-super-added-)-'--test_expect_success'after add: existence of info/alternates''-test_line_count=1super/.git/modules/sub/objects/info/alternates+gitcommit-mB-super-added&&+gitrepack-ad+)&&+test_alternate_is_usedsuper/.git/modules/sub/objects/info/alternatessuper/sub'-test_expect_success'that reference gets used with add''-(-cdsuper/sub&&-echo"0 objects, 0 kilobytes">expected&&-gitcount-objects>current&&-diffexpectedcurrent-)-'--test_expect_success'cloning superproject''-gitclonesupersuper-clone-'--test_expect_success'update with reference''-cdsuper-clone&&gitsubmoduleupdate--init--reference../B-'--test_expect_success'after update: existence of info/alternates''-test_line_count=1super-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''-cdsuper-clone/sub&&-echo"0 objects, 0 kilobytes">expected&&-gitcount-objects>current&&-diffexpectedcurrent+test_expect_success'updating superproject keeps alternates''+test_when_finished"rm -rf super-clone"&&+gitclonesupersuper-clone&&+git-Csuper-clonesubmoduleupdate--init--reference../B&&+test_alternate_is_usedsuper-clone/.git/modules/sub/objects/info/alternatessuper-clone/sub' test_done
From: Stefan Beller <hidden> Date: 2016-08-09 04:08:38
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 <redacted>
---
builtin/submodule--helper.c | 14 +++++++++-----
git-submodule.sh | 2 +-
2 files changed, 10 insertions(+), 6 deletions(-)
@@ -584,7 +584,7 @@ struct submodule_update_clone {/* configuration parameters which are passed on to the children */intquiet;intrecommend_shallow;-constchar*reference;+structstring_listreferences;constchar*depth;constchar*recursive_prefix;constchar*prefix;
From: Stefan Beller <hidden> Date: 2016-08-09 04:08:40
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 <redacted>
---
builtin/clone.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
@@ -50,7 +50,7 @@ static int option_verbosity;staticintoption_progress=-1;staticenumtransport_familyfamily;staticstructstring_listoption_config=STRING_LIST_INIT_NODUP;-staticstructstring_listoption_reference=STRING_LIST_INIT_NODUP;+staticstructstring_listoption_required_reference=STRING_LIST_INIT_NODUP;staticintoption_dissociate;staticintmax_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")),
From: Stefan Beller <hidden> Date: 2016-08-09 04:08:45
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 <redacted>
---
Documentation/git-submodule.txt | 8 +++++++-
builtin/submodule--helper.c | 14 +++++++++++++-
git-submodule.sh | 10 ++++++++++
3 files changed, 30 insertions(+), 2 deletions(-)
@@ -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.
@@ -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){+structstring_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")),
From: Stefan Beller <hidden> Date: 2016-08-09 04:08:48
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 <redacted>
---
Documentation/git-clone.txt | 5 +++++
builtin/clone.c | 22 ++++++++++++++++++----
t/t7408-submodule-reference.sh | 31 ++++++++++++++++++++++++++++++-
3 files changed, 53 insertions(+), 5 deletions(-)
@@ -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
@@ -65,4 +66,32 @@ test_expect_success 'updating superproject keeps alternates' 'test_alternate_usagesuper-clone/.git/modules/sub/objects/info/alternatessuper-clone/sub'+test_expect_success'submodules use alternates when cloning a superproject''+test_when_finished"rm -rf super-clone"&&+gitclone--referencesuper--recursivesupersuper-clone&&+(+cdsuper-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/alternatessub+)+'++test_expect_success'cloning superproject, missing submodule alternates''+test_when_finished"rm -rf super-clone"&&+gitclonesupersuper2&&+test_must_failgitclone--recursive--referencesuper2super2super-clone&&+(+cdsuper-clone&&+# test superproject has alternates setup correctly+test_alternate_usage.git/objects/info/alternates.&&+# update of the submodule succeeds+gitsubmoduleupdate--init&&+# and we have no alternates:+test_must_failtest_alternate_usage.git/modules/sub/objects/info/alternatessub&&+test_path_is_filesub/file1+)+'+ test_done
From: Stefan Beller <hidden> Date: 2016-08-09 04:08:49
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 <redacted>
---
builtin/submodule--helper.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
@@ -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");+return0;+}p=xmalloc(sizeof(*p));*p=suc->current;*idx_task_cb=p;
@@ -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
@@ -73,4 +72,32 @@ test_expect_success 'updating superproject keeps alternates' 'test_alternate_is_usedsuper-clone/.git/modules/sub/objects/info/alternatessuper-clone/sub'+test_expect_success'submodules use alternates when cloning a superproject''+test_when_finished"rm -rf super-clone"&&+gitclone--referencesuper--recursivesupersuper-clone&&+(+cdsuper-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/alternatessub+)+'++test_expect_success'cloning superproject, missing submodule alternates''+test_when_finished"rm -rf super-clone"&&+gitclonesupersuper2&&+gitclone--recursive--referencesuper2super2super-clone&&+(+cdsuper-clone&&+# test superproject has alternates setup correctly+test_alternate_is_used.git/objects/info/alternates.&&+# update of the submodule succeeds+gitsubmoduleupdate--init&&+# and we have no alternates:+test_must_failtest_alternate_is_used.git/modules/sub/objects/info/alternatessub&&+test_path_is_filesub/file1+)+'+ test_done
From: Stefan Beller <hidden> Date: 2016-08-09 04:13:40
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 <redacted>
---
builtin/submodule--helper.c | 20 ++++++++++++++++----
1 file changed, 16 insertions(+), 4 deletions(-)
From: Stefan Beller <hidden> Date: 2016-08-09 04:14:09
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 <redacted>
---
Documentation/git-clone.txt | 5 ++++-
builtin/clone.c | 32 ++++++++++++++++++++++++++------
2 files changed, 30 insertions(+), 7 deletions(-)
@@ -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.
@@ -51,6 +51,7 @@ static int option_progress = -1;staticenumtransport_familyfamily;staticstructstring_listoption_config=STRING_LIST_INIT_NODUP;staticstructstring_listoption_required_reference=STRING_LIST_INIT_NODUP;+staticstructstring_listoption_optional_reference=STRING_LIST_INIT_NODUP;staticintoption_dissociate;staticintmax_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"),
@@ -304,7 +318,8 @@ static int add_one_reference(struct string_list_item *item, void *cb_data)}elseif(!is_directory(mkpath("%s/objects",ref_git))){structstrbufsb=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);
From: Jacob Keller <hidden> Date: 2016-08-09 05:24:15
On Mon, Aug 8, 2016 at 9:08 PM, Stefan Beller [off-list ref] 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