{"thread":{"id":"57348","subject":"[PATCH 0/6] [RFC] partial-clone: add ability to refetch with expanded filter","startedAt":"2022-02-01T15:49:58Z","lastAt":"2022-04-01T10:52:20Z","messageCount":76,"participants":["Robert Coup via GitGitGadget","Junio C Hamano","Robert Coup","Jonathan Tan","Jeff Hostetler","Ævar Arnfjörð Bjarmason","Calvin Wan"],"isPatch":true,"patchVersion":1,"patchTotal":6},"messages":[{"id":"447418","messageId":"pull.1138.git.1643730593.gitgitgadget@gmail.com","threadId":"57348","inReplyTo":null,"subject":"[PATCH 0/6] [RFC] partial-clone: add ability to refetch with expanded filter","fromName":"Robert Coup via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-02-01T15:49:47Z","receivedAt":"2022-02-01T15:49:58Z","isPatch":true,"sender":{"key":"robert@coup.net.nz","avatar":"https://avatars.githubusercontent.com/u/59874?v=4"},"body":"If a filter is changed on a partial clone repository, for example from\nblob:none to blob:limit=1m, there is currently no straightforward way to\nbulk-refetch the objects that match the new filter for existing local\ncommits. This is because the client will report commits as \"have\" during\nnegotiation and any dependent objects won't be included in the transferred\npack. Another use case is discussed at [1].\n\nThis patch series proposes adding a --refilter option to fetch & fetch-pack\nto enable doing a full fetch with a different filter, as if the local has no\ncommits in common with the remote. It builds upon cbe566a071\n(\"negotiator/noop: add noop fetch negotiator\", 2020-08-18).\n\nTo note:\n\n 1. This will produce duplicated objects between the existing and newly\n fetched packs, but gc will clean them up.\n 2. This series doesn't check that there's a new filter in any way, whether\n configured via config or passed via --filter=. Personally I think that's\n fine.\n 3. If a user fetches with --refilter applying a more restrictive filter\n than previously (eg: blob:limit=1m then blob:limit=1k) the eventual\n state is a no-op, since any referenced object already in the local\n repository is never removed. Potentially this could be improved in\n future by more advanced gc, possibly along the lines discussed at [2].\n\n[1]\nhttps://public-inbox.org/git/aa7b89ee-08aa-7943-6a00-28dcf344426e@syntevo.com/\n[2]\nhttps://public-inbox.org/git/A4BAD509-FA1F-49C3-87AF-CF4B73C559F1@gmail.com/\n\nRobert Coup (6):\n fetch-negotiator: add specific noop initializor\n fetch-pack: add partial clone refiltering\n builtin/fetch-pack: add --refilter option\n fetch: add --refilter option\n t5615-partial-clone: add test for --refilter\n doc/partial-clone: mention --refilter option\n\n Documentation/fetch-options.txt | 9 ++++\n Documentation/git-fetch-pack.txt | 4 ++\n Documentation/technical/partial-clone.txt | 3 ++\n builtin/fetch-pack.c | 4 ++\n builtin/fetch.c | 18 ++++++-\n fetch-negotiator.c | 5 ++\n fetch-negotiator.h | 8 ++++\n fetch-pack.c | 57 +++++++++++++++--------\n fetch-pack.h | 1 +\n remote-curl.c | 6 +++\n t/t5616-partial-clone.sh | 42 ++++++++++++++++-\n transport-helper.c | 3 ++\n transport.c | 4 ++\n transport.h | 4 ++\n 14 files changed, 146 insertions(+), 22 deletions(-)\n\n\nbase-commit: 5d01301f2b865aa8dba1654d3f447ce9d21db0b5\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1138%2Frcoup%2Frc-partial-clone-refilter-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1138/rcoup/rc-partial-clone-refilter-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1138\n-- \ngitgitgadget\n"},{"id":"447419","messageId":"ca50ce77ae47268a3aab0c9366fad8a79ab8ecdb.1643730593.git.gitgitgadget@gmail.com","threadId":"57348","inReplyTo":"pull.1138.git.1643730593.gitgitgadget@gmail.com","subject":"[PATCH 1/6] fetch-negotiator: add specific noop initializor","fromName":"Robert Coup via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-02-01T15:49:48Z","receivedAt":"2022-02-01T15:50:00Z","isPatch":true,"sender":{"key":"robert@coup.net.nz","avatar":"https://avatars.githubusercontent.com/u/59874?v=4"},"body":"From: Robert Coup \n\nAdd a specific initializor for the noop fetch negotiator. This is\nintroduced to support allowing partial clones to skip commit negotiation\nwhen refetching to apply a modified filter.\n\nSigned-off-by: Robert Coup \n---\n fetch-negotiator.c | 5 +++++\n fetch-negotiator.h | 8 ++++++++\n 2 files changed, 13 insertions(+)\n\ndiff --git a/fetch-negotiator.c b/fetch-negotiator.c\nindex 273390229fe..fe316afbf03 100644\n--- a/fetch-negotiator.c\n+++ b/fetch-negotiator.c\n@@ -23,3 +23,8 @@ void fetch_negotiator_init(struct repository *r,\n \t\treturn;\n \t}\n }\n+\n+void fetch_negotiator_init_noop(struct fetch_negotiator *negotiator)\n+{\n+\tnoop_negotiator_init(negotiator);\n+}\ndiff --git a/fetch-negotiator.h b/fetch-negotiator.h\nindex ea78868504b..e348905a1f0 100644\n--- a/fetch-negotiator.h\n+++ b/fetch-negotiator.h\n@@ -53,7 +53,15 @@ struct fetch_negotiator {\n \tvoid *data;\n };\n \n+/*\n+ * Initialize a negotiator based on the repository settings.\n+ */\n void fetch_negotiator_init(struct repository *r,\n \t\t\t struct fetch_negotiator *negotiator);\n \n+/*\n+ * Initialize a noop negotiator.\n+ */\n+void fetch_negotiator_init_noop(struct fetch_negotiator *negotiator);\n+\n #endif\n-- \ngitgitgadget\n\n"},{"id":"447420","messageId":"a3c1999936d241fdec2a958cd5a4ebc21f4c7aec.1643730593.git.gitgitgadget@gmail.com","threadId":"57348","inReplyTo":"pull.1138.git.1643730593.gitgitgadget@gmail.com","subject":"[PATCH 2/6] fetch-pack: add partial clone refiltering","fromName":"Robert Coup via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-02-01T15:49:49Z","receivedAt":"2022-02-01T15:50:01Z","isPatch":true,"sender":{"key":"robert@coup.net.nz","avatar":"https://avatars.githubusercontent.com/u/59874?v=4"},"body":"From: Robert Coup \n\nAllow refetching with a new partial clone filter by not attempting to\nfind or negotiate common commits with the remote, and always forcing\na full filtered fetch.\n\nSigned-off-by: Robert Coup \n---\n fetch-pack.c | 57 ++++++++++++++++++++++++++++++++++------------------\n fetch-pack.h | 1 +\n 2 files changed, 38 insertions(+), 20 deletions(-)\n\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex dd6ec449f2d..dd670441656 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -312,19 +312,21 @@ static int find_common(struct fetch_negotiator *negotiator,\n \t\tconst char *remote_hex;\n \t\tstruct object *o;\n \n-\t\t/*\n-\t\t * If that object is complete (i.e. it is an ancestor of a\n-\t\t * local ref), we tell them we have it but do not have to\n-\t\t * tell them about its ancestors, which they already know\n-\t\t * about.\n-\t\t *\n-\t\t * We use lookup_object here because we are only\n-\t\t * interested in the case we *know* the object is\n-\t\t * reachable and we have already scanned it.\n-\t\t */\n-\t\tif (((o = lookup_object(the_repository, remote)) != NULL) &&\n-\t\t\t\t(o->flags & COMPLETE)) {\n-\t\t\tcontinue;\n+\t\tif (!args->refilter) {\n+\t\t\t/*\n+\t\t\t* If that object is complete (i.e. it is an ancestor of a\n+\t\t\t* local ref), we tell them we have it but do not have to\n+\t\t\t* tell them about its ancestors, which they already know\n+\t\t\t* about.\n+\t\t\t*\n+\t\t\t* We use lookup_object here because we are only\n+\t\t\t* interested in the case we *know* the object is\n+\t\t\t* reachable and we have already scanned it.\n+\t\t\t*/\n+\t\t\tif (((o = lookup_object(the_repository, remote)) != NULL) &&\n+\t\t\t\t\t(o->flags & COMPLETE)) {\n+\t\t\t\tcontinue;\n+\t\t\t}\n \t\t}\n \n \t\tremote_hex = oid_to_hex(remote);\n@@ -694,6 +696,9 @@ static void mark_complete_and_common_ref(struct fetch_negotiator *negotiator,\n \n \tsave_commit_buffer = 0;\n \n+\tif (args->refilter)\n+\t\treturn;\n+\n \ttrace2_region_enter(\"fetch-pack\", \"parse_remote_refs_and_find_cutoff\", NULL);\n \tfor (ref = *refs; ref; ref = ref->next) {\n \t\tstruct object *o;\n@@ -1022,9 +1027,7 @@ static struct ref *do_fetch_pack(struct fetch_pack_args *args,\n \tint agent_len;\n \tstruct fetch_negotiator negotiator_alloc;\n \tstruct fetch_negotiator *negotiator;\n-\n-\tnegotiator = &negotiator_alloc;\n-\tfetch_negotiator_init(r, negotiator);\n+\tunsigned is_refiltering = 0;\n \n \tsort_ref_list(&ref, ref_compare_name);\n \tQSORT(sought, nr_sought, cmp_ref_by_name);\n@@ -1094,10 +1097,14 @@ static struct ref *do_fetch_pack(struct fetch_pack_args *args,\n \tif (server_supports(\"filter\")) {\n \t\tserver_supports_filtering = 1;\n \t\tprint_verbose(args, _(\"Server supports %s\"), \"filter\");\n-\t} else if (args->filter_options.choice) {\n+\t} else if (args->filter_options.choice || args->refilter) {\n \t\twarning(\"filtering not recognized by server, ignoring\");\n \t}\n \n+\tif (server_supports_filtering && args->refilter) {\n+\t\tis_refiltering = 1;\n+\t}\n+\n \tif (server_supports(\"deepen-since\")) {\n \t\tprint_verbose(args, _(\"Server supports %s\"), \"deepen-since\");\n \t\tdeepen_since_ok = 1;\n@@ -1115,9 +1122,16 @@ static struct ref *do_fetch_pack(struct fetch_pack_args *args,\n \tif (!server_supports_hash(the_hash_algo->name, NULL))\n \t\tdie(_(\"Server does not support this repository's object format\"));\n \n+\tnegotiator = &negotiator_alloc;\n+\tif (is_refiltering) {\n+\t\tfetch_negotiator_init_noop(negotiator);\n+\t} else {\n+\t\tfetch_negotiator_init(r, negotiator);\n+\t}\n+\n \tmark_complete_and_common_ref(negotiator, args, &ref);\n \tfilter_refs(args, &ref, sought, nr_sought);\n-\tif (everything_local(args, &ref)) {\n+\tif (!is_refiltering && everything_local(args, &ref)) {\n \t\tpacket_flush(fd[1]);\n \t\tgoto all_done;\n \t}\n@@ -1575,7 +1589,10 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,\n \tstruct strvec index_pack_args = STRVEC_INIT;\n \n \tnegotiator = &negotiator_alloc;\n-\tfetch_negotiator_init(r, negotiator);\n+\tif (args->refilter)\n+\t\tfetch_negotiator_init_noop(negotiator);\n+\telse\n+\t\tfetch_negotiator_init(r, negotiator);\n \n \tpacket_reader_init(&reader, fd[0], NULL, 0,\n \t\t\t PACKET_READ_CHOMP_NEWLINE |\n@@ -1601,7 +1618,7 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,\n \t\t\t/* Filter 'ref' by 'sought' and those that aren't local */\n \t\t\tmark_complete_and_common_ref(negotiator, args, &ref);\n \t\t\tfilter_refs(args, &ref, sought, nr_sought);\n-\t\t\tif (everything_local(args, &ref))\n+\t\t\tif (!args->refilter && everything_local(args, &ref))\n \t\t\t\tstate = FETCH_DONE;\n \t\t\telse\n \t\t\t\tstate = FETCH_SEND_REQUEST;\ndiff --git a/fetch-pack.h b/fetch-pack.h\nindex 7f94a2a5831..68df2230c55 100644\n--- a/fetch-pack.h\n+++ b/fetch-pack.h\n@@ -42,6 +42,7 @@ struct fetch_pack_args {\n \tunsigned update_shallow:1;\n \tunsigned reject_shallow_remote:1;\n \tunsigned deepen:1;\n+\tunsigned refilter:1;\n \n \t/*\n \t * Indicate that the remote of this request is a promisor remote. The\n-- \ngitgitgadget\n\n"},{"id":"447421","messageId":"d7f4b6c8052ddfebe477228057500e6ec5025b62.1643730593.git.gitgitgadget@gmail.com","threadId":"57348","inReplyTo":"pull.1138.git.1643730593.gitgitgadget@gmail.com","subject":"[PATCH 3/6] builtin/fetch-pack: add --refilter option","fromName":"Robert Coup via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-02-01T15:49:50Z","receivedAt":"2022-02-01T15:50:03Z","isPatch":true,"sender":{"key":"robert@coup.net.nz","avatar":"https://avatars.githubusercontent.com/u/59874?v=4"},"body":"From: Robert Coup \n\nAdd a --refilter option to fetch-pack to force a full fetch when\napplying a new filter.\n\nSigned-off-by: Robert Coup \n---\n Documentation/git-fetch-pack.txt | 4 ++++\n builtin/fetch-pack.c | 4 ++++\n remote-curl.c | 6 ++++++\n 3 files changed, 14 insertions(+)\n\ndiff --git a/Documentation/git-fetch-pack.txt b/Documentation/git-fetch-pack.txt\nindex c9758847937..ad21c627d75 100644\n--- a/Documentation/git-fetch-pack.txt\n+++ b/Documentation/git-fetch-pack.txt\n@@ -101,6 +101,10 @@ be in a separate packet, and the list must end with a flush packet.\n \tcurrent shallow boundary instead of from the tip of each\n \tremote branch history.\n \n+--refilter::\n+\tSkips negotiating commits with the server in order to reapply a new\n+\tpartial clone filter and fetch all matching objects.\n+\n --no-progress::\n \tDo not show the progress.\n \ndiff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c\nindex c2d96f4c89a..eb478929236 100644\n--- a/builtin/fetch-pack.c\n+++ b/builtin/fetch-pack.c\n@@ -153,6 +153,10 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)\n \t\t\targs.from_promisor = 1;\n \t\t\tcontinue;\n \t\t}\n+\t\tif (!strcmp(\"--refilter\", arg)) {\n+\t\t\targs.refilter = 1;\n+\t\t\tcontinue;\n+\t\t}\n \t\tif (skip_prefix(arg, (\"--\" CL_ARG__FILTER \"=\"), &arg)) {\n \t\t\tparse_list_objects_filter(&args.filter_options, arg);\n \t\t\tcontinue;\ndiff --git a/remote-curl.c b/remote-curl.c\nindex 0dabef2dd7c..0679f71e935 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -43,6 +43,7 @@ struct options {\n \t\t/* see documentation of corresponding flag in fetch-pack.h */\n \t\tfrom_promisor : 1,\n \n+\t\trefilter : 1,\n \t\tatomic : 1,\n \t\tobject_format : 1,\n \t\tforce_if_includes : 1;\n@@ -198,6 +199,9 @@ static int set_option(const char *name, const char *value)\n \t} else if (!strcmp(name, \"from-promisor\")) {\n \t\toptions.from_promisor = 1;\n \t\treturn 0;\n+\t} else if (!strcmp(name, \"refilter\")) {\n+\t\toptions.refilter = 1;\n+\t\treturn 0;\n \t} else if (!strcmp(name, \"filter\")) {\n \t\toptions.filter = xstrdup(value);\n \t\treturn 0;\n@@ -1182,6 +1186,8 @@ static int fetch_git(struct discovery *heads,\n \t\tstrvec_push(&args, \"--deepen-relative\");\n \tif (options.from_promisor)\n \t\tstrvec_push(&args, \"--from-promisor\");\n+\tif (options.refilter)\n+\t\tstrvec_push(&args, \"--refilter\");\n \tif (options.filter)\n \t\tstrvec_pushf(&args, \"--filter=%s\", options.filter);\n \tstrvec_push(&args, url.buf);\n-- \ngitgitgadget\n\n"},{"id":"447422","messageId":"3e102724196eb2b7e2336aeb5265b76999e78e2e.1643730593.git.gitgitgadget@gmail.com","threadId":"57348","inReplyTo":"pull.1138.git.1643730593.gitgitgadget@gmail.com","subject":"[PATCH 4/6] fetch: add --refilter option","fromName":"Robert Coup via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-02-01T15:49:51Z","receivedAt":"2022-02-01T15:50:05Z","isPatch":true,"sender":{"key":"robert@coup.net.nz","avatar":"https://avatars.githubusercontent.com/u/59874?v=4"},"body":"From: Robert Coup \n\nTeach fetch and transports the --refilter option to force a full fetch\nwhen applying a new filter.\n\nSigned-off-by: Robert Coup \n---\n Documentation/fetch-options.txt | 9 +++++++++\n builtin/fetch.c | 18 +++++++++++++++++-\n transport-helper.c | 3 +++\n transport.c | 4 ++++\n transport.h | 4 ++++\n 5 files changed, 37 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/fetch-options.txt b/Documentation/fetch-options.txt\nindex e967ff1874c..004078bfea9 100644\n--- a/Documentation/fetch-options.txt\n+++ b/Documentation/fetch-options.txt\n@@ -162,6 +162,15 @@ endif::git-pull[]\n \tbehavior for a remote may be specified with the remote..tagOpt\n \tsetting. See linkgit:git-config[1].\n \n+ifndef::git-pull[]\n+--refilter::\n+\tReapply a partial clone filter from configuration or `--filter=`, such\n+\tas when the filter definition has changed. Instead of negotiating with\n+\tthe server to avoid transferring commits and associated objects that are\n+\talready present locally, this option fetches all objects that match the\n+\tfilter.\n+endif::git-pull[]\n+\n --refmap=::\n \tWhen fetching refs listed on the command line, use the\n \tspecified refspec (can be given more than once) to map the\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 5f06b21f8e9..1dec90fb25f 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -59,7 +59,7 @@ static int prune_tags = -1; /* unspecified */\n \n static int all, append, dry_run, force, keep, multiple, update_head_ok;\n static int write_fetch_head = 1;\n-static int verbosity, deepen_relative, set_upstream;\n+static int verbosity, deepen_relative, set_upstream, refilter;\n static int progress = -1;\n static int enable_auto_gc = 1;\n static int tags = TAGS_DEFAULT, unshallow, update_shallow, deepen;\n@@ -189,6 +189,9 @@ static struct option builtin_fetch_options[] = {\n \tOPT_SET_INT_F(0, \"unshallow\", &unshallow,\n \t\t N_(\"convert to a complete repository\"),\n \t\t 1, PARSE_OPT_NONEG),\n+\tOPT_SET_INT_F(0, \"refilter\", &refilter,\n+\t\t N_(\"re-fetch with a modified filter\"),\n+\t\t 1, PARSE_OPT_NONEG),\n \t{ OPTION_STRING, 0, \"submodule-prefix\", &submodule_prefix, N_(\"dir\"),\n \t\t N_(\"prepend this to submodule path output\"), PARSE_OPT_HIDDEN },\n \tOPT_CALLBACK_F(0, \"recurse-submodules-default\",\n@@ -1292,6 +1295,17 @@ static int check_exist_and_connected(struct ref *ref_map)\n \tif (deepen)\n \t\treturn -1;\n \n+\t/*\n+\t * Similarly, if we need to refilter a partial clone we already have\n+\t * these commits reachable. Running rev-list here will return with\n+\t * a good (0) exit status and we'll bypass the fetch that we\n+\t * really need to perform. Claiming failure now will ensure\n+\t * we perform the network exchange to reapply the filter.\n+\t */\n+\tif (refilter)\n+\t\treturn -1;\n+\n+\n \t/*\n \t * check_connected() allows objects to merely be promised, but\n \t * we need all direct targets to exist.\n@@ -1487,6 +1501,8 @@ static struct transport *prepare_transport(struct remote *remote, int deepen)\n \t\tset_option(transport, TRANS_OPT_DEEPEN_RELATIVE, \"yes\");\n \tif (update_shallow)\n \t\tset_option(transport, TRANS_OPT_UPDATE_SHALLOW, \"yes\");\n+\tif (refilter)\n+\t\tset_option(transport, TRANS_OPT_REFILTER, \"yes\");\n \tif (filter_options.choice) {\n \t\tconst char *spec =\n \t\t\texpand_list_objects_filter_spec(&filter_options);\ndiff --git a/transport-helper.c b/transport-helper.c\nindex a0297b0986c..3cab1ccc8d5 100644\n--- a/transport-helper.c\n+++ b/transport-helper.c\n@@ -715,6 +715,9 @@ static int fetch_refs(struct transport *transport,\n \tif (data->transport_options.update_shallow)\n \t\tset_helper_option(transport, \"update-shallow\", \"true\");\n \n+\tif (data->transport_options.refilter)\n+\t\tset_helper_option(transport, \"refilter\", \"true\");\n+\n \tif (data->transport_options.filter_options.choice) {\n \t\tconst char *spec = expand_list_objects_filter_spec(\n \t\t\t&data->transport_options.filter_options);\ndiff --git a/transport.c b/transport.c\nindex 2a3e3241545..a150964e20d 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -243,6 +243,9 @@ static int set_git_option(struct git_transport_options *opts,\n \t\tlist_objects_filter_die_if_populated(&opts->filter_options);\n \t\tparse_list_objects_filter(&opts->filter_options, value);\n \t\treturn 0;\n+\t} else if (!strcmp(name, TRANS_OPT_REFILTER)) {\n+\t\topts->refilter = !!value;\n+\t\treturn 0;\n \t} else if (!strcmp(name, TRANS_OPT_REJECT_SHALLOW)) {\n \t\topts->reject_shallow = !!value;\n \t\treturn 0;\n@@ -377,6 +380,7 @@ static int fetch_refs_via_pack(struct transport *transport,\n \targs.update_shallow = data->options.update_shallow;\n \targs.from_promisor = data->options.from_promisor;\n \targs.filter_options = data->options.filter_options;\n+\targs.refilter = data->options.refilter;\n \targs.stateless_rpc = transport->stateless_rpc;\n \targs.server_options = transport->server_options;\n \targs.negotiation_tips = data->options.negotiation_tips;\ndiff --git a/transport.h b/transport.h\nindex 3f16e50c196..6e21a5b0393 100644\n--- a/transport.h\n+++ b/transport.h\n@@ -16,6 +16,7 @@ struct git_transport_options {\n \tunsigned update_shallow : 1;\n \tunsigned reject_shallow : 1;\n \tunsigned deepen_relative : 1;\n+\tunsigned refilter : 1;\n \n \t/* see documentation of corresponding flag in fetch-pack.h */\n \tunsigned from_promisor : 1;\n@@ -216,6 +217,9 @@ void transport_check_allowed(const char *type);\n /* Filter objects for partial clone and fetch */\n #define TRANS_OPT_LIST_OBJECTS_FILTER \"filter\"\n \n+/* Refilter a previous partial clone */\n+#define TRANS_OPT_REFILTER \"refilter\"\n+\n /* Request atomic (all-or-nothing) updates when pushing */\n #define TRANS_OPT_ATOMIC \"atomic\"\n \n-- \ngitgitgadget\n\n"},{"id":"447423","messageId":"66c18850aadb636a922e7bf08c671d4e96998b12.1643730593.git.gitgitgadget@gmail.com","threadId":"57348","inReplyTo":"pull.1138.git.1643730593.gitgitgadget@gmail.com","subject":"[PATCH 5/6] t5615-partial-clone: add test for --refilter","fromName":"Robert Coup via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-02-01T15:49:52Z","receivedAt":"2022-02-01T15:50:06Z","isPatch":true,"sender":{"key":"robert@coup.net.nz","avatar":"https://avatars.githubusercontent.com/u/59874?v=4"},"body":"From: Robert Coup \n\nAdd a test for partial clone refiltering under protocol v0 and v2.\n\nSigned-off-by: Robert Coup \n---\n t/t5616-partial-clone.sh | 42 +++++++++++++++++++++++++++++++++++++++-\n 1 file changed, 41 insertions(+), 1 deletion(-)\n\ndiff --git a/t/t5616-partial-clone.sh b/t/t5616-partial-clone.sh\nindex 34469b6ac10..87b8095258f 100755\n--- a/t/t5616-partial-clone.sh\n+++ b/t/t5616-partial-clone.sh\n@@ -166,6 +166,46 @@ test_expect_success 'manual prefetch of missing objects' '\n \ttest_line_count = 0 observed.oids\n '\n \n+# create new commits in \"src\" repo to establish a history on file.4.txt\n+# and push to \"srv.bare\".\n+test_expect_success 'push new commits to server for file.4.txt' '\n+\tfor x in a b c d e f\n+\tdo\n+\t\techo \"Mod file.4.txt $x\" >src/file.4.txt &&\n+\t\tif list_contains \"a,b\" \"$x\"; then\n+\t\t\tprintf \"%10000s\" X >>src/file.4.txt\n+\t\tfi &&\n+\t\tif list_contains \"c,d\" \"$x\"; then\n+\t\t\tprintf \"%20000s\" X >>src/file.4.txt\n+\t\tfi &&\n+\t\tgit -C src add file.4.txt &&\n+\t\tgit -C src commit -m \"mod $x\" || return 1\n+\tdone &&\n+\tgit -C src push -u srv main\n+'\n+\n+# Do partial fetch to fetch smaller files; then verify that without --refilter\n+# applying a new filter does not refetch missing large objects. Then use\n+# --refilter to apply the new filter on existing commits. Test it under both\n+# protocol v2 & v0.\n+test_expect_success 'apply a different filter using --refilter' '\n+\tgit -C pc1 fetch --filter=blob:limit=999 origin &&\n+\tgit -C pc1 rev-list --quiet --objects --missing=print \\\n+\t\tmain..origin/main >observed &&\n+\ttest_line_count = 4 observed &&\n+\n+\tgit -C pc1 fetch --filter=blob:limit=19999 --refilter origin &&\n+\tgit -C pc1 rev-list --quiet --objects --missing=print \\\n+\t\tmain..origin/main >observed &&\n+\ttest_line_count = 2 observed &&\n+\n+\tgit -c protocol.version=0 -C pc1 fetch --filter=blob:limit=29999 \\\n+\t\t--refilter origin &&\n+\tgit -C pc1 rev-list --quiet --objects --missing=print \\\n+\t\tmain..origin/main >observed &&\n+\ttest_line_count = 0 observed\n+'\n+\n test_expect_success 'partial clone with transfer.fsckobjects=1 works with submodules' '\n \ttest_create_repo submodule &&\n \ttest_commit -C submodule mycommit &&\n@@ -225,7 +265,7 @@ test_expect_success 'use fsck before and after manually fetching a missing subtr\n \n \t# Auto-fetch all remaining trees and blobs with --missing=error\n \tgit -C dst rev-list --missing=error --objects main >fetched_objects &&\n-\ttest_line_count = 70 fetched_objects &&\n+\ttest_line_count = 88 fetched_objects &&\n \n \tawk -f print_1.awk fetched_objects |\n \txargs -n1 git -C dst cat-file -t >fetched_types &&\n-- \ngitgitgadget\n\n"},{"id":"447424","messageId":"6c4d4260bfc565d4aec9776e4709fea3e531ad2b.1643730593.git.gitgitgadget@gmail.com","threadId":"57348","inReplyTo":"pull.1138.git.1643730593.gitgitgadget@gmail.com","subject":"[PATCH 6/6] doc/partial-clone: mention --refilter option","fromName":"Robert Coup via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-02-01T15:49:53Z","receivedAt":"2022-02-01T15:50:08Z","isPatch":true,"sender":{"key":"robert@coup.net.nz","avatar":"https://avatars.githubusercontent.com/u/59874?v=4"},"body":"From: Robert Coup \n\nAdd the fetch --refilter option to the partial clone documentation.\n\nSigned-off-by: Robert Coup \n---\n Documentation/technical/partial-clone.txt | 3 +++\n 1 file changed, 3 insertions(+)\n\ndiff --git a/Documentation/technical/partial-clone.txt b/Documentation/technical/partial-clone.txt\nindex a0dd7c66f24..e246b0778e5 100644\n--- a/Documentation/technical/partial-clone.txt\n+++ b/Documentation/technical/partial-clone.txt\n@@ -181,6 +181,9 @@ Fetching Missing Objects\n currently fetches all objects referred to by the requested objects, even\n though they are not necessary.\n \n+- Fetching with `--refilter` will request a complete new filtered packfile from\n+ the remote, which can be used to change a filter without needing to\n+ dynamically fetch missing objects.\n \n Using many promisor remotes\n ---------------------------\n-- \ngitgitgadget\n"},{"id":"447454","messageId":"xmqqk0eecpl9.fsf@gitster.g","threadId":"57348","inReplyTo":"pull.1138.git.1643730593.gitgitgadget@gmail.com","subject":"Re: [PATCH 0/6] [RFC] partial-clone: add ability to refetch with expanded filter","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-02-01T20:13:38Z","receivedAt":"2022-02-01T20:13:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Robert Coup via GitGitGadget\" writes:\n\n> If a filter is changed on a partial clone repository, for example from\n> blob:none to blob:limit=1m, there is currently no straightforward way to\n> bulk-refetch the objects that match the new filter for existing local\n> commits. This is because the client will report commits as \"have\" during\n> negotiation and any dependent objects won't be included in the transferred\n> pack.\n\nIt sounds like a useful thing to have such a \"refetch things\"\noption.\n\nA lazy/partial clone is narrower than the full tree in the width\ndimension, while a shallow clone is shallower than the full history\nin the time dimension. The latter already has the \"--deepen\"\nsupport to say \"the commits listed in my shallow boundary list may\nclaim that I already have these, but I actually don't have them;\nplease stop lying to the other side and refetch what I should have\nfetched earlier\". I understand that this works in the other\ndimension to \"--widen\" things?\n\nMakes me wonder how well these two features work together (or if\nthey are mutually exclusive, that is fine as well as a starting\npoint).\n\nIf you update the filter specification to make it narrower (e.g. you\nstart from blob:limit=1m down to blob:limit=512k), would we transfer\nnothing (which would be ideal), or would we end up refetching\neverything that are smaller than 512k?\n\n> This patch series proposes adding a --refilter option to fetch & fetch-pack\n> to enable doing a full fetch with a different filter, as if the local has no\n> commits in common with the remote. It builds upon cbe566a071\n> (\"negotiator/noop: add noop fetch negotiator\", 2020-08-18).\n\nI guess the answer to the last question is ...\n\n> To note:\n>\n> 1. This will produce duplicated objects between the existing and newly\n> fetched packs, but gc will clean them up.\n\n... it is not smart enough to stell them to exclude what we _ought_\nto have by telling them what the _old_ filter spec was. That's OK\nfor a starting point, I guess. Hopefully, at the end of this\noperation, we should garbage collect the duplicated objects by\ndefault (with an option to turn it off)?\n\n> 2. This series doesn't check that there's a new filter in any way, whether\n> configured via config or passed via --filter=. Personally I think that's\n> fine.\n\nIn other words, a repository that used to be a partial clone can\nbecome a full clone by using the option _and_ not giving any filter.\nI think that is an intuitive enough behaviour and a natural\nconsequence to the extreme of what the feature is. Compared to\nmaking a full \"git clone\", fetching from the old local (and narrow)\nrepository into it and then discarding the old one, it would not\nhave any performance or storage advantage, but it probably is more\nconvenient.\n\n> 3. If a user fetches with --refilter applying a more restrictive filter\n> than previously (eg: blob:limit=1m then blob:limit=1k) the eventual\n> state is a no-op, since any referenced object already in the local\n> repository is never removed. Potentially this could be improved in\n> future by more advanced gc, possibly along the lines discussed at [2].\n\nOK. That matches my reaction to 1. above.\n"},{"id":"447540","messageId":"CACf-nVdmWyDnhJC=ikx85UST=u2ENAc4DhJJCp5c030Vb9=4+Q@mail.gmail.com","threadId":"57348","inReplyTo":"xmqqk0eecpl9.fsf@gitster.g","subject":"Re: [PATCH 0/6] [RFC] partial-clone: add ability to refetch with expanded filter","fromName":"Robert Coup","fromEmail":"robert@coup.net.nz","sentAt":"2022-02-02T15:02:55Z","receivedAt":"2022-02-02T15:03:08Z","isPatch":true,"sender":{"key":"robert@coup.net.nz","avatar":"https://avatars.githubusercontent.com/u/59874?v=4"},"body":"Hi Junio\n\nThanks for your input. Hopefully some of the other partial-clone\ninterested folks will chime in too.\n\nOn Tue, 1 Feb 2022 at 20:13, Junio C Hamano wrote:\n>\n> It sounds like a useful thing to have such a \"refetch things\"\n> option.\n\nAny improved suggestions on the argument name? I thought of\n--refetch but `fetch --refetch` seemed more confusing to explain.\n\n> Makes me wonder how well these two features work together (or if\n> they are mutually exclusive, that is fine as well as a starting\n> point).\n\nI don't see any particular reason they can't work together - as you say,\nthe filtering is orthogonal to shallow on a conceptual level. I haven't\nadded a test for that scenario yet but will do for a v1.\n\n> If you update the filter specification to make it narrower (e.g. you\n> start from blob:limit=1m down to blob:limit=512k), would we transfer\n> nothing (which would be ideal), or would we end up refetching\n> everything that are smaller than 512k?\n\nAs you spot, the latter. I can't see a straightforward way of telling the\nserver \"I have these trees/blobs already\" without generating (one way\nor the other) a list of millions of oids, then transferring & negotiating\nwith it.\n\n> ... it is not smart enough to stell them to exclude what we _ought_\n> to have by telling them what the _old_ filter spec was. That's OK\n> for a starting point, I guess.\n\nThe client doesn't really know what the local repository *has* —\npotentially several filters could have been applied and used for fetches\nat different points in the commit history, as well as objects dynamically\nfetched in. Even a filter set in the config only applies to subsequent\nfetches, and only if --filter isn't used to override it.\n\n> Hopefully, at the end of this\n> operation, we should garbage collect the duplicated objects by\n> default (with an option to turn it off)?\n\nI haven't explicitly looked into invoking gc yet, but yes, it'd be a bit of\na waste if it wasn't kicked off by default. Maybe reusing gc.auto\n\n> In other words, a repository that used to be a partial clone can\n> become a full clone by using the option _and_ not giving any filter.\n\nFor that specific case I think you can already do it by removing the\npromisor flag in the remote config, potentially adding it back if you\nwanted to keep it partial again from that point forward.\n\n> I think that is an intuitive enough behaviour and a natural\n> consequence to the extreme of what the feature is. Compared to\n> making a full \"git clone\", fetching from the old local (and narrow)\n> repository into it and then discarding the old one, it would not\n> have any performance or storage advantage, but it probably is more\n> convenient.\n\nIt's certainly cleaner than abusing --deepen, or temporarily moving pack\nfiles out of the way, or starting over with a fresh clone & copying config.\n\nThanks,\n\nRob :)\n"},{"id":"447552","messageId":"20220202185957.1928631-1-jonathantanmy@google.com","threadId":"57348","inReplyTo":"pull.1138.git.1643730593.gitgitgadget@gmail.com","subject":"Re: [PATCH 0/6] [RFC] partial-clone: add ability to refetch with expanded filter","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2022-02-02T18:59:57Z","receivedAt":"2022-02-02T19:00:03Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"\"Robert Coup via GitGitGadget\" writes:\n> If a filter is changed on a partial clone repository, for example from\n> blob:none to blob:limit=1m, there is currently no straightforward way to\n> bulk-refetch the objects that match the new filter for existing local\n> commits. This is because the client will report commits as \"have\" during\n> negotiation and any dependent objects won't be included in the transferred\n> pack. Another use case is discussed at [1].\n\nReporting commits as \"have\" can be solved by forcing the noop\nnegotiator, but there is another issue (which you seem to have\ndiscovered, glancing through your patches) in that fetch will abort (and\nreport success) if all wanted commits are present, even if not all\nobjects directly or indirectly referenced by those commits are present.\n\n> This patch series proposes adding a --refilter option to fetch & fetch-pack\n> to enable doing a full fetch with a different filter, as if the local has no\n> commits in common with the remote. It builds upon cbe566a071\n> (\"negotiator/noop: add noop fetch negotiator\", 2020-08-18).\n\nThanks - I think this is a useful feature. This is useful even in a\nnon-partial-clone repo, to repair objects that were, say, accidentally\ndeleted from the local object store.\n\nIf it's acceptable to have a separate command to configure the new\nfilter in the repo config (or to delete it, if we want to convert a\npartial clone into a regular repo), I think it's clearer to name this\noption \"--repair\" or something like that, and explain it as a fetch that\ndoes not take into account the contents of the local object store (not\nas a fetch that changes the filter).\n\nHaving said that, the overall feature is worth having. While we decide\non the name, the implementation of this will not change much. I'll try\nto review the implementation by the end of this week (but other reviews\nare welcome too, needless to say).\n\n> To note:\n> \n> 1. This will produce duplicated objects between the existing and newly\n> fetched packs, but gc will clean them up.\n\nNoted. This might be an argument for naming the option \"--repair\", since\nthe user would probably understand that there would be duplicate\nobjects.\n\n> 2. This series doesn't check that there's a new filter in any way, whether\n> configured via config or passed via --filter=. Personally I think that's\n> fine.\n\nAgreed.\n\n> 3. If a user fetches with --refilter applying a more restrictive filter\n> than previously (eg: blob:limit=1m then blob:limit=1k) the eventual\n> state is a no-op, since any referenced object already in the local\n> repository is never removed. Potentially this could be improved in\n> future by more advanced gc, possibly along the lines discussed at [2].\n\nThat's true.\n"},{"id":"447573","messageId":"CACf-nVevXOSoGTLp4qK2c6HE60+pwotUS3p0+N_zia1+opAdXA@mail.gmail.com","threadId":"57348","inReplyTo":"20220202185957.1928631-1-jonathantanmy@google.com","subject":"Re: [PATCH 0/6] [RFC] partial-clone: add ability to refetch with expanded filter","fromName":"Robert Coup","fromEmail":"robert@coup.net.nz","sentAt":"2022-02-02T21:58:28Z","receivedAt":"2022-02-02T21:58:41Z","isPatch":true,"sender":{"key":"robert@coup.net.nz","avatar":"https://avatars.githubusercontent.com/u/59874?v=4"},"body":"Hi Jonathan,\n\nOn Wed, 2 Feb 2022 at 19:00, Jonathan Tan wrote:\n>\n> Thanks - I think this is a useful feature. This is useful even in a\n> non-partial-clone repo, to repair objects that were, say, accidentally\n> deleted from the local object store.\n\nI'd\n\n> If it's acceptable to have a separate command to configure the new\n> filter in the repo config (or to delete it, if we want to convert a\n> partial clone into a regular repo), I think it's clearer to name this\n> option \"--repair\" or something like that, and explain it as a fetch that\n> does not take into account the contents of the local object store (not\n> as a fetch that changes the filter).\n\nI quite like --repair, since the implementation really has zero to do with\nfiltering or partial clones beyond that being my use case for it. Specifying\na filter, shallow options, or even using a promisor remote aren't\neven slightly necessary for the implementation as it turns out.\n\nAnd as you say, that makes it easier to explain too:\n\n\"fetch --repair will fetch all objects from the remote (applying any filters\nor shallow options as requested). Unlike a normal fetch it does not take into\naccount any content already in the local repository and acts more like an\ninitial clone. Any duplicate objects will get cleaned up during subsequent\nmaintenance.\"\n\n\"If you want to update your local repository with a different partial clone\nfilter, use `fetch --repair` to re-download all matching objects from the\nremote.\"\n\n> I'll try\n> to review the implementation by the end of this week (but other reviews\n> are welcome too, needless to say).\n\nThanks!\n\nRob :)\n"},{"id":"447574","messageId":"CACf-nVd-z-qi2VhW2P1YamaqQkZKEGwMa1m4ZvxqtM9Nmmz2QA@mail.gmail.com","threadId":"57348","inReplyTo":"CACf-nVevXOSoGTLp4qK2c6HE60+pwotUS3p0+N_zia1+opAdXA@mail.gmail.com","subject":"Re: [PATCH 0/6] [RFC] partial-clone: add ability to refetch with expanded filter","fromName":"Robert Coup","fromEmail":"robert@coup.net.nz","sentAt":"2022-02-02T21:59:47Z","receivedAt":"2022-02-02T22:00:00Z","isPatch":true,"sender":{"key":"robert@coup.net.nz","avatar":"https://avatars.githubusercontent.com/u/59874?v=4"},"body":"On Wed, 2 Feb 2022 at 21:58, Robert Coup wrote:\n>\n> Hi Jonathan,\n>\n> On Wed, 2 Feb 2022 at 19:00, Jonathan Tan wrote:\n> >\n> > Thanks - I think this is a useful feature. This is useful even in a\n> > non-partial-clone repo, to repair objects that were, say, accidentally\n> > deleted from the local object store.\n>\n> I'd\n\nI'd be keen to hear the stories here :) Just losing loose object files?\n\nRob.\n"},{"id":"447758","messageId":"20220204180230.2346654-1-jonathantanmy@google.com","threadId":"57348","inReplyTo":"a3c1999936d241fdec2a958cd5a4ebc21f4c7aec.1643730593.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 2/6] fetch-pack: add partial clone refiltering","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2022-02-04T18:02:30Z","receivedAt":"2022-02-04T18:02:37Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"\"Robert Coup via GitGitGadget\" writes:\n> @@ -312,19 +312,21 @@ static int find_common(struct fetch_negotiator *negotiator,\n> \t\tconst char *remote_hex;\n> \t\tstruct object *o;\n> \n> -\t\t/*\n> -\t\t * If that object is complete (i.e. it is an ancestor of a\n> -\t\t * local ref), we tell them we have it but do not have to\n> -\t\t * tell them about its ancestors, which they already know\n> -\t\t * about.\n> -\t\t *\n> -\t\t * We use lookup_object here because we are only\n> -\t\t * interested in the case we *know* the object is\n> -\t\t * reachable and we have already scanned it.\n> -\t\t */\n> -\t\tif (((o = lookup_object(the_repository, remote)) != NULL) &&\n> -\t\t\t\t(o->flags & COMPLETE)) {\n> -\t\t\tcontinue;\n> +\t\tif (!args->refilter) {\n> +\t\t\t/*\n> +\t\t\t* If that object is complete (i.e. it is an ancestor of a\n> +\t\t\t* local ref), we tell them we have it but do not have to\n> +\t\t\t* tell them about its ancestors, which they already know\n> +\t\t\t* about.\n> +\t\t\t*\n> +\t\t\t* We use lookup_object here because we are only\n> +\t\t\t* interested in the case we *know* the object is\n> +\t\t\t* reachable and we have already scanned it.\n> +\t\t\t*/\n> +\t\t\tif (((o = lookup_object(the_repository, remote)) != NULL) &&\n> +\t\t\t\t\t(o->flags & COMPLETE)) {\n> +\t\t\t\tcontinue;\n> +\t\t\t}\n\nThe approach that I would have expected is to not call\nmark_complete_and_common_ref(), filter_refs(), everything_local(), and\nfind_common(), but your approach here is to ensure that\nmark_complete_and_common_ref() and find_common() do not do anything.\nComparing the two approaches, the advantage of yours is that we only\nneed to make the change once to support both protocol v0 and v2\n(although the change looks more substantial than just skipping function\ncalls), but it makes the code more difficult to read in that we now have\nfunction calls that do nothing. What do you think about my approach?\n"},{"id":"447901","messageId":"596fe1fa-2499-665b-007d-c87ca3399d03@jeffhostetler.com","threadId":"57348","inReplyTo":"pull.1138.git.1643730593.gitgitgadget@gmail.com","subject":"Re: [PATCH 0/6] [RFC] partial-clone: add ability to refetch with expanded filter","fromName":"Jeff Hostetler","fromEmail":"git@jeffhostetler.com","sentAt":"2022-02-07T19:37:52Z","receivedAt":"2022-02-07T19:46:53Z","isPatch":true,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"\n\nOn 2/1/22 10:49 AM, Robert Coup via GitGitGadget wrote:\n> If a filter is changed on a partial clone repository, for example from\n> blob:none to blob:limit=1m, there is currently no straightforward way to\n> bulk-refetch the objects that match the new filter for existing local\n> commits. This is because the client will report commits as \"have\" during\n> negotiation and any dependent objects won't be included in the transferred\n> pack. Another use case is discussed at [1].\n> \n> This patch series proposes adding a --refilter option to fetch & fetch-pack\n> to enable doing a full fetch with a different filter, as if the local has no\n> commits in common with the remote. It builds upon cbe566a071\n> (\"negotiator/noop: add noop fetch negotiator\", 2020-08-18).\n> \n> To note:\n> \n> 1. This will produce duplicated objects between the existing and newly\n> fetched packs, but gc will clean them up.\n> 2. This series doesn't check that there's a new filter in any way, whether\n> configured via config or passed via --filter=. Personally I think that's\n> fine.\n> 3. If a user fetches with --refilter applying a more restrictive filter\n> than previously (eg: blob:limit=1m then blob:limit=1k) the eventual\n> state is a no-op, since any referenced object already in the local\n> repository is never removed. Potentially this could be improved in\n> future by more advanced gc, possibly along the lines discussed at [2].\n> \n\nYes, it would be nice to have a way to efficiently extend a\npartial clone with a more inclusive filter.\n\nIt would be nice to be able to send the old filter-spec and the\nnew filter-spec and ask the server to send \"new && !old\" to keep\nfrom having to resend the objects that the client already has.\nBut I'm not sure we know enough (on either side) to make that\ncomputation. And as you say, there is no guarantee that the client\nhas only used one filter in the past.\n\nJeff\n"},{"id":"448220","messageId":"CACf-nVePhtm_HAzAKzcap0E8kiyyEJPY_+N+bbPcYPVUkjweFg@mail.gmail.com","threadId":"57348","inReplyTo":"20220204180230.2346654-1-jonathantanmy@google.com","subject":"Re: [PATCH 2/6] fetch-pack: add partial clone refiltering","fromName":"Robert Coup","fromEmail":"robert@coup.net.nz","sentAt":"2022-02-11T14:56:40Z","receivedAt":"2022-02-11T14:56:55Z","isPatch":true,"sender":{"key":"robert@coup.net.nz","avatar":"https://avatars.githubusercontent.com/u/59874?v=4"},"body":"Hi Jonathan,\n\nThanks for taking a look at this.\n\nOn Fri, 4 Feb 2022 at 18:02, Jonathan Tan wrote:\n>\n> The approach that I would have expected is to not call\n> mark_complete_and_common_ref(), filter_refs(), everything_local(), and\n> find_common(), but your approach here is to ensure that\n> mark_complete_and_common_ref() and find_common() do not do anything.\n\nv0: find_common() definitely still does something: during refiltering it skips\nchecking the local object db, but it's still responsible for sending\nthe \"wants\".\n\nfilter_refs() is necessary under v0 & v2 so the remote refs all get marked as\nmatched, otherwise we end up erroring after the transfer with\n\"error: no such remote ref refs/heads/main\" etc.\n\n> Comparing the two approaches, the advantage of yours is that we only\n> need to make the change once to support both protocol v0 and v2\n> (although the change looks more substantial than just skipping function\n> calls), but it makes the code more difficult to read in that we now have\n> function calls that do nothing. What do you think about my approach?\n\nMy original approach was to leave the negotiator in place, and just conditional\naround it in do_fetch_pack_v2 — this worked ok for protocol v2 but the v0\nimplementation didn't work. After that I switched to forcing noop in [1/6]\nwhich made both implementations match (& tidier imo).\n\nTo make the test pass and skip those calls I need a patch like the below\n— filter_refs() is still required during refiltering for the ref-matching. To me\nthis looks more complicated, but I'm happy to defer to your thinking.\n\nThanks,\n\nRob :)\n\n\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex dd67044165..870bfba267 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -1125,15 +1125,16 @@ static struct ref *do_fetch_pack(struct\nfetch_pack_args *args,\n negotiator = &negotiator_alloc;\n if (is_refiltering) {\n fetch_negotiator_init_noop(negotiator);\n+ filter_refs(args, &ref, sought, nr_sought);\n } else {\n fetch_negotiator_init(r, negotiator);\n- }\n\n- mark_complete_and_common_ref(negotiator, args, &ref);\n- filter_refs(args, &ref, sought, nr_sought);\n- if (!is_refiltering && everything_local(args, &ref)) {\n- packet_flush(fd[1]);\n- goto all_done;\n+ mark_complete_and_common_ref(negotiator, args, &ref);\n+ filter_refs(args, &ref, sought, nr_sought);\n+ if (everything_local(args, &ref)) {\n+ packet_flush(fd[1]);\n+ goto all_done;\n+ }\n }\n if (find_common(negotiator, args, fd, &oid, ref) < 0)\n if (!args->keep_pack)\n@@ -1615,13 +1616,18 @@ static struct ref *do_fetch_pack_v2(struct\nfetch_pack_args *args,\n if (args->depth > 0 || args->deepen_since ||\nargs->deepen_not)\n args->deepen = 1;\n\n- /* Filter 'ref' by 'sought' and those that\naren't local */\n- mark_complete_and_common_ref(negotiator, args, &ref);\n- filter_refs(args, &ref, sought, nr_sought);\n- if (!args->refilter && everything_local(args, &ref))\n- state = FETCH_DONE;\n- else\n+ if (args->refilter) {\n+ filter_refs(args, &ref, sought, nr_sought);\n state = FETCH_SEND_REQUEST;\n+ } else {\n+ /* Filter 'ref' by 'sought' and those\nthat aren't local */\n+\nmark_complete_and_common_ref(negotiator, args, &ref);\n+ filter_refs(args, &ref, sought, nr_sought);\n+ if (everything_local(args, &ref))\n+ state = FETCH_DONE;\n+ else\n+ state = FETCH_SEND_REQUEST;\n+ }\n\n mark_tips(negotiator, args->negotiation_tips);\n for_each_cached_alternate(negotiator,\n"},{"id":"448589","messageId":"CACf-nVfX-R6wDc+YVVOzQsm9werTON1zjwitScr-Kh4yUvc6VA@mail.gmail.com","threadId":"57348","inReplyTo":"CACf-nVdmWyDnhJC=ikx85UST=u2ENAc4DhJJCp5c030Vb9=4+Q@mail.gmail.com","subject":"Re: [PATCH 0/6] [RFC] partial-clone: add ability to refetch with expanded filter","fromName":"Robert Coup","fromEmail":"robert@coup.net.nz","sentAt":"2022-02-16T13:24:08Z","receivedAt":"2022-02-16T13:24:25Z","isPatch":true,"sender":{"key":"robert@coup.net.nz","avatar":"https://avatars.githubusercontent.com/u/59874?v=4"},"body":"Hi Derrick,\n\nOn Wed, 2 Feb 2022 at 15:02, Robert Coup wrote:\n> On Tue, 1 Feb 2022 at 20:13, Junio C Hamano wrote:\n> > \"Robert Coup via GitGitGadget\" writes:\n> > > 1. This will produce duplicated objects between the existing and newly\n> > > fetched packs, but gc will clean them up.\n> >\n> > Hopefully, at the end of this\n> > operation, we should garbage collect the duplicated objects by\n> > default (with an option to turn it off)?\n>\n> I haven't explicitly looked into invoking gc yet, but yes, it'd be a bit of\n> a waste if it wasn't kicked off by default. Maybe reusing gc.auto\n\nJust looking into this: after a fetch, run_auto_maintenance() is called, which\ninvokes `git maintenance run --auto` (being the successor to `git gc --auto`)...\n\nIn the refilter (repair) case, we'll have a new pack which substantially\nduplicates what's in our existing packs. I'm after some advice here: I'm not\nsure whether I want to encourage a gc pack consolidation, an incremental\nrepack, both, neither, or something else?\n\nMy current train of thought is it invokes `git maintenance run --auto` with\ngc.autoPackLimit=1 to force a gc pack consolidation if it's not currently\n=0 (disabled), and with maintenance.incremental-repack.auto=-1 if it's not\ncurrently =0 (disabled) to force an incremental repack if someone doesn't\nwant to do pack consolidations.\n\nIn all cases the various enabled-ness settings should still be honoured to\nskip tasks if the user doesn't want them ever happening automatically.\n\nImplementation-wise I guess we'll need run_auto_maintenance() to be able\nto pass some config options through to the maintenance subprocess.\n\nDoes this sound like a reasonable approach?\n\nThanks,\n\nRob :)\n"},{"id":"448649","messageId":"20220217000554.1978909-1-jonathantanmy@google.com","threadId":"57348","inReplyTo":"CACf-nVePhtm_HAzAKzcap0E8kiyyEJPY_+N+bbPcYPVUkjweFg@mail.gmail.com","subject":"Re: [PATCH 2/6] fetch-pack: add partial clone refiltering","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2022-02-17T00:05:54Z","receivedAt":"2022-02-17T00:05:59Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Robert Coup writes:\n> On Fri, 4 Feb 2022 at 18:02, Jonathan Tan wrote:\n> >\n> > The approach that I would have expected is to not call\n> > mark_complete_and_common_ref(), filter_refs(), everything_local(), and\n> > find_common(), but your approach here is to ensure that\n> > mark_complete_and_common_ref() and find_common() do not do anything.\n> \n> v0: find_common() definitely still does something: during refiltering it skips\n> checking the local object db, but it's still responsible for sending\n> the \"wants\".\n> \n> filter_refs() is necessary under v0 & v2 so the remote refs all get marked as\n> matched, otherwise we end up erroring after the transfer with\n> \"error: no such remote ref refs/heads/main\" etc.\n> \n> > Comparing the two approaches, the advantage of yours is that we only\n> > need to make the change once to support both protocol v0 and v2\n> > (although the change looks more substantial than just skipping function\n> > calls), but it makes the code more difficult to read in that we now have\n> > function calls that do nothing. What do you think about my approach?\n> \n> My original approach was to leave the negotiator in place, and just conditional\n> around it in do_fetch_pack_v2 — this worked ok for protocol v2 but the v0\n> implementation didn't work. After that I switched to forcing noop in [1/6]\n> which made both implementations match (& tidier imo).\n> \n> To make the test pass and skip those calls I need a patch like the below\n> — filter_refs() is still required during refiltering for the ref-matching. To me\n> this looks more complicated, but I'm happy to defer to your thinking.\n> \n> Thanks,\n> \n> Rob :)\n\nThanks for investigating this; looks like I was perhaps too optimistic\nin thinking that the code could be reorganized in the way I'm\nsuggesting.\n\nI think that it's worth checking if we could refactor the parts that are\nneeded with --refilter out of filter_refs() and find_common() (and in\ndoing so, make these functions do only what their names imply). I\ndon't have time to do so right now, but I don't want to unnecessarily\nblock this patch set either, so I'll say that ideally another reviewer\n(or you) would do that investigation, but I have no objection to merging\nthis patch set in this state (other than the change of the argument\nname, perhaps to \"--repair\", and associated documentation).\n"},{"id":"449464","messageId":"20942562a66bd1dcc6966a1d9150cff9a9ad11e3.1645719219.git.gitgitgadget@gmail.com","threadId":"57348","inReplyTo":"pull.1138.v2.git.1645719218.gitgitgadget@gmail.com","subject":"[PATCH v2 8/8] doc/partial-clone: mention --repair fetch option","fromName":"Robert Coup via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-02-24T16:13:38Z","receivedAt":"2022-02-24T16:25:16Z","isPatch":true,"sender":{"key":"robert@coup.net.nz","avatar":"https://avatars.githubusercontent.com/u/59874?v=4"},"body":"From: Robert Coup \n\nDocument it for partial clones as a means to refetch with a new filter.\n\nSigned-off-by: Robert Coup \n---\n Documentation/technical/partial-clone.txt | 3 +++\n 1 file changed, 3 insertions(+)\n\ndiff --git a/Documentation/technical/partial-clone.txt b/Documentation/technical/partial-clone.txt\nindex a0dd7c66f24..268939f781d 100644\n--- a/Documentation/technical/partial-clone.txt\n+++ b/Documentation/technical/partial-clone.txt\n@@ -181,6 +181,9 @@ Fetching Missing Objects\n currently fetches all objects referred to by the requested objects, even\n though they are not necessary.\n \n+- Fetching with `--repair` will request a complete new filtered packfile from\n+ the remote, which can be used to change a filter without needing to\n+ dynamically fetch missing objects.\n \n Using many promisor remotes\n ---------------------------\n-- \ngitgitgadget\n"},{"id":"449466","messageId":"a42d40ac2949f6393e0f5985f8ecfcaa6503486e.1645719218.git.gitgitgadget@gmail.com","threadId":"57348","inReplyTo":"pull.1138.v2.git.1645719218.gitgitgadget@gmail.com","subject":"[PATCH v2 3/8] builtin/fetch-pack: add --repair option","fromName":"Robert Coup via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-02-24T16:13:33Z","receivedAt":"2022-02-24T16:27:12Z","isPatch":true,"sender":{"key":"robert@coup.net.nz","avatar":"https://avatars.githubusercontent.com/u/59874?v=4"},"body":"From: Robert Coup \n\nAdd a --repair option to fetch-pack to force a full fetch. Use when\napplying a new partial clone filter to refetch all matching objects.\n\nSigned-off-by: Robert Coup \n---\n Documentation/git-fetch-pack.txt | 4 ++++\n builtin/fetch-pack.c | 4 ++++\n remote-curl.c | 6 ++++++\n 3 files changed, 14 insertions(+)\n\ndiff --git a/Documentation/git-fetch-pack.txt b/Documentation/git-fetch-pack.txt\nindex c9758847937..b9276813fa1 100644\n--- a/Documentation/git-fetch-pack.txt\n+++ b/Documentation/git-fetch-pack.txt\n@@ -101,6 +101,10 @@ be in a separate packet, and the list must end with a flush packet.\n \tcurrent shallow boundary instead of from the tip of each\n \tremote branch history.\n \n+--repair::\n+\tSkips negotiating commits with the server in order to fetch all matching\n+\tobjects. Use to reapply a new partial clone blob/tree filter.\n+\n --no-progress::\n \tDo not show the progress.\n \ndiff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c\nindex c2d96f4c89a..60c0082d52b 100644\n--- a/builtin/fetch-pack.c\n+++ b/builtin/fetch-pack.c\n@@ -153,6 +153,10 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)\n \t\t\targs.from_promisor = 1;\n \t\t\tcontinue;\n \t\t}\n+\t\tif (!strcmp(\"--repair\", arg)) {\n+\t\t\targs.repair = 1;\n+\t\t\tcontinue;\n+\t\t}\n \t\tif (skip_prefix(arg, (\"--\" CL_ARG__FILTER \"=\"), &arg)) {\n \t\t\tparse_list_objects_filter(&args.filter_options, arg);\n \t\t\tcontinue;\ndiff --git a/remote-curl.c b/remote-curl.c\nindex 0dabef2dd7c..941ed627db5 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -43,6 +43,7 @@ struct options {\n \t\t/* see documentation of corresponding flag in fetch-pack.h */\n \t\tfrom_promisor : 1,\n \n+\t\trepair : 1,\n \t\tatomic : 1,\n \t\tobject_format : 1,\n \t\tforce_if_includes : 1;\n@@ -198,6 +199,9 @@ static int set_option(const char *name, const char *value)\n \t} else if (!strcmp(name, \"from-promisor\")) {\n \t\toptions.from_promisor = 1;\n \t\treturn 0;\n+\t} else if (!strcmp(name, \"repair\")) {\n+\t\toptions.repair = 1;\n+\t\treturn 0;\n \t} else if (!strcmp(name, \"filter\")) {\n \t\toptions.filter = xstrdup(value);\n \t\treturn 0;\n@@ -1182,6 +1186,8 @@ static int fetch_git(struct discovery *heads,\n \t\tstrvec_push(&args, \"--deepen-relative\");\n \tif (options.from_promisor)\n \t\tstrvec_push(&args, \"--from-promisor\");\n+\tif (options.repair)\n+\t\tstrvec_push(&args, \"--repair\");\n \tif (options.filter)\n \t\tstrvec_pushf(&args, \"--filter=%s\", options.filter);\n \tstrvec_push(&args, url.buf);\n-- \ngitgitgadget\n\n"},{"id":"449468","messageId":"pull.1138.v2.git.1645719218.gitgitgadget@gmail.com","threadId":"57348","inReplyTo":"pull.1138.git.1643730593.gitgitgadget@gmail.com","subject":"[PATCH v2 0/8] fetch: add repair: full refetch without negotiation (was: \"refiltering\")","fromName":"Robert Coup via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-02-24T16:13:30Z","receivedAt":"2022-02-24T16:27:40Z","isPatch":true,"sender":{"key":"robert@coup.net.nz","avatar":"https://avatars.githubusercontent.com/u/59874?v=4"},"body":"If a filter is changed on a partial clone repository, for example from\nblob:none to blob:limit=1m, there is currently no straightforward way to\nbulk-refetch the objects that match the new filter for existing local\ncommits. This is because the client will report commits as \"have\" during\nfetch negotiation and any dependent objects won't be included in the\ntransferred pack. Another use case is discussed at [1].\n\nThis patch series introduces a --repair option to fetch & fetch-pack to\nenable doing a full fetch without performing any commit negotiation with the\nremote, as for a fresh clone. It builds upon cbe566a071 (\"negotiator/noop:\nadd noop fetch negotiator\", 2020-08-18). While a key use case is described\nabove for partial clones, a user could also use --repair to fix a corrupted\nobject database by performing a refetch of objects that should already be\npresent, establishing a better workflow than deleting the local repository\nand re-cloning.\n\n * Using --repair will produce duplicated objects between the existing and\n newly fetched packs, but maintenance will clean them up when it runs\n automatically post-fetch (if enabled).\n * If a user fetches with --repair applying a more restrictive partial clone\n filter than previously (eg: blob:limit=1m then blob:limit=1k) the\n eventual state is a no-op, since any referenced object already in the\n local repository is never removed. More advanced repacking which could\n improve this scenario is currently proposed at [2].\n\n[1]\nhttps://public-inbox.org/git/aa7b89ee-08aa-7943-6a00-28dcf344426e@syntevo.com/\n[2]\nhttps://public-inbox.org/git/21ED346B-A906-4905-B061-EDE53691C586@gmail.com/\n\nChanges since RFC (v1):\n\n * Changed the name from \"refilter\" to \"repair\"\n * Removed dependency between server-side support for filtering and repair\n * Added a test case for a shallow clone\n * Post-fetch auto maintenance now strongly encourages\n repacking/consolidation\n\nRobert Coup (8):\n fetch-negotiator: add specific noop initializor\n fetch-pack: add repairing\n builtin/fetch-pack: add --repair option\n fetch: add --repair option\n t5615-partial-clone: add test for fetch --repair\n maintenance: add ability to pass config options\n fetch: after repair, encourage auto gc repacking\n doc/partial-clone: mention --repair fetch option\n\n Documentation/fetch-options.txt | 10 +++++\n Documentation/git-fetch-pack.txt | 4 ++\n Documentation/technical/partial-clone.txt | 3 ++\n builtin/am.c | 2 +-\n builtin/commit.c | 2 +-\n builtin/fetch-pack.c | 4 ++\n builtin/fetch.c | 38 ++++++++++++++--\n builtin/merge.c | 2 +-\n builtin/rebase.c | 2 +-\n fetch-negotiator.c | 5 +++\n fetch-negotiator.h | 8 ++++\n fetch-pack.c | 50 +++++++++++++--------\n fetch-pack.h | 1 +\n remote-curl.c | 6 +++\n run-command.c | 8 +++-\n run-command.h | 5 ++-\n t/t5616-partial-clone.sh | 54 ++++++++++++++++++++++-\n transport-helper.c | 3 ++\n transport.c | 4 ++\n transport.h | 4 ++\n 20 files changed, 186 insertions(+), 29 deletions(-)\n\n\nbase-commit: dab1b7905d0b295f1acef9785bb2b9cbb0fdec84\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1138%2Frcoup%2Frc-partial-clone-refilter-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1138/rcoup/rc-partial-clone-refilter-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/1138\n\nRange-diff vs v1:\n\n 1: ca50ce77ae4 ! 1: d146d8aaaaf fetch-negotiator: add specific noop initializor\n @@ Commit message\n \n Add a specific initializor for the noop fetch negotiator. This is\n introduced to support allowing partial clones to skip commit negotiation\n - when refetching to apply a modified filter.\n + when fetching to repair or apply a modified filter.\n \n Signed-off-by: Robert Coup \n \n 2: a3c1999936d ! 2: 2d817a65db5 fetch-pack: add partial clone refiltering\n @@ Metadata\n Author: Robert Coup \n \n ## Commit message ##\n - fetch-pack: add partial clone refiltering\n + fetch-pack: add repairing\n \n - Allow refetching with a new partial clone filter by not attempting to\n - find or negotiate common commits with the remote, and always forcing\n - a full filtered fetch.\n + Allow a 'repair fetch' where the contents of the local object store are\n + ignored and a full fetch is performed, not attempting to find or\n + negotiate common commits with the remote.\n +\n + A key use case is to apply a new partial clone blob/tree filter and\n + refetch all the associated matching content, which would otherwise not\n + be transferred when the commit objects are already present locally.\n \n Signed-off-by: Robert Coup \n \n @@ fetch-pack.c: static int find_common(struct fetch_negotiator *negotiator,\n -\t\tif (((o = lookup_object(the_repository, remote)) != NULL) &&\n -\t\t\t\t(o->flags & COMPLETE)) {\n -\t\t\tcontinue;\n -+\t\tif (!args->refilter) {\n ++\t\tif (!args->repair) {\n +\t\t\t/*\n +\t\t\t* If that object is complete (i.e. it is an ancestor of a\n +\t\t\t* local ref), we tell them we have it but do not have to\n @@ fetch-pack.c: static void mark_complete_and_common_ref(struct fetch_negotiator *\n \n \tsave_commit_buffer = 0;\n \n -+\tif (args->refilter)\n ++\tif (args->repair)\n +\t\treturn;\n +\n \ttrace2_region_enter(\"fetch-pack\", \"parse_remote_refs_and_find_cutoff\", NULL);\n \tfor (ref = *refs; ref; ref = ref->next) {\n - \t\tstruct object *o;\n + \t\tstruct commit *commit;\n @@ fetch-pack.c: static struct ref *do_fetch_pack(struct fetch_pack_args *args,\n - \tint agent_len;\n \tstruct fetch_negotiator negotiator_alloc;\n \tstruct fetch_negotiator *negotiator;\n --\n + \n -\tnegotiator = &negotiator_alloc;\n -\tfetch_negotiator_init(r, negotiator);\n -+\tunsigned is_refiltering = 0;\n - \n +-\n \tsort_ref_list(&ref, ref_compare_name);\n \tQSORT(sought, nr_sought, cmp_ref_by_name);\n -@@ fetch-pack.c: static struct ref *do_fetch_pack(struct fetch_pack_args *args,\n - \tif (server_supports(\"filter\")) {\n - \t\tserver_supports_filtering = 1;\n - \t\tprint_verbose(args, _(\"Server supports %s\"), \"filter\");\n --\t} else if (args->filter_options.choice) {\n -+\t} else if (args->filter_options.choice || args->refilter) {\n - \t\twarning(\"filtering not recognized by server, ignoring\");\n - \t}\n \n -+\tif (server_supports_filtering && args->refilter) {\n -+\t\tis_refiltering = 1;\n -+\t}\n -+\n - \tif (server_supports(\"deepen-since\")) {\n - \t\tprint_verbose(args, _(\"Server supports %s\"), \"deepen-since\");\n - \t\tdeepen_since_ok = 1;\n @@ fetch-pack.c: static struct ref *do_fetch_pack(struct fetch_pack_args *args,\n \tif (!server_supports_hash(the_hash_algo->name, NULL))\n \t\tdie(_(\"Server does not support this repository's object format\"));\n \n +\tnegotiator = &negotiator_alloc;\n -+\tif (is_refiltering) {\n ++\tif (args->repair) {\n +\t\tfetch_negotiator_init_noop(negotiator);\n +\t} else {\n +\t\tfetch_negotiator_init(r, negotiator);\n @@ fetch-pack.c: static struct ref *do_fetch_pack(struct fetch_pack_args *args,\n \tmark_complete_and_common_ref(negotiator, args, &ref);\n \tfilter_refs(args, &ref, sought, nr_sought);\n -\tif (everything_local(args, &ref)) {\n -+\tif (!is_refiltering && everything_local(args, &ref)) {\n ++\tif (!args->repair && everything_local(args, &ref)) {\n \t\tpacket_flush(fd[1]);\n \t\tgoto all_done;\n \t}\n @@ fetch-pack.c: static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,\n \n \tnegotiator = &negotiator_alloc;\n -\tfetch_negotiator_init(r, negotiator);\n -+\tif (args->refilter)\n ++\tif (args->repair)\n +\t\tfetch_negotiator_init_noop(negotiator);\n +\telse\n +\t\tfetch_negotiator_init(r, negotiator);\n @@ fetch-pack.c: static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,\n \t\t\tmark_complete_and_common_ref(negotiator, args, &ref);\n \t\t\tfilter_refs(args, &ref, sought, nr_sought);\n -\t\t\tif (everything_local(args, &ref))\n -+\t\t\tif (!args->refilter && everything_local(args, &ref))\n ++\t\t\tif (!args->repair && everything_local(args, &ref))\n \t\t\t\tstate = FETCH_DONE;\n \t\t\telse\n \t\t\t\tstate = FETCH_SEND_REQUEST;\n @@ fetch-pack.h: struct fetch_pack_args {\n \tunsigned update_shallow:1;\n \tunsigned reject_shallow_remote:1;\n \tunsigned deepen:1;\n -+\tunsigned refilter:1;\n ++\tunsigned repair:1;\n \n \t/*\n \t * Indicate that the remote of this request is a promisor remote. The\n 3: d7f4b6c8052 ! 3: a42d40ac294 builtin/fetch-pack: add --refilter option\n @@ Metadata\n Author: Robert Coup \n \n ## Commit message ##\n - builtin/fetch-pack: add --refilter option\n + builtin/fetch-pack: add --repair option\n \n - Add a --refilter option to fetch-pack to force a full fetch when\n - applying a new filter.\n + Add a --repair option to fetch-pack to force a full fetch. Use when\n + applying a new partial clone filter to refetch all matching objects.\n \n Signed-off-by: Robert Coup \n \n @@ Documentation/git-fetch-pack.txt: be in a separate packet, and the list must end\n \tcurrent shallow boundary instead of from the tip of each\n \tremote branch history.\n \n -+--refilter::\n -+\tSkips negotiating commits with the server in order to reapply a new\n -+\tpartial clone filter and fetch all matching objects.\n ++--repair::\n ++\tSkips negotiating commits with the server in order to fetch all matching\n ++\tobjects. Use to reapply a new partial clone blob/tree filter.\n +\n --no-progress::\n \tDo not show the progress.\n @@ builtin/fetch-pack.c: int cmd_fetch_pack(int argc, const char **argv, const char\n \t\t\targs.from_promisor = 1;\n \t\t\tcontinue;\n \t\t}\n -+\t\tif (!strcmp(\"--refilter\", arg)) {\n -+\t\t\targs.refilter = 1;\n ++\t\tif (!strcmp(\"--repair\", arg)) {\n ++\t\t\targs.repair = 1;\n +\t\t\tcontinue;\n +\t\t}\n \t\tif (skip_prefix(arg, (\"--\" CL_ARG__FILTER \"=\"), &arg)) {\n @@ remote-curl.c: struct options {\n \t\t/* see documentation of corresponding flag in fetch-pack.h */\n \t\tfrom_promisor : 1,\n \n -+\t\trefilter : 1,\n ++\t\trepair : 1,\n \t\tatomic : 1,\n \t\tobject_format : 1,\n \t\tforce_if_includes : 1;\n @@ remote-curl.c: static int set_option(const char *name, const char *value)\n \t} else if (!strcmp(name, \"from-promisor\")) {\n \t\toptions.from_promisor = 1;\n \t\treturn 0;\n -+\t} else if (!strcmp(name, \"refilter\")) {\n -+\t\toptions.refilter = 1;\n ++\t} else if (!strcmp(name, \"repair\")) {\n ++\t\toptions.repair = 1;\n +\t\treturn 0;\n \t} else if (!strcmp(name, \"filter\")) {\n \t\toptions.filter = xstrdup(value);\n @@ remote-curl.c: static int fetch_git(struct discovery *heads,\n \t\tstrvec_push(&args, \"--deepen-relative\");\n \tif (options.from_promisor)\n \t\tstrvec_push(&args, \"--from-promisor\");\n -+\tif (options.refilter)\n -+\t\tstrvec_push(&args, \"--refilter\");\n ++\tif (options.repair)\n ++\t\tstrvec_push(&args, \"--repair\");\n \tif (options.filter)\n \t\tstrvec_pushf(&args, \"--filter=%s\", options.filter);\n \tstrvec_push(&args, url.buf);\n 4: 3e102724196 ! 4: 79c409d0542 fetch: add --refilter option\n @@ Metadata\n Author: Robert Coup \n \n ## Commit message ##\n - fetch: add --refilter option\n + fetch: add --repair option\n \n - Teach fetch and transports the --refilter option to force a full fetch\n - when applying a new filter.\n + Teach fetch and transports the --repair option to force a full fetch\n + without negotiating common commits with the remote. Use when applying a\n + new partial clone filter to refetch all matching objects.\n \n Signed-off-by: Robert Coup \n \n @@ Documentation/fetch-options.txt: endif::git-pull[]\n \tsetting. See linkgit:git-config[1].\n \n +ifndef::git-pull[]\n -+--refilter::\n -+\tReapply a partial clone filter from configuration or `--filter=`, such\n -+\tas when the filter definition has changed. Instead of negotiating with\n -+\tthe server to avoid transferring commits and associated objects that are\n -+\talready present locally, this option fetches all objects that match the\n -+\tfilter.\n ++--repair::\n ++\tInstead of negotiating with the server to avoid transferring commits and\n ++\tassociated objects that are already present locally, this option fetches\n ++\tall objects as a fresh clone would. Use this to reapply a partial clone\n ++\tfilter from configuration or using `--filter=` when the filter\n ++\tdefinition has changed.\n +endif::git-pull[]\n +\n --refmap=::\n @@ builtin/fetch.c: static int prune_tags = -1; /* unspecified */\n static int all, append, dry_run, force, keep, multiple, update_head_ok;\n static int write_fetch_head = 1;\n -static int verbosity, deepen_relative, set_upstream;\n -+static int verbosity, deepen_relative, set_upstream, refilter;\n ++static int verbosity, deepen_relative, set_upstream, repair;\n static int progress = -1;\n static int enable_auto_gc = 1;\n static int tags = TAGS_DEFAULT, unshallow, update_shallow, deepen;\n @@ builtin/fetch.c: static struct option builtin_fetch_options[] = {\n \tOPT_SET_INT_F(0, \"unshallow\", &unshallow,\n \t\t N_(\"convert to a complete repository\"),\n \t\t 1, PARSE_OPT_NONEG),\n -+\tOPT_SET_INT_F(0, \"refilter\", &refilter,\n -+\t\t N_(\"re-fetch with a modified filter\"),\n ++\tOPT_SET_INT_F(0, \"repair\", &repair,\n ++\t\t N_(\"re-fetch without negotiating common commits\"),\n +\t\t 1, PARSE_OPT_NONEG),\n \t{ OPTION_STRING, 0, \"submodule-prefix\", &submodule_prefix, N_(\"dir\"),\n \t\t N_(\"prepend this to submodule path output\"), PARSE_OPT_HIDDEN },\n @@ builtin/fetch.c: static int check_exist_and_connected(struct ref *ref_map)\n \t\treturn -1;\n \n +\t/*\n -+\t * Similarly, if we need to refilter a partial clone we already have\n -+\t * these commits reachable. Running rev-list here will return with\n -+\t * a good (0) exit status and we'll bypass the fetch that we\n -+\t * really need to perform. Claiming failure now will ensure\n -+\t * we perform the network exchange to reapply the filter.\n ++\t * Similarly, if we need to repair, we always want to perform a full\n ++\t * fetch ignoring existing objects.\n +\t */\n -+\tif (refilter)\n ++\tif (repair)\n +\t\treturn -1;\n +\n +\n @@ builtin/fetch.c: static struct transport *prepare_transport(struct remote *remot\n \t\tset_option(transport, TRANS_OPT_DEEPEN_RELATIVE, \"yes\");\n \tif (update_shallow)\n \t\tset_option(transport, TRANS_OPT_UPDATE_SHALLOW, \"yes\");\n -+\tif (refilter)\n -+\t\tset_option(transport, TRANS_OPT_REFILTER, \"yes\");\n ++\tif (repair)\n ++\t\tset_option(transport, TRANS_OPT_REPAIR, \"yes\");\n \tif (filter_options.choice) {\n \t\tconst char *spec =\n \t\t\texpand_list_objects_filter_spec(&filter_options);\n @@ transport-helper.c: static int fetch_refs(struct transport *transport,\n \tif (data->transport_options.update_shallow)\n \t\tset_helper_option(transport, \"update-shallow\", \"true\");\n \n -+\tif (data->transport_options.refilter)\n -+\t\tset_helper_option(transport, \"refilter\", \"true\");\n ++\tif (data->transport_options.repair)\n ++\t\tset_helper_option(transport, \"repair\", \"true\");\n +\n \tif (data->transport_options.filter_options.choice) {\n \t\tconst char *spec = expand_list_objects_filter_spec(\n @@ transport.c: static int set_git_option(struct git_transport_options *opts,\n \t\tlist_objects_filter_die_if_populated(&opts->filter_options);\n \t\tparse_list_objects_filter(&opts->filter_options, value);\n \t\treturn 0;\n -+\t} else if (!strcmp(name, TRANS_OPT_REFILTER)) {\n -+\t\topts->refilter = !!value;\n ++\t} else if (!strcmp(name, TRANS_OPT_REPAIR)) {\n ++\t\topts->repair = !!value;\n +\t\treturn 0;\n \t} else if (!strcmp(name, TRANS_OPT_REJECT_SHALLOW)) {\n \t\topts->reject_shallow = !!value;\n @@ transport.c: static int fetch_refs_via_pack(struct transport *transport,\n \targs.update_shallow = data->options.update_shallow;\n \targs.from_promisor = data->options.from_promisor;\n \targs.filter_options = data->options.filter_options;\n -+\targs.refilter = data->options.refilter;\n ++\targs.repair = data->options.repair;\n \targs.stateless_rpc = transport->stateless_rpc;\n \targs.server_options = transport->server_options;\n \targs.negotiation_tips = data->options.negotiation_tips;\n @@ transport.h: struct git_transport_options {\n \tunsigned update_shallow : 1;\n \tunsigned reject_shallow : 1;\n \tunsigned deepen_relative : 1;\n -+\tunsigned refilter : 1;\n ++\tunsigned repair : 1;\n \n \t/* see documentation of corresponding flag in fetch-pack.h */\n \tunsigned from_promisor : 1;\n @@ transport.h: void transport_check_allowed(const char *type);\n /* Filter objects for partial clone and fetch */\n #define TRANS_OPT_LIST_OBJECTS_FILTER \"filter\"\n \n -+/* Refilter a previous partial clone */\n -+#define TRANS_OPT_REFILTER \"refilter\"\n ++/* Refetch all objects without negotiating */\n ++#define TRANS_OPT_REPAIR \"repair\"\n +\n /* Request atomic (all-or-nothing) updates when pushing */\n #define TRANS_OPT_ATOMIC \"atomic\"\n 5: 66c18850aad ! 5: 38af2bbee79 t5615-partial-clone: add test for --refilter\n @@ Metadata\n Author: Robert Coup \n \n ## Commit message ##\n - t5615-partial-clone: add test for --refilter\n + t5615-partial-clone: add test for fetch --repair\n \n - Add a test for partial clone refiltering under protocol v0 and v2.\n + Add a test for doing a repair fetch to apply a changed partial clone\n + filter under protocol v0 and v2.\n \n Signed-off-by: Robert Coup \n \n @@ t/t5616-partial-clone.sh: test_expect_success 'manual prefetch of missing object\n +\tgit -C src push -u srv main\n +'\n +\n -+# Do partial fetch to fetch smaller files; then verify that without --refilter\n ++# Do partial fetch to fetch smaller files; then verify that without --repair\n +# applying a new filter does not refetch missing large objects. Then use\n -+# --refilter to apply the new filter on existing commits. Test it under both\n ++# --repair to apply the new filter on existing commits. Test it under both\n +# protocol v2 & v0.\n -+test_expect_success 'apply a different filter using --refilter' '\n ++test_expect_success 'apply a different filter using --repair' '\n +\tgit -C pc1 fetch --filter=blob:limit=999 origin &&\n +\tgit -C pc1 rev-list --quiet --objects --missing=print \\\n +\t\tmain..origin/main >observed &&\n +\ttest_line_count = 4 observed &&\n +\n -+\tgit -C pc1 fetch --filter=blob:limit=19999 --refilter origin &&\n ++\tgit -C pc1 fetch --filter=blob:limit=19999 --repair origin &&\n +\tgit -C pc1 rev-list --quiet --objects --missing=print \\\n +\t\tmain..origin/main >observed &&\n +\ttest_line_count = 2 observed &&\n +\n +\tgit -c protocol.version=0 -C pc1 fetch --filter=blob:limit=29999 \\\n -+\t\t--refilter origin &&\n ++\t\t--repair origin &&\n +\tgit -C pc1 rev-list --quiet --objects --missing=print \\\n +\t\tmain..origin/main >observed &&\n +\ttest_line_count = 0 observed\n +'\n ++\n ++test_expect_success 'fetch --repair works with a shallow clone' '\n ++\tgit clone --no-checkout --depth=1 --filter=blob:none \"file://$(pwd)/srv.bare\" pc1s &&\n ++\tgit -C pc1s rev-list --objects --missing=print HEAD >observed &&\n ++\ttest_line_count = 6 observed &&\n ++\n ++\tGIT_TRACE=1 git -C pc1s fetch --filter=blob:limit=999 --repair origin &&\n ++\tgit -C pc1s rev-list --objects --missing=print HEAD >observed &&\n ++\ttest_line_count = 6 observed\n ++'\n +\n test_expect_success 'partial clone with transfer.fsckobjects=1 works with submodules' '\n \ttest_create_repo submodule &&\n -: ----------- > 6: cfa6dca8ef4 maintenance: add ability to pass config options\n -: ----------- > 7: 2338c15249a fetch: after repair, encourage auto gc repacking\n 6: 6c4d4260bfc ! 8: 20942562a66 doc/partial-clone: mention --refilter option\n @@ Metadata\n Author: Robert Coup \n \n ## Commit message ##\n - doc/partial-clone: mention --refilter option\n + doc/partial-clone: mention --repair fetch option\n \n - Add the fetch --refilter option to the partial clone documentation.\n + Document it for partial clones as a means to refetch with a new filter.\n \n Signed-off-by: Robert Coup \n \n @@ Documentation/technical/partial-clone.txt: Fetching Missing Objects\n currently fetches all objects referred to by the requested objects, even\n though they are not necessary.\n \n -+- Fetching with `--refilter` will request a complete new filtered packfile from\n ++- Fetching with `--repair` will request a complete new filtered packfile from\n + the remote, which can be used to change a filter without needing to\n + dynamically fetch missing objects.\n \n\n-- \ngitgitgadget\n"},{"id":"449469","messageId":"2d817a65db55750d27986c2e2ab5b1723d17081e.1645719218.git.gitgitgadget@gmail.com","threadId":"57348","inReplyTo":"pull.1138.v2.git.1645719218.gitgitgadget@gmail.com","subject":"[PATCH v2 2/8] fetch-pack: add repairing","fromName":"Robert Coup via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-02-24T16:13:32Z","receivedAt":"2022-02-24T16:27:51Z","isPatch":true,"sender":{"key":"robert@coup.net.nz","avatar":"https://avatars.githubusercontent.com/u/59874?v=4"},"body":"From: Robert Coup \n\nAllow a 'repair fetch' where the contents of the local object store are\nignored and a full fetch is performed, not attempting to find or\nnegotiate common commits with the remote.\n\nA key use case is to apply a new partial clone blob/tree filter and\nrefetch all the associated matching content, which would otherwise not\nbe transferred when the commit objects are already present locally.\n\nSigned-off-by: Robert Coup \n---\n fetch-pack.c | 50 +++++++++++++++++++++++++++++++-------------------\n fetch-pack.h | 1 +\n 2 files changed, 32 insertions(+), 19 deletions(-)\n\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex 87657907e78..8103243947a 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -312,19 +312,21 @@ static int find_common(struct fetch_negotiator *negotiator,\n \t\tconst char *remote_hex;\n \t\tstruct object *o;\n \n-\t\t/*\n-\t\t * If that object is complete (i.e. it is an ancestor of a\n-\t\t * local ref), we tell them we have it but do not have to\n-\t\t * tell them about its ancestors, which they already know\n-\t\t * about.\n-\t\t *\n-\t\t * We use lookup_object here because we are only\n-\t\t * interested in the case we *know* the object is\n-\t\t * reachable and we have already scanned it.\n-\t\t */\n-\t\tif (((o = lookup_object(the_repository, remote)) != NULL) &&\n-\t\t\t\t(o->flags & COMPLETE)) {\n-\t\t\tcontinue;\n+\t\tif (!args->repair) {\n+\t\t\t/*\n+\t\t\t* If that object is complete (i.e. it is an ancestor of a\n+\t\t\t* local ref), we tell them we have it but do not have to\n+\t\t\t* tell them about its ancestors, which they already know\n+\t\t\t* about.\n+\t\t\t*\n+\t\t\t* We use lookup_object here because we are only\n+\t\t\t* interested in the case we *know* the object is\n+\t\t\t* reachable and we have already scanned it.\n+\t\t\t*/\n+\t\t\tif (((o = lookup_object(the_repository, remote)) != NULL) &&\n+\t\t\t\t\t(o->flags & COMPLETE)) {\n+\t\t\t\tcontinue;\n+\t\t\t}\n \t\t}\n \n \t\tremote_hex = oid_to_hex(remote);\n@@ -694,6 +696,9 @@ static void mark_complete_and_common_ref(struct fetch_negotiator *negotiator,\n \n \tsave_commit_buffer = 0;\n \n+\tif (args->repair)\n+\t\treturn;\n+\n \ttrace2_region_enter(\"fetch-pack\", \"parse_remote_refs_and_find_cutoff\", NULL);\n \tfor (ref = *refs; ref; ref = ref->next) {\n \t\tstruct commit *commit;\n@@ -1027,9 +1032,6 @@ static struct ref *do_fetch_pack(struct fetch_pack_args *args,\n \tstruct fetch_negotiator negotiator_alloc;\n \tstruct fetch_negotiator *negotiator;\n \n-\tnegotiator = &negotiator_alloc;\n-\tfetch_negotiator_init(r, negotiator);\n-\n \tsort_ref_list(&ref, ref_compare_name);\n \tQSORT(sought, nr_sought, cmp_ref_by_name);\n \n@@ -1119,9 +1121,16 @@ static struct ref *do_fetch_pack(struct fetch_pack_args *args,\n \tif (!server_supports_hash(the_hash_algo->name, NULL))\n \t\tdie(_(\"Server does not support this repository's object format\"));\n \n+\tnegotiator = &negotiator_alloc;\n+\tif (args->repair) {\n+\t\tfetch_negotiator_init_noop(negotiator);\n+\t} else {\n+\t\tfetch_negotiator_init(r, negotiator);\n+\t}\n+\n \tmark_complete_and_common_ref(negotiator, args, &ref);\n \tfilter_refs(args, &ref, sought, nr_sought);\n-\tif (everything_local(args, &ref)) {\n+\tif (!args->repair && everything_local(args, &ref)) {\n \t\tpacket_flush(fd[1]);\n \t\tgoto all_done;\n \t}\n@@ -1587,7 +1596,10 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,\n \tstruct strvec index_pack_args = STRVEC_INIT;\n \n \tnegotiator = &negotiator_alloc;\n-\tfetch_negotiator_init(r, negotiator);\n+\tif (args->repair)\n+\t\tfetch_negotiator_init_noop(negotiator);\n+\telse\n+\t\tfetch_negotiator_init(r, negotiator);\n \n \tpacket_reader_init(&reader, fd[0], NULL, 0,\n \t\t\t PACKET_READ_CHOMP_NEWLINE |\n@@ -1613,7 +1625,7 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,\n \t\t\t/* Filter 'ref' by 'sought' and those that aren't local */\n \t\t\tmark_complete_and_common_ref(negotiator, args, &ref);\n \t\t\tfilter_refs(args, &ref, sought, nr_sought);\n-\t\t\tif (everything_local(args, &ref))\n+\t\t\tif (!args->repair && everything_local(args, &ref))\n \t\t\t\tstate = FETCH_DONE;\n \t\t\telse\n \t\t\t\tstate = FETCH_SEND_REQUEST;\ndiff --git a/fetch-pack.h b/fetch-pack.h\nindex 7f94a2a5831..bbb663edda8 100644\n--- a/fetch-pack.h\n+++ b/fetch-pack.h\n@@ -42,6 +42,7 @@ struct fetch_pack_args {\n \tunsigned update_shallow:1;\n \tunsigned reject_shallow_remote:1;\n \tunsigned deepen:1;\n+\tunsigned repair:1;\n \n \t/*\n \t * Indicate that the remote of this request is a promisor remote. The\n-- \ngitgitgadget\n\n"},{"id":"449470","messageId":"d146d8aaaafee91de2e7c50b3a598a470319b541.1645719218.git.gitgitgadget@gmail.com","threadId":"57348","inReplyTo":"pull.1138.v2.git.1645719218.gitgitgadget@gmail.com","subject":"[PATCH v2 1/8] fetch-negotiator: add specific noop initializor","fromName":"Robert Coup via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-02-24T16:13:31Z","receivedAt":"2022-02-24T16:29:07Z","isPatch":true,"sender":{"key":"robert@coup.net.nz","avatar":"https://avatars.githubusercontent.com/u/59874?v=4"},"body":"From: Robert Coup \n\nAdd a specific initializor for the noop fetch negotiator. This is\nintroduced to support allowing partial clones to skip commit negotiation\nwhen fetching to repair or apply a modified filter.\n\nSigned-off-by: Robert Coup \n---\n fetch-negotiator.c | 5 +++++\n fetch-negotiator.h | 8 ++++++++\n 2 files changed, 13 insertions(+)\n\ndiff --git a/fetch-negotiator.c b/fetch-negotiator.c\nindex 874797d767b..be383367f55 100644\n--- a/fetch-negotiator.c\n+++ b/fetch-negotiator.c\n@@ -23,3 +23,8 @@ void fetch_negotiator_init(struct repository *r,\n \t\treturn;\n \t}\n }\n+\n+void fetch_negotiator_init_noop(struct fetch_negotiator *negotiator)\n+{\n+\tnoop_negotiator_init(negotiator);\n+}\ndiff --git a/fetch-negotiator.h b/fetch-negotiator.h\nindex ea78868504b..e348905a1f0 100644\n--- a/fetch-negotiator.h\n+++ b/fetch-negotiator.h\n@@ -53,7 +53,15 @@ struct fetch_negotiator {\n \tvoid *data;\n };\n \n+/*\n+ * Initialize a negotiator based on the repository settings.\n+ */\n void fetch_negotiator_init(struct repository *r,\n \t\t\t struct fetch_negotiator *negotiator);\n \n+/*\n+ * Initialize a noop negotiator.\n+ */\n+void fetch_negotiator_init_noop(struct fetch_negotiator *negotiator);\n+\n #endif\n-- \ngitgitgadget\n\n"},{"id":"449471","messageId":"79c409d05422b02f7df9135af39cd0fa565f027a.1645719218.git.gitgitgadget@gmail.com","threadId":"57348","inReplyTo":"pull.1138.v2.git.1645719218.gitgitgadget@gmail.com","subject":"[PATCH v2 4/8] fetch: add --repair option","fromName":"Robert Coup via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-02-24T16:13:34Z","receivedAt":"2022-02-24T16:29:37Z","isPatch":true,"sender":{"key":"robert@coup.net.nz","avatar":"https://avatars.githubusercontent.com/u/59874?v=4"},"body":"From: Robert Coup \n\nTeach fetch and transports the --repair option to force a full fetch\nwithout negotiating common commits with the remote. Use when applying a\nnew partial clone filter to refetch all matching objects.\n\nSigned-off-by: Robert Coup \n---\n Documentation/fetch-options.txt | 9 +++++++++\n builtin/fetch.c | 15 ++++++++++++++-\n transport-helper.c | 3 +++\n transport.c | 4 ++++\n transport.h | 4 ++++\n 5 files changed, 34 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/fetch-options.txt b/Documentation/fetch-options.txt\nindex f9036831898..1131aaad252 100644\n--- a/Documentation/fetch-options.txt\n+++ b/Documentation/fetch-options.txt\n@@ -163,6 +163,15 @@ endif::git-pull[]\n \tbehavior for a remote may be specified with the remote..tagOpt\n \tsetting. See linkgit:git-config[1].\n \n+ifndef::git-pull[]\n+--repair::\n+\tInstead of negotiating with the server to avoid transferring commits and\n+\tassociated objects that are already present locally, this option fetches\n+\tall objects as a fresh clone would. Use this to reapply a partial clone\n+\tfilter from configuration or using `--filter=` when the filter\n+\tdefinition has changed.\n+endif::git-pull[]\n+\n --refmap=::\n \tWhen fetching refs listed on the command line, use the\n \tspecified refspec (can be given more than once) to map the\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 79ee9591859..8e5e590dd6e 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -59,7 +59,7 @@ static int prune_tags = -1; /* unspecified */\n \n static int all, append, dry_run, force, keep, multiple, update_head_ok;\n static int write_fetch_head = 1;\n-static int verbosity, deepen_relative, set_upstream;\n+static int verbosity, deepen_relative, set_upstream, repair;\n static int progress = -1;\n static int enable_auto_gc = 1;\n static int tags = TAGS_DEFAULT, unshallow, update_shallow, deepen;\n@@ -190,6 +190,9 @@ static struct option builtin_fetch_options[] = {\n \tOPT_SET_INT_F(0, \"unshallow\", &unshallow,\n \t\t N_(\"convert to a complete repository\"),\n \t\t 1, PARSE_OPT_NONEG),\n+\tOPT_SET_INT_F(0, \"repair\", &repair,\n+\t\t N_(\"re-fetch without negotiating common commits\"),\n+\t\t 1, PARSE_OPT_NONEG),\n \t{ OPTION_STRING, 0, \"submodule-prefix\", &submodule_prefix, N_(\"dir\"),\n \t\t N_(\"prepend this to submodule path output\"), PARSE_OPT_HIDDEN },\n \tOPT_CALLBACK_F(0, \"recurse-submodules-default\",\n@@ -1296,6 +1299,14 @@ static int check_exist_and_connected(struct ref *ref_map)\n \tif (deepen)\n \t\treturn -1;\n \n+\t/*\n+\t * Similarly, if we need to repair, we always want to perform a full\n+\t * fetch ignoring existing objects.\n+\t */\n+\tif (repair)\n+\t\treturn -1;\n+\n+\n \t/*\n \t * check_connected() allows objects to merely be promised, but\n \t * we need all direct targets to exist.\n@@ -1492,6 +1503,8 @@ static struct transport *prepare_transport(struct remote *remote, int deepen)\n \t\tset_option(transport, TRANS_OPT_DEEPEN_RELATIVE, \"yes\");\n \tif (update_shallow)\n \t\tset_option(transport, TRANS_OPT_UPDATE_SHALLOW, \"yes\");\n+\tif (repair)\n+\t\tset_option(transport, TRANS_OPT_REPAIR, \"yes\");\n \tif (filter_options.choice) {\n \t\tconst char *spec =\n \t\t\texpand_list_objects_filter_spec(&filter_options);\ndiff --git a/transport-helper.c b/transport-helper.c\nindex a0297b0986c..a16a9626421 100644\n--- a/transport-helper.c\n+++ b/transport-helper.c\n@@ -715,6 +715,9 @@ static int fetch_refs(struct transport *transport,\n \tif (data->transport_options.update_shallow)\n \t\tset_helper_option(transport, \"update-shallow\", \"true\");\n \n+\tif (data->transport_options.repair)\n+\t\tset_helper_option(transport, \"repair\", \"true\");\n+\n \tif (data->transport_options.filter_options.choice) {\n \t\tconst char *spec = expand_list_objects_filter_spec(\n \t\t\t&data->transport_options.filter_options);\ndiff --git a/transport.c b/transport.c\nindex 253d6671b1f..e9c9c35b8e5 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -243,6 +243,9 @@ static int set_git_option(struct git_transport_options *opts,\n \t\tlist_objects_filter_die_if_populated(&opts->filter_options);\n \t\tparse_list_objects_filter(&opts->filter_options, value);\n \t\treturn 0;\n+\t} else if (!strcmp(name, TRANS_OPT_REPAIR)) {\n+\t\topts->repair = !!value;\n+\t\treturn 0;\n \t} else if (!strcmp(name, TRANS_OPT_REJECT_SHALLOW)) {\n \t\topts->reject_shallow = !!value;\n \t\treturn 0;\n@@ -377,6 +380,7 @@ static int fetch_refs_via_pack(struct transport *transport,\n \targs.update_shallow = data->options.update_shallow;\n \targs.from_promisor = data->options.from_promisor;\n \targs.filter_options = data->options.filter_options;\n+\targs.repair = data->options.repair;\n \targs.stateless_rpc = transport->stateless_rpc;\n \targs.server_options = transport->server_options;\n \targs.negotiation_tips = data->options.negotiation_tips;\ndiff --git a/transport.h b/transport.h\nindex a0bc6a1e9eb..f3621e8b43c 100644\n--- a/transport.h\n+++ b/transport.h\n@@ -16,6 +16,7 @@ struct git_transport_options {\n \tunsigned update_shallow : 1;\n \tunsigned reject_shallow : 1;\n \tunsigned deepen_relative : 1;\n+\tunsigned repair : 1;\n \n \t/* see documentation of corresponding flag in fetch-pack.h */\n \tunsigned from_promisor : 1;\n@@ -216,6 +217,9 @@ void transport_check_allowed(const char *type);\n /* Filter objects for partial clone and fetch */\n #define TRANS_OPT_LIST_OBJECTS_FILTER \"filter\"\n \n+/* Refetch all objects without negotiating */\n+#define TRANS_OPT_REPAIR \"repair\"\n+\n /* Request atomic (all-or-nothing) updates when pushing */\n #define TRANS_OPT_ATOMIC \"atomic\"\n \n-- \ngitgitgadget\n\n"},{"id":"449474","messageId":"38af2bbee791c09b33932d87244ddba99f7c3dc2.1645719218.git.gitgitgadget@gmail.com","threadId":"57348","inReplyTo":"pull.1138.v2.git.1645719218.gitgitgadget@gmail.com","subject":"[PATCH v2 5/8] t5615-partial-clone: add test for fetch --repair","fromName":"Robert Coup via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-02-24T16:13:35Z","receivedAt":"2022-02-24T16:42:10Z","isPatch":true,"sender":{"key":"robert@coup.net.nz","avatar":"https://avatars.githubusercontent.com/u/59874?v=4"},"body":"From: Robert Coup \n\nAdd a test for doing a repair fetch to apply a changed partial clone\nfilter under protocol v0 and v2.\n\nSigned-off-by: Robert Coup \n---\n t/t5616-partial-clone.sh | 52 +++++++++++++++++++++++++++++++++++++++-\n 1 file changed, 51 insertions(+), 1 deletion(-)\n\ndiff --git a/t/t5616-partial-clone.sh b/t/t5616-partial-clone.sh\nindex 34469b6ac10..230b2dcbc94 100755\n--- a/t/t5616-partial-clone.sh\n+++ b/t/t5616-partial-clone.sh\n@@ -166,6 +166,56 @@ test_expect_success 'manual prefetch of missing objects' '\n \ttest_line_count = 0 observed.oids\n '\n \n+# create new commits in \"src\" repo to establish a history on file.4.txt\n+# and push to \"srv.bare\".\n+test_expect_success 'push new commits to server for file.4.txt' '\n+\tfor x in a b c d e f\n+\tdo\n+\t\techo \"Mod file.4.txt $x\" >src/file.4.txt &&\n+\t\tif list_contains \"a,b\" \"$x\"; then\n+\t\t\tprintf \"%10000s\" X >>src/file.4.txt\n+\t\tfi &&\n+\t\tif list_contains \"c,d\" \"$x\"; then\n+\t\t\tprintf \"%20000s\" X >>src/file.4.txt\n+\t\tfi &&\n+\t\tgit -C src add file.4.txt &&\n+\t\tgit -C src commit -m \"mod $x\" || return 1\n+\tdone &&\n+\tgit -C src push -u srv main\n+'\n+\n+# Do partial fetch to fetch smaller files; then verify that without --repair\n+# applying a new filter does not refetch missing large objects. Then use\n+# --repair to apply the new filter on existing commits. Test it under both\n+# protocol v2 & v0.\n+test_expect_success 'apply a different filter using --repair' '\n+\tgit -C pc1 fetch --filter=blob:limit=999 origin &&\n+\tgit -C pc1 rev-list --quiet --objects --missing=print \\\n+\t\tmain..origin/main >observed &&\n+\ttest_line_count = 4 observed &&\n+\n+\tgit -C pc1 fetch --filter=blob:limit=19999 --repair origin &&\n+\tgit -C pc1 rev-list --quiet --objects --missing=print \\\n+\t\tmain..origin/main >observed &&\n+\ttest_line_count = 2 observed &&\n+\n+\tgit -c protocol.version=0 -C pc1 fetch --filter=blob:limit=29999 \\\n+\t\t--repair origin &&\n+\tgit -C pc1 rev-list --quiet --objects --missing=print \\\n+\t\tmain..origin/main >observed &&\n+\ttest_line_count = 0 observed\n+'\n+\n+test_expect_success 'fetch --repair works with a shallow clone' '\n+\tgit clone --no-checkout --depth=1 --filter=blob:none \"file://$(pwd)/srv.bare\" pc1s &&\n+\tgit -C pc1s rev-list --objects --missing=print HEAD >observed &&\n+\ttest_line_count = 6 observed &&\n+\n+\tGIT_TRACE=1 git -C pc1s fetch --filter=blob:limit=999 --repair origin &&\n+\tgit -C pc1s rev-list --objects --missing=print HEAD >observed &&\n+\ttest_line_count = 6 observed\n+'\n+\n test_expect_success 'partial clone with transfer.fsckobjects=1 works with submodules' '\n \ttest_create_repo submodule &&\n \ttest_commit -C submodule mycommit &&\n@@ -225,7 +275,7 @@ test_expect_success 'use fsck before and after manually fetching a missing subtr\n \n \t# Auto-fetch all remaining trees and blobs with --missing=error\n \tgit -C dst rev-list --missing=error --objects main >fetched_objects &&\n-\ttest_line_count = 70 fetched_objects &&\n+\ttest_line_count = 88 fetched_objects &&\n \n \tawk -f print_1.awk fetched_objects |\n \txargs -n1 git -C dst cat-file -t >fetched_types &&\n-- \ngitgitgadget\n\n"},{"id":"449475","messageId":"cfa6dca8ef4a64b6233e3b7b6021571447fc5ba9.1645719218.git.gitgitgadget@gmail.com","threadId":"57348","inReplyTo":"pull.1138.v2.git.1645719218.gitgitgadget@gmail.com","subject":"[PATCH v2 6/8] maintenance: add ability to pass config options","fromName":"Robert Coup via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-02-24T16:13:36Z","receivedAt":"2022-02-24T16:43:55Z","isPatch":true,"sender":{"key":"robert@coup.net.nz","avatar":"https://avatars.githubusercontent.com/u/59874?v=4"},"body":"From: Robert Coup \n\nMake run_auto_maintenance() accept optional config options for a\nspecific invocation of the auto-maintenance process.\n\nSigned-off-by: Robert Coup \n---\n builtin/am.c | 2 +-\n builtin/commit.c | 2 +-\n builtin/fetch.c | 2 +-\n builtin/merge.c | 2 +-\n builtin/rebase.c | 2 +-\n run-command.c | 8 +++++++-\n run-command.h | 5 ++++-\n 7 files changed, 16 insertions(+), 7 deletions(-)\n\ndiff --git a/builtin/am.c b/builtin/am.c\nindex 7de2c89ef22..298c6093bff 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -1899,7 +1899,7 @@ next:\n \t */\n \tif (!state->rebasing) {\n \t\tam_destroy(state);\n-\t\trun_auto_maintenance(state->quiet);\n+\t\trun_auto_maintenance(state->quiet, NULL);\n \t}\n }\n \ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex b9ed0374e30..84e7ab0a4cc 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -1844,7 +1844,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n \tgit_test_write_commit_graph_or_die();\n \n \trepo_rerere(the_repository, 0);\n-\trun_auto_maintenance(quiet);\n+\trun_auto_maintenance(quiet, NULL);\n \trun_commit_hook(use_editor, get_index_file(), \"post-commit\", NULL);\n \tif (amend && !no_post_rewrite) {\n \t\tcommit_post_rewrite(the_repository, current_head, &oid);\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 8e5e590dd6e..f32b24d182b 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -2227,7 +2227,7 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n \t}\n \n \tif (enable_auto_gc)\n-\t\trun_auto_maintenance(verbosity < 0);\n+\t\trun_auto_maintenance(verbosity < 0, NULL);\n \n cleanup:\n \tstring_list_clear(&list, 0);\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex a94a03384ae..8d3e6d0de03 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -472,7 +472,7 @@ static void finish(struct commit *head_commit,\n \t\t\t * We ignore errors in 'gc --auto', since the\n \t\t\t * user should see them.\n \t\t\t */\n-\t\t\trun_auto_maintenance(verbosity < 0);\n+\t\t\trun_auto_maintenance(verbosity < 0, NULL);\n \t\t}\n \t}\n \tif (new_head && show_diffstat) {\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex d858add3fe8..cbab6c05373 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -552,7 +552,7 @@ static int finish_rebase(struct rebase_options *opts)\n \t * We ignore errors in 'git maintenance run --auto', since the\n \t * user should see them.\n \t */\n-\trun_auto_maintenance(!(opts->flags & (REBASE_NO_QUIET|REBASE_VERBOSE)));\n+\trun_auto_maintenance(!(opts->flags & (REBASE_NO_QUIET|REBASE_VERBOSE)), NULL);\n \tif (opts->type == REBASE_MERGE) {\n \t\tstruct replay_opts replay = REPLAY_OPTS_INIT;\n \ndiff --git a/run-command.c b/run-command.c\nindex a8501e38ceb..720fd7820c8 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -1798,9 +1798,10 @@ int run_processes_parallel_tr2(int n, get_next_task_fn get_next_task,\n \treturn result;\n }\n \n-int run_auto_maintenance(int quiet)\n+int run_auto_maintenance(int quiet, const struct strvec *config_opts)\n {\n \tint enabled;\n+\tint i;\n \tstruct child_process maint = CHILD_PROCESS_INIT;\n \n \tif (!git_config_get_bool(\"maintenance.auto\", &enabled) &&\n@@ -1809,6 +1810,11 @@ int run_auto_maintenance(int quiet)\n \n \tmaint.git_cmd = 1;\n \tmaint.close_object_store = 1;\n+\n+\tif (config_opts)\n+\t\tfor (i = 0; inr; i++)\n+\t\t\tstrvec_pushl(&maint.args, \"-c\", config_opts->v[i], NULL);\n+\n \tstrvec_pushl(&maint.args, \"maintenance\", \"run\", \"--auto\", NULL);\n \tstrvec_push(&maint.args, quiet ? \"--quiet\" : \"--no-quiet\");\n \ndiff --git a/run-command.h b/run-command.h\nindex 07bed6c31b4..24021abd41f 100644\n--- a/run-command.h\n+++ b/run-command.h\n@@ -222,8 +222,11 @@ int run_command(struct child_process *);\n \n /*\n * Trigger an auto-gc\n+ *\n+ * config_opts is an optional list of additional config options to\n+ * pass to the maintenance process in the form \"some.option=value\".\n */\n-int run_auto_maintenance(int quiet);\n+int run_auto_maintenance(int quiet, const struct strvec *config_opts);\n \n #define RUN_COMMAND_NO_STDIN\t\t(1<<0)\n #define RUN_GIT_CMD\t\t\t(1<<1)\n-- \ngitgitgadget\n\n"},{"id":"449478","messageId":"2338c15249a3a58032bc1f8b0cd029f3897b4e88.1645719219.git.gitgitgadget@gmail.com","threadId":"57348","inReplyTo":"pull.1138.v2.git.1645719218.gitgitgadget@gmail.com","subject":"[PATCH v2 7/8] fetch: after repair, encourage auto gc repacking","fromName":"Robert Coup via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-02-24T16:13:37Z","receivedAt":"2022-02-24T16:48:48Z","isPatch":true,"sender":{"key":"robert@coup.net.nz","avatar":"https://avatars.githubusercontent.com/u/59874?v=4"},"body":"From: Robert Coup \n\nAfter invoking `fetch --repair`, the object db will likely contain many\nduplicate objects. If auto-maintenance is enabled, invoke it with\nappropriate settings to encourage repacking/consolidation.\n\n* gc.autoPackLimit: unless this is set to 0 (disabled), override the\n value to 1 to force pack consolidation.\n* maintenance.incremental-repack.auto: unless this is set to 0, override\n the value to -1 to force incremental repacking.\n\nSigned-off-by: Robert Coup \n---\n Documentation/fetch-options.txt | 3 ++-\n builtin/fetch.c | 23 +++++++++++++++++++++--\n t/t5616-partial-clone.sh | 6 ++++--\n 3 files changed, 27 insertions(+), 5 deletions(-)\n\ndiff --git a/Documentation/fetch-options.txt b/Documentation/fetch-options.txt\nindex 1131aaad252..73abafdfc41 100644\n--- a/Documentation/fetch-options.txt\n+++ b/Documentation/fetch-options.txt\n@@ -169,7 +169,8 @@ ifndef::git-pull[]\n \tassociated objects that are already present locally, this option fetches\n \tall objects as a fresh clone would. Use this to reapply a partial clone\n \tfilter from configuration or using `--filter=` when the filter\n-\tdefinition has changed.\n+\tdefinition has changed. Automatic post-fetch maintenance will perform\n+\tobject database pack consolidation to remove any duplicate objects.\n endif::git-pull[]\n \n --refmap=::\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex f32b24d182b..7d023341ac0 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -2020,6 +2020,8 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n \tstruct remote *remote = NULL;\n \tint result = 0;\n \tint prune_tags_ok = 1;\n+\tstruct strvec auto_maint_opts = STRVEC_INIT;\n+\tint opt_val;\n \n \tpacket_trace_identity(\"fetch\");\n \n@@ -2226,10 +2228,27 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n \t\t\t\t\t NULL);\n \t}\n \n-\tif (enable_auto_gc)\n-\t\trun_auto_maintenance(verbosity < 0, NULL);\n+\tif (enable_auto_gc) {\n+\t\tif (repair) {\n+\t\t\t/*\n+\t\t\t * Hint auto-maintenance strongly to encourage repacking,\n+\t\t\t * but respect config settings disabling it.\n+\t\t\t */\n+\t\t\tif (git_config_get_int(\"gc.autopacklimit\", &opt_val))\n+\t\t\t\topt_val = -1;\n+\t\t\tif (opt_val != 0)\n+\t\t\t\tstrvec_push(&auto_maint_opts, \"gc.autoPackLimit=1\");\n+\n+\t\t\tif (git_config_get_int(\"maintenance.incremental-repack.auto\", &opt_val))\n+\t\t\t\topt_val = -1;\n+\t\t\tif (opt_val != 0)\n+\t\t\t\tstrvec_push(&auto_maint_opts, \"maintenance.incremental-repack.auto=-1\");\n+\t\t}\n+\t\trun_auto_maintenance(verbosity < 0, &auto_maint_opts);\n+\t}\n \n cleanup:\n \tstring_list_clear(&list, 0);\n+\tstrvec_clear(&auto_maint_opts);\n \treturn result;\n }\ndiff --git a/t/t5616-partial-clone.sh b/t/t5616-partial-clone.sh\nindex 230b2dcbc94..60f1817cda6 100755\n--- a/t/t5616-partial-clone.sh\n+++ b/t/t5616-partial-clone.sh\n@@ -187,7 +187,7 @@ test_expect_success 'push new commits to server for file.4.txt' '\n # Do partial fetch to fetch smaller files; then verify that without --repair\n # applying a new filter does not refetch missing large objects. Then use\n # --repair to apply the new filter on existing commits. Test it under both\n-# protocol v2 & v0.\n+# protocol v2 & v0. Check repacking auto-maintenance is kicked off.\n test_expect_success 'apply a different filter using --repair' '\n \tgit -C pc1 fetch --filter=blob:limit=999 origin &&\n \tgit -C pc1 rev-list --quiet --objects --missing=print \\\n@@ -199,11 +199,13 @@ test_expect_success 'apply a different filter using --repair' '\n \t\tmain..origin/main >observed &&\n \ttest_line_count = 2 observed &&\n \n+\tGIT_TRACE2_EVENT=\"$(pwd)/trace.log\" \\\n \tgit -c protocol.version=0 -C pc1 fetch --filter=blob:limit=29999 \\\n \t\t--repair origin &&\n \tgit -C pc1 rev-list --quiet --objects --missing=print \\\n \t\tmain..origin/main >observed &&\n-\ttest_line_count = 0 observed\n+\ttest_line_count = 0 observed &&\n+\ttest_subcommand git -c gc.autoPackLimit=1 -c maintenance.incremental-repack.auto=-1 maintenance run --auto --no-quiet writes:\n\n> From: Robert Coup \n>\n> Add a specific initializor for the noop fetch negotiator. This is\n\n\"initializer\"?\n\n> introduced to support allowing partial clones to skip commit negotiation\n> when fetching to repair or apply a modified filter.\n>\n> Signed-off-by: Robert Coup \n> ---\n> fetch-negotiator.c | 5 +++++\n> fetch-negotiator.h | 8 ++++++++\n> 2 files changed, 13 insertions(+)\n>\n> diff --git a/fetch-negotiator.c b/fetch-negotiator.c\n> index 874797d767b..be383367f55 100644\n> --- a/fetch-negotiator.c\n> +++ b/fetch-negotiator.c\n> @@ -23,3 +23,8 @@ void fetch_negotiator_init(struct repository *r,\n> \t\treturn;\n> \t}\n> }\n> +\n> +void fetch_negotiator_init_noop(struct fetch_negotiator *negotiator)\n> +{\n> +\tnoop_negotiator_init(negotiator);\n> +}\n\nPuzzling. What makes this better than allowing noop-negotiator-init\nto be called directly?\n\n> diff --git a/fetch-negotiator.h b/fetch-negotiator.h\n> index ea78868504b..e348905a1f0 100644\n> --- a/fetch-negotiator.h\n> +++ b/fetch-negotiator.h\n> @@ -53,7 +53,15 @@ struct fetch_negotiator {\n> \tvoid *data;\n> };\n> \n> +/*\n> + * Initialize a negotiator based on the repository settings.\n> + */\n> void fetch_negotiator_init(struct repository *r,\n> \t\t\t struct fetch_negotiator *negotiator);\n> \n> +/*\n> + * Initialize a noop negotiator.\n> + */\n> +void fetch_negotiator_init_noop(struct fetch_negotiator *negotiator);\n> +\n> #endif\n"},{"id":"449550","messageId":"xmqqwnhjfnpy.fsf@gitster.g","threadId":"57348","inReplyTo":"2d817a65db55750d27986c2e2ab5b1723d17081e.1645719218.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 2/8] fetch-pack: add repairing","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-02-25T06:46:01Z","receivedAt":"2022-02-25T06:46:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Robert Coup via GitGitGadget\" writes:\n\n> @@ -694,6 +696,9 @@ static void mark_complete_and_common_ref(struct fetch_negotiator *negotiator,\n> \n> \tsave_commit_buffer = 0;\n> \n> +\tif (args->repair)\n> +\t\treturn;\n> +\n\nReading how the original value of save_commit_buffer is saved away,\nthe variable gets cleared and then gets restored before the function\nreturns in the normal codepath, this new code looks wrong. Hitting\nthis early return after clearing the variable means nobody will\nrestore the saved value of the variable, no?\n\n> @@ -1027,9 +1032,6 @@ static struct ref *do_fetch_pack(struct fetch_pack_args *args,\n> \tstruct fetch_negotiator negotiator_alloc;\n> \tstruct fetch_negotiator *negotiator;\n> \n> -\tnegotiator = &negotiator_alloc;\n> -\tfetch_negotiator_init(r, negotiator);\n\nI know why you want to force the \"noop\" negotiator while repairing,\nbut it is unclear why you need to move this down in the function.\n\n> \tsort_ref_list(&ref, ref_compare_name);\n> \tQSORT(sought, nr_sought, cmp_ref_by_name);\n> \n> @@ -1119,9 +1121,16 @@ static struct ref *do_fetch_pack(struct fetch_pack_args *args,\n> \tif (!server_supports_hash(the_hash_algo->name, NULL))\n> \t\tdie(_(\"Server does not support this repository's object format\"));\n> \n> +\tnegotiator = &negotiator_alloc;\n> +\tif (args->repair) {\n> +\t\tfetch_negotiator_init_noop(negotiator);\n> +\t} else {\n> +\t\tfetch_negotiator_init(r, negotiator);\n> +\t}\n\nHmph. I am debating myself if hardcoding the implementation detail\nof \"when repairing, the noop negitiator is the only useful one\" like\nthis code does is a sensible thing to do. If we later need to tweak\nthe choice of negotiator used depending on the caller's needs,\nperhaps fetch_negotiator_init() should gain a new flags word, i.e.\n\n\tfetch_negotiator_init(struct repository *,\n\t\t\t struct fetch_negotiator *,\n\t\t\t unsigned flags)\n\nwhere \"Use negotiator suitable for the repairing fetch\" could be a\nsingle bit in the flags word, making this caller more like:\n\n\tnegotiator = &negotiator_alloc;\n\tflags = 0;\n\tif (args->repair)\n\t\tflags |= FETCH_NEGOTIATOR_REPAIRING;\n\tfetch_negotiator_init(r, negotiator, flags);\n\nperhaps. That way, [1/8] becomes unnecessary.\n\n> \tmark_complete_and_common_ref(negotiator, args, &ref);\n> \tfilter_refs(args, &ref, sought, nr_sought);\n> -\tif (everything_local(args, &ref)) {\n> +\tif (!args->repair && everything_local(args, &ref)) {\n> \t\tpacket_flush(fd[1]);\n> \t\tgoto all_done;\n> \t}\n> @@ -1587,7 +1596,10 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,\n> \tstruct strvec index_pack_args = STRVEC_INIT;\n> \n> \tnegotiator = &negotiator_alloc;\n> -\tfetch_negotiator_init(r, negotiator);\n> +\tif (args->repair)\n> +\t\tfetch_negotiator_init_noop(negotiator);\n> +\telse\n> +\t\tfetch_negotiator_init(r, negotiator);\n\nLikewise.\n\n> \tpacket_reader_init(&reader, fd[0], NULL, 0,\n> \t\t\t PACKET_READ_CHOMP_NEWLINE |\n> @@ -1613,7 +1625,7 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,\n> \t\t\t/* Filter 'ref' by 'sought' and those that aren't local */\n> \t\t\tmark_complete_and_common_ref(negotiator, args, &ref);\n> \t\t\tfilter_refs(args, &ref, sought, nr_sought);\n> -\t\t\tif (everything_local(args, &ref))\n> +\t\t\tif (!args->repair && everything_local(args, &ref))\n> \t\t\t\tstate = FETCH_DONE;\n> \t\t\telse\n> \t\t\t\tstate = FETCH_SEND_REQUEST;\n> diff --git a/fetch-pack.h b/fetch-pack.h\n> index 7f94a2a5831..bbb663edda8 100644\n> --- a/fetch-pack.h\n> +++ b/fetch-pack.h\n> @@ -42,6 +42,7 @@ struct fetch_pack_args {\n> \tunsigned update_shallow:1;\n> \tunsigned reject_shallow_remote:1;\n> \tunsigned deepen:1;\n> +\tunsigned repair:1;\n> \n> \t/*\n> \t * Indicate that the remote of this request is a promisor remote. The\n"},{"id":"449551","messageId":"xmqqsfs7fn6v.fsf@gitster.g","threadId":"57348","inReplyTo":"cfa6dca8ef4a64b6233e3b7b6021571447fc5ba9.1645719218.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 6/8] maintenance: add ability to pass config options","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-02-25T06:57:28Z","receivedAt":"2022-02-25T06:57:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Robert Coup via GitGitGadget\" writes:\n\n> +\tif (config_opts)\n> +\t\tfor (i = 0; inr; i++)\n\nStyle. SP on both sides of '<'.\n\n> +\t\t\tstrvec_pushl(&maint.args, \"-c\", config_opts->v[i], NULL);\n\n> \tstrvec_pushl(&maint.args, \"maintenance\", \"run\", \"--auto\", NULL);\n> \tstrvec_push(&maint.args, quiet ? \"--quiet\" : \"--no-quiet\");\n\nIt is unclear if it is generally a good idea to pass hardcoded set\nof configuration variables to begin with, but provided if it makes\nsense [*], the implementation seems OK.\n\n\tSide note. And this is a big *IF*, as we can see in all\n\tother helper functions in run-commands.h, nobody has such a\n\tprivision. If supporting such a \"feature\" makes sense, we\n\tprobably would need to do so with a common interface that\n\tcan be used across run_command() API, not with an ad-hoc\n\tinterface that is only usable with run_auto_maintenance(),\n\twhich may look somewhat similar to how we have a common way\n\tto pass set of environment variables.\n\n> diff --git a/run-command.h b/run-command.h\n> index 07bed6c31b4..24021abd41f 100644\n> --- a/run-command.h\n> +++ b/run-command.h\n> @@ -222,8 +222,11 @@ int run_command(struct child_process *);\n> \n> /*\n> * Trigger an auto-gc\n> + *\n> + * config_opts is an optional list of additional config options to\n> + * pass to the maintenance process in the form \"some.option=value\".\n> */\n> -int run_auto_maintenance(int quiet);\n> +int run_auto_maintenance(int quiet, const struct strvec *config_opts);\n> \n> #define RUN_COMMAND_NO_STDIN\t\t(1<<0)\n> #define RUN_GIT_CMD\t\t\t(1<<1)\nn\n"},{"id":"449567","messageId":"220225.86tucn8ch4.gmgdl@evledraar.gmail.com","threadId":"57348","inReplyTo":"cfa6dca8ef4a64b6233e3b7b6021571447fc5ba9.1645719218.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 6/8] maintenance: add ability to pass config options","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-02-25T10:29:51Z","receivedAt":"2022-02-25T10:30:53Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Feb 24 2022, Robert Coup via GitGitGadget wrote:\n\n> From: Robert Coup \n>\n> Make run_auto_maintenance() accept optional config options for a\n> specific invocation of the auto-maintenance process.\n> [...]\n> -int run_auto_maintenance(int quiet)\n> +int run_auto_maintenance(int quiet, const struct strvec *config_opts)\n> {\n> \tint enabled;\n> +\tint i;\n> \tstruct child_process maint = CHILD_PROCESS_INIT;\n> \n> \tif (!git_config_get_bool(\"maintenance.auto\", &enabled) &&\n> @@ -1809,6 +1810,11 @@ int run_auto_maintenance(int quiet)\n> \n> \tmaint.git_cmd = 1;\n> \tmaint.close_object_store = 1;\n> +\n> +\tif (config_opts)\n> +\t\tfor (i = 0; inr; i++)\n> +\t\t\tstrvec_pushl(&maint.args, \"-c\", config_opts->v[i], NULL);\n> +\n> \tstrvec_pushl(&maint.args, \"maintenance\", \"run\", \"--auto\", NULL);\n> \tstrvec_push(&maint.args, quiet ? \"--quiet\" : \"--no-quiet\");\n> \n> diff --git a/run-command.h b/run-command.h\n> index 07bed6c31b4..24021abd41f 100644\n> --- a/run-command.h\n> +++ b/run-command.h\n> @@ -222,8 +222,11 @@ int run_command(struct child_process *);\n> \n> /*\n> * Trigger an auto-gc\n> + *\n> + * config_opts is an optional list of additional config options to\n> + * pass to the maintenance process in the form \"some.option=value\".\n> */\n> -int run_auto_maintenance(int quiet);\n> +int run_auto_maintenance(int quiet, const struct strvec *config_opts);\n> \n> #define RUN_COMMAND_NO_STDIN\t\t(1<<0)\n> #define RUN_GIT_CMD\t\t\t(1<<1)\n\n\nShouldn't this bei using git grep the git_config_push.*parameter()\nfunctions instead of adding a new custom method to do this.\n\nPerhaps there's some subtle distinction between the two that's important\nhere, or perhaps you just didn't know about that API...\n"},{"id":"449748","messageId":"CACf-nVd77JCO9tGcO3ELg3975H73_+yuTi2GvR96rpqFh0zS0w@mail.gmail.com","threadId":"57348","inReplyTo":"220225.86tucn8ch4.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH v2 6/8] maintenance: add ability to pass config options","fromName":"Robert Coup","fromEmail":"robert@coup.net.nz","sentAt":"2022-02-28T11:51:42Z","receivedAt":"2022-02-28T11:51:57Z","isPatch":true,"sender":{"key":"robert@coup.net.nz","avatar":"https://avatars.githubusercontent.com/u/59874?v=4"},"body":"Hi Ævar,\n\nOn Fri, 25 Feb 2022 at 10:30, Ævar Arnfjörð Bjarmason wrote:\n>\n> Shouldn't this bei using git grep the git_config_push.*parameter()\n> functions instead of adding a new custom method to do this.\n>\n> Perhaps there's some subtle distinction between the two that's important\n> here, or perhaps you just didn't know about that API...\n\nI didn't know of it — it could probably use a docstring too ;-) A few\nother sub-commands seemed to use the `-c key=value` approach so it\ndidn't seem like GIT_CONFIG_PARAMETERS was universal.\n\nEasy enough to swap to, thanks. Assuming we want it, but I'm about to\nreply to Junio's email.\n\nRob :)\n"},{"id":"449749","messageId":"CACf-nVeh9p1ba+tPSFQvqDBq3SaNtnhCQMibd__EED0w0Btozg@mail.gmail.com","threadId":"57348","inReplyTo":"xmqqsfs7fn6v.fsf@gitster.g","subject":"Re: [PATCH v2 6/8] maintenance: add ability to pass config options","fromName":"Robert Coup","fromEmail":"robert@coup.net.nz","sentAt":"2022-02-28T12:02:31Z","receivedAt":"2022-02-28T12:02:45Z","isPatch":true,"sender":{"key":"robert@coup.net.nz","avatar":"https://avatars.githubusercontent.com/u/59874?v=4"},"body":"Hi Junio,\n\nOn Fri, 25 Feb 2022 at 06:57, Junio C Hamano wrote:\n>\n> Style. SP on both sides of '<'.\n\nThanks\n\n> It is unclear if it is generally a good idea to pass hardcoded set\n> of configuration variables to begin with, but provided if it makes\n> sense [*], the implementation seems OK.\n\nMy RFC didn't do any repacking, and you (rightly) raised it at\nhttps://lore.kernel.org/git/xmqqk0eecpl9.fsf@gitster.g/\n\n>> To note:\n>>\n>> 1. This will produce duplicated objects between the existing and newly\n>> fetched packs, but gc will clean them up.\n>\n> ... it is not smart enough to stell them to exclude what we _ought_\n> to have by telling them what the _old_ filter spec was. That's OK\n> for a starting point, I guess. Hopefully, at the end of this\n> operation, we should garbage collect the duplicated objects by\n> default (with an option to turn it off)?\n\nThis was my best stab at (I think) safely doing it — it respects users\nwho have disabled auto-gc/repacking/maintenance, and doesn't add yet\nanother config variable. But we could also:\n\n1. do nothing — the user's object DB will contain duplicate objects\nuntil some repacking happens. This could be up to twice the size it\n\"should\" be.\n2. print a message to suggest running `git gc` / `git maintenance`\n\nI'm keen on some input from Derrick or someone deeply familiar with\nthe maintenance/GC bits (particularly wrt incremental repacking)\nwhether what I'm doing would cause issues for people with complex\nsetups I haven't thought of.\n\n> Side note. And this is a big *IF*, as we can see in all\n> other helper functions in run-commands.h, nobody has such a\n> privision. If supporting such a \"feature\" makes sense, we\n> probably would need to do so with a common interface that\n> can be used across run_command() API, not with an ad-hoc\n> interface that is only usable with run_auto_maintenance(),\n> which may look somewhat similar to how we have a common way\n> to pass set of environment variables.\n\nÆvar pointed out GIT_CONFIG_PARAMETERS has an API to wrap it's use, so\nI can adapt the implementation to use that.\n\nThanks\n\nRob :)\n"},{"id":"449750","messageId":"CACf-nVe=vK=eGeiX4s4fBjEevfMp5jogP=Eq2f-kbfiNoOOfsw@mail.gmail.com","threadId":"57348","inReplyTo":"xmqqwnhjfnpy.fsf@gitster.g","subject":"Re: [PATCH v2 2/8] fetch-pack: add repairing","fromName":"Robert Coup","fromEmail":"robert@coup.net.nz","sentAt":"2022-02-28T12:14:54Z","receivedAt":"2022-02-28T12:15:08Z","isPatch":true,"sender":{"key":"robert@coup.net.nz","avatar":"https://avatars.githubusercontent.com/u/59874?v=4"},"body":"Hi Junio,\n\nOn Fri, 25 Feb 2022 at 06:46, Junio C Hamano wrote:\n>\n> \"Robert Coup via GitGitGadget\" writes:\n>\n> > @@ -694,6 +696,9 @@ static void mark_complete_and_common_ref(struct fetch_negotiator *negotiator,\n> >\n> > save_commit_buffer = 0;\n> >\n> > + if (args->repair)\n> > + return;\n> > +\n>\n> Reading how the original value of save_commit_buffer is saved away,\n> the variable gets cleared and then gets restored before the function\n> returns in the normal codepath, this new code looks wrong. Hitting\n> this early return after clearing the variable means nobody will\n> restore the saved value of the variable, no?\n\nGood spotting, thank you.\n\n>\n> > @@ -1027,9 +1032,6 @@ static struct ref *do_fetch_pack(struct fetch_pack_args *args,\n> > struct fetch_negotiator negotiator_alloc;\n> > struct fetch_negotiator *negotiator;\n> >\n> > - negotiator = &negotiator_alloc;\n> > - fetch_negotiator_init(r, negotiator);\n>\n> I know why you want to force the \"noop\" negotiator while repairing,\n> but it is unclear why you need to move this down in the function.\n\nSeemed cleaner to initialise the right negotiator once, rather than\nclearing and re-initialising depending on repair mode.\n\n> Hmph. I am debating myself if hardcoding the implementation detail\n> of \"when repairing, the noop negitiator is the only useful one\" like\n> this code does is a sensible thing to do. If we later need to tweak\n> the choice of negotiator used depending on the caller's needs,\n> perhaps fetch_negotiator_init() should gain a new flags word, i.e.\n\nTo me this feels a bit hypothetical, but maybe I'm missing a use case?\nThe point of repairing is not to negotiate common commits and do\n(effectively) a clone-style fresh fetch. If some future special\nnegotiator that has a repair mode arrives, or likewise a more complex\nrepair mode then other things will probably need adapting?\n\n> where \"Use negotiator suitable for the repairing fetch\" could be a\n> single bit in the flags word, making this caller more like:\n>\n> negotiator = &negotiator_alloc;\n> flags = 0;\n> if (args->repair)\n> flags |= FETCH_NEGOTIATOR_REPAIRING;\n> fetch_negotiator_init(r, negotiator, flags);\n>\n> perhaps. That way, [1/8] becomes unnecessary.\n\nWith the current patch it is clear what's happening, that the user's\nnegotiator selection is deliberately being ignored for the purposes of\nrepairing. Conversely, calling negotiator_init() asking for a skipping\nnegotiator in repair mode and getting back a noop negotiator seems\nunobvious.\n\nThanks,\n\nRob :)\n"},{"id":"449751","messageId":"CACf-nVcLXcD6G_b_pYWj+7KJBu4iV6FqHWB8U6F4j6Rr72zXVg@mail.gmail.com","threadId":"57348","inReplyTo":"xmqqbkyvh3i3.fsf@gitster.g","subject":"Re: [PATCH v2 1/8] fetch-negotiator: add specific noop initializor","fromName":"Robert Coup","fromEmail":"robert@coup.net.nz","sentAt":"2022-02-28T12:22:35Z","receivedAt":"2022-02-28T12:23:08Z","isPatch":true,"sender":{"key":"robert@coup.net.nz","avatar":"https://avatars.githubusercontent.com/u/59874?v=4"},"body":"Hi,\n\nOn Fri, 25 Feb 2022 at 06:19, Junio C Hamano wrote:\n>\n> > Add a specific initializor for the noop fetch negotiator. This is\n>\n> \"initializer\"?\n\nThanks. A poor translation effort to American English ;-)\n\n> > +\n> > +void fetch_negotiator_init_noop(struct fetch_negotiator *negotiator)\n> > +{\n> > + noop_negotiator_init(negotiator);\n> > +}\n>\n> Puzzling. What makes this better than allowing noop-negotiator-init\n> to be called directly?\n\nIs simply for naming/API consistency with the regular method. Happy to\ncall noop_negotiator_init() if you prefer?\n\nRob :)\n"},{"id":"449780","messageId":"220228.86v8wz2b85.gmgdl@evledraar.gmail.com","threadId":"57348","inReplyTo":"2338c15249a3a58032bc1f8b0cd029f3897b4e88.1645719219.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 7/8] fetch: after repair, encourage auto gc repacking","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-02-28T16:40:30Z","receivedAt":"2022-02-28T16:43:28Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Feb 24 2022, Robert Coup via GitGitGadget wrote:\n\n> From: Robert Coup \n>\n> After invoking `fetch --repair`, the object db will likely contain many\n> duplicate objects. If auto-maintenance is enabled, invoke it with\n> appropriate settings to encourage repacking/consolidation.\n>\n> * gc.autoPackLimit: unless this is set to 0 (disabled), override the\n> value to 1 to force pack consolidation.\n> * maintenance.incremental-repack.auto: unless this is set to 0, override\n> the value to -1 to force incremental repacking.\n>\n> Signed-off-by: Robert Coup \n> ---\n> Documentation/fetch-options.txt | 3 ++-\n> builtin/fetch.c | 23 +++++++++++++++++++++--\n> t/t5616-partial-clone.sh | 6 ++++--\n> 3 files changed, 27 insertions(+), 5 deletions(-)\n>\n> diff --git a/Documentation/fetch-options.txt b/Documentation/fetch-options.txt\n> index 1131aaad252..73abafdfc41 100644\n> --- a/Documentation/fetch-options.txt\n> +++ b/Documentation/fetch-options.txt\n> @@ -169,7 +169,8 @@ ifndef::git-pull[]\n> \tassociated objects that are already present locally, this option fetches\n> \tall objects as a fresh clone would. Use this to reapply a partial clone\n> \tfilter from configuration or using `--filter=` when the filter\n> -\tdefinition has changed.\n> +\tdefinition has changed. Automatic post-fetch maintenance will perform\n> +\tobject database pack consolidation to remove any duplicate objects.\n> endif::git-pull[]\n> \n> --refmap=::\n> diff --git a/builtin/fetch.c b/builtin/fetch.c\n> index f32b24d182b..7d023341ac0 100644\n> --- a/builtin/fetch.c\n> +++ b/builtin/fetch.c\n> @@ -2020,6 +2020,8 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n> \tstruct remote *remote = NULL;\n> \tint result = 0;\n> \tint prune_tags_ok = 1;\n> +\tstruct strvec auto_maint_opts = STRVEC_INIT;\n\n[Nits, but aside from earlier comments about config options v.s. config[]\n\nthis variable...\n\n> +\tint opt_val;\n\n...and this...\n> \n> \tpacket_trace_identity(\"fetch\");\n> \n> @@ -2226,10 +2228,27 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n> \t\t\t\t\t NULL);\n> \t}\n> \n> -\tif (enable_auto_gc)\n> -\t\trun_auto_maintenance(verbosity < 0, NULL);\n> +\tif (enable_auto_gc) {\n\n...can just be declared in this scope.\n\n> +\t\tif (repair) {\n\nI think having:\n\n if (enable_auto_gc && repair)\n\nMight make this more readable without the extra indentation, but of\ncourse then the variables need to be at the top-level... :)\n\n> +\t\t\t/*\n> +\t\t\t * Hint auto-maintenance strongly to encourage repacking,\n> +\t\t\t * but respect config settings disabling it.\n> +\t\t\t */\n> +\t\t\tif (git_config_get_int(\"gc.autopacklimit\", &opt_val))\n> +\t\t\t\topt_val = -1;\n> +\t\t\tif (opt_val != 0)\n> +\t\t\t\tstrvec_push(&auto_maint_opts, \"gc.autoPackLimit=1\");\n> +\n> +\t\t\tif (git_config_get_int(\"maintenance.incremental-repack.auto\", &opt_val))\n> +\t\t\t\topt_val = -1;\n> +\t\t\tif (opt_val != 0)\n> +\t\t\t\tstrvec_push(&auto_maint_opts, \"maintenance.incremental-repack.auto=-1\");\n> +\t\t}\n> +\t\trun_auto_maintenance(verbosity < 0, &auto_maint_opts);\n> +\t}\n> \n> cleanup:\n> \tstring_list_clear(&list, 0);\n> +\tstrvec_clear(&auto_maint_opts);\n> \treturn result;\n> }\n> diff --git a/t/t5616-partial-clone.sh b/t/t5616-partial-clone.sh\n> index 230b2dcbc94..60f1817cda6 100755\n> --- a/t/t5616-partial-clone.sh\n> +++ b/t/t5616-partial-clone.sh\n> @@ -187,7 +187,7 @@ test_expect_success 'push new commits to server for file.4.txt' '\n> # Do partial fetch to fetch smaller files; then verify that without --repair\n> # applying a new filter does not refetch missing large objects. Then use\n> # --repair to apply the new filter on existing commits. Test it under both\n> -# protocol v2 & v0.\n> +# protocol v2 & v0. Check repacking auto-maintenance is kicked off.\n> test_expect_success 'apply a different filter using --repair' '\n> \tgit -C pc1 fetch --filter=blob:limit=999 origin &&\n> \tgit -C pc1 rev-list --quiet --objects --missing=print \\\n> @@ -199,11 +199,13 @@ test_expect_success 'apply a different filter using --repair' '\n> \t\tmain..origin/main >observed &&\n> \ttest_line_count = 2 observed &&\n> \n> +\tGIT_TRACE2_EVENT=\"$(pwd)/trace.log\" \\\n\nNit: Better to use $PWD instead of $(pwd), works here, but won't be\ncompatible with -x if we ever want to test stderr.\n\n> \tgit -c protocol.version=0 -C pc1 fetch --filter=blob:limit=29999 \\\n> \t\t--repair origin &&\n> \tgit -C pc1 rev-list --quiet --objects --missing=print \\\n> \t\tmain..origin/main >observed &&\n> -\ttest_line_count = 0 observed\n> +\ttest_line_count = 0 observed &&\n> +\ttest_subcommand git -c gc.autoPackLimit=1 -c maintenance.incremental-repack.auto=-1 maintenance run --auto --no-quiet '\n> \n> test_expect_success 'fetch --repair works with a shallow clone' '\n\n"},{"id":"449782","messageId":"220228.86r17n2aqd.gmgdl@evledraar.gmail.com","threadId":"57348","inReplyTo":"pull.1138.v2.git.1645719218.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 0/8] fetch: add repair: full refetch without negotiation (was: \"refiltering\")","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-02-28T16:43:28Z","receivedAt":"2022-02-28T16:54:09Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Feb 24 2022, Robert Coup via GitGitGadget wrote:\n\n> [...] While a key use case is described\n> above for partial clones, a user could also use --repair to fix a corrupted\n> object database by performing a refetch of objects that should already be\n> present, establishing a better workflow than deleting the local repository\n> and re-cloning.\n>\n> * Using --repair will produce duplicated objects between the existing and\n> newly fetched packs, but maintenance will clean them up when it runs\n> automatically post-fetch (if enabled).\n> * If a user fetches with --repair applying a more restrictive partial clone\n> filter than previously (eg: blob:limit=1m then blob:limit=1k) the\n> eventual state is a no-op, since any referenced object already in the\n> local repository is never removed. More advanced repacking which could\n> improve this scenario is currently proposed at [2].\n\nI realize this was probably based on feedback on v1 (I didn't go back\nand re-read it, sorry).\n\nBut I feel strongly that we really should name this something other than\n--repair. I don't care much if it isn't that :) But maybe\n--expand-filters, --fleshen-partial or something like that?\n\nSo first (and partially as an aside): Is a \"noop\" negotiatior really\nwant we want at all? Don't we instead want to be discovering those parts\nof our history that are closed under reachability (if any) and say we\nHAVE those things during negotiation?\n\nI haven't tested, but maybe that's just more complex, e.g. you have a\nfilter that's excluding >500MB blobs or whatever you might have the full\nhistory already, or maybe that 500MB blob was added last week, so you\nhave almost all of it.\n\nBut wouldn't that be a lot kinder to server resources + network at the\nexpense of some (presumably rare) extra local computation?\n\nBut secondly, on the \"--repair\" name: The reason I mentioned that is\nthat I'd really like us to actually have a \"my repo is screwed, please\nrepair it\".\n\nBut (and I haven't tested, but I'm pretty sure), this patch series isn't\ngoing to give you that. The reasons are elaborated on in [1], basically\nwe try really hard to re-use local data, and due to that & the collision\ndetection will often just hard die early in object walking.\n\nBut maybe I'm wrong, have you actually tested this with *broken* objects\nas opposed to just missing ones with repo filters + promisors in play?\nOur t/*fsck* and t/*corrupt*/ etc. tests have some of those.\n\nAnd maybe I'm making a big deal out of nothing, but I fear that by\nnaming it --repair and giving it these semantics that we'd be closing\nthe door on things that are actually needed for some of the trickier\nedge cases when it comes to repairing a bad repository.\n\nIncluding but not limited to having a loose BAD_OBJ, and needing to\nreplace it with another loose object (due to the unpack limit), the\nbranch we're updating can't be read locally, but is at an OID that's\n(re-)included in the incoming pack and is hopefully about to repair our\nrepository.\n\nOr even that we have a SHA-1 collision, but we intentionally want to\noverride the collision detection because we know our local repo is bad,\nbut the remote can be trusted.\n\nAll of which are much more involved than just the \"fleshen partial data\"\nyou're aiming for here...\n\n1. https://lore.kernel.org/git/87czo7haha.fsf@evledraar.gmail.com/\n"},{"id":"449785","messageId":"xmqq1qzm538p.fsf@gitster.g","threadId":"57348","inReplyTo":"CACf-nVeh9p1ba+tPSFQvqDBq3SaNtnhCQMibd__EED0w0Btozg@mail.gmail.com","subject":"Re: [PATCH v2 6/8] maintenance: add ability to pass config options","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-02-28T17:07:34Z","receivedAt":"2022-02-28T17:07:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Robert Coup writes:\n\n> Ævar pointed out GIT_CONFIG_PARAMETERS has an API to wrap it's use, so\n> I can adapt the implementation to use that.\n\nDepending on how that \"API to wrap\" looks like, it may not be a wise\nidea, though. You'd need to consider how it interacts with those\nwho run \"git -c var=val\" to enter this codepath.\n"},{"id":"449787","messageId":"CACf-nVcy8xsf+STJoE5vwcsUauHRcR5wmwmCfnUnSW=4jDcgYQ@mail.gmail.com","threadId":"57348","inReplyTo":"220228.86r17n2aqd.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH v2 0/8] fetch: add repair: full refetch without negotiation (was: \"refiltering\")","fromName":"Robert Coup","fromEmail":"robert@coup.net.nz","sentAt":"2022-02-28T17:27:45Z","receivedAt":"2022-02-28T17:28:35Z","isPatch":true,"sender":{"key":"robert@coup.net.nz","avatar":"https://avatars.githubusercontent.com/u/59874?v=4"},"body":"Hi Ævar,\n\nThanks for taking the time to look into this,\n\nOn Mon, 28 Feb 2022 at 16:54, Ævar Arnfjörð Bjarmason wrote:\n> I realize this was probably based on feedback on v1 (I didn't go back\n> and re-read it, sorry).\n\nYes, `fetch --repair` was from Jonathan Tan's v1 feedback[1], where he\npointed out it could fill in lost objects from any remote in a more\ngenerally useful fashion.\n\nMy goal here is to refetch with a different filter so that I get the\noutcome of a `clone --filter=` without having to chuck my object\ndirectory. But the actual implementation doesn't need to know anything\nspecific about filters, so the original \"refilter\" name I had isn't\nreally right.\n\n> But I feel strongly that we really should name this something other than\n> --repair. I don't care much if it isn't that :) But maybe\n> --expand-filters, --fleshen-partial or something like that?\n\nfleshen-partial sounds like a horror movie scene to me.\n\n1. `--refetch`\n2. `--as-clone`\n3. `--expand-filter` (though TBC you don't necessarily need a filter)\n4. `--refilter`\n5. something else\n\n> So first (and partially as an aside): Is a \"noop\" negotiatior really\n> want we want at all? Don't we instead want to be discovering those parts\n> of our history that are closed under reachability (if any) and say we\n> HAVE those things during negotiation?\n\nAt an object level we don't have any means of knowing what has or\nhasn't been obtained via fetch to a partial clone with different\n`--filter` args (via config or cli), dynamic fault-ins, or sourced\nfrom a different remote. Fetch negotiation only occurs for refs and\ntheir associated commits/histories, but filtering occurs at the blob\nor tree level — so we often HAVE a commit but not all of its\ntrees/blobs, whereupon negotiation skips that commit and all it's\nassociated objects.\n\n> But secondly, on the \"--repair\" name: The reason I mentioned that is\n> that I'd really like us to actually have a \"my repo is screwed, please\n> repair it\".\n\nFeels like people would look at `fsck` for that over `fetch`? Maybe\nnot. Anyway, I get the point about the naming still not being right\n:-)\n\n> But (and I haven't tested, but I'm pretty sure), this patch series isn't\n> going to give you that. The reasons are elaborated on in [1], basically\n> we try really hard to re-use local data, and due to that & the collision\n> detection will often just hard die early in object walking.\n>\n> But maybe I'm wrong, have you actually tested this with *broken* objects\n> as opposed to just missing ones with repo filters + promisors in play?\n> Our t/*fsck* and t/*corrupt*/ etc. tests have some of those.\n\nCorrect: I haven't tested with such objects/broken ODBs. Ideally\nrepack/gc/etc would prefer a new-fixed pack over the old-broken\npack/object but that's not really what I'm aiming to achieve here or\nam interested in.\n\n1. https://lore.kernel.org/git/20220202185957.1928631-1-jonathantanmy@google.com/\n\nThanks,\n\nRob :)\n"},{"id":"449799","messageId":"xmqqpmn6255r.fsf@gitster.g","threadId":"57348","inReplyTo":"CACf-nVcy8xsf+STJoE5vwcsUauHRcR5wmwmCfnUnSW=4jDcgYQ@mail.gmail.com","subject":"Re: [PATCH v2 0/8] fetch: add repair: full refetch without negotiation","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-02-28T18:54:24Z","receivedAt":"2022-02-28T18:54:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Robert Coup writes:\n\n> fleshen-partial sounds like a horror movie scene to me.\n>\n> 1. `--refetch`\n\nI do think, as part of a larger \"repair\" task, fetching objects\nagain from a known-good remote would be a sensible one step. So\n\"re\"fetch would be a good way to phrase it.\n\n> 2. `--as-clone`\n\nThis I think is in the same vein.\n\n> 3. `--expand-filter` (though TBC you don't necessarily need a filter)\n> 4. `--refilter`\n\nThese focus on a wrong thing, I would think. \"I suspect my object\nstore may be missing some objects, and I want to repair it by\ngetting another copy from a known-good remote\" would be the simplest\nand easiest-to-explain form of the end-user requirement this feature\nhelps. The end-user wish does not require \"filter\" at all.\n\nBut this reminds of me about a few caveats.\n\nThe patch does implement a good way to lie about what we have to the\nother end, forcing them to send everything that is necessary to\ncreate a new clone.\n\nBut that is only a half of what is required to \"repair a possibly\nbroken object store by re-fetching\". It is sufficient for repairing\n\"missing objects\", but not enough for repairing \"broken/corrupt\"\nobjects. The object store has a mechanism to resist collision\nattacks by ignoring objects coming over the network that do not\nmatch what we locally already have.\n\nI offhand do not think it is a practical problem that this new\nmechanism is incapable of repair a repository with broken/corrupt\nobjects [*]. But tying the bland and broad word \"repair\" to the\nfeature that is meant to only deal with \"missing\" but not \"curropt\"\nform of object store breakage may lead to confusion.\n\n\tSide note: others who spent more time on partial/lazy clone\n\tmight realize that broken/corrupt objects are real problems\n\tthey want to tackle, though. I dunno, and that is why I am\n\traising it as a potential issue.\n\nThanks.\n"},{"id":"449814","messageId":"220228.86ee3m39jf.gmgdl@evledraar.gmail.com","threadId":"57348","inReplyTo":"CACf-nVcy8xsf+STJoE5vwcsUauHRcR5wmwmCfnUnSW=4jDcgYQ@mail.gmail.com","subject":"Re: [PATCH v2 0/8] fetch: add repair: full refetch without negotiation (was: \"refiltering\")","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-02-28T22:20:59Z","receivedAt":"2022-02-28T22:34:34Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Feb 28 2022, Robert Coup wrote:\n\n> Hi Ævar,\n>\n> Thanks for taking the time to look into this,\n>\n> On Mon, 28 Feb 2022 at 16:54, Ævar Arnfjörð Bjarmason wrote:\n>> I realize this was probably based on feedback on v1 (I didn't go back\n>> and re-read it, sorry).\n>\n> Yes, `fetch --repair` was from Jonathan Tan's v1 feedback[1], where he\n> pointed out it could fill in lost objects from any remote in a more\n> generally useful fashion.\n>\n> My goal here is to refetch with a different filter so that I get the\n> outcome of a `clone --filter=` without having to chuck my object\n> directory. But the actual implementation doesn't need to know anything\n> specific about filters, so the original \"refilter\" name I had isn't\n> really right.\n\n*nod*\n\n>> But I feel strongly that we really should name this something other than\n>> --repair. I don't care much if it isn't that :) But maybe\n>> --expand-filters, --fleshen-partial or something like that?\n>\n> fleshen-partial sounds like a horror movie scene to me.\n>\n> 1. `--refetch`\n> 2. `--as-clone`\n> 3. `--expand-filter` (though TBC you don't necessarily need a filter)\n> 4. `--refilter`\n> 5. something else\n\n*nod*\n\n>> So first (and partially as an aside): Is a \"noop\" negotiatior really\n>> want we want at all? Don't we instead want to be discovering those parts\n>> of our history that are closed under reachability (if any) and say we\n>> HAVE those things during negotiation?\n>\n> At an object level we don't have any means of knowing what has or\n> hasn't been obtained via fetch to a partial clone with different\n> `--filter` args (via config or cli), dynamic fault-ins, or sourced\n> from a different remote. Fetch negotiation only occurs for refs and\n> their associated commits/histories, but filtering occurs at the blob\n> or tree level — so we often HAVE a commit but not all of its\n> trees/blobs, whereupon negotiation skips that commit and all it's\n> associated objects.\n\nYes, I'm basically asking if the negotiation part wouldn't *ideally* be\ndoing basically the same \"everything is connected\" check\nreceive-pack/fsck do.\n\nI.e. you've got partial data with promisors locally, but if you walk\nyour branch histor(y|ies) you'll discover that N commits down we have\nall the prerequisite objects locally.\n\nAs an aside there's a 1=1 mapping between that and what \"git bundle\ncreate\" will do/verify to create a bundle without listed\nprerequisites.\n\nI.e. I think you'll find what it does with revision.c and PREREQ_* and\nother flags INTERESTING (a lame pun on its use of UNINTERESTING :).\n\nPresumably the code needed to drive such a negotiation would be useful\nfor other neat stuff, e.g. having a some-partial repo locally, wanting\nto fetch the PACK to complete it from the server, and knowing you have\nthat data to create a fully connected (or incremental) bundle for that\nrepository, but I digress.\n\n>> But secondly, on the \"--repair\" name: The reason I mentioned that is\n>> that I'd really like us to actually have a \"my repo is screwed, please\n>> repair it\".\n>\n> Feels like people would look at `fsck` for that over `fetch`? Maybe\n> not. Anyway, I get the point about the naming still not being right\n> :-)\n\nI think that definitely would be fetch/gc over \"fsck\". I.e. if you've\ngot corruption fsck can only tell you that it's screwed.\n\nIt's fetch/gc (or \"git bundle unbundle\") that stand any chance of\nactually doing the repair, since we'd need to stitch together the\n(partially) corrupted local/remote content with a hopefully good\ncompliment to it.\n\nFWIW I had an ad-hoc implementation of this basically working by\ndisabling the negotiation + not doing any object existence/collision\nchecks before writing content to the repository.\n\nThat and teaching \"repack\" to not die and instead to carry on in the\nface of object decoding failure (and hopefully discover a \"duplicate\"\nbut good copy later) + \"gc\" is enough to repair most corruption,\ne.g. truncated loose object etc.\n\n>> But (and I haven't tested, but I'm pretty sure), this patch series isn't\n>> going to give you that. The reasons are elaborated on in [1], basically\n>> we try really hard to re-use local data, and due to that & the collision\n>> detection will often just hard die early in object walking.\n>>\n>> But maybe I'm wrong, have you actually tested this with *broken* objects\n>> as opposed to just missing ones with repo filters + promisors in play?\n>> Our t/*fsck* and t/*corrupt*/ etc. tests have some of those.\n>\n> Correct: I haven't tested with such objects/broken ODBs. Ideally\n> repack/gc/etc would prefer a new-fixed pack over the old-broken\n> pack/object but that's not really what I'm aiming to achieve here or\n> am interested in.\n\nI think I've only tested loose (bad) + pack (good), I think pack (bad) +\npack (good) probably has some bigger caveats (like the first error\naborting the whole pack read, due to deltas etc.).\n\nBut yeah, I'm not saying this should be on your radar at all, other than\nthe bikeshedding comment of having a --repair that doesn't really do\n\"repair\" would be unfortunate naming, especially if we're locked into\nbehavior orthagonal to that needed for an \"actual\" repair.\n\n> 1. https://lore.kernel.org/git/20220202185957.1928631-1-jonathantanmy@google.com/\n\n"},{"id":"450402","messageId":"96a75be3d8acf983db256206706c2e9651d4a2c2.1646406275.git.gitgitgadget@gmail.com","threadId":"57348","inReplyTo":"pull.1138.v3.git.1646406274.gitgitgadget@gmail.com","subject":"[PATCH v3 1/7] fetch-negotiator: add specific noop initializer","fromName":"Robert Coup via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-03-04T15:04:28Z","receivedAt":"2022-03-04T15:04:42Z","isPatch":true,"sender":{"key":"robert@coup.net.nz","avatar":"https://avatars.githubusercontent.com/u/59874?v=4"},"body":"From: Robert Coup \n\nAdd a specific initializer for the noop fetch negotiator. This is\nintroduced to support allowing partial clones to skip commit negotiation\nwhen performing a \"refetch\".\n\nSigned-off-by: Robert Coup \n---\n fetch-negotiator.c | 5 +++++\n fetch-negotiator.h | 8 ++++++++\n 2 files changed, 13 insertions(+)\n\ndiff --git a/fetch-negotiator.c b/fetch-negotiator.c\nindex 874797d767b..be383367f55 100644\n--- a/fetch-negotiator.c\n+++ b/fetch-negotiator.c\n@@ -23,3 +23,8 @@ void fetch_negotiator_init(struct repository *r,\n \t\treturn;\n \t}\n }\n+\n+void fetch_negotiator_init_noop(struct fetch_negotiator *negotiator)\n+{\n+\tnoop_negotiator_init(negotiator);\n+}\ndiff --git a/fetch-negotiator.h b/fetch-negotiator.h\nindex ea78868504b..e348905a1f0 100644\n--- a/fetch-negotiator.h\n+++ b/fetch-negotiator.h\n@@ -53,7 +53,15 @@ struct fetch_negotiator {\n \tvoid *data;\n };\n \n+/*\n+ * Initialize a negotiator based on the repository settings.\n+ */\n void fetch_negotiator_init(struct repository *r,\n \t\t\t struct fetch_negotiator *negotiator);\n \n+/*\n+ * Initialize a noop negotiator.\n+ */\n+void fetch_negotiator_init_noop(struct fetch_negotiator *negotiator);\n+\n #endif\n-- \ngitgitgadget\n\n"},{"id":"450404","messageId":"pull.1138.v3.git.1646406274.gitgitgadget@gmail.com","threadId":"57348","inReplyTo":"pull.1138.v2.git.1645719218.gitgitgadget@gmail.com","subject":"[PATCH v3 0/7] fetch: add repair: full refetch without negotiation (was: \"refiltering\")","fromName":"Robert Coup via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-03-04T15:04:27Z","receivedAt":"2022-03-04T15:04:43Z","isPatch":true,"sender":{"key":"robert@coup.net.nz","avatar":"https://avatars.githubusercontent.com/u/59874?v=4"},"body":"If a filter is changed on a partial clone repository, for example from\nblob:none to blob:limit=1m, there is currently no straightforward way to\nbulk-refetch the objects that match the new filter for existing local\ncommits. This is because the client will report commits as \"have\" during\nfetch negotiation and any dependent objects won't be included in the\ntransferred pack. Another use case is discussed at [1].\n\nThis patch series introduces a --refetch option to fetch & fetch-pack to\nenable doing a full fetch without performing any commit negotiation with the\nremote, as a fresh clone does. It builds upon cbe566a071 (\"negotiator/noop:\nadd noop fetch negotiator\", 2020-08-18).\n\n * Using --refetch will produce duplicated objects between the existing and\n newly fetched packs, but maintenance will clean them up when it runs\n automatically post-fetch (if enabled).\n * If a user fetches with --refetch applying a more restrictive partial\n clone filter than previously (eg: blob:limit=1m then blob:limit=1k) the\n eventual state is a no-op, since any referenced object already in the\n local repository is never removed. More advanced repacking which could\n improve this scenario is currently proposed at [2].\n\n[1]\nhttps://lore.kernel.org/git/aa7b89ee-08aa-7943-6a00-28dcf344426e@syntevo.com/\n[2]\nhttps://lore.kernel.org/git/21ED346B-A906-4905-B061-EDE53691C586@gmail.com/\n\nChanges since v2:\n\n * Changed the name from \"repair\" to \"refetch\". While it's conceivable to\n use it in some object DB repair situations that's not the focus of these\n changes.\n * Pass config options to maintenance via GIT_CONFIG_PARAMETERS\n * Split out auto-maintenance to a separate & more robust test\n * Minor fixes/improvements from reviews by Junio & Ævar\n\nChanges since RFC (v1):\n\n * Changed the name from \"refilter\" to \"repair\"\n * Removed dependency between server-side support for filtering and repair\n * Added a test case for a shallow clone\n * Post-fetch auto maintenance now strongly encourages\n repacking/consolidation\n\nRobert Coup (7):\n fetch-negotiator: add specific noop initializer\n fetch-pack: add refetch\n builtin/fetch-pack: add --refetch option\n fetch: add --refetch option\n t5615-partial-clone: add test for fetch --refetch\n fetch: after refetch, encourage auto gc repacking\n doc/partial-clone: mention --refetch fetch option\n\n Documentation/fetch-options.txt | 10 +++\n Documentation/git-fetch-pack.txt | 4 ++\n Documentation/technical/partial-clone.txt | 3 +\n builtin/fetch-pack.c | 4 ++\n builtin/fetch.c | 34 +++++++++-\n fetch-negotiator.c | 5 ++\n fetch-negotiator.h | 8 +++\n fetch-pack.c | 46 ++++++++-----\n fetch-pack.h | 1 +\n remote-curl.c | 6 ++\n t/t5616-partial-clone.sh | 81 ++++++++++++++++++++++-\n transport-helper.c | 3 +\n transport.c | 4 ++\n transport.h | 4 ++\n 14 files changed, 193 insertions(+), 20 deletions(-)\n\n\nbase-commit: 715d08a9e51251ad8290b181b6ac3b9e1f9719d7\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1138%2Frcoup%2Frc-partial-clone-refilter-v3\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1138/rcoup/rc-partial-clone-refilter-v3\nPull-Request: https://github.com/gitgitgadget/git/pull/1138\n\nRange-diff vs v2:\n\n 1: d146d8aaaaf ! 1: 96a75be3d8a fetch-negotiator: add specific noop initializor\n @@ Metadata\n Author: Robert Coup \n \n ## Commit message ##\n - fetch-negotiator: add specific noop initializor\n + fetch-negotiator: add specific noop initializer\n \n - Add a specific initializor for the noop fetch negotiator. This is\n + Add a specific initializer for the noop fetch negotiator. This is\n introduced to support allowing partial clones to skip commit negotiation\n - when fetching to repair or apply a modified filter.\n + when performing a \"refetch\".\n \n Signed-off-by: Robert Coup \n \n 2: 2d817a65db5 ! 2: 04ca6a07f85 fetch-pack: add repairing\n @@ Metadata\n Author: Robert Coup \n \n ## Commit message ##\n - fetch-pack: add repairing\n + fetch-pack: add refetch\n \n - Allow a 'repair fetch' where the contents of the local object store are\n + Allow a \"refetch\" where the contents of the local object store are\n ignored and a full fetch is performed, not attempting to find or\n negotiate common commits with the remote.\n \n @@ fetch-pack.c: static int find_common(struct fetch_negotiator *negotiator,\n -\t\tif (((o = lookup_object(the_repository, remote)) != NULL) &&\n -\t\t\t\t(o->flags & COMPLETE)) {\n -\t\t\tcontinue;\n -+\t\tif (!args->repair) {\n ++\t\tif (!args->refetch) {\n +\t\t\t/*\n +\t\t\t* If that object is complete (i.e. it is an ancestor of a\n +\t\t\t* local ref), we tell them we have it but do not have to\n @@ fetch-pack.c: static int find_common(struct fetch_negotiator *negotiator,\n \n \t\tremote_hex = oid_to_hex(remote);\n @@ fetch-pack.c: static void mark_complete_and_common_ref(struct fetch_negotiator *negotiator,\n + \tint old_save_commit_buffer = save_commit_buffer;\n + \ttimestamp_t cutoff = 0;\n \n - \tsave_commit_buffer = 0;\n - \n -+\tif (args->repair)\n ++\tif (args->refetch)\n +\t\treturn;\n +\n + \tsave_commit_buffer = 0;\n + \n \ttrace2_region_enter(\"fetch-pack\", \"parse_remote_refs_and_find_cutoff\", NULL);\n - \tfor (ref = *refs; ref; ref = ref->next) {\n - \t\tstruct commit *commit;\n @@ fetch-pack.c: static struct ref *do_fetch_pack(struct fetch_pack_args *args,\n - \tstruct fetch_negotiator negotiator_alloc;\n \tstruct fetch_negotiator *negotiator;\n \n --\tnegotiator = &negotiator_alloc;\n + \tnegotiator = &negotiator_alloc;\n -\tfetch_negotiator_init(r, negotiator);\n --\n - \tsort_ref_list(&ref, ref_compare_name);\n - \tQSORT(sought, nr_sought, cmp_ref_by_name);\n - \n -@@ fetch-pack.c: static struct ref *do_fetch_pack(struct fetch_pack_args *args,\n - \tif (!server_supports_hash(the_hash_algo->name, NULL))\n - \t\tdie(_(\"Server does not support this repository's object format\"));\n - \n -+\tnegotiator = &negotiator_alloc;\n -+\tif (args->repair) {\n ++\tif (args->refetch) {\n +\t\tfetch_negotiator_init_noop(negotiator);\n +\t} else {\n +\t\tfetch_negotiator_init(r, negotiator);\n +\t}\n -+\n + \n + \tsort_ref_list(&ref, ref_compare_name);\n + \tQSORT(sought, nr_sought, cmp_ref_by_name);\n +@@ fetch-pack.c: static struct ref *do_fetch_pack(struct fetch_pack_args *args,\n + \n \tmark_complete_and_common_ref(negotiator, args, &ref);\n \tfilter_refs(args, &ref, sought, nr_sought);\n -\tif (everything_local(args, &ref)) {\n -+\tif (!args->repair && everything_local(args, &ref)) {\n ++\tif (!args->refetch && everything_local(args, &ref)) {\n \t\tpacket_flush(fd[1]);\n \t\tgoto all_done;\n \t}\n @@ fetch-pack.c: static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,\n \n \tnegotiator = &negotiator_alloc;\n -\tfetch_negotiator_init(r, negotiator);\n -+\tif (args->repair)\n ++\tif (args->refetch)\n +\t\tfetch_negotiator_init_noop(negotiator);\n +\telse\n +\t\tfetch_negotiator_init(r, negotiator);\n @@ fetch-pack.c: static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,\n \t\t\tmark_complete_and_common_ref(negotiator, args, &ref);\n \t\t\tfilter_refs(args, &ref, sought, nr_sought);\n -\t\t\tif (everything_local(args, &ref))\n -+\t\t\tif (!args->repair && everything_local(args, &ref))\n ++\t\t\tif (!args->refetch && everything_local(args, &ref))\n \t\t\t\tstate = FETCH_DONE;\n \t\t\telse\n \t\t\t\tstate = FETCH_SEND_REQUEST;\n @@ fetch-pack.h: struct fetch_pack_args {\n \tunsigned update_shallow:1;\n \tunsigned reject_shallow_remote:1;\n \tunsigned deepen:1;\n -+\tunsigned repair:1;\n ++\tunsigned refetch:1;\n \n \t/*\n \t * Indicate that the remote of this request is a promisor remote. The\n 3: a42d40ac294 ! 3: 879d30c4473 builtin/fetch-pack: add --repair option\n @@ Metadata\n Author: Robert Coup \n \n ## Commit message ##\n - builtin/fetch-pack: add --repair option\n + builtin/fetch-pack: add --refetch option\n \n - Add a --repair option to fetch-pack to force a full fetch. Use when\n + Add a refetch option to fetch-pack to force a full fetch. Use when\n applying a new partial clone filter to refetch all matching objects.\n \n Signed-off-by: Robert Coup \n @@ Documentation/git-fetch-pack.txt: be in a separate packet, and the list must end\n \tcurrent shallow boundary instead of from the tip of each\n \tremote branch history.\n \n -+--repair::\n ++--refetch::\n +\tSkips negotiating commits with the server in order to fetch all matching\n +\tobjects. Use to reapply a new partial clone blob/tree filter.\n +\n @@ builtin/fetch-pack.c: int cmd_fetch_pack(int argc, const char **argv, const char\n \t\t\targs.from_promisor = 1;\n \t\t\tcontinue;\n \t\t}\n -+\t\tif (!strcmp(\"--repair\", arg)) {\n -+\t\t\targs.repair = 1;\n ++\t\tif (!strcmp(\"--refetch\", arg)) {\n ++\t\t\targs.refetch = 1;\n +\t\t\tcontinue;\n +\t\t}\n \t\tif (skip_prefix(arg, (\"--\" CL_ARG__FILTER \"=\"), &arg)) {\n @@ remote-curl.c: struct options {\n \t\t/* see documentation of corresponding flag in fetch-pack.h */\n \t\tfrom_promisor : 1,\n \n -+\t\trepair : 1,\n ++\t\trefetch : 1,\n \t\tatomic : 1,\n \t\tobject_format : 1,\n \t\tforce_if_includes : 1;\n @@ remote-curl.c: static int set_option(const char *name, const char *value)\n \t} else if (!strcmp(name, \"from-promisor\")) {\n \t\toptions.from_promisor = 1;\n \t\treturn 0;\n -+\t} else if (!strcmp(name, \"repair\")) {\n -+\t\toptions.repair = 1;\n ++\t} else if (!strcmp(name, \"refetch\")) {\n ++\t\toptions.refetch = 1;\n +\t\treturn 0;\n \t} else if (!strcmp(name, \"filter\")) {\n \t\toptions.filter = xstrdup(value);\n @@ remote-curl.c: static int fetch_git(struct discovery *heads,\n \t\tstrvec_push(&args, \"--deepen-relative\");\n \tif (options.from_promisor)\n \t\tstrvec_push(&args, \"--from-promisor\");\n -+\tif (options.repair)\n -+\t\tstrvec_push(&args, \"--repair\");\n ++\tif (options.refetch)\n ++\t\tstrvec_push(&args, \"--refetch\");\n \tif (options.filter)\n \t\tstrvec_pushf(&args, \"--filter=%s\", options.filter);\n \tstrvec_push(&args, url.buf);\n 4: 79c409d0542 ! 4: a503b98f333 fetch: add --repair option\n @@ Metadata\n Author: Robert Coup \n \n ## Commit message ##\n - fetch: add --repair option\n + fetch: add --refetch option\n \n - Teach fetch and transports the --repair option to force a full fetch\n + Teach fetch and transports the --refetch option to force a full fetch\n without negotiating common commits with the remote. Use when applying a\n new partial clone filter to refetch all matching objects.\n \n @@ Documentation/fetch-options.txt: endif::git-pull[]\n \tsetting. See linkgit:git-config[1].\n \n +ifndef::git-pull[]\n -+--repair::\n ++--refetch::\n +\tInstead of negotiating with the server to avoid transferring commits and\n +\tassociated objects that are already present locally, this option fetches\n +\tall objects as a fresh clone would. Use this to reapply a partial clone\n @@ builtin/fetch.c: static int prune_tags = -1; /* unspecified */\n static int all, append, dry_run, force, keep, multiple, update_head_ok;\n static int write_fetch_head = 1;\n -static int verbosity, deepen_relative, set_upstream;\n -+static int verbosity, deepen_relative, set_upstream, repair;\n ++static int verbosity, deepen_relative, set_upstream, refetch;\n static int progress = -1;\n static int enable_auto_gc = 1;\n static int tags = TAGS_DEFAULT, unshallow, update_shallow, deepen;\n @@ builtin/fetch.c: static struct option builtin_fetch_options[] = {\n \tOPT_SET_INT_F(0, \"unshallow\", &unshallow,\n \t\t N_(\"convert to a complete repository\"),\n \t\t 1, PARSE_OPT_NONEG),\n -+\tOPT_SET_INT_F(0, \"repair\", &repair,\n ++\tOPT_SET_INT_F(0, \"refetch\", &refetch,\n +\t\t N_(\"re-fetch without negotiating common commits\"),\n +\t\t 1, PARSE_OPT_NONEG),\n \t{ OPTION_STRING, 0, \"submodule-prefix\", &submodule_prefix, N_(\"dir\"),\n @@ builtin/fetch.c: static int check_exist_and_connected(struct ref *ref_map)\n \t\treturn -1;\n \n +\t/*\n -+\t * Similarly, if we need to repair, we always want to perform a full\n ++\t * Similarly, if we need to refetch, we always want to perform a full\n +\t * fetch ignoring existing objects.\n +\t */\n -+\tif (repair)\n ++\tif (refetch)\n +\t\treturn -1;\n +\n +\n @@ builtin/fetch.c: static struct transport *prepare_transport(struct remote *remot\n \t\tset_option(transport, TRANS_OPT_DEEPEN_RELATIVE, \"yes\");\n \tif (update_shallow)\n \t\tset_option(transport, TRANS_OPT_UPDATE_SHALLOW, \"yes\");\n -+\tif (repair)\n -+\t\tset_option(transport, TRANS_OPT_REPAIR, \"yes\");\n ++\tif (refetch)\n ++\t\tset_option(transport, TRANS_OPT_REFETCH, \"yes\");\n \tif (filter_options.choice) {\n \t\tconst char *spec =\n \t\t\texpand_list_objects_filter_spec(&filter_options);\n @@ transport-helper.c: static int fetch_refs(struct transport *transport,\n \tif (data->transport_options.update_shallow)\n \t\tset_helper_option(transport, \"update-shallow\", \"true\");\n \n -+\tif (data->transport_options.repair)\n -+\t\tset_helper_option(transport, \"repair\", \"true\");\n ++\tif (data->transport_options.refetch)\n ++\t\tset_helper_option(transport, \"refetch\", \"true\");\n +\n \tif (data->transport_options.filter_options.choice) {\n \t\tconst char *spec = expand_list_objects_filter_spec(\n @@ transport.c: static int set_git_option(struct git_transport_options *opts,\n \t\tlist_objects_filter_die_if_populated(&opts->filter_options);\n \t\tparse_list_objects_filter(&opts->filter_options, value);\n \t\treturn 0;\n -+\t} else if (!strcmp(name, TRANS_OPT_REPAIR)) {\n -+\t\topts->repair = !!value;\n ++\t} else if (!strcmp(name, TRANS_OPT_REFETCH)) {\n ++\t\topts->refetch = !!value;\n +\t\treturn 0;\n \t} else if (!strcmp(name, TRANS_OPT_REJECT_SHALLOW)) {\n \t\topts->reject_shallow = !!value;\n @@ transport.c: static int fetch_refs_via_pack(struct transport *transport,\n \targs.update_shallow = data->options.update_shallow;\n \targs.from_promisor = data->options.from_promisor;\n \targs.filter_options = data->options.filter_options;\n -+\targs.repair = data->options.repair;\n ++\targs.refetch = data->options.refetch;\n \targs.stateless_rpc = transport->stateless_rpc;\n \targs.server_options = transport->server_options;\n \targs.negotiation_tips = data->options.negotiation_tips;\n @@ transport.h: struct git_transport_options {\n \tunsigned update_shallow : 1;\n \tunsigned reject_shallow : 1;\n \tunsigned deepen_relative : 1;\n -+\tunsigned repair : 1;\n ++\tunsigned refetch : 1;\n \n \t/* see documentation of corresponding flag in fetch-pack.h */\n \tunsigned from_promisor : 1;\n @@ transport.h: void transport_check_allowed(const char *type);\n #define TRANS_OPT_LIST_OBJECTS_FILTER \"filter\"\n \n +/* Refetch all objects without negotiating */\n -+#define TRANS_OPT_REPAIR \"repair\"\n ++#define TRANS_OPT_REFETCH \"refetch\"\n +\n /* Request atomic (all-or-nothing) updates when pushing */\n #define TRANS_OPT_ATOMIC \"atomic\"\n 5: 38af2bbee79 ! 5: 01f22e784a5 t5615-partial-clone: add test for fetch --repair\n @@ Metadata\n Author: Robert Coup \n \n ## Commit message ##\n - t5615-partial-clone: add test for fetch --repair\n + t5615-partial-clone: add test for fetch --refetch\n \n - Add a test for doing a repair fetch to apply a changed partial clone\n - filter under protocol v0 and v2.\n + Add a test for doing a refetch to apply a changed partial clone filter\n + under protocol v0 and v2.\n \n Signed-off-by: Robert Coup \n \n @@ t/t5616-partial-clone.sh: test_expect_success 'manual prefetch of missing object\n +\tgit -C src push -u srv main\n +'\n +\n -+# Do partial fetch to fetch smaller files; then verify that without --repair\n ++# Do partial fetch to fetch smaller files; then verify that without --refetch\n +# applying a new filter does not refetch missing large objects. Then use\n -+# --repair to apply the new filter on existing commits. Test it under both\n ++# --refetch to apply the new filter on existing commits. Test it under both\n +# protocol v2 & v0.\n -+test_expect_success 'apply a different filter using --repair' '\n ++test_expect_success 'apply a different filter using --refetch' '\n +\tgit -C pc1 fetch --filter=blob:limit=999 origin &&\n +\tgit -C pc1 rev-list --quiet --objects --missing=print \\\n +\t\tmain..origin/main >observed &&\n +\ttest_line_count = 4 observed &&\n +\n -+\tgit -C pc1 fetch --filter=blob:limit=19999 --repair origin &&\n ++\tgit -C pc1 fetch --filter=blob:limit=19999 --refetch origin &&\n +\tgit -C pc1 rev-list --quiet --objects --missing=print \\\n +\t\tmain..origin/main >observed &&\n +\ttest_line_count = 2 observed &&\n +\n +\tgit -c protocol.version=0 -C pc1 fetch --filter=blob:limit=29999 \\\n -+\t\t--repair origin &&\n ++\t\t--refetch origin &&\n +\tgit -C pc1 rev-list --quiet --objects --missing=print \\\n +\t\tmain..origin/main >observed &&\n +\ttest_line_count = 0 observed\n +'\n +\n -+test_expect_success 'fetch --repair works with a shallow clone' '\n ++test_expect_success 'fetch --refetch works with a shallow clone' '\n +\tgit clone --no-checkout --depth=1 --filter=blob:none \"file://$(pwd)/srv.bare\" pc1s &&\n +\tgit -C pc1s rev-list --objects --missing=print HEAD >observed &&\n +\ttest_line_count = 6 observed &&\n +\n -+\tGIT_TRACE=1 git -C pc1s fetch --filter=blob:limit=999 --repair origin &&\n ++\tGIT_TRACE=1 git -C pc1s fetch --filter=blob:limit=999 --refetch origin &&\n +\tgit -C pc1s rev-list --objects --missing=print HEAD >observed &&\n +\ttest_line_count = 6 observed\n +'\n 6: cfa6dca8ef4 < -: ----------- maintenance: add ability to pass config options\n 7: 2338c15249a ! 6: 31046625987 fetch: after repair, encourage auto gc repacking\n @@ Metadata\n Author: Robert Coup \n \n ## Commit message ##\n - fetch: after repair, encourage auto gc repacking\n + fetch: after refetch, encourage auto gc repacking\n \n - After invoking `fetch --repair`, the object db will likely contain many\n + After invoking `fetch --refetch`, the object db will likely contain many\n duplicate objects. If auto-maintenance is enabled, invoke it with\n appropriate settings to encourage repacking/consolidation.\n \n @@ Documentation/fetch-options.txt: ifndef::git-pull[]\n --refmap=::\n \n ## builtin/fetch.c ##\n -@@ builtin/fetch.c: int cmd_fetch(int argc, const char **argv, const char *prefix)\n - \tstruct remote *remote = NULL;\n - \tint result = 0;\n - \tint prune_tags_ok = 1;\n -+\tstruct strvec auto_maint_opts = STRVEC_INIT;\n -+\tint opt_val;\n - \n - \tpacket_trace_identity(\"fetch\");\n - \n @@ builtin/fetch.c: int cmd_fetch(int argc, const char **argv, const char *prefix)\n \t\t\t\t\t NULL);\n \t}\n \n -\tif (enable_auto_gc)\n --\t\trun_auto_maintenance(verbosity < 0, NULL);\n +\tif (enable_auto_gc) {\n -+\t\tif (repair) {\n ++\t\tif (refetch) {\n +\t\t\t/*\n +\t\t\t * Hint auto-maintenance strongly to encourage repacking,\n +\t\t\t * but respect config settings disabling it.\n +\t\t\t */\n ++\t\t\tint opt_val;\n +\t\t\tif (git_config_get_int(\"gc.autopacklimit\", &opt_val))\n +\t\t\t\topt_val = -1;\n +\t\t\tif (opt_val != 0)\n -+\t\t\t\tstrvec_push(&auto_maint_opts, \"gc.autoPackLimit=1\");\n ++\t\t\t\tgit_config_push_parameter(\"gc.autoPackLimit=1\");\n +\n +\t\t\tif (git_config_get_int(\"maintenance.incremental-repack.auto\", &opt_val))\n +\t\t\t\topt_val = -1;\n +\t\t\tif (opt_val != 0)\n -+\t\t\t\tstrvec_push(&auto_maint_opts, \"maintenance.incremental-repack.auto=-1\");\n ++\t\t\t\tgit_config_push_parameter(\"maintenance.incremental-repack.auto=-1\");\n +\t\t}\n -+\t\trun_auto_maintenance(verbosity < 0, &auto_maint_opts);\n + \t\trun_auto_maintenance(verbosity < 0);\n +\t}\n \n cleanup:\n \tstring_list_clear(&list, 0);\n -+\tstrvec_clear(&auto_maint_opts);\n - \treturn result;\n - }\n \n ## t/t5616-partial-clone.sh ##\n -@@ t/t5616-partial-clone.sh: test_expect_success 'push new commits to server for file.4.txt' '\n - # Do partial fetch to fetch smaller files; then verify that without --repair\n - # applying a new filter does not refetch missing large objects. Then use\n - # --repair to apply the new filter on existing commits. Test it under both\n --# protocol v2 & v0.\n -+# protocol v2 & v0. Check repacking auto-maintenance is kicked off.\n - test_expect_success 'apply a different filter using --repair' '\n - \tgit -C pc1 fetch --filter=blob:limit=999 origin &&\n - \tgit -C pc1 rev-list --quiet --objects --missing=print \\\n -@@ t/t5616-partial-clone.sh: test_expect_success 'apply a different filter using --repair' '\n - \t\tmain..origin/main >observed &&\n - \ttest_line_count = 2 observed &&\n - \n -+\tGIT_TRACE2_EVENT=\"$(pwd)/trace.log\" \\\n - \tgit -c protocol.version=0 -C pc1 fetch --filter=blob:limit=29999 \\\n - \t\t--repair origin &&\n - \tgit -C pc1 rev-list --quiet --objects --missing=print \\\n - \t\tmain..origin/main >observed &&\n --\ttest_line_count = 0 observed\n -+\ttest_line_count = 0 observed &&\n -+\ttest_subcommand git -c gc.autoPackLimit=1 -c maintenance.incremental-repack.auto=-1 maintenance run --auto --no-quiet \n \n ## Commit message ##\n - doc/partial-clone: mention --repair fetch option\n + doc/partial-clone: mention --refetch fetch option\n \n - Document it for partial clones as a means to refetch with a new filter.\n + Document it for partial clones as a means to apply a new filter.\n \n Signed-off-by: Robert Coup \n \n @@ Documentation/technical/partial-clone.txt: Fetching Missing Objects\n currently fetches all objects referred to by the requested objects, even\n though they are not necessary.\n \n -+- Fetching with `--repair` will request a complete new filtered packfile from\n ++- Fetching with `--refetch` will request a complete new filtered packfile from\n + the remote, which can be used to change a filter without needing to\n + dynamically fetch missing objects.\n \n\n-- \ngitgitgadget\n"},{"id":"450403","messageId":"879d30c4473ee31ad26b324cabf02eb37b3bd62a.1646406275.git.gitgitgadget@gmail.com","threadId":"57348","inReplyTo":"pull.1138.v3.git.1646406274.gitgitgadget@gmail.com","subject":"[PATCH v3 3/7] builtin/fetch-pack: add --refetch option","fromName":"Robert Coup via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-03-04T15:04:30Z","receivedAt":"2022-03-04T15:04:44Z","isPatch":true,"sender":{"key":"robert@coup.net.nz","avatar":"https://avatars.githubusercontent.com/u/59874?v=4"},"body":"From: Robert Coup \n\nAdd a refetch option to fetch-pack to force a full fetch. Use when\napplying a new partial clone filter to refetch all matching objects.\n\nSigned-off-by: Robert Coup \n---\n Documentation/git-fetch-pack.txt | 4 ++++\n builtin/fetch-pack.c | 4 ++++\n remote-curl.c | 6 ++++++\n 3 files changed, 14 insertions(+)\n\ndiff --git a/Documentation/git-fetch-pack.txt b/Documentation/git-fetch-pack.txt\nindex c9758847937..46747d5f429 100644\n--- a/Documentation/git-fetch-pack.txt\n+++ b/Documentation/git-fetch-pack.txt\n@@ -101,6 +101,10 @@ be in a separate packet, and the list must end with a flush packet.\n \tcurrent shallow boundary instead of from the tip of each\n \tremote branch history.\n \n+--refetch::\n+\tSkips negotiating commits with the server in order to fetch all matching\n+\tobjects. Use to reapply a new partial clone blob/tree filter.\n+\n --no-progress::\n \tDo not show the progress.\n \ndiff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c\nindex c2d96f4c89a..1f8aec97d47 100644\n--- a/builtin/fetch-pack.c\n+++ b/builtin/fetch-pack.c\n@@ -153,6 +153,10 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)\n \t\t\targs.from_promisor = 1;\n \t\t\tcontinue;\n \t\t}\n+\t\tif (!strcmp(\"--refetch\", arg)) {\n+\t\t\targs.refetch = 1;\n+\t\t\tcontinue;\n+\t\t}\n \t\tif (skip_prefix(arg, (\"--\" CL_ARG__FILTER \"=\"), &arg)) {\n \t\t\tparse_list_objects_filter(&args.filter_options, arg);\n \t\t\tcontinue;\ndiff --git a/remote-curl.c b/remote-curl.c\nindex 0dabef2dd7c..fc75600d4c6 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -43,6 +43,7 @@ struct options {\n \t\t/* see documentation of corresponding flag in fetch-pack.h */\n \t\tfrom_promisor : 1,\n \n+\t\trefetch : 1,\n \t\tatomic : 1,\n \t\tobject_format : 1,\n \t\tforce_if_includes : 1;\n@@ -198,6 +199,9 @@ static int set_option(const char *name, const char *value)\n \t} else if (!strcmp(name, \"from-promisor\")) {\n \t\toptions.from_promisor = 1;\n \t\treturn 0;\n+\t} else if (!strcmp(name, \"refetch\")) {\n+\t\toptions.refetch = 1;\n+\t\treturn 0;\n \t} else if (!strcmp(name, \"filter\")) {\n \t\toptions.filter = xstrdup(value);\n \t\treturn 0;\n@@ -1182,6 +1186,8 @@ static int fetch_git(struct discovery *heads,\n \t\tstrvec_push(&args, \"--deepen-relative\");\n \tif (options.from_promisor)\n \t\tstrvec_push(&args, \"--from-promisor\");\n+\tif (options.refetch)\n+\t\tstrvec_push(&args, \"--refetch\");\n \tif (options.filter)\n \t\tstrvec_pushf(&args, \"--filter=%s\", options.filter);\n \tstrvec_push(&args, url.buf);\n-- \ngitgitgadget\n\n"},{"id":"450405","messageId":"04ca6a07f859165cb06197dac862db251b342406.1646406275.git.gitgitgadget@gmail.com","threadId":"57348","inReplyTo":"pull.1138.v3.git.1646406274.gitgitgadget@gmail.com","subject":"[PATCH v3 2/7] fetch-pack: add refetch","fromName":"Robert Coup via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-03-04T15:04:29Z","receivedAt":"2022-03-04T15:04:47Z","isPatch":true,"sender":{"key":"robert@coup.net.nz","avatar":"https://avatars.githubusercontent.com/u/59874?v=4"},"body":"From: Robert Coup \n\nAllow a \"refetch\" where the contents of the local object store are\nignored and a full fetch is performed, not attempting to find or\nnegotiate common commits with the remote.\n\nA key use case is to apply a new partial clone blob/tree filter and\nrefetch all the associated matching content, which would otherwise not\nbe transferred when the commit objects are already present locally.\n\nSigned-off-by: Robert Coup \n---\n fetch-pack.c | 46 +++++++++++++++++++++++++++++-----------------\n fetch-pack.h | 1 +\n 2 files changed, 30 insertions(+), 17 deletions(-)\n\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex 87657907e78..4e1e88eea09 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -312,19 +312,21 @@ static int find_common(struct fetch_negotiator *negotiator,\n \t\tconst char *remote_hex;\n \t\tstruct object *o;\n \n-\t\t/*\n-\t\t * If that object is complete (i.e. it is an ancestor of a\n-\t\t * local ref), we tell them we have it but do not have to\n-\t\t * tell them about its ancestors, which they already know\n-\t\t * about.\n-\t\t *\n-\t\t * We use lookup_object here because we are only\n-\t\t * interested in the case we *know* the object is\n-\t\t * reachable and we have already scanned it.\n-\t\t */\n-\t\tif (((o = lookup_object(the_repository, remote)) != NULL) &&\n-\t\t\t\t(o->flags & COMPLETE)) {\n-\t\t\tcontinue;\n+\t\tif (!args->refetch) {\n+\t\t\t/*\n+\t\t\t* If that object is complete (i.e. it is an ancestor of a\n+\t\t\t* local ref), we tell them we have it but do not have to\n+\t\t\t* tell them about its ancestors, which they already know\n+\t\t\t* about.\n+\t\t\t*\n+\t\t\t* We use lookup_object here because we are only\n+\t\t\t* interested in the case we *know* the object is\n+\t\t\t* reachable and we have already scanned it.\n+\t\t\t*/\n+\t\t\tif (((o = lookup_object(the_repository, remote)) != NULL) &&\n+\t\t\t\t\t(o->flags & COMPLETE)) {\n+\t\t\t\tcontinue;\n+\t\t\t}\n \t\t}\n \n \t\tremote_hex = oid_to_hex(remote);\n@@ -692,6 +694,9 @@ static void mark_complete_and_common_ref(struct fetch_negotiator *negotiator,\n \tint old_save_commit_buffer = save_commit_buffer;\n \ttimestamp_t cutoff = 0;\n \n+\tif (args->refetch)\n+\t\treturn;\n+\n \tsave_commit_buffer = 0;\n \n \ttrace2_region_enter(\"fetch-pack\", \"parse_remote_refs_and_find_cutoff\", NULL);\n@@ -1028,7 +1033,11 @@ static struct ref *do_fetch_pack(struct fetch_pack_args *args,\n \tstruct fetch_negotiator *negotiator;\n \n \tnegotiator = &negotiator_alloc;\n-\tfetch_negotiator_init(r, negotiator);\n+\tif (args->refetch) {\n+\t\tfetch_negotiator_init_noop(negotiator);\n+\t} else {\n+\t\tfetch_negotiator_init(r, negotiator);\n+\t}\n \n \tsort_ref_list(&ref, ref_compare_name);\n \tQSORT(sought, nr_sought, cmp_ref_by_name);\n@@ -1121,7 +1130,7 @@ static struct ref *do_fetch_pack(struct fetch_pack_args *args,\n \n \tmark_complete_and_common_ref(negotiator, args, &ref);\n \tfilter_refs(args, &ref, sought, nr_sought);\n-\tif (everything_local(args, &ref)) {\n+\tif (!args->refetch && everything_local(args, &ref)) {\n \t\tpacket_flush(fd[1]);\n \t\tgoto all_done;\n \t}\n@@ -1587,7 +1596,10 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,\n \tstruct strvec index_pack_args = STRVEC_INIT;\n \n \tnegotiator = &negotiator_alloc;\n-\tfetch_negotiator_init(r, negotiator);\n+\tif (args->refetch)\n+\t\tfetch_negotiator_init_noop(negotiator);\n+\telse\n+\t\tfetch_negotiator_init(r, negotiator);\n \n \tpacket_reader_init(&reader, fd[0], NULL, 0,\n \t\t\t PACKET_READ_CHOMP_NEWLINE |\n@@ -1613,7 +1625,7 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,\n \t\t\t/* Filter 'ref' by 'sought' and those that aren't local */\n \t\t\tmark_complete_and_common_ref(negotiator, args, &ref);\n \t\t\tfilter_refs(args, &ref, sought, nr_sought);\n-\t\t\tif (everything_local(args, &ref))\n+\t\t\tif (!args->refetch && everything_local(args, &ref))\n \t\t\t\tstate = FETCH_DONE;\n \t\t\telse\n \t\t\t\tstate = FETCH_SEND_REQUEST;\ndiff --git a/fetch-pack.h b/fetch-pack.h\nindex 7f94a2a5831..8c7752fc821 100644\n--- a/fetch-pack.h\n+++ b/fetch-pack.h\n@@ -42,6 +42,7 @@ struct fetch_pack_args {\n \tunsigned update_shallow:1;\n \tunsigned reject_shallow_remote:1;\n \tunsigned deepen:1;\n+\tunsigned refetch:1;\n \n \t/*\n \t * Indicate that the remote of this request is a promisor remote. The\n-- \ngitgitgadget\n\n"},{"id":"450406","messageId":"a503b98f33328133f9a89ad7eba986b73535c461.1646406275.git.gitgitgadget@gmail.com","threadId":"57348","inReplyTo":"pull.1138.v3.git.1646406274.gitgitgadget@gmail.com","subject":"[PATCH v3 4/7] fetch: add --refetch option","fromName":"Robert Coup via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-03-04T15:04:31Z","receivedAt":"2022-03-04T15:04:51Z","isPatch":true,"sender":{"key":"robert@coup.net.nz","avatar":"https://avatars.githubusercontent.com/u/59874?v=4"},"body":"From: Robert Coup \n\nTeach fetch and transports the --refetch option to force a full fetch\nwithout negotiating common commits with the remote. Use when applying a\nnew partial clone filter to refetch all matching objects.\n\nSigned-off-by: Robert Coup \n---\n Documentation/fetch-options.txt | 9 +++++++++\n builtin/fetch.c | 15 ++++++++++++++-\n transport-helper.c | 3 +++\n transport.c | 4 ++++\n transport.h | 4 ++++\n 5 files changed, 34 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/fetch-options.txt b/Documentation/fetch-options.txt\nindex f9036831898..21a247abfa4 100644\n--- a/Documentation/fetch-options.txt\n+++ b/Documentation/fetch-options.txt\n@@ -163,6 +163,15 @@ endif::git-pull[]\n \tbehavior for a remote may be specified with the remote..tagOpt\n \tsetting. See linkgit:git-config[1].\n \n+ifndef::git-pull[]\n+--refetch::\n+\tInstead of negotiating with the server to avoid transferring commits and\n+\tassociated objects that are already present locally, this option fetches\n+\tall objects as a fresh clone would. Use this to reapply a partial clone\n+\tfilter from configuration or using `--filter=` when the filter\n+\tdefinition has changed.\n+endif::git-pull[]\n+\n --refmap=::\n \tWhen fetching refs listed on the command line, use the\n \tspecified refspec (can be given more than once) to map the\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 95832ba1dfd..f7bcf6fa64d 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -59,7 +59,7 @@ static int prune_tags = -1; /* unspecified */\n \n static int all, append, dry_run, force, keep, multiple, update_head_ok;\n static int write_fetch_head = 1;\n-static int verbosity, deepen_relative, set_upstream;\n+static int verbosity, deepen_relative, set_upstream, refetch;\n static int progress = -1;\n static int enable_auto_gc = 1;\n static int tags = TAGS_DEFAULT, unshallow, update_shallow, deepen;\n@@ -190,6 +190,9 @@ static struct option builtin_fetch_options[] = {\n \tOPT_SET_INT_F(0, \"unshallow\", &unshallow,\n \t\t N_(\"convert to a complete repository\"),\n \t\t 1, PARSE_OPT_NONEG),\n+\tOPT_SET_INT_F(0, \"refetch\", &refetch,\n+\t\t N_(\"re-fetch without negotiating common commits\"),\n+\t\t 1, PARSE_OPT_NONEG),\n \t{ OPTION_STRING, 0, \"submodule-prefix\", &submodule_prefix, N_(\"dir\"),\n \t\t N_(\"prepend this to submodule path output\"), PARSE_OPT_HIDDEN },\n \tOPT_CALLBACK_F(0, \"recurse-submodules-default\",\n@@ -1296,6 +1299,14 @@ static int check_exist_and_connected(struct ref *ref_map)\n \tif (deepen)\n \t\treturn -1;\n \n+\t/*\n+\t * Similarly, if we need to refetch, we always want to perform a full\n+\t * fetch ignoring existing objects.\n+\t */\n+\tif (refetch)\n+\t\treturn -1;\n+\n+\n \t/*\n \t * check_connected() allows objects to merely be promised, but\n \t * we need all direct targets to exist.\n@@ -1492,6 +1503,8 @@ static struct transport *prepare_transport(struct remote *remote, int deepen)\n \t\tset_option(transport, TRANS_OPT_DEEPEN_RELATIVE, \"yes\");\n \tif (update_shallow)\n \t\tset_option(transport, TRANS_OPT_UPDATE_SHALLOW, \"yes\");\n+\tif (refetch)\n+\t\tset_option(transport, TRANS_OPT_REFETCH, \"yes\");\n \tif (filter_options.choice) {\n \t\tconst char *spec =\n \t\t\texpand_list_objects_filter_spec(&filter_options);\ndiff --git a/transport-helper.c b/transport-helper.c\nindex a0297b0986c..b4dbbabb0c2 100644\n--- a/transport-helper.c\n+++ b/transport-helper.c\n@@ -715,6 +715,9 @@ static int fetch_refs(struct transport *transport,\n \tif (data->transport_options.update_shallow)\n \t\tset_helper_option(transport, \"update-shallow\", \"true\");\n \n+\tif (data->transport_options.refetch)\n+\t\tset_helper_option(transport, \"refetch\", \"true\");\n+\n \tif (data->transport_options.filter_options.choice) {\n \t\tconst char *spec = expand_list_objects_filter_spec(\n \t\t\t&data->transport_options.filter_options);\ndiff --git a/transport.c b/transport.c\nindex 253d6671b1f..e2817b7a715 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -243,6 +243,9 @@ static int set_git_option(struct git_transport_options *opts,\n \t\tlist_objects_filter_die_if_populated(&opts->filter_options);\n \t\tparse_list_objects_filter(&opts->filter_options, value);\n \t\treturn 0;\n+\t} else if (!strcmp(name, TRANS_OPT_REFETCH)) {\n+\t\topts->refetch = !!value;\n+\t\treturn 0;\n \t} else if (!strcmp(name, TRANS_OPT_REJECT_SHALLOW)) {\n \t\topts->reject_shallow = !!value;\n \t\treturn 0;\n@@ -377,6 +380,7 @@ static int fetch_refs_via_pack(struct transport *transport,\n \targs.update_shallow = data->options.update_shallow;\n \targs.from_promisor = data->options.from_promisor;\n \targs.filter_options = data->options.filter_options;\n+\targs.refetch = data->options.refetch;\n \targs.stateless_rpc = transport->stateless_rpc;\n \targs.server_options = transport->server_options;\n \targs.negotiation_tips = data->options.negotiation_tips;\ndiff --git a/transport.h b/transport.h\nindex a0bc6a1e9eb..12bc08fc339 100644\n--- a/transport.h\n+++ b/transport.h\n@@ -16,6 +16,7 @@ struct git_transport_options {\n \tunsigned update_shallow : 1;\n \tunsigned reject_shallow : 1;\n \tunsigned deepen_relative : 1;\n+\tunsigned refetch : 1;\n \n \t/* see documentation of corresponding flag in fetch-pack.h */\n \tunsigned from_promisor : 1;\n@@ -216,6 +217,9 @@ void transport_check_allowed(const char *type);\n /* Filter objects for partial clone and fetch */\n #define TRANS_OPT_LIST_OBJECTS_FILTER \"filter\"\n \n+/* Refetch all objects without negotiating */\n+#define TRANS_OPT_REFETCH \"refetch\"\n+\n /* Request atomic (all-or-nothing) updates when pushing */\n #define TRANS_OPT_ATOMIC \"atomic\"\n \n-- \ngitgitgadget\n\n"},{"id":"450407","messageId":"01f22e784a527f11c34703241bdb63b3d99f6fe8.1646406275.git.gitgitgadget@gmail.com","threadId":"57348","inReplyTo":"pull.1138.v3.git.1646406274.gitgitgadget@gmail.com","subject":"[PATCH v3 5/7] t5615-partial-clone: add test for fetch --refetch","fromName":"Robert Coup via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-03-04T15:04:32Z","receivedAt":"2022-03-04T15:05:07Z","isPatch":true,"sender":{"key":"robert@coup.net.nz","avatar":"https://avatars.githubusercontent.com/u/59874?v=4"},"body":"From: Robert Coup \n\nAdd a test for doing a refetch to apply a changed partial clone filter\nunder protocol v0 and v2.\n\nSigned-off-by: Robert Coup \n---\n t/t5616-partial-clone.sh | 52 +++++++++++++++++++++++++++++++++++++++-\n 1 file changed, 51 insertions(+), 1 deletion(-)\n\ndiff --git a/t/t5616-partial-clone.sh b/t/t5616-partial-clone.sh\nindex 34469b6ac10..87ebf4b0b1c 100755\n--- a/t/t5616-partial-clone.sh\n+++ b/t/t5616-partial-clone.sh\n@@ -166,6 +166,56 @@ test_expect_success 'manual prefetch of missing objects' '\n \ttest_line_count = 0 observed.oids\n '\n \n+# create new commits in \"src\" repo to establish a history on file.4.txt\n+# and push to \"srv.bare\".\n+test_expect_success 'push new commits to server for file.4.txt' '\n+\tfor x in a b c d e f\n+\tdo\n+\t\techo \"Mod file.4.txt $x\" >src/file.4.txt &&\n+\t\tif list_contains \"a,b\" \"$x\"; then\n+\t\t\tprintf \"%10000s\" X >>src/file.4.txt\n+\t\tfi &&\n+\t\tif list_contains \"c,d\" \"$x\"; then\n+\t\t\tprintf \"%20000s\" X >>src/file.4.txt\n+\t\tfi &&\n+\t\tgit -C src add file.4.txt &&\n+\t\tgit -C src commit -m \"mod $x\" || return 1\n+\tdone &&\n+\tgit -C src push -u srv main\n+'\n+\n+# Do partial fetch to fetch smaller files; then verify that without --refetch\n+# applying a new filter does not refetch missing large objects. Then use\n+# --refetch to apply the new filter on existing commits. Test it under both\n+# protocol v2 & v0.\n+test_expect_success 'apply a different filter using --refetch' '\n+\tgit -C pc1 fetch --filter=blob:limit=999 origin &&\n+\tgit -C pc1 rev-list --quiet --objects --missing=print \\\n+\t\tmain..origin/main >observed &&\n+\ttest_line_count = 4 observed &&\n+\n+\tgit -C pc1 fetch --filter=blob:limit=19999 --refetch origin &&\n+\tgit -C pc1 rev-list --quiet --objects --missing=print \\\n+\t\tmain..origin/main >observed &&\n+\ttest_line_count = 2 observed &&\n+\n+\tgit -c protocol.version=0 -C pc1 fetch --filter=blob:limit=29999 \\\n+\t\t--refetch origin &&\n+\tgit -C pc1 rev-list --quiet --objects --missing=print \\\n+\t\tmain..origin/main >observed &&\n+\ttest_line_count = 0 observed\n+'\n+\n+test_expect_success 'fetch --refetch works with a shallow clone' '\n+\tgit clone --no-checkout --depth=1 --filter=blob:none \"file://$(pwd)/srv.bare\" pc1s &&\n+\tgit -C pc1s rev-list --objects --missing=print HEAD >observed &&\n+\ttest_line_count = 6 observed &&\n+\n+\tGIT_TRACE=1 git -C pc1s fetch --filter=blob:limit=999 --refetch origin &&\n+\tgit -C pc1s rev-list --objects --missing=print HEAD >observed &&\n+\ttest_line_count = 6 observed\n+'\n+\n test_expect_success 'partial clone with transfer.fsckobjects=1 works with submodules' '\n \ttest_create_repo submodule &&\n \ttest_commit -C submodule mycommit &&\n@@ -225,7 +275,7 @@ test_expect_success 'use fsck before and after manually fetching a missing subtr\n \n \t# Auto-fetch all remaining trees and blobs with --missing=error\n \tgit -C dst rev-list --missing=error --objects main >fetched_objects &&\n-\ttest_line_count = 70 fetched_objects &&\n+\ttest_line_count = 88 fetched_objects &&\n \n \tawk -f print_1.awk fetched_objects |\n \txargs -n1 git -C dst cat-file -t >fetched_types &&\n-- \ngitgitgadget\n\n"},{"id":"450408","messageId":"310466259878bcd32e09092b8de1db0b3e500c2a.1646406275.git.gitgitgadget@gmail.com","threadId":"57348","inReplyTo":"pull.1138.v3.git.1646406274.gitgitgadget@gmail.com","subject":"[PATCH v3 6/7] fetch: after refetch, encourage auto gc repacking","fromName":"Robert Coup via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-03-04T15:04:33Z","receivedAt":"2022-03-04T15:05:10Z","isPatch":true,"sender":{"key":"robert@coup.net.nz","avatar":"https://avatars.githubusercontent.com/u/59874?v=4"},"body":"From: Robert Coup \n\nAfter invoking `fetch --refetch`, the object db will likely contain many\nduplicate objects. If auto-maintenance is enabled, invoke it with\nappropriate settings to encourage repacking/consolidation.\n\n* gc.autoPackLimit: unless this is set to 0 (disabled), override the\n value to 1 to force pack consolidation.\n* maintenance.incremental-repack.auto: unless this is set to 0, override\n the value to -1 to force incremental repacking.\n\nSigned-off-by: Robert Coup \n---\n Documentation/fetch-options.txt | 3 ++-\n builtin/fetch.c | 19 ++++++++++++++++++-\n t/t5616-partial-clone.sh | 29 +++++++++++++++++++++++++++++\n 3 files changed, 49 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/fetch-options.txt b/Documentation/fetch-options.txt\nindex 21a247abfa4..49ae48dca32 100644\n--- a/Documentation/fetch-options.txt\n+++ b/Documentation/fetch-options.txt\n@@ -169,7 +169,8 @@ ifndef::git-pull[]\n \tassociated objects that are already present locally, this option fetches\n \tall objects as a fresh clone would. Use this to reapply a partial clone\n \tfilter from configuration or using `--filter=` when the filter\n-\tdefinition has changed.\n+\tdefinition has changed. Automatic post-fetch maintenance will perform\n+\tobject database pack consolidation to remove any duplicate objects.\n endif::git-pull[]\n \n --refmap=::\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex f7bcf6fa64d..1557e8d57c5 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -2226,8 +2226,25 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n \t\t\t\t\t NULL);\n \t}\n \n-\tif (enable_auto_gc)\n+\tif (enable_auto_gc) {\n+\t\tif (refetch) {\n+\t\t\t/*\n+\t\t\t * Hint auto-maintenance strongly to encourage repacking,\n+\t\t\t * but respect config settings disabling it.\n+\t\t\t */\n+\t\t\tint opt_val;\n+\t\t\tif (git_config_get_int(\"gc.autopacklimit\", &opt_val))\n+\t\t\t\topt_val = -1;\n+\t\t\tif (opt_val != 0)\n+\t\t\t\tgit_config_push_parameter(\"gc.autoPackLimit=1\");\n+\n+\t\t\tif (git_config_get_int(\"maintenance.incremental-repack.auto\", &opt_val))\n+\t\t\t\topt_val = -1;\n+\t\t\tif (opt_val != 0)\n+\t\t\t\tgit_config_push_parameter(\"maintenance.incremental-repack.auto=-1\");\n+\t\t}\n \t\trun_auto_maintenance(verbosity < 0);\n+\t}\n \n cleanup:\n \tstring_list_clear(&list, 0);\ndiff --git a/t/t5616-partial-clone.sh b/t/t5616-partial-clone.sh\nindex 87ebf4b0b1c..4a3778d04a8 100755\n--- a/t/t5616-partial-clone.sh\n+++ b/t/t5616-partial-clone.sh\n@@ -216,6 +216,35 @@ test_expect_success 'fetch --refetch works with a shallow clone' '\n \ttest_line_count = 6 observed\n '\n \n+test_expect_success 'fetch --refetch triggers repacking' '\n+\tGIT_TRACE2_CONFIG_PARAMS=gc.autoPackLimit,maintenance.incremental-repack.auto &&\n+\texport GIT_TRACE2_CONFIG_PARAMS &&\n+\n+\tGIT_TRACE2_EVENT=\"$PWD/trace1.event\" \\\n+\tgit -C pc1 fetch --refetch origin &&\n+\ttest_subcommand git maintenance run --auto --no-quiet \n\nDocument it for partial clones as a means to apply a new filter.\n\nSigned-off-by: Robert Coup \n---\n Documentation/technical/partial-clone.txt | 3 +++\n 1 file changed, 3 insertions(+)\n\ndiff --git a/Documentation/technical/partial-clone.txt b/Documentation/technical/partial-clone.txt\nindex a0dd7c66f24..99f0eb30406 100644\n--- a/Documentation/technical/partial-clone.txt\n+++ b/Documentation/technical/partial-clone.txt\n@@ -181,6 +181,9 @@ Fetching Missing Objects\n currently fetches all objects referred to by the requested objects, even\n though they are not necessary.\n \n+- Fetching with `--refetch` will request a complete new filtered packfile from\n+ the remote, which can be used to change a filter without needing to\n+ dynamically fetch missing objects.\n \n Using many promisor remotes\n ---------------------------\n-- \ngitgitgadget\n"},{"id":"450441","messageId":"xmqqa6e51kms.fsf@gitster.g","threadId":"57348","inReplyTo":"a503b98f33328133f9a89ad7eba986b73535c461.1646406275.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v3 4/7] fetch: add --refetch option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-03-04T21:19:07Z","receivedAt":"2022-03-04T21:19:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Robert Coup via GitGitGadget\" writes:\n\n> +static int verbosity, deepen_relative, set_upstream, refetch;\n> static int progress = -1;\n> static int enable_auto_gc = 1;\n> static int tags = TAGS_DEFAULT, unshallow, update_shallow, deepen;\n> @@ -190,6 +190,9 @@ static struct option builtin_fetch_options[] = {\n> \tOPT_SET_INT_F(0, \"unshallow\", &unshallow,\n> \t\t N_(\"convert to a complete repository\"),\n> \t\t 1, PARSE_OPT_NONEG),\n> +\tOPT_SET_INT_F(0, \"refetch\", &refetch,\n> +\t\t N_(\"re-fetch without negotiating common commits\"),\n> +\t\t 1, PARSE_OPT_NONEG),\n\nI guess the existing --unshallow has the same problem, but it strikes\nme odd that these aren't doing a bog-standard OPT_BOOL(), with default\nvalue of \"false\", like, say \"--update-head-ok\" does.\n\nThat will naturally support things like\n\n\tgit fetch --refetch --no-refetch\n\nwhere a later option overrides what an earlier option did.\n\n"},{"id":"450528","messageId":"CACf-nVfEt8xWgg-d8JmtSD09Mvp=YvshN=cfMQ=304--h5Rjcg@mail.gmail.com","threadId":"57348","inReplyTo":"xmqqa6e51kms.fsf@gitster.g","subject":"Re: [PATCH v3 4/7] fetch: add --refetch option","fromName":"Robert Coup","fromEmail":"robert@coup.net.nz","sentAt":"2022-03-07T11:31:11Z","receivedAt":"2022-03-07T11:33:28Z","isPatch":true,"sender":{"key":"robert@coup.net.nz","avatar":"https://avatars.githubusercontent.com/u/59874?v=4"},"body":"Hi Junio,\n\nOn Fri, 4 Mar 2022 at 21:19, Junio C Hamano wrote:\n> I guess the existing --unshallow has the same problem, but it strikes\n> me odd that these aren't doing a bog-standard OPT_BOOL(), with default\n> value of \"false\", like, say \"--update-head-ok\" does.\n>\n> That will naturally support things like\n>\n> git fetch --refetch --no-refetch\n>\n> where a later option overrides what an earlier option did.\n>\n\nAh, I literally copied the unshallow one since it seemed boolean-ish\nand that's what I wanted. I'll look into it.\n\nThanks,\nRob :)\n"},{"id":"450622","messageId":"xmqq4k49u0za.fsf@gitster.g","threadId":"57348","inReplyTo":"CACf-nVfEt8xWgg-d8JmtSD09Mvp=YvshN=cfMQ=304--h5Rjcg@mail.gmail.com","subject":"Re: [PATCH v3 4/7] fetch: add --refetch option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-03-07T17:27:37Z","receivedAt":"2022-03-07T17:27:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Robert Coup writes:\n\n> Ah, I literally copied the unshallow one since it seemed boolean-ish\n> and that's what I wanted. I'll look into it.\n\nYou may (or may not --- I didn't look) find options that need to be\nOPT_BOOL() other than \"unshallow\" one.\n\nWhile it would be excellent to add a separate \"preliminary clean-up\"\nstep before you add OPT_BOOL(\"refetch\") to fix them to use\nOPT_BOOL() instead of OPT_SET_INT(), that will add extra review\ncycles to the series by extending the scope of it.\n\nIt is OK to leave them, in addition to the new one you are adding,\nto use the old and a bit incorrect pattern, as long as you leave a\nprominent note that we need to clean them up later.\n\nThanks.\n"},{"id":"450816","messageId":"20220309002729.3581315-1-calvinwan@google.com","threadId":"57348","inReplyTo":"pull.1138.v3.git.1646406274.gitgitgadget@gmail.com","subject":"Re: [PATCH v3 0/7] fetch: add repair: full refetch without negotiation (was: \"refiltering\")","fromName":"Calvin Wan","fromEmail":"calvinwan@google.com","sentAt":"2022-03-09T00:27:29Z","receivedAt":"2022-03-09T01:13:18Z","isPatch":true,"sender":{"key":"calvinwan@google.com","avatar":"https://avatars.githubusercontent.com/u/92547554?v=4"},"body":"\"Robert Coup via GitGitGadget\" writes:\n> If a filter is changed on a partial clone repository, for example from\n> blob:none to blob:limit=1m, there is currently no straightforward way to\n> bulk-refetch the objects that match the new filter for existing local\n> commits. This is because the client will report commits as \"have\" during\n> fetch negotiation and any dependent objects won't be included in the\n> transferred pack. Another use case is discussed at [1].\n> \n> This patch series introduces a --refetch option to fetch & fetch-pack to\n> enable doing a full fetch without performing any commit negotiation with the\n> remote, as a fresh clone does. It builds upon cbe566a071 (\"negotiator/noop:\n> add noop fetch negotiator\", 2020-08-18).\n\nHi Robert,\n\nThis is my first time sending a review to the list, so forgive me for any\nmistakes I make or conventions missed. Feedback about my review would be well\nappreciated!\n\nOverall I think your patch is well written and the implementation accomplishes\nwhat you describe in your cover letter, however, I would like to discuss another\npossible design I thought of. Currently, the user has to know to run ‘--refetch’\nafter changing the partial clone filter configuration in order to fetch the\ncommits that match the new filter. Ideally I believe this behavior should be\ndefault so therefore instead of adding an option, if git knew what filter was\nlast used in the fetch, it could automatically ‘refetch’ everything if there is\na change between the last used filter and the default filter. I’m not sure if\nthe config is the best location to store the last used filter, but we can use it\nas an example for now. The tradeoff here is balancing between having an\nadditional config variable and having the user know to specify a parameter to\nfetch after changing the config. And in the future, if there are other use cases\nfor needing a fetch that bypasses commit negotiation (I know you described one\nsuch possible use case in v2), the ‘--refetch’ option can easily be readded to\nhook into this patch.\n\nLooking forward to hearing your thoughts!\n"},{"id":"450822","messageId":"CACf-nVeEBDQse0coA7QpQmQ92y9kDwXoTmayD8_NY2OHNZ5v+g@mail.gmail.com","threadId":"57348","inReplyTo":"20220309002729.3581315-1-calvinwan@google.com","subject":"Re: [PATCH v3 0/7] fetch: add repair: full refetch without negotiation (was: \"refiltering\")","fromName":"Robert Coup","fromEmail":"robert@coup.net.nz","sentAt":"2022-03-09T09:57:37Z","receivedAt":"2022-03-09T09:57:53Z","isPatch":true,"sender":{"key":"robert@coup.net.nz","avatar":"https://avatars.githubusercontent.com/u/59874?v=4"},"body":"Hi Calvin,\n\nOn Wed, 9 Mar 2022 at 00:27, Calvin Wan wrote:\n>\n> This is my first time sending a review to the list, so forgive me for any\n> mistakes I make or conventions missed. Feedback about my review would be well\n> appreciated!\n\nI'm as new as you to contributing to Git :-)\n\n> Overall I think your patch is well written and the implementation accomplishes\n> what you describe in your cover letter,\n\nThanks! I think if you're completely content you can add your\nReviewed-By as described at\nhttps://git-scm.com/docs/SubmittingPatches#commit-trailers\n\n> I would like to discuss another\n> possible design I thought of. Currently, the user has to know to run ‘--refetch’\n> after changing the partial clone filter configuration in order to fetch the\n> commits that match the new filter. Ideally I believe this behavior should be\n> default so therefore instead of adding an option, if git knew what filter was\n> last used in the fetch, it could automatically ‘refetch’ everything if there is\n> a change between the last used filter and the default filter.\n\nSo, if you do a partial clone using `git clone --filter=...` then the\nfilter is saved into the config at `remote..partialclonefilter`\nand is re-used by default for subsequent fetches from that remote. But\nthere's nothing to stop `git fetch --filter=...` being run multiple\ntimes with different filters to carefully setup a repository for a\nparticular use case, or any notion that there has to be \"one\" filter\nin place for a remote.\n\nRunning `git fetch --filter=...` doesn't update the remote's partial\nclone filter in the config, and IMO it shouldn't for the above reason.\n\nI think having `git config remote..partialclonefilter\n` print out something to the user along the lines of \"Your\nchange to/removal of the filter won't fetch in additional objects\nassociated with existing commits, you can do this with `fetch\n--refetch `\" could be helpful, but after a very quick look I\ncan't see anything like that at the moment for other config settings\n(ie. no plumbing in place to easily reuse), and I'm not motivated to\nadd such plumbing.\n\nCheers,\n\nRob :)\n"},{"id":"450823","messageId":"CACf-nVdER0KGsaBPdcxbM6njy4ek8Ww0cW8KDLOd=bYih7VF8w@mail.gmail.com","threadId":"57348","inReplyTo":"xmqq4k49u0za.fsf@gitster.g","subject":"Re: [PATCH v3 4/7] fetch: add --refetch option","fromName":"Robert Coup","fromEmail":"robert@coup.net.nz","sentAt":"2022-03-09T10:00:17Z","receivedAt":"2022-03-09T10:00:35Z","isPatch":true,"sender":{"key":"robert@coup.net.nz","avatar":"https://avatars.githubusercontent.com/u/59874?v=4"},"body":"Hi Junio,\n\nOn Mon, 7 Mar 2022 at 17:27, Junio C Hamano wrote:\n>\n> You may (or may not --- I didn't look) find options that need to be\n> OPT_BOOL() other than \"unshallow\" one.\n>\n> While it would be excellent to add a separate \"preliminary clean-up\"\n> step before you add OPT_BOOL(\"refetch\") to fix them to use\n> OPT_BOOL() instead of OPT_SET_INT(), that will add extra review\n> cycles to the series by extending the scope of it.\n>\n> It is OK to leave them, in addition to the new one you are adding,\n> to use the old and a bit incorrect pattern, as long as you leave a\n> prominent note that we need to clean them up later.\n\nI think for this series I will just do a re-roll updating refetch to\nuse OPT_BOOL, and I can do a separate series to look for other\nexisting booleans using OPT_SET_INT.\n\nRob :)\n"},{"id":"450931","messageId":"xmqqzglylslh.fsf@gitster.g","threadId":"57348","inReplyTo":"CACf-nVeEBDQse0coA7QpQmQ92y9kDwXoTmayD8_NY2OHNZ5v+g@mail.gmail.com","subject":"Re: [PATCH v3 0/7] fetch: add repair: full refetch without negotiation","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-03-09T21:32:42Z","receivedAt":"2022-03-09T21:32:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Robert Coup writes:\n\n> So, if you do a partial clone using `git clone --filter=...` then the\n> filter is saved into the config at `remote..partialclonefilter`\n> and is re-used by default for subsequent fetches from that remote. But\n> there's nothing to stop `git fetch --filter=...` being run multiple\n> times with different filters to carefully setup a repository for a\n> particular use case, or any notion that there has to be \"one\" filter\n> in place for a remote.\n\nThe way I read Calvin's suggestion was that you won't allow such a\nrandom series of \"git fetch\"es without updating the \"this is the\nfilter that is consistent with the contents of this repository\"\nrecord, which will lead to inconsistencies. I.e.\n\n - we must maintain the \"filter that is consistent with the contents\n of this repository\", which this series does not do, but we should.\n\n - the \"--refetch\" is unnecessary and redundant, as long as such a\n record is maintained; when a filter settings changes, we should\n do the equivalent of \"--refetch\" automatically.\n\nIOW, ...\n\n> Running `git fetch --filter=...` doesn't update the remote's partial\n> clone filter in the config, and IMO it shouldn't for the above reason.\n\n... isn't \"git fetch --fitler\" that does not update the configured\nfilter (and does not do a refetch automatically) a bug that made the\n\"refetch\" necessary in the first place?\n\nOr perhaps I read Calvin incorrectly?\n"},{"id":"450969","messageId":"CAFySSZCMq+6_8p-UVu_dzgcNSv3zqCN76WjD4XWKWB9TQ0y40g@mail.gmail.com","threadId":"57348","inReplyTo":"xmqqzglylslh.fsf@gitster.g","subject":"Re: [PATCH v3 0/7] fetch: add repair: full refetch without negotiation","fromName":"Calvin Wan","fromEmail":"calvinwan@google.com","sentAt":"2022-03-10T01:07:27Z","receivedAt":"2022-03-10T01:07:42Z","isPatch":true,"sender":{"key":"calvinwan@google.com","avatar":"https://avatars.githubusercontent.com/u/92547554?v=4"},"body":"On 3/9/2022 1:32 PM, Junio C Hamano wrote:\n\n > - we must maintain the \"filter that is consistent with the contents\n > of this repository\", which this series does not do, but we should.\n\nIf this is possible/reasonably implementable then...\n\n > - the \"--refetch\" is unnecessary and redundant, as long as such a\n > record is maintained; when a filter settings changes, we should\n > do the equivalent of \"--refetch\" automatically.\n\n... this is the logical conclusion that can be drawn.\n\n> isn't \"git fetch --filter\" that does not update the configured\n> filter (and does not do a refetch automatically) a bug that made the\n> \"refetch\" necessary in the first place?\n\nThere are two cases to be considered here:\n1. The user changes the filter config using \"git config\"\n2. The user runs \"git fetch --filter\"\n\nThe first case is the use case advocated by Robert for \"--refetch\".\nI originally suggested saving the last used filter into the config\nand if the last used filter != the configured filter, then fetch\nautomatically \"refetches\" everything.\n\nThe second use case is the one Junio believes I am referring to,\nhowever, whether to classify this as a bug or feature, I am\nuncertain. Like above, I suggested saving this as the last used\nfilter to be used as a comparison to the configured filter.\nSetting this filter as the configured filter can be a separate\ndiscussion to be had because I can see pros/cons for it.\n\nUltimately the expectation is that if I run \"git fetch\" without --filter,\nthen I will fetch based on the config filter. And if I have previously\nrun \"git fetch\" with the same filter, whether through the config or\nmy own filter, then Git will only fetch the latest refs/objects.\n\n\n\nOn Wed, Mar 9, 2022 at 1:32 PM Junio C Hamano wrote:\n>\n> Robert Coup writes:\n>\n> > So, if you do a partial clone using `git clone --filter=...` then the\n> > filter is saved into the config at `remote..partialclonefilter`\n> > and is re-used by default for subsequent fetches from that remote. But\n> > there's nothing to stop `git fetch --filter=...` being run multiple\n> > times with different filters to carefully setup a repository for a\n> > particular use case, or any notion that there has to be \"one\" filter\n> > in place for a remote.\n>\n> The way I read Calvin's suggestion was that you won't allow such a\n> random series of \"git fetch\"es without updating the \"this is the\n> filter that is consistent with the contents of this repository\"\n> record, which will lead to inconsistencies. I.e.\n>\n> - we must maintain the \"filter that is consistent with the contents\n> of this repository\", which this series does not do, but we should.\n>\n> - the \"--refetch\" is unnecessary and redundant, as long as such a\n> record is maintained; when a filter settings changes, we should\n> do the equivalent of \"--refetch\" automatically.\n>\n> IOW, ...\n>\n> > Running `git fetch --filter=...` doesn't update the remote's partial\n> > clone filter in the config, and IMO it shouldn't for the above reason.\n>\n> ... isn't \"git fetch --fitler\" that does not update the configured\n> filter (and does not do a refetch automatically) a bug that made the\n> \"refetch\" necessary in the first place?\n>\n> Or perhaps I read Calvin incorrectly?\n"},{"id":"451019","messageId":"CACf-nVf-O5+_nMrdBiRJ5sHg7g+DjTkPy1VuErDnEXJ0-9OJ0A@mail.gmail.com","threadId":"57348","inReplyTo":"xmqqzglylslh.fsf@gitster.g","subject":"Re: [PATCH v3 0/7] fetch: add repair: full refetch without negotiation","fromName":"Robert Coup","fromEmail":"robert@coup.net.nz","sentAt":"2022-03-10T14:29:02Z","receivedAt":"2022-03-10T14:31:33Z","isPatch":true,"sender":{"key":"robert@coup.net.nz","avatar":"https://avatars.githubusercontent.com/u/59874?v=4"},"body":"Hi,\n\nOn Wed, 9 Mar 2022 at 21:32, Junio C Hamano wrote:\n>\n> The way I read Calvin's suggestion was that you won't allow such a\n> random series of \"git fetch\"es without updating the \"this is the\n> filter that is consistent with the contents of this repository\"\n> record, which will lead to inconsistencies. I.e.\n>\n> - we must maintain the \"filter that is consistent with the contents\n> of this repository\", which this series does not do, but we should.\n\nI don't think we should strive to keep this \"consistency\" —\n\n> - the \"--refetch\" is unnecessary and redundant, as long as such a\n> record is maintained; when a filter settings changes, we should\n> do the equivalent of \"--refetch\" automatically.\n\n— we don't know how much data has been pulled in by fetches from\ndifferent promisor and non-promisor remotes (past & present); or\ndynamically faulted in through branch switching or history\nexploration. And I can't see any particular benefit in attempting to\nkeep track of that?\n\nÆvar suggested in future maybe we could figure out which commits a\nuser definitively has all the blobs & trees for and refetch could\nnegotiate from that position to improve efficiency: nothing in this\nseries precludes such an enhancement.\n\n> ... isn't \"git fetch --fitler\" that does not update the configured\n> filter (and does not do a refetch automatically) a bug that made the\n> \"refetch\" necessary in the first place?\n\nI don't believe it's a bug. A fairly obvious partial clone example\nI've used before on repos where I want the commit history but not all\nthe associated data (especially when the history is littered with\ngiant blobs I don't care about):\n\n git clone example.com/myrepo --filter=blob:none\n # does a partial clone with no blobs\n # checkout faults in the blobs present at HEAD in bulk to populate\nthe working tree\n git config --unset remote.origin.partialclonefilter\n # going forward, future fetches include all associated blobs for new commits\n\nGetting all the blobs for all history is something I'm explicitly\ntrying not to do in this example, but if the next fetch from origin\nautomatically did a \"refetch\" after I removed the filter that's\nexactly what would happen.\n\nWe don't expect users to update `diff.algorithm` in config to run a\nminimal diff: using the `--diff-algorithm=` option on the command line\noverrides the config. And the same philosophy applies with fetch:\n`remote..partialclonefilter` provides the default filter for\nfetches, and a user can override it via `git fetch --filter=`. To me\nthis is how Git commands are expected to work.\n\nPartial clones are still relatively new and advanced, and I don't\nbelieve we should try and over-predict too much what the correct\nbehaviour is for a user.\n\nI'd be happy adding something to the documentation for the\n`remote..partialclonefilter` config setting to explain that\nchanging or removing the filter won't backfill the local object DB and\nthe user would need `fetch --refetch` for that.\n\nThanks,\nRob :)\n"},{"id":"451742","messageId":"CAFySSZAunjiEsSMETdYB3TFVppPD5RfjQjMmvTqwy6mEteninA@mail.gmail.com","threadId":"57348","inReplyTo":"CACf-nVf-O5+_nMrdBiRJ5sHg7g+DjTkPy1VuErDnEXJ0-9OJ0A@mail.gmail.com","subject":"Re: [PATCH v3 0/7] fetch: add repair: full refetch without negotiation","fromName":"Calvin Wan","fromEmail":"calvinwan@google.com","sentAt":"2022-03-21T17:58:00Z","receivedAt":"2022-03-21T17:58:19Z","isPatch":true,"sender":{"key":"calvinwan@google.com","avatar":"https://avatars.githubusercontent.com/u/92547554?v=4"},"body":"Hi Robert,\n\nDocumentation for the config setting is an acceptable solution!\nApologies for the late response -- wanted to wait and see if anyone\nelse on the list had any last thoughts. Also I noticed you were hoping\nthat Jonathan Tan could take a look at your patch on the What's\nCooking thread. Before I sent my first review out, I discussed your\npatch with him so he's been briefed.\n\nReviewed-by: Calvin Wan \n\n\nOn Thu, Mar 10, 2022 at 6:29 AM Robert Coup wrote:\n>\n> Hi,\n>\n> On Wed, 9 Mar 2022 at 21:32, Junio C Hamano wrote:\n> >\n> > The way I read Calvin's suggestion was that you won't allow such a\n> > random series of \"git fetch\"es without updating the \"this is the\n> > filter that is consistent with the contents of this repository\"\n> > record, which will lead to inconsistencies. I.e.\n> >\n> > - we must maintain the \"filter that is consistent with the contents\n> > of this repository\", which this series does not do, but we should.\n>\n> I don't think we should strive to keep this \"consistency\" —\n>\n> > - the \"--refetch\" is unnecessary and redundant, as long as such a\n> > record is maintained; when a filter settings changes, we should\n> > do the equivalent of \"--refetch\" automatically.\n>\n> — we don't know how much data has been pulled in by fetches from\n> different promisor and non-promisor remotes (past & present); or\n> dynamically faulted in through branch switching or history\n> exploration. And I can't see any particular benefit in attempting to\n> keep track of that?\n>\n> Ævar suggested in future maybe we could figure out which commits a\n> user definitively has all the blobs & trees for and refetch could\n> negotiate from that position to improve efficiency: nothing in this\n> series precludes such an enhancement.\n>\n> > ... isn't \"git fetch --fitler\" that does not update the configured\n> > filter (and does not do a refetch automatically) a bug that made the\n> > \"refetch\" necessary in the first place?\n>\n> I don't believe it's a bug. A fairly obvious partial clone example\n> I've used before on repos where I want the commit history but not all\n> the associated data (especially when the history is littered with\n> giant blobs I don't care about):\n>\n> git clone example.com/myrepo --filter=blob:none\n> # does a partial clone with no blobs\n> # checkout faults in the blobs present at HEAD in bulk to populate\n> the working tree\n> git config --unset remote.origin.partialclonefilter\n> # going forward, future fetches include all associated blobs for new commits\n>\n> Getting all the blobs for all history is something I'm explicitly\n> trying not to do in this example, but if the next fetch from origin\n> automatically did a \"refetch\" after I removed the filter that's\n> exactly what would happen.\n>\n> We don't expect users to update `diff.algorithm` in config to run a\n> minimal diff: using the `--diff-algorithm=` option on the command line\n> overrides the config. And the same philosophy applies with fetch:\n> `remote..partialclonefilter` provides the default filter for\n> fetches, and a user can override it via `git fetch --filter=`. To me\n> this is how Git commands are expected to work.\n>\n> Partial clones are still relatively new and advanced, and I don't\n> believe we should try and over-predict too much what the correct\n> behaviour is for a user.\n>\n> I'd be happy adding something to the documentation for the\n> `remote..partialclonefilter` config setting to explain that\n> changing or removing the filter won't backfill the local object DB and\n> the user would need `fetch --refetch` for that.\n>\n> Thanks,\n> Rob :)\n"},{"id":"451771","messageId":"CACf-nVf8=2wHSYXinU5M-wm9btyCg4hKjaaX2HxvsHb=YoqhdA@mail.gmail.com","threadId":"57348","inReplyTo":"CAFySSZAunjiEsSMETdYB3TFVppPD5RfjQjMmvTqwy6mEteninA@mail.gmail.com","subject":"Re: [PATCH v3 0/7] fetch: add repair: full refetch without negotiation","fromName":"Robert Coup","fromEmail":"robert@coup.net.nz","sentAt":"2022-03-21T21:34:34Z","receivedAt":"2022-03-21T21:48:30Z","isPatch":true,"sender":{"key":"robert@coup.net.nz","avatar":"https://avatars.githubusercontent.com/u/59874?v=4"},"body":"Hi Calvin\n\nOn Mon, 21 Mar 2022 at 17:58, Calvin Wan wrote:\n> Documentation for the config setting is an acceptable solution!\n\nGreat :)\n\n> Apologies for the late response -- wanted to wait and see if anyone\n> else on the list had any last thoughts. Also I noticed you were hoping\n> that Jonathan Tan could take a look at your patch on the What's\n> Cooking thread. Before I sent my first review out, I discussed your\n> patch with him so he's been briefed.\n>\n> Reviewed-by: Calvin Wan \n\nThank you.\n\n@Junio - I'm on leave for the remainder of this week, so expect the\nre-roll sometime next week.\n\nRob :)\n"},{"id":"452468","messageId":"pull.1138.v4.git.1648476131.gitgitgadget@gmail.com","threadId":"57348","inReplyTo":"pull.1138.v3.git.1646406274.gitgitgadget@gmail.com","subject":"[PATCH v4 0/7] fetch: add repair: full refetch without negotiation (was: \"refiltering\")","fromName":"Robert Coup via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-03-28T14:02:04Z","receivedAt":"2022-03-28T14:02:19Z","isPatch":true,"sender":{"key":"robert@coup.net.nz","avatar":"https://avatars.githubusercontent.com/u/59874?v=4"},"body":"If a filter is changed on a partial clone repository, for example from\nblob:none to blob:limit=1m, there is currently no straightforward way to\nbulk-refetch the objects that match the new filter for existing local\ncommits. This is because the client will report commits as \"have\" during\nfetch negotiation and any dependent objects won't be included in the\ntransferred pack. Another use case is discussed at [1].\n\nThis patch series introduces a --refetch option to fetch & fetch-pack to\nenable doing a full fetch without performing any commit negotiation with the\nremote, as a fresh clone does. It builds upon cbe566a071 (\"negotiator/noop:\nadd noop fetch negotiator\", 2020-08-18).\n\n * Using --refetch will produce duplicated objects between the existing and\n newly fetched packs, but maintenance will clean them up when it runs\n automatically post-fetch (if enabled).\n * If a user fetches with --refetch applying a more restrictive partial\n clone filter than previously (eg: blob:limit=1m then blob:limit=1k) the\n eventual state is a no-op, since any referenced object already in the\n local repository is never removed. More advanced repacking which could\n improve this scenario is currently proposed at [2].\n\n[1]\nhttps://lore.kernel.org/git/aa7b89ee-08aa-7943-6a00-28dcf344426e@syntevo.com/\n[2]\nhttps://lore.kernel.org/git/21ED346B-A906-4905-B061-EDE53691C586@gmail.com/\n\nChanges since v3:\n\n * Mention fetch --refetch in the remote..partialclonefilter\n documentation.\n\nChanges since v2:\n\n * Changed the name from \"repair\" to \"refetch\". While it's conceivable to\n use it in some object DB repair situations that's not the focus of these\n changes.\n * Pass config options to maintenance via GIT_CONFIG_PARAMETERS\n * Split out auto-maintenance to a separate & more robust test\n * Minor fixes/improvements from reviews by Junio & Ævar\n\nChanges since RFC (v1):\n\n * Changed the name from \"refilter\" to \"repair\"\n * Removed dependency between server-side support for filtering and repair\n * Added a test case for a shallow clone\n * Post-fetch auto maintenance now strongly encourages\n repacking/consolidation\n\nReviewed-by: Calvin Wan calvinwan@google.com\n\nRobert Coup (7):\n fetch-negotiator: add specific noop initializer\n fetch-pack: add refetch\n builtin/fetch-pack: add --refetch option\n fetch: add --refetch option\n t5615-partial-clone: add test for fetch --refetch\n fetch: after refetch, encourage auto gc repacking\n docs: mention --refetch fetch option\n\n Documentation/config/remote.txt | 6 +-\n Documentation/fetch-options.txt | 10 +++\n Documentation/git-fetch-pack.txt | 4 ++\n Documentation/technical/partial-clone.txt | 3 +\n builtin/fetch-pack.c | 4 ++\n builtin/fetch.c | 34 +++++++++-\n fetch-negotiator.c | 5 ++\n fetch-negotiator.h | 8 +++\n fetch-pack.c | 46 ++++++++-----\n fetch-pack.h | 1 +\n remote-curl.c | 6 ++\n t/t5616-partial-clone.sh | 81 ++++++++++++++++++++++-\n transport-helper.c | 3 +\n transport.c | 4 ++\n transport.h | 4 ++\n 15 files changed, 197 insertions(+), 22 deletions(-)\n\n\nbase-commit: abf474a5dd901f28013c52155411a48fd4c09922\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1138%2Frcoup%2Frc-partial-clone-refilter-v4\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1138/rcoup/rc-partial-clone-refilter-v4\nPull-Request: https://github.com/gitgitgadget/git/pull/1138\n\nRange-diff vs v3:\n\n 1: 96a75be3d8a = 1: 6cd6d4a59f6 fetch-negotiator: add specific noop initializer\n 2: 04ca6a07f85 = 2: 03f0de3d28c fetch-pack: add refetch\n 3: 879d30c4473 = 3: f7942344ff8 builtin/fetch-pack: add --refetch option\n 4: a503b98f333 = 4: 78501bbf281 fetch: add --refetch option\n 5: 01f22e784a5 = 5: 6c17167ac1e t5615-partial-clone: add test for fetch --refetch\n 6: 31046625987 = 6: 28c07219fd8 fetch: after refetch, encourage auto gc repacking\n 7: f923a06aab5 ! 7: da1e6de7a9f doc/partial-clone: mention --refetch fetch option\n @@ Metadata\n Author: Robert Coup \n \n ## Commit message ##\n - doc/partial-clone: mention --refetch fetch option\n + docs: mention --refetch fetch option\n \n - Document it for partial clones as a means to apply a new filter.\n + Document it for partial clones as a means to apply a new filter, and\n + reference it from the remote..partialclonefilter config parameter.\n \n Signed-off-by: Robert Coup \n \n + ## Documentation/config/remote.txt ##\n +@@ Documentation/config/remote.txt: remote..promisor::\n + \tobjects.\n + \n + remote..partialclonefilter::\n +-\tThe filter that will be applied when fetching from this\n +-\tpromisor remote.\n ++\tThe filter that will be applied when fetching from this\tpromisor remote.\n ++\tChanging or clearing this value will only affect fetches for new commits.\n ++\tTo fetch associated objects for commits already present in the local object\n ++\tdatabase, use the `--refetch` option of linkgit:git-fetch[1].\n +\n ## Documentation/technical/partial-clone.txt ##\n @@ Documentation/technical/partial-clone.txt: Fetching Missing Objects\n currently fetches all objects referred to by the requested objects, even\n\n-- \ngitgitgadget\n"},{"id":"452469","messageId":"6cd6d4a59f6596ee190e97f72e788a23c4fa5534.1648476132.git.gitgitgadget@gmail.com","threadId":"57348","inReplyTo":"pull.1138.v4.git.1648476131.gitgitgadget@gmail.com","subject":"[PATCH v4 1/7] fetch-negotiator: add specific noop initializer","fromName":"Robert Coup via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-03-28T14:02:05Z","receivedAt":"2022-03-28T14:02:22Z","isPatch":true,"sender":{"key":"robert@coup.net.nz","avatar":"https://avatars.githubusercontent.com/u/59874?v=4"},"body":"From: Robert Coup \n\nAdd a specific initializer for the noop fetch negotiator. This is\nintroduced to support allowing partial clones to skip commit negotiation\nwhen performing a \"refetch\".\n\nSigned-off-by: Robert Coup \n---\n fetch-negotiator.c | 5 +++++\n fetch-negotiator.h | 8 ++++++++\n 2 files changed, 13 insertions(+)\n\ndiff --git a/fetch-negotiator.c b/fetch-negotiator.c\nindex 874797d767b..be383367f55 100644\n--- a/fetch-negotiator.c\n+++ b/fetch-negotiator.c\n@@ -23,3 +23,8 @@ void fetch_negotiator_init(struct repository *r,\n \t\treturn;\n \t}\n }\n+\n+void fetch_negotiator_init_noop(struct fetch_negotiator *negotiator)\n+{\n+\tnoop_negotiator_init(negotiator);\n+}\ndiff --git a/fetch-negotiator.h b/fetch-negotiator.h\nindex ea78868504b..e348905a1f0 100644\n--- a/fetch-negotiator.h\n+++ b/fetch-negotiator.h\n@@ -53,7 +53,15 @@ struct fetch_negotiator {\n \tvoid *data;\n };\n \n+/*\n+ * Initialize a negotiator based on the repository settings.\n+ */\n void fetch_negotiator_init(struct repository *r,\n \t\t\t struct fetch_negotiator *negotiator);\n \n+/*\n+ * Initialize a noop negotiator.\n+ */\n+void fetch_negotiator_init_noop(struct fetch_negotiator *negotiator);\n+\n #endif\n-- \ngitgitgadget\n\n"},{"id":"452470","messageId":"03f0de3d28c00cbfda73f31a21e0fa8f8fe51742.1648476132.git.gitgitgadget@gmail.com","threadId":"57348","inReplyTo":"pull.1138.v4.git.1648476131.gitgitgadget@gmail.com","subject":"[PATCH v4 2/7] fetch-pack: add refetch","fromName":"Robert Coup via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-03-28T14:02:06Z","receivedAt":"2022-03-28T14:02:25Z","isPatch":true,"sender":{"key":"robert@coup.net.nz","avatar":"https://avatars.githubusercontent.com/u/59874?v=4"},"body":"From: Robert Coup \n\nAllow a \"refetch\" where the contents of the local object store are\nignored and a full fetch is performed, not attempting to find or\nnegotiate common commits with the remote.\n\nA key use case is to apply a new partial clone blob/tree filter and\nrefetch all the associated matching content, which would otherwise not\nbe transferred when the commit objects are already present locally.\n\nSigned-off-by: Robert Coup \n---\n fetch-pack.c | 46 +++++++++++++++++++++++++++++-----------------\n fetch-pack.h | 1 +\n 2 files changed, 30 insertions(+), 17 deletions(-)\n\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex 87657907e78..4e1e88eea09 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -312,19 +312,21 @@ static int find_common(struct fetch_negotiator *negotiator,\n \t\tconst char *remote_hex;\n \t\tstruct object *o;\n \n-\t\t/*\n-\t\t * If that object is complete (i.e. it is an ancestor of a\n-\t\t * local ref), we tell them we have it but do not have to\n-\t\t * tell them about its ancestors, which they already know\n-\t\t * about.\n-\t\t *\n-\t\t * We use lookup_object here because we are only\n-\t\t * interested in the case we *know* the object is\n-\t\t * reachable and we have already scanned it.\n-\t\t */\n-\t\tif (((o = lookup_object(the_repository, remote)) != NULL) &&\n-\t\t\t\t(o->flags & COMPLETE)) {\n-\t\t\tcontinue;\n+\t\tif (!args->refetch) {\n+\t\t\t/*\n+\t\t\t* If that object is complete (i.e. it is an ancestor of a\n+\t\t\t* local ref), we tell them we have it but do not have to\n+\t\t\t* tell them about its ancestors, which they already know\n+\t\t\t* about.\n+\t\t\t*\n+\t\t\t* We use lookup_object here because we are only\n+\t\t\t* interested in the case we *know* the object is\n+\t\t\t* reachable and we have already scanned it.\n+\t\t\t*/\n+\t\t\tif (((o = lookup_object(the_repository, remote)) != NULL) &&\n+\t\t\t\t\t(o->flags & COMPLETE)) {\n+\t\t\t\tcontinue;\n+\t\t\t}\n \t\t}\n \n \t\tremote_hex = oid_to_hex(remote);\n@@ -692,6 +694,9 @@ static void mark_complete_and_common_ref(struct fetch_negotiator *negotiator,\n \tint old_save_commit_buffer = save_commit_buffer;\n \ttimestamp_t cutoff = 0;\n \n+\tif (args->refetch)\n+\t\treturn;\n+\n \tsave_commit_buffer = 0;\n \n \ttrace2_region_enter(\"fetch-pack\", \"parse_remote_refs_and_find_cutoff\", NULL);\n@@ -1028,7 +1033,11 @@ static struct ref *do_fetch_pack(struct fetch_pack_args *args,\n \tstruct fetch_negotiator *negotiator;\n \n \tnegotiator = &negotiator_alloc;\n-\tfetch_negotiator_init(r, negotiator);\n+\tif (args->refetch) {\n+\t\tfetch_negotiator_init_noop(negotiator);\n+\t} else {\n+\t\tfetch_negotiator_init(r, negotiator);\n+\t}\n \n \tsort_ref_list(&ref, ref_compare_name);\n \tQSORT(sought, nr_sought, cmp_ref_by_name);\n@@ -1121,7 +1130,7 @@ static struct ref *do_fetch_pack(struct fetch_pack_args *args,\n \n \tmark_complete_and_common_ref(negotiator, args, &ref);\n \tfilter_refs(args, &ref, sought, nr_sought);\n-\tif (everything_local(args, &ref)) {\n+\tif (!args->refetch && everything_local(args, &ref)) {\n \t\tpacket_flush(fd[1]);\n \t\tgoto all_done;\n \t}\n@@ -1587,7 +1596,10 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,\n \tstruct strvec index_pack_args = STRVEC_INIT;\n \n \tnegotiator = &negotiator_alloc;\n-\tfetch_negotiator_init(r, negotiator);\n+\tif (args->refetch)\n+\t\tfetch_negotiator_init_noop(negotiator);\n+\telse\n+\t\tfetch_negotiator_init(r, negotiator);\n \n \tpacket_reader_init(&reader, fd[0], NULL, 0,\n \t\t\t PACKET_READ_CHOMP_NEWLINE |\n@@ -1613,7 +1625,7 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,\n \t\t\t/* Filter 'ref' by 'sought' and those that aren't local */\n \t\t\tmark_complete_and_common_ref(negotiator, args, &ref);\n \t\t\tfilter_refs(args, &ref, sought, nr_sought);\n-\t\t\tif (everything_local(args, &ref))\n+\t\t\tif (!args->refetch && everything_local(args, &ref))\n \t\t\t\tstate = FETCH_DONE;\n \t\t\telse\n \t\t\t\tstate = FETCH_SEND_REQUEST;\ndiff --git a/fetch-pack.h b/fetch-pack.h\nindex 7f94a2a5831..8c7752fc821 100644\n--- a/fetch-pack.h\n+++ b/fetch-pack.h\n@@ -42,6 +42,7 @@ struct fetch_pack_args {\n \tunsigned update_shallow:1;\n \tunsigned reject_shallow_remote:1;\n \tunsigned deepen:1;\n+\tunsigned refetch:1;\n \n \t/*\n \t * Indicate that the remote of this request is a promisor remote. The\n-- \ngitgitgadget\n\n"},{"id":"452471","messageId":"f7942344ff897fc064346f283734022fc0ffc540.1648476132.git.gitgitgadget@gmail.com","threadId":"57348","inReplyTo":"pull.1138.v4.git.1648476131.gitgitgadget@gmail.com","subject":"[PATCH v4 3/7] builtin/fetch-pack: add --refetch option","fromName":"Robert Coup via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-03-28T14:02:07Z","receivedAt":"2022-03-28T14:02:28Z","isPatch":true,"sender":{"key":"robert@coup.net.nz","avatar":"https://avatars.githubusercontent.com/u/59874?v=4"},"body":"From: Robert Coup \n\nAdd a refetch option to fetch-pack to force a full fetch. Use when\napplying a new partial clone filter to refetch all matching objects.\n\nSigned-off-by: Robert Coup \n---\n Documentation/git-fetch-pack.txt | 4 ++++\n builtin/fetch-pack.c | 4 ++++\n remote-curl.c | 6 ++++++\n 3 files changed, 14 insertions(+)\n\ndiff --git a/Documentation/git-fetch-pack.txt b/Documentation/git-fetch-pack.txt\nindex c9758847937..46747d5f429 100644\n--- a/Documentation/git-fetch-pack.txt\n+++ b/Documentation/git-fetch-pack.txt\n@@ -101,6 +101,10 @@ be in a separate packet, and the list must end with a flush packet.\n \tcurrent shallow boundary instead of from the tip of each\n \tremote branch history.\n \n+--refetch::\n+\tSkips negotiating commits with the server in order to fetch all matching\n+\tobjects. Use to reapply a new partial clone blob/tree filter.\n+\n --no-progress::\n \tDo not show the progress.\n \ndiff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c\nindex c2d96f4c89a..1f8aec97d47 100644\n--- a/builtin/fetch-pack.c\n+++ b/builtin/fetch-pack.c\n@@ -153,6 +153,10 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)\n \t\t\targs.from_promisor = 1;\n \t\t\tcontinue;\n \t\t}\n+\t\tif (!strcmp(\"--refetch\", arg)) {\n+\t\t\targs.refetch = 1;\n+\t\t\tcontinue;\n+\t\t}\n \t\tif (skip_prefix(arg, (\"--\" CL_ARG__FILTER \"=\"), &arg)) {\n \t\t\tparse_list_objects_filter(&args.filter_options, arg);\n \t\t\tcontinue;\ndiff --git a/remote-curl.c b/remote-curl.c\nindex ff44f41011e..67f178b1120 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -43,6 +43,7 @@ struct options {\n \t\t/* see documentation of corresponding flag in fetch-pack.h */\n \t\tfrom_promisor : 1,\n \n+\t\trefetch : 1,\n \t\tatomic : 1,\n \t\tobject_format : 1,\n \t\tforce_if_includes : 1;\n@@ -198,6 +199,9 @@ static int set_option(const char *name, const char *value)\n \t} else if (!strcmp(name, \"from-promisor\")) {\n \t\toptions.from_promisor = 1;\n \t\treturn 0;\n+\t} else if (!strcmp(name, \"refetch\")) {\n+\t\toptions.refetch = 1;\n+\t\treturn 0;\n \t} else if (!strcmp(name, \"filter\")) {\n \t\toptions.filter = xstrdup(value);\n \t\treturn 0;\n@@ -1182,6 +1186,8 @@ static int fetch_git(struct discovery *heads,\n \t\tstrvec_push(&args, \"--deepen-relative\");\n \tif (options.from_promisor)\n \t\tstrvec_push(&args, \"--from-promisor\");\n+\tif (options.refetch)\n+\t\tstrvec_push(&args, \"--refetch\");\n \tif (options.filter)\n \t\tstrvec_pushf(&args, \"--filter=%s\", options.filter);\n \tstrvec_push(&args, url.buf);\n-- \ngitgitgadget\n\n"},{"id":"452472","messageId":"6c17167ac1edbeea5f6d375a989aa49f5748d099.1648476132.git.gitgitgadget@gmail.com","threadId":"57348","inReplyTo":"pull.1138.v4.git.1648476131.gitgitgadget@gmail.com","subject":"[PATCH v4 5/7] t5615-partial-clone: add test for fetch --refetch","fromName":"Robert Coup via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-03-28T14:02:09Z","receivedAt":"2022-03-28T14:02:31Z","isPatch":true,"sender":{"key":"robert@coup.net.nz","avatar":"https://avatars.githubusercontent.com/u/59874?v=4"},"body":"From: Robert Coup \n\nAdd a test for doing a refetch to apply a changed partial clone filter\nunder protocol v0 and v2.\n\nSigned-off-by: Robert Coup \n---\n t/t5616-partial-clone.sh | 52 +++++++++++++++++++++++++++++++++++++++-\n 1 file changed, 51 insertions(+), 1 deletion(-)\n\ndiff --git a/t/t5616-partial-clone.sh b/t/t5616-partial-clone.sh\nindex 34469b6ac10..87ebf4b0b1c 100755\n--- a/t/t5616-partial-clone.sh\n+++ b/t/t5616-partial-clone.sh\n@@ -166,6 +166,56 @@ test_expect_success 'manual prefetch of missing objects' '\n \ttest_line_count = 0 observed.oids\n '\n \n+# create new commits in \"src\" repo to establish a history on file.4.txt\n+# and push to \"srv.bare\".\n+test_expect_success 'push new commits to server for file.4.txt' '\n+\tfor x in a b c d e f\n+\tdo\n+\t\techo \"Mod file.4.txt $x\" >src/file.4.txt &&\n+\t\tif list_contains \"a,b\" \"$x\"; then\n+\t\t\tprintf \"%10000s\" X >>src/file.4.txt\n+\t\tfi &&\n+\t\tif list_contains \"c,d\" \"$x\"; then\n+\t\t\tprintf \"%20000s\" X >>src/file.4.txt\n+\t\tfi &&\n+\t\tgit -C src add file.4.txt &&\n+\t\tgit -C src commit -m \"mod $x\" || return 1\n+\tdone &&\n+\tgit -C src push -u srv main\n+'\n+\n+# Do partial fetch to fetch smaller files; then verify that without --refetch\n+# applying a new filter does not refetch missing large objects. Then use\n+# --refetch to apply the new filter on existing commits. Test it under both\n+# protocol v2 & v0.\n+test_expect_success 'apply a different filter using --refetch' '\n+\tgit -C pc1 fetch --filter=blob:limit=999 origin &&\n+\tgit -C pc1 rev-list --quiet --objects --missing=print \\\n+\t\tmain..origin/main >observed &&\n+\ttest_line_count = 4 observed &&\n+\n+\tgit -C pc1 fetch --filter=blob:limit=19999 --refetch origin &&\n+\tgit -C pc1 rev-list --quiet --objects --missing=print \\\n+\t\tmain..origin/main >observed &&\n+\ttest_line_count = 2 observed &&\n+\n+\tgit -c protocol.version=0 -C pc1 fetch --filter=blob:limit=29999 \\\n+\t\t--refetch origin &&\n+\tgit -C pc1 rev-list --quiet --objects --missing=print \\\n+\t\tmain..origin/main >observed &&\n+\ttest_line_count = 0 observed\n+'\n+\n+test_expect_success 'fetch --refetch works with a shallow clone' '\n+\tgit clone --no-checkout --depth=1 --filter=blob:none \"file://$(pwd)/srv.bare\" pc1s &&\n+\tgit -C pc1s rev-list --objects --missing=print HEAD >observed &&\n+\ttest_line_count = 6 observed &&\n+\n+\tGIT_TRACE=1 git -C pc1s fetch --filter=blob:limit=999 --refetch origin &&\n+\tgit -C pc1s rev-list --objects --missing=print HEAD >observed &&\n+\ttest_line_count = 6 observed\n+'\n+\n test_expect_success 'partial clone with transfer.fsckobjects=1 works with submodules' '\n \ttest_create_repo submodule &&\n \ttest_commit -C submodule mycommit &&\n@@ -225,7 +275,7 @@ test_expect_success 'use fsck before and after manually fetching a missing subtr\n \n \t# Auto-fetch all remaining trees and blobs with --missing=error\n \tgit -C dst rev-list --missing=error --objects main >fetched_objects &&\n-\ttest_line_count = 70 fetched_objects &&\n+\ttest_line_count = 88 fetched_objects &&\n \n \tawk -f print_1.awk fetched_objects |\n \txargs -n1 git -C dst cat-file -t >fetched_types &&\n-- \ngitgitgadget\n\n"},{"id":"452473","messageId":"78501bbf28105ef252976266abb437a278585609.1648476132.git.gitgitgadget@gmail.com","threadId":"57348","inReplyTo":"pull.1138.v4.git.1648476131.gitgitgadget@gmail.com","subject":"[PATCH v4 4/7] fetch: add --refetch option","fromName":"Robert Coup via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-03-28T14:02:08Z","receivedAt":"2022-03-28T14:02:33Z","isPatch":true,"sender":{"key":"robert@coup.net.nz","avatar":"https://avatars.githubusercontent.com/u/59874?v=4"},"body":"From: Robert Coup \n\nTeach fetch and transports the --refetch option to force a full fetch\nwithout negotiating common commits with the remote. Use when applying a\nnew partial clone filter to refetch all matching objects.\n\nSigned-off-by: Robert Coup \n---\n Documentation/fetch-options.txt | 9 +++++++++\n builtin/fetch.c | 15 ++++++++++++++-\n transport-helper.c | 3 +++\n transport.c | 4 ++++\n transport.h | 4 ++++\n 5 files changed, 34 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/fetch-options.txt b/Documentation/fetch-options.txt\nindex 6cdd9d43c5a..d03fce5aae0 100644\n--- a/Documentation/fetch-options.txt\n+++ b/Documentation/fetch-options.txt\n@@ -163,6 +163,15 @@ endif::git-pull[]\n \tbehavior for a remote may be specified with the remote..tagOpt\n \tsetting. See linkgit:git-config[1].\n \n+ifndef::git-pull[]\n+--refetch::\n+\tInstead of negotiating with the server to avoid transferring commits and\n+\tassociated objects that are already present locally, this option fetches\n+\tall objects as a fresh clone would. Use this to reapply a partial clone\n+\tfilter from configuration or using `--filter=` when the filter\n+\tdefinition has changed.\n+endif::git-pull[]\n+\n --refmap=::\n \tWhen fetching refs listed on the command line, use the\n \tspecified refspec (can be given more than once) to map the\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 9b4018f62c4..e391a5dbc55 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -59,7 +59,7 @@ static int prune_tags = -1; /* unspecified */\n \n static int all, append, dry_run, force, keep, multiple, update_head_ok;\n static int write_fetch_head = 1;\n-static int verbosity, deepen_relative, set_upstream;\n+static int verbosity, deepen_relative, set_upstream, refetch;\n static int progress = -1;\n static int enable_auto_gc = 1;\n static int tags = TAGS_DEFAULT, unshallow, update_shallow, deepen;\n@@ -190,6 +190,9 @@ static struct option builtin_fetch_options[] = {\n \tOPT_SET_INT_F(0, \"unshallow\", &unshallow,\n \t\t N_(\"convert to a complete repository\"),\n \t\t 1, PARSE_OPT_NONEG),\n+\tOPT_SET_INT_F(0, \"refetch\", &refetch,\n+\t\t N_(\"re-fetch without negotiating common commits\"),\n+\t\t 1, PARSE_OPT_NONEG),\n \t{ OPTION_STRING, 0, \"submodule-prefix\", &submodule_prefix, N_(\"dir\"),\n \t\t N_(\"prepend this to submodule path output\"), PARSE_OPT_HIDDEN },\n \tOPT_CALLBACK_F(0, \"recurse-submodules-default\",\n@@ -1304,6 +1307,14 @@ static int check_exist_and_connected(struct ref *ref_map)\n \tif (deepen)\n \t\treturn -1;\n \n+\t/*\n+\t * Similarly, if we need to refetch, we always want to perform a full\n+\t * fetch ignoring existing objects.\n+\t */\n+\tif (refetch)\n+\t\treturn -1;\n+\n+\n \t/*\n \t * check_connected() allows objects to merely be promised, but\n \t * we need all direct targets to exist.\n@@ -1517,6 +1528,8 @@ static struct transport *prepare_transport(struct remote *remote, int deepen)\n \t\tset_option(transport, TRANS_OPT_DEEPEN_RELATIVE, \"yes\");\n \tif (update_shallow)\n \t\tset_option(transport, TRANS_OPT_UPDATE_SHALLOW, \"yes\");\n+\tif (refetch)\n+\t\tset_option(transport, TRANS_OPT_REFETCH, \"yes\");\n \tif (filter_options.choice) {\n \t\tconst char *spec =\n \t\t\texpand_list_objects_filter_spec(&filter_options);\ndiff --git a/transport-helper.c b/transport-helper.c\nindex a0297b0986c..b4dbbabb0c2 100644\n--- a/transport-helper.c\n+++ b/transport-helper.c\n@@ -715,6 +715,9 @@ static int fetch_refs(struct transport *transport,\n \tif (data->transport_options.update_shallow)\n \t\tset_helper_option(transport, \"update-shallow\", \"true\");\n \n+\tif (data->transport_options.refetch)\n+\t\tset_helper_option(transport, \"refetch\", \"true\");\n+\n \tif (data->transport_options.filter_options.choice) {\n \t\tconst char *spec = expand_list_objects_filter_spec(\n \t\t\t&data->transport_options.filter_options);\ndiff --git a/transport.c b/transport.c\nindex 70e9840a90e..3d64a43ab39 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -250,6 +250,9 @@ static int set_git_option(struct git_transport_options *opts,\n \t\tlist_objects_filter_die_if_populated(&opts->filter_options);\n \t\tparse_list_objects_filter(&opts->filter_options, value);\n \t\treturn 0;\n+\t} else if (!strcmp(name, TRANS_OPT_REFETCH)) {\n+\t\topts->refetch = !!value;\n+\t\treturn 0;\n \t} else if (!strcmp(name, TRANS_OPT_REJECT_SHALLOW)) {\n \t\topts->reject_shallow = !!value;\n \t\treturn 0;\n@@ -384,6 +387,7 @@ static int fetch_refs_via_pack(struct transport *transport,\n \targs.update_shallow = data->options.update_shallow;\n \targs.from_promisor = data->options.from_promisor;\n \targs.filter_options = data->options.filter_options;\n+\targs.refetch = data->options.refetch;\n \targs.stateless_rpc = transport->stateless_rpc;\n \targs.server_options = transport->server_options;\n \targs.negotiation_tips = data->options.negotiation_tips;\ndiff --git a/transport.h b/transport.h\nindex a0bc6a1e9eb..12bc08fc339 100644\n--- a/transport.h\n+++ b/transport.h\n@@ -16,6 +16,7 @@ struct git_transport_options {\n \tunsigned update_shallow : 1;\n \tunsigned reject_shallow : 1;\n \tunsigned deepen_relative : 1;\n+\tunsigned refetch : 1;\n \n \t/* see documentation of corresponding flag in fetch-pack.h */\n \tunsigned from_promisor : 1;\n@@ -216,6 +217,9 @@ void transport_check_allowed(const char *type);\n /* Filter objects for partial clone and fetch */\n #define TRANS_OPT_LIST_OBJECTS_FILTER \"filter\"\n \n+/* Refetch all objects without negotiating */\n+#define TRANS_OPT_REFETCH \"refetch\"\n+\n /* Request atomic (all-or-nothing) updates when pushing */\n #define TRANS_OPT_ATOMIC \"atomic\"\n \n-- \ngitgitgadget\n\n"},{"id":"452474","messageId":"28c07219fd830196af1171320b86bc2a58ba3d79.1648476132.git.gitgitgadget@gmail.com","threadId":"57348","inReplyTo":"pull.1138.v4.git.1648476131.gitgitgadget@gmail.com","subject":"[PATCH v4 6/7] fetch: after refetch, encourage auto gc repacking","fromName":"Robert Coup via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-03-28T14:02:10Z","receivedAt":"2022-03-28T14:02:35Z","isPatch":true,"sender":{"key":"robert@coup.net.nz","avatar":"https://avatars.githubusercontent.com/u/59874?v=4"},"body":"From: Robert Coup \n\nAfter invoking `fetch --refetch`, the object db will likely contain many\nduplicate objects. If auto-maintenance is enabled, invoke it with\nappropriate settings to encourage repacking/consolidation.\n\n* gc.autoPackLimit: unless this is set to 0 (disabled), override the\n value to 1 to force pack consolidation.\n* maintenance.incremental-repack.auto: unless this is set to 0, override\n the value to -1 to force incremental repacking.\n\nSigned-off-by: Robert Coup \n---\n Documentation/fetch-options.txt | 3 ++-\n builtin/fetch.c | 19 ++++++++++++++++++-\n t/t5616-partial-clone.sh | 29 +++++++++++++++++++++++++++++\n 3 files changed, 49 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/fetch-options.txt b/Documentation/fetch-options.txt\nindex d03fce5aae0..622bd84768b 100644\n--- a/Documentation/fetch-options.txt\n+++ b/Documentation/fetch-options.txt\n@@ -169,7 +169,8 @@ ifndef::git-pull[]\n \tassociated objects that are already present locally, this option fetches\n \tall objects as a fresh clone would. Use this to reapply a partial clone\n \tfilter from configuration or using `--filter=` when the filter\n-\tdefinition has changed.\n+\tdefinition has changed. Automatic post-fetch maintenance will perform\n+\tobject database pack consolidation to remove any duplicate objects.\n endif::git-pull[]\n \n --refmap=::\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex e391a5dbc55..e3791f09ed5 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -2306,8 +2306,25 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n \t\t\t\t\t NULL);\n \t}\n \n-\tif (enable_auto_gc)\n+\tif (enable_auto_gc) {\n+\t\tif (refetch) {\n+\t\t\t/*\n+\t\t\t * Hint auto-maintenance strongly to encourage repacking,\n+\t\t\t * but respect config settings disabling it.\n+\t\t\t */\n+\t\t\tint opt_val;\n+\t\t\tif (git_config_get_int(\"gc.autopacklimit\", &opt_val))\n+\t\t\t\topt_val = -1;\n+\t\t\tif (opt_val != 0)\n+\t\t\t\tgit_config_push_parameter(\"gc.autoPackLimit=1\");\n+\n+\t\t\tif (git_config_get_int(\"maintenance.incremental-repack.auto\", &opt_val))\n+\t\t\t\topt_val = -1;\n+\t\t\tif (opt_val != 0)\n+\t\t\t\tgit_config_push_parameter(\"maintenance.incremental-repack.auto=-1\");\n+\t\t}\n \t\trun_auto_maintenance(verbosity < 0);\n+\t}\n \n cleanup:\n \tstring_list_clear(&list, 0);\ndiff --git a/t/t5616-partial-clone.sh b/t/t5616-partial-clone.sh\nindex 87ebf4b0b1c..4a3778d04a8 100755\n--- a/t/t5616-partial-clone.sh\n+++ b/t/t5616-partial-clone.sh\n@@ -216,6 +216,35 @@ test_expect_success 'fetch --refetch works with a shallow clone' '\n \ttest_line_count = 6 observed\n '\n \n+test_expect_success 'fetch --refetch triggers repacking' '\n+\tGIT_TRACE2_CONFIG_PARAMS=gc.autoPackLimit,maintenance.incremental-repack.auto &&\n+\texport GIT_TRACE2_CONFIG_PARAMS &&\n+\n+\tGIT_TRACE2_EVENT=\"$PWD/trace1.event\" \\\n+\tgit -C pc1 fetch --refetch origin &&\n+\ttest_subcommand git maintenance run --auto --no-quiet \n\nDocument it for partial clones as a means to apply a new filter, and\nreference it from the remote..partialclonefilter config parameter.\n\nSigned-off-by: Robert Coup \n---\n Documentation/config/remote.txt | 6 ++++--\n Documentation/technical/partial-clone.txt | 3 +++\n 2 files changed, 7 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/config/remote.txt b/Documentation/config/remote.txt\nindex a8e6437a903..0678b4bcfef 100644\n--- a/Documentation/config/remote.txt\n+++ b/Documentation/config/remote.txt\n@@ -82,5 +82,7 @@ remote..promisor::\n \tobjects.\n \n remote..partialclonefilter::\n-\tThe filter that will be applied when fetching from this\n-\tpromisor remote.\n+\tThe filter that will be applied when fetching from this\tpromisor remote.\n+\tChanging or clearing this value will only affect fetches for new commits.\n+\tTo fetch associated objects for commits already present in the local object\n+\tdatabase, use the `--refetch` option of linkgit:git-fetch[1].\ndiff --git a/Documentation/technical/partial-clone.txt b/Documentation/technical/partial-clone.txt\nindex a0dd7c66f24..99f0eb30406 100644\n--- a/Documentation/technical/partial-clone.txt\n+++ b/Documentation/technical/partial-clone.txt\n@@ -181,6 +181,9 @@ Fetching Missing Objects\n currently fetches all objects referred to by the requested objects, even\n though they are not necessary.\n \n+- Fetching with `--refetch` will request a complete new filtered packfile from\n+ the remote, which can be used to change a filter without needing to\n+ dynamically fetch missing objects.\n \n Using many promisor remotes\n ---------------------------\n-- \ngitgitgadget\n"},{"id":"452488","messageId":"xmqqee2m2d0y.fsf@gitster.g","threadId":"57348","inReplyTo":"da1e6de7a9f87c4d5a1970fba17380c3ea7b76ca.1648476132.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v4 7/7] docs: mention --refetch fetch option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-03-28T17:38:21Z","receivedAt":"2022-03-28T17:38:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Robert Coup via GitGitGadget\" writes:\n\n> remote..partialclonefilter::\n> -\tThe filter that will be applied when fetching from this\n> -\tpromisor remote.\n> +\tThe filter that will be applied when fetching from this\tpromisor remote.\n> +\tChanging or clearing this value will only affect fetches for new commits.\n> +\tTo fetch associated objects for commits already present in the local object\n> +\tdatabase, use the `--refetch` option of linkgit:git-fetch[1].\n\nGood advice to add.\n\nWill replace. I think we've seen this topic enough times and it\nlooked reasonably well done. Let's mark it for 'next' unless we\nhear objections soonish.\n"},{"id":"452795","messageId":"220331.86o81mp2w1.gmgdl@evledraar.gmail.com","threadId":"57348","inReplyTo":"03f0de3d28c00cbfda73f31a21e0fa8f8fe51742.1648476132.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v4 2/7] fetch-pack: add refetch","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-03-31T15:09:19Z","receivedAt":"2022-03-31T15:17:56Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Mar 28 2022, Robert Coup via GitGitGadget wrote:\n\n> From: Robert Coup \n>\n> Allow a \"refetch\" where the contents of the local object store are\n> ignored and a full fetch is performed, not attempting to find or\n> negotiate common commits with the remote.\n>\n> A key use case is to apply a new partial clone blob/tree filter and\n> refetch all the associated matching content, which would otherwise not\n> be transferred when the commit objects are already present locally.\n\nFWIW it's not clear to me re earlier comments on earlier iterations\nwhether the \"noop fetch\" is per-se wanted for this feature for some\nreason, or if it's just much easier to implement than doing what I\nsuggested in:\nhttps://lore.kernel.org/git/220228.86ee3m39jf.gmgdl@evledraar.gmail.com/\n\nI don't think such a thing should hold this series up, but as it would\nbe a bit kinder to servers I think it's worth at least noting in the\ncommit message what's desired per-se here, v.s. what's just needed for\nthe convenience of implementation.\n\nI.e. when this series was in an earlier iteration the scope was to\nrepair repository corruption, which I pointed out to you it really\ncouldn't do without more wider changes to the object store management,\nand at that point having it be NOOP definitely makes sense. The object\nlookups etc. take shortcuts that \"fsck\" wouldn't do, so we could be\nnegotiating on the basis of corrupt content.\n\nBut now that it's a \"fetch what's missing\" wouldn't it make more sense\nto descend from our otherwise-negotiated tips, and find the OIDs that\nare \"complete\", if any, and negotiate with those?\n\nWhich again, I think it's fine to say \"yeah, that would be ideal, but\nthis is easier\". I'm just checking if I'm missing some subtlety here...\n\n> Signed-off-by: Robert Coup \n> ---\n> fetch-pack.c | 46 +++++++++++++++++++++++++++++-----------------\n> fetch-pack.h | 1 +\n> 2 files changed, 30 insertions(+), 17 deletions(-)\n>\n> diff --git a/fetch-pack.c b/fetch-pack.c\n> index 87657907e78..4e1e88eea09 100644\n> --- a/fetch-pack.c\n> +++ b/fetch-pack.c\n> @@ -312,19 +312,21 @@ static int find_common(struct fetch_negotiator *negotiator,\n> \t\tconst char *remote_hex;\n> \t\tstruct object *o;\n> \n> -\t\t/*\n> -\t\t * If that object is complete (i.e. it is an ancestor of a\n> -\t\t * local ref), we tell them we have it but do not have to\n> -\t\t * tell them about its ancestors, which they already know\n> -\t\t * about.\n> -\t\t *\n> -\t\t * We use lookup_object here because we are only\n> -\t\t * interested in the case we *know* the object is\n> -\t\t * reachable and we have already scanned it.\n> -\t\t */\n> -\t\tif (((o = lookup_object(the_repository, remote)) != NULL) &&\n> -\t\t\t\t(o->flags & COMPLETE)) {\n> -\t\t\tcontinue;\n> +\t\tif (!args->refetch) {\n> +\t\t\t/*\n> +\t\t\t* If that object is complete (i.e. it is an ancestor of a\n> +\t\t\t* local ref), we tell them we have it but do not have to\n> +\t\t\t* tell them about its ancestors, which they already know\n> +\t\t\t* about.\n> +\t\t\t*\n> +\t\t\t* We use lookup_object here because we are only\n> +\t\t\t* interested in the case we *know* the object is\n> +\t\t\t* reachable and we have already scanned it.\n> +\t\t\t*/\n> +\t\t\tif (((o = lookup_object(the_repository, remote)) != NULL) &&\n> +\t\t\t\t\t(o->flags & COMPLETE)) {\n> +\t\t\t\tcontinue;\n> +\t\t\t}\n\nnit: remove the {} here\nnit: this double-indented if can just be one if-statement\nnit: don't compare against NULL, use ! instead.\n> \t\t}\n> \n> \t\tremote_hex = oid_to_hex(remote);\n> @@ -692,6 +694,9 @@ static void mark_complete_and_common_ref(struct fetch_negotiator *negotiator,\n> \tint old_save_commit_buffer = save_commit_buffer;\n> \ttimestamp_t cutoff = 0;\n> \n> +\tif (args->refetch)\n> +\t\treturn;\n> +\n> \tsave_commit_buffer = 0;\n\nnit: This function has only two callers, perhaps it's clearer to do do\nthis \"early abort\" in those calls?\n\n> \ttrace2_region_enter(\"fetch-pack\", \"parse_remote_refs_and_find_cutoff\", NULL);\n> @@ -1028,7 +1033,11 @@ static struct ref *do_fetch_pack(struct fetch_pack_args *args,\n> \tstruct fetch_negotiator *negotiator;\n> \n> \tnegotiator = &negotiator_alloc;\n> -\tfetch_negotiator_init(r, negotiator);\n> +\tif (args->refetch) {\n> +\t\tfetch_negotiator_init_noop(negotiator);\n> +\t} else {\n> +\t\tfetch_negotiator_init(r, negotiator);\n> +\t}\n\nMore needless {}\n\n> \tsort_ref_list(&ref, ref_compare_name);\n> \tQSORT(sought, nr_sought, cmp_ref_by_name);\n> @@ -1121,7 +1130,7 @@ static struct ref *do_fetch_pack(struct fetch_pack_args *args,\n> \n> \tmark_complete_and_common_ref(negotiator, args, &ref);\n> \tfilter_refs(args, &ref, sought, nr_sought);\n> -\tif (everything_local(args, &ref)) {\n> +\tif (!args->refetch && everything_local(args, &ref)) {\n> \t\tpacket_flush(fd[1]);\n> \t\tgoto all_done;\n> \t}\n\nHere everything_local() is doing what I suggested for\nmark_complete_and_common_ref() above, i.e. we check args->refetch first.\n\n> @@ -1587,7 +1596,10 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,\n> \tstruct strvec index_pack_args = STRVEC_INIT;\n> \n> \tnegotiator = &negotiator_alloc;\n> -\tfetch_negotiator_init(r, negotiator);\n> +\tif (args->refetch)\n> +\t\tfetch_negotiator_init_noop(negotiator);\n> +\telse\n> +\t\tfetch_negotiator_init(r, negotiator);\n\nThis one doesn't have braces (good), unlike do_fetch_pack()\n"},{"id":"452796","messageId":"220331.86k0cap2rc.gmgdl@evledraar.gmail.com","threadId":"57348","inReplyTo":"78501bbf28105ef252976266abb437a278585609.1648476132.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v4 4/7] fetch: add --refetch option","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-03-31T15:18:36Z","receivedAt":"2022-03-31T15:20:44Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Mar 28 2022, Robert Coup via GitGitGadget wrote:\n\n> From: Robert Coup \n>\n> Teach fetch and transports the --refetch option to force a full fetch\n> without negotiating common commits with the remote. Use when applying a\n> new partial clone filter to refetch all matching objects.\n>\n> [...]\n> +ifndef::git-pull[]\n> +--refetch::\n> +\tInstead of negotiating with the server to avoid transferring commits and\n> +\tassociated objects that are already present locally, this option fetches\n> +\tall objects as a fresh clone would. Use this to reapply a partial clone\n> +\tfilter from configuration or using `--filter=` when the filter\n> +\tdefinition has changed.\n> +endif::git-pull[]\n\nRe my comment on negotiation specifics in 2/7, this documentation is\nreally over-promising depending on what the answer to that is:\nhttps://lore.kernel.org/git/220331.86o81mp2w1.gmgdl@evledraar.gmail.com/\n\nI.e. instead of saying that we WILL fetch all objects \"just like a\nclone\" shouldn't we have less focus on implementation details here, and\nassure the user that we'll make their object store \"complete\" as though\n--filter hadn't been used, without going into the specifics of what'll\nhappen over the wire?\n> \t\treturn -1;\n> \n> +\t/*\n> +\t * Similarly, if we need to refetch, we always want to perform a full\n> +\t * fetch ignoring existing objects.\n> +\t */\n> +\tif (refetch)\n> +\t\treturn -1;\n> +\n> +\n\nOne too many \\n here.\n"},{"id":"452797","messageId":"220331.86fsmyp2o0.gmgdl@evledraar.gmail.com","threadId":"57348","inReplyTo":"6c17167ac1edbeea5f6d375a989aa49f5748d099.1648476132.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v4 5/7] t5615-partial-clone: add test for fetch --refetch","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-03-31T15:20:45Z","receivedAt":"2022-03-31T15:22:44Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Mar 28 2022, Robert Coup via GitGitGadget wrote:\n\n> From: Robert Coup \n>\n> Add a test for doing a refetch to apply a changed partial clone filter\n> under protocol v0 and v2.\n>\n> Signed-off-by: Robert Coup \n> ---\n> t/t5616-partial-clone.sh | 52 +++++++++++++++++++++++++++++++++++++++-\n> 1 file changed, 51 insertions(+), 1 deletion(-)\n>\n> diff --git a/t/t5616-partial-clone.sh b/t/t5616-partial-clone.sh\n> index 34469b6ac10..87ebf4b0b1c 100755\n> --- a/t/t5616-partial-clone.sh\n> +++ b/t/t5616-partial-clone.sh\n> @@ -166,6 +166,56 @@ test_expect_success 'manual prefetch of missing objects' '\n> \ttest_line_count = 0 observed.oids\n> '\n> \n> +# create new commits in \"src\" repo to establish a history on file.4.txt\n> +# and push to \"srv.bare\".\n> +test_expect_success 'push new commits to server for file.4.txt' '\n> +\tfor x in a b c d e f\n> +\tdo\n> +\t\techo \"Mod file.4.txt $x\" >src/file.4.txt &&\n> +\t\tif list_contains \"a,b\" \"$x\"; then\n> +\t\t\tprintf \"%10000s\" X >>src/file.4.txt\n> +\t\tfi &&\n> +\t\tif list_contains \"c,d\" \"$x\"; then\n> +\t\t\tprintf \"%20000s\" X >>src/file.4.txt\n> +\t\tfi &&\n> +\t\tgit -C src add file.4.txt &&\n> +\t\tgit -C src commit -m \"mod $x\" || return 1\n> +\tdone &&\n> +\tgit -C src push -u srv main\n> +'\n> +\n> +# Do partial fetch to fetch smaller files; then verify that without --refetch\n> +# applying a new filter does not refetch missing large objects. Then use\n> +# --refetch to apply the new filter on existing commits. Test it under both\n> +# protocol v2 & v0.\n> +test_expect_success 'apply a different filter using --refetch' '\n> +\tgit -C pc1 fetch --filter=blob:limit=999 origin &&\n> +\tgit -C pc1 rev-list --quiet --objects --missing=print \\\n> +\t\tmain..origin/main >observed &&\n> +\ttest_line_count = 4 observed &&\n> +\n> +\tgit -C pc1 fetch --filter=blob:limit=19999 --refetch origin &&\n\nIs 19999 just \"arbitrary big number\" here?\n\n> +\tgit -C pc1 rev-list --quiet --objects --missing=print \\\n> +\t\tmain..origin/main >observed &&\n> +\ttest_line_count = 2 observed &&\n> +\n> +\tgit -c protocol.version=0 -C pc1 fetch --filter=blob:limit=29999 \\\n> +\t\t--refetch origin &&\n> +\tgit -C pc1 rev-list --quiet --objects --missing=print \\\n> +\t\tmain..origin/main >observed &&\n> +\ttest_line_count = 0 observed\n\nDoes this test_line_count *really* want to be = 0, or does this mean\ntest_must_be_empty?\n\nI.e. are we expecting content here, just not ending in a \\n, or nothing\nat all?\n\n> +'\n> +\n> +test_expect_success 'fetch --refetch works with a shallow clone' '\n> +\tgit clone --no-checkout --depth=1 --filter=blob:none \"file://$(pwd)/srv.bare\" pc1s &&\n> +\tgit -C pc1s rev-list --objects --missing=print HEAD >observed &&\n> +\ttest_line_count = 6 observed &&\n> +\n> +\tGIT_TRACE=1 git -C pc1s fetch --filter=blob:limit=999 --refetch origin &&\n\nWhy the GIT_TRACE=1? Seems to not be used.\n"},{"id":"452798","messageId":"220331.86bkxmp25k.gmgdl@evledraar.gmail.com","threadId":"57348","inReplyTo":"28c07219fd830196af1171320b86bc2a58ba3d79.1648476132.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v4 6/7] fetch: after refetch, encourage auto gc repacking","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-03-31T15:22:52Z","receivedAt":"2022-03-31T15:39:17Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Mar 28 2022, Robert Coup via GitGitGadget wrote:\n\n> From: Robert Coup \n>\n> After invoking `fetch --refetch`, the object db will likely contain many\n> duplicate objects. If auto-maintenance is enabled, invoke it with\n> appropriate settings to encourage repacking/consolidation.\n>\n> * gc.autoPackLimit: unless this is set to 0 (disabled), override the\n> value to 1 to force pack consolidation.\n> * maintenance.incremental-repack.auto: unless this is set to 0, override\n> the value to -1 to force incremental repacking.\n>\n> Signed-off-by: Robert Coup \n> ---\n> Documentation/fetch-options.txt | 3 ++-\n> builtin/fetch.c | 19 ++++++++++++++++++-\n> t/t5616-partial-clone.sh | 29 +++++++++++++++++++++++++++++\n> 3 files changed, 49 insertions(+), 2 deletions(-)\n>\n> diff --git a/Documentation/fetch-options.txt b/Documentation/fetch-options.txt\n> index d03fce5aae0..622bd84768b 100644\n> --- a/Documentation/fetch-options.txt\n> +++ b/Documentation/fetch-options.txt\n> @@ -169,7 +169,8 @@ ifndef::git-pull[]\n> \tassociated objects that are already present locally, this option fetches\n> \tall objects as a fresh clone would. Use this to reapply a partial clone\n> \tfilter from configuration or using `--filter=` when the filter\n> -\tdefinition has changed.\n> +\tdefinition has changed. Automatic post-fetch maintenance will perform\n> +\tobject database pack consolidation to remove any duplicate objects.\n> endif::git-pull[]\n> \n> --refmap=::\n> diff --git a/builtin/fetch.c b/builtin/fetch.c\n> index e391a5dbc55..e3791f09ed5 100644\n> --- a/builtin/fetch.c\n> +++ b/builtin/fetch.c\n> @@ -2306,8 +2306,25 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n> \t\t\t\t\t NULL);\n> \t}\n> \n> -\tif (enable_auto_gc)\n> +\tif (enable_auto_gc) {\n> +\t\tif (refetch) {\n> +\t\t\t/*\n> +\t\t\t * Hint auto-maintenance strongly to encourage repacking,\n> +\t\t\t * but respect config settings disabling it.\n> +\t\t\t */\n> +\t\t\tint opt_val;\n\nnit: add a \\n after this.\n\n> +\t\t\tif (git_config_get_int(\"gc.autopacklimit\", &opt_val))\n> +\t\t\t\topt_val = -1;\n> +\t\t\tif (opt_val != 0)\n\nnit: don't compare against 0 or null, just !opt_val\n\nIsn't this whole thing also clearer as:\n\n\tint &forget;\n\n if (git_conf...(..., &forget))\n\t\tgit_config_push_parameter(\"gc.autoPackLimit=1\");\n\nMaybe I haven't eyeballed this enough, but aren't you ignoring explicit\ngc.autoPackLimit=0 configuration? Whereas what you seem to want is \"set\nthis config unlress the user has it set\", for which we only need to\ncheck the git_config...(...) return value, no?\n\n> +\t\t\t\tgit_config_push_parameter(\"gc.autoPackLimit=1\");\n> +\n> +\t\t\tif (git_config_get_int(\"maintenance.incremental-repack.auto\", &opt_val))\n> +\t\t\t\topt_val = -1;\n> +\t\t\tif (opt_val != 0)\n> +\t\t\t\tgit_config_push_parameter(\"maintenance.incremental-repack.auto=-1\");\n\nhrm, do we really need to set both of these these days (not saying we\ndon't, just surprised). I.e. both gc.* an maintenance.* config.\n\n*skims the code*\n\nUrgh, yes? too_many_packs() seems to check gc.* only, but\nincremental_repack_auto_condition() check this variable... :(\n\n> +test_expect_success 'fetch --refetch triggers repacking' '\n> +\tGIT_TRACE2_CONFIG_PARAMS=gc.autoPackLimit,maintenance.incremental-repack.auto &&\n\nNit: Can we use GIT_CONFIG_KEY_* et al for this these days, or do we\nstill need this trace2 thingy?\n\n> +\texport GIT_TRACE2_CONFIG_PARAMS &&\n> +\n"},{"id":"452860","messageId":"CACf-nVdahRDwtbYFWAbJQ-f9XHDB3rJXYKcqzuURQUePCb-AjA@mail.gmail.com","threadId":"57348","inReplyTo":"220331.86o81mp2w1.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH v4 2/7] fetch-pack: add refetch","fromName":"Robert Coup","fromEmail":"robert@coup.net.nz","sentAt":"2022-04-01T10:26:30Z","receivedAt":"2022-04-01T10:26:47Z","isPatch":true,"sender":{"key":"robert@coup.net.nz","avatar":"https://avatars.githubusercontent.com/u/59874?v=4"},"body":"Hi Ævar,\n\n< just when I was thinking I'm done... ;-) >\n\nOn Thu, 31 Mar 2022 at 16:17, Ævar Arnfjörð Bjarmason wrote:\n>\n> FWIW it's not clear to me re earlier comments on earlier iterations\n> whether the \"noop fetch\" is per-se wanted for this feature for some\n> reason, or if it's just much easier to implement than doing what I\n> suggested in:\n> https://lore.kernel.org/git/220228.86ee3m39jf.gmgdl@evledraar.gmail.com/\n\nThe noop-fetch is an implementation detail IMO, and can be improved\nif/when someone is motivated later.\n\n> I don't think such a thing should hold this series up, but as it would\n> be a bit kinder to servers I think it's worth at least noting in the\n> commit message what's desired per-se here, v.s. what's just needed for\n> the convenience of implementation.\n\nYes, doing some per-commit negotiation is conceptually nicer. The\nuser-facing alternative at this point is basically running \"clone\" in\nsome form; or temporarily moving aside the object DB and running\nfetch. Both those (and the new approach) all end up putting the same\nload on the server.\n\nI can note it in the commit message.\n\n> I.e. when this series was in an earlier iteration the scope was to\n> repair repository corruption\n\nThat was never _my_ motivation, I was a bit eager in reflecting some\ncomments I received suggesting it would be useful for that as well.\n\n> But now that it's a \"fetch what's missing\" wouldn't it make more sense\n> to descend from our otherwise-negotiated tips, and find the OIDs that\n> are \"complete\", if any, and negotiate with those?\n>\n> Which again, I think it's fine to say \"yeah, that would be ideal, but\n> this is easier\". I'm just checking if I'm missing some subtlety here...\n\nYeah, that would be ideal, but this is easier. AFAICS I'm not putting\nin place any barriers to making it smarter later.\n\n> nit: remove the {} here\n> nit: this double-indented if can just be one if-statement\n> nit: don't compare against NULL, use ! instead.\n\nIs the common interpretation for nits \"Do another re-roll\"; \"If you're\ndoing another re-roll\"; or \"For future reference\"?\n\n> nit: This function has only two callers, perhaps it's clearer to do do\n> this \"early abort\" in those calls?\n\nI discussed similar in\nhttps://lore.kernel.org/git/CACf-nVePhtm_HAzAKzcap0E8kiyyEJPY_+N+bbPcYPVUkjweFg@mail.gmail.com/\nbut yes, I think you're right about mark_complete_and_common_ref().\nWill see what it looks like.\n\nThanks, Rob.\n"},{"id":"452862","messageId":"CACf-nVcwWCaJS7x5Qri+ZUucQKTTyL75=WscTEZkjg014+zMhw@mail.gmail.com","threadId":"57348","inReplyTo":"220331.86k0cap2rc.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH v4 4/7] fetch: add --refetch option","fromName":"Robert Coup","fromEmail":"robert@coup.net.nz","sentAt":"2022-04-01T10:31:56Z","receivedAt":"2022-04-01T10:32:12Z","isPatch":true,"sender":{"key":"robert@coup.net.nz","avatar":"https://avatars.githubusercontent.com/u/59874?v=4"},"body":"Hi Ævar,\n\nOn Thu, 31 Mar 2022 at 16:20, Ævar Arnfjörð Bjarmason wrote:\n> > +--refetch::\n> > + Instead of negotiating with the server to avoid transferring commits and\n> > + associated objects that are already present locally, this option fetches\n> > + all objects as a fresh clone would. Use this to reapply a partial clone\n> > + filter from configuration or using `--filter=` when the filter\n> > + definition has changed.\n>\n> Re my comment on negotiation specifics in 2/7, this documentation is\n> really over-promising depending on what the answer to that is:\n> https://lore.kernel.org/git/220331.86o81mp2w1.gmgdl@evledraar.gmail.com/\n>\n> I.e. instead of saying that we WILL fetch all objects \"just like a\n> clone\" shouldn't we have less focus on implementation details here, and\n> assure the user that we'll make their object store \"complete\" as though\n> --filter hadn't been used, without going into the specifics of what'll\n> happen over the wire?\n\nThat's not quite the case though: we'll make their object db complete\nas if a fresh clone with any specified --filter (including none) from\nthat remote has been done.\n\nIMO explaining what will happen and to expect (particularly as this is\n_not_ transferring some sort of object-delta-set, clone transfers can\nbe big) is a good thing. If it improves later; then change the docs.\n\nAlternatively, rewording along the lines of \"it will fix up your\nobject db to match a changed filter\" is implying that only the missing\nbits will be fetched.\n\nThanks, Rob.\n"},{"id":"452863","messageId":"CACf-nVcor-0hbnW-bbRTgXVYJ+Fjd5HkJam7Hh3C=47QwwO6xQ@mail.gmail.com","threadId":"57348","inReplyTo":"220331.86fsmyp2o0.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH v4 5/7] t5615-partial-clone: add test for fetch --refetch","fromName":"Robert Coup","fromEmail":"robert@coup.net.nz","sentAt":"2022-04-01T10:36:15Z","receivedAt":"2022-04-01T10:36:52Z","isPatch":true,"sender":{"key":"robert@coup.net.nz","avatar":"https://avatars.githubusercontent.com/u/59874?v=4"},"body":"Hi,\n\nOn Thu, 31 Mar 2022 at 16:22, Ævar Arnfjörð Bjarmason wrote:\n>\n>\n\n> > +\n> > + git -C pc1 fetch --filter=blob:limit=19999 --refetch origin &&\n>\n> Is 19999 just \"arbitrary big number\" here?\n\nA selected big number so we can change it and have different objects match.\n\n>\n> > + git -C pc1 rev-list --quiet --objects --missing=print \\\n> > + main..origin/main >observed &&\n> > + test_line_count = 2 observed &&\n> > +\n> > + git -c protocol.version=0 -C pc1 fetch --filter=blob:limit=29999 \\\n> > + --refetch origin &&\n> > + git -C pc1 rev-list --quiet --objects --missing=print \\\n> > + main..origin/main >observed &&\n> > + test_line_count = 0 observed\n>\n> Does this test_line_count *really* want to be = 0, or does this mean\n> test_must_be_empty?\n>\n> I.e. are we expecting content here, just not ending in a \\n, or nothing\n> at all?\n\nI'm expecting no missing objects after a refetch with a new filter\nthat matches all the objects in the repo.\n\n> > + GIT_TRACE=1 git -C pc1s fetch --filter=blob:limit=999 --refetch origin &&\n>\n> Why the GIT_TRACE=1? Seems to not be used.\n\nAh, extraneous debugging on my part.\n\nThanks, Rob :)\n"},{"id":"452865","messageId":"CACf-nVeTMx8y+8HzQbaTjHq06Ldon+JV+rp0yzhP_RC=6La2gg@mail.gmail.com","threadId":"57348","inReplyTo":"220331.86bkxmp25k.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH v4 6/7] fetch: after refetch, encourage auto gc repacking","fromName":"Robert Coup","fromEmail":"robert@coup.net.nz","sentAt":"2022-04-01T10:51:56Z","receivedAt":"2022-04-01T10:52:20Z","isPatch":true,"sender":{"key":"robert@coup.net.nz","avatar":"https://avatars.githubusercontent.com/u/59874?v=4"},"body":"Hi Ævar,\n\nOn Thu, 31 Mar 2022 at 16:33, Ævar Arnfjörð Bjarmason wrote:\n\n\n> > + if (git_config_get_int(\"gc.autopacklimit\", &opt_val))\n> > + opt_val = -1;\n> > + if (opt_val != 0)\n>\n> nit: don't compare against 0 or null, just !opt_val\n\nI did this since 0 has a specific meaning (\"Setting this to 0\ndisables\"), it's not just false-y in this context. Tomayto, tomahto?\n\n>\n> Isn't this whole thing also clearer as:\n>\n> int &forget;\n>\n> if (git_conf...(..., &forget))\n> git_config_push_parameter(\"gc.autoPackLimit=1\");\n>\n> Maybe I haven't eyeballed this enough, but aren't you ignoring explicit\n> gc.autoPackLimit=0 configuration? Whereas what you seem to want is \"set\n> this config unlress the user has it set\", for which we only need to\n> check the git_config...(...) return value, no?\n\nWhat I'm trying to achieve: if the user has not disabled auto-packing\n(autoPackLimit=0), then pass autoPackLimit=1 to the subprocess to\nencourage repacking.\nContext/why: so we don't 2x the object store size and not even attempt\nto repack it now, rather than at some unspecified point in the future.\nMaybe.\n\nHow the code achieves it:\n load autoPackLimit into opt_val\n if autoPackLimit is not specified in config: set opt_val to -1\n if opt_val is not 0: pass autoPackLimit=1 to the subprocess\n\nAFAICT if we just if(git_config_get_int()) then if they haven't set it\nat all in config, we wouldn't encourage repacking in the subprocess.\nWhich isn't what I'm trying to achieve.\n\n> hrm, do we really need to set both of these these days (not saying we\n> don't, just surprised). I.e. both gc.* an maintenance.* config.\n>\n> *skims the code*\n>\n> Urgh, yes? too_many_packs() seems to check gc.* only, but\n> incremental_repack_auto_condition() check this variable... :(\n\nYes.\n\n>\n> > +test_expect_success 'fetch --refetch triggers repacking' '\n> > + GIT_TRACE2_CONFIG_PARAMS=gc.autoPackLimit,maintenance.incremental-repack.auto &&\n>\n> Nit: Can we use GIT_CONFIG_KEY_* et al for this these days, or do we\n> still need this trace2 thingy?\n\nI copied a pattern existing tests are using.\n\nThanks, Rob.\n"}]}