{"thread":{"id":"43008","subject":"[PATCH 0/6] git clone: Marry --recursive and --reference","startedAt":"2016-08-04T19:52:12Z","lastAt":"2016-08-07T09:24:57Z","messageCount":25,"participants":["Stefan Beller","Junio C Hamano","René Scharfe"],"isPatch":true,"patchVersion":1,"patchTotal":6},"messages":[{"id":"293123","messageId":"20160804195159.7788-1-sbeller@google.com","threadId":"43008","inReplyTo":null,"subject":"[PATCH 0/6] git clone: Marry --recursive and --reference","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-08-04T19:51:53Z","receivedAt":"2016-08-04T19:52:12Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":" 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":"293124","messageId":"20160804195159.7788-3-sbeller@google.com","threadId":"43008","inReplyTo":"20160804195159.7788-1-sbeller@google.com","subject":"[PATCH 2/6] t7408: merge short tests, factor out testing method","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-08-04T19:51:55Z","receivedAt":"2016-08-04T19:52:16Z","isPatch":true,"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 | 50 +++++++++++++++---------------------------\n 1 file changed, 18 insertions(+), 32 deletions(-)\n\ndiff --git a/t/t7408-submodule-reference.sh b/t/t7408-submodule-reference.sh\nindex afcc629..1416cbd 100755\n--- a/t/t7408-submodule-reference.sh\n+++ b/t/t7408-submodule-reference.sh\n@@ -10,6 +10,16 @@ base_dir=$(pwd)\n \n U=$base_dir/UPLOAD_LOG\n \n+test_alternate_usage()\n+{\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 >current &&\n+\tdiff expect current\n+}\n+\n test_expect_success 'preparing first repository' '\n \ttest_create_repo A &&\n \t(\n@@ -42,44 +52,20 @@ 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-'\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+\t) &&\n+\ttest_alternate_usage super/.git/modules/sub/objects/info/alternates super/sub\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+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_usage super-clone/.git/modules/sub/objects/info/alternates super-clone/sub\n '\n \n test_done\n-- \n2.9.2.572.g9d9644e.dirty\n\n"},{"id":"293125","messageId":"20160804195159.7788-5-sbeller@google.com","threadId":"43008","inReplyTo":"20160804195159.7788-1-sbeller@google.com","subject":"[PATCH 4/6] submodule--helper update-clone: allow multiple references","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-08-04T19:51:57Z","receivedAt":"2016-08-04T19:52:20Z","isPatch":true,"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\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 896a3ec..b6f297b 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -583,7 +583,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@@ -599,7 +599,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@@ -709,8 +710,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@@ -829,7 +833,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 2b23ce6..526ea5d 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.572.g9d9644e.dirty\n\n"},{"id":"293126","messageId":"20160804195159.7788-6-sbeller@google.com","threadId":"43008","inReplyTo":"20160804195159.7788-1-sbeller@google.com","subject":"[PATCH 5/6] submodule update: add super-reference flag","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-08-04T19:51:58Z","receivedAt":"2016-08-04T19:52:24Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Signed-off-by: Stefan Beller <sbeller@google.com>\n---\n builtin/submodule--helper.c | 14 +++++++++++++-\n git-submodule.sh            | 10 ++++++++++\n 2 files changed, 23 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex b6f297b..707f201 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -584,6 +584,7 @@ struct submodule_update_clone {\n \tint quiet;\n \tint recommend_shallow;\n \tstruct string_list references;\n+\tstruct string_list superreferences;\n \tconst char *depth;\n \tconst char *recursive_prefix;\n \tconst char *prefix;\n@@ -600,7 +601,7 @@ struct submodule_update_clone {\n };\n #define SUBMODULE_UPDATE_CLONE_INIT {0, MODULE_LIST_INIT, 0, \\\n \tSUBMODULE_UPDATE_STRATEGY_INIT, 0, -1, STRING_LIST_INIT_DUP, \\\n-\tNULL, NULL, NULL, \\\n+\tSTRING_LIST_INIT_DUP, NULL, NULL, NULL, \\\n \tSTRING_LIST_INIT_DUP, 0, NULL, 0, 0}\n \n \n@@ -715,6 +716,15 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce,\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->superreferences.nr) {\n+\t\tstruct string_list_item *item;\n+\t\tfor_each_string_list_item(item, &suc->superreferences) {\n+\t\t\tstrbuf_reset(&sb);\n+\t\t\targv_array_pushf(&child->args, \"--reference=%s/%s\",\n+\t\t\t\t\t relative_path(item->string, suc->prefix, &sb),\n+\t\t\t\t\t sub->path);\n+\t\t}\n+\t}\n \tif (suc->depth)\n \t\targv_array_push(&child->args, suc->depth);\n \n@@ -835,6 +845,8 @@ static int update_clone(int argc, const char **argv, const char *prefix)\n \t\t\t   N_(\"rebase, merge, checkout or none\")),\n \t\tOPT_STRING_LIST(0, \"reference\", &suc.references, N_(\"repo\"),\n \t\t\t   N_(\"reference repository\")),\n+\t\tOPT_STRING_LIST(0, \"super-reference\", &suc.superreferences, N_(\"repo\"),\n+\t\t\t   N_(\"superproject of a reference repository\")),\n \t\tOPT_STRING(0, \"depth\", &suc.depth, \"<depth>\",\n \t\t\t   N_(\"Create a shallow clone truncated to the \"\n \t\t\t      \"specified number of revisions\")),\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex 526ea5d..99d45c8 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -34,6 +34,7 @@ command=\n branch=\n force=\n reference=\n+superreference=\n cached=\n recursive=\n init=\n@@ -520,6 +521,14 @@ cmd_update()\n \t\t--reference=*)\n \t\t\treference=\"$1\"\n \t\t\t;;\n+\t\t--super-reference)\n+\t\t\tcase \"$2\" in '') usage ;; esac\n+\t\t\tsuperreference=\"--super-reference=$2\"\n+\t\t\tshift\n+\t\t\t;;\n+\t\t--super-reference=*)\n+\t\t\tsuperreference=\"$1\"\n+\t\t\t;;\n \t\t-m|--merge)\n \t\t\tupdate=\"merge\"\n \t\t\t;;\n@@ -576,6 +585,7 @@ cmd_update()\n \t\t${prefix:+--recursive-prefix \"$prefix\"} \\\n \t\t${update:+--update \"$update\"} \\\n \t\t${reference:+\"$reference\"} \\\n+\t\t${superreference:+\"$superreference\"} \\\n \t\t${depth:+--depth \"$depth\"} \\\n \t\t${recommend_shallow:+\"$recommend_shallow\"} \\\n \t\t${jobs:+$jobs} \\\n-- \n2.9.2.572.g9d9644e.dirty\n\n"},{"id":"293127","messageId":"20160804195159.7788-2-sbeller@google.com","threadId":"43008","inReplyTo":"20160804195159.7788-1-sbeller@google.com","subject":"[PATCH 1/6] t7408: modernize style","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-08-04T19:51:54Z","receivedAt":"2016-08-04T19:52:27Z","isPatch":true,"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 | 138 +++++++++++++++++++++--------------------\n 1 file changed, 71 insertions(+), 67 deletions(-)\n\ndiff --git a/t/t7408-submodule-reference.sh b/t/t7408-submodule-reference.sh\nindex eaea19b..afcc629 100755\n--- a/t/t7408-submodule-reference.sh\n+++ b/t/t7408-submodule-reference.sh\n@@ -10,72 +10,76 @@ 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.572.g9d9644e.dirty\n\n"},{"id":"293128","messageId":"20160804195159.7788-4-sbeller@google.com","threadId":"43008","inReplyTo":"20160804195159.7788-1-sbeller@google.com","subject":"[PATCH 3/6] submodule--helper module-clone: allow multiple references","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-08-04T19:51:56Z","receivedAt":"2016-08-04T19:52:30Z","isPatch":true,"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 | 19 +++++++++++--------\n 1 file changed, 11 insertions(+), 8 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex b22352b..896a3ec 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,11 @@ 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\", 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 +473,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 +494,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 +531,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.572.g9d9644e.dirty\n\n"},{"id":"293129","messageId":"20160804195159.7788-7-sbeller@google.com","threadId":"43008","inReplyTo":"20160804195159.7788-1-sbeller@google.com","subject":"[PATCH 6/6] clone: reference flag is used for submodules as well","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-08-04T19:51:59Z","receivedAt":"2016-08-04T19:52:33Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"When giving a --reference while also giving --recurse, the alternates\nfor the submodules are assumed to be in the superproject as well.\n\nIn case they are not, we error out when cloning the submodule.\nHowever the update command succeeds as usual (with no alternates).\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n builtin/clone.c                | 22 ++++++++++++++++++----\n t/t7408-submodule-reference.sh | 31 ++++++++++++++++++++++++++++++-\n 2 files changed, 48 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex f044a8c..f586df5 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_reference = STRING_LIST_INIT_NODUP;\n+static struct string_list superreferences = STRING_LIST_INIT_DUP;\n static int option_dissociate;\n static int max_jobs = -1;\n \n@@ -280,10 +281,11 @@ static void strip_trailing_slashes(char *dir)\n \t*end = '\\0';\n }\n \n-static int add_one_reference(struct string_list_item *item, void *cb_data)\n+static int add_one_reference(struct string_list_item *item, void *cb_dir)\n {\n \tchar *ref_git;\n \tconst char *repo;\n+\tconst char *dir = cb_dir;\n \tstruct strbuf alternate = STRBUF_INIT;\n \n \t/* Beware: read_gitfile(), real_path() and mkpath() return static buffer */\n@@ -316,6 +318,13 @@ static int add_one_reference(struct string_list_item *item, void *cb_data)\n \tif (!access(mkpath(\"%s/info/grafts\", ref_git), F_OK))\n \t\tdie(_(\"reference repository '%s' is grafted\"), item->string);\n \n+\tif (option_recursive) {\n+\t\tstruct strbuf sb = STRBUF_INIT;\n+\t\tchar *rel = xstrdup(relative_path(item->string, dir, &sb));\n+\t\tstring_list_append(&superreferences, rel);\n+\t\tstrbuf_reset(&sb);\n+\t}\n+\n \tstrbuf_addf(&alternate, \"%s/objects\", ref_git);\n \tadd_to_alternates_file(alternate.buf);\n \tstrbuf_release(&alternate);\n@@ -323,9 +332,9 @@ static int add_one_reference(struct string_list_item *item, void *cb_data)\n \treturn 0;\n }\n \n-static void setup_reference(void)\n+static void setup_reference(char *dir)\n {\n-\tfor_each_string_list(&option_reference, add_one_reference, NULL);\n+\tfor_each_string_list(&option_reference, add_one_reference, dir);\n }\n \n static void copy_alternates(struct strbuf *src, struct strbuf *dst,\n@@ -736,6 +745,7 @@ static int checkout(void)\n \n \tif (!err && option_recursive) {\n \t\tstruct argv_array args = ARGV_ARRAY_INIT;\n+\t\tstatic struct string_list_item *item;\n \t\targv_array_pushl(&args, \"submodule\", \"update\", \"--init\", \"--recursive\", NULL);\n \n \t\tif (option_shallow_submodules == 1)\n@@ -744,6 +754,10 @@ static int checkout(void)\n \t\tif (max_jobs != -1)\n \t\t\targv_array_pushf(&args, \"--jobs=%d\", max_jobs);\n \n+\t\tif (superreferences.nr)\n+\t\t\tfor_each_string_list_item(item, &superreferences)\n+\t\t\t\targv_array_pushf(&args, \"--super-reference=%s\", item->string);\n+\n \t\terr = run_command_v_opt(args.argv, RUN_GIT_CMD);\n \t\targv_array_clear(&args);\n \t}\n@@ -978,7 +992,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \tstrbuf_reset(&key);\n \n \tif (option_reference.nr)\n-\t\tsetup_reference();\n+\t\tsetup_reference(dir);\n \n \tfetch_pattern = value.buf;\n \trefspec = parse_fetch_refspec(1, &fetch_pattern);\ndiff --git a/t/t7408-submodule-reference.sh b/t/t7408-submodule-reference.sh\nindex 1416cbd..2652cfe 100755\n--- a/t/t7408-submodule-reference.sh\n+++ b/t/t7408-submodule-reference.sh\n@@ -56,7 +56,8 @@ 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\tgit commit -m B-super-added &&\n+\t\tgit repack -ad\n \t) &&\n \ttest_alternate_usage super/.git/modules/sub/objects/info/alternates super/sub\n '\n@@ -68,4 +69,32 @@ test_expect_success 'updating superproject keeps alternates' '\n \ttest_alternate_usage 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_usage .git/objects/info/alternates . &&\n+\t\t# test submodule has correct setup\n+\t\ttest_alternate_usage .git/modules/sub/objects/info/alternates sub\n+\t)\n+'\n+\n+test_expect_success 'cloning superproject, missing submodule alternates' '\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_usage .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_usage .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.572.g9d9644e.dirty\n\n"},{"id":"293222","messageId":"CAGZ79kY4YxftihTP_cNqLrVTn0wrfNd5_mb40AB-t2beyzvdLA@mail.gmail.com","threadId":"43008","inReplyTo":"20160804195159.7788-5-sbeller@google.com","subject":"Re: [PATCH 4/6] submodule--helper update-clone: allow multiple references","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-08-05T19:08:46Z","receivedAt":"2016-08-05T19:08:54Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"> -               ${reference:+--reference \"$reference\"} \\\n> +               ${reference:+\"$reference\"} \\\n\nNote how this changed the API of the submodule--helper.\nCurrently we pass in --reference $reference\nand $reference consists of the string \"--reference\" and the actual\nreference. So it looked like '--reference' '--reference=foo'\n\nThat is why we can pass the argument unseen to clone in the helper\nvia\n\n    argv_array_push(&child->args, suc->reference);\n\nThis is fixed now.\n"},{"id":"293225","messageId":"xmqqoa57vvzl.fsf@gitster.mtv.corp.google.com","threadId":"43008","inReplyTo":"20160804195159.7788-1-sbeller@google.com","subject":"Re: [PATCH 0/6] git clone: Marry --recursive and --reference","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-08-05T19:47:26Z","receivedAt":"2016-08-05T19:47:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n>  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\nI am a bit fuzzy about what scenario you are trying to help.  Is it\nsomething like this? [*1*]\n\n * The superproject is at git://frotz.xz/super.git but you locally\n   have a mirror at /var/cache/super.git (bare) repository.\n\n * \"git clone --refererence /var/cache/super.git git://frotz.xz/super.git\"\n   would borrow that existing local mirror to cut down the traffic.\n\n * The super.git project uses git://nitfol.xz/xyzzy.git as its\n   submodule.  You locally have a mirror of it at\n   /var/cache/xyzzy.git (bare) repository.\n\n * The xyzzy submodule is bound at libs/xyzzy in the superproject\n   tree.\n\n * You want the \"clone\" command above with \"--recursive\" to do \"the\n   right thing\".  That is, the clone of the superproject borrows\n   from /var/cache/super.git local mirror, and the clone of xyzzy\n   that would be made at .git/modules/xyzzy in the superproject\n   would borrow from /var/cache/xyzzy.git local mirror.\n\nWhat I am not sure about is how /var/cache/xyzzy.git should be\nautomatically derived from the information given from the command\nline of \"clone\" and what the clone of the superproject contains.\nYou'd need to make certain assumptions, like\n\n - The parent directory of the --reference for superproject,\n   i.e. /var/cache/, is the directry that has many other mirrors;\n\n - The .gitmodules file in superproject says a \"xyzzy\" project from\n   git://nitfol.xz/xyzzy.git must be checked out at \"libs/xyzzy\",\n   and it is a reasonable assumption that we would find a local\n   mirror at /var/cache/xyzzy.git (the /var/cache prefix comes from\n   the above assumptino).\n\nThere may be some other assumptions you are making.  I am wondering\nif your specific assumption is applicable generally.\n\n\n[Footnote]\n\n*1* Another common arrangement could be that you have one full clone\n    of the superproject at /var/cache/super.git and its submodule\n    xyzzy in /var/cache/super.git/modules/xyzzy and assuming that\n    would allow you to guess where to borrow submodules from.  But\n    the result of assuming this layout would already be different\n    from the scenario I gave the above.  If the organization uses\n    the xyzzy project only in the context of the frotz superproject,\n    \"borrow from submodule repository inside $GIT_DIR/modules/ of\n    the superproject\" may be sensible, but that makes use of \"xyzzy\"\n    as an independent project quite awkward (also you may have\n    another superproject that is unrelated to super.git that uses\n    the same \"xyzzy\" project as its submodule).  The more \"xyzzy\"\n    project is suitable to be used as \"submodule\", the more it makes\n    sense to have its mirror independent from the superprojects that\n    use it, by using /var/cache/{super,xyzzy}.git layout.\n\n    IOW, both layouts are equally sensible; what layout (either one\n    of the above two, or something entirely different) is your \"at\n    the same paths\" assumption meant to serve well, and what is the\n    plan to serve other layouts?\n\n\n\n"},{"id":"293228","messageId":"xmqqk2fvvu0c.fsf@gitster.mtv.corp.google.com","threadId":"43008","inReplyTo":"20160804195159.7788-2-sbeller@google.com","subject":"Re: [PATCH 1/6] t7408: modernize style","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-08-05T20:30:11Z","receivedAt":"2016-08-05T20:30:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> 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\nAll look sensible; I think it is OK to also do minor and trivial\ntweaks here, but let's see what 2/6 does.\n"},{"id":"293232","messageId":"xmqqeg63vtar.fsf@gitster.mtv.corp.google.com","threadId":"43008","inReplyTo":"20160804195159.7788-3-sbeller@google.com","subject":"Re: [PATCH 2/6] t7408: merge short tests, factor out testing method","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-08-05T20:45:32Z","receivedAt":"2016-08-05T20:45:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> 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\nGood motivations.\n\n> Signed-off-by: Stefan Beller <sbeller@google.com>\n> ---\n>  t/t7408-submodule-reference.sh | 50 +++++++++++++++---------------------------\n>  1 file changed, 18 insertions(+), 32 deletions(-)\n>\n> diff --git a/t/t7408-submodule-reference.sh b/t/t7408-submodule-reference.sh\n> index afcc629..1416cbd 100755\n> --- a/t/t7408-submodule-reference.sh\n> +++ b/t/t7408-submodule-reference.sh\n> @@ -10,6 +10,16 @@ base_dir=$(pwd)\n>  \n>  U=$base_dir/UPLOAD_LOG\n\nIs this line needed anywhere?\n\nWe (perhaps unfortunately) still need $base_dir because we want to\ngive an absolute file:/// URL to \"submodule add\", but I do not think\nwe use $U, so let's get rid of it.\n\n> +test_alternate_usage()\n> +{\n\nAccording to Documentation/CodingGuidelines, this should be:\n\n    test_alternate_usage () {\n\nSomehow the helper name sounds as if it is testing if an alternate\nis used correctly (i.e. the machinery may attempt to use alternate\nbut not in a correct way), not testing if an alternate is correctly\nused (i.e. the machinery incorrectly forgets to use an alternate at\nall), but maybe it is just me.\n\n> +\talternates_file=$1\n> +\tworking_dir=$2\n\nThese are good (they can be on a single line), but you would\nwant &&-chain just as other lines.\n\n> +\ttest_line_count = 1 $alternates_file &&\n\nThis needs to quote \"$alternates_file\" especially in a helper\nfunction you have no control over future callers of.\n\nI wonder if we want to check the actual contents of the alternate;\nit may force us to worry about the infamous \"should we expect\n$(pwd)/something or $PWD/something\" if we did so, so it is not a\nstrong suggestion.\n\n> +\techo \"0 objects, 0 kilobytes\" >expect &&\n> +\tgit -C $working_dir count-objects >current &&\n> +\tdiff expect current\n\nIt is more customary to name two \"expect\" vs \"actual\", and compare\nthem using \"test_cmp\" not \"diff\".\n\n> +}\n> +\n>  test_expect_success 'preparing first repository' '\n>  \ttest_create_repo A &&\n>  \t(\n> @@ -42,44 +52,20 @@ 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> -'\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\nCompletely unrelated tangent, but after seeing the \"how would we\nmake a more intelligent choice of the diff boundary\" topic, I\nwondered if we can notice that at this point there is a logical\nboundary and do something intelligent about it.  All the removed\nlines above have become \"test_alternate\" we see below, while all the\nremoved lines below upto \"test_alternate\" correspond to the updated\ntest at the end.\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> +\t) &&\n> +\ttest_alternate_usage super/.git/modules/sub/objects/info/alternates super/sub\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> +test_expect_success 'updating superproject keeps alternates' '\n> +\ttest_when_finished \"rm -rf super-clone\" &&\n\nThis one is new; we do not remove A, B or super.  Does it matter if\nwe leave super-clone behind?  Is super-clone so special?\n\n> +\tgit clone super super-clone &&\n> +\tgit -C super-clone submodule update --init --reference ../B &&\n> +\ttest_alternate_usage super-clone/.git/modules/sub/objects/info/alternates super-clone/sub\n>  '\n>  \n>  test_done\n"},{"id":"293233","messageId":"xmqqa8grvswj.fsf@gitster.mtv.corp.google.com","threadId":"43008","inReplyTo":"20160804195159.7788-4-sbeller@google.com","subject":"Re: [PATCH 3/6] submodule--helper module-clone: allow multiple references","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-08-05T20:54:04Z","receivedAt":"2016-08-05T20:54:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> Allow users to pass in multiple references, just as clone accepts multiple\n> references as well.\n\nAs these are passed-thru to the underlying \"git clone\", this change\nmakes perfect sense to me.\n\nWill queue.\n\n"},{"id":"293239","messageId":"xmqq60rfvsbq.fsf@gitster.mtv.corp.google.com","threadId":"43008","inReplyTo":"CAGZ79kY4YxftihTP_cNqLrVTn0wrfNd5_mb40AB-t2beyzvdLA@mail.gmail.com","subject":"Re: [PATCH 4/6] submodule--helper update-clone: allow multiple references","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-08-05T21:06:33Z","receivedAt":"2016-08-05T21:07:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n>> -               ${reference:+--reference \"$reference\"} \\\n>> +               ${reference:+\"$reference\"} \\\n>\n> Note how this changed the API of the submodule--helper.\n> Currently we pass in --reference $reference\n> and $reference consists of the string \"--reference\" and the actual\n> reference. So it looked like '--reference' '--reference=foo'\n\nSo this change is a strict bugfix.  The code without this patch\nhad cmd_update that places \"--reference=foo\" in $reference, but\nthe call to \"git submodule--helper\" it makes added an unnecessary\n\"--reference\" in front of it.\n\nI was wondering why there is no corresponding change to add\n\"--reference\" on the code that calls cmd_update().  Thanks for\nsaving my time ;-)\n\nPerhaps that needs to go into the log message, though.  Allowing\nmultiple references is not that interesting from the POV of the\ncodebase immediately after this step and only deserves a \"by the\nway\" mention.\n\n    Subject: submodule--helper: fix cmd_update()\n\n    cmd_update places \"--reference=foo\" in $reference, but the call to\n    \"git submodule--helper\" it makes adds an unnecessary \"--reference\"\n    in front of it.  The underlying \"git clone\" was called with two\n    command options \"--reference\" and \"--reference=foo\" because of\n    this.\n\n    While at it, prepare the code to accept multiple references, as\n    the underlying \"git clone\" can happily take more than one.\n\nor something like that.\n\nNeedless to say, \"While at it\" could become a separate step, and a\npure bugfix part may even want to become a separate and more urgent\ntopic that can go to 'maint', with a test to prevent regression.\n"},{"id":"293242","messageId":"xmqq1t22x6fj.fsf@gitster.mtv.corp.google.com","threadId":"43008","inReplyTo":"20160804195159.7788-6-sbeller@google.com","subject":"Re: [PATCH 5/6] submodule update: add super-reference flag","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-08-05T21:16:32Z","receivedAt":"2016-08-05T21:16:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> Signed-off-by: Stefan Beller <sbeller@google.com>\n> ---\n\nThat's a bit sketchy description.  From the title, I expected that\nwe would see one additional 'unsigned super_reference : 1;' field in\nsome structure, but that is not what is happening.  The log message\nneeds to describe what these string list are and why they are needed\na bit better.\n\nAt least, please don't name a multiple_word field \"multipleword\" ;-)\n\n> @@ -715,6 +716,15 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce,\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->superreferences.nr) {\n> +\t\tstruct string_list_item *item;\n> +\t\tfor_each_string_list_item(item, &suc->superreferences) {\n> +\t\t\tstrbuf_reset(&sb);\n> +\t\t\targv_array_pushf(&child->args, \"--reference=%s/%s\",\n> +\t\t\t\t\t relative_path(item->string, suc->prefix, &sb),\n> +\t\t\t\t\t sub->path);\n\nThe phrase \"super reference\" made me imagine it is some kind of\n\"reference\" that is applicable to the superproject, but this code\nsmells like it is a \"prefix to create reference suited for use in\nsubmodule\".  Whatever it is, it should be explained better (than \"no\ndesciption\" which is what we have here ;-), and given a name that\nmatch the explanation.\n\nThanks.\n"},{"id":"293243","messageId":"CAGZ79kbTcHN+18z+eXx+QDOCXL9vKeGYKukQJJ+ZnxT55_8htg@mail.gmail.com","threadId":"43008","inReplyTo":"xmqq60rfvsbq.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 4/6] submodule--helper update-clone: allow multiple references","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-08-05T21:19:40Z","receivedAt":"2016-08-05T21:19:50Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, Aug 5, 2016 at 2:06 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Stefan Beller <sbeller@google.com> writes:\n>\n>>> -               ${reference:+--reference \"$reference\"} \\\n>>> +               ${reference:+\"$reference\"} \\\n>>\n>> Note how this changed the API of the submodule--helper.\n>> Currently we pass in --reference $reference\n>> and $reference consists of the string \"--reference\" and the actual\n>> reference. So it looked like '--reference' '--reference=foo'\n>\n> So this change is a strict bugfix.  The code without this patch\n> had cmd_update that places \"--reference=foo\" in $reference, but\n> the call to \"git submodule--helper\" it makes added an unnecessary\n> \"--reference\" in front of it.\n\nI thought about rolling it as a strict bugfix; but the bug is shaded by the\ninverse bug in the helper, so the user would never see an issue.\n\nIt is more a cleanup of the internal communication between the shell\nscript and the helper written in C. (And that communication is\nsupposed to not be user visible or relevant)\n\n>\n> I was wondering why there is no corresponding change to add\n> \"--reference\" on the code that calls cmd_update().  Thanks for\n> saving my time ;-)\n\nWhen I wrote the patch I cared more about the while-at-it part than\nthe bug fix as I do not consider the bug worth to merge down to maint\nor even fast tracking it as a bug fix. It's just changing the internal\ncommunication to be clearer.\n\n>\n> Perhaps that needs to go into the log message, though.  Allowing\n> multiple references is not that interesting from the POV of the\n> codebase immediately after this step and only deserves a \"by the\n> way\" mention.\n>\n>     Subject: submodule--helper: fix cmd_update()\n>\n>     cmd_update places \"--reference=foo\" in $reference, but the call to\n>     \"git submodule--helper\" it makes adds an unnecessary \"--reference\"\n>     in front of it.  The underlying \"git clone\" was called with two\n>     command options \"--reference\" and \"--reference=foo\" because of\n>     this.\n>\n>     While at it, prepare the code to accept multiple references, as\n>     the underlying \"git clone\" can happily take more than one.\n>\n> or something like that.\n>\n> Needless to say, \"While at it\" could become a separate step, and a\n> pure bugfix part may even want to become a separate and more urgent\n> topic that can go to 'maint', with a test to prevent regression.\n\nSure. I'll do so in a reroll.\n"},{"id":"293247","messageId":"CAGZ79kY9Wry-vu9ByzW7Qc37SSkKf3doyWuzGzQeMTy4NfLMhA@mail.gmail.com","threadId":"43008","inReplyTo":"xmqqoa57vvzl.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 0/6] git clone: Marry --recursive and --reference","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-08-05T21:23:49Z","receivedAt":"2016-08-05T21:24:14Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, Aug 5, 2016 at 12:47 PM, Junio C Hamano <gitster@pobox.com> wrote:\n>  * You want the \"clone\" command above with \"--recursive\" to do \"the\n>    right thing\".  That is, the clone of the superproject borrows\n>    from /var/cache/super.git local mirror, and the clone of xyzzy\n>    that would be made at .git/modules/xyzzy in the superproject\n>    would borrow from /var/cache/xyzzy.git local mirror.\n\nThis is not what I intend to solve here. The solution in 6/6 solves\nthe scenario as you outlined in [1].\n\n>\n> What I am not sure about is how /var/cache/xyzzy.git should be\n> automatically derived from the information given from the command\n> line of \"clone\" and what the clone of the superproject contains.\n\nGenerally speaking you cannot do that without assumptions.\n\nThe scenario in [1] can be done without assumptions of the locations\nof the submodules. The only requirement for [1] is to have submodules\nchecked out, which is a rather strong requirement, as that doesn't\nhelp you when you want to reference multiple superrpojects with\nincomplete submodule checkout. (Given all of them together may or\nmay not produce the full set of references)\n\n>\n>     IOW, both layouts are equally sensible; what layout (either one\n>     of the above two, or something entirely different) is your \"at\n>     the same paths\" assumption meant to serve well, and what is the\n>     plan to serve other layouts?\n>\n\nThe plan for other layouts might be\n\n    git submodule update --reference-dir /var/cache/\n\n?\n"},{"id":"293251","messageId":"xmqqwpjuvr6i.fsf@gitster.mtv.corp.google.com","threadId":"43008","inReplyTo":"CAGZ79kbTcHN+18z+eXx+QDOCXL9vKeGYKukQJJ+ZnxT55_8htg@mail.gmail.com","subject":"Re: [PATCH 4/6] submodule--helper update-clone: allow multiple references","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-08-05T21:31:17Z","receivedAt":"2016-08-05T21:31:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> I thought about rolling it as a strict bugfix; but the bug is shaded by the\n> inverse bug in the helper, so the user would never see an issue.\n\nAhh, OK, because the helper accepts \"--reference\" \"--reference=foo\"\nas a OPT_STRING whose value happens to be \"--reference=foo\", and\nthen uses\n\n\tif (suc->reference)\n        \targv_array_push(&child->args, suc->reference)\n\nwhere suc->reference _is_ \"--reference=foo\" when invoking the\nunderlying \"git clone\", it cancels out.\n\nThen it is OK.\n\nIn fact there is NO bug.  It just is that update_clone subcommand\nused a convention different from others that took the whole\n--option=arg as a parameter to --reference option.  It could be\nargued that it is an API bug between git-submodule.sh and\ngit-submodule--helper, but nobody else goes through this \"weird\"\ninterface, so it is not worth splitting the patch.\n"},{"id":"293252","messageId":"CAGZ79kYpvzXOA_X9yYO+rBV3q5WwRnSmExU7JdwotC6cuZ_YKA@mail.gmail.com","threadId":"43008","inReplyTo":"xmqqwpjuvr6i.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 4/6] submodule--helper update-clone: allow multiple references","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-08-05T21:33:57Z","receivedAt":"2016-08-05T21:34:20Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, Aug 5, 2016 at 2:31 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Stefan Beller <sbeller@google.com> writes:\n>\n>> I thought about rolling it as a strict bugfix; but the bug is shaded by the\n>> inverse bug in the helper, so the user would never see an issue.\n>\n> Ahh, OK, because 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> Then it is OK.\n>\n> In fact there is NO bug.  It just is that update_clone subcommand\n> used a convention different from others that took the whole\n> --option=arg as a parameter to --reference option.  It could be\n> argued that it is an API bug between git-submodule.sh and\n> git-submodule--helper, but nobody else goes through this \"weird\"\n> interface, so it is not worth splitting the patch.\n\n\nI'll mention the fix of the API bug in the reroll then.\n"},{"id":"293254","messageId":"xmqqshuivqxp.fsf@gitster.mtv.corp.google.com","threadId":"43008","inReplyTo":"20160804195159.7788-7-sbeller@google.com","subject":"Re: [PATCH 6/6] clone: reference flag is used for submodules as well","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-08-05T21:36:34Z","receivedAt":"2016-08-05T21:36:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> When giving a --reference while also giving --recurse, the alternates\n> for the submodules are assumed to be in the superproject as well.\n>\n> In case they are not, we error out when cloning the submodule.\n> However the update command succeeds as usual (with no alternates).\n\nI covered most of what I want to say on this in my reply to 0/6; I\ndo not have strong objection against what single layout you chose to\nsupport, nor I have strong opinion on which single layout we should\nsupport by default, or what mechanism, if any, we should give users\nto specify different layout.\n\nBut please make sure the choice you made is explained for the users.\nThe end-user documentation should talk about the effect of giving\nthese two options together.\n\nThanks.\n"},{"id":"293256","messageId":"xmqqoa56vqfd.fsf@gitster.mtv.corp.google.com","threadId":"43008","inReplyTo":"CAGZ79kY9Wry-vu9ByzW7Qc37SSkKf3doyWuzGzQeMTy4NfLMhA@mail.gmail.com","subject":"Re: [PATCH 0/6] git clone: Marry --recursive and --reference","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-08-05T21:47:34Z","receivedAt":"2016-08-05T21:47:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> The plan for other layouts might be\n>\n>     git submodule update --reference-dir /var/cache/\n\nThat is not a plan for \"other layouts\", but a plan for \"the other\nlayout that was mentioned as a possibility\".  As I said, both\nlayouts are equally plausible, and I do not know if we need to plan\nfor layouts other than these two, but we should consider if we want\nto add one new option every time we find a new layout we need to\nsupport, or we want a general framework that is more flexible and\nallows us to make the \"borrow from the $GIT_DIR/modules/ of the\nrepository the superproject borrows from\" a mere special case of it.\n\nThanks.\n\n"},{"id":"293276","messageId":"xmqqwpjuu82s.fsf@gitster.mtv.corp.google.com","threadId":"43008","inReplyTo":"CAGZ79kZChXebtSJeTPGoZgnoKySDrWnD11NX7fyze+rOU=Jwvg@mail.gmail.com","subject":"Re: [PATCH 2/6] t7408: merge short tests, factor out testing method","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-08-05T23:09:15Z","receivedAt":"2016-08-06T20:05:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n>>> -test_expect_success 'that reference gets used with add' '\n>>> -     (\n>>> -             cd super/sub &&\n>>> -             echo \"0 objects, 0 kilobytes\" > expected &&\n>>> -             git count-objects > current &&\n>>> -             diff expected current\n>\n> This is where the \"diff\" and \"current\" above came from.\n\nI KNOW that, but I thought this effort is a continuation of the\nmodernise-and-make-it-maintainable effort started at 1/6.\n\n> To stop that from happening again I want to have a clean slate,\n> i.e. all clones are deleted shortly after using, so it is obvious what\n> to use for testing.\n\nOK.  So A, B and super (and super/sub) are the ones that survive\nthroughout the test, and individual test (including the \"let's try\ncloning the super into super-clone\" at the end of existing tests)\nwould follow create-use-remove pattern?  Perhaps that can be left\nas a comment before the last existing test, e.g. \"A, B and super are\npermanent, and from here on, if you create a clone of them for\ntesting, remove your clone once you are done\", to inform the test\nwriters of this convention.\n\nThanks.\n"},{"id":"293298","messageId":"CAGZ79kZChXebtSJeTPGoZgnoKySDrWnD11NX7fyze+rOU=Jwvg@mail.gmail.com","threadId":"43008","inReplyTo":"xmqqeg63vtar.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 2/6] t7408: merge short tests, factor out testing method","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-08-05T22:45:13Z","receivedAt":"2016-08-06T20:47:05Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, Aug 5, 2016 at 1:45 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Stefan Beller <sbeller@google.com> writes:\n>\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> Good motivations.\n>\n>> Signed-off-by: Stefan Beller <sbeller@google.com>\n>> ---\n>>  t/t7408-submodule-reference.sh | 50 +++++++++++++++---------------------------\n>>  1 file changed, 18 insertions(+), 32 deletions(-)\n>>\n>> diff --git a/t/t7408-submodule-reference.sh b/t/t7408-submodule-reference.sh\n>> index afcc629..1416cbd 100755\n>> --- a/t/t7408-submodule-reference.sh\n>> +++ b/t/t7408-submodule-reference.sh\n>> @@ -10,6 +10,16 @@ base_dir=$(pwd)\n>>\n>>  U=$base_dir/UPLOAD_LOG\n>\n> Is this line needed anywhere?\n>\n> We (perhaps unfortunately) still need $base_dir because we want to\n> give an absolute file:/// URL to \"submodule add\", but I do not think\n> we use $U, so let's get rid of it.\n>\n>> +test_alternate_usage()\n>> +{\n>\n> According to Documentation/CodingGuidelines, this should be:\n>\n>     test_alternate_usage () {\n>\n> Somehow the helper name sounds as if it is testing if an alternate\n> is used correctly (i.e. the machinery may attempt to use alternate\n> but not in a correct way), not testing if an alternate is correctly\n> used (i.e. the machinery incorrectly forgets to use an alternate at\n> all), but maybe it is just me.\n>\n>> +     alternates_file=$1\n>> +     working_dir=$2\n>\n> These are good (they can be on a single line), but you would\n> want &&-chain just as other lines.\n>\n>> +     test_line_count = 1 $alternates_file &&\n>\n> This needs to quote \"$alternates_file\" especially in a helper\n> function you have no control over future callers of.\n>\n> I wonder if we want to check the actual contents of the alternate;\n> it may force us to worry about the infamous \"should we expect\n> $(pwd)/something or $PWD/something\" if we did so, so it is not a\n> strong suggestion.\n>\n>> +     echo \"0 objects, 0 kilobytes\" >expect &&\n>> +     git -C $working_dir count-objects >current &&\n>> +     diff expect current\n>\n> It is more customary to name two \"expect\" vs \"actual\", and compare\n> them using \"test_cmp\" not \"diff\".\n>\n>> +}\n>> +\n>>  test_expect_success 'preparing first repository' '\n>>       test_create_repo A &&\n>>       (\n>> @@ -42,44 +52,20 @@ test_expect_success 'preparing superproject' '\n>>       )\n>>  '\n>>\n>> -test_expect_success 'submodule add --reference' '\n>> +test_expect_success 'submodule add --reference uses alternates' '\n>>       (\n>>               cd super &&\n>>               git submodule add --reference ../B \"file://$base_dir/A\" sub &&\n>>               git commit -m B-super-added\n>> -     )\n>> -'\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>> -\n>> -test_expect_success 'that reference gets used with add' '\n>> -     (\n>> -             cd super/sub &&\n>> -             echo \"0 objects, 0 kilobytes\" > expected &&\n>> -             git count-objects > current &&\n>> -             diff expected current\n\nThis is where the \"diff\" and \"current\" above came from.\n\n>> -     )\n>> -'\n>\n> Completely unrelated tangent, but after seeing the \"how would we\n> make a more intelligent choice of the diff boundary\" topic, I\n> wondered if we can notice that at this point there is a logical\n> boundary and do something intelligent about it.  All the removed\n> lines above have become \"test_alternate\" we see below, while all the\n> removed lines below upto \"test_alternate\" correspond to the updated\n> test at the end.\n\nI guess that would require even more knowledge of the underlying\ncontent that we track in Git.\n\nOriginally I started the diff boundary topic, as I assumed that the\nnew line detection is not doing harm. What Michael came up with\nis impressive though I fear there might be a selection bias in the corpus,\ni.e. we are missing some projects that get worse by it and these projects\nwould have had a great influence on the selection of the tuning parameters.\nI guess we'll just wait until someone speaks up and points at worse\nexamples there.\n\nWhat you're asking here is a complete new ballpark IMHO\nas it takes diff to a whole new (syntactical) level. As of now\nthe world agrees that '\\n' seems to be a good character to\nput in text documents, so we use it as a splitting token\nin our underlying diff driver, i.e. the diffs are primarily by line,\nno matter how many characters change in that line. Sometimes\nthere is a \"secondary\" aspect such as coloring and pointing out\nthe characters that changed in a line[1].\n\n[1] random example:\nhttps://gerrit-review.googlesource.com/#/c/79354/3/project.py\nVisually we focus on the changed characters, but the underlying\ndiff is still \"by line\".\n\nWe could write another \"diff driver\" that would not segment the\ntext by '\\n' as a primary method and then showing the changed\nhunks with +/-, but it would try to find rearrangements in the\nunderlying text:\n\nAs an example, this patch with such a diff driver could read partially as:\n----8<----\n***diff-driver: moved-text line 42 -> 14, length:7, post-operations: 1\n test_expect_success 'that reference gets used with add' '\n      (\n              cd super/sub &&\n*             echo \"0 objects, 0 kilobytes\" > expected &&\n*             git count-objects > current &&\n*             diff expected current\n      )\n***diff-driver:post-operation 1:\n*** modify moved text with:\n\n+test_alternate_usage() {\n+        alternates_file=\"$1\" &&\n+        working_dir=\"$2\" &&\n*             echo \"0 objects, 0 kilobytes\" > expected &&\n*             git count-objects > current &&\n*             diff expected current\n+ }\n\n***diff-driver: deletion 40,50\n\n -     )\n -'\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 -\n----8<----\n\nThinking about this further, this would not require knowledge of\nthe underlying content, it is actually \"just\" an intra-file-rename\ndetection.\n\nWe have a rename detection from one blob to another, so we\ncould also have a similar thing to deduplicate within one blob,\nwhich would track moving code around.\n\n\n>\n>> -test_expect_success 'cloning superproject' '\n>> -     git clone super super-clone\n>> -'\n>> -\n>> -test_expect_success 'update with reference' '\n>> -     cd super-clone && git submodule update --init --reference ../B\n>> -'\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>> +     test_alternate_usage super/.git/modules/sub/objects/info/alternates super/sub\n>>  '\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>> +test_expect_success 'updating superproject keeps alternates' '\n>> +     test_when_finished \"rm -rf super-clone\" &&\n>\n> This one is new; we do not remove A, B or super.  Does it matter if\n> we leave super-clone behind?  Is super-clone so special?\n\n\"We need it in a later test.\"\n\nIt comes down to philosophy of how to write tests.\n\nI spent some time in 740* and this is a surprising short test.\nCompare to 7404 and 7400 for example. These are very long,\nso when you want to add another test (no matter if testing for a\nfixed regression or a new feature), you have lots of repos like\n\n    super, super2, super3, sub, sub1, sumodule, anothersub,\n\nand none of the names make sense. (Can I reuse them?\ndo they have some weird corner case configuration\nthat I need to undo? etc)\n\nTo stop that from happening again I want to have a clean slate,\ni.e. all clones are deleted shortly after using, so it is obvious what\nto use for testing.\n\n>\n>> +     git clone super super-clone &&\n>> +     git -C super-clone submodule update --init --reference ../B &&\n>> +     test_alternate_usage super-clone/.git/modules/sub/objects/info/alternates super-clone/sub\n>>  '\n>>\n>>  test_done\n"},{"id":"293301","messageId":"20160805232643.23837-1-sbeller@google.com","threadId":"43008","inReplyTo":"20160804195159.7788-1-sbeller@google.com","subject":"Rename detection within in files WAS: [PATCH 2/6] t7408: merge short tests, factor out testing method","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-08-05T23:26:43Z","receivedAt":"2016-08-06T21:10:19Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"When moving code around, we usually get large chunks of text. If the contributor\nis not 100% trustworthy, we need to review all the code without much intelectual\njoy. Essentially the reviewer is just making sure the parts of the text are the\nsame.\n\nI'd like to propose a new addition to the diff format that makes this use case\neasier. The idea is to mark up lines that were just moved around in the file\ninstead of adding and removing them.\n\nCurrently we have 3 characters that\nare allowed to start a line within a hunk:\n' ' to indicate context\n'+' to add a line\n'-' to remove a line\n\nI'd propose to add the following characters:\n'*' which is the same as '+', but it indicates that the line was moved\n    from somewhere else without change.\n'X' The same as '-', with the addition that this line was moved to a different\n    place without change.\n\nThe patch below uses these new '*' and 'X'. Each hunk that makes use of these\nadditions, is followed other sections, [moved-from, moved-to] that indicate\nwhere the corresponding line is.\n\nThere are multiple things to tackle when going for such an addition:\n* How to present this to the user (it's covered in this email)\n* how to find the renamed lines algorithmically.\n  (there are already approaches to that, e.g. https://github.com/stefanbeller/duplo\n  which is http://duplo.sourceforge.net/ with no substantial additions)\n\nAny comments welcome,\n\nThanks,\nStefan\n\n---\n t/t7408-submodule-reference.sh | 50 +++++++++++++++---------------------------\n 1 file changed, 15 insertions(+), 29 deletions(-), 6 moved lines\n\ndiff --git a/t/t7408-submodule-reference.sh b/t/t7408-submodule-reference.sh\nindex afcc629..1416cbd 100755\n--- a/t/t7408-submodule-reference.sh\n+++ b/t/t7408-submodule-reference.sh\n@@ -10,6 +10,16 @@ base_dir=$(pwd)\n\n U=$base_dir/UPLOAD_LOG\n\n+test_alternate_usage()\n+{\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 >current &&\n*\tdiff expect current\n+}\n+\n test_expect_success 'preparing first repository' '\n \ttest_create_repo A &&\n \t(\n@@ move-source 42,6 @@ test_expect_success 'that reference gets used with add' '\n test_expect_success 'that reference gets used with add' '\n \t(\n \t\tcd super/sub &&\nX\t\techo \"0 objects, 0 kilobytes\" > expected &&\nX\t\tgit count-objects > current &&\nX\t\tdiff expected current\n \t)\n '\n@@ -42,44 +52,20 @@ 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-'\n-\n-test_expect_success 'that reference gets used with add' '\n-\t(\n-\t\tcd super/sub &&\nX\t\techo \"0 objects, 0 kilobytes\" > expected &&\nX\t\tgit count-objects > current &&\nX\t\tdiff expected current\n-\t)\n-'\n-\n-test_expect_success 'cloning superproject' '\n-\tgit clone super super-clone\n-'\n-\n@@ move-to 10,6 @@ test_alternate_usage\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 >current &&\n*\tdiff expect current\n+}\n+\n--\n2.9.2.572.g9d9644e.dirty\n"},{"id":"293303","messageId":"CAGZ79kYsdqffWykzcEQcd2HigYqVNt0Y1H0Biw3QA2RdQaMOOw@mail.gmail.com","threadId":"43008","inReplyTo":"xmqq1t22x6fj.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 5/6] submodule update: add super-reference flag","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-08-06T00:22:01Z","receivedAt":"2016-08-06T21:49:42Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, Aug 5, 2016 at 2:16 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Stefan Beller <sbeller@google.com> writes:\n>\n>> Signed-off-by: Stefan Beller <sbeller@google.com>\n>> ---\n>\n> That's a bit sketchy description.  From the title, I expected that\n> we would see one additional 'unsigned super_reference : 1;' field in\n> some structure, but that is not what is happening.  The log message\n> needs to describe what these string list are and why they are needed\n> a bit better.\n\nA \"--super-reference\" allows giving a reference to a superproject instead\nof a direct reference.\n\nWith --reference you had to point to the direct repository, e.g.\n\n    git clone --reference /usr/share/foo git://xyxxz.example/foo\n\nassumed that /usr/share/foo is either the working dir or the git dir\nof said project.\n\nWhen giving references in the submodule context (this part of the helper\nis called from \"git submodule update\", which itself is called from\n\"git clone --recursive\"  we do not know the place where the submodule\nreference is living. As we do know where the superproject reference is\nliving (i.e. \"git clone --recursive --reference <superproject>\"),\nwe can construct one possible path where the submodules are located\nat <superproject>/<submodule-path>. To differentiate the reference flag\nfrom a direct reference this will be called \"--super-reference\" as it\nreferences a super project.\n\nThe next patch makes use of it by just passing on the --reference\ngiven to \"git clone\" as --super-reference to the \"submodule update\"\nprocedure, which then uses the code in this patch.\n\n>\n> At least, please don't name a multiple_word field \"multipleword\" ;-)\n\nuh :( OK.\n\n>\n>> @@ -715,6 +716,15 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce,\n>>               for_each_string_list_item(item, &suc->references)\n>>                       argv_array_pushl(&child->args, \"--reference\", item->string, NULL);\n>>       }\n>> +     if (suc->superreferences.nr) {\n>> +             struct string_list_item *item;\n>> +             for_each_string_list_item(item, &suc->superreferences) {\n>> +                     strbuf_reset(&sb);\n>> +                     argv_array_pushf(&child->args, \"--reference=%s/%s\",\n>> +                                      relative_path(item->string, suc->prefix, &sb),\n>> +                                      sub->path);\n>\n> The phrase \"super reference\" made me imagine it is some kind of\n> \"reference\" that is applicable to the superproject,\n\nOh I see. Except that from the superprojects point of view, it is not a\nsuperproject; it is just a project with submodules. From a submodules point\nof view you have a superproject however, so a super-reference is a reference\nthat points to another superproject, so we can find a sibling reference.\n\n> but this code\n> smells like it is a \"prefix to create reference suited for use in\n> submodule\".\n\nYeah that's it.\n\n> Whatever it is, it should be explained better (than \"no\n> desciption\" which is what we have here ;-), and given a name that\n> match the explanation.\n>\n> Thanks.\n\nOK.\n\nThanks,\nStefan\n"},{"id":"293320","messageId":"57A6FE5A.9050803@web.de","threadId":"43008","inReplyTo":"20160805232643.23837-1-sbeller@google.com","subject":"Re: Rename detection within in files WAS: [PATCH 2/6] t7408: merge short tests, factor out testing method","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2016-08-07T09:24:42Z","receivedAt":"2016-08-07T09:24:57Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 06.08.2016 um 01:26 schrieb Stefan Beller:\n> When moving code around, we usually get large chunks of text. If the contributor\n> is not 100% trustworthy, we need to review all the code without much intelectual\n> joy. Essentially the reviewer is just making sure the parts of the text are the\n> same.\n>\n> I'd like to propose a new addition to the diff format that makes this use case\n> easier. The idea is to mark up lines that were just moved around in the file\n> instead of adding and removing them.\n>\n> Currently we have 3 characters that\n> are allowed to start a line within a hunk:\n> ' ' to indicate context\n> '+' to add a line\n> '-' to remove a line\n>\n> I'd propose to add the following characters:\n> '*' which is the same as '+', but it indicates that the line was moved\n>      from somewhere else without change.\n> 'X' The same as '-', with the addition that this line was moved to a different\n>      place without change.\n>\n> The patch below uses these new '*' and 'X'. Each hunk that makes use of these\n> additions, is followed other sections, [moved-from, moved-to] that indicate\n> where the corresponding line is.\n\nInteresting idea. It should be easy to convert the result into a regular \nunified diff for consumption with patch(1) or git am/apply by replacing \nthe new flags with + and - and removing the moved-* hunks.\n\nYour example ignores whitespace changes at the start of the line and \nwithin it, the added \"-C $working_dir\", s/expected/expect/; is this all \nintended?  Only a single blank line was moved verbatim.\n\nThe moved-from and moved-to hunks make this diff quite verbose.\n\nIf multiple lines from different sources are moved to the same hunk then \nyou'd get multiple moved-from hunks following that single destination, \nright?  (Same with lines moved from a single hunk to multiple \ndestinations and moved-to.)\n\nBut does it even warrent a new format? It's a display problem; the \nnecessary information is already in the diffs we have today.  A \ngraphical diff viewer could connect moved blocks with lines, like \nhttp://www.araxis.com/merge/ does in its side-by-side view.  A \nThunderbird extension (or a bookmarklet or browser extendiion for \nwebmail users) could do that for an email-based workflow.\n\nStill, what about adding information about moved lines as an extended \nheader (like that index line)?  Line numbers are included in hunk \nheaders and can serve as orientation.  A reader would have to do some \nmental arithmetic (ugh), but incompatible format changes would be \navoided.  For your example it should look something like this:\n\n\tmove from t/t7408-submodule-reference.sh:52,1\n\tmove to t/t7408-submodule-reference.sh:22,1\n\n>\n> There are multiple things to tackle when going for such an addition:\n> * How to present this to the user (it's covered in this email)\n> * how to find the renamed lines algorithmically.\n>    (there are already approaches to that, e.g. https://github.com/stefanbeller/duplo\n>    which is http://duplo.sourceforge.net/ with no substantial additions)\n>\n> Any comments welcome,\n>\n> Thanks,\n> Stefan\n>\n> ---\n>   t/t7408-submodule-reference.sh | 50 +++++++++++++++---------------------------\n>   1 file changed, 15 insertions(+), 29 deletions(-), 6 moved lines\n>\n> diff --git a/t/t7408-submodule-reference.sh b/t/t7408-submodule-reference.sh\n> index afcc629..1416cbd 100755\n> --- a/t/t7408-submodule-reference.sh\n> +++ b/t/t7408-submodule-reference.sh\n> @@ -10,6 +10,16 @@ base_dir=$(pwd)\n>\n>   U=$base_dir/UPLOAD_LOG\n>\n> +test_alternate_usage()\n> +{\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 >current &&\n> *\tdiff expect current\n> +}\n> +\n\nPost-image line 22.\n\n>   test_expect_success 'preparing first repository' '\n>   \ttest_create_repo A &&\n>   \t(\n> @@ move-source 42,6 @@ test_expect_success 'that reference gets used with add' '\n>   test_expect_success 'that reference gets used with add' '\n>   \t(\n>   \t\tcd super/sub &&\n> X\t\techo \"0 objects, 0 kilobytes\" > expected &&\n> X\t\tgit count-objects > current &&\n> X\t\tdiff expected current\n>   \t)\n>   '\n> @@ -42,44 +52,20 @@ 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\nPre-image line 52.\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> X\t\techo \"0 objects, 0 kilobytes\" > expected &&\n> X\t\tgit count-objects > current &&\n> X\t\tdiff expected current\n> -\t)\n> -'\n> -\n> -test_expect_success 'cloning superproject' '\n> -\tgit clone super super-clone\n> -'\n> -\n> @@ move-to 10,6 @@ test_alternate_usage\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 >current &&\n> *\tdiff expect current\n> +}\n> +\n> --\n> 2.9.2.572.g9d9644e.dirty\n>\n\n"}]}