{"thread":{"id":"44648","subject":"[PATCH] submodule--helper: set alternateLocation for cloned submodules","startedAt":"2016-12-08T00:40:02Z","lastAt":"2016-12-08T18:53:41Z","messageCount":7,"participants":["vi0oss@gmail.com","Stefan Beller","Jeff King","vi0oss"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"307205","messageId":"20161208003940.28794-1-vi0oss@gmail.com","threadId":"44648","inReplyTo":null,"subject":"[PATCH] submodule--helper: set alternateLocation for cloned submodules","fromName":"","fromEmail":"vi0oss@gmail.com","sentAt":"2016-12-08T00:39:40Z","receivedAt":"2016-12-08T00:40:02Z","isPatch":true,"sender":{"key":"vi0oss@gmail.com","avatar":"https://avatars.githubusercontent.com/u/173219?v=4"},"body":"From: \"Vitaly \\\"_Vi\\\" Shukela\" \n\nIn 31224cbdc7 (clone: recursive and reference option triggers\nsubmodule alternates, 2016-08-17) a mechanism was added to\nhave submodules referenced. It did not address _nested_\nsubmodules, however.\n\nThis patch makes all not just the root repository, but also\nall submodules (recursively) have submodule.alternateLocation\nand submodule.alternateErrorStrategy configured, making Git\nsearch for possible alternates for nested submodules as well.\n\nAs submodule's alternate target does not end in .git/objects\n(rather .git/modules/qqqqqq/objects), this alternate target\npath restriction for in add_possible_reference_from_superproject\nrelates from \"*.git/objects\" to just */objects\".\n\nNew tests have been added to t7408-submodule-reference.\n\nSigned-off-by: Vitaly _Vi Shukela \n---\n\nNotes:\n Another round of fixups:\n \n * Corrected code style\n * Refactored and fixed the test\n \n Previously test contained errorneous\n test_must_fail, which was masked by\n missing &&.\n \n Mailmap patch omitted this time.\n\n builtin/submodule--helper.c | 19 ++++++++++--\n t/t7408-submodule-reference.sh | 66 ++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 83 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 4beeda5..92fd676 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -498,9 +498,9 @@ static int add_possible_reference_from_superproject(\n \n \t/*\n \t * If the alternate object store is another repository, try the\n-\t * standard layout with .git/modules//objects\n+\t * standard layout with .git/(modules/)+/objects\n \t */\n-\tif (ends_with(alt->path, \".git/objects\")) {\n+\tif (ends_with(alt->path, \"/objects\")) {\n \t\tchar *sm_alternate;\n \t\tstruct strbuf sb = STRBUF_INIT;\n \t\tstruct strbuf err = STRBUF_INIT;\n@@ -583,6 +583,7 @@ static int module_clone(int argc, const char **argv, const char *prefix)\n \tstruct strbuf rel_path = STRBUF_INIT;\n \tstruct strbuf sb = STRBUF_INIT;\n \tstruct string_list reference = STRING_LIST_INIT_NODUP;\n+\tchar *sm_alternate = NULL, *error_strategy = NULL;\n \n \tstruct option module_clone_options[] = {\n \t\tOPT_STRING(0, \"prefix\", &prefix,\n@@ -672,6 +673,20 @@ static int module_clone(int argc, const char **argv, const char *prefix)\n \t\tdie(_(\"could not get submodule directory for '%s'\"), path);\n \tgit_config_set_in_file(p, \"core.worktree\",\n \t\t\t relative_path(path, sm_gitdir, &rel_path));\n+\n+\t/* setup alternateLocation and alternateErrorStrategy in the cloned submodule if needed */\n+\tgit_config_get_string(\"submodule.alternateLocation\", &sm_alternate);\n+\tif (sm_alternate)\n+\t\tgit_config_set_in_file(p, \"submodule.alternateLocation\",\n+\t\t\t\t\t sm_alternate);\n+\tgit_config_get_string(\"submodule.alternateErrorStrategy\", &error_strategy);\n+\tif (error_strategy)\n+\t\tgit_config_set_in_file(p, \"submodule.alternateErrorStrategy\",\n+\t\t\t\t\t error_strategy);\n+\n+\tfree(sm_alternate);\n+\tfree(error_strategy);\n+\n \tstrbuf_release(&sb);\n \tstrbuf_release(&rel_path);\n \tfree(sm_gitdir);\ndiff --git a/t/t7408-submodule-reference.sh b/t/t7408-submodule-reference.sh\nindex 1c1e289..9325297 100755\n--- a/t/t7408-submodule-reference.sh\n+++ b/t/t7408-submodule-reference.sh\n@@ -125,4 +125,70 @@ test_expect_success 'ignoring missing submodule alternates passes clone and subm\n \t)\n '\n \n+test_expect_success 'preparing second superproject with a nested submodule plus partial clone' '\n+\ttest_create_repo supersuper &&\n+\t(\n+\t\tcd supersuper &&\n+\t\techo \"I am super super.\" >file &&\n+\t\tgit add file &&\n+\t\tgit commit -m B-super-super-initial\n+\t\tgit submodule add \"file://$base_dir/super\" subwithsub &&\n+\t\tgit commit -m B-super-super-added &&\n+\t\tgit submodule update --init --recursive &&\n+\t\tgit repack -ad\n+\t) &&\n+\tgit clone supersuper supersuper2 &&\n+\t(\n+\t\tcd supersuper2 &&\n+\t\tgit submodule update --init\n+\t)\n+'\n+\n+# At this point there are three root-level positories: A, B, super and super2\n+\n+test_expect_success 'nested submodule alternate in works and is actually used' '\n+\ttest_when_finished \"rm -rf supersuper-clone\" &&\n+\tgit clone --recursive --reference supersuper supersuper supersuper-clone &&\n+\t(\n+\t\tcd supersuper-clone &&\n+\t\t# test superproject has alternates setup correctly\n+\t\ttest_alternate_is_used .git/objects/info/alternates . &&\n+\t\t# immediate submodule has alternate:\n+\t\ttest_alternate_is_used .git/modules/subwithsub/objects/info/alternates subwithsub\n+\t\t# nested submodule also has alternate:\n+\t\ttest_alternate_is_used .git/modules/subwithsub/modules/sub/objects/info/alternates subwithsub/sub\n+\t)\n+'\n+\n+check_that_two_of_three_alternates_are_used() {\n+\ttest_alternate_is_used .git/objects/info/alternates . &&\n+\t# immediate submodule has alternate:\n+\ttest_alternate_is_used .git/modules/subwithsub/objects/info/alternates subwithsub &&\n+\t# but nested submodule has no alternate:\n+\ttest_must_fail test_alternate_is_used .git/modules/subwithsub/modules/sub/objects/info/alternates subwithsub/sub\n+}\n+\n+\n+test_expect_success 'missing nested submodule alternate fails clone and submodule update' '\n+\ttest_when_finished \"rm -rf supersuper-clone\" &&\n+\ttest_must_fail git clone --recursive --reference supersuper2 supersuper2 supersuper-clone &&\n+\t(\n+\t\tcd supersuper-clone &&\n+\t\tcheck_that_two_of_three_alternates_are_used &&\n+\t\t# update of the submodule fails\n+\t\ttest_must_fail git submodule update --init --recursive\n+\t)\n+'\n+\n+test_expect_success 'missing nested submodule alternate in --reference-if-able mode' '\n+\ttest_when_finished \"rm -rf supersuper-clone\" &&\n+\tgit clone --recursive --reference-if-able supersuper2 supersuper2 supersuper-clone &&\n+\t(\n+\t\tcd supersuper-clone &&\n+\t\tcheck_that_two_of_three_alternates_are_used &&\n+\t\t# update of the submodule succeeds\n+\t\tgit submodule update --init --recursive\n+\t)\n+'\n+\n test_done\n-- \n2.10.2\n\n"},{"id":"307209","messageId":"CAGZ79kZU401JRp4EtwTGHzk3Zq+snQhX3GArDfF6SpKxsSwtWg@mail.gmail.com","threadId":"44648","inReplyTo":"20161208003940.28794-1-vi0oss@gmail.com","subject":"Re: [PATCH] submodule--helper: set alternateLocation for cloned submodules","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-12-08T01:22:30Z","receivedAt":"2016-12-08T01:22:40Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, Dec 7, 2016 at 4:39 PM, wrote:\n\n>\n> Previously test contained errorneous\n> test_must_fail, which was masked by\n> missing &&.\n\nI wonder if we could make either\nthe test_must_fail intelligent to detect such a broken && call chain\nor the test_expect_success macro to see for those broken chains.\n\nPatch looks good to me except one very minor nit. :)\n\n> +test_expect_success 'nested submodule alternate in works and is actually used' '\n> + test_when_finished \"rm -rf supersuper-clone\" &&\n> + git clone --recursive --reference supersuper supersuper supersuper-clone &&\n> + (\n> + cd supersuper-clone &&\n> + # test superproject has alternates setup correctly\n> + test_alternate_is_used .git/objects/info/alternates . &&\n> + # immediate submodule has alternate:\n> + test_alternate_is_used .git/modules/subwithsub/objects/info/alternates subwithsub\n\nhere is a && missing ;)\n"},{"id":"307267","messageId":"20161208174633.bsktiflql6jpn5t3@sigill.intra.peff.net","threadId":"44648","inReplyTo":"CAGZ79kZU401JRp4EtwTGHzk3Zq+snQhX3GArDfF6SpKxsSwtWg@mail.gmail.com","subject":"Re: [PATCH] submodule--helper: set alternateLocation for cloned submodules","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-12-08T17:46:33Z","receivedAt":"2016-12-08T17:46:39Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Dec 07, 2016 at 05:22:30PM -0800, Stefan Beller wrote:\n\n> On Wed, Dec 7, 2016 at 4:39 PM, wrote:\n> \n> >\n> > Previously test contained errorneous\n> > test_must_fail, which was masked by\n> > missing &&.\n> \n> I wonder if we could make either\n> the test_must_fail intelligent to detect such a broken && call chain\n> or the test_expect_success macro to see for those broken chains.\n\nI don't think test_must_fail is relevant for &&-chains. Even something\nlike:\n\n test_must_fail foo\n bar\n\nor:\n\n bar\n test_must_fail foo\n\nwill both trigger on the &&-chain linter, because it uses a magic exit\ncode to detect the breakage. I think the problem is just that the\n&&-chain linter cannot peek inside subshells, and that's where the bug\nwas in this case.\n\nI wish we could improve that, but I spend a lot of brain cycles on it at\none point and couldn't come up with a workable solution.\n\n-Peff\n"},{"id":"307270","messageId":"CAGZ79kbbi1J3BgQF-Ws0Ud-4h9efuQJTdsiA7Skh1fZXH5xqgg@mail.gmail.com","threadId":"44648","inReplyTo":"20161208174633.bsktiflql6jpn5t3@sigill.intra.peff.net","subject":"Re: [PATCH] submodule--helper: set alternateLocation for cloned submodules","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-12-08T18:04:03Z","receivedAt":"2016-12-08T18:04:08Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Dec 8, 2016 at 9:46 AM, Jeff King wrote:\n>\n> will both trigger on the &&-chain linter, because it uses a magic exit\n> code to detect the breakage. I think the problem is just that the\n> &&-chain linter cannot peek inside subshells, and that's where the bug\n> was in this case.\n\nUh, yeah in the subshell, but the patch v2 did have it not in\nsubshells, I'll take another look.\n\n>\n> I wish we could improve that, but I spend a lot of brain cycles on it at\n> one point and couldn't come up with a workable solution.\n\nIs it possible to overwrite what happens when you open a subshell with ( ) ?\ni.e. I imagine in the global test-setup we'd setup that ( ) not just\nopens /bin/sh\nbut a shell with benefits such that we can execute just one && chain.\n"},{"id":"307271","messageId":"d445a6c3-5375-22cf-4f03-1717559f1157@gmail.com","threadId":"44648","inReplyTo":"20161208174633.bsktiflql6jpn5t3@sigill.intra.peff.net","subject":"Re: [PATCH] submodule--helper: set alternateLocation for cloned submodules","fromName":"vi0oss","fromEmail":"vi0oss@gmail.com","sentAt":"2016-12-08T18:04:46Z","receivedAt":"2016-12-08T18:04:55Z","isPatch":true,"sender":{"key":"vi0oss@gmail.com","avatar":"https://avatars.githubusercontent.com/u/173219?v=4"},"body":"On 12/08/2016 08:46 PM, Jeff King wrote:\n> On Wed, Dec 07, 2016 at 05:22:30PM -0800, Stefan Beller wrote:\n>\n>> On Wed, Dec 7, 2016 at 4:39 PM, wrote:\n>>\n>>> Previously test contained errorneous\n>>> test_must_fail, which was masked by\n>>> missing &&.\n>> I wonder if we could make either\n>> the test_must_fail intelligent to detect such a broken && call chain\n>> or the test_expect_success macro to see for those broken chains.\n>>\n>>\n>> I wish we could improve that, but I spend a lot of brain cycles on it at\n>> one point and couldn't come up with a workable solution.\n>>\n>> -Peff\n>>\nWhy Git test use &&-chains instead of proper \"set -e\"?\n\n"},{"id":"307275","messageId":"CAGZ79kZs4vy3-1jAxY+95CNRE2_YgTcBTsxR-Mg0xLqCp7UHcA@mail.gmail.com","threadId":"44648","inReplyTo":"d445a6c3-5375-22cf-4f03-1717559f1157@gmail.com","subject":"Re: [PATCH] submodule--helper: set alternateLocation for cloned submodules","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-12-08T18:21:51Z","receivedAt":"2016-12-08T18:21:57Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Dec 8, 2016 at 10:04 AM, vi0oss wrote:\n> On 12/08/2016 08:46 PM, Jeff King wrote:\n>>\n>> On Wed, Dec 07, 2016 at 05:22:30PM -0800, Stefan Beller wrote:\n>>\n>>> On Wed, Dec 7, 2016 at 4:39 PM, wrote:\n>>>\n>>>> Previously test contained errorneous\n>>>> test_must_fail, which was masked by\n>>>> missing &&.\n>>>\n>>> I wonder if we could make either\n>>> the test_must_fail intelligent to detect such a broken && call chain\n>>> or the test_expect_success macro to see for those broken chains.\n>>>\n>>>\n>>> I wish we could improve that, but I spend a lot of brain cycles on it at\n>>> one point and couldn't come up with a workable solution.\n>>>\n>>> -Peff\n>>>\n> Why Git test use &&-chains instead of proper \"set -e\"?\n>\n\n\"Because set -e kills the shell and we would want to keep going\nuntil the test suite is finished and display a summary what failed\"\nwould be my first reaction, but let's dig into history:\nbb79af9d09 might be helpful on that, but it doesn't explain why\nwe use && chains.\nI could not find any commit explaining the use of && chains.\n\ne1970ce43abf might be interesting (the introduction of the test\nsuite), as that did not contain && chains.\n\nI guess it would be hard(er) to implement e.g. test_must_fail\nin an environment where -e is set.\n"},{"id":"307278","messageId":"20161208185326.osknwhc654sqaq3t@sigill.intra.peff.net","threadId":"44648","inReplyTo":"d445a6c3-5375-22cf-4f03-1717559f1157@gmail.com","subject":"Re: [PATCH] submodule--helper: set alternateLocation for cloned submodules","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-12-08T18:53:27Z","receivedAt":"2016-12-08T18:53:41Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Dec 08, 2016 at 09:04:46PM +0300, vi0oss wrote:\n\n> Why Git test use &&-chains instead of proper \"set -e\"?\n\nBecause \"set -e\" comes with all kinds of confusing corner cases. Using\n&& chains is annoying, but rarely surprising.\n\nOne of my favorite examples is:\n\n set -e\n (\n false\n echo 1\n ) || {\n echo outcome=$?\n false\n }\n echo 2\n\nwhich prints both \"1\" and \"2\".\n\nInside the subshell, \"set -e\" has no effect, and you cannot re-enable it\nby setting \"-e\" (it's suppressed entirely because we are on the\nleft-hand side of an || conditional).\n\nSo you could write a function like this:\n\n foo() {\n do_one\n do_two\n }\n\nthat relies on catching the failure from do_one. And it works here:\n\n set -e\n foo\n\nbut not here:\n\n set -e\n if foo then\n do_something\n fi\n\nAnd there's no way to make it work without adding back in the\n&&-chaining.\n\n-Peff\n"}]}