{"thread":{"id":"43790","subject":"[PATCHv4 0/6] git clone: Marry --recursive and --reference","startedAt":"2016-08-11T23:14:15Z","lastAt":"2016-08-17T21:42:02Z","messageCount":18,"participants":["Stefan Beller","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"298804","messageId":"20160811231405.17318-1-sbeller@google.com","threadId":"43790","inReplyTo":null,"subject":"[PATCHv4 0/6] git clone: Marry --recursive and --reference","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-08-11T23:13:57Z","receivedAt":"2016-08-11T23:14:15Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"v4:\nThanks to Junios critial questions regarding the design, I took a step back\nto look at the bigger picture, again.\n\nnew patches:\n  clone: factor out checking for an alternate path\n  clone: recursive and reference option triggers submodule alternates\n  \nThe last patch redesigns completely how we approach the problem.\nNow there are no new command line options (that relate to the problem\nof marrying --recursive and --reference), but instead we communicate\neverything via configuration options to have a lasting effect (i.e.\nsubmodule update remembers the decision of the initial setup)\n\nThanks,\nStefan\n\nv3:\n\nThanks to Junios critial questions regarding the design, I took a step back\nto look at the bigger picture. \n\n--super-reference sounds confusing. (what is the super referring to?)\nSo drop that approach.\n\nInstead we'll compute where the reference might be in the superproject scope\nand ask the submodule clone operation to consider an optional reference.\nIf the referenced alternate is not there, we'll just warn about it and\ncarry on.\n\n\n* fixed the style in patch 2.\n\n* fixed another bug in the last patch, that is unrelated, but would have helped\n  me a lot.\n\nThanks,\nStefan\n\n Documentation/git-clone.txt    |   9 ++++++++-\n builtin/clone.c                |  36 ++++++++++++++++++++++++++++--------\n builtin/submodule--helper.c    | 105 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-----------------\n git-submodule.sh               |   2 +-\n t/t7408-submodule-reference.sh | 162 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++----------------------------------------------------------------------\n 5 files changed, 217 insertions(+), 97 deletions(-)\n\nv2:\n * fixed the p1,2 cleanups\n * added documentation to patches 5,6\n * improved commit message in v4\n \n Thanks,\n Stefan\n \nv1:\n \n Currently when cloning a superproject with --recursive and --reference\n only the superproject learns about its alternates. The submodules are\n cloned independently, which may incur lots of network costs.\n \n Assume that the reference repository has the submodules at the same\n paths as the to-be-cloned submodule and try to setup alternates from\n there.\n \n Some submodules in the referenced superproject may not be there, \n (they are just not initialized/cloned/checked out), which yields\n an error for now. In future work we may want to soften the alternate\n check and not die in the clone when one of the given alternates doesn't\n exist.\n \n patch 1,2 are modernizing style of t7408, \n patches 3,4 are not strictly necessary, but I think it is a good thing\n to not leave the submodule related C code in a crippled state (i.e.\n allowing only one reference). The shell code would also need this update,\n but it looked ugly to me, so I postpone it until more of the submodule code\n is written in C. \n \n Thanks,\n Stefan \n\nStefan Beller (6):\n  t7408: modernize style\n  t7408: merge short tests, factor out testing method\n  submodule--helper module-clone: allow multiple references\n  submodule--helper update-clone: allow multiple references\n  submodule update: add super-reference flag\n  clone: reference flag is used for submodules as well\n\n builtin/clone.c                |  22 ++++--\n builtin/submodule--helper.c    |  45 ++++++++----\n git-submodule.sh               |  12 +++-\n t/t7408-submodule-reference.sh | 153 +++++++++++++++++++++++------------------\n 4 files changed, 147 insertions(+), 85 deletions(-)\n\n-- \n2.9.2.572.g9d9644e.dirty\n\n"},{"id":"298805","messageId":"20160811231405.17318-2-sbeller@google.com","threadId":"43790","inReplyTo":"20160811231405.17318-1-sbeller@google.com","subject":"[PATCHv4 1/8] t7408: modernize style","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-08-11T23:13:58Z","receivedAt":"2016-08-11T23:14:18Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"No functional change intended. This commit only changes formatting\nto the style we recently use, e.g. starting the body of a test with a\nsingle quote on the same line as the header, and then having the test\nindented in the following lines.\n\nWhenever we change directories, we do that in subshells.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n t/t7408-submodule-reference.sh | 140 +++++++++++++++++++++--------------------\n 1 file changed, 71 insertions(+), 69 deletions(-)\n\ndiff --git a/t/t7408-submodule-reference.sh b/t/t7408-submodule-reference.sh\nindex eaea19b..b84c6748 100755\n--- a/t/t7408-submodule-reference.sh\n+++ b/t/t7408-submodule-reference.sh\n@@ -8,74 +8,76 @@ test_description='test clone --reference'\n \n base_dir=$(pwd)\n \n-U=$base_dir/UPLOAD_LOG\n-\n-test_expect_success 'preparing first repository' \\\n-'test_create_repo A && cd A &&\n-echo first > file1 &&\n-git add file1 &&\n-git commit -m A-initial'\n-\n-cd \"$base_dir\"\n-\n-test_expect_success 'preparing second repository' \\\n-'git clone A B && cd B &&\n-echo second > file2 &&\n-git add file2 &&\n-git commit -m B-addition &&\n-git repack -a -d &&\n-git prune'\n-\n-cd \"$base_dir\"\n-\n-test_expect_success 'preparing superproject' \\\n-'test_create_repo super && cd super &&\n-echo file > file &&\n-git add file &&\n-git commit -m B-super-initial'\n-\n-cd \"$base_dir\"\n-\n-test_expect_success 'submodule add --reference' \\\n-'cd super && git submodule add --reference ../B \"file://$base_dir/A\" sub &&\n-git commit -m B-super-added'\n-\n-cd \"$base_dir\"\n-\n-test_expect_success 'after add: existence of info/alternates' \\\n-'test_line_count = 1 super/.git/modules/sub/objects/info/alternates'\n-\n-cd \"$base_dir\"\n-\n-test_expect_success 'that reference gets used with add' \\\n-'cd super/sub &&\n-echo \"0 objects, 0 kilobytes\" > expected &&\n-git count-objects > current &&\n-diff expected current'\n-\n-cd \"$base_dir\"\n-\n-test_expect_success 'cloning superproject' \\\n-'git clone super super-clone'\n-\n-cd \"$base_dir\"\n-\n-test_expect_success 'update with reference' \\\n-'cd super-clone && git submodule update --init --reference ../B'\n-\n-cd \"$base_dir\"\n-\n-test_expect_success 'after update: existence of info/alternates' \\\n-'test_line_count = 1 super-clone/.git/modules/sub/objects/info/alternates'\n-\n-cd \"$base_dir\"\n-\n-test_expect_success 'that reference gets used with update' \\\n-'cd super-clone/sub &&\n-echo \"0 objects, 0 kilobytes\" > expected &&\n-git count-objects > current &&\n-diff expected current'\n-\n-cd \"$base_dir\"\n+test_expect_success 'preparing first repository' '\n+\ttest_create_repo A &&\n+\t(\n+\t\tcd A &&\n+\t\techo first >file1 &&\n+\t\tgit add file1 &&\n+\t\tgit commit -m A-initial\n+\t)\n+'\n+\n+test_expect_success 'preparing second repository' '\n+\tgit clone A B &&\n+\t(\n+\t\tcd B &&\n+\t\techo second >file2 &&\n+\t\tgit add file2 &&\n+\t\tgit commit -m B-addition &&\n+\t\tgit repack -a -d &&\n+\t\tgit prune\n+\t)\n+'\n+\n+test_expect_success 'preparing superproject' '\n+\ttest_create_repo super &&\n+\t(\n+\t\tcd super &&\n+\t\techo file >file &&\n+\t\tgit add file &&\n+\t\tgit commit -m B-super-initial\n+\t)\n+'\n+\n+test_expect_success 'submodule add --reference' '\n+\t(\n+\t\tcd super &&\n+\t\tgit submodule add --reference ../B \"file://$base_dir/A\" sub &&\n+\t\tgit commit -m B-super-added\n+\t)\n+'\n+\n+test_expect_success 'after add: existence of info/alternates' '\n+\ttest_line_count = 1 super/.git/modules/sub/objects/info/alternates\n+'\n+\n+test_expect_success 'that reference gets used with add' '\n+\t(\n+\t\tcd super/sub &&\n+\t\techo \"0 objects, 0 kilobytes\" >expected &&\n+\t\tgit count-objects >current &&\n+\t\tdiff expected current\n+\t)\n+'\n+\n+test_expect_success 'cloning superproject' '\n+\tgit clone super super-clone\n+'\n+\n+test_expect_success 'update with reference' '\n+\tcd super-clone && git submodule update --init --reference ../B\n+'\n+\n+test_expect_success 'after update: existence of info/alternates' '\n+\ttest_line_count = 1 super-clone/.git/modules/sub/objects/info/alternates\n+'\n+\n+test_expect_success 'that reference gets used with update' '\n+\tcd super-clone/sub &&\n+\techo \"0 objects, 0 kilobytes\" >expected &&\n+\tgit count-objects >current &&\n+\tdiff expected current\n+'\n \n test_done\n-- \n2.9.2.737.g4a14654\n\n"},{"id":"298806","messageId":"20160811231405.17318-3-sbeller@google.com","threadId":"43790","inReplyTo":"20160811231405.17318-1-sbeller@google.com","subject":"[PATCHv4 2/8] t7408: merge short tests, factor out testing method","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-08-11T23:13:59Z","receivedAt":"2016-08-11T23:14:25Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Tests consisting of one line each can be consolidated to have fewer tests\nto run as well as fewer lines of code.\n\nWhen having just a few git commands, do not create a new shell but\nuse the -C flag in Git to execute in the correct directory.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n t/t7408-submodule-reference.sh | 48 ++++++++++++++++++++++--------------------\n 1 file changed, 25 insertions(+), 23 deletions(-)\n\ndiff --git a/t/t7408-submodule-reference.sh b/t/t7408-submodule-reference.sh\nindex b84c6748..dff47af 100755\n--- a/t/t7408-submodule-reference.sh\n+++ b/t/t7408-submodule-reference.sh\n@@ -8,6 +8,15 @@ test_description='test clone --reference'\n \n base_dir=$(pwd)\n \n+test_alternate_is_used () {\n+\talternates_file=\"$1\" &&\n+\tworking_dir=\"$2\" &&\n+\ttest_line_count = 1 \"$alternates_file\" &&\n+\techo \"0 objects, 0 kilobytes\" >expect &&\n+\tgit -C \"$working_dir\" count-objects >actual &&\n+\ttest_cmp expect actual\n+}\n+\n test_expect_success 'preparing first repository' '\n \ttest_create_repo A &&\n \t(\n@@ -40,16 +49,14 @@ test_expect_success 'preparing superproject' '\n \t)\n '\n \n-test_expect_success 'submodule add --reference' '\n+test_expect_success 'submodule add --reference uses alternates' '\n \t(\n \t\tcd super &&\n \t\tgit submodule add --reference ../B \"file://$base_dir/A\" sub &&\n-\t\tgit commit -m B-super-added\n-\t)\n-'\n-\n-test_expect_success 'after add: existence of info/alternates' '\n-\ttest_line_count = 1 super/.git/modules/sub/objects/info/alternates\n+\t\tgit commit -m B-super-added &&\n+\t\tgit repack -ad\n+\t) &&\n+\ttest_alternate_is_used super/.git/modules/sub/objects/info/alternates super/sub\n '\n \n test_expect_success 'that reference gets used with add' '\n@@ -61,23 +68,18 @@ test_expect_success 'that reference gets used with add' '\n \t)\n '\n \n-test_expect_success 'cloning superproject' '\n-\tgit clone super super-clone\n-'\n-\n-test_expect_success 'update with reference' '\n-\tcd super-clone && git submodule update --init --reference ../B\n-'\n-\n-test_expect_success 'after update: existence of info/alternates' '\n-\ttest_line_count = 1 super-clone/.git/modules/sub/objects/info/alternates\n-'\n+# The tests up to this point, and repositories created by them\n+# (A, B, super and super/sub), are about setting up the stage\n+# for subsequent tests and meant to be kept throughout the\n+# remainder of the test.\n+# Tests from here on, if they create their own test repository,\n+# are expected to clean after themselves.\n \n-test_expect_success 'that reference gets used with update' '\n-\tcd super-clone/sub &&\n-\techo \"0 objects, 0 kilobytes\" >expected &&\n-\tgit count-objects >current &&\n-\tdiff expected current\n+test_expect_success 'updating superproject keeps alternates' '\n+\ttest_when_finished \"rm -rf super-clone\" &&\n+\tgit clone super super-clone &&\n+\tgit -C super-clone submodule update --init --reference ../B &&\n+\ttest_alternate_is_used super-clone/.git/modules/sub/objects/info/alternates super-clone/sub\n '\n \n test_done\n-- \n2.9.2.737.g4a14654\n\n"},{"id":"298807","messageId":"20160811231405.17318-4-sbeller@google.com","threadId":"43790","inReplyTo":"20160811231405.17318-1-sbeller@google.com","subject":"[PATCHv4 3/8] submodule--helper module-clone: allow multiple references","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-08-11T23:14:00Z","receivedAt":"2016-08-11T23:14:27Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Allow users to pass in multiple references, just as clone accepts multiple\nreferences as well.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n builtin/submodule--helper.c | 20 ++++++++++++--------\n 1 file changed, 12 insertions(+), 8 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex b09632e..db9270e 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -442,7 +442,7 @@ static int module_name(int argc, const char **argv, const char *prefix)\n }\n \n static int clone_submodule(const char *path, const char *gitdir, const char *url,\n-\t\t\t   const char *depth, const char *reference, int quiet)\n+\t\t\t   const char *depth, struct string_list *reference, int quiet)\n {\n \tstruct child_process cp;\n \tchild_process_init(&cp);\n@@ -453,8 +453,12 @@ static int clone_submodule(const char *path, const char *gitdir, const char *url\n \t\targv_array_push(&cp.args, \"--quiet\");\n \tif (depth && *depth)\n \t\targv_array_pushl(&cp.args, \"--depth\", depth, NULL);\n-\tif (reference && *reference)\n-\t\targv_array_pushl(&cp.args, \"--reference\", reference, NULL);\n+\tif (reference->nr) {\n+\t\tstruct string_list_item *item;\n+\t\tfor_each_string_list_item(item, reference)\n+\t\t\targv_array_pushl(&cp.args, \"--reference\",\n+\t\t\t\t\t item->string, NULL);\n+\t}\n \tif (gitdir && *gitdir)\n \t\targv_array_pushl(&cp.args, \"--separate-git-dir\", gitdir, NULL);\n \n@@ -470,13 +474,13 @@ static int clone_submodule(const char *path, const char *gitdir, const char *url\n \n static int module_clone(int argc, const char **argv, const char *prefix)\n {\n-\tconst char *name = NULL, *url = NULL;\n-\tconst char *reference = NULL, *depth = NULL;\n+\tconst char *name = NULL, *url = NULL, *depth = NULL;\n \tint quiet = 0;\n \tFILE *submodule_dot_git;\n \tchar *p, *path = NULL, *sm_gitdir;\n \tstruct strbuf rel_path = STRBUF_INIT;\n \tstruct strbuf sb = STRBUF_INIT;\n+\tstruct string_list reference = STRING_LIST_INIT_NODUP;\n \n \tstruct option module_clone_options[] = {\n \t\tOPT_STRING(0, \"prefix\", &prefix,\n@@ -491,8 +495,8 @@ static int module_clone(int argc, const char **argv, const char *prefix)\n \t\tOPT_STRING(0, \"url\", &url,\n \t\t\t   N_(\"string\"),\n \t\t\t   N_(\"url where to clone the submodule from\")),\n-\t\tOPT_STRING(0, \"reference\", &reference,\n-\t\t\t   N_(\"string\"),\n+\t\tOPT_STRING_LIST(0, \"reference\", &reference,\n+\t\t\t   N_(\"repo\"),\n \t\t\t   N_(\"reference repository\")),\n \t\tOPT_STRING(0, \"depth\", &depth,\n \t\t\t   N_(\"string\"),\n@@ -528,7 +532,7 @@ 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-\t\tif (clone_submodule(path, sm_gitdir, url, depth, reference, quiet))\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 \t} else {\n-- \n2.9.2.737.g4a14654\n\n"},{"id":"298808","messageId":"20160811231405.17318-5-sbeller@google.com","threadId":"43790","inReplyTo":"20160811231405.17318-1-sbeller@google.com","subject":"[PATCHv4 4/8] submodule--helper update-clone: allow multiple references","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-08-11T23:14:01Z","receivedAt":"2016-08-11T23:14:29Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Allow the user to pass in multiple references to update_clone.\nCurrently this is only internal API, but once the shell script is\nreplaced by a C version, this is needed.\n\nThis fixes an API bug between the shell script and the helper.\nCurrently the helper accepts \"--reference\" \"--reference=foo\"\nas a OPT_STRING whose value happens to be \"--reference=foo\", and\nthen uses\n\n        if (suc->reference)\n                argv_array_push(&child->args, suc->reference)\n\nwhere suc->reference _is_ \"--reference=foo\" when invoking the\nunderlying \"git clone\", it cancels out.\n\nWith this change we omit one of the \"--reference\" arguments when\npassing references from the shell script to the helper.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n builtin/submodule--helper.c | 14 +++++++++-----\n git-submodule.sh            |  2 +-\n 2 files changed, 10 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex db9270e..a10a777 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -584,7 +584,7 @@ struct submodule_update_clone {\n \t/* configuration parameters which are passed on to the children */\n \tint quiet;\n \tint recommend_shallow;\n-\tconst char *reference;\n+\tstruct string_list references;\n \tconst char *depth;\n \tconst char *recursive_prefix;\n \tconst char *prefix;\n@@ -600,7 +600,8 @@ struct submodule_update_clone {\n \tint failed_clones_nr, failed_clones_alloc;\n };\n #define SUBMODULE_UPDATE_CLONE_INIT {0, MODULE_LIST_INIT, 0, \\\n-\tSUBMODULE_UPDATE_STRATEGY_INIT, 0, -1, NULL, NULL, NULL, NULL, \\\n+\tSUBMODULE_UPDATE_STRATEGY_INIT, 0, -1, STRING_LIST_INIT_DUP, \\\n+\tNULL, NULL, NULL, \\\n \tSTRING_LIST_INIT_DUP, 0, NULL, 0, 0}\n \n \n@@ -710,8 +711,11 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce,\n \targv_array_pushl(&child->args, \"--path\", sub->path, NULL);\n \targv_array_pushl(&child->args, \"--name\", sub->name, NULL);\n \targv_array_pushl(&child->args, \"--url\", url, NULL);\n-\tif (suc->reference)\n-\t\targv_array_push(&child->args, suc->reference);\n+\tif (suc->references.nr) {\n+\t\tstruct string_list_item *item;\n+\t\tfor_each_string_list_item(item, &suc->references)\n+\t\t\targv_array_pushl(&child->args, \"--reference\", item->string, NULL);\n+\t}\n \tif (suc->depth)\n \t\targv_array_push(&child->args, suc->depth);\n \n@@ -830,7 +834,7 @@ static int update_clone(int argc, const char **argv, const char *prefix)\n \t\tOPT_STRING(0, \"update\", &update,\n \t\t\t   N_(\"string\"),\n \t\t\t   N_(\"rebase, merge, checkout or none\")),\n-\t\tOPT_STRING(0, \"reference\", &suc.reference, N_(\"repo\"),\n+\t\tOPT_STRING_LIST(0, \"reference\", &suc.references, N_(\"repo\"),\n \t\t\t   N_(\"reference repository\")),\n \t\tOPT_STRING(0, \"depth\", &suc.depth, \"<depth>\",\n \t\t\t   N_(\"Create a shallow clone truncated to the \"\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex b57f87d..a1cc71b 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -576,7 +576,7 @@ cmd_update()\n \t\t${wt_prefix:+--prefix \"$wt_prefix\"} \\\n \t\t${prefix:+--recursive-prefix \"$prefix\"} \\\n \t\t${update:+--update \"$update\"} \\\n-\t\t${reference:+--reference \"$reference\"} \\\n+\t\t${reference:+\"$reference\"} \\\n \t\t${depth:+--depth \"$depth\"} \\\n \t\t${recommend_shallow:+\"$recommend_shallow\"} \\\n \t\t${jobs:+$jobs} \\\n-- \n2.9.2.737.g4a14654\n\n"},{"id":"298809","messageId":"20160811231405.17318-6-sbeller@google.com","threadId":"43790","inReplyTo":"20160811231405.17318-1-sbeller@google.com","subject":"[PATCHv4 5/8] clone: factor out checking for an alternate path","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-08-11T23:14:02Z","receivedAt":"2016-08-11T23:14:34Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"In a later patch we want to determine if a path is suitable as an\nalternate from other commands than builtin/clone. Move the checking\nfunctionality of `add_one_reference` to `compute_alternate_path` that is\ndefined in cache.h.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n builtin/clone.c | 42 ++++++--------------------------\n cache.h         |  1 +\n sha1_file.c     | 74 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++\n 3 files changed, 82 insertions(+), 35 deletions(-)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex f044a8c..24b17539 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -282,44 +282,16 @@ static void strip_trailing_slashes(char *dir)\n \n static int add_one_reference(struct string_list_item *item, void *cb_data)\n {\n-\tchar *ref_git;\n-\tconst char *repo;\n-\tstruct strbuf alternate = STRBUF_INIT;\n-\n-\t/* Beware: read_gitfile(), real_path() and mkpath() return static buffer */\n-\tref_git = xstrdup(real_path(item->string));\n-\n-\trepo = read_gitfile(ref_git);\n-\tif (!repo)\n-\t\trepo = read_gitfile(mkpath(\"%s/.git\", ref_git));\n-\tif (repo) {\n-\t\tfree(ref_git);\n-\t\tref_git = xstrdup(repo);\n-\t}\n-\n-\tif (!repo && is_directory(mkpath(\"%s/.git/objects\", ref_git))) {\n-\t\tchar *ref_git_git = mkpathdup(\"%s/.git\", ref_git);\n-\t\tfree(ref_git);\n-\t\tref_git = ref_git_git;\n-\t} else if (!is_directory(mkpath(\"%s/objects\", ref_git))) {\n-\t\tstruct strbuf sb = STRBUF_INIT;\n-\t\tif (get_common_dir(&sb, ref_git))\n-\t\t\tdie(_(\"reference repository '%s' as a linked checkout is not supported yet.\"),\n-\t\t\t    item->string);\n-\t\tdie(_(\"reference repository '%s' is not a local repository.\"),\n-\t\t    item->string);\n-\t}\n+\tstruct strbuf sb = STRBUF_INIT;\n+\tchar *ref_git = compute_alternate_path(item->string, &sb);\n \n-\tif (!access(mkpath(\"%s/shallow\", ref_git), F_OK))\n-\t\tdie(_(\"reference repository '%s' is shallow\"), item->string);\n+\tif (!ref_git)\n+\t\tdie(\"%s\", sb.buf);\n \n-\tif (!access(mkpath(\"%s/info/grafts\", ref_git), F_OK))\n-\t\tdie(_(\"reference repository '%s' is grafted\"), item->string);\n+\tstrbuf_addf(&sb, \"%s/objects\", ref_git);\n+\tadd_to_alternates_file(sb.buf);\n \n-\tstrbuf_addf(&alternate, \"%s/objects\", ref_git);\n-\tadd_to_alternates_file(alternate.buf);\n-\tstrbuf_release(&alternate);\n-\tfree(ref_git);\n+\tstrbuf_release(&sb);\n \treturn 0;\n }\n \ndiff --git a/cache.h b/cache.h\nindex 95a0bd3..35f41f7 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1344,6 +1344,7 @@ extern struct alternate_object_database {\n } *alt_odb_list;\n extern void prepare_alt_odb(void);\n extern void read_info_alternates(const char * relative_base, int depth);\n+extern char *compute_alternate_path(const char *path, struct strbuf *err);\n extern void add_to_alternates_file(const char *reference);\n typedef int alt_odb_fn(struct alternate_object_database *, void *);\n extern int foreach_alt_odb(alt_odb_fn, void*);\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 02940f1..7351d8c 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -418,6 +418,80 @@ void add_to_alternates_file(const char *reference)\n \tfree(alts);\n }\n \n+/*\n+ * Compute the exact path an alternate is at and returns it. In case of\n+ * error NULL is returned and the human readable error is added to `err`\n+ * `path` may be relative and should point to $GITDIR.\n+ * `err` must not be null.\n+ */\n+char *compute_alternate_path(const char *path, struct strbuf *err)\n+{\n+\tchar *ref_git = NULL;\n+\tconst char *repo, *ref_git_s;\n+\tstruct strbuf err_buf = STRBUF_INIT;\n+\n+\tref_git_s = real_path_if_valid(path);\n+\tif (!ref_git_s) {\n+\t\tstrbuf_addf(&err_buf, _(\"path '%s' does not exist\"), path);\n+\t\tgoto out;\n+\t} else\n+\t\t/*\n+\t\t * Beware: read_gitfile(), real_path() and mkpath()\n+\t\t * return static buffer\n+\t\t */\n+\t\tref_git = xstrdup(ref_git_s);\n+\n+\trepo = read_gitfile(ref_git);\n+\tif (!repo)\n+\t\trepo = read_gitfile(mkpath(\"%s/.git\", ref_git));\n+\tif (repo) {\n+\t\tfree(ref_git);\n+\t\tref_git = xstrdup(repo);\n+\t}\n+\n+\tif (!repo && is_directory(mkpath(\"%s/.git/objects\", ref_git))) {\n+\t\tchar *ref_git_git = mkpathdup(\"%s/.git\", ref_git);\n+\t\tfree(ref_git);\n+\t\tref_git = ref_git_git;\n+\t} else if (!is_directory(mkpath(\"%s/objects\", ref_git))) {\n+\t\tstruct strbuf sb = STRBUF_INIT;\n+\t\tif (get_common_dir(&sb, ref_git)) {\n+\t\t\tstrbuf_addf(&err_buf,\n+\t\t\t\t    _(\"reference repository '%s' as a linked \"\n+\t\t\t\t      \"checkout is not supported yet.\"),\n+\t\t\t\t    path);\n+\t\t\tgoto out;\n+\t\t}\n+\n+\t\tstrbuf_addf(&err_buf, _(\"reference repository '%s' is not a \"\n+\t\t\t\t\t\"local repository.\"), path);\n+\t\tgoto out;\n+\t}\n+\n+\tif (!access(mkpath(\"%s/shallow\", ref_git), F_OK)) {\n+\t\tstrbuf_addf(&err_buf, _(\"reference repository '%s' is shallow\"),\n+\t\t\t    path);\n+\t\tgoto out;\n+\t}\n+\n+\tif (!access(mkpath(\"%s/info/grafts\", ref_git), F_OK)) {\n+\t\tstrbuf_addf(&err_buf,\n+\t\t\t    _(\"reference repository '%s' is grafted\"),\n+\t\t\t    path);\n+\t\tgoto out;\n+\t}\n+\n+out:\n+\tif (err_buf.len) {\n+\t\tstrbuf_addbuf(err, &err_buf);\n+\t\tfree(ref_git);\n+\t\tref_git = NULL;\n+\t}\n+\n+\tstrbuf_release(&err_buf);\n+\treturn ref_git;\n+}\n+\n int foreach_alt_odb(alt_odb_fn fn, void *cb)\n {\n \tstruct alternate_object_database *ent;\n-- \n2.9.2.737.g4a14654\n\n"},{"id":"298810","messageId":"20160811231405.17318-8-sbeller@google.com","threadId":"43790","inReplyTo":"20160811231405.17318-1-sbeller@google.com","subject":"[PATCHv4 7/8] clone: implement optional references","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-08-11T23:14:04Z","receivedAt":"2016-08-11T23:14:37Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"In a later patch we want to try to create alternates for submodules,\nbut they might not exist in the referenced superproject. So add a way\nto skip the non existing references and report them.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n Documentation/git-clone.txt |  5 ++++-\n builtin/clone.c             | 29 ++++++++++++++++++++++-------\n 2 files changed, 26 insertions(+), 8 deletions(-)\n\ndiff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt\nindex ec41d3d..e316c4b 100644\n--- a/Documentation/git-clone.txt\n+++ b/Documentation/git-clone.txt\n@@ -90,13 +90,16 @@ If you want to break the dependency of a repository cloned with `-s` on\n its source repository, you can simply run `git repack -a` to copy all\n objects from the source repository into a pack in the cloned repository.\n \n---reference <repository>::\n+--reference[-if-able] <repository>::\n \tIf the reference repository is on the local machine,\n \tautomatically setup `.git/objects/info/alternates` to\n \tobtain objects from the reference repository.  Using\n \tan already existing repository as an alternate will\n \trequire fewer objects to be copied from the repository\n \tbeing cloned, reducing network and local storage costs.\n+\tWhen using the `--reference-if-able`, a non existing\n+\tdirectory is skipped with a warning instead of aborting\n+\tthe clone.\n +\n *NOTE*: see the NOTE for the `--shared` option, and also the\n `--dissociate` option.\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex 7ccbdef..fdb2ca9 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -51,6 +51,7 @@ static int option_progress = -1;\n static enum transport_family family;\n static struct string_list option_config = STRING_LIST_INIT_NODUP;\n static struct string_list option_required_reference = STRING_LIST_INIT_NODUP;\n+static struct string_list option_optional_reference = STRING_LIST_INIT_NODUP;\n static int option_dissociate;\n static int max_jobs = -1;\n \n@@ -81,6 +82,8 @@ static struct option builtin_clone_options[] = {\n \t\t   N_(\"directory from which templates will be used\")),\n \tOPT_STRING_LIST(0, \"reference\", &option_required_reference, N_(\"repo\"),\n \t\t\tN_(\"reference repository\")),\n+\tOPT_STRING_LIST(0, \"reference-if-able\", &option_optional_reference,\n+\t\t\tN_(\"repo\"), N_(\"reference repository\")),\n \tOPT_BOOL(0, \"dissociate\", &option_dissociate,\n \t\t N_(\"use --reference only while cloning\")),\n \tOPT_STRING('o', \"origin\", &option_origin, N_(\"name\"),\n@@ -283,13 +286,20 @@ static void strip_trailing_slashes(char *dir)\n static int add_one_reference(struct string_list_item *item, void *cb_data)\n {\n \tstruct strbuf sb = STRBUF_INIT;\n+\tint *required = cb_data;\n \tchar *ref_git = compute_alternate_path(item->string, &sb);\n \n-\tif (!ref_git)\n-\t\tdie(\"%s\", sb.buf);\n-\n-\tstrbuf_addf(&sb, \"%s/objects\", ref_git);\n-\tadd_to_alternates_file(sb.buf);\n+\tif (!ref_git) {\n+\t\tif (*required)\n+\t\t\tdie(\"%s\", sb.buf);\n+\t\telse\n+\t\t\tfprintf(stderr,\n+\t\t\t\t_(\"info: Could not add alternate for '%s': %s\\n\"),\n+\t\t\t\titem->string, sb.buf);\n+\t} else {\n+\t\tstrbuf_addf(&sb, \"%s/objects\", ref_git);\n+\t\tadd_to_alternates_file(sb.buf);\n+\t}\n \n \tstrbuf_release(&sb);\n \treturn 0;\n@@ -297,7 +307,12 @@ static int add_one_reference(struct string_list_item *item, void *cb_data)\n \n static void setup_reference(void)\n {\n-\tfor_each_string_list(&option_required_reference, add_one_reference, NULL);\n+\tint required = 1;\n+\tfor_each_string_list(&option_required_reference,\n+\t\t\t     add_one_reference, &required);\n+\trequired = 0;\n+\tfor_each_string_list(&option_optional_reference,\n+\t\t\t     add_one_reference, &required);\n }\n \n static void copy_alternates(struct strbuf *src, struct strbuf *dst,\n@@ -949,7 +964,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \tgit_config_set(key.buf, repo);\n \tstrbuf_reset(&key);\n \n-\tif (option_required_reference.nr)\n+\tif (option_required_reference.nr || option_optional_reference.nr)\n \t\tsetup_reference();\n \n \tfetch_pattern = value.buf;\n-- \n2.9.2.737.g4a14654\n\n"},{"id":"298811","messageId":"20160811231405.17318-7-sbeller@google.com","threadId":"43790","inReplyTo":"20160811231405.17318-1-sbeller@google.com","subject":"[PATCHv4 6/8] clone: clarify option_reference as required","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-08-11T23:14:03Z","receivedAt":"2016-08-11T23:14:39Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"In the next patch we introduce optional references; To better distinguish\nbetween optional and required references we rename the variable.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n builtin/clone.c | 8 ++++----\n 1 file changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex 24b17539..7ccbdef 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -50,7 +50,7 @@ static int option_verbosity;\n static int option_progress = -1;\n static enum transport_family family;\n static struct string_list option_config = STRING_LIST_INIT_NODUP;\n-static struct string_list option_reference = STRING_LIST_INIT_NODUP;\n+static struct string_list option_required_reference = STRING_LIST_INIT_NODUP;\n static int option_dissociate;\n static int max_jobs = -1;\n \n@@ -79,7 +79,7 @@ static struct option builtin_clone_options[] = {\n \t\t    N_(\"number of submodules cloned in parallel\")),\n \tOPT_STRING(0, \"template\", &option_template, N_(\"template-directory\"),\n \t\t   N_(\"directory from which templates will be used\")),\n-\tOPT_STRING_LIST(0, \"reference\", &option_reference, N_(\"repo\"),\n+\tOPT_STRING_LIST(0, \"reference\", &option_required_reference, N_(\"repo\"),\n \t\t\tN_(\"reference repository\")),\n \tOPT_BOOL(0, \"dissociate\", &option_dissociate,\n \t\t N_(\"use --reference only while cloning\")),\n@@ -297,7 +297,7 @@ static int add_one_reference(struct string_list_item *item, void *cb_data)\n \n static void setup_reference(void)\n {\n-\tfor_each_string_list(&option_reference, add_one_reference, NULL);\n+\tfor_each_string_list(&option_required_reference, add_one_reference, NULL);\n }\n \n static void copy_alternates(struct strbuf *src, struct strbuf *dst,\n@@ -949,7 +949,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \tgit_config_set(key.buf, repo);\n \tstrbuf_reset(&key);\n \n-\tif (option_reference.nr)\n+\tif (option_required_reference.nr)\n \t\tsetup_reference();\n \n \tfetch_pattern = value.buf;\n-- \n2.9.2.737.g4a14654\n\n"},{"id":"298812","messageId":"20160811231405.17318-9-sbeller@google.com","threadId":"43790","inReplyTo":"20160811231405.17318-1-sbeller@google.com","subject":"[PATCHv4 8/8] clone: recursive and reference option triggers submodule alternates","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-08-11T23:14:05Z","receivedAt":"2016-08-11T23:14:40Z","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 Documentation/config.txt       | 12 ++++++\n builtin/clone.c                | 19 +++++++++\n builtin/submodule--helper.c    | 87 ++++++++++++++++++++++++++++++++++++++++++\n t/t7408-submodule-reference.sh | 43 +++++++++++++++++++++\n 4 files changed, 161 insertions(+)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 0bcb679..e09d32c 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -2847,6 +2847,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`.\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 fdb2ca9..0593aee 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -944,6 +944,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 a10a777..a71c903 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -472,6 +472,90 @@ 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+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+\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+\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 +616,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.737.g4a14654\n\n"},{"id":"298931","messageId":"xmqqa8ghvd3j.fsf@gitster.mtv.corp.google.com","threadId":"43790","inReplyTo":"20160811231405.17318-6-sbeller@google.com","subject":"Re: [PATCHv4 5/8] clone: factor out checking for an alternate path","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-08-12T22:25:52Z","receivedAt":"2016-08-12T22:26:00Z","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> +\tstruct strbuf sb = STRBUF_INIT;\n> +\tchar *ref_git = compute_alternate_path(item->string, &sb);\n\nWho owns the memory for ref_git?\n\n> -\tif (!access(mkpath(\"%s/shallow\", ref_git), F_OK))\n> -\t\tdie(_(\"reference repository '%s' is shallow\"), item->string);\n> +\tif (!ref_git)\n> +\t\tdie(\"%s\", sb.buf);\n\nPresumably the second argument to compute_alternate_path() is a\nstrbuf to receive the error message?  It is unfortunate that the\nvariable used for this purpose is a bland \"sb\", but perhaps that\ncannot be helped as you would reuse that strbuf for a different\npurpose (i.e. not to store the error message, but to formulate a\npathname).\n\n> -\tif (!access(mkpath(\"%s/info/grafts\", ref_git), F_OK))\n> -\t\tdie(_(\"reference repository '%s' is grafted\"), item->string);\n> +\tstrbuf_addf(&sb, \"%s/objects\", ref_git);\n> +\tadd_to_alternates_file(sb.buf);\n>  \n> -\tstrbuf_addf(&alternate, \"%s/objects\", ref_git);\n> -\tadd_to_alternates_file(alternate.buf);\n> -\tstrbuf_release(&alternate);\n> -\tfree(ref_git);\n> +\tstrbuf_release(&sb);\n\nI am wondering about the loss of free() here in the first comment.\n\n> +/*\n> + * Compute the exact path an alternate is at and returns it. In case of\n> + * error NULL is returned and the human readable error is added to `err`\n> + * `path` may be relative and should point to $GITDIR.\n> + * `err` must not be null.\n> + */\n> +char *compute_alternate_path(const char *path, struct strbuf *err)\n> +{\n> +\tchar *ref_git = NULL;\n> +\tconst char *repo, *ref_git_s;\n> +\tstruct strbuf err_buf = STRBUF_INIT;\n\nWhy do you need \"err_buf\", instead of directly writing the error to\n\"err\", especially if \"err\" is not optional?\n\n> + ...\n> +out:\n> +\tif (err_buf.len) {\n> +\t\tstrbuf_addbuf(err, &err_buf);\n> +\t\tfree(ref_git);\n> +\t\tref_git = NULL;\n> +\t}\n> +\n> +\tstrbuf_release(&err_buf);\n> +\treturn ref_git;\n> +}\n\nSo ref_git is a piece of memory on heap, and the caller is\nresponsible for not leaking it.\n"},{"id":"298932","messageId":"xmqq60r5vcrs.fsf@gitster.mtv.corp.google.com","threadId":"43790","inReplyTo":"20160811231405.17318-9-sbeller@google.com","subject":"Re: [PATCHv4 8/8] clone: recursive and reference option triggers submodule alternates","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-08-12T22:32:55Z","receivedAt":"2016-08-12T22:33:04Z","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> +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> + ...\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\nOK.  Thanks to the refactoring in the earlier step, this has become\nquite straight-forward and robust; this code will always decide if\nthe other directory can be used as an alternate the same way the\n\"git clone\" we will call would decide.\n\nGood.\n"},{"id":"299377","messageId":"CAGZ79kYVh7N-H7iO4E7HydeOzdUfxGTc3sUo8RhCzR33u9gRHw@mail.gmail.com","threadId":"43790","inReplyTo":"xmqqa8ghvd3j.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCHv4 5/8] clone: factor out checking for an alternate path","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-08-15T19:03:22Z","receivedAt":"2016-08-15T19:03:34Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, Aug 12, 2016 at 3:25 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Stefan Beller <sbeller@google.com> writes:\n>\n>> +     struct strbuf sb = STRBUF_INIT;\n>> +     char *ref_git = compute_alternate_path(item->string, &sb);\n>\n> Who owns the memory for ref_git?\n\nThe caller of compute_alternate_path(..), which makes\nadd_one_reference faulty as of this patch.\n\n>\n>> -     if (!access(mkpath(\"%s/shallow\", ref_git), F_OK))\n>> -             die(_(\"reference repository '%s' is shallow\"), item->string);\n>> +     if (!ref_git)\n>> +             die(\"%s\", sb.buf);\n>\n> Presumably the second argument to compute_alternate_path() is a\n> strbuf to receive the error message?  It is unfortunate that the\n> variable used for this purpose is a bland \"sb\", but perhaps that\n> cannot be helped as you would reuse that strbuf for a different\n> purpose (i.e. not to store the error message, but to formulate a\n> pathname).\n\nOk. I had an intermediate version with 2 strbufs but for some reason I\ndecided one is better. We'll have 2 again. (err and sb; sb will have a\nsmaller scope only in the else part.)\n\n>\n>> -     if (!access(mkpath(\"%s/info/grafts\", ref_git), F_OK))\n>> -             die(_(\"reference repository '%s' is grafted\"), item->string);\n>> +     strbuf_addf(&sb, \"%s/objects\", ref_git);\n>> +     add_to_alternates_file(sb.buf);\n>>\n>> -     strbuf_addf(&alternate, \"%s/objects\", ref_git);\n>> -     add_to_alternates_file(alternate.buf);\n>> -     strbuf_release(&alternate);\n>> -     free(ref_git);\n>> +     strbuf_release(&sb);\n>\n> I am wondering about the loss of free() here in the first comment.\n\nfixed in a reroll.\n\n>\n>> +/*\n>> + * Compute the exact path an alternate is at and returns it. In case of\n>> + * error NULL is returned and the human readable error is added to `err`\n>> + * `path` may be relative and should point to $GITDIR.\n>> + * `err` must not be null.\n>> + */\n>> +char *compute_alternate_path(const char *path, struct strbuf *err)\n>> +{\n>> +     char *ref_git = NULL;\n>> +     const char *repo, *ref_git_s;\n>> +     struct strbuf err_buf = STRBUF_INIT;\n>\n> Why do you need \"err_buf\", instead of directly writing the error to\n> \"err\", especially if \"err\" is not optional?\n>\n>> + ...\n>> +out:\n>> +     if (err_buf.len) {\n\nIf we were directly writing to err, we would have checked\nerr.len here. Then you open up a subtle way of saying \"dry run\"\nby giving a non empty error buffer.\n\nI contemplated doing that actually instead of splitting up into 2 functions,\nbut I considered that bad taste as it would require documentation.\n\n>> +             strbuf_addbuf(err, &err_buf);\n>> +             free(ref_git);\n>> +             ref_git = NULL;\n>> +     }\n>> +\n>> +     strbuf_release(&err_buf);\n>> +     return ref_git;\n>> +}\n>\n> So ref_git is a piece of memory on heap, and the caller is\n> responsible for not leaking it.\n\nCorrect.\n"},{"id":"299392","messageId":"xmqq1t1phir4.fsf@gitster.mtv.corp.google.com","threadId":"43790","inReplyTo":"CAGZ79kYVh7N-H7iO4E7HydeOzdUfxGTc3sUo8RhCzR33u9gRHw@mail.gmail.com","subject":"Re: [PATCHv4 5/8] clone: factor out checking for an alternate path","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-08-15T20:36:31Z","receivedAt":"2016-08-15T20:36:40Z","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> On Fri, Aug 12, 2016 at 3:25 PM, Junio C Hamano <gitster@pobox.com> wrote:\n>> Stefan Beller <sbeller@google.com> writes:\n>>\n>>> +     struct strbuf sb = STRBUF_INIT;\n>>> +     char *ref_git = compute_alternate_path(item->string, &sb);\n>>\n>> Who owns the memory for ref_git?\n>\n> The caller of compute_alternate_path(..), which makes\n> add_one_reference faulty as of this patch.\n>\n>>\n>>> -     if (!access(mkpath(\"%s/shallow\", ref_git), F_OK))\n>>> -             die(_(\"reference repository '%s' is shallow\"), item->string);\n>>> +     if (!ref_git)\n>>> +             die(\"%s\", sb.buf);\n>>\n>> Presumably the second argument to compute_alternate_path() is a\n>> strbuf to receive the error message?  It is unfortunate that the\n>> variable used for this purpose is a bland \"sb\", but perhaps that\n>> cannot be helped as you would reuse that strbuf for a different\n>> purpose (i.e. not to store the error message, but to formulate a\n>> pathname).\n>\n> Ok. I had an intermediate version with 2 strbufs but for some reason I\n> decided one is better. We'll have 2 again. (err and sb; sb will have a\n> smaller scope only in the else part.)\n\nMy \"unfortunate\" was meant to say \"it is unfortunate that this is\nthe best, adding one extra variable is not worth the cost\".\n\n>> Why do you need \"err_buf\", instead of directly writing the error to\n>> \"err\", especially if \"err\" is not optional?\n>>\n>>> + ...\n>>> +out:\n>>> +     if (err_buf.len) {\n>\n> If we were directly writing to err, we would have checked\n> err.len here. Then you open up a subtle way of saying \"dry run\"\n> by giving a non empty error buffer.\n>\n> I contemplated doing that actually instead of splitting up into 2 functions,\n> but I considered that bad taste as it would require documentation.\n\nSorry, but I do not understand this response at all.  I am still\ntalking about keeping one function \"compute_alternate_path()\".  The\nsuggestion was just to drop \"struct strbuf err_buf\" local variable,\nand instead add the error messages the patch adds to err_buf with\ncode like:\n\n> +\tif (!ref_git_s) {\n> +\t\tstrbuf_addf(&err_buf, _(\"path '%s' does not exist\"), path);\n> +\t\tgoto out;\n\nto directly do\n\n\t\tstrbuf_addf(err, _(\"path '%s' does not exist\"), path);\n\ninstead.  That way you do not have to move the bytes around from one\nbuffer to the other, like this:\n\n>>> +             strbuf_addbuf(err, &err_buf);\n>>> +             free(ref_git);\n>>> +             ref_git = NULL;\n>>> +     }\n\nIf you allow err->len to be non-zero upon entry, you'd need a way to\nremember if you noticed an error, so that you can free and clear\nref_git after \"out:\" label, and doing so with a separate variable\nwould make the code more readable, I would think, either by (1)\nrecording the original err->len upon entry, and comparing it with\nerr->len at \"out:\" label, or (2) using a new bool \"seen_error = 0\"\nand setting it to true when you diagnose an error.  The latter would\nmake the code a bit more verbose but I suspect the result would be\neasier to read than the former.\n\n"},{"id":"299404","messageId":"CAGZ79kbFjvwXHVwCb4cBGeCpgmByVbf0mBdZYxuRMKVpg+1sWg@mail.gmail.com","threadId":"43790","inReplyTo":"xmqq1t1phir4.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCHv4 5/8] clone: factor out checking for an alternate path","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-08-15T21:44:28Z","receivedAt":"2016-08-15T21:44:36Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Mon, Aug 15, 2016 at 1:36 PM, Junio C Hamano <gitster@pobox.com> wrote:\n>\n>>> Why do you need \"err_buf\", instead of directly writing the error to\n>>> \"err\", especially if \"err\" is not optional?\n>>>\n>>>> + ...\n>>>> +out:\n>>>> +     if (err_buf.len) {\n>>\n>> If we were directly writing to err, we would have checked\n>> err.len here. Then you open up a subtle way of saying \"dry run\"\n>> by giving a non empty error buffer.\n>>\n>> I contemplated doing that actually instead of splitting up into 2 functions,\n>> but I considered that bad taste as it would require documentation.\n>\n> Sorry, but I do not understand this response at all.\n\nMe neither. I think I elided the interesting part.\n\n> I am still\n> talking about keeping one function \"compute_alternate_path()\".  The\n> suggestion was just to drop \"struct strbuf err_buf\" local variable,\n> and instead add the error messages the patch adds to err_buf with\n> code like:\n>\n>> +     if (!ref_git_s) {\n>> +             strbuf_addf(&err_buf, _(\"path '%s' does not exist\"), path);\n>> +             goto out;\n>\n> to directly do\n>\n>                 strbuf_addf(err, _(\"path '%s' does not exist\"), path);\n\ndone.\n\n> err->len at \"out:\" label, or (2) using a new bool \"seen_error = 0\"\n> and setting it to true when you diagnose an error.  The latter would\n> make the code a bit more verbose but I suspect the result would be\n> easier to read than the former.\n>\n\n(2) is very readable, will reroll with that.\n"},{"id":"299570","messageId":"xmqqr39n88pg.fsf@gitster.mtv.corp.google.com","threadId":"43790","inReplyTo":"20160811231405.17318-9-sbeller@google.com","subject":"Re: [PATCHv4 8/8] clone: recursive and reference option triggers submodule alternates","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-08-17T20:02:51Z","receivedAt":"2016-08-17T20:03:00Z","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> +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\nI have to admit that I need to follow the codepath in config.c every\ntime to check, but I _think_ git_config_get_string() gives you your\nown copy of the value.  As this function does not give ownership of\nsm_alternate or error_strategy to something else, they are leaked\nevery time this function is called, I think.\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"},{"id":"299572","messageId":"CAGZ79kZChE=y9=DiQw9rw70S1ToEJK3mqFo1JLHRueVvst4VoQ@mail.gmail.com","threadId":"43790","inReplyTo":"xmqqr39n88pg.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCHv4 8/8] clone: recursive and reference option triggers submodule alternates","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-08-17T20:53:00Z","receivedAt":"2016-08-17T20:53:09Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, Aug 17, 2016 at 1:02 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Stefan Beller <sbeller@google.com> writes:\n>\n>> +static void prepare_possible_alternates(const char *sm_name,\n>> +             struct string_list *reference)\n>> +{\n>> +     char *sm_alternate = NULL, *error_strategy = NULL;\n>> +     struct submodule_alternate_setup sas = SUBMODULE_ALTERNATE_SETUP_INIT;\n>> +\n>> +     git_config_get_string(\"submodule.alternateLocation\", &sm_alternate);\n>> +     if (!sm_alternate)\n>> +             return;\n>> +\n>> +     git_config_get_string(\"submodule.alternateErrorStrategy\", &error_strategy);\n>\n> I have to admit that I need to follow the codepath in config.c every\n> time to check, but I _think_ git_config_get_string() gives you your\n> own copy of the value.  As this function does not give ownership of\n> sm_alternate or error_strategy to something else, they are leaked\n> every time this function is called, I think.\n\nThere are quite a few more occurrences of git_config_get_string\nin the submodule helper, I'll also look at those.\n\nThanks,\nStefan\n"},{"id":"299577","messageId":"xmqqd1l784lf.fsf@gitster.mtv.corp.google.com","threadId":"43790","inReplyTo":"xmqqr39n88pg.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCHv4 8/8] clone: recursive and reference option triggers submodule alternates","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-08-17T21:31:40Z","receivedAt":"2016-08-17T21:32:43Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Stefan Beller <sbeller@google.com> writes:\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> I have to admit that I need to follow the codepath in config.c every\n> time to check, but I _think_ git_config_get_string() gives you your\n> own copy of the value.  As this function does not give ownership of\n> sm_alternate or error_strategy to something else, they are leaked\n> every time this function is called, I think.\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\nAnother thing I noticed but forgot to mention.  Can error_strategy\nbe NULL here?  We are assuming sm_alternate can be, so I presume\nthat it is sensible to protect against dereferencing a NULL here,\ntoo?\n\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"},{"id":"299578","messageId":"CAGZ79kaqy16PLm8aZkAzp6PtTPFDuN80CiPPi-bbU9tec3X_oA@mail.gmail.com","threadId":"43790","inReplyTo":"xmqqd1l784lf.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCHv4 8/8] clone: recursive and reference option triggers submodule alternates","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-08-17T21:41:57Z","receivedAt":"2016-08-17T21:42:02Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, Aug 17, 2016 at 2:31 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Junio C Hamano <gitster@pobox.com> writes:\n>\n>> Stefan Beller <sbeller@google.com> writes:\n>>\n>>> +static void prepare_possible_alternates(const char *sm_name,\n>>> +            struct string_list *reference)\n>>> +{\n>>> +    char *sm_alternate = NULL, *error_strategy = NULL;\n>>> +    struct submodule_alternate_setup sas = SUBMODULE_ALTERNATE_SETUP_INIT;\n>>> +\n>>> +    git_config_get_string(\"submodule.alternateLocation\", &sm_alternate);\n>>> +    if (!sm_alternate)\n>>> +            return;\n>>> +\n>>> +    git_config_get_string(\"submodule.alternateErrorStrategy\", &error_strategy);\n>>\n>> I have to admit that I need to follow the codepath in config.c every\n>> time to check, but I _think_ git_config_get_string() gives you your\n>> own copy of the value.  As this function does not give ownership of\n>> sm_alternate or error_strategy to something else, they are leaked\n>> every time this function is called, I think.\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>\n> Another thing I noticed but forgot to mention.  Can error_strategy\n> be NULL here?  We are assuming sm_alternate can be, so I presume\n> that it is sensible to protect against dereferencing a NULL here,\n> too?\n\nOh!\n\nOf course we need to fix that too. That slipped in because I assumed that those\ntwo variables go in tandem (if sm_alternate is set, then\nerror_strategy is set as well),\nbut that is not the case of course.\n\nWhich leads to another thing: we need to have a default for error_strategy.\nI think \"die\" is reasonable here. That also needs a documentation update.\n\n>\n>>> +    if (!strcmp(error_strategy, \"info\"))\n>>> +            sas.error_mode = SUBMODULE_ALTERNATE_ERROR_INFO;\n>>> +    if (!strcmp(sm_alternate, \"superproject\"))\n>>> +            foreach_alt_odb(add_possible_reference_from_superproject, &sas);\n>>> +}\n"}]}