{"thread":{"id":"43858","subject":"[PATCHv5 0/8] git clone: Marry --recursive and --reference","startedAt":"2016-08-15T21:53:43Z","lastAt":"2016-08-31T06:22:07Z","messageCount":24,"participants":["Stefan Beller","Junio C Hamano","Jacob Keller"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"299405","messageId":"20160815215327.15682-1-sbeller@google.com","threadId":"43858","inReplyTo":null,"subject":"[PATCHv5 0/8] git clone: Marry --recursive and --reference","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-08-15T21:53:19Z","receivedAt":"2016-08-15T21:53:43Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"v5:\n\nThanks Junio, Ramsay for comments.\n\nAs the comments only address programming work as opposed to design, the \ninterdiff is rather small:\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex 0593aee..404c5e8 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -285,23 +285,26 @@ static void strip_trailing_slashes(char *dir)\n \n static int add_one_reference(struct string_list_item *item, void *cb_data)\n {\n-\tstruct strbuf sb = STRBUF_INIT;\n+\tstruct strbuf err = STRBUF_INIT;\n \tint *required = cb_data;\n-\tchar *ref_git = compute_alternate_path(item->string, &sb);\n+\tchar *ref_git = compute_alternate_path(item->string, &err);\n \n \tif (!ref_git) {\n \t\tif (*required)\n-\t\t\tdie(\"%s\", sb.buf);\n+\t\t\tdie(\"%s\", err.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\t\t\titem->string, err.buf);\n \t} else {\n+\t\tstruct strbuf sb = STRBUF_INIT;\n \t\tstrbuf_addf(&sb, \"%s/objects\", ref_git);\n \t\tadd_to_alternates_file(sb.buf);\n+\t\tstrbuf_release(&sb);\n \t}\n \n-\tstrbuf_release(&sb);\n+\tstrbuf_release(&err);\n+\tfree(ref_git);\n \treturn 0;\n }\n \ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 472b1d9..681b91d 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -484,7 +484,7 @@ struct submodule_alternate_setup {\n #define SUBMODULE_ALTERNATE_SETUP_INIT { NULL, \\\n \tSUBMODULE_ALTERNATE_ERROR_IGNORE, NULL }\n \n-int add_possible_reference_from_superproject(\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;\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 2489653..0fe5aa3 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -435,11 +435,12 @@ 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+\tint seen_error = 0;\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\tseen_error = 1;\n+\t\tstrbuf_addf(err, _(\"path '%s' does not exist\"), path);\n \t\tgoto out;\n \t} else\n \t\t/*\n@@ -462,40 +463,41 @@ char *compute_alternate_path(const char *path, struct strbuf *err)\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\tseen_error = 1;\n \t\tif (get_common_dir(&sb, ref_git)) {\n-\t\t\tstrbuf_addf(&err_buf,\n+\t\t\tstrbuf_addf(err,\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\tstrbuf_addf(err, _(\"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\tstrbuf_addf(err, _(\"reference repository '%s' is shallow\"),\n \t\t\t path);\n+\t\tseen_error = 1;\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\tstrbuf_addf(err,\n \t\t\t _(\"reference repository '%s' is grafted\"),\n \t\t\t path);\n+\t\tseen_error = 1;\n \t\tgoto out;\n \t}\n \n out:\n-\tif (err_buf.len) {\n-\t\tstrbuf_addbuf(err, &err_buf);\n+\tif (seen_error) {\n \t\tfree(ref_git);\n \t\tref_git = NULL;\n \t}\n \n-\tstrbuf_release(&err_buf);\n \treturn ref_git;\n }\n \nThanks,\nStefan\n\n\nv4:\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":"299406","messageId":"20160815215327.15682-2-sbeller@google.com","threadId":"43858","inReplyTo":"20160815215327.15682-1-sbeller@google.com","subject":"[PATCHv5 1/8] t7408: modernize style","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-08-15T21:53:20Z","receivedAt":"2016-08-15T21:53:48Z","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 \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.730.g525ad04.dirty\n\n"},{"id":"299407","messageId":"20160815215327.15682-3-sbeller@google.com","threadId":"43858","inReplyTo":"20160815215327.15682-1-sbeller@google.com","subject":"[PATCHv5 2/8] t7408: merge short tests, factor out testing method","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-08-15T21:53:21Z","receivedAt":"2016-08-15T21:53:50Z","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 \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.730.g525ad04.dirty\n\n"},{"id":"299408","messageId":"20160815215327.15682-5-sbeller@google.com","threadId":"43858","inReplyTo":"20160815215327.15682-1-sbeller@google.com","subject":"[PATCHv5 4/8] submodule--helper update-clone: allow multiple references","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-08-15T21:53:23Z","receivedAt":"2016-08-15T21:53:55Z","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 \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 ce9b3e9..7096848 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, \"\",\n \t\t\t N_(\"Create a shallow clone truncated to the \"\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex c90dc33..3b412f5 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -575,7 +575,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.730.g525ad04.dirty\n\n"},{"id":"299409","messageId":"20160815215327.15682-6-sbeller@google.com","threadId":"43858","inReplyTo":"20160815215327.15682-1-sbeller@google.com","subject":"[PATCHv5 5/8] clone: factor out checking for an alternate path","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-08-15T21:53:24Z","receivedAt":"2016-08-15T21:53:58Z","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 \n---\n builtin/clone.c | 43 +++++++-------------------------\n cache.h | 1 +\n sha1_file.c | 76 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++\n 3 files changed, 86 insertions(+), 34 deletions(-)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex f044a8c..4cd3a66 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -282,44 +282,19 @@ 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 err = STRBUF_INIT;\n+\tstruct strbuf sb = STRBUF_INIT;\n+\tchar *ref_git = compute_alternate_path(item->string, &err);\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\", err.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(&err);\n+\tstrbuf_release(&sb);\n \treturn 0;\n }\n \ndiff --git a/cache.h b/cache.h\nindex b5f76a4..1087dca 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1342,6 +1342,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 cb571ac..0fe5aa3 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -425,6 +425,82 @@ 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+\tint seen_error = 0;\n+\n+\tref_git_s = real_path_if_valid(path);\n+\tif (!ref_git_s) {\n+\t\tseen_error = 1;\n+\t\tstrbuf_addf(err, _(\"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\tseen_error = 1;\n+\t\tif (get_common_dir(&sb, ref_git)) {\n+\t\t\tstrbuf_addf(err,\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, _(\"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, _(\"reference repository '%s' is shallow\"),\n+\t\t\t path);\n+\t\tseen_error = 1;\n+\t\tgoto out;\n+\t}\n+\n+\tif (!access(mkpath(\"%s/info/grafts\", ref_git), F_OK)) {\n+\t\tstrbuf_addf(err,\n+\t\t\t _(\"reference repository '%s' is grafted\"),\n+\t\t\t path);\n+\t\tseen_error = 1;\n+\t\tgoto out;\n+\t}\n+\n+out:\n+\tif (seen_error) {\n+\t\tfree(ref_git);\n+\t\tref_git = NULL;\n+\t}\n+\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.730.g525ad04.dirty\n\n"},{"id":"299410","messageId":"20160815215327.15682-7-sbeller@google.com","threadId":"43858","inReplyTo":"20160815215327.15682-1-sbeller@google.com","subject":"[PATCHv5 6/8] clone: clarify option_reference as required","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-08-15T21:53:25Z","receivedAt":"2016-08-15T21:54:03Z","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 \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 4cd3a66..ef5db7c 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@@ -300,7 +300,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@@ -952,7 +952,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.730.g525ad04.dirty\n\n"},{"id":"299411","messageId":"20160815215327.15682-4-sbeller@google.com","threadId":"43858","inReplyTo":"20160815215327.15682-1-sbeller@google.com","subject":"[PATCHv5 3/8] submodule--helper module-clone: allow multiple references","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-08-15T21:53:22Z","receivedAt":"2016-08-15T21:54:06Z","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 \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 6f6d67a..ce9b3e9 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.730.g525ad04.dirty\n\n"},{"id":"299412","messageId":"20160815215327.15682-8-sbeller@google.com","threadId":"43858","inReplyTo":"20160815215327.15682-1-sbeller@google.com","subject":"[PATCHv5 7/8] clone: implement optional references","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-08-15T21:53:26Z","receivedAt":"2016-08-15T21:54:08Z","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 \n---\n Documentation/git-clone.txt | 5 ++++-\n builtin/clone.c | 35 +++++++++++++++++++++++++----------\n 2 files changed, 29 insertions(+), 11 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 ::\n+--reference[-if-able] ::\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 ef5db7c..0182665 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,24 +286,36 @@ 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 err = STRBUF_INIT;\n-\tstruct strbuf sb = STRBUF_INIT;\n+\tint *required = cb_data;\n \tchar *ref_git = compute_alternate_path(item->string, &err);\n \n-\tif (!ref_git)\n-\t\tdie(\"%s\", err.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\", err.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, err.buf);\n+\t} else {\n+\t\tstruct strbuf sb = STRBUF_INIT;\n+\t\tstrbuf_addf(&sb, \"%s/objects\", ref_git);\n+\t\tadd_to_alternates_file(sb.buf);\n+\t\tstrbuf_release(&sb);\n+\t}\n \n-\tfree(ref_git);\n \tstrbuf_release(&err);\n-\tstrbuf_release(&sb);\n+\tfree(ref_git);\n \treturn 0;\n }\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@@ -952,7 +967,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.730.g525ad04.dirty\n\n"},{"id":"299413","messageId":"20160815215327.15682-9-sbeller@google.com","threadId":"43858","inReplyTo":"20160815215327.15682-1-sbeller@google.com","subject":"[PATCHv5 8/8] clone: recursive and reference option triggers submodule alternates","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-08-15T21:53:27Z","receivedAt":"2016-08-15T21:54:11Z","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 \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 bc1c433..602f43a 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`.\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..681b91d 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+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//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.730.g525ad04.dirty\n\n"},{"id":"299417","messageId":"xmqqziodekbw.fsf@gitster.mtv.corp.google.com","threadId":"43858","inReplyTo":"20160815215327.15682-1-sbeller@google.com","subject":"Re: [PATCHv5 0/8] git clone: Marry --recursive and --reference","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-08-15T22:30:43Z","receivedAt":"2016-08-15T22:30:51Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> As the comments only address programming work as opposed to design, the \n> interdiff is rather small:\n\nThanks. Will queue. I think we do not need that extra err strbuf\nin add_one_reference(), but we can go either way and it is not worth\nrerolling.\n\n"},{"id":"300000","messageId":"CA+P7+xrm0ujjydLFzD2joGi=Gbg9ie__21qYrpfHM9g5hqm=EQ@mail.gmail.com","threadId":"43858","inReplyTo":"20160815215327.15682-2-sbeller@google.com","subject":"Re: [PATCHv5 1/8] t7408: modernize style","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2016-08-23T22:04:27Z","receivedAt":"2016-08-23T22:05:07Z","isPatch":false,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Mon, Aug 15, 2016 at 2:53 PM, Stefan Beller wrote:\n> No functional change intended. This commit only changes formatting\n> to the style we recently use, e.g. starting the body of a test with a\n> single quote on the same line as the header, and then having the test\n> indented in the following lines.\n>\n> Whenever we change directories, we do that in subshells.\n>\n\nI looked this over using -w to ignore whitespace changes, and it\nappears to have no functional changes. Much cleaner style overall, and\neasier to read the new file as it is now. The tests pass fine both\nbefore and after this commit, and I don't see anything that should\nfunctionally change the results.\n\nThanks,\nJake\n"},{"id":"300001","messageId":"CA+P7+xrorjJvOe=y9dipxgvTtVTVp98jz3ubCkmCMUfvAcf7xA@mail.gmail.com","threadId":"43858","inReplyTo":"20160815215327.15682-4-sbeller@google.com","subject":"Re: [PATCHv5 3/8] submodule--helper module-clone: allow multiple references","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2016-08-23T22:12:51Z","receivedAt":"2016-08-23T22:13:17Z","isPatch":false,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Mon, Aug 15, 2016 at 2:53 PM, Stefan Beller wrote:\n> Allow users to pass in multiple references, just as clone accepts multiple\n> references as well.\n>\n\nStraight forward and reasonable change.\n\nRegards,\nJake\n"},{"id":"300005","messageId":"CA+P7+xrmsNBxmoR6+2C9ZVvXcVH7D+O3iYqKhVrbAsaNUXzF_A@mail.gmail.com","threadId":"43858","inReplyTo":"20160815215327.15682-3-sbeller@google.com","subject":"Re: [PATCHv5 2/8] t7408: merge short tests, factor out testing method","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2016-08-23T22:10:23Z","receivedAt":"2016-08-23T22:27:23Z","isPatch":false,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Mon, Aug 15, 2016 at 2:53 PM, Stefan Beller wrote:\n> Tests consisting of one line each can be consolidated to have fewer tests\n> to run as well as fewer lines of code.\n>\n> When having just a few git commands, do not create a new shell but\n> use the -C flag in Git to execute in the correct directory.\n>\n> Signed-off-by: Stefan Beller \n> ---\n\nLooks good. The resulting test file is easier to read, and you created\na common function to perform checking if we used alternates instead of\nduplicating that part multiple times.\n\n> t/t7408-submodule-reference.sh | 48 ++++++++++++++++++++++--------------------\n> 1 file changed, 25 insertions(+), 23 deletions(-)\n>\n> diff --git a/t/t7408-submodule-reference.sh b/t/t7408-submodule-reference.sh\n> index 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> + alternates_file=\"$1\" &&\n> + working_dir=\"$2\" &&\n> + test_line_count = 1 \"$alternates_file\" &&\n> + echo \"0 objects, 0 kilobytes\" >expect &&\n> + git -C \"$working_dir\" count-objects >actual &&\n> + test_cmp expect actual\n> +}\n> +\n\nThis change wasn't mentioned in the description. You updated the tests\nto use a stronger check for when alternates is in use. This is good. I\nmight have mentioned it in the description but it's not worth a\nre-roll.\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\nThe stronger variant was used once here, but you use it in several\nlocations. It's good to have extracted this as the new tests are more\nreadable.\n\nThanks,\nJake\n"},{"id":"300006","messageId":"CA+P7+xrd4eagXSFQen+TZop7u+h2eyLgQ_0vjT03Bjov-KNL7w@mail.gmail.com","threadId":"43858","inReplyTo":"20160815215327.15682-6-sbeller@google.com","subject":"Re: [PATCHv5 5/8] clone: factor out checking for an alternate path","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2016-08-23T22:29:51Z","receivedAt":"2016-08-23T22:30:17Z","isPatch":false,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Mon, Aug 15, 2016 at 2:53 PM, Stefan Beller wrote:\n> + if (!repo && is_directory(mkpath(\"%s/.git/objects\", ref_git))) {\n> + char *ref_git_git = mkpathdup(\"%s/.git\", ref_git);\n> + free(ref_git);\n> + ref_git = ref_git_git;\n> + } else if (!is_directory(mkpath(\"%s/objects\", ref_git))) {\n\nI realize this was directly copied but the use of !repo to determine\nwhat file path here was definitely not easy to process, because it\nmade me think we were adding .git twice to the path. Not sure if there\nis an easier way to expand this and avoid that confusion?\n\nAlso, what happens if repo is false, so we run the first block above,\nbut is_directory fails? We just continue along even though it appears\nlike we should fail? I'm not 100% sure I follow the logic here.\nRegardless this appears to be a pretty direct copy of what was there\nbefore so I don't think it's worse.\n\nThanks,\nJake\n"},{"id":"300008","messageId":"CA+P7+xq9_kx7d5NMk2QxwZRQn8o6qgkJq5ryZL2FVrHaUmqKQQ@mail.gmail.com","threadId":"43858","inReplyTo":"20160815215327.15682-5-sbeller@google.com","subject":"Re: [PATCHv5 4/8] submodule--helper update-clone: allow multiple references","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2016-08-23T22:20:33Z","receivedAt":"2016-08-23T22:34:38Z","isPatch":false,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Mon, Aug 15, 2016 at 2:53 PM, Stefan Beller wrote:\n> Allow the user to pass in multiple references to update_clone.\n> Currently this is only internal API, but once the shell script is\n> replaced by a C version, this is needed.\n>\n> This fixes an API bug between the shell script and the helper.\n> Currently the helper accepts \"--reference\" \"--reference=foo\"\n> as a OPT_STRING whose value happens to be \"--reference=foo\", and\n> then uses\n>\n> if (suc->reference)\n> argv_array_push(&child->args, suc->reference)\n>\n> where suc->reference _is_ \"--reference=foo\" when invoking the\n> underlying \"git clone\", it cancels out.\n>\n> With this change we omit one of the \"--reference\" arguments when\n> passing references from the shell script to the helper.\n>\n\nYep, I see the API fix, and it looks correct. Makes use of the helper\neasier and more likely to be done correctly.\n\n> Signed-off-by: Stefan Beller \n> ---\n> builtin/submodule--helper.c | 14 +++++++++-----\n> git-submodule.sh | 2 +-\n> 2 files changed, 10 insertions(+), 6 deletions(-)\n>\n> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\n> index ce9b3e9..7096848 100644\n> --- a/builtin/submodule--helper.c\n> +++ b/builtin/submodule--helper.c\n> @@ -584,7 +584,7 @@ struct submodule_update_clone {\n> /* configuration parameters which are passed on to the children */\n> int quiet;\n> int recommend_shallow;\n> - const char *reference;\n> + struct string_list references;\n> const char *depth;\n> const char *recursive_prefix;\n> const char *prefix;\n> @@ -600,7 +600,8 @@ struct submodule_update_clone {\n> int failed_clones_nr, failed_clones_alloc;\n> };\n> #define SUBMODULE_UPDATE_CLONE_INIT {0, MODULE_LIST_INIT, 0, \\\n> - SUBMODULE_UPDATE_STRATEGY_INIT, 0, -1, NULL, NULL, NULL, NULL, \\\n> + SUBMODULE_UPDATE_STRATEGY_INIT, 0, -1, STRING_LIST_INIT_DUP, \\\n> + NULL, NULL, NULL, \\\n> STRING_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> argv_array_pushl(&child->args, \"--path\", sub->path, NULL);\n> argv_array_pushl(&child->args, \"--name\", sub->name, NULL);\n> argv_array_pushl(&child->args, \"--url\", url, NULL);\n> - if (suc->reference)\n> - argv_array_push(&child->args, suc->reference);\n\nHere, you now no longer pass in the reference as a whole, but assume\nthat it is actually just the value to pass to --reference. And below\nyou correctly replace the extra --reference. Ok so I see how you fixed\nthe API bug.\n\n> diff --git a/git-submodule.sh b/git-submodule.sh\n> index c90dc33..3b412f5 100755\n> --- a/git-submodule.sh\n> +++ b/git-submodule.sh\n> @@ -575,7 +575,7 @@ cmd_update()\n> ${wt_prefix:+--prefix \"$wt_prefix\"} \\\n> ${prefix:+--recursive-prefix \"$prefix\"} \\\n> ${update:+--update \"$update\"} \\\n> - ${reference:+--reference \"$reference\"} \\\n> + ${reference:+\"$reference\"} \\\n\nFrom above, I see how you fixed this to assume (as above) that\n$reference is \"--reference \" and pass it directly. It worked\nbefore but was definitely a bit \"brittle\" in that direct calling of\nthe helper function would not behave as expected. Nice!\n\nRegards,\nJake\n"},{"id":"300009","messageId":"CA+P7+xp1AQe6N9jANaRhO-QsNUkBh2kK5SR-=utihEGt=tzFnA@mail.gmail.com","threadId":"43858","inReplyTo":"20160815215327.15682-8-sbeller@google.com","subject":"Re: [PATCHv5 7/8] clone: implement optional references","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2016-08-23T22:35:19Z","receivedAt":"2016-08-23T22:36:43Z","isPatch":false,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Mon, Aug 15, 2016 at 2:53 PM, Stefan Beller wrote:\n> In a later patch we want to try to create alternates for submodules,\n> but they might not exist in the referenced superproject. So add a way\n> to skip the non existing references and report them.\n>\n\nSeems pretty straight forward to me.\n\n> Signed-off-by: Stefan Beller \n> ---\n> Documentation/git-clone.txt | 5 ++++-\n> builtin/clone.c | 35 +++++++++++++++++++++++++----------\n> 2 files changed, 29 insertions(+), 11 deletions(-)\n>\n> diff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt\n> index 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 ::\n> +--reference[-if-able] ::\n> If the reference repository is on the local machine,\n> automatically setup `.git/objects/info/alternates` to\n> obtain objects from the reference repository. Using\n> an already existing repository as an alternate will\n> require fewer objects to be copied from the repository\n> being cloned, reducing network and local storage costs.\n> + When using the `--reference-if-able`, a non existing\n> + directory is skipped with a warning instead of aborting\n> + the clone.\n> +\n> *NOTE*: see the NOTE for the `--shared` option, and also the\n> `--dissociate` option.\n> diff --git a/builtin/clone.c b/builtin/clone.c\n> index ef5db7c..0182665 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> N_(\"directory from which templates will be used\")),\n> OPT_STRING_LIST(0, \"reference\", &option_required_reference, N_(\"repo\"),\n> N_(\"reference repository\")),\n> + OPT_STRING_LIST(0, \"reference-if-able\", &option_optional_reference,\n> + N_(\"repo\"), N_(\"reference repository\")),\n> OPT_BOOL(0, \"dissociate\", &option_dissociate,\n> N_(\"use --reference only while cloning\")),\n> OPT_STRING('o', \"origin\", &option_origin, N_(\"name\"),\n> @@ -283,24 +286,36 @@ static void strip_trailing_slashes(char *dir)\n> static int add_one_reference(struct string_list_item *item, void *cb_data)\n> {\n> struct strbuf err = STRBUF_INIT;\n> - struct strbuf sb = STRBUF_INIT;\n> + int *required = cb_data;\n> char *ref_git = compute_alternate_path(item->string, &err);\n>\n> - if (!ref_git)\n> - die(\"%s\", err.buf);\n> -\n> - strbuf_addf(&sb, \"%s/objects\", ref_git);\n> - add_to_alternates_file(sb.buf);\n> + if (!ref_git) {\n> + if (*required)\n> + die(\"%s\", err.buf);\n> + else\n> + fprintf(stderr,\n> + _(\"info: Could not add alternate for '%s': %s\\n\"),\n> + item->string, err.buf);\n> + } else {\n> + struct strbuf sb = STRBUF_INIT;\n> + strbuf_addf(&sb, \"%s/objects\", ref_git);\n> + add_to_alternates_file(sb.buf);\n> + strbuf_release(&sb);\n> + }\n\nI might have done this with a \"goto out\" instead of the else block,\nbut this is reasonable as well.\n\nRegards,\nJake\n"},{"id":"300010","messageId":"CA+P7+xo0Rpva_jkfyk8+eAn6cKLS4kBgHwvTvzj+2A1mp=iG0A@mail.gmail.com","threadId":"43858","inReplyTo":"20160815215327.15682-7-sbeller@google.com","subject":"Re: [PATCHv5 6/8] clone: clarify option_reference as required","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2016-08-23T22:31:14Z","receivedAt":"2016-08-23T22:39:39Z","isPatch":false,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Mon, Aug 15, 2016 at 2:53 PM, Stefan Beller wrote:\n> In the next patch we introduce optional references; To better distinguish\n> between optional and required references we rename the variable.\n>\n\nMakes sense. It's a bit weird to process \"option_required_reference\"\nbut I think it is reasonable.\n\nRegards,\nJake\n"},{"id":"300013","messageId":"CA+P7+xpDqkTFLUJBhSwWiVnXw-iy1fmGBWzVBLmybOcPOmevBw@mail.gmail.com","threadId":"43858","inReplyTo":"20160815215327.15682-9-sbeller@google.com","subject":"Re: [PATCHv5 8/8] clone: recursive and reference option triggers submodule alternates","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2016-08-23T22:43:30Z","receivedAt":"2016-08-23T22:59:29Z","isPatch":false,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Mon, Aug 15, 2016 at 2:53 PM, Stefan Beller wrote:\n> When `--recursive` and `--reference` is given, it is reasonable to\n> expect that the submodules are created with references to the submodules\n> of 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>\n> To solve this problem introduce new options that store the configuration\n> for what the user wanted originally.\n>\n> Signed-off-by: Stefan Beller \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>\n> diff --git a/Documentation/config.txt b/Documentation/config.txt\n> index bc1c433..602f43a 100644\n> --- a/Documentation/config.txt\n> +++ b/Documentation/config.txt\n> @@ -2837,6 +2837,18 @@ submodule.fetchJobs::\n> in parallel. A value of 0 will give some reasonable default.\n> If unset, it defaults to 1.\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> +submodule.alternateErrorStrategy\n> + Specifies how to treat errors with the alternates for a submodule\n> + as computed via `submodule.alternateLocation`. Possible values are\n> + `ignore`, `info`, `die`.\n> +\n> tag.forceSignAnnotated::\n> A boolean to specify whether annotated tags created should be GPG signed.\n> If `--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> else\n> fprintf(stderr, _(\"Cloning into '%s'...\\n\"), dir);\n> }\n> +\n> + if (option_recursive) {\n> + if (option_required_reference.nr &&\n> + option_optional_reference.nr)\n> + die(_(\"clone --recursive is not compatible with \"\n> + \"both --reference and --reference-if-able\"));\n\nSo if you have multiple references that don't all match we basically\njust refuse to allow recursive?\n\nWould it be better to simply assume that we want to die on missing\nreferences instead of failing the clone here? That is, treat it so\nthat multiple reference and reference-if-able will die, and only info\nif we got only reference-if-able?\n\nProbably what's here is fine, and mixing reference and\nreference-if-able doesn't make much sense.\n\nThanks,\nJake\n"},{"id":"300014","messageId":"CAGZ79ka6nwYjBRcUKAxCqAodq=Hw6f86J0Mq6GWyKgMO_PNi4A@mail.gmail.com","threadId":"43858","inReplyTo":"CA+P7+xpDqkTFLUJBhSwWiVnXw-iy1fmGBWzVBLmybOcPOmevBw@mail.gmail.com","subject":"Re: [PATCHv5 8/8] clone: recursive and reference option triggers submodule alternates","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-08-23T23:03:21Z","receivedAt":"2016-08-23T23:03:28Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":">> +\n>> + if (option_recursive) {\n>> + if (option_required_reference.nr &&\n>> + option_optional_reference.nr)\n>> + die(_(\"clone --recursive is not compatible with \"\n>> + \"both --reference and --reference-if-able\"));\n>\n> So if you have multiple references that don't all match we basically\n> just refuse to allow recursive?\n>\n> Would it be better to simply assume that we want to die on missing\n> references instead of failing the clone here?\n\nThe new config options are per repo (or even set globally), and not\nper alternate. And as we communicate the [if-able] part via the config\noptions to the submodules it is not feasible to transport both\nkinds of (reference-or-die and reference-but-ignore-misses).\n\nThat is why I introduced this check in the first place. If we'd go back\nto the drawing board and come up with a solution that is on a\n\"per alternate\" basis we could allow such things.\n\n> That is, treat it so\n> that multiple reference and reference-if-able will die, and only info\n> if we got only reference-if-able?\n>\n> Probably what's here is fine, and mixing reference and\n> reference-if-able doesn't make much sense.\n\nI think the reference-if-able doesn't make sense for one project alone\nas you can easily script around that, but is only useful if you have\nsubmodules in a partially checked out superproject that you want\nto reference to.\n\nThanks,\nStefan\n"},{"id":"300023","messageId":"CA+P7+xpmyx+QsdOpS7JC1i9Z6cdsy_=MK7J_rGYiukPsqAJBVQ@mail.gmail.com","threadId":"43858","inReplyTo":"CAGZ79ka6nwYjBRcUKAxCqAodq=Hw6f86J0Mq6GWyKgMO_PNi4A@mail.gmail.com","subject":"Re: [PATCHv5 8/8] clone: recursive and reference option triggers submodule alternates","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2016-08-24T06:29:55Z","receivedAt":"2016-08-24T06:30:23Z","isPatch":false,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Tue, Aug 23, 2016 at 4:03 PM, Stefan Beller wrote:\n>>> +\n>>> + if (option_recursive) {\n>>> + if (option_required_reference.nr &&\n>>> + option_optional_reference.nr)\n>>> + die(_(\"clone --recursive is not compatible with \"\n>>> + \"both --reference and --reference-if-able\"));\n>>\n>> So if you have multiple references that don't all match we basically\n>> just refuse to allow recursive?\n>>\n>> Would it be better to simply assume that we want to die on missing\n>> references instead of failing the clone here?\n>\n> The new config options are per repo (or even set globally), and not\n> per alternate. And as we communicate the [if-able] part via the config\n> options to the submodules it is not feasible to transport both\n> kinds of (reference-or-die and reference-but-ignore-misses).\n>\n> That is why I introduced this check in the first place. If we'd go back\n> to the drawing board and come up with a solution that is on a\n> \"per alternate\" basis we could allow such things.\n>\n>> That is, treat it so\n>> that multiple reference and reference-if-able will die, and only info\n>> if we got only reference-if-able?\n>>\n>> Probably what's here is fine, and mixing reference and\n>> reference-if-able doesn't make much sense.\n>\n> I think the reference-if-able doesn't make sense for one project alone\n> as you can easily script around that, but is only useful if you have\n> submodules in a partially checked out superproject that you want\n> to reference to.\n>\n> Thanks,\n> Stefan\n\nI'm not sure there is a better design. How are alternates stored? In\na config section? Or is there some way we can store the is-able per\nalternate and look it up when adding them to submodule?\n\nThanks,\nJake\n"},{"id":"300127","messageId":"CAGZ79kah4sY0NJkaqDiUqcwsCHn0SECkMjN8SoXQ8vGi6zRkuw@mail.gmail.com","threadId":"43858","inReplyTo":"CA+P7+xpmyx+QsdOpS7JC1i9Z6cdsy_=MK7J_rGYiukPsqAJBVQ@mail.gmail.com","subject":"Re: [PATCHv5 8/8] clone: recursive and reference option triggers submodule alternates","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-08-24T22:52:53Z","receivedAt":"2016-08-24T22:52:59Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Aug 23, 2016 at 11:29 PM, Jacob Keller wrote:\n> On Tue, Aug 23, 2016 at 4:03 PM, Stefan Beller wrote:\n>>>> +\n>>>> + if (option_recursive) {\n>>>> + if (option_required_reference.nr &&\n>>>> + option_optional_reference.nr)\n>>>> + die(_(\"clone --recursive is not compatible with \"\n>>>> + \"both --reference and --reference-if-able\"));\n>>>\n>>> So if you have multiple references that don't all match we basically\n>>> just refuse to allow recursive?\n>>>\n>>> Would it be better to simply assume that we want to die on missing\n>>> references instead of failing the clone here?\n>>\n>> The new config options are per repo (or even set globally), and not\n>> per alternate. And as we communicate the [if-able] part via the config\n>> options to the submodules it is not feasible to transport both\n>> kinds of (reference-or-die and reference-but-ignore-misses).\n>>\n>> That is why I introduced this check in the first place. If we'd go back\n>> to the drawing board and come up with a solution that is on a\n>> \"per alternate\" basis we could allow such things.\n>>\n>>> That is, treat it so\n>>> that multiple reference and reference-if-able will die, and only info\n>>> if we got only reference-if-able?\n>>>\n>>> Probably what's here is fine, and mixing reference and\n>>> reference-if-able doesn't make much sense.\n>>\n>> I think the reference-if-able doesn't make sense for one project alone\n>> as you can easily script around that, but is only useful if you have\n>> submodules in a partially checked out superproject that you want\n>> to reference to.\n>>\n>> Thanks,\n>> Stefan\n>\n> I'm not sure there is a better design. How are alternates stored? In\n> a config section?\n\nAlternates are stored in .git/objects/info/alternates\nwith each alternate in a new line. On that file (from\n(man gitrepository-layout):\n\nobjects/info/alternates\n\nThis file records paths to alternate object stores that this object store\nborrows objects from, one pathname per line. Note that not only native\nGit tools use it locally, but the HTTP fetcher also tries to use it remotely;\nthis will usually work if you have relative paths (relative to the object\ndatabase, not to the repository!) in your alternates file, but it will not work\nif you use absolute paths unless the absolute path in filesystem and web\nURL is the same. See also objects/info/http-alternates.\n\nSo changing that file is out of question.\nIdeally we would have a flag for each path here, though.\n\n> Or is there some way we can store the is-able per\n> alternate and look it up when adding them to submodule?\n\nI guess we could invent a file as alternate-flags that is matches\nline by line to the alternates file.\n\nI don't think we'd want to go that way for now as it would really only\nhelp in an edge case?\n\nIf we later find out we need the flag on a per-alternate basis we can\nstill come up with a solution and just not set these config variables,\nso I think we'll be fine for now with this approach.\n\nThanks,\nStefan\n"},{"id":"300130","messageId":"CA+P7+xrokr0ZGidQFuvpN+-J_WDjkaUropcnPGVjZHafc12AnQ@mail.gmail.com","threadId":"43858","inReplyTo":"CAGZ79kah4sY0NJkaqDiUqcwsCHn0SECkMjN8SoXQ8vGi6zRkuw@mail.gmail.com","subject":"Re: [PATCHv5 8/8] clone: recursive and reference option triggers submodule alternates","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2016-08-24T23:37:29Z","receivedAt":"2016-08-25T04:55:27Z","isPatch":false,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Wed, Aug 24, 2016 at 3:52 PM, Stefan Beller wrote:\n> On Tue, Aug 23, 2016 at 11:29 PM, Jacob Keller wrote:\n>> On Tue, Aug 23, 2016 at 4:03 PM, Stefan Beller wrote:\n>>>>> +\n>>>>> + if (option_recursive) {\n>>>>> + if (option_required_reference.nr &&\n>>>>> + option_optional_reference.nr)\n>>>>> + die(_(\"clone --recursive is not compatible with \"\n>>>>> + \"both --reference and --reference-if-able\"));\n>>>>\n>>>> So if you have multiple references that don't all match we basically\n>>>> just refuse to allow recursive?\n>>>>\n>>>> Would it be better to simply assume that we want to die on missing\n>>>> references instead of failing the clone here?\n>>>\n>>> The new config options are per repo (or even set globally), and not\n>>> per alternate. And as we communicate the [if-able] part via the config\n>>> options to the submodules it is not feasible to transport both\n>>> kinds of (reference-or-die and reference-but-ignore-misses).\n>>>\n>>> That is why I introduced this check in the first place. If we'd go back\n>>> to the drawing board and come up with a solution that is on a\n>>> \"per alternate\" basis we could allow such things.\n>>>\n>>>> That is, treat it so\n>>>> that multiple reference and reference-if-able will die, and only info\n>>>> if we got only reference-if-able?\n>>>>\n>>>> Probably what's here is fine, and mixing reference and\n>>>> reference-if-able doesn't make much sense.\n>>>\n>>> I think the reference-if-able doesn't make sense for one project alone\n>>> as you can easily script around that, but is only useful if you have\n>>> submodules in a partially checked out superproject that you want\n>>> to reference to.\n>>>\n>>> Thanks,\n>>> Stefan\n>>\n>> I'm not sure there is a better design. How are alternates stored? In\n>> a config section?\n>\n> Alternates are stored in .git/objects/info/alternates\n> with each alternate in a new line. On that file (from\n> (man gitrepository-layout):\n>\n> objects/info/alternates\n>\n> This file records paths to alternate object stores that this object store\n> borrows objects from, one pathname per line. Note that not only native\n> Git tools use it locally, but the HTTP fetcher also tries to use it remotely;\n> this will usually work if you have relative paths (relative to the object\n> database, not to the repository!) in your alternates file, but it will not work\n> if you use absolute paths unless the absolute path in filesystem and web\n> URL is the same. See also objects/info/http-alternates.\n>\n> So changing that file is out of question.\n> Ideally we would have a flag for each path here, though.\n>\n>> Or is there some way we can store the is-able per\n>> alternate and look it up when adding them to submodule?\n>\n> I guess we could invent a file as alternate-flags that is matches\n> line by line to the alternates file.\n>\n> I don't think we'd want to go that way for now as it would really only\n> help in an edge case?\n>\n> If we later find out we need the flag on a per-alternate basis we can\n> still come up with a solution and just not set these config variables,\n> so I think we'll be fine for now with this approach.\n>\n\nYes that seems reasonable.\n\nThanks,\nJake\n"},{"id":"300622","messageId":"CAGZ79kYAHXct0Fz-sw0-FbN5-Mij-C4Qwak_S0mxKHyW=U5jWQ@mail.gmail.com","threadId":"43858","inReplyTo":"CA+P7+xrokr0ZGidQFuvpN+-J_WDjkaUropcnPGVjZHafc12AnQ@mail.gmail.com","subject":"Re: [PATCHv5 8/8] clone: recursive and reference option triggers submodule alternates","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-08-31T05:04:07Z","receivedAt":"2016-08-31T05:04:12Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, Aug 24, 2016 at 4:37 PM, Jacob Keller wrote:\n\n> Yes that seems reasonable.\n>\n> Thanks,\n> Jake\n\nI reviewed all your comments and you seem to be ok with including this\nseries as it is queued currently?\n\nThanks,\nStefan\n"},{"id":"300628","messageId":"CA+P7+xoc12ns8OriSQQwhKJPSTuG8hniOcUkbGbW3fZYZiqViw@mail.gmail.com","threadId":"43858","inReplyTo":"CAGZ79kYAHXct0Fz-sw0-FbN5-Mij-C4Qwak_S0mxKHyW=U5jWQ@mail.gmail.com","subject":"Re: [PATCHv5 8/8] clone: recursive and reference option triggers submodule alternates","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2016-08-31T06:21:42Z","receivedAt":"2016-08-31T06:22:07Z","isPatch":false,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Tue, Aug 30, 2016 at 10:04 PM, Stefan Beller wrote:\n> On Wed, Aug 24, 2016 at 4:37 PM, Jacob Keller wrote:\n>\n>> Yes that seems reasonable.\n>>\n>> Thanks,\n>> Jake\n>\n> I reviewed all your comments and you seem to be ok with including this\n> series as it is queued currently?\n>\n> Thanks,\n> Stefan\n\nYea based on what's in Junio's tree, I think we've squashed in all the\nsuggested changes, unless there have been more suggestions I missed.\n\nRegards,\nJake\n"}]}