submodule--helper: set alternateLocation for cloned submodules
PATCH 1/1 submodule--helper: set alternateLocation for cloned submodules
From: Vitaly _Vi Shukela <[email protected]>
Git v2.11 introduced "git clone --recursive --referece ...", but it didn't put the alternates for _nested_ submodules.
This patch makes all not just the root repository, but also all submodules (recursively) have submodule.alternateLocation and submodule.alternateErrorStrategy configured, making Git search for possible alternates for nested submodules as well.
As submodule's alternate target does not end in .git/objects (rather .git/modules/qqqqqq/objects), this alternate target path restriction for in add_possible_reference_from_superproject relates from "*.git/objects" to just */objects".
New tests have been added to t7408-submodule-reference.
--- builtin/submodule--helper.c | 24 ++++++++++++-- t/t7408-submodule-reference.sh | 73 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 95 insertions(+), 2 deletions(-)
builtin/submodule--helper.c +22 -2
@@ -498,9 +498,9 @@ static int add_possible_reference_from_superproject(/* * If the alternate object store is another repository, try the -* standard layout with .git/modules/<name>/objects +* standard layout with .git/(modules/<name>)+/objects */ -if (ends_with (alt->path,".git/objects" )) { +if (ends_with (alt->path,"/objects" )) {char *sm_alternate;struct strbuf sb =STRBUF_INIT ;struct strbuf err =STRBUF_INIT ;
@@ -672,6 +672,26 @@ static int module_clone(int argc, const char **argv, const char *prefix)die (_ ("could not get submodule directory for '%s'" ), path);git_config_set_in_file (p,"core.worktree" ,relative_path (path, sm_gitdir, &rel_path)); + +/* setup alternateLocation and alternateErrorStrategy in the cloned submodule if needed */ + { +char *sm_alternate =NULL , *error_strategy =NULL ; + +git_config_get_string ("submodule.alternateLocation" , &sm_alternate); +if (sm_alternate) { +git_config_set_in_file (p,"submodule.alternateLocation" , + sm_alternate); + } +git_config_get_string ("submodule.alternateErrorStrategy" , &error_strategy); +if (error_strategy) { +git_config_set_in_file (p,"submodule.alternateErrorStrategy" , + error_strategy); + } + +free (sm_alternate); +free (error_strategy); + } +strbuf_release (&sb);strbuf_release (&rel_path);free (sm_gitdir);
t/t7408-submodule-reference.sh +73 -0
77 lines not on this page · raw message
Signature · 1 line
2.10.2
Quoted 1 line · On Wed, Dec 7, 2016 at 10:42 AM, <[email protected]> wrote:
From: Vitaly
_ViShukela <[email protected]>
Thanks for contributing to Git!
(/me looks up if you have sent patches already as you
seem to know how to do that. :) unrelated side note: Maybe you want to send a patch for the .mailmap file mapping your two email addresses together, c.f. "git log -- .mailmap")
Quoted 3 lines
Git v2.11 introduced "git clone --recursive --referece ...", but it didn't put the alternates for _nested_ submodules.
This message is targeted at people familiar with gits code base, so we can be more specific. e.g.
In 31224cbdc7 (clone: recursive and reference option triggers submodule alternates, 2016-08-17) a mechanism was added to have submodules referenced. It did not address _nested_ submodules, however.
Quoted 5 lines
This patch makes all not just the root repository, but also all submodules (recursively) have submodule.alternateLocation and submodule.alternateErrorStrategy configured, making Git search for possible alternates for nested submodules as well.
Sounds great!
Quoted 5 lines
As submodule's alternate target does not end in .git/objects (rather .git/modules/qqqqqq/objects), this alternate target path restriction for in
add_possible_reference_from_superprojectrelates from "*.git/objects" to just */objects".
I wonder if this check is too weak and we actually have to check for either .git/objects or modules/<name/possibly/having/slashes>/objects. When writing the referenced commit I assumed we'd need a stronger check to be safer and not add some random location as a possible alternate.
Quoted 2 lines
New tests have been added to t7408-submodule-reference.
Thanks!
Quoted 24 lines
Signed-off-by Vitaly _Vi Shukela <[email protected]>--- builtin/submodule--helper.c | 24 ++++++++++++-- t/t7408-submodule-reference.sh | 73 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 95 insertions(+), 2 deletions(-)builtin/submodule--helper.c +2
-2@@ -498,9 +498,9 @@ static int add_possible_reference_from_superproject(/* * If the alternate object store is another repository, try the -* standard layout with .git/modules/<name>/objects +* standard layout with .git/(modules/<name>)+/objects */ -if (ends_with (alt->path,".git/objects" )) { +if (ends_with (alt->path,"/objects" )) {char *sm_alternate;struct strbuf sb =STRBUF_INIT ;struct strbuf err =STRBUF_INIT ;
builtin/submodule--helper.c static int module_clone(int argc, const char **argv, const char *prefix) · line 677
Quoted 6 lines
builtin/submodule--helper.c +3
-0@@ -672,6 +672,26 @@ static int module_clone(int argc, const char **argv, const char *prefix)die (_ ("could not get submodule directory for '%s'" ), path);git_config_set_in_file (p,"core.worktree" ,relative_path (path, sm_gitdir, &rel_path)); + +/* setup alternateLocation and alternateErrorStrategy in the cloned submodule if needed */ + {
Usually we do not use braces to further nest code, please remove this nesting.
Quoted 25 lines
builtin/submodule--helper.c +17
-0+char *sm_alternate =NULL , *error_strategy =NULL ; + +git_config_get_string ("submodule.alternateLocation" , &sm_alternate); +if (sm_alternate) { +git_config_set_in_file (p,"submodule.alternateLocation" , + sm_alternate); + } +git_config_get_string ("submodule.alternateErrorStrategy" , &error_strategy); +if (error_strategy) { +git_config_set_in_file (p,"submodule.alternateErrorStrategy" , + error_strategy); + } + +free (sm_alternate); +free (error_strategy); + } +strbuf_release (&sb);strbuf_release (&rel_path);free (sm_gitdir);t/t7408-submodule-reference.sh +0
-0
t/t7408-submodule-reference.sh test_expect_success 'ignoring missing submodule alternates passes clone and subm · line 132
Quoted 8 lines
t/t7408-submodule-reference.sh +5
-0@@ -125,4 +125,77 @@ test_expect_success 'ignoring missing submodule alternates passes clone and subm ) ' +test_expect_success 'preparing second superproject with a nested submodule' ' + test_create_repo supersuper && + ( + cd supersuper && + echo I am super super. >file &&
Usually we quote strings containing white space, e.g. echo "I am ..." >actual
t/t7408-submodule-reference.sh test_expect_success 'ignoring missing submodule alternates passes clone and subm · line 140
Quoted 8 lines
t/t7408-submodule-reference.sh +8
-0+ git add file && + git commit -m B-super-super-initial + git submodule add "file://$base_dir/super" subwithsub && + git commit -m B-super-super-added && + git submodule update --init --recursive && + git repack -ad + ) && + echo not cleaning supersuper
This echo is left in for debugging purposes?
Quoted 6 lines · Stefan Beller
Quoted 4 lines
This patch makes all not just the root repository, but also all submodules (recursively) have submodule.alternateLocation and submodule.alternateErrorStrategy configured, making Git search for possible alternates for nested submodules as well.
Sounds great!
Is it safe to assume that all the submodules used recursively by submodules share the same structure upstream? Does the alternate location mechanism degrades sensibly if this assumption turns out to be false (i.e. "possible alternates" above turns out to be mere possibility and not there)?
Quoted 7 lines · Junio C Hamano
Quoted 5 lines · Stefan Beller
Quoted 4 lines
This patch makes all not just the root repository, but also all submodules (recursively) have submodule.alternateLocation and submodule.alternateErrorStrategy configured, making Git search for possible alternates for nested submodules as well.
Quoted 7 lines
Quoted 1 line
Sounds great!
Is it safe to assume that all the submodules used recursively by submodules share the same structure upstream? Does the alternate location mechanism degrades sensibly if this assumption turns out to be false (i.e. "possible alternates" above turns out to be mere possibility and not there)?
According to the last test in the patch, this seems to be doing the sensible thing.
Quoted 10 lines · Stefan Beller
Quoted 9 lines · Junio C Hamano
Quoted 6 lines · Stefan Beller
Quoted 4 lines
This patch makes all not just the root repository, but also all submodules (recursively) have submodule.alternateLocation and submodule.alternateErrorStrategy configured, making Git search for possible alternates for nested submodules as well.
Sounds great!
Quoted 8 lines
Quoted 5 lines
Is it safe to assume that all the submodules used recursively by submodules share the same structure upstream? Does the alternate location mechanism degrades sensibly if this assumption turns out to be false (i.e. "possible alternates" above turns out to be mere possibility and not there)?
According to the last test in the patch, this seems to be doing the sensible thing.
OK, that sounds great. Thanks.
Quoted 9 lines · Stefan Beller
Quoted 4 lines
As submodule's alternate target does not end in .git/objects (rather .git/modules/qqqqqq/objects), this alternate target path restriction for in
add_possible_reference_from_superprojectrelates from "*.git/objects" to just */objects".I wonder if this check is too weak and we actually have to check for either .git/objects or modules/<name/possibly/having/slashes>/objects. When writing the referenced commit I assumed we'd need a stronger check to be safer and not add some random location as a possible alternate.
1. Do we really need to check that it is named ".git"? Although "git clone --mirror --recursive" is not (directly) supported now, user may create one bare repository with [remnants of] submodules by converting reqular repository into bare one. Why not take advantage of additional information available locally in this case?
2. Is the check need to be strict because of we need to traverse one directory level up? Normally this "/objects" part is added by Git, so just one "../" seems to be OK. User can't specify "--reference somerepo/.git/objects", a strange reference can appear only if user manually creates alternates. Maybe better to document this case instead of restricting the feature?
3. If nonetheless check for ".git/*/objects", then
a. What functions should be used in Git codebase for such checks? b. Should we handle tricks like "smth/.git/../../objects" and so on?
4. Should we print (or print in verbose mode) each used alternate, to inform operator what his or her new clone will depend on?
P.S. Actually I discovered the --recursive --reference feature when tried to put reference to a mega-repo with all possible submodules added as remotes. I expected --reference to just get though across all submodules, but it complained to missing "/modules/..." instead (the check went though becase the repository was named like "megarepo.git", so it did ended in ".git/objects").
Quoted 12 lines · On Wed, Dec 7, 2016 at 1:24 PM, vi0oss <[email protected]> wrote:
Quoted 11 lines · Stefan Beller
Quoted 5 lines
As submodule's alternate target does not end in .git/objects (rather .git/modules/qqqqqq/objects), this alternate target path restriction for in
add_possible_reference_from_superprojectrelates from "*.git/objects" to just */objects".I wonder if this check is too weak and we actually have to check for either .git/objects or modules/<name/possibly/having/slashes>/objects. When writing the referenced commit I assumed we'd need a stronger check to be safer and not add some random location as a possible alternate.
Quoted 6 lines
1. Do we really need to check that it is named ".git"? Although "git clone --mirror --recursive" is not (directly) supported now, user may create one bare repository with [remnants of] submodules by converting reqular repository into bare one. Why not take advantage of additional information available locally in this case?
Oh, great point.
Quoted 7 lines
2. Is the check need to be strict because of we need to traverse one directory level up? Normally this "/objects" part is added by Git, so just one "../" seems to be OK. User can't specify "--reference somerepo/.git/objects", a strange reference can appear only if user manually creates alternates. Maybe better to document this case instead of restricting the feature?
Not sure I understand what needs better documentation here?
Quoted 4 lines
3. If nonetheless check for ".git/*/objects", then
a. What functions should be used in Git codebase for such checks? b. Should we handle tricks like "smth/.git/../../objects" and so on?
I see we're getting into problems here.
Quoted 11 lines
4. Should we print (or print in verbose mode) each used alternate, to inform operator what his or her new clone will depend on?
P.S. Actually I discovered the --recursive --reference feature when tried to put reference to a mega-repo with all possible submodules added as remotes. I expected --reference to just get though across all submodules, but it complained
to missing "/modules/..." instead (the check went though becase the repository
was named like "megarepo.git", so it did ended in ".git/objects").
Oh :(
With that said, I think the original patch is a sensible approach.
Thanks,
Stefan