# [PATCH] submodule--helper: set alternateLocation for cloned submodules 7 messages from 2016-12-08 to 2016-12-08. Participants: vi0oss@gmail.com, Stefan Beller, Jeff King, vi0oss. Thread: https://gitlist.dev/t/44648 ## vi0oss@gmail.com, 2016-12-08 00:39 Subject: [PATCH] submodule--helper: set alternateLocation for cloned submodules Message-ID: <20161208003940.28794-1-vi0oss@gmail.com> URL: https://gitlist.dev/e/20161208003940.28794-1-vi0oss%40gmail.com ``` From: "Vitaly \"_Vi\" Shukela" 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. 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. Signed-off-by: Vitaly _Vi Shukela --- Notes: Another round of fixups: * Corrected code style * Refactored and fixed the test Previously test contained errorneous test_must_fail, which was masked by missing &&. Mailmap patch omitted this time. builtin/submodule--helper.c | 19 ++++++++++-- t/t7408-submodule-reference.sh | 66 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 83 insertions(+), 2 deletions(-) diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c index 4beeda5..92fd676 100644 --- a/builtin/submodule--helper.c +++ b/builtin/submodule--helper.c @@ -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//objects + * standard layout with .git/(modules/)+/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; @@ -583,6 +583,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; + char *sm_alternate = NULL, *error_strategy = NULL; struct option module_clone_options[] = { OPT_STRING(0, "prefix", &prefix, @@ -672,6 +673,20 @@ 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 */ + 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); diff --git a/t/t7408-submodule-reference.sh b/t/t7408-submodule-reference.sh index 1c1e289..9325297 100755 --- a/t/t7408-submodule-reference.sh +++ b/t/t7408-submodule-reference.sh @@ -125,4 +125,70 @@ test_expect_success 'ignoring missing submodule alternates passes clone and subm ) ' +test_expect_success 'preparing second superproject with a nested submodule plus partial clone' ' + test_create_repo supersuper && + ( + cd supersuper && + echo "I am super super." >file && + 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 + ) && + git clone supersuper supersuper2 && + ( + cd supersuper2 && + git submodule update --init + ) +' + +# At this point there are three root-level positories: A, B, super and super2 + +test_expect_success 'nested submodule alternate in works and is actually used' ' + test_when_finished "rm -rf supersuper-clone" && + git clone --recursive --reference supersuper supersuper supersuper-clone && + ( + cd supersuper-clone && + # test superproject has alternates setup correctly + test_alternate_is_used .git/objects/info/alternates . && + # immediate submodule has alternate: + test_alternate_is_used .git/modules/subwithsub/objects/info/alternates subwithsub + # nested submodule also has alternate: + test_alternate_is_used .git/modules/subwithsub/modules/sub/objects/info/alternates subwithsub/sub + ) +' + +check_that_two_of_three_alternates_are_used() { + test_alternate_is_used .git/objects/info/alternates . && + # immediate submodule has alternate: + test_alternate_is_used .git/modules/subwithsub/objects/info/alternates subwithsub && + # but nested submodule has no alternate: + test_must_fail test_alternate_is_used .git/modules/subwithsub/modules/sub/objects/info/alternates subwithsub/sub +} + + +test_expect_success 'missing nested submodule alternate fails clone and submodule update' ' + test_when_finished "rm -rf supersuper-clone" && + test_must_fail git clone --recursive --reference supersuper2 supersuper2 supersuper-clone && + ( + cd supersuper-clone && + check_that_two_of_three_alternates_are_used && + # update of the submodule fails + test_must_fail git submodule update --init --recursive + ) +' + +test_expect_success 'missing nested submodule alternate in --reference-if-able mode' ' + test_when_finished "rm -rf supersuper-clone" && + git clone --recursive --reference-if-able supersuper2 supersuper2 supersuper-clone && + ( + cd supersuper-clone && + check_that_two_of_three_alternates_are_used && + # update of the submodule succeeds + git submodule update --init --recursive + ) +' + test_done -- 2.10.2 ``` ## Stefan Beller, 2016-12-08 01:22 Subject: Re: [PATCH] submodule--helper: set alternateLocation for cloned submodules Message-ID: URL: https://gitlist.dev/e/CAGZ79kZU401JRp4EtwTGHzk3Zq%2BsnQhX3GArDfF6SpKxsSwtWg%40mail.gmail.com In-Reply-To: <20161208003940.28794-1-vi0oss@gmail.com> ``` On Wed, Dec 7, 2016 at 4:39 PM, wrote: > > Previously test contained errorneous > test_must_fail, which was masked by > missing &&. I wonder if we could make either the test_must_fail intelligent to detect such a broken && call chain or the test_expect_success macro to see for those broken chains. Patch looks good to me except one very minor nit. :) > +test_expect_success 'nested submodule alternate in works and is actually used' ' > + test_when_finished "rm -rf supersuper-clone" && > + git clone --recursive --reference supersuper supersuper supersuper-clone && > + ( > + cd supersuper-clone && > + # test superproject has alternates setup correctly > + test_alternate_is_used .git/objects/info/alternates . && > + # immediate submodule has alternate: > + test_alternate_is_used .git/modules/subwithsub/objects/info/alternates subwithsub here is a && missing ;) ``` ## Jeff King, 2016-12-08 17:46 Subject: Re: [PATCH] submodule--helper: set alternateLocation for cloned submodules Message-ID: <20161208174633.bsktiflql6jpn5t3@sigill.intra.peff.net> URL: https://gitlist.dev/e/20161208174633.bsktiflql6jpn5t3%40sigill.intra.peff.net In-Reply-To: ``` On Wed, Dec 07, 2016 at 05:22:30PM -0800, Stefan Beller wrote: > On Wed, Dec 7, 2016 at 4:39 PM, wrote: > > > > > Previously test contained errorneous > > test_must_fail, which was masked by > > missing &&. > > I wonder if we could make either > the test_must_fail intelligent to detect such a broken && call chain > or the test_expect_success macro to see for those broken chains. I don't think test_must_fail is relevant for &&-chains. Even something like: test_must_fail foo bar or: bar test_must_fail foo will both trigger on the &&-chain linter, because it uses a magic exit code to detect the breakage. I think the problem is just that the &&-chain linter cannot peek inside subshells, and that's where the bug was in this case. I wish we could improve that, but I spend a lot of brain cycles on it at one point and couldn't come up with a workable solution. -Peff ``` ## Stefan Beller, 2016-12-08 18:04 Subject: Re: [PATCH] submodule--helper: set alternateLocation for cloned submodules Message-ID: URL: https://gitlist.dev/e/CAGZ79kbbi1J3BgQF-Ws0Ud-4h9efuQJTdsiA7Skh1fZXH5xqgg%40mail.gmail.com In-Reply-To: <20161208174633.bsktiflql6jpn5t3@sigill.intra.peff.net> ``` On Thu, Dec 8, 2016 at 9:46 AM, Jeff King wrote: > > will both trigger on the &&-chain linter, because it uses a magic exit > code to detect the breakage. I think the problem is just that the > &&-chain linter cannot peek inside subshells, and that's where the bug > was in this case. Uh, yeah in the subshell, but the patch v2 did have it not in subshells, I'll take another look. > > I wish we could improve that, but I spend a lot of brain cycles on it at > one point and couldn't come up with a workable solution. Is it possible to overwrite what happens when you open a subshell with ( ) ? i.e. I imagine in the global test-setup we'd setup that ( ) not just opens /bin/sh but a shell with benefits such that we can execute just one && chain. ``` ## vi0oss, 2016-12-08 18:04 Subject: Re: [PATCH] submodule--helper: set alternateLocation for cloned submodules Message-ID: URL: https://gitlist.dev/e/d445a6c3-5375-22cf-4f03-1717559f1157%40gmail.com In-Reply-To: <20161208174633.bsktiflql6jpn5t3@sigill.intra.peff.net> ``` On 12/08/2016 08:46 PM, Jeff King wrote: > On Wed, Dec 07, 2016 at 05:22:30PM -0800, Stefan Beller wrote: > >> On Wed, Dec 7, 2016 at 4:39 PM, wrote: >> >>> Previously test contained errorneous >>> test_must_fail, which was masked by >>> missing &&. >> I wonder if we could make either >> the test_must_fail intelligent to detect such a broken && call chain >> or the test_expect_success macro to see for those broken chains. >> >> >> I wish we could improve that, but I spend a lot of brain cycles on it at >> one point and couldn't come up with a workable solution. >> >> -Peff >> Why Git test use &&-chains instead of proper "set -e"? ``` ## Stefan Beller, 2016-12-08 18:21 Subject: Re: [PATCH] submodule--helper: set alternateLocation for cloned submodules Message-ID: URL: https://gitlist.dev/e/CAGZ79kZs4vy3-1jAxY%2B95CNRE2_YgTcBTsxR-Mg0xLqCp7UHcA%40mail.gmail.com In-Reply-To: ``` On Thu, Dec 8, 2016 at 10:04 AM, vi0oss wrote: > On 12/08/2016 08:46 PM, Jeff King wrote: >> >> On Wed, Dec 07, 2016 at 05:22:30PM -0800, Stefan Beller wrote: >> >>> On Wed, Dec 7, 2016 at 4:39 PM, wrote: >>> >>>> Previously test contained errorneous >>>> test_must_fail, which was masked by >>>> missing &&. >>> >>> I wonder if we could make either >>> the test_must_fail intelligent to detect such a broken && call chain >>> or the test_expect_success macro to see for those broken chains. >>> >>> >>> I wish we could improve that, but I spend a lot of brain cycles on it at >>> one point and couldn't come up with a workable solution. >>> >>> -Peff >>> > Why Git test use &&-chains instead of proper "set -e"? > "Because set -e kills the shell and we would want to keep going until the test suite is finished and display a summary what failed" would be my first reaction, but let's dig into history: bb79af9d09 might be helpful on that, but it doesn't explain why we use && chains. I could not find any commit explaining the use of && chains. e1970ce43abf might be interesting (the introduction of the test suite), as that did not contain && chains. I guess it would be hard(er) to implement e.g. test_must_fail in an environment where -e is set. ``` ## Jeff King, 2016-12-08 18:53 Subject: Re: [PATCH] submodule--helper: set alternateLocation for cloned submodules Message-ID: <20161208185326.osknwhc654sqaq3t@sigill.intra.peff.net> URL: https://gitlist.dev/e/20161208185326.osknwhc654sqaq3t%40sigill.intra.peff.net In-Reply-To: ``` On Thu, Dec 08, 2016 at 09:04:46PM +0300, vi0oss wrote: > Why Git test use &&-chains instead of proper "set -e"? Because "set -e" comes with all kinds of confusing corner cases. Using && chains is annoying, but rarely surprising. One of my favorite examples is: set -e ( false echo 1 ) || { echo outcome=$? false } echo 2 which prints both "1" and "2". Inside the subshell, "set -e" has no effect, and you cannot re-enable it by setting "-e" (it's suppressed entirely because we are on the left-hand side of an || conditional). So you could write a function like this: foo() { do_one do_two } that relies on catching the failure from do_one. And it works here: set -e foo but not here: set -e if foo then do_something fi And there's no way to make it work without adding back in the &&-chaining. -Peff ```