{"thread":{"id":"51487","subject":"[PATCH] submodule: plumb --filter to cloned submodules","startedAt":"2019-07-17T00:11:56Z","lastAt":"2019-07-24T22:18:21Z","messageCount":2,"participants":["Josh Steadmon","Jonathan Tan"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"378996","messageId":"52bf9d45b8e2b72ff32aa773f2415bf7b2b86da2.1563322192.git.steadmon@google.com","threadId":"51487","inReplyTo":null,"subject":"[PATCH] submodule: plumb --filter to cloned submodules","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2019-07-17T00:11:49Z","receivedAt":"2019-07-17T00:11:56Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"When cloning a repo with a --filter and with --recurse-submodules\nenabled, the partial clone filter only applies to the top-level repo.\nThis can lead to unexpected bandwidth and disk usage for projects which\ninclude large submodules.\n\nFix this by plumbing the --filter argument from git-clone through\ngit-submodule and git-submodule--helper.\n\nSigned-off-by: Josh Steadmon <steadmon@google.com>\n---\n builtin/clone.c                    |  9 +++++++\n builtin/submodule--helper.c        | 41 +++++++++++++++++++++++++-----\n git-submodule.sh                   | 17 ++++++++++++-\n t/t5617-clone-submodules-remote.sh | 19 ++++++++++++++\n 4 files changed, 79 insertions(+), 7 deletions(-)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex a4fe72879d..2e7ecbd019 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -798,6 +798,15 @@ static int checkout(int submodule_progress)\n \t\t\targv_array_push(&args, \"--no-fetch\");\n \t\t}\n \n+\t\tif (filter_options.choice) {\n+\t\t\tstruct strbuf expanded_filter = STRBUF_INIT;\n+\t\t\texpand_list_objects_filter_spec(&filter_options,\n+\t\t\t\t\t\t\t&expanded_filter);\n+\t\t\targv_array_pushf(&args, \"--filter=%s\",\n+\t\t\t\t\t expanded_filter.buf);\n+\t\t\tstrbuf_release(&expanded_filter);\n+\t\t}\n+\n \t\terr = run_command_v_opt(args.argv, RUN_GIT_CMD);\n \t\targv_array_clear(&args);\n \t}\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 909e77e802..1383a5ae74 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -19,6 +19,7 @@\n #include \"diffcore.h\"\n #include \"diff.h\"\n #include \"object-store.h\"\n+#include \"list-objects-filter-options.h\"\n \n #define OPT_QUIET (1 << 0)\n #define OPT_CACHED (1 << 1)\n@@ -1222,7 +1223,8 @@ static int module_deinit(int argc, const char **argv, const char *prefix)\n \n static int clone_submodule(const char *path, const char *gitdir, const char *url,\n \t\t\t   const char *depth, struct string_list *reference, int dissociate,\n-\t\t\t   int quiet, int progress)\n+\t\t\t   int quiet, int progress,\n+\t\t\t   const struct list_objects_filter_options *filter_options)\n {\n \tstruct child_process cp = CHILD_PROCESS_INIT;\n \n@@ -1244,6 +1246,12 @@ static int clone_submodule(const char *path, const char *gitdir, const char *url\n \t\targv_array_push(&cp.args, \"--dissociate\");\n \tif (gitdir && *gitdir)\n \t\targv_array_pushl(&cp.args, \"--separate-git-dir\", gitdir, NULL);\n+\tif (filter_options->choice) {\n+\t\tstruct strbuf expanded_filter = STRBUF_INIT;\n+\t\texpand_list_objects_filter_spec(filter_options, &expanded_filter);\n+\t\targv_array_pushf(&cp.args, \"--filter=%s\", expanded_filter.buf);\n+\t\tstrbuf_release(&expanded_filter);\n+\t}\n \n \targv_array_push(&cp.args, \"--\");\n \targv_array_push(&cp.args, url);\n@@ -1359,6 +1367,7 @@ static int module_clone(int argc, const char **argv, const char *prefix)\n \tchar *p, *path = NULL, *sm_gitdir;\n \tstruct strbuf sb = STRBUF_INIT;\n \tstruct string_list reference = STRING_LIST_INIT_NODUP;\n+\tstruct list_objects_filter_options filter_options;\n \tint dissociate = 0;\n \tchar *sm_alternate = NULL, *error_strategy = NULL;\n \n@@ -1386,16 +1395,18 @@ static int module_clone(int argc, const char **argv, const char *prefix)\n \t\tOPT__QUIET(&quiet, \"Suppress output for cloning a submodule\"),\n \t\tOPT_BOOL(0, \"progress\", &progress,\n \t\t\t   N_(\"force cloning progress\")),\n+\t\tOPT_PARSE_LIST_OBJECTS_FILTER(&filter_options),\n \t\tOPT_END()\n \t};\n \n \tconst char *const git_submodule_helper_usage[] = {\n \t\tN_(\"git submodule--helper clone [--prefix=<path>] [--quiet] \"\n \t\t   \"[--reference <repository>] [--name <name>] [--depth <depth>] \"\n-\t\t   \"--url <url> --path <path>\"),\n+\t\t   \"[--filter <filter-spec>] --url <url> --path <path>\"),\n \t\tNULL\n \t};\n \n+\tmemset(&filter_options, 0, sizeof(filter_options));\n \targc = parse_options(argc, argv, prefix, module_clone_options,\n \t\t\t     git_submodule_helper_usage, 0);\n \n@@ -1420,7 +1431,7 @@ static int module_clone(int argc, const char **argv, const char *prefix)\n \t\tprepare_possible_alternates(name, &reference);\n \n \t\tif (clone_submodule(path, sm_gitdir, url, depth, &reference, dissociate,\n-\t\t\t\t    quiet, progress))\n+\t\t\t\t    quiet, progress, &filter_options))\n \t\t\tdie(_(\"clone of '%s' into submodule path '%s' failed\"),\n \t\t\t    url, path);\n \t} else {\n@@ -1454,6 +1465,7 @@ static int module_clone(int argc, const char **argv, const char *prefix)\n \tfree(sm_gitdir);\n \tfree(path);\n \tfree(p);\n+\tlist_objects_filter_release(&filter_options);\n \treturn 0;\n }\n \n@@ -1539,6 +1551,7 @@ struct submodule_update_clone {\n \tconst char *depth;\n \tconst char *recursive_prefix;\n \tconst char *prefix;\n+\tconst struct list_objects_filter_options *filter_options;\n \n \t/* to be consumed by git-submodule.sh */\n \tstruct update_clone_data *update_clone;\n@@ -1555,7 +1568,7 @@ struct submodule_update_clone {\n };\n #define SUBMODULE_UPDATE_CLONE_INIT {0, MODULE_LIST_INIT, 0, \\\n \tSUBMODULE_UPDATE_STRATEGY_INIT, 0, 0, -1, STRING_LIST_INIT_DUP, 0, \\\n-\tNULL, NULL, NULL, \\\n+\tNULL, NULL, NULL, NULL, \\\n \tNULL, 0, 0, 0, NULL, 0, 0, 1}\n \n \n@@ -1681,6 +1694,12 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce,\n \t\targv_array_pushl(&child->args, \"--prefix\", suc->prefix, NULL);\n \tif (suc->recommend_shallow && sub->recommend_shallow == 1)\n \t\targv_array_push(&child->args, \"--depth=1\");\n+\tif (suc->filter_options->choice) {\n+\t\tstruct strbuf expanded_filter = STRBUF_INIT;\n+\t\texpand_list_objects_filter_spec(suc->filter_options, &expanded_filter);\n+\t\targv_array_pushf(&child->args, \"--filter=%s\", expanded_filter.buf);\n+\t\tstrbuf_release(&expanded_filter);\n+\t}\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@@ -1844,6 +1863,8 @@ static int update_clone(int argc, const char **argv, const char *prefix)\n \tconst char *update = NULL;\n \tstruct pathspec pathspec;\n \tstruct submodule_update_clone suc = SUBMODULE_UPDATE_CLONE_INIT;\n+\tstruct list_objects_filter_options filter_options;\n+\tint ret;\n \n \tstruct option module_update_clone_options[] = {\n \t\tOPT_STRING(0, \"prefix\", &prefix,\n@@ -1870,6 +1891,7 @@ static int update_clone(int argc, const char **argv, const char *prefix)\n \t\tOPT__QUIET(&suc.quiet, N_(\"don't print cloning progress\")),\n \t\tOPT_BOOL(0, \"progress\", &suc.progress,\n \t\t\t    N_(\"force cloning progress\")),\n+\t\tOPT_PARSE_LIST_OBJECTS_FILTER(&filter_options),\n \t\tOPT_END()\n \t};\n \n@@ -1882,6 +1904,7 @@ static int update_clone(int argc, const char **argv, const char *prefix)\n \tupdate_clone_config_from_gitmodules(&suc.max_jobs);\n \tgit_config(git_update_clone_config, &suc.max_jobs);\n \n+\tmemset(&filter_options, 0, sizeof(filter_options));\n \targc = parse_options(argc, argv, prefix, module_update_clone_options,\n \t\t\t     git_submodule_helper_usage, 0);\n \n@@ -1889,13 +1912,19 @@ static int update_clone(int argc, const char **argv, const char *prefix)\n \t\tif (parse_submodule_update_strategy(update, &suc.update) < 0)\n \t\t\tdie(_(\"bad value for update parameter\"));\n \n-\tif (module_list_compute(argc, argv, prefix, &pathspec, &suc.list) < 0)\n+\tsuc.filter_options = &filter_options;\n+\n+\tif (module_list_compute(argc, argv, prefix, &pathspec, &suc.list) < 0) {\n+\t\tlist_objects_filter_release(&filter_options);\n \t\treturn 1;\n+\t}\n \n \tif (pathspec.nr)\n \t\tsuc.warn_if_uninitialized = 1;\n \n-\treturn update_submodules(&suc);\n+\tret = update_submodules(&suc);\n+\tlist_objects_filter_release(&filter_options);\n+\treturn ret;\n }\n \n static int resolve_relative_path(int argc, const char **argv, const char *prefix)\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex c7f58c5756..64c5bdaacc 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -10,7 +10,7 @@ USAGE=\"[--quiet] [--cached]\n    or: $dashless [--quiet] status [--cached] [--recursive] [--] [<path>...]\n    or: $dashless [--quiet] init [--] [<path>...]\n    or: $dashless [--quiet] deinit [-f|--force] (--all| [--] <path>...)\n-   or: $dashless [--quiet] update [--init] [--remote] [-N|--no-fetch] [-f|--force] [--checkout|--merge|--rebase] [--[no-]recommend-shallow] [--reference <repository>] [--recursive] [--] [<path>...]\n+   or: $dashless [--quiet] update [--init [--filter=<filter-spec>]] [--remote] [-N|--no-fetch] [-f|--force] [--checkout|--merge|--rebase] [--[no-]recommend-shallow] [--reference <repository>] [--recursive] [--] [<path>...]\n    or: $dashless [--quiet] set-branch (--default|--branch <branch>) [--] <path>\n    or: $dashless [--quiet] summary [--cached|--files] [--summary-limit <n>] [commit] [--] [<path>...]\n    or: $dashless [--quiet] foreach [--recursive] <command>\n@@ -45,6 +45,7 @@ custom_name=\n depth=\n progress=\n dissociate=\n+filter=\n \n die_if_unmatched ()\n {\n@@ -520,6 +521,14 @@ cmd_update()\n \t\t--jobs=*)\n \t\t\tjobs=$1\n \t\t\t;;\n+\t\t--filter)\n+\t\t\tcase \"$2\" in '') usage ;; esac\n+\t\t\tfilter=\"--filter=$2\"\n+\t\t\tshift\n+\t\t\t;;\n+\t\t--filter=*)\n+\t\t\tfilter=$1\n+\t\t\t;;\n \t\t--)\n \t\t\tshift\n \t\t\tbreak\n@@ -534,6 +543,11 @@ cmd_update()\n \t\tshift\n \tdone\n \n+\tif test -n \"$filter\" && test \"$init\" != \"1\"\n+\tthen\n+\t\tusage\n+\tfi\n+\n \tif test -n \"$init\"\n \tthen\n \t\tcmd_init \"--\" \"$@\" || return\n@@ -550,6 +564,7 @@ cmd_update()\n \t\t${depth:+--depth \"$depth\"} \\\n \t\t$recommend_shallow \\\n \t\t$jobs \\\n+\t\t$filter \\\n \t\t-- \\\n \t\t\"$@\" || echo \"#unmatched\" $?\n \t} | {\ndiff --git a/t/t5617-clone-submodules-remote.sh b/t/t5617-clone-submodules-remote.sh\nindex 37fcce9c40..49448e5a88 100755\n--- a/t/t5617-clone-submodules-remote.sh\n+++ b/t/t5617-clone-submodules-remote.sh\n@@ -24,6 +24,13 @@ test_expect_success 'setup' '\n \t)\n '\n \n+# bare clone giving \"srv.bare\" for use as our server.\n+test_expect_success 'setup bare clone for server' '\n+\tgit clone --bare \"file://$(pwd)/.\" srv.bare &&\n+\tgit -C srv.bare config --local uploadpack.allowfilter 1 &&\n+\tgit -C srv.bare config --local uploadpack.allowanysha1inwant 1\n+'\n+\n test_expect_success 'clone with --no-remote-submodules' '\n \ttest_when_finished \"rm -rf super_clone\" &&\n \tgit clone --recurse-submodules --no-remote-submodules \"file://$pwd/.\" super_clone &&\n@@ -51,4 +58,16 @@ test_expect_success 'check the default is --no-remote-submodules' '\n \t)\n '\n \n+# do basic partial clone from \"srv.bare\"\n+# confirm partial clone was registered in the local config for super and sub.\n+test_expect_success 'clone with --filter' '\n+\tgit clone --recurse-submodules --filter blob:none \"file://$pwd/srv.bare\" super_clone &&\n+\ttest \"$(git -C super_clone config --local core.repositoryformatversion)\" = \"1\" &&\n+\ttest \"$(git -C super_clone config --local extensions.partialclone)\" = \"origin\" &&\n+\ttest \"$(git -C super_clone config --local core.partialclonefilter)\" = \"blob:none\" &&\n+\ttest \"$(git -C super_clone/sub config --local core.repositoryformatversion)\" = \"1\" &&\n+\ttest \"$(git -C super_clone/sub config --local extensions.partialclone)\" = \"origin\" &&\n+\ttest \"$(git -C super_clone/sub config --local core.partialclonefilter)\" = \"blob:none\"\n+'\n+\n test_done\n-- \n2.22.0.657.g960e92d24f-goog\n\n"},{"id":"379224","messageId":"20190724221815.187788-1-jonathantanmy@google.com","threadId":"51487","inReplyTo":"52bf9d45b8e2b72ff32aa773f2415bf7b2b86da2.1563322192.git.steadmon@google.com","subject":"Re: [PATCH] submodule: plumb --filter to cloned submodules","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2019-07-24T22:18:15Z","receivedAt":"2019-07-24T22:18:21Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> When cloning a repo with a --filter and with --recurse-submodules\n> enabled, the partial clone filter only applies to the top-level repo.\n> This can lead to unexpected bandwidth and disk usage for projects which\n> include large submodules.\n> \n> Fix this by plumbing the --filter argument from git-clone through\n> git-submodule and git-submodule--helper.\n> \n> Signed-off-by: Josh Steadmon <steadmon@google.com>\n\nSorry for not catching this earlier. The cloning itself might work, but\nI don't think we support partial-clone submodules right now. In\nparticular, anything that operates on submodules in-process like grep\n(which either adds submodule objects as alternates or calls the\nobject-loading functions using a different struct repo) will not work,\nbecause we only support lazy loading on the_repository. So if we want\nthis feature, there is some other work that we will need to do first.\n"}]}