{"thread":{"id":"44644","subject":"[PATCH] submodule--helper: set alternateLocation for cloned submodules","startedAt":"2016-12-07T18:43:55Z","lastAt":"2016-12-07T22:09:48Z","messageCount":7,"participants":["vi0oss@gmail.com","Stefan Beller","Junio C Hamano","vi0oss"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"307144","messageId":"20161207184248.6130-1-vi0oss@gmail.com","threadId":"44644","inReplyTo":null,"subject":"[PATCH] submodule--helper: set alternateLocation for cloned submodules","fromName":"","fromEmail":"vi0oss@gmail.com","sentAt":"2016-12-07T18:42:48Z","receivedAt":"2016-12-07T18:43:55Z","isPatch":true,"sender":{"key":"vi0oss@gmail.com","avatar":"https://avatars.githubusercontent.com/u/173219?v=4"},"body":"From: Vitaly _Vi Shukela <vi0oss@gmail.com>\n\nGit v2.11 introduced \"git clone --recursive --referece ...\",\nbut it didn't put the alternates for _nested_ submodules.\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 <vi0oss@gmail.com>\n---\n builtin/submodule--helper.c    | 24 ++++++++++++--\n t/t7408-submodule-reference.sh | 73 ++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 95 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 4beeda5..93dae62 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/<name>/objects\n+\t * standard layout with .git/(modules/<name>)+/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@@ -672,6 +672,26 @@ 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+\t{\n+\t\tchar *sm_alternate = NULL, *error_strategy = NULL;\n+\n+\t\tgit_config_get_string(\"submodule.alternateLocation\", &sm_alternate);\n+\t\tif (sm_alternate) {\n+\t\t\tgit_config_set_in_file(p, \"submodule.alternateLocation\",\n+\t\t\t\t\t\t   sm_alternate);\n+\t\t}\n+\t\tgit_config_get_string(\"submodule.alternateErrorStrategy\", &error_strategy);\n+\t\tif (error_strategy) {\n+\t\t\tgit_config_set_in_file(p, \"submodule.alternateErrorStrategy\",\n+\t\t\t\t\t\t   error_strategy);\n+\t\t}\n+\n+\t\tfree(sm_alternate);\n+\t\tfree(error_strategy);\n+\t}\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..7b64725 100755\n--- a/t/t7408-submodule-reference.sh\n+++ b/t/t7408-submodule-reference.sh\n@@ -125,4 +125,77 @@ 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' '\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+\techo not cleaning supersuper\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+test_expect_success 'missing nested submodule alternate fails clone and submodule update' '\n+\ttest_when_finished \"rm -rf supersuper-clone supersuper2\" &&\n+\tgit clone supersuper supersuper2 &&\n+\t(\n+\t\tcd supersuper2 &&\n+\t\tgit submodule update --init\n+\t) &&\n+\ttest_must_fail git clone --recursive --reference supersuper2 supersuper2 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# update of the submodule fails\n+\t\ttest_must_fail git submodule update --init --recursive &&\n+\t\t# immediate submodule has alternate:\n+\t\ttest_alternate_is_used .git/modules/subwithsub/objects/info/alternates subwithsub\n+\t\t# but nested submodule has no alternate:\n+\t\ttest_must_fail test_alternate_is_used .git/modules/subwithsub/modules/sub/objects/info/alternates subwithsub/sub\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 supersuper2\" &&\n+\tgit clone supersuper supersuper2 &&\n+\t(\n+\t\tcd supersuper2 &&\n+\t\tgit submodule update --init\n+\t) &&\n+\tgit clone --recursive --reference-if-able supersuper2 supersuper2 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# update of the submodule fails\n+\t\ttest_must_fail git submodule update --init --recursive &&\n+\t\t# immediate submodule has alternate:\n+\t\ttest_alternate_is_used .git/modules/subwithsub/objects/info/alternates subwithsub\n+\t\t# but nested submodule has no alternate:\n+\t\ttest_must_fail test_alternate_is_used .git/modules/subwithsub/modules/sub/objects/info/alternates subwithsub/sub\n+\t)\n+'\n+\n test_done\n-- \n2.10.2\n\n"},{"id":"307150","messageId":"CAGZ79kY3LR2KA69b4iDJb164EhJLb3JuVSRRcN0-4-kp-eryog@mail.gmail.com","threadId":"44644","inReplyTo":"20161207184248.6130-1-vi0oss@gmail.com","subject":"Re: [PATCH] submodule--helper: set alternateLocation for cloned submodules","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-12-07T20:09:39Z","receivedAt":"2016-12-07T20:09:46Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, Dec 7, 2016 at 10:42 AM,  <vi0oss@gmail.com> wrote:\n> From: Vitaly _Vi Shukela <vi0oss@gmail.com>\n\nThanks for contributing to Git!\n(/me looks up if you have sent patches already as you\nseem to know how to do that. :) unrelated side note: Maybe you want\nto send a patch for the .mailmap file mapping your two email addresses\ntogether, c.f. \"git log -- .mailmap\")\n\n>\n> Git v2.11 introduced \"git clone --recursive --referece ...\",\n> but it didn't put the alternates for _nested_ submodules.\n\nThis message is targeted at people familiar with gits code base,\nso we can be more specific. e.g.\n\n    In 31224cbdc7 (clone: recursive and reference option triggers\n    submodule alternates, 2016-08-17) a mechanism was added to\n    have submodules referenced.  It did not address _nested_\n    submodules, however.\n\n>\n> This patch makes all not just the root repository, but also\n> all submodules (recursively) have submodule.alternateLocation\n> and submodule.alternateErrorStrategy configured, making Git\n> search for possible alternates for nested submodules as well.\n\nSounds great!\n\n>\n> As submodule's alternate target does not end in .git/objects\n> (rather .git/modules/qqqqqq/objects), this alternate target\n> path restriction for in add_possible_reference_from_superproject\n> relates from \"*.git/objects\" to just */objects\".\n\nI wonder if this check is too weak and we actually have to check for\neither .git/objects or modules/<name/possibly/having/slashes>/objects.\nWhen writing the referenced commit I assumed we'd need a stronger check\nto be safer and not add some random location as a possible alternate.\n\n>\n> New tests have been added to t7408-submodule-reference.\n\nThanks!\n\n>\n> Signed-off-by: Vitaly _Vi Shukela <vi0oss@gmail.com>\n> ---\n>  builtin/submodule--helper.c    | 24 ++++++++++++--\n>  t/t7408-submodule-reference.sh | 73 ++++++++++++++++++++++++++++++++++++++++++\n>  2 files changed, 95 insertions(+), 2 deletions(-)\n>\n> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\n> index 4beeda5..93dae62 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>         /*\n>          * If the alternate object store is another repository, try the\n> -        * standard layout with .git/modules/<name>/objects\n> +        * standard layout with .git/(modules/<name>)+/objects\n>          */\n> -       if (ends_with(alt->path, \".git/objects\")) {\n> +       if (ends_with(alt->path, \"/objects\")) {\n>                 char *sm_alternate;\n>                 struct strbuf sb = STRBUF_INIT;\n>                 struct strbuf err = STRBUF_INIT;\n> @@ -672,6 +672,26 @@ static int module_clone(int argc, const char **argv, const char *prefix)\n>                 die(_(\"could not get submodule directory for '%s'\"), path);\n>         git_config_set_in_file(p, \"core.worktree\",\n>                                relative_path(path, sm_gitdir, &rel_path));\n> +\n> +       /* setup alternateLocation and alternateErrorStrategy in the cloned submodule if needed */\n> +       {\n\nUsually we do not use braces to further nest code, please remove this nesting.\n\n> +               char *sm_alternate = NULL, *error_strategy = NULL;\n> +\n> +               git_config_get_string(\"submodule.alternateLocation\", &sm_alternate);\n> +               if (sm_alternate) {\n> +                       git_config_set_in_file(p, \"submodule.alternateLocation\",\n> +                                                  sm_alternate);\n> +               }\n> +               git_config_get_string(\"submodule.alternateErrorStrategy\", &error_strategy);\n> +               if (error_strategy) {\n> +                       git_config_set_in_file(p, \"submodule.alternateErrorStrategy\",\n> +                                                  error_strategy);\n> +               }\n> +\n> +               free(sm_alternate);\n> +               free(error_strategy);\n> +       }\n> +\n>         strbuf_release(&sb);\n>         strbuf_release(&rel_path);\n>         free(sm_gitdir);\n> diff --git a/t/t7408-submodule-reference.sh b/t/t7408-submodule-reference.sh\n> index 1c1e289..7b64725 100755\n> --- a/t/t7408-submodule-reference.sh\n> +++ b/t/t7408-submodule-reference.sh\n> @@ -125,4 +125,77 @@ test_expect_success 'ignoring missing submodule alternates passes clone and subm\n>         )\n>  '\n>\n> +test_expect_success 'preparing second superproject with a nested submodule' '\n> +       test_create_repo supersuper &&\n> +       (\n> +               cd supersuper &&\n> +               echo I am super super. >file &&\n\nUsually we quote strings containing white space, e.g. echo \"I am ...\" >actual\n\n> +               git add file &&\n> +               git commit -m B-super-super-initial\n> +               git submodule add \"file://$base_dir/super\" subwithsub &&\n> +               git commit -m B-super-super-added &&\n> +               git submodule update --init --recursive &&\n> +               git repack -ad\n> +       ) &&\n> +       echo not cleaning supersuper\n\nThis echo is left in for debugging purposes?\n"},{"id":"307152","messageId":"xmqq4m2fxzl2.fsf@gitster.mtv.corp.google.com","threadId":"44644","inReplyTo":"CAGZ79kY3LR2KA69b4iDJb164EhJLb3JuVSRRcN0-4-kp-eryog@mail.gmail.com","subject":"Re: [PATCH] submodule--helper: set alternateLocation for cloned submodules","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-12-07T20:18:33Z","receivedAt":"2016-12-07T20:18:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n>> This patch makes all not just the root repository, but also\n>> all submodules (recursively) have submodule.alternateLocation\n>> and submodule.alternateErrorStrategy configured, making Git\n>> search for possible alternates for nested submodules as well.\n>\n> Sounds great!\n\nIs it safe to assume that all the submodules used recursively by\nsubmodules share the same structure upstream?  Does the alternate\nlocation mechanism degrades sensibly if this assumption turns out to\nbe false (i.e. \"possible alternates\" above turns out to be mere\npossibility and not there)?\n"},{"id":"307153","messageId":"CAGZ79ka=XgO-VyS+Jqq2Fy28kPMy6HnXBAHb79Dt3NegZFd6kw@mail.gmail.com","threadId":"44644","inReplyTo":"xmqq4m2fxzl2.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] submodule--helper: set alternateLocation for cloned submodules","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-12-07T20:26:36Z","receivedAt":"2016-12-07T20:26:41Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, Dec 7, 2016 at 12:18 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Stefan Beller <sbeller@google.com> writes:\n>\n>>> This patch makes all not just the root repository, but also\n>>> all submodules (recursively) have submodule.alternateLocation\n>>> and submodule.alternateErrorStrategy configured, making Git\n>>> search for possible alternates for nested submodules as well.\n>>\n>> Sounds great!\n>\n> Is it safe to assume that all the submodules used recursively by\n> submodules share the same structure upstream?  Does the alternate\n> location mechanism degrades sensibly if this assumption turns out to\n> be false (i.e. \"possible alternates\" above turns out to be mere\n> possibility and not there)?\n\nAccording to the last test in the patch, this seems to be doing the\nsensible thing.\n"},{"id":"307154","messageId":"xmqqzik7wkj9.fsf@gitster.mtv.corp.google.com","threadId":"44644","inReplyTo":"CAGZ79ka=XgO-VyS+Jqq2Fy28kPMy6HnXBAHb79Dt3NegZFd6kw@mail.gmail.com","subject":"Re: [PATCH] submodule--helper: set alternateLocation for cloned submodules","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-12-07T20:28:58Z","receivedAt":"2016-12-07T20:29:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> On Wed, Dec 7, 2016 at 12:18 PM, Junio C Hamano <gitster@pobox.com> wrote:\n>> Stefan Beller <sbeller@google.com> writes:\n>>\n>>>> This patch makes all not just the root repository, but also\n>>>> all submodules (recursively) have submodule.alternateLocation\n>>>> and submodule.alternateErrorStrategy configured, making Git\n>>>> search for possible alternates for nested submodules as well.\n>>>\n>>> Sounds great!\n>>\n>> Is it safe to assume that all the submodules used recursively by\n>> submodules share the same structure upstream?  Does the alternate\n>> location mechanism degrades sensibly if this assumption turns out to\n>> be false (i.e. \"possible alternates\" above turns out to be mere\n>> possibility and not there)?\n>\n> According to the last test in the patch, this seems to be doing the\n> sensible thing.\n\nOK, that sounds great.  Thanks.\n"},{"id":"307168","messageId":"9493435c-16e5-d8af-9455-ec3f3cba390d@gmail.com","threadId":"44644","inReplyTo":"CAGZ79kY3LR2KA69b4iDJb164EhJLb3JuVSRRcN0-4-kp-eryog@mail.gmail.com","subject":"Re: [PATCH] submodule--helper: set alternateLocation for cloned submodules","fromName":"vi0oss","fromEmail":"vi0oss@gmail.com","sentAt":"2016-12-07T21:24:07Z","receivedAt":"2016-12-07T21:24:41Z","isPatch":true,"sender":{"key":"vi0oss@gmail.com","avatar":"https://avatars.githubusercontent.com/u/173219?v=4"},"body":"On 12/07/2016 11:09 PM, Stefan Beller wrote:\n>> As submodule's alternate target does not end in .git/objects\n>> (rather .git/modules/qqqqqq/objects), this alternate target\n>> path restriction for in add_possible_reference_from_superproject\n>> relates from \"*.git/objects\" to just */objects\".\n> I wonder if this check is too weak and we actually have to check for\n> either .git/objects or modules/<name/possibly/having/slashes>/objects.\n> When writing the referenced commit I assumed we'd need a stronger check\n> to be safer and not add some random location as a possible alternate.\n>\n1. Do we really need to check that it is named \".git\"? Although\n\"git clone --mirror --recursive\" is not (directly) supported\nnow, user may create one bare repository with [remnants of]\nsubmodules by converting reqular repository into bare one.\nWhy not take advantage of additional information available locally\nin this case?\n\n2. Is the check need to be strict because of we need to traverse\none directory level up? Normally this \"/objects\" part is added by\nGit, so just one \"../\" seems to be OK. User can't specify \"--reference\nsomerepo/.git/objects\", a strange reference can appear only if user\nmanually creates alternates. Maybe better to document this case\ninstead of restricting the feature?\n\n3. If nonetheless check for \".git/*/objects\", then\na. What functions should be used in Git codebase for such checks?\nb. Should we handle tricks like \"smth/.git/../../objects\" and so on?\n\n4. Should we print (or print in verbose mode) each used alternate,\nto inform operator what his or her new clone will depend on?\n\nP.S. Actually I discovered the --recursive --reference feature when tried to\nput reference to a mega-repo with all possible submodules added as remotes.\nI expected --reference to just get though across all submodules, but it \ncomplained\nto missing \"/modules/...\" instead (the check went though becase the \nrepository\nwas named like \"megarepo.git\", so it did ended in \".git/objects\").\n"},{"id":"307178","messageId":"CAGZ79kYuNri2aVtvRESxA9=E5OuBBqVkU_TcekxdyNJobFn5Mg@mail.gmail.com","threadId":"44644","inReplyTo":"9493435c-16e5-d8af-9455-ec3f3cba390d@gmail.com","subject":"Re: [PATCH] submodule--helper: set alternateLocation for cloned submodules","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-12-07T22:09:42Z","receivedAt":"2016-12-07T22:09:48Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, Dec 7, 2016 at 1:24 PM, vi0oss <vi0oss@gmail.com> wrote:\n> On 12/07/2016 11:09 PM, Stefan Beller wrote:\n>>>\n>>> As submodule's alternate target does not end in .git/objects\n>>> (rather .git/modules/qqqqqq/objects), this alternate target\n>>> path restriction for in add_possible_reference_from_superproject\n>>> relates from \"*.git/objects\" to just */objects\".\n>>\n>> I wonder if this check is too weak and we actually have to check for\n>> either .git/objects or modules/<name/possibly/having/slashes>/objects.\n>> When writing the referenced commit I assumed we'd need a stronger check\n>> to be safer and not add some random location as a possible alternate.\n>>\n> 1. Do we really need to check that it is named \".git\"? Although\n> \"git clone --mirror --recursive\" is not (directly) supported\n> now, user may create one bare repository with [remnants of]\n> submodules by converting reqular repository into bare one.\n> Why not take advantage of additional information available locally\n> in this case?\n\nOh, great point.\n\n>\n> 2. Is the check need to be strict because of we need to traverse\n> one directory level up? Normally this \"/objects\" part is added by\n> Git, so just one \"../\" seems to be OK. User can't specify \"--reference\n> somerepo/.git/objects\", a strange reference can appear only if user\n> manually creates alternates. Maybe better to document this case\n> instead of restricting the feature?\n\nNot sure I understand what needs better documentation here?\n\n>\n> 3. If nonetheless check for \".git/*/objects\", then\n> a. What functions should be used in Git codebase for such checks?\n> b. Should we handle tricks like \"smth/.git/../../objects\" and so on?\n\nI see we're getting into problems here.\n\n>\n> 4. Should we print (or print in verbose mode) each used alternate,\n> to inform operator what his or her new clone will depend on?\n>\n> P.S. Actually I discovered the --recursive --reference feature when tried to\n> put reference to a mega-repo with all possible submodules added as remotes.\n> I expected --reference to just get though across all submodules, but it\n> complained\n> to missing \"/modules/...\" instead (the check went though becase the\n> repository\n> was named like \"megarepo.git\", so it did ended in \".git/objects\").\n\nOh :(\n\nWith that said, I think the original patch is a sensible approach.\n\nThanks,\nStefan\n"}]}