{"thread":{"id":"43873","subject":"[PATCHv7 8/8] clone: recursive and reference option triggers submodule alternates","startedAt":"2016-08-17T21:51:57Z","lastAt":"2016-08-17T22:18:57Z","messageCount":3,"participants":["Stefan Beller","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"299579","messageId":"20160817214635.22389-1-sbeller@google.com","threadId":"43873","inReplyTo":null,"subject":"[PATCHv7 8/8] clone: recursive and reference option triggers submodule alternates","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-08-17T21:46:35Z","receivedAt":"2016-08-17T21:51:57Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"When `--recursive` and `--reference` is given, it is reasonable to\nexpect that the submodules are created with references to the submodules\nof the given alternate for the superproject.\n\n  An initial attempt to do this was presented to the mailing list, which\n  used flags that are passed around (\"--super-reference\") that instructed\n  the submodule clone to look for a reference in the submodules of the\n  referenced superproject. This is not well thought out, as any further\n  `submodule update` should also respect the initial setup.\n\n  When a new  submodule is added to the superproject and the alternate\n  of the superproject does not know about that submodule yet, we rather\n  error out informing the user instead of being unclear if we did or did\n  not use a submodules alternate.\n\nTo solve this problem introduce new options that store the configuration\nfor what the user wanted originally.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n\n Added a default for alternateErrorStrategy and hence fixed the null pointer\n for error_strategy in prepare_possible_alternates(),\n \n Thanks,\n Stefan\n\n Documentation/config.txt       | 12 ++++++\n builtin/clone.c                | 19 +++++++++\n builtin/submodule--helper.c    | 93 ++++++++++++++++++++++++++++++++++++++++++\n t/t7408-submodule-reference.sh | 43 +++++++++++++++++++\n 4 files changed, 167 insertions(+)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex bc1c433..e2571ea 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -2837,6 +2837,18 @@ submodule.fetchJobs::\n \tin parallel. A value of 0 will give some reasonable default.\n \tIf unset, it defaults to 1.\n \n+submodule.alternateLocation::\n+\tSpecifies how the submodules obtain alternates when submodules are\n+\tcloned. Possible values are `no`, `superproject`.\n+\tBy default `no` is assumed, which doesn't add references. When the\n+\tvalue is set to `superproject` the submodule to be cloned computes\n+\tits alternates location relative to the superprojects alternate.\n+\n+submodule.alternateErrorStrategy\n+\tSpecifies how to treat errors with the alternates for a submodule\n+\tas computed via `submodule.alternateLocation`. Possible values are\n+\t`ignore`, `info`, `die`. Default is `die`.\n+\n tag.forceSignAnnotated::\n \tA boolean to specify whether annotated tags created should be GPG signed.\n \tIf `--annotate` is specified on the command line, it takes\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex 0182665..404c5e8 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -947,6 +947,25 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \t\telse\n \t\t\tfprintf(stderr, _(\"Cloning into '%s'...\\n\"), dir);\n \t}\n+\n+\tif (option_recursive) {\n+\t\tif (option_required_reference.nr &&\n+\t\t    option_optional_reference.nr)\n+\t\t\tdie(_(\"clone --recursive is not compatible with \"\n+\t\t\t      \"both --reference and --reference-if-able\"));\n+\t\telse if (option_required_reference.nr) {\n+\t\t\tstring_list_append(&option_config,\n+\t\t\t\t\"submodule.alternateLocation=superproject\");\n+\t\t\tstring_list_append(&option_config,\n+\t\t\t\t\"submodule.alternateErrorStrategy=die\");\n+\t\t} else if (option_optional_reference.nr) {\n+\t\t\tstring_list_append(&option_config,\n+\t\t\t\t\"submodule.alternateLocation=superproject\");\n+\t\t\tstring_list_append(&option_config,\n+\t\t\t\t\"submodule.alternateErrorStrategy=info\");\n+\t\t}\n+\t}\n+\n \tinit_db(option_template, INIT_DB_QUIET);\n \twrite_config(&option_config);\n \ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 7096848..f8f35c1 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -472,6 +472,96 @@ static int clone_submodule(const char *path, const char *gitdir, const char *url\n \treturn run_command(&cp);\n }\n \n+struct submodule_alternate_setup {\n+\tconst char *submodule_name;\n+\tenum SUBMODULE_ALTERNATE_ERROR_MODE {\n+\t\tSUBMODULE_ALTERNATE_ERROR_DIE,\n+\t\tSUBMODULE_ALTERNATE_ERROR_INFO,\n+\t\tSUBMODULE_ALTERNATE_ERROR_IGNORE\n+\t} error_mode;\n+\tstruct string_list *reference;\n+};\n+#define SUBMODULE_ALTERNATE_SETUP_INIT { NULL, \\\n+\tSUBMODULE_ALTERNATE_ERROR_IGNORE, NULL }\n+\n+static int add_possible_reference_from_superproject(\n+\t\tstruct alternate_object_database *alt, void *sas_cb)\n+{\n+\tstruct submodule_alternate_setup *sas = sas_cb;\n+\n+\t/* directory name, minus trailing slash */\n+\tsize_t namelen = alt->name - alt->base - 1;\n+\tstruct strbuf name = STRBUF_INIT;\n+\tstrbuf_add(&name, alt->base, namelen);\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 */\n+\tif (ends_with(name.buf, \".git/objects\")) {\n+\t\tchar *sm_alternate;\n+\t\tstruct strbuf sb = STRBUF_INIT;\n+\t\tstruct strbuf err = STRBUF_INIT;\n+\t\tstrbuf_add(&sb, name.buf, name.len - strlen(\"objects\"));\n+\t\t/*\n+\t\t * We need to end the new path with '/' to mark it as a dir,\n+\t\t * otherwise a submodule name containing '/' will be broken\n+\t\t * as the last part of a missing submodule reference would\n+\t\t * be taken as a file name.\n+\t\t */\n+\t\tstrbuf_addf(&sb, \"modules/%s/\", sas->submodule_name);\n+\n+\t\tsm_alternate = compute_alternate_path(sb.buf, &err);\n+\t\tif (sm_alternate) {\n+\t\t\tstring_list_append(sas->reference, xstrdup(sb.buf));\n+\t\t\tfree(sm_alternate);\n+\t\t} else {\n+\t\t\tswitch (sas->error_mode) {\n+\t\t\tcase SUBMODULE_ALTERNATE_ERROR_DIE:\n+\t\t\t\tdie(_(\"submodule '%s' cannot add alternate: %s\"),\n+\t\t\t\t    sas->submodule_name, err.buf);\n+\t\t\tcase SUBMODULE_ALTERNATE_ERROR_INFO:\n+\t\t\t\tfprintf(stderr, _(\"submodule '%s' cannot add alternate: %s\"),\n+\t\t\t\t\tsas->submodule_name, err.buf);\n+\t\t\tcase SUBMODULE_ALTERNATE_ERROR_IGNORE:\n+\t\t\t\t; /* nothing */\n+\t\t\t}\n+\t\t}\n+\t\tstrbuf_release(&sb);\n+\t}\n+\n+\tstrbuf_release(&name);\n+\treturn 0;\n+}\n+\n+static void prepare_possible_alternates(const char *sm_name,\n+\t\tstruct string_list *reference)\n+{\n+\tchar *sm_alternate = NULL, *error_strategy = NULL;\n+\tstruct submodule_alternate_setup sas = SUBMODULE_ALTERNATE_SETUP_INIT;\n+\n+\tgit_config_get_string(\"submodule.alternateLocation\", &sm_alternate);\n+\tif (!sm_alternate)\n+\t\treturn;\n+\n+\tgit_config_get_string(\"submodule.alternateErrorStrategy\", &error_strategy);\n+\n+\tif (!error_strategy)\n+\t\terror_strategy = xstrdup(\"die\");\n+\n+\tsas.submodule_name = sm_name;\n+\tsas.reference = reference;\n+\tif (!strcmp(error_strategy, \"die\"))\n+\t\tsas.error_mode = SUBMODULE_ALTERNATE_ERROR_DIE;\n+\tif (!strcmp(error_strategy, \"info\"))\n+\t\tsas.error_mode = SUBMODULE_ALTERNATE_ERROR_INFO;\n+\tif (!strcmp(sm_alternate, \"superproject\"))\n+\t\tforeach_alt_odb(add_possible_reference_from_superproject, &sas);\n+\n+\tfree(sm_alternate);\n+\tfree(error_strategy);\n+}\n+\n static int module_clone(int argc, const char **argv, const char *prefix)\n {\n \tconst char *name = NULL, *url = NULL, *depth = NULL;\n@@ -532,6 +622,9 @@ static int module_clone(int argc, const char **argv, const char *prefix)\n \tif (!file_exists(sm_gitdir)) {\n \t\tif (safe_create_leading_directories_const(sm_gitdir) < 0)\n \t\t\tdie(_(\"could not create directory '%s'\"), sm_gitdir);\n+\n+\t\tprepare_possible_alternates(name, &reference);\n+\n \t\tif (clone_submodule(path, sm_gitdir, url, depth, &reference, quiet))\n \t\t\tdie(_(\"clone of '%s' into submodule path '%s' failed\"),\n \t\t\t    url, path);\ndiff --git a/t/t7408-submodule-reference.sh b/t/t7408-submodule-reference.sh\nindex dff47af..1c1e289 100755\n--- a/t/t7408-submodule-reference.sh\n+++ b/t/t7408-submodule-reference.sh\n@@ -82,4 +82,47 @@ test_expect_success 'updating superproject keeps alternates' '\n \ttest_alternate_is_used super-clone/.git/modules/sub/objects/info/alternates super-clone/sub\n '\n \n+test_expect_success 'submodules use alternates when cloning a superproject' '\n+\ttest_when_finished \"rm -rf super-clone\" &&\n+\tgit clone --reference super --recursive super super-clone &&\n+\t(\n+\t\tcd super-clone &&\n+\t\t# test superproject has alternates setup correctly\n+\t\ttest_alternate_is_used .git/objects/info/alternates . &&\n+\t\t# test submodule has correct setup\n+\t\ttest_alternate_is_used .git/modules/sub/objects/info/alternates sub\n+\t)\n+'\n+\n+test_expect_success 'missing submodule alternate fails clone and submodule update' '\n+\ttest_when_finished \"rm -rf super-clone\" &&\n+\tgit clone super super2 &&\n+\ttest_must_fail git clone --recursive --reference super2 super2 super-clone &&\n+\t(\n+\t\tcd super-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 succeeds\n+\t\ttest_must_fail git submodule update --init &&\n+\t\t# and we have no alternates:\n+\t\ttest_must_fail test_alternate_is_used .git/modules/sub/objects/info/alternates sub &&\n+\t\ttest_must_fail test_path_is_file sub/file1\n+\t)\n+'\n+\n+test_expect_success 'ignoring missing submodule alternates passes clone and submodule update' '\n+\ttest_when_finished \"rm -rf super-clone\" &&\n+\tgit clone --reference-if-able super2 --recursive super2 super-clone &&\n+\t(\n+\t\tcd super-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 succeeds\n+\t\tgit submodule update --init &&\n+\t\t# and we have no alternates:\n+\t\ttest_must_fail test_alternate_is_used .git/modules/sub/objects/info/alternates sub &&\n+\t\ttest_path_is_file sub/file1\n+\t)\n+'\n+\n test_done\n-- \n2.9.2.582.g4b18b4a\n\n"},{"id":"299581","messageId":"xmqq8tvv82pg.fsf@gitster.mtv.corp.google.com","threadId":"43873","inReplyTo":"20160817214635.22389-1-sbeller@google.com","subject":"Re: [PATCHv7 8/8] clone: recursive and reference option triggers submodule alternates","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-08-17T22:12:27Z","receivedAt":"2016-08-17T22:12:36Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n>  Added a default for alternateErrorStrategy and hence fixed the null pointer\n>  for error_strategy in prepare_possible_alternates(),\n\nLooks much better.\n\n> +submodule.alternateLocation::\n> +\tSpecifies how the submodules obtain alternates when submodules are\n> +\tcloned. Possible values are `no`, `superproject`.\n> +\tBy default `no` is assumed, which doesn't add references. When the\n> +\tvalue is set to `superproject` the submodule to be cloned computes\n> +\tits alternates location relative to the superprojects alternate.\n\nI am not seeing a code that handles 'no' and any other string that\nis not 'superproject' differently, though.\n\nI can see that \"clone\" has codepath that writes 'superproject' to\nthe variable, but the only thing that seems to care what value the\nvariable is set to checks \"no\".  When somebody sets the variable to\n\"yes\", shouldn't we at least say \"Sorry, I do not understand\" and\npreferrably stop before spreading potential damage?  We'd surely end\nup doing something that the user who set the value to \"yes\" did not\nexpect.\n\nThere is something still missing?\n\n> +submodule.alternateErrorStrategy\n> +\tSpecifies how to treat errors with the alternates for a submodule\n> +\tas computed via `submodule.alternateLocation`. Possible values are\n> +\t`ignore`, `info`, `die`. Default is `die`.\n> +\n>  tag.forceSignAnnotated::\n>  \tA boolean to specify whether annotated tags created should be GPG signed.\n>  \tIf `--annotate` is specified on the command line, it takes\n> diff --git a/builtin/clone.c b/builtin/clone.c\n> index 0182665..404c5e8 100644\n> --- a/builtin/clone.c\n> +++ b/builtin/clone.c\n> @@ -947,6 +947,25 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n>  \t\telse\n>  \t\t\tfprintf(stderr, _(\"Cloning into '%s'...\\n\"), dir);\n>  \t}\n> +\n> +\tif (option_recursive) {\n> +\t\tif (option_required_reference.nr &&\n> +\t\t    option_optional_reference.nr)\n> +\t\t\tdie(_(\"clone --recursive is not compatible with \"\n> +\t\t\t      \"both --reference and --reference-if-able\"));\n> +\t\telse if (option_required_reference.nr) {\n> +\t\t\tstring_list_append(&option_config,\n> +\t\t\t\t\"submodule.alternateLocation=superproject\");\n> +\t\t\tstring_list_append(&option_config,\n> +\t\t\t\t\"submodule.alternateErrorStrategy=die\");\n> +\t\t} else if (option_optional_reference.nr) {\n> +\t\t\tstring_list_append(&option_config,\n> +\t\t\t\t\"submodule.alternateLocation=superproject\");\n> +\t\t\tstring_list_append(&option_config,\n> +\t\t\t\t\"submodule.alternateErrorStrategy=info\");\n> +\t\t}\n> +\t}\n> +\n>  \tinit_db(option_template, INIT_DB_QUIET);\n>  \twrite_config(&option_config);\n>  \n> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\n> index 7096848..f8f35c1 100644\n> --- a/builtin/submodule--helper.c\n> +++ b/builtin/submodule--helper.c\n> @@ -472,6 +472,96 @@ static int clone_submodule(const char *path, const char *gitdir, const char *url\n>  \treturn run_command(&cp);\n>  }\n>  \n> +struct submodule_alternate_setup {\n> +\tconst char *submodule_name;\n> +\tenum SUBMODULE_ALTERNATE_ERROR_MODE {\n> +\t\tSUBMODULE_ALTERNATE_ERROR_DIE,\n> +\t\tSUBMODULE_ALTERNATE_ERROR_INFO,\n> +\t\tSUBMODULE_ALTERNATE_ERROR_IGNORE\n> +\t} error_mode;\n> +\tstruct string_list *reference;\n> +};\n> +#define SUBMODULE_ALTERNATE_SETUP_INIT { NULL, \\\n> +\tSUBMODULE_ALTERNATE_ERROR_IGNORE, NULL }\n> +\n> +static int add_possible_reference_from_superproject(\n> +\t\tstruct alternate_object_database *alt, void *sas_cb)\n> +{\n> +\tstruct submodule_alternate_setup *sas = sas_cb;\n> +\n> +\t/* directory name, minus trailing slash */\n> +\tsize_t namelen = alt->name - alt->base - 1;\n> +\tstruct strbuf name = STRBUF_INIT;\n> +\tstrbuf_add(&name, alt->base, namelen);\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 */\n> +\tif (ends_with(name.buf, \".git/objects\")) {\n> +\t\tchar *sm_alternate;\n> +\t\tstruct strbuf sb = STRBUF_INIT;\n> +\t\tstruct strbuf err = STRBUF_INIT;\n> +\t\tstrbuf_add(&sb, name.buf, name.len - strlen(\"objects\"));\n> +\t\t/*\n> +\t\t * We need to end the new path with '/' to mark it as a dir,\n> +\t\t * otherwise a submodule name containing '/' will be broken\n> +\t\t * as the last part of a missing submodule reference would\n> +\t\t * be taken as a file name.\n> +\t\t */\n> +\t\tstrbuf_addf(&sb, \"modules/%s/\", sas->submodule_name);\n> +\n> +\t\tsm_alternate = compute_alternate_path(sb.buf, &err);\n> +\t\tif (sm_alternate) {\n> +\t\t\tstring_list_append(sas->reference, xstrdup(sb.buf));\n> +\t\t\tfree(sm_alternate);\n> +\t\t} else {\n> +\t\t\tswitch (sas->error_mode) {\n> +\t\t\tcase SUBMODULE_ALTERNATE_ERROR_DIE:\n> +\t\t\t\tdie(_(\"submodule '%s' cannot add alternate: %s\"),\n> +\t\t\t\t    sas->submodule_name, err.buf);\n> +\t\t\tcase SUBMODULE_ALTERNATE_ERROR_INFO:\n> +\t\t\t\tfprintf(stderr, _(\"submodule '%s' cannot add alternate: %s\"),\n> +\t\t\t\t\tsas->submodule_name, err.buf);\n> +\t\t\tcase SUBMODULE_ALTERNATE_ERROR_IGNORE:\n> +\t\t\t\t; /* nothing */\n> +\t\t\t}\n> +\t\t}\n> +\t\tstrbuf_release(&sb);\n> +\t}\n> +\n> +\tstrbuf_release(&name);\n> +\treturn 0;\n> +}\n> +\n> +static void prepare_possible_alternates(const char *sm_name,\n> +\t\tstruct string_list *reference)\n> +{\n> +\tchar *sm_alternate = NULL, *error_strategy = NULL;\n> +\tstruct submodule_alternate_setup sas = SUBMODULE_ALTERNATE_SETUP_INIT;\n> +\n> +\tgit_config_get_string(\"submodule.alternateLocation\", &sm_alternate);\n> +\tif (!sm_alternate)\n> +\t\treturn;\n> +\n> +\tgit_config_get_string(\"submodule.alternateErrorStrategy\", &error_strategy);\n> +\n> +\tif (!error_strategy)\n> +\t\terror_strategy = xstrdup(\"die\");\n> +\n> +\tsas.submodule_name = sm_name;\n> +\tsas.reference = reference;\n> +\tif (!strcmp(error_strategy, \"die\"))\n> +\t\tsas.error_mode = SUBMODULE_ALTERNATE_ERROR_DIE;\n> +\tif (!strcmp(error_strategy, \"info\"))\n> +\t\tsas.error_mode = SUBMODULE_ALTERNATE_ERROR_INFO;\n> +\tif (!strcmp(sm_alternate, \"superproject\"))\n> +\t\tforeach_alt_odb(add_possible_reference_from_superproject, &sas);\n> +\n> +\tfree(sm_alternate);\n> +\tfree(error_strategy);\n> +}\n> +\n>  static int module_clone(int argc, const char **argv, const char *prefix)\n>  {\n>  \tconst char *name = NULL, *url = NULL, *depth = NULL;\n> @@ -532,6 +622,9 @@ static int module_clone(int argc, const char **argv, const char *prefix)\n>  \tif (!file_exists(sm_gitdir)) {\n>  \t\tif (safe_create_leading_directories_const(sm_gitdir) < 0)\n>  \t\t\tdie(_(\"could not create directory '%s'\"), sm_gitdir);\n> +\n> +\t\tprepare_possible_alternates(name, &reference);\n> +\n>  \t\tif (clone_submodule(path, sm_gitdir, url, depth, &reference, quiet))\n>  \t\t\tdie(_(\"clone of '%s' into submodule path '%s' failed\"),\n>  \t\t\t    url, path);\n> diff --git a/t/t7408-submodule-reference.sh b/t/t7408-submodule-reference.sh\n> index dff47af..1c1e289 100755\n> --- a/t/t7408-submodule-reference.sh\n> +++ b/t/t7408-submodule-reference.sh\n> @@ -82,4 +82,47 @@ test_expect_success 'updating superproject keeps alternates' '\n>  \ttest_alternate_is_used super-clone/.git/modules/sub/objects/info/alternates super-clone/sub\n>  '\n>  \n> +test_expect_success 'submodules use alternates when cloning a superproject' '\n> +\ttest_when_finished \"rm -rf super-clone\" &&\n> +\tgit clone --reference super --recursive super super-clone &&\n> +\t(\n> +\t\tcd super-clone &&\n> +\t\t# test superproject has alternates setup correctly\n> +\t\ttest_alternate_is_used .git/objects/info/alternates . &&\n> +\t\t# test submodule has correct setup\n> +\t\ttest_alternate_is_used .git/modules/sub/objects/info/alternates sub\n> +\t)\n> +'\n> +\n> +test_expect_success 'missing submodule alternate fails clone and submodule update' '\n> +\ttest_when_finished \"rm -rf super-clone\" &&\n> +\tgit clone super super2 &&\n> +\ttest_must_fail git clone --recursive --reference super2 super2 super-clone &&\n> +\t(\n> +\t\tcd super-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 succeeds\n> +\t\ttest_must_fail git submodule update --init &&\n> +\t\t# and we have no alternates:\n> +\t\ttest_must_fail test_alternate_is_used .git/modules/sub/objects/info/alternates sub &&\n> +\t\ttest_must_fail test_path_is_file sub/file1\n> +\t)\n> +'\n> +\n> +test_expect_success 'ignoring missing submodule alternates passes clone and submodule update' '\n> +\ttest_when_finished \"rm -rf super-clone\" &&\n> +\tgit clone --reference-if-able super2 --recursive super2 super-clone &&\n> +\t(\n> +\t\tcd super-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 succeeds\n> +\t\tgit submodule update --init &&\n> +\t\t# and we have no alternates:\n> +\t\ttest_must_fail test_alternate_is_used .git/modules/sub/objects/info/alternates sub &&\n> +\t\ttest_path_is_file sub/file1\n> +\t)\n> +'\n> +\n>  test_done\n"},{"id":"299582","messageId":"CAGZ79kaBX8UX4Hbq3Ff6DeTT6jfZTHcCXPYGOaDyMQTuWUvbyg@mail.gmail.com","threadId":"43873","inReplyTo":"xmqq8tvv82pg.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCHv7 8/8] clone: recursive and reference option triggers submodule alternates","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-08-17T22:18:40Z","receivedAt":"2016-08-17T22:18:57Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, Aug 17, 2016 at 3:12 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Stefan Beller <sbeller@google.com> writes:\n>\n>>  Added a default for alternateErrorStrategy and hence fixed the null pointer\n>>  for error_strategy in prepare_possible_alternates(),\n>\n> Looks much better.\n>\n>> +submodule.alternateLocation::\n>> +     Specifies how the submodules obtain alternates when submodules are\n>> +     cloned. Possible values are `no`, `superproject`.\n>> +     By default `no` is assumed, which doesn't add references. When the\n>> +     value is set to `superproject` the submodule to be cloned computes\n>> +     its alternates location relative to the superprojects alternate.\n>\n> I am not seeing a code that handles 'no' and any other string that\n> is not 'superproject' differently, though.\n>\n> I can see that \"clone\" has codepath that writes 'superproject' to\n> the variable, but the only thing that seems to care what value the\n> variable is set to checks \"no\".  When somebody sets the variable to\n> \"yes\", shouldn't we at least say \"Sorry, I do not understand\" and\n> preferrably stop before spreading potential damage?  We'd surely end\n> up doing something that the user who set the value to \"yes\" did not\n> expect.\n>\n> There is something still missing?\n>\n\n>> +static void prepare_possible_alternates(const char *sm_name,\n>> +             struct string_list *reference)\n>> +{\n...\n>> +     sas.submodule_name = sm_name;\n>> +     sas.reference = reference;\n>> +     if (!strcmp(error_strategy, \"die\"))\n>> +             sas.error_mode = SUBMODULE_ALTERNATE_ERROR_DIE;\n>> +     if (!strcmp(error_strategy, \"info\"))\n>> +             sas.error_mode = SUBMODULE_ALTERNATE_ERROR_INFO;\n\n(see below first)\nHere goes the same for alternateErrorStrategy\n\n>> +     if (!strcmp(sm_alternate, \"superproject\"))\n>> +             foreach_alt_odb(add_possible_reference_from_superproject, &sas);\n\nhere is where we would add\n\n    else if (!strcmp(sm_alternate, \"no\")\n        ; /* do nothing */\n    else\n        die(_(\"What's wrong with you?\"));\n\nInitially I did not add that as I considered it not relevant. But I\nguess it helps as a typo checker as well and it is more comforting\nif a wrong value yields an error. Also it is consistent with the rest of\noptions.\n\nThanks again,\nStefan\n"}]}