patch, 6 parts[RFC] partial-clone: add ability to refetch with expanded filter

If a filter is changed on a partial clone repository, for example from blob:none to blob:limit=1m, there is currently no straightforward way to bulk-refetch the objects that match the new filter for existing local commits. This is because the client will report commits as "have" during negotiation and any dependent objects won't be included in the transferred pack. Another use case is discussed at [1].

This patch series proposes adding a --refilter option to fetch & fetch-pack to enable doing a full fetch with a different filter, as if the local has no commits in common with the remote. It builds upon cbe566a071 ("negotiator/noop: add noop fetch negotiator", 2020-08-18).

To note:

 1. This will produce duplicated objects between the existing and newly
    fetched packs, but gc will clean them up.
 2. This series doesn't check that there's a new filter in any way, whether
    configured via config or passed via --filter=. Personally I think that's
    fine.
 3. If a user fetches with --refilter applying a more restrictive filter
    than previously (eg: blob:limit=1m then blob:limit=1k) the eventual
    state is a no-op, since any referenced object already in the local
    repository is never removed. Potentially this could be improved in
    future by more advanced gc, possibly along the lines discussed at [2].

[1] https://public-inbox.org/git/aa7b89ee-08aa-7943-6a00-28dcf344426e@syntevo.com/ [2] https://public-inbox.org/git/A4BAD509-FA1F-49C3-87AF-CF4B73C559F1@gmail.com/

Robert Coup (6):
  fetch-negotiator: add specific noop initializor
  fetch-pack: add partial clone refiltering
  builtin/fetch-pack: add --refilter option
  fetch: add --refilter option
  t5615-partial-clone: add test for --refilter
  doc/partial-clone: mention --refilter option

 Documentation/fetch-options.txt           |  9 ++++
 Documentation/git-fetch-pack.txt          |  4 ++
 Documentation/technical/partial-clone.txt |  3 ++
 builtin/fetch-pack.c                      |  4 ++
 builtin/fetch.c                           | 18 ++++++-
 fetch-negotiator.c                        |  5 ++
 fetch-negotiator.h                        |  8 ++++
 fetch-pack.c                              | 57 +++++++++++++++--------
 fetch-pack.h                              |  1 +
 remote-curl.c                             |  6 +++
 t/t5616-partial-clone.sh                  | 42 ++++++++++++++++-
 transport-helper.c                        |  3 ++
 transport.c                               |  4 ++
 transport.h                               |  4 ++
 14 files changed, 146 insertions(+), 22 deletions(-)

base-commit: 5d01301f2b865aa8dba1654d3f447ce9d21db0b5
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1138%2Frcoup%2Frc-partial-clone-refilter-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1138/rcoup/rc-partial-clone-refilter-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1138

From: Robert Coup <robert@coup.net.nz>

Add a specific initializor for the noop fetch negotiator. This is introduced to support allowing partial clones to skip commit negotiation when refetching to apply a modified filter.

Signed-off-by: Robert Coup <robert@coup.net.nz>
---
 fetch-negotiator.c | 5 +++++
 fetch-negotiator.h | 8 ++++++++
 2 files changed, 13 insertions(+)

From: Robert Coup <robert@coup.net.nz>

Allow refetching with a new partial clone filter by not attempting to find or negotiate common commits with the remote, and always forcing a full filtered fetch.

Signed-off-by: Robert Coup <robert@coup.net.nz>
---
 fetch-pack.c | 57 ++++++++++++++++++++++++++++++++++------------------
 fetch-pack.h |  1 +
 2 files changed, 38 insertions(+), 20 deletions(-)

"Robert Coup via GitGitGadget" <gitgitgadget@gmail.com> writes:

The approach that I would have expected is to not call mark_complete_and_common_ref(), filter_refs(), everything_local(), and find_common(), but your approach here is to ensure that mark_complete_and_common_ref() and find_common() do not do anything. Comparing the two approaches, the advantage of yours is that we only need to make the change once to support both protocol v0 and v2 (although the change looks more substantial than just skipping function calls), but it makes the code more difficult to read in that we now have function calls that do nothing. What do you think about my approach?

Hi Jonathan,

Thanks for taking a look at this.

On Fri, 4 Feb 2022 at 18:02, Jonathan Tan <jonathantanmy@google.com> wrote:

v0: find_common() definitely still does something: during refiltering it skips checking the local object db, but it's still responsible for sending the "wants".

filter_refs() is necessary under v0 & v2 so the remote refs all get marked as matched, otherwise we end up erroring after the transfer with "error: no such remote ref refs/heads/main" etc.

My original approach was to leave the negotiator in place, and just conditional around it in do_fetch_pack_v2 — this worked ok for protocol v2 but the v0 implementation didn't work. After that I switched to forcing noop in [1/6] which made both implementations match (& tidier imo).

To make the test pass and skip those calls I need a patch like the below — filter_refs() is still required during refiltering for the ref-matching. To me this looks more complicated, but I'm happy to defer to your thinking.

Thanks,

Rob :)

Robert Coup <robert@coup.net.nz> writes:

Thanks for investigating this; looks like I was perhaps too optimistic in thinking that the code could be reorganized in the way I'm suggesting.

I think that it's worth checking if we could refactor the parts that are needed with --refilter out of filter_refs() and find_common() (and in doing so, make these functions do only what their names imply). I don't have time to do so right now, but I don't want to unnecessarily block this patch set either, so I'll say that ideally another reviewer (or you) would do that investigation, but I have no objection to merging this patch set in this state (other than the change of the argument name, perhaps to "--repair", and associated documentation).

From: Robert Coup <robert@coup.net.nz>

Add a --refilter option to fetch-pack to force a full fetch when applying a new filter.

Signed-off-by: Robert Coup <robert@coup.net.nz>
---
 Documentation/git-fetch-pack.txt | 4 ++++
 builtin/fetch-pack.c             | 4 ++++
 remote-curl.c                    | 6 ++++++
 3 files changed, 14 insertions(+)

From: Robert Coup <robert@coup.net.nz>

Teach fetch and transports the --refilter option to force a full fetch when applying a new filter.

Signed-off-by: Robert Coup <robert@coup.net.nz>
---
 Documentation/fetch-options.txt |  9 +++++++++
 builtin/fetch.c                 | 18 +++++++++++++++++-
 transport-helper.c              |  3 +++
 transport.c                     |  4 ++++
 transport.h                     |  4 ++++
 5 files changed, 37 insertions(+), 1 deletion(-)

From: Robert Coup <robert@coup.net.nz>

Add a test for partial clone refiltering under protocol v0 and v2.

Signed-off-by: Robert Coup <robert@coup.net.nz>
---
 t/t5616-partial-clone.sh | 42 +++++++++++++++++++++++++++++++++++++++-
 1 file changed, 41 insertions(+), 1 deletion(-)

From: Robert Coup <robert@coup.net.nz>

Add the fetch --refilter option to the partial clone documentation.

Signed-off-by: Robert Coup <robert@coup.net.nz>
---
 Documentation/technical/partial-clone.txt | 3 +++
 1 file changed, 3 insertions(+)

"Robert Coup via GitGitGadget" <gitgitgadget@gmail.com> writes:

It sounds like a useful thing to have such a "refetch things" option.

A lazy/partial clone is narrower than the full tree in the width dimension, while a shallow clone is shallower than the full history in the time dimension. The latter already has the "--deepen" support to say "the commits listed in my shallow boundary list may claim that I already have these, but I actually don't have them; please stop lying to the other side and refetch what I should have fetched earlier". I understand that this works in the other dimension to "--widen" things?

Makes me wonder how well these two features work together (or if they are mutually exclusive, that is fine as well as a starting point).

If you update the filter specification to make it narrower (e.g. you start from blob:limit=1m down to blob:limit=512k), would we transfer nothing (which would be ideal), or would we end up refetching everything that are smaller than 512k?

I guess the answer to the last question is ...

... it is not smart enough to stell them to exclude what we _ought_ to have by telling them what the _old_ filter spec was. That's OK for a starting point, I guess. Hopefully, at the end of this operation, we should garbage collect the duplicated objects by default (with an option to turn it off)?

In other words, a repository that used to be a partial clone can become a full clone by using the option _and_ not giving any filter. I think that is an intuitive enough behaviour and a natural consequence to the extreme of what the feature is. Compared to making a full "git clone", fetching from the old local (and narrow) repository into it and then discarding the old one, it would not have any performance or storage advantage, but it probably is more convenient.

OK.  That matches my reaction to 1. above.

Hi Junio

Thanks for your input. Hopefully some of the other partial-clone interested folks will chime in too.

On Tue, 1 Feb 2022 at 20:13, Junio C Hamano <gitster@pobox.com> wrote:

Any improved suggestions on the argument name? I thought of --refetch but `fetch --refetch` seemed more confusing to explain.

I don't see any particular reason they can't work together - as you say, the filtering is orthogonal to shallow on a conceptual level. I haven't added a test for that scenario yet but will do for a v1.

As you spot, the latter. I can't see a straightforward way of telling the server "I have these trees/blobs already" without generating (one way or the other) a list of millions of oids, then transferring & negotiating with it.

The client doesn't really know what the local repository *has* — potentially several filters could have been applied and used for fetches at different points in the commit history, as well as objects dynamically fetched in. Even a filter set in the config only applies to subsequent fetches, and only if --filter isn't used to override it.

I haven't explicitly looked into invoking gc yet, but yes, it'd be a bit of a waste if it wasn't kicked off by default. Maybe reusing gc.auto

For that specific case I think you can already do it by removing the promisor flag in the remote config, potentially adding it back if you wanted to keep it partial again from that point forward.

It's certainly cleaner than abusing --deepen, or temporarily moving pack files out of the way, or starting over with a fresh clone & copying config.

Thanks,

Rob :)

Hi Derrick,

On Wed, 2 Feb 2022 at 15:02, Robert Coup <robert@coup.net.nz> wrote:

Just looking into this: after a fetch, run_auto_maintenance() is called, which invokes `git maintenance run --auto` (being the successor to `git gc --auto`)...

In the refilter (repair) case, we'll have a new pack which substantially duplicates what's in our existing packs. I'm after some advice here: I'm not sure whether I want to encourage a gc pack consolidation, an incremental repack, both, neither, or something else?

My current train of thought is it invokes `git maintenance run --auto` with gc.autoPackLimit=1 to force a gc pack consolidation if it's not currently =0 (disabled), and with maintenance.incremental-repack.auto=-1 if it's not currently =0 (disabled) to force an incremental repack if someone doesn't want to do pack consolidations.

In all cases the various enabled-ness settings should still be honoured to skip tasks if the user doesn't want them ever happening automatically.

Implementation-wise I guess we'll need run_auto_maintenance() to be able to pass some config options through to the maintenance subprocess.

Does this sound like a reasonable approach?

Thanks,

Rob :)

"Robert Coup via GitGitGadget" <gitgitgadget@gmail.com> writes:

Reporting commits as "have" can be solved by forcing the noop negotiator, but there is another issue (which you seem to have discovered, glancing through your patches) in that fetch will abort (and report success) if all wanted commits are present, even if not all objects directly or indirectly referenced by those commits are present.

Thanks - I think this is a useful feature. This is useful even in a non-partial-clone repo, to repair objects that were, say, accidentally deleted from the local object store.

If it's acceptable to have a separate command to configure the new filter in the repo config (or to delete it, if we want to convert a partial clone into a regular repo), I think it's clearer to name this option "--repair" or something like that, and explain it as a fetch that does not take into account the contents of the local object store (not as a fetch that changes the filter).

Having said that, the overall feature is worth having. While we decide on the name, the implementation of this will not change much. I'll try to review the implementation by the end of this week (but other reviews are welcome too, needless to say).

Noted. This might be an argument for naming the option "--repair", since the user would probably understand that there would be duplicate objects.

Agreed.

That's true.

Hi Jonathan,

On Wed, 2 Feb 2022 at 19:00, Jonathan Tan <jonathantanmy@google.com> wrote:

I'd

I quite like --repair, since the implementation really has zero to do with filtering or partial clones beyond that being my use case for it. Specifying a filter, shallow options, or even using a promisor remote aren't even slightly necessary for the implementation as it turns out.

And as you say, that makes it easier to explain too:

"fetch --repair will fetch all objects from the remote (applying any filters or shallow options as requested). Unlike a normal fetch it does not take into account any content already in the local repository and acts more like an initial clone. Any duplicate objects will get cleaned up during subsequent maintenance."

"If you want to update your local repository with a different partial clone filter, use `fetch --repair` to re-download all matching objects from the remote."

Thanks!

Rob :)

On Wed, 2 Feb 2022 at 21:58, Robert Coup <robert@coup.net.nz> wrote:

I'd be keen to hear the stories here :) Just losing loose object files?

Rob.

On 2/1/22 10:49 AM, Robert Coup via GitGitGadget wrote:

Yes, it would be nice to have a way to efficiently extend a partial clone with a more inclusive filter.

It would be nice to be able to send the old filter-spec and the new filter-spec and ask the server to send "new && !old" to keep from having to resend the objects that the client already has. But I'm not sure we know enough (on either side) to make that computation. And as you say, there is no guarantee that the client has only used one filter in the past.

Jeff

If a filter is changed on a partial clone repository, for example from blob:none to blob:limit=1m, there is currently no straightforward way to bulk-refetch the objects that match the new filter for existing local commits. This is because the client will report commits as "have" during fetch negotiation and any dependent objects won't be included in the transferred pack. Another use case is discussed at [1].

This patch series introduces a --repair option to fetch & fetch-pack to enable doing a full fetch without performing any commit negotiation with the remote, as for a fresh clone. It builds upon cbe566a071 ("negotiator/noop: add noop fetch negotiator", 2020-08-18). While a key use case is described above for partial clones, a user could also use --repair to fix a corrupted object database by performing a refetch of objects that should already be present, establishing a better workflow than deleting the local repository and re-cloning.

 * Using --repair will produce duplicated objects between the existing and
   newly fetched packs, but maintenance will clean them up when it runs
   automatically post-fetch (if enabled).
 * If a user fetches with --repair applying a more restrictive partial clone
   filter than previously (eg: blob:limit=1m then blob:limit=1k) the
   eventual state is a no-op, since any referenced object already in the
   local repository is never removed. More advanced repacking which could
   improve this scenario is currently proposed at [2].

[1] https://public-inbox.org/git/aa7b89ee-08aa-7943-6a00-28dcf344426e@syntevo.com/ [2] https://public-inbox.org/git/21ED346B-A906-4905-B061-EDE53691C586@gmail.com/

Changes since RFC (v1):

 * Changed the name from "refilter" to "repair"
 * Removed dependency between server-side support for filtering and repair
 * Added a test case for a shallow clone
 * Post-fetch auto maintenance now strongly encourages
   repacking/consolidation

Robert Coup (8):
  fetch-negotiator: add specific noop initializor
  fetch-pack: add repairing
  builtin/fetch-pack: add --repair option
  fetch: add --repair option
  t5615-partial-clone: add test for fetch --repair
  maintenance: add ability to pass config options
  fetch: after repair, encourage auto gc repacking
  doc/partial-clone: mention --repair fetch option

 Documentation/fetch-options.txt           | 10 +++++
 Documentation/git-fetch-pack.txt          |  4 ++
 Documentation/technical/partial-clone.txt |  3 ++
 builtin/am.c                              |  2 +-
 builtin/commit.c                          |  2 +-
 builtin/fetch-pack.c                      |  4 ++
 builtin/fetch.c                           | 38 ++++++++++++++--
 builtin/merge.c                           |  2 +-
 builtin/rebase.c                          |  2 +-
 fetch-negotiator.c                        |  5 +++
 fetch-negotiator.h                        |  8 ++++
 fetch-pack.c                              | 50 +++++++++++++--------
 fetch-pack.h                              |  1 +
 remote-curl.c                             |  6 +++
 run-command.c                             |  8 +++-
 run-command.h                             |  5 ++-
 t/t5616-partial-clone.sh                  | 54 ++++++++++++++++++++++-
 transport-helper.c                        |  3 ++
 transport.c                               |  4 ++
 transport.h                               |  4 ++
 20 files changed, 186 insertions(+), 29 deletions(-)

base-commit: dab1b7905d0b295f1acef9785bb2b9cbb0fdec84
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1138%2Frcoup%2Frc-partial-clone-refilter-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1138/rcoup/rc-partial-clone-refilter-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1138

Range-diff vs v1:

 1:  ca50ce77ae4 ! 1:  d146d8aaaaf fetch-negotiator: add specific noop initializor
     @@ Commit message
      
          Add a specific initializor for the noop fetch negotiator. This is
          introduced to support allowing partial clones to skip commit negotiation
     -    when refetching to apply a modified filter.
     +    when fetching to repair or apply a modified filter.
      
          Signed-off-by: Robert Coup <robert@coup.net.nz>
      
 2:  a3c1999936d ! 2:  2d817a65db5 fetch-pack: add partial clone refiltering
     @@ Metadata
      Author: Robert Coup <robert@coup.net.nz>
      
       ## Commit message ##
     -    fetch-pack: add partial clone refiltering
     +    fetch-pack: add repairing
      
     -    Allow refetching with a new partial clone filter by not attempting to
     -    find or negotiate common commits with the remote, and always forcing
     -    a full filtered fetch.
     +    Allow a 'repair fetch' where the contents of the local object store are
     +    ignored and a full fetch is performed, not attempting to find or
     +    negotiate common commits with the remote.
     +
     +    A key use case is to apply a new partial clone blob/tree filter and
     +    refetch all the associated matching content, which would otherwise not
     +    be transferred when the commit objects are already present locally.
      
          Signed-off-by: Robert Coup <robert@coup.net.nz>
      
     @@ fetch-pack.c: static int find_common(struct fetch_negotiator *negotiator,
      -		if (((o = lookup_object(the_repository, remote)) != NULL) &&
      -				(o->flags & COMPLETE)) {
      -			continue;
     -+		if (!args->refilter) {
     ++		if (!args->repair) {
      +			/*
      +			* If that object is complete (i.e. it is an ancestor of a
      +			* local ref), we tell them we have it but do not have to
     @@ fetch-pack.c: static void mark_complete_and_common_ref(struct fetch_negotiator *
       
       	save_commit_buffer = 0;
       
     -+	if (args->refilter)
     ++	if (args->repair)
      +		return;
      +
       	trace2_region_enter("fetch-pack", "parse_remote_refs_and_find_cutoff", NULL);
       	for (ref = *refs; ref; ref = ref->next) {
     - 		struct object *o;
     + 		struct commit *commit;
      @@ fetch-pack.c: static struct ref *do_fetch_pack(struct fetch_pack_args *args,
     - 	int agent_len;
       	struct fetch_negotiator negotiator_alloc;
       	struct fetch_negotiator *negotiator;
     --
     + 
      -	negotiator = &negotiator_alloc;
      -	fetch_negotiator_init(r, negotiator);
     -+	unsigned is_refiltering = 0;
     - 
     +-
       	sort_ref_list(&ref, ref_compare_name);
       	QSORT(sought, nr_sought, cmp_ref_by_name);
     -@@ fetch-pack.c: static struct ref *do_fetch_pack(struct fetch_pack_args *args,
     - 	if (server_supports("filter")) {
     - 		server_supports_filtering = 1;
     - 		print_verbose(args, _("Server supports %s"), "filter");
     --	} else if (args->filter_options.choice) {
     -+	} else if (args->filter_options.choice || args->refilter) {
     - 		warning("filtering not recognized by server, ignoring");
     - 	}
       
     -+	if (server_supports_filtering && args->refilter) {
     -+		is_refiltering = 1;
     -+	}
     -+
     - 	if (server_supports("deepen-since")) {
     - 		print_verbose(args, _("Server supports %s"), "deepen-since");
     - 		deepen_since_ok = 1;
      @@ fetch-pack.c: static struct ref *do_fetch_pack(struct fetch_pack_args *args,
       	if (!server_supports_hash(the_hash_algo->name, NULL))
       		die(_("Server does not support this repository's object format"));
       
      +	negotiator = &negotiator_alloc;
     -+	if (is_refiltering) {
     ++	if (args->repair) {
      +		fetch_negotiator_init_noop(negotiator);
      +	} else {
      +		fetch_negotiator_init(r, negotiator);
     @@ fetch-pack.c: static struct ref *do_fetch_pack(struct fetch_pack_args *args,
       	mark_complete_and_common_ref(negotiator, args, &ref);
       	filter_refs(args, &ref, sought, nr_sought);
      -	if (everything_local(args, &ref)) {
     -+	if (!is_refiltering && everything_local(args, &ref)) {
     ++	if (!args->repair && everything_local(args, &ref)) {
       		packet_flush(fd[1]);
       		goto all_done;
       	}
     @@ fetch-pack.c: static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
       
       	negotiator = &negotiator_alloc;
      -	fetch_negotiator_init(r, negotiator);
     -+	if (args->refilter)
     ++	if (args->repair)
      +		fetch_negotiator_init_noop(negotiator);
      +	else
      +		fetch_negotiator_init(r, negotiator);
     @@ fetch-pack.c: static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
       			mark_complete_and_common_ref(negotiator, args, &ref);
       			filter_refs(args, &ref, sought, nr_sought);
      -			if (everything_local(args, &ref))
     -+			if (!args->refilter && everything_local(args, &ref))
     ++			if (!args->repair && everything_local(args, &ref))
       				state = FETCH_DONE;
       			else
       				state = FETCH_SEND_REQUEST;
     @@ fetch-pack.h: struct fetch_pack_args {
       	unsigned update_shallow:1;
       	unsigned reject_shallow_remote:1;
       	unsigned deepen:1;
     -+	unsigned refilter:1;
     ++	unsigned repair:1;
       
       	/*
       	 * Indicate that the remote of this request is a promisor remote. The
 3:  d7f4b6c8052 ! 3:  a42d40ac294 builtin/fetch-pack: add --refilter option
     @@ Metadata
      Author: Robert Coup <robert@coup.net.nz>
      
       ## Commit message ##
     -    builtin/fetch-pack: add --refilter option
     +    builtin/fetch-pack: add --repair option
      
     -    Add a --refilter option to fetch-pack to force a full fetch when
     -    applying a new filter.
     +    Add a --repair option to fetch-pack to force a full fetch. Use when
     +    applying a new partial clone filter to refetch all matching objects.
      
          Signed-off-by: Robert Coup <robert@coup.net.nz>
      
     @@ Documentation/git-fetch-pack.txt: be in a separate packet, and the list must end
       	current shallow boundary instead of from the tip of each
       	remote branch history.
       
     -+--refilter::
     -+	Skips negotiating commits with the server in order to reapply a new
     -+	partial clone filter and fetch all matching objects.
     ++--repair::
     ++	Skips negotiating commits with the server in order to fetch all matching
     ++	objects. Use to reapply a new partial clone blob/tree filter.
      +
       --no-progress::
       	Do not show the progress.
     @@ builtin/fetch-pack.c: int cmd_fetch_pack(int argc, const char **argv, const char
       			args.from_promisor = 1;
       			continue;
       		}
     -+		if (!strcmp("--refilter", arg)) {
     -+			args.refilter = 1;
     ++		if (!strcmp("--repair", arg)) {
     ++			args.repair = 1;
      +			continue;
      +		}
       		if (skip_prefix(arg, ("--" CL_ARG__FILTER "="), &arg)) {
     @@ remote-curl.c: struct options {
       		/* see documentation of corresponding flag in fetch-pack.h */
       		from_promisor : 1,
       
     -+		refilter : 1,
     ++		repair : 1,
       		atomic : 1,
       		object_format : 1,
       		force_if_includes : 1;
     @@ remote-curl.c: static int set_option(const char *name, const char *value)
       	} else if (!strcmp(name, "from-promisor")) {
       		options.from_promisor = 1;
       		return 0;
     -+	} else if (!strcmp(name, "refilter")) {
     -+		options.refilter = 1;
     ++	} else if (!strcmp(name, "repair")) {
     ++		options.repair = 1;
      +		return 0;
       	} else if (!strcmp(name, "filter")) {
       		options.filter = xstrdup(value);
     @@ remote-curl.c: static int fetch_git(struct discovery *heads,
       		strvec_push(&args, "--deepen-relative");
       	if (options.from_promisor)
       		strvec_push(&args, "--from-promisor");
     -+	if (options.refilter)
     -+		strvec_push(&args, "--refilter");
     ++	if (options.repair)
     ++		strvec_push(&args, "--repair");
       	if (options.filter)
       		strvec_pushf(&args, "--filter=%s", options.filter);
       	strvec_push(&args, url.buf);
 4:  3e102724196 ! 4:  79c409d0542 fetch: add --refilter option
     @@ Metadata
      Author: Robert Coup <robert@coup.net.nz>
      
       ## Commit message ##
     -    fetch: add --refilter option
     +    fetch: add --repair option
      
     -    Teach fetch and transports the --refilter option to force a full fetch
     -    when applying a new filter.
     +    Teach fetch and transports the --repair option to force a full fetch
     +    without negotiating common commits with the remote. Use when applying a
     +    new partial clone filter to refetch all matching objects.
      
          Signed-off-by: Robert Coup <robert@coup.net.nz>
      
     @@ Documentation/fetch-options.txt: endif::git-pull[]
       	setting. See linkgit:git-config[1].
       
      +ifndef::git-pull[]
     -+--refilter::
     -+	Reapply a partial clone filter from configuration or `--filter=`, such
     -+	as when the filter definition has changed. Instead of negotiating with
     -+	the server to avoid transferring commits and associated objects that are
     -+	already present locally, this option fetches all objects that match the
     -+	filter.
     ++--repair::
     ++	Instead of negotiating with the server to avoid transferring commits and
     ++	associated objects that are already present locally, this option fetches
     ++	all objects as a fresh clone would. Use this to reapply a partial clone
     ++	filter from configuration or using `--filter=` when the filter
     ++	definition has changed.
      +endif::git-pull[]
      +
       --refmap=<refspec>::
     @@ builtin/fetch.c: static int prune_tags = -1; /* unspecified */
       static int all, append, dry_run, force, keep, multiple, update_head_ok;
       static int write_fetch_head = 1;
      -static int verbosity, deepen_relative, set_upstream;
     -+static int verbosity, deepen_relative, set_upstream, refilter;
     ++static int verbosity, deepen_relative, set_upstream, repair;
       static int progress = -1;
       static int enable_auto_gc = 1;
       static int tags = TAGS_DEFAULT, unshallow, update_shallow, deepen;
     @@ builtin/fetch.c: static struct option builtin_fetch_options[] = {
       	OPT_SET_INT_F(0, "unshallow", &unshallow,
       		      N_("convert to a complete repository"),
       		      1, PARSE_OPT_NONEG),
     -+	OPT_SET_INT_F(0, "refilter", &refilter,
     -+		      N_("re-fetch with a modified filter"),
     ++	OPT_SET_INT_F(0, "repair", &repair,
     ++		      N_("re-fetch without negotiating common commits"),
      +		      1, PARSE_OPT_NONEG),
       	{ OPTION_STRING, 0, "submodule-prefix", &submodule_prefix, N_("dir"),
       		   N_("prepend this to submodule path output"), PARSE_OPT_HIDDEN },
     @@ builtin/fetch.c: static int check_exist_and_connected(struct ref *ref_map)
       		return -1;
       
      +	/*
     -+	 * Similarly, if we need to refilter a partial clone we already have
     -+	 * these commits reachable.  Running rev-list here will return with
     -+	 * a good (0) exit status and we'll bypass the fetch that we
     -+	 * really need to perform.  Claiming failure now will ensure
     -+	 * we perform the network exchange to reapply the filter.
     ++	 * Similarly, if we need to repair, we always want to perform a full
     ++	 * fetch ignoring existing objects.
      +	 */
     -+	if (refilter)
     ++	if (repair)
      +		return -1;
      +
      +
     @@ builtin/fetch.c: static struct transport *prepare_transport(struct remote *remot
       		set_option(transport, TRANS_OPT_DEEPEN_RELATIVE, "yes");
       	if (update_shallow)
       		set_option(transport, TRANS_OPT_UPDATE_SHALLOW, "yes");
     -+	if (refilter)
     -+		set_option(transport, TRANS_OPT_REFILTER, "yes");
     ++	if (repair)
     ++		set_option(transport, TRANS_OPT_REPAIR, "yes");
       	if (filter_options.choice) {
       		const char *spec =
       			expand_list_objects_filter_spec(&filter_options);
     @@ transport-helper.c: static int fetch_refs(struct transport *transport,
       	if (data->transport_options.update_shallow)
       		set_helper_option(transport, "update-shallow", "true");
       
     -+	if (data->transport_options.refilter)
     -+		set_helper_option(transport, "refilter", "true");
     ++	if (data->transport_options.repair)
     ++		set_helper_option(transport, "repair", "true");
      +
       	if (data->transport_options.filter_options.choice) {
       		const char *spec = expand_list_objects_filter_spec(
     @@ transport.c: static int set_git_option(struct git_transport_options *opts,
       		list_objects_filter_die_if_populated(&opts->filter_options);
       		parse_list_objects_filter(&opts->filter_options, value);
       		return 0;
     -+	} else if (!strcmp(name, TRANS_OPT_REFILTER)) {
     -+		opts->refilter = !!value;
     ++	} else if (!strcmp(name, TRANS_OPT_REPAIR)) {
     ++		opts->repair = !!value;
      +		return 0;
       	} else if (!strcmp(name, TRANS_OPT_REJECT_SHALLOW)) {
       		opts->reject_shallow = !!value;
     @@ transport.c: static int fetch_refs_via_pack(struct transport *transport,
       	args.update_shallow = data->options.update_shallow;
       	args.from_promisor = data->options.from_promisor;
       	args.filter_options = data->options.filter_options;
     -+	args.refilter = data->options.refilter;
     ++	args.repair = data->options.repair;
       	args.stateless_rpc = transport->stateless_rpc;
       	args.server_options = transport->server_options;
       	args.negotiation_tips = data->options.negotiation_tips;
     @@ transport.h: struct git_transport_options {
       	unsigned update_shallow : 1;
       	unsigned reject_shallow : 1;
       	unsigned deepen_relative : 1;
     -+	unsigned refilter : 1;
     ++	unsigned repair : 1;
       
       	/* see documentation of corresponding flag in fetch-pack.h */
       	unsigned from_promisor : 1;
     @@ transport.h: void transport_check_allowed(const char *type);
       /* Filter objects for partial clone and fetch */
       #define TRANS_OPT_LIST_OBJECTS_FILTER "filter"
       
     -+/* Refilter a previous partial clone */
     -+#define TRANS_OPT_REFILTER "refilter"
     ++/* Refetch all objects without negotiating */
     ++#define TRANS_OPT_REPAIR "repair"
      +
       /* Request atomic (all-or-nothing) updates when pushing */
       #define TRANS_OPT_ATOMIC "atomic"
 5:  66c18850aad ! 5:  38af2bbee79 t5615-partial-clone: add test for --refilter
     @@ Metadata
      Author: Robert Coup <robert@coup.net.nz>
      
       ## Commit message ##
     -    t5615-partial-clone: add test for --refilter
     +    t5615-partial-clone: add test for fetch --repair
      
     -    Add a test for partial clone refiltering under protocol v0 and v2.
     +    Add a test for doing a repair fetch to apply a changed partial clone
     +    filter under protocol v0 and v2.
      
          Signed-off-by: Robert Coup <robert@coup.net.nz>
      
     @@ t/t5616-partial-clone.sh: test_expect_success 'manual prefetch of missing object
      +	git -C src push -u srv main
      +'
      +
     -+# Do partial fetch to fetch smaller files; then verify that without --refilter
     ++# Do partial fetch to fetch smaller files; then verify that without --repair
      +# applying a new filter does not refetch missing large objects. Then use
     -+# --refilter to apply the new filter on existing commits. Test it under both
     ++# --repair to apply the new filter on existing commits. Test it under both
      +# protocol v2 & v0.
     -+test_expect_success 'apply a different filter using --refilter' '
     ++test_expect_success 'apply a different filter using --repair' '
      +	git -C pc1 fetch --filter=blob:limit=999 origin &&
      +	git -C pc1 rev-list --quiet --objects --missing=print \
      +		main..origin/main >observed &&
      +	test_line_count = 4 observed &&
      +
     -+	git -C pc1 fetch --filter=blob:limit=19999 --refilter origin &&
     ++	git -C pc1 fetch --filter=blob:limit=19999 --repair origin &&
      +	git -C pc1 rev-list --quiet --objects --missing=print \
      +		main..origin/main >observed &&
      +	test_line_count = 2 observed &&
      +
      +	git -c protocol.version=0 -C pc1 fetch --filter=blob:limit=29999 \
     -+		--refilter origin &&
     ++		--repair origin &&
      +	git -C pc1 rev-list --quiet --objects --missing=print \
      +		main..origin/main >observed &&
      +	test_line_count = 0 observed
      +'
     ++
     ++test_expect_success 'fetch --repair works with a shallow clone' '
     ++	git clone --no-checkout --depth=1 --filter=blob:none "file://$(pwd)/srv.bare" pc1s &&
     ++	git -C pc1s rev-list --objects --missing=print HEAD >observed &&
     ++	test_line_count = 6 observed &&
     ++
     ++	GIT_TRACE=1 git -C pc1s fetch --filter=blob:limit=999 --repair origin &&
     ++	git -C pc1s rev-list --objects --missing=print HEAD >observed &&
     ++	test_line_count = 6 observed
     ++'
      +
       test_expect_success 'partial clone with transfer.fsckobjects=1 works with submodules' '
       	test_create_repo submodule &&
 -:  ----------- > 6:  cfa6dca8ef4 maintenance: add ability to pass config options
 -:  ----------- > 7:  2338c15249a fetch: after repair, encourage auto gc repacking
 6:  6c4d4260bfc ! 8:  20942562a66 doc/partial-clone: mention --refilter option
     @@ Metadata
      Author: Robert Coup <robert@coup.net.nz>
      
       ## Commit message ##
     -    doc/partial-clone: mention --refilter option
     +    doc/partial-clone: mention --repair fetch option
      
     -    Add the fetch --refilter option to the partial clone documentation.
     +    Document it for partial clones as a means to refetch with a new filter.
      
          Signed-off-by: Robert Coup <robert@coup.net.nz>
      
     @@ Documentation/technical/partial-clone.txt: Fetching Missing Objects
         currently fetches all objects referred to by the requested objects, even
         though they are not necessary.
       
     -+- Fetching with `--refilter` will request a complete new filtered packfile from
     ++- Fetching with `--repair` will request a complete new filtered packfile from
      +  the remote, which can be used to change a filter without needing to
      +  dynamically fetch missing objects.
       

From: Robert Coup <robert@coup.net.nz>

Document it for partial clones as a means to refetch with a new filter.

Signed-off-by: Robert Coup <robert@coup.net.nz>
---
 Documentation/technical/partial-clone.txt | 3 +++
 1 file changed, 3 insertions(+)

From: Robert Coup <robert@coup.net.nz>

Add a --repair option to fetch-pack to force a full fetch. Use when applying a new partial clone filter to refetch all matching objects.

Signed-off-by: Robert Coup <robert@coup.net.nz>
---
 Documentation/git-fetch-pack.txt | 4 ++++
 builtin/fetch-pack.c             | 4 ++++
 remote-curl.c                    | 6 ++++++
 3 files changed, 14 insertions(+)

From: Robert Coup <robert@coup.net.nz>

Allow a 'repair fetch' where the contents of the local object store are ignored and a full fetch is performed, not attempting to find or negotiate common commits with the remote.

A key use case is to apply a new partial clone blob/tree filter and refetch all the associated matching content, which would otherwise not be transferred when the commit objects are already present locally.

Signed-off-by: Robert Coup <robert@coup.net.nz>
---
 fetch-pack.c | 50 +++++++++++++++++++++++++++++++-------------------
 fetch-pack.h |  1 +
 2 files changed, 32 insertions(+), 19 deletions(-)

"Robert Coup via GitGitGadget" <gitgitgadget@gmail.com> writes:

Reading how the original value of save_commit_buffer is saved away, the variable gets cleared and then gets restored before the function returns in the normal codepath, this new code looks wrong. Hitting this early return after clearing the variable means nobody will restore the saved value of the variable, no?

I know why you want to force the "noop" negotiator while repairing, but it is unclear why you need to move this down in the function.

Hmph. I am debating myself if hardcoding the implementation detail of "when repairing, the noop negitiator is the only useful one" like this code does is a sensible thing to do. If we later need to tweak the choice of negotiator used depending on the caller's needs, perhaps fetch_negotiator_init() should gain a new flags word, i.e.

	fetch_negotiator_init(struct repository *,
			      struct fetch_negotiator *,
			      unsigned flags)

where "Use negotiator suitable for the repairing fetch" could be a single bit in the flags word, making this caller more like:

	negotiator = &negotiator_alloc;
	flags = 0;
	if (args->repair)
		flags |= FETCH_NEGOTIATOR_REPAIRING;
	fetch_negotiator_init(r, negotiator, flags);

perhaps.  That way, [1/8] becomes unnecessary.

Likewise.

Hi Junio,

On Fri, 25 Feb 2022 at 06:46, Junio C Hamano <gitster@pobox.com> wrote:

Good spotting, thank you.

Seemed cleaner to initialise the right negotiator once, rather than clearing and re-initialising depending on repair mode.

To me this feels a bit hypothetical, but maybe I'm missing a use case? The point of repairing is not to negotiate common commits and do (effectively) a clone-style fresh fetch. If some future special negotiator that has a repair mode arrives, or likewise a more complex repair mode then other things will probably need adapting?

With the current patch it is clear what's happening, that the user's negotiator selection is deliberately being ignored for the purposes of repairing. Conversely, calling negotiator_init() asking for a skipping negotiator in repair mode and getting back a noop negotiator seems unobvious.

Thanks,

Rob :)

From: Robert Coup <robert@coup.net.nz>

Add a specific initializor for the noop fetch negotiator. This is introduced to support allowing partial clones to skip commit negotiation when fetching to repair or apply a modified filter.

Signed-off-by: Robert Coup <robert@coup.net.nz>
---
 fetch-negotiator.c | 5 +++++
 fetch-negotiator.h | 8 ++++++++
 2 files changed, 13 insertions(+)

"Robert Coup via GitGitGadget" <gitgitgadget@gmail.com> writes:

"initializer"?

Puzzling. What makes this better than allowing noop-negotiator-init to be called directly?

Hi,

On Fri, 25 Feb 2022 at 06:19, Junio C Hamano <gitster@pobox.com> wrote:

Thanks. A poor translation effort to American English ;-)

Is simply for naming/API consistency with the regular method. Happy to call noop_negotiator_init() if you prefer?

Rob :)

From: Robert Coup <robert@coup.net.nz>

Teach fetch and transports the --repair option to force a full fetch without negotiating common commits with the remote. Use when applying a new partial clone filter to refetch all matching objects.

Signed-off-by: Robert Coup <robert@coup.net.nz>
---
 Documentation/fetch-options.txt |  9 +++++++++
 builtin/fetch.c                 | 15 ++++++++++++++-
 transport-helper.c              |  3 +++
 transport.c                     |  4 ++++
 transport.h                     |  4 ++++
 5 files changed, 34 insertions(+), 1 deletion(-)

From: Robert Coup <robert@coup.net.nz>

Add a test for doing a repair fetch to apply a changed partial clone filter under protocol v0 and v2.

Signed-off-by: Robert Coup <robert@coup.net.nz>
---
 t/t5616-partial-clone.sh | 52 +++++++++++++++++++++++++++++++++++++++-
 1 file changed, 51 insertions(+), 1 deletion(-)

From: Robert Coup <robert@coup.net.nz>

Make run_auto_maintenance() accept optional config options for a specific invocation of the auto-maintenance process.

Signed-off-by: Robert Coup <robert@coup.net.nz>
---
 builtin/am.c     | 2 +-
 builtin/commit.c | 2 +-
 builtin/fetch.c  | 2 +-
 builtin/merge.c  | 2 +-
 builtin/rebase.c | 2 +-
 run-command.c    | 8 +++++++-
 run-command.h    | 5 ++++-
 7 files changed, 16 insertions(+), 7 deletions(-)

"Robert Coup via GitGitGadget" <gitgitgadget@gmail.com> writes:

Style.  SP on both sides of '<'.

It is unclear if it is generally a good idea to pass hardcoded set of configuration variables to begin with, but provided if it makes sense [*], the implementation seems OK.

	Side note.  And this is a big *IF*, as we can see in all
	other helper functions in run-commands.h, nobody has such a
	privision.  If supporting such a "feature" makes sense, we
	probably would need to do so with a common interface that
	can be used across run_command() API, not with an ad-hoc
	interface that is only usable with run_auto_maintenance(),
	which may look somewhat similar to how we have a common way
	to pass set of environment variables.

n

Hi Junio,

On Fri, 25 Feb 2022 at 06:57, Junio C Hamano <gitster@pobox.com> wrote:

Thanks

My RFC didn't do any repacking, and you (rightly) raised it at https://lore.kernel.org/git/xmqqk0eecpl9.fsf@gitster.g/

This was my best stab at (I think) safely doing it — it respects users who have disabled auto-gc/repacking/maintenance, and doesn't add yet another config variable. But we could also:

1. do nothing — the user's object DB will contain duplicate objects
until some repacking happens. This could be up to twice the size it
"should" be.
2. print a message to suggest running `git gc` / `git maintenance`

I'm keen on some input from Derrick or someone deeply familiar with the maintenance/GC bits (particularly wrt incremental repacking) whether what I'm doing would cause issues for people with complex setups I haven't thought of.

Ævar pointed out GIT_CONFIG_PARAMETERS has an API to wrap it's use, so I can adapt the implementation to use that.

Thanks

Rob :)

Robert Coup <robert@coup.net.nz> writes:

Depending on how that "API to wrap" looks like, it may not be a wise idea, though. You'd need to consider how it interacts with those who run "git -c var=val" to enter this codepath.

On Thu, Feb 24 2022, Robert Coup via GitGitGadget wrote:

Shouldn't this bei using git grep the git_config_push.*parameter() functions instead of adding a new custom method to do this.

Perhaps there's some subtle distinction between the two that's important here, or perhaps you just didn't know about that API...

Hi Ævar,

On Fri, 25 Feb 2022 at 10:30, Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

I didn't know of it — it could probably use a docstring too ;-) A few other sub-commands seemed to use the `-c key=value` approach so it didn't seem like GIT_CONFIG_PARAMETERS was universal.

Easy enough to swap to, thanks. Assuming we want it, but I'm about to reply to Junio's email.

Rob :)

From: Robert Coup <robert@coup.net.nz>

After invoking `fetch --repair`, the object db will likely contain many duplicate objects. If auto-maintenance is enabled, invoke it with appropriate settings to encourage repacking/consolidation.

* gc.autoPackLimit: unless this is set to 0 (disabled), override the
  value to 1 to force pack consolidation.
* maintenance.incremental-repack.auto: unless this is set to 0, override
  the value to -1 to force incremental repacking.

Signed-off-by: Robert Coup <robert@coup.net.nz>
---
 Documentation/fetch-options.txt |  3 ++-
 builtin/fetch.c                 | 23 +++++++++++++++++++++--
 t/t5616-partial-clone.sh        |  6 ++++--
 3 files changed, 27 insertions(+), 5 deletions(-)

On Thu, Feb 24 2022, Robert Coup via GitGitGadget wrote:

[Nits, but aside from earlier comments about config options v.s. config[]

this variable...

...and this...

...can just be declared in this scope.

I think having:

    if (enable_auto_gc && repair)

Might make this more readable without the extra indentation, but of course then the variables need to be at the top-level... :)

Nit: Better to use $PWD instead of $(pwd), works here, but won't be
compatible with -x if we ever want to test stderr.

On Thu, Feb 24 2022, Robert Coup via GitGitGadget wrote:

I realize this was probably based on feedback on v1 (I didn't go back and re-read it, sorry).

But I feel strongly that we really should name this something other than --repair. I don't care much if it isn't that :) But maybe --expand-filters, --fleshen-partial or something like that?

So first (and partially as an aside): Is a "noop" negotiatior really want we want at all? Don't we instead want to be discovering those parts of our history that are closed under reachability (if any) and say we HAVE those things during negotiation?

I haven't tested, but maybe that's just more complex, e.g. you have a filter that's excluding >500MB blobs or whatever you might have the full history already, or maybe that 500MB blob was added last week, so you have almost all of it.

But wouldn't that be a lot kinder to server resources + network at the expense of some (presumably rare) extra local computation?

But secondly, on the "--repair" name: The reason I mentioned that is that I'd really like us to actually have a "my repo is screwed, please repair it".

But (and I haven't tested, but I'm pretty sure), this patch series isn't going to give you that. The reasons are elaborated on in [1], basically we try really hard to re-use local data, and due to that & the collision detection will often just hard die early in object walking.

But maybe I'm wrong, have you actually tested this with *broken* objects as opposed to just missing ones with repo filters + promisors in play? Our t/*fsck* and t/*corrupt*/ etc. tests have some of those.

And maybe I'm making a big deal out of nothing, but I fear that by naming it --repair and giving it these semantics that we'd be closing the door on things that are actually needed for some of the trickier edge cases when it comes to repairing a bad repository.

Including but not limited to having a loose BAD_OBJ, and needing to replace it with another loose object (due to the unpack limit), the branch we're updating can't be read locally, but is at an OID that's (re-)included in the incoming pack and is hopefully about to repair our repository.

Or even that we have a SHA-1 collision, but we intentionally want to override the collision detection because we know our local repo is bad, but the remote can be trusted.

All of which are much more involved than just the "fleshen partial data" you're aiming for here...

1. https://lore.kernel.org/git/87czo7haha.fsf@evledraar.gmail.com/

Hi Ævar,

Thanks for taking the time to look into this,

On Mon, 28 Feb 2022 at 16:54, Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

Yes, `fetch --repair` was from Jonathan Tan's v1 feedback[1], where he pointed out it could fill in lost objects from any remote in a more generally useful fashion.

My goal here is to refetch with a different filter so that I get the outcome of a `clone --filter=` without having to chuck my object directory. But the actual implementation doesn't need to know anything specific about filters, so the original "refilter" name I had isn't really right.

fleshen-partial sounds like a horror movie scene to me.

1. `--refetch`
2. `--as-clone`
3. `--expand-filter` (though TBC you don't necessarily need a filter)
4. `--refilter`
5. something else

At an object level we don't have any means of knowing what has or hasn't been obtained via fetch to a partial clone with different `--filter` args (via config or cli), dynamic fault-ins, or sourced from a different remote. Fetch negotiation only occurs for refs and their associated commits/histories, but filtering occurs at the blob or tree level — so we often HAVE a commit but not all of its trees/blobs, whereupon negotiation skips that commit and all it's associated objects.

Feels like people would look at `fsck` for that over `fetch`? Maybe not. Anyway, I get the point about the naming still not being right :-)

Correct: I haven't tested with such objects/broken ODBs. Ideally
repack/gc/etc would prefer a new-fixed pack over the old-broken
pack/object but that's not really what I'm aiming to achieve here or
am interested in.

1. https://lore.kernel.org/git/20220202185957.1928631-1-jonathantanmy@google.com/

Thanks,

Rob :)

Robert Coup <robert@coup.net.nz> writes:

I do think, as part of a larger "repair" task, fetching objects again from a known-good remote would be a sensible one step. So "re"fetch would be a good way to phrase it.

This I think is in the same vein.

These focus on a wrong thing, I would think. "I suspect my object store may be missing some objects, and I want to repair it by getting another copy from a known-good remote" would be the simplest and easiest-to-explain form of the end-user requirement this feature helps. The end-user wish does not require "filter" at all.

But this reminds of me about a few caveats.

The patch does implement a good way to lie about what we have to the other end, forcing them to send everything that is necessary to create a new clone.

But that is only a half of what is required to "repair a possibly broken object store by re-fetching". It is sufficient for repairing "missing objects", but not enough for repairing "broken/corrupt" objects. The object store has a mechanism to resist collision attacks by ignoring objects coming over the network that do not match what we locally already have.

I offhand do not think it is a practical problem that this new mechanism is incapable of repair a repository with broken/corrupt objects [*]. But tying the bland and broad word "repair" to the feature that is meant to only deal with "missing" but not "curropt" form of object store breakage may lead to confusion.

	Side note: others who spent more time on partial/lazy clone
	might realize that broken/corrupt objects are real problems
	they want to tackle, though.  I dunno, and that is why I am
	raising it as a potential issue.

Thanks.

On Mon, Feb 28 2022, Robert Coup wrote:

*nod*

*nod*

Yes, I'm basically asking if the negotiation part wouldn't *ideally* be doing basically the same "everything is connected" check receive-pack/fsck do.

I.e. you've got partial data with promisors locally, but if you walk your branch histor(y|ies) you'll discover that N commits down we have all the prerequisite objects locally.

As an aside there's a 1=1 mapping between that and what "git bundle create" will do/verify to create a bundle without listed prerequisites.

I.e. I think you'll find what it does with revision.c and PREREQ_* and other flags INTERESTING (a lame pun on its use of UNINTERESTING :).

Presumably the code needed to drive such a negotiation would be useful for other neat stuff, e.g. having a some-partial repo locally, wanting to fetch the PACK to complete it from the server, and knowing you have that data to create a fully connected (or incremental) bundle for that repository, but I digress.

I think that definitely would be fetch/gc over "fsck". I.e. if you've got corruption fsck can only tell you that it's screwed.

It's fetch/gc (or "git bundle unbundle") that stand any chance of actually doing the repair, since we'd need to stitch together the (partially) corrupted local/remote content with a hopefully good compliment to it.

FWIW I had an ad-hoc implementation of this basically working by disabling the negotiation + not doing any object existence/collision checks before writing content to the repository.

That and teaching "repack" to not die and instead to carry on in the face of object decoding failure (and hopefully discover a "duplicate" but good copy later) + "gc" is enough to repair most corruption, e.g. truncated loose object etc.

I think I've only tested loose (bad) + pack (good), I think pack (bad) + pack (good) probably has some bigger caveats (like the first error aborting the whole pack read, due to deltas etc.).

But yeah, I'm not saying this should be on your radar at all, other than the bikeshedding comment of having a --repair that doesn't really do "repair" would be unfortunate naming, especially if we're locked into behavior orthagonal to that needed for an "actual" repair.

If a filter is changed on a partial clone repository, for example from blob:none to blob:limit=1m, there is currently no straightforward way to bulk-refetch the objects that match the new filter for existing local commits. This is because the client will report commits as "have" during fetch negotiation and any dependent objects won't be included in the transferred pack. Another use case is discussed at [1].

This patch series introduces a --refetch option to fetch & fetch-pack to enable doing a full fetch without performing any commit negotiation with the remote, as a fresh clone does. It builds upon cbe566a071 ("negotiator/noop: add noop fetch negotiator", 2020-08-18).

 * Using --refetch will produce duplicated objects between the existing and
   newly fetched packs, but maintenance will clean them up when it runs
   automatically post-fetch (if enabled).
 * If a user fetches with --refetch applying a more restrictive partial
   clone filter than previously (eg: blob:limit=1m then blob:limit=1k) the
   eventual state is a no-op, since any referenced object already in the
   local repository is never removed. More advanced repacking which could
   improve this scenario is currently proposed at [2].

[1] https://lore.kernel.org/git/aa7b89ee-08aa-7943-6a00-28dcf344426e@syntevo.com/ [2] https://lore.kernel.org/git/21ED346B-A906-4905-B061-EDE53691C586@gmail.com/

Changes since v2:

 * Changed the name from "repair" to "refetch". While it's conceivable to
   use it in some object DB repair situations that's not the focus of these
   changes.
 * Pass config options to maintenance via GIT_CONFIG_PARAMETERS
 * Split out auto-maintenance to a separate & more robust test
 * Minor fixes/improvements from reviews by Junio & Ævar

Changes since RFC (v1):

 * Changed the name from "refilter" to "repair"
 * Removed dependency between server-side support for filtering and repair
 * Added a test case for a shallow clone
 * Post-fetch auto maintenance now strongly encourages
   repacking/consolidation

Robert Coup (7):
  fetch-negotiator: add specific noop initializer
  fetch-pack: add refetch
  builtin/fetch-pack: add --refetch option
  fetch: add --refetch option
  t5615-partial-clone: add test for fetch --refetch
  fetch: after refetch, encourage auto gc repacking
  doc/partial-clone: mention --refetch fetch option

 Documentation/fetch-options.txt           | 10 +++
 Documentation/git-fetch-pack.txt          |  4 ++
 Documentation/technical/partial-clone.txt |  3 +
 builtin/fetch-pack.c                      |  4 ++
 builtin/fetch.c                           | 34 +++++++++-
 fetch-negotiator.c                        |  5 ++
 fetch-negotiator.h                        |  8 +++
 fetch-pack.c                              | 46 ++++++++-----
 fetch-pack.h                              |  1 +
 remote-curl.c                             |  6 ++
 t/t5616-partial-clone.sh                  | 81 ++++++++++++++++++++++-
 transport-helper.c                        |  3 +
 transport.c                               |  4 ++
 transport.h                               |  4 ++
 14 files changed, 193 insertions(+), 20 deletions(-)

base-commit: 715d08a9e51251ad8290b181b6ac3b9e1f9719d7
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1138%2Frcoup%2Frc-partial-clone-refilter-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1138/rcoup/rc-partial-clone-refilter-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/1138

Range-diff vs v2:

 1:  d146d8aaaaf ! 1:  96a75be3d8a fetch-negotiator: add specific noop initializor
     @@ Metadata
      Author: Robert Coup <robert@coup.net.nz>
      
       ## Commit message ##
     -    fetch-negotiator: add specific noop initializor
     +    fetch-negotiator: add specific noop initializer
      
     -    Add a specific initializor for the noop fetch negotiator. This is
     +    Add a specific initializer for the noop fetch negotiator. This is
          introduced to support allowing partial clones to skip commit negotiation
     -    when fetching to repair or apply a modified filter.
     +    when performing a "refetch".
      
          Signed-off-by: Robert Coup <robert@coup.net.nz>
      
 2:  2d817a65db5 ! 2:  04ca6a07f85 fetch-pack: add repairing
     @@ Metadata
      Author: Robert Coup <robert@coup.net.nz>
      
       ## Commit message ##
     -    fetch-pack: add repairing
     +    fetch-pack: add refetch
      
     -    Allow a 'repair fetch' where the contents of the local object store are
     +    Allow a "refetch" where the contents of the local object store are
          ignored and a full fetch is performed, not attempting to find or
          negotiate common commits with the remote.
      
     @@ fetch-pack.c: static int find_common(struct fetch_negotiator *negotiator,
      -		if (((o = lookup_object(the_repository, remote)) != NULL) &&
      -				(o->flags & COMPLETE)) {
      -			continue;
     -+		if (!args->repair) {
     ++		if (!args->refetch) {
      +			/*
      +			* If that object is complete (i.e. it is an ancestor of a
      +			* local ref), we tell them we have it but do not have to
     @@ fetch-pack.c: static int find_common(struct fetch_negotiator *negotiator,
       
       		remote_hex = oid_to_hex(remote);
      @@ fetch-pack.c: static void mark_complete_and_common_ref(struct fetch_negotiator *negotiator,
     + 	int old_save_commit_buffer = save_commit_buffer;
     + 	timestamp_t cutoff = 0;
       
     - 	save_commit_buffer = 0;
     - 
     -+	if (args->repair)
     ++	if (args->refetch)
      +		return;
      +
     + 	save_commit_buffer = 0;
     + 
       	trace2_region_enter("fetch-pack", "parse_remote_refs_and_find_cutoff", NULL);
     - 	for (ref = *refs; ref; ref = ref->next) {
     - 		struct commit *commit;
      @@ fetch-pack.c: static struct ref *do_fetch_pack(struct fetch_pack_args *args,
     - 	struct fetch_negotiator negotiator_alloc;
       	struct fetch_negotiator *negotiator;
       
     --	negotiator = &negotiator_alloc;
     + 	negotiator = &negotiator_alloc;
      -	fetch_negotiator_init(r, negotiator);
     --
     - 	sort_ref_list(&ref, ref_compare_name);
     - 	QSORT(sought, nr_sought, cmp_ref_by_name);
     - 
     -@@ fetch-pack.c: static struct ref *do_fetch_pack(struct fetch_pack_args *args,
     - 	if (!server_supports_hash(the_hash_algo->name, NULL))
     - 		die(_("Server does not support this repository's object format"));
     - 
     -+	negotiator = &negotiator_alloc;
     -+	if (args->repair) {
     ++	if (args->refetch) {
      +		fetch_negotiator_init_noop(negotiator);
      +	} else {
      +		fetch_negotiator_init(r, negotiator);
      +	}
     -+
     + 
     + 	sort_ref_list(&ref, ref_compare_name);
     + 	QSORT(sought, nr_sought, cmp_ref_by_name);
     +@@ fetch-pack.c: static struct ref *do_fetch_pack(struct fetch_pack_args *args,
     + 
       	mark_complete_and_common_ref(negotiator, args, &ref);
       	filter_refs(args, &ref, sought, nr_sought);
      -	if (everything_local(args, &ref)) {
     -+	if (!args->repair && everything_local(args, &ref)) {
     ++	if (!args->refetch && everything_local(args, &ref)) {
       		packet_flush(fd[1]);
       		goto all_done;
       	}
     @@ fetch-pack.c: static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
       
       	negotiator = &negotiator_alloc;
      -	fetch_negotiator_init(r, negotiator);
     -+	if (args->repair)
     ++	if (args->refetch)
      +		fetch_negotiator_init_noop(negotiator);
      +	else
      +		fetch_negotiator_init(r, negotiator);
     @@ fetch-pack.c: static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
       			mark_complete_and_common_ref(negotiator, args, &ref);
       			filter_refs(args, &ref, sought, nr_sought);
      -			if (everything_local(args, &ref))
     -+			if (!args->repair && everything_local(args, &ref))
     ++			if (!args->refetch && everything_local(args, &ref))
       				state = FETCH_DONE;
       			else
       				state = FETCH_SEND_REQUEST;
     @@ fetch-pack.h: struct fetch_pack_args {
       	unsigned update_shallow:1;
       	unsigned reject_shallow_remote:1;
       	unsigned deepen:1;
     -+	unsigned repair:1;
     ++	unsigned refetch:1;
       
       	/*
       	 * Indicate that the remote of this request is a promisor remote. The
 3:  a42d40ac294 ! 3:  879d30c4473 builtin/fetch-pack: add --repair option
     @@ Metadata
      Author: Robert Coup <robert@coup.net.nz>
      
       ## Commit message ##
     -    builtin/fetch-pack: add --repair option
     +    builtin/fetch-pack: add --refetch option
      
     -    Add a --repair option to fetch-pack to force a full fetch. Use when
     +    Add a refetch option to fetch-pack to force a full fetch. Use when
          applying a new partial clone filter to refetch all matching objects.
      
          Signed-off-by: Robert Coup <robert@coup.net.nz>
     @@ Documentation/git-fetch-pack.txt: be in a separate packet, and the list must end
       	current shallow boundary instead of from the tip of each
       	remote branch history.
       
     -+--repair::
     ++--refetch::
      +	Skips negotiating commits with the server in order to fetch all matching
      +	objects. Use to reapply a new partial clone blob/tree filter.
      +
     @@ builtin/fetch-pack.c: int cmd_fetch_pack(int argc, const char **argv, const char
       			args.from_promisor = 1;
       			continue;
       		}
     -+		if (!strcmp("--repair", arg)) {
     -+			args.repair = 1;
     ++		if (!strcmp("--refetch", arg)) {
     ++			args.refetch = 1;
      +			continue;
      +		}
       		if (skip_prefix(arg, ("--" CL_ARG__FILTER "="), &arg)) {
     @@ remote-curl.c: struct options {
       		/* see documentation of corresponding flag in fetch-pack.h */
       		from_promisor : 1,
       
     -+		repair : 1,
     ++		refetch : 1,
       		atomic : 1,
       		object_format : 1,
       		force_if_includes : 1;
     @@ remote-curl.c: static int set_option(const char *name, const char *value)
       	} else if (!strcmp(name, "from-promisor")) {
       		options.from_promisor = 1;
       		return 0;
     -+	} else if (!strcmp(name, "repair")) {
     -+		options.repair = 1;
     ++	} else if (!strcmp(name, "refetch")) {
     ++		options.refetch = 1;
      +		return 0;
       	} else if (!strcmp(name, "filter")) {
       		options.filter = xstrdup(value);
     @@ remote-curl.c: static int fetch_git(struct discovery *heads,
       		strvec_push(&args, "--deepen-relative");
       	if (options.from_promisor)
       		strvec_push(&args, "--from-promisor");
     -+	if (options.repair)
     -+		strvec_push(&args, "--repair");
     ++	if (options.refetch)
     ++		strvec_push(&args, "--refetch");
       	if (options.filter)
       		strvec_pushf(&args, "--filter=%s", options.filter);
       	strvec_push(&args, url.buf);
 4:  79c409d0542 ! 4:  a503b98f333 fetch: add --repair option
     @@ Metadata
      Author: Robert Coup <robert@coup.net.nz>
      
       ## Commit message ##
     -    fetch: add --repair option
     +    fetch: add --refetch option
      
     -    Teach fetch and transports the --repair option to force a full fetch
     +    Teach fetch and transports the --refetch option to force a full fetch
          without negotiating common commits with the remote. Use when applying a
          new partial clone filter to refetch all matching objects.
      
     @@ Documentation/fetch-options.txt: endif::git-pull[]
       	setting. See linkgit:git-config[1].
       
      +ifndef::git-pull[]
     -+--repair::
     ++--refetch::
      +	Instead of negotiating with the server to avoid transferring commits and
      +	associated objects that are already present locally, this option fetches
      +	all objects as a fresh clone would. Use this to reapply a partial clone
     @@ builtin/fetch.c: static int prune_tags = -1; /* unspecified */
       static int all, append, dry_run, force, keep, multiple, update_head_ok;
       static int write_fetch_head = 1;
      -static int verbosity, deepen_relative, set_upstream;
     -+static int verbosity, deepen_relative, set_upstream, repair;
     ++static int verbosity, deepen_relative, set_upstream, refetch;
       static int progress = -1;
       static int enable_auto_gc = 1;
       static int tags = TAGS_DEFAULT, unshallow, update_shallow, deepen;
     @@ builtin/fetch.c: static struct option builtin_fetch_options[] = {
       	OPT_SET_INT_F(0, "unshallow", &unshallow,
       		      N_("convert to a complete repository"),
       		      1, PARSE_OPT_NONEG),
     -+	OPT_SET_INT_F(0, "repair", &repair,
     ++	OPT_SET_INT_F(0, "refetch", &refetch,
      +		      N_("re-fetch without negotiating common commits"),
      +		      1, PARSE_OPT_NONEG),
       	{ OPTION_STRING, 0, "submodule-prefix", &submodule_prefix, N_("dir"),
     @@ builtin/fetch.c: static int check_exist_and_connected(struct ref *ref_map)
       		return -1;
       
      +	/*
     -+	 * Similarly, if we need to repair, we always want to perform a full
     ++	 * Similarly, if we need to refetch, we always want to perform a full
      +	 * fetch ignoring existing objects.
      +	 */
     -+	if (repair)
     ++	if (refetch)
      +		return -1;
      +
      +
     @@ builtin/fetch.c: static struct transport *prepare_transport(struct remote *remot
       		set_option(transport, TRANS_OPT_DEEPEN_RELATIVE, "yes");
       	if (update_shallow)
       		set_option(transport, TRANS_OPT_UPDATE_SHALLOW, "yes");
     -+	if (repair)
     -+		set_option(transport, TRANS_OPT_REPAIR, "yes");
     ++	if (refetch)
     ++		set_option(transport, TRANS_OPT_REFETCH, "yes");
       	if (filter_options.choice) {
       		const char *spec =
       			expand_list_objects_filter_spec(&filter_options);
     @@ transport-helper.c: static int fetch_refs(struct transport *transport,
       	if (data->transport_options.update_shallow)
       		set_helper_option(transport, "update-shallow", "true");
       
     -+	if (data->transport_options.repair)
     -+		set_helper_option(transport, "repair", "true");
     ++	if (data->transport_options.refetch)
     ++		set_helper_option(transport, "refetch", "true");
      +
       	if (data->transport_options.filter_options.choice) {
       		const char *spec = expand_list_objects_filter_spec(
     @@ transport.c: static int set_git_option(struct git_transport_options *opts,
       		list_objects_filter_die_if_populated(&opts->filter_options);
       		parse_list_objects_filter(&opts->filter_options, value);
       		return 0;
     -+	} else if (!strcmp(name, TRANS_OPT_REPAIR)) {
     -+		opts->repair = !!value;
     ++	} else if (!strcmp(name, TRANS_OPT_REFETCH)) {
     ++		opts->refetch = !!value;
      +		return 0;
       	} else if (!strcmp(name, TRANS_OPT_REJECT_SHALLOW)) {
       		opts->reject_shallow = !!value;
     @@ transport.c: static int fetch_refs_via_pack(struct transport *transport,
       	args.update_shallow = data->options.update_shallow;
       	args.from_promisor = data->options.from_promisor;
       	args.filter_options = data->options.filter_options;
     -+	args.repair = data->options.repair;
     ++	args.refetch = data->options.refetch;
       	args.stateless_rpc = transport->stateless_rpc;
       	args.server_options = transport->server_options;
       	args.negotiation_tips = data->options.negotiation_tips;
     @@ transport.h: struct git_transport_options {
       	unsigned update_shallow : 1;
       	unsigned reject_shallow : 1;
       	unsigned deepen_relative : 1;
     -+	unsigned repair : 1;
     ++	unsigned refetch : 1;
       
       	/* see documentation of corresponding flag in fetch-pack.h */
       	unsigned from_promisor : 1;
     @@ transport.h: void transport_check_allowed(const char *type);
       #define TRANS_OPT_LIST_OBJECTS_FILTER "filter"
       
      +/* Refetch all objects without negotiating */
     -+#define TRANS_OPT_REPAIR "repair"
     ++#define TRANS_OPT_REFETCH "refetch"
      +
       /* Request atomic (all-or-nothing) updates when pushing */
       #define TRANS_OPT_ATOMIC "atomic"
 5:  38af2bbee79 ! 5:  01f22e784a5 t5615-partial-clone: add test for fetch --repair
     @@ Metadata
      Author: Robert Coup <robert@coup.net.nz>
      
       ## Commit message ##
     -    t5615-partial-clone: add test for fetch --repair
     +    t5615-partial-clone: add test for fetch --refetch
      
     -    Add a test for doing a repair fetch to apply a changed partial clone
     -    filter under protocol v0 and v2.
     +    Add a test for doing a refetch to apply a changed partial clone filter
     +    under protocol v0 and v2.
      
          Signed-off-by: Robert Coup <robert@coup.net.nz>
      
     @@ t/t5616-partial-clone.sh: test_expect_success 'manual prefetch of missing object
      +	git -C src push -u srv main
      +'
      +
     -+# Do partial fetch to fetch smaller files; then verify that without --repair
     ++# Do partial fetch to fetch smaller files; then verify that without --refetch
      +# applying a new filter does not refetch missing large objects. Then use
     -+# --repair to apply the new filter on existing commits. Test it under both
     ++# --refetch to apply the new filter on existing commits. Test it under both
      +# protocol v2 & v0.
     -+test_expect_success 'apply a different filter using --repair' '
     ++test_expect_success 'apply a different filter using --refetch' '
      +	git -C pc1 fetch --filter=blob:limit=999 origin &&
      +	git -C pc1 rev-list --quiet --objects --missing=print \
      +		main..origin/main >observed &&
      +	test_line_count = 4 observed &&
      +
     -+	git -C pc1 fetch --filter=blob:limit=19999 --repair origin &&
     ++	git -C pc1 fetch --filter=blob:limit=19999 --refetch origin &&
      +	git -C pc1 rev-list --quiet --objects --missing=print \
      +		main..origin/main >observed &&
      +	test_line_count = 2 observed &&
      +
      +	git -c protocol.version=0 -C pc1 fetch --filter=blob:limit=29999 \
     -+		--repair origin &&
     ++		--refetch origin &&
      +	git -C pc1 rev-list --quiet --objects --missing=print \
      +		main..origin/main >observed &&
      +	test_line_count = 0 observed
      +'
      +
     -+test_expect_success 'fetch --repair works with a shallow clone' '
     ++test_expect_success 'fetch --refetch works with a shallow clone' '
      +	git clone --no-checkout --depth=1 --filter=blob:none "file://$(pwd)/srv.bare" pc1s &&
      +	git -C pc1s rev-list --objects --missing=print HEAD >observed &&
      +	test_line_count = 6 observed &&
      +
     -+	GIT_TRACE=1 git -C pc1s fetch --filter=blob:limit=999 --repair origin &&
     ++	GIT_TRACE=1 git -C pc1s fetch --filter=blob:limit=999 --refetch origin &&
      +	git -C pc1s rev-list --objects --missing=print HEAD >observed &&
      +	test_line_count = 6 observed
      +'
 6:  cfa6dca8ef4 < -:  ----------- maintenance: add ability to pass config options
 7:  2338c15249a ! 6:  31046625987 fetch: after repair, encourage auto gc repacking
     @@ Metadata
      Author: Robert Coup <robert@coup.net.nz>
      
       ## Commit message ##
     -    fetch: after repair, encourage auto gc repacking
     +    fetch: after refetch, encourage auto gc repacking
      
     -    After invoking `fetch --repair`, the object db will likely contain many
     +    After invoking `fetch --refetch`, the object db will likely contain many
          duplicate objects. If auto-maintenance is enabled, invoke it with
          appropriate settings to encourage repacking/consolidation.
      
     @@ Documentation/fetch-options.txt: ifndef::git-pull[]
       --refmap=<refspec>::
      
       ## builtin/fetch.c ##
     -@@ builtin/fetch.c: int cmd_fetch(int argc, const char **argv, const char *prefix)
     - 	struct remote *remote = NULL;
     - 	int result = 0;
     - 	int prune_tags_ok = 1;
     -+	struct strvec auto_maint_opts = STRVEC_INIT;
     -+	int opt_val;
     - 
     - 	packet_trace_identity("fetch");
     - 
      @@ builtin/fetch.c: int cmd_fetch(int argc, const char **argv, const char *prefix)
       					     NULL);
       	}
       
      -	if (enable_auto_gc)
     --		run_auto_maintenance(verbosity < 0, NULL);
      +	if (enable_auto_gc) {
     -+		if (repair) {
     ++		if (refetch) {
      +			/*
      +			 * Hint auto-maintenance strongly to encourage repacking,
      +			 * but respect config settings disabling it.
      +			 */
     ++			int opt_val;
      +			if (git_config_get_int("gc.autopacklimit", &opt_val))
      +				opt_val = -1;
      +			if (opt_val != 0)
     -+				strvec_push(&auto_maint_opts, "gc.autoPackLimit=1");
     ++				git_config_push_parameter("gc.autoPackLimit=1");
      +
      +			if (git_config_get_int("maintenance.incremental-repack.auto", &opt_val))
      +				opt_val = -1;
      +			if (opt_val != 0)
     -+				strvec_push(&auto_maint_opts, "maintenance.incremental-repack.auto=-1");
     ++				git_config_push_parameter("maintenance.incremental-repack.auto=-1");
      +		}
     -+		run_auto_maintenance(verbosity < 0, &auto_maint_opts);
     + 		run_auto_maintenance(verbosity < 0);
      +	}
       
        cleanup:
       	string_list_clear(&list, 0);
     -+	strvec_clear(&auto_maint_opts);
     - 	return result;
     - }
      
       ## t/t5616-partial-clone.sh ##
     -@@ t/t5616-partial-clone.sh: test_expect_success 'push new commits to server for file.4.txt' '
     - # Do partial fetch to fetch smaller files; then verify that without --repair
     - # applying a new filter does not refetch missing large objects. Then use
     - # --repair to apply the new filter on existing commits. Test it under both
     --# protocol v2 & v0.
     -+# protocol v2 & v0. Check repacking auto-maintenance is kicked off.
     - test_expect_success 'apply a different filter using --repair' '
     - 	git -C pc1 fetch --filter=blob:limit=999 origin &&
     - 	git -C pc1 rev-list --quiet --objects --missing=print \
     -@@ t/t5616-partial-clone.sh: test_expect_success 'apply a different filter using --repair' '
     - 		main..origin/main >observed &&
     - 	test_line_count = 2 observed &&
     - 
     -+	GIT_TRACE2_EVENT="$(pwd)/trace.log" \
     - 	git -c protocol.version=0 -C pc1 fetch --filter=blob:limit=29999 \
     - 		--repair origin &&
     - 	git -C pc1 rev-list --quiet --objects --missing=print \
     - 		main..origin/main >observed &&
     --	test_line_count = 0 observed
     -+	test_line_count = 0 observed &&
     -+	test_subcommand git -c gc.autoPackLimit=1 -c maintenance.incremental-repack.auto=-1 maintenance run --auto --no-quiet <trace.log
     +@@ t/t5616-partial-clone.sh: test_expect_success 'fetch --refetch works with a shallow clone' '
     + 	test_line_count = 6 observed
       '
       
     - test_expect_success 'fetch --repair works with a shallow clone' '
     ++test_expect_success 'fetch --refetch triggers repacking' '
     ++	GIT_TRACE2_CONFIG_PARAMS=gc.autoPackLimit,maintenance.incremental-repack.auto &&
     ++	export GIT_TRACE2_CONFIG_PARAMS &&
     ++
     ++	GIT_TRACE2_EVENT="$PWD/trace1.event" \
     ++	git -C pc1 fetch --refetch origin &&
     ++	test_subcommand git maintenance run --auto --no-quiet <trace1.event &&
     ++	grep \"param\":\"gc.autopacklimit\",\"value\":\"1\" trace1.event &&
     ++	grep \"param\":\"maintenance.incremental-repack.auto\",\"value\":\"-1\" trace1.event &&
     ++
     ++	GIT_TRACE2_EVENT="$PWD/trace2.event" \
     ++	git -c protocol.version=0 \
     ++		-c gc.autoPackLimit=0 \
     ++		-c maintenance.incremental-repack.auto=1234 \
     ++		-C pc1 fetch --refetch origin &&
     ++	test_subcommand git maintenance run --auto --no-quiet <trace2.event &&
     ++	grep \"param\":\"gc.autopacklimit\",\"value\":\"0\" trace2.event &&
     ++	grep \"param\":\"maintenance.incremental-repack.auto\",\"value\":\"-1\" trace2.event &&
     ++
     ++	GIT_TRACE2_EVENT="$PWD/trace3.event" \
     ++	git -c protocol.version=0 \
     ++		-c gc.autoPackLimit=1234 \
     ++		-c maintenance.incremental-repack.auto=0 \
     ++		-C pc1 fetch --refetch origin &&
     ++	test_subcommand git maintenance run --auto --no-quiet <trace3.event &&
     ++	grep \"param\":\"gc.autopacklimit\",\"value\":\"1\" trace3.event &&
     ++	grep \"param\":\"maintenance.incremental-repack.auto\",\"value\":\"0\" trace3.event
     ++'
     ++
     + test_expect_success 'partial clone with transfer.fsckobjects=1 works with submodules' '
     + 	test_create_repo submodule &&
     + 	test_commit -C submodule mycommit &&
 8:  20942562a66 ! 7:  f923a06aab5 doc/partial-clone: mention --repair fetch option
     @@ Metadata
      Author: Robert Coup <robert@coup.net.nz>
      
       ## Commit message ##
     -    doc/partial-clone: mention --repair fetch option
     +    doc/partial-clone: mention --refetch fetch option
      
     -    Document it for partial clones as a means to refetch with a new filter.
     +    Document it for partial clones as a means to apply a new filter.
      
          Signed-off-by: Robert Coup <robert@coup.net.nz>
      
     @@ Documentation/technical/partial-clone.txt: Fetching Missing Objects
         currently fetches all objects referred to by the requested objects, even
         though they are not necessary.
       
     -+- Fetching with `--repair` will request a complete new filtered packfile from
     ++- Fetching with `--refetch` will request a complete new filtered packfile from
      +  the remote, which can be used to change a filter without needing to
      +  dynamically fetch missing objects.
       

From: Robert Coup <robert@coup.net.nz>

Add a specific initializer for the noop fetch negotiator. This is introduced to support allowing partial clones to skip commit negotiation when performing a "refetch".

Signed-off-by: Robert Coup <robert@coup.net.nz>
---
 fetch-negotiator.c | 5 +++++
 fetch-negotiator.h | 8 ++++++++
 2 files changed, 13 insertions(+)

From: Robert Coup <robert@coup.net.nz>

Add a refetch option to fetch-pack to force a full fetch. Use when applying a new partial clone filter to refetch all matching objects.

Signed-off-by: Robert Coup <robert@coup.net.nz>
---
 Documentation/git-fetch-pack.txt | 4 ++++
 builtin/fetch-pack.c             | 4 ++++
 remote-curl.c                    | 6 ++++++
 3 files changed, 14 insertions(+)

From: Robert Coup <robert@coup.net.nz>

Allow a "refetch" where the contents of the local object store are ignored and a full fetch is performed, not attempting to find or negotiate common commits with the remote.

A key use case is to apply a new partial clone blob/tree filter and refetch all the associated matching content, which would otherwise not be transferred when the commit objects are already present locally.

Signed-off-by: Robert Coup <robert@coup.net.nz>
---
 fetch-pack.c | 46 +++++++++++++++++++++++++++++-----------------
 fetch-pack.h |  1 +
 2 files changed, 30 insertions(+), 17 deletions(-)

From: Robert Coup <robert@coup.net.nz>

Teach fetch and transports the --refetch option to force a full fetch without negotiating common commits with the remote. Use when applying a new partial clone filter to refetch all matching objects.

Signed-off-by: Robert Coup <robert@coup.net.nz>
---
 Documentation/fetch-options.txt |  9 +++++++++
 builtin/fetch.c                 | 15 ++++++++++++++-
 transport-helper.c              |  3 +++
 transport.c                     |  4 ++++
 transport.h                     |  4 ++++
 5 files changed, 34 insertions(+), 1 deletion(-)

"Robert Coup via GitGitGadget" <gitgitgadget@gmail.com> writes:

I guess the existing --unshallow has the same problem, but it strikes me odd that these aren't doing a bog-standard OPT_BOOL(), with default value of "false", like, say "--update-head-ok" does.

That will naturally support things like

	git fetch --refetch --no-refetch

where a later option overrides what an earlier option did.

Hi Junio,

On Fri, 4 Mar 2022 at 21:19, Junio C Hamano <gitster@pobox.com> wrote:

Ah, I literally copied the unshallow one since it seemed boolean-ish and that's what I wanted. I'll look into it.

Thanks, Rob :)

Robert Coup <robert@coup.net.nz> writes:

You may (or may not --- I didn't look) find options that need to be OPT_BOOL() other than "unshallow" one.

While it would be excellent to add a separate "preliminary clean-up" step before you add OPT_BOOL("refetch") to fix them to use OPT_BOOL() instead of OPT_SET_INT(), that will add extra review cycles to the series by extending the scope of it.

It is OK to leave them, in addition to the new one you are adding, to use the old and a bit incorrect pattern, as long as you leave a prominent note that we need to clean them up later.

Thanks.

Hi Junio,

On Mon, 7 Mar 2022 at 17:27, Junio C Hamano <gitster@pobox.com> wrote:

I think for this series I will just do a re-roll updating refetch to use OPT_BOOL, and I can do a separate series to look for other existing booleans using OPT_SET_INT.

Rob :)

From: Robert Coup <robert@coup.net.nz>

Add a test for doing a refetch to apply a changed partial clone filter under protocol v0 and v2.

Signed-off-by: Robert Coup <robert@coup.net.nz>
---
 t/t5616-partial-clone.sh | 52 +++++++++++++++++++++++++++++++++++++++-
 1 file changed, 51 insertions(+), 1 deletion(-)

From: Robert Coup <robert@coup.net.nz>

After invoking `fetch --refetch`, the object db will likely contain many duplicate objects. If auto-maintenance is enabled, invoke it with appropriate settings to encourage repacking/consolidation.

* gc.autoPackLimit: unless this is set to 0 (disabled), override the
  value to 1 to force pack consolidation.
* maintenance.incremental-repack.auto: unless this is set to 0, override
  the value to -1 to force incremental repacking.

Signed-off-by: Robert Coup <robert@coup.net.nz>
---
 Documentation/fetch-options.txt |  3 ++-
 builtin/fetch.c                 | 19 ++++++++++++++++++-
 t/t5616-partial-clone.sh        | 29 +++++++++++++++++++++++++++++
 3 files changed, 49 insertions(+), 2 deletions(-)

From: Robert Coup <robert@coup.net.nz>

Document it for partial clones as a means to apply a new filter.

Signed-off-by: Robert Coup <robert@coup.net.nz>
---
 Documentation/technical/partial-clone.txt | 3 +++
 1 file changed, 3 insertions(+)

"Robert Coup via GitGitGadget" <gitgitgadget@gmail.com> writes:

Hi Robert,

This is my first time sending a review to the list, so forgive me for any mistakes I make or conventions missed. Feedback about my review would be well appreciated!

Overall I think your patch is well written and the implementation accomplishes what you describe in your cover letter, however, I would like to discuss another possible design I thought of. Currently, the user has to know to run ‘--refetch’ after changing the partial clone filter configuration in order to fetch the commits that match the new filter. Ideally I believe this behavior should be default so therefore instead of adding an option, if git knew what filter was last used in the fetch, it could automatically ‘refetch’ everything if there is a change between the last used filter and the default filter. I’m not sure if the config is the best location to store the last used filter, but we can use it as an example for now. The tradeoff here is balancing between having an additional config variable and having the user know to specify a parameter to fetch after changing the config. And in the future, if there are other use cases for needing a fetch that bypasses commit negotiation (I know you described one such possible use case in v2), the ‘--refetch’ option can easily be readded to hook into this patch.

Looking forward to hearing your thoughts!

Hi Calvin,

On Wed, 9 Mar 2022 at 00:27, Calvin Wan <calvinwan@google.com> wrote:

I'm as new as you to contributing to Git :-)

Thanks! I think if you're completely content you can add your Reviewed-By as described at https://git-scm.com/docs/SubmittingPatches#commit-trailers

So, if you do a partial clone using `git clone --filter=...` then the filter is saved into the config at `remote.<name>.partialclonefilter` and is re-used by default for subsequent fetches from that remote. But there's nothing to stop `git fetch --filter=...` being run multiple times with different filters to carefully setup a repository for a particular use case, or any notion that there has to be "one" filter in place for a remote.

Running `git fetch --filter=...` doesn't update the remote's partial clone filter in the config, and IMO it shouldn't for the above reason.

I think having `git config remote.<name>.partialclonefilter <new-filter>` print out something to the user along the lines of "Your change to/removal of the filter won't fetch in additional objects associated with existing commits, you can do this with `fetch --refetch <remote>`" could be helpful, but after a very quick look I can't see anything like that at the moment for other config settings (ie. no plumbing in place to easily reuse), and I'm not motivated to add such plumbing.

Cheers,

Rob :)

Robert Coup <robert@coup.net.nz> writes:

The way I read Calvin's suggestion was that you won't allow such a random series of "git fetch"es without updating the "this is the filter that is consistent with the contents of this repository" record, which will lead to inconsistencies. I.e.

 - we must maintain the "filter that is consistent with the contents
   of this repository", which this series does not do, but we should.

 - the "--refetch" is unnecessary and redundant, as long as such a
   record is maintained; when a filter settings changes, we should
   do the equivalent of "--refetch" automatically.

IOW, ...

... isn't "git fetch --fitler" that does not update the configured filter (and does not do a refetch automatically) a bug that made the "refetch" necessary in the first place?

Or perhaps I read Calvin incorrectly?

On 3/9/2022 1:32 PM, Junio C Hamano wrote:

 > - we must maintain the "filter that is consistent with the contents
 >   of this repository", which this series does not do, but we should.

If this is possible/reasonably implementable then...

 > - the "--refetch" is unnecessary and redundant, as long as such a
 >  record is maintained; when a filter settings changes, we should
 >  do the equivalent of "--refetch" automatically.

... this is the logical conclusion that can be drawn.

There are two cases to be considered here:
1. The user changes the filter config using "git config"
2. The user runs "git fetch --filter"

The first case is the use case advocated by Robert for "--refetch". I originally suggested saving the last used filter into the config and if the last used filter != the configured filter, then fetch automatically "refetches" everything.

The second use case is the one Junio believes I am referring to, however, whether to classify this as a bug or feature, I am uncertain. Like above, I suggested saving this as the last used filter to be used as a comparison to the configured filter. Setting this filter as the configured filter can be a separate discussion to be had because I can see pros/cons for it.

Ultimately the expectation is that if I run "git fetch" without --filter, then I will fetch based on the config filter. And if I have previously run "git fetch" with the same filter, whether through the config or my own filter, then Git will only fetch the latest refs/objects.

On Wed, Mar 9, 2022 at 1:32 PM Junio C Hamano <gitster@pobox.com> wrote:

Hi,

On Wed, 9 Mar 2022 at 21:32, Junio C Hamano <gitster@pobox.com> wrote:

I don't think we should strive to keep this "consistency" —

— we don't know how much data has been pulled in by fetches from different promisor and non-promisor remotes (past & present); or dynamically faulted in through branch switching or history exploration. And I can't see any particular benefit in attempting to keep track of that?

Ævar suggested in future maybe we could figure out which commits a user definitively has all the blobs & trees for and refetch could negotiate from that position to improve efficiency: nothing in this series precludes such an enhancement.

I don't believe it's a bug. A fairly obvious partial clone example I've used before on repos where I want the commit history but not all the associated data (especially when the history is littered with giant blobs I don't care about):

  git clone example.com/myrepo --filter=blob:none
  # does a partial clone with no blobs
  # checkout faults in the blobs present at HEAD in bulk to populate
the working tree
  git config --unset remote.origin.partialclonefilter
  # going forward, future fetches include all associated blobs for new commits

Getting all the blobs for all history is something I'm explicitly trying not to do in this example, but if the next fetch from origin automatically did a "refetch" after I removed the filter that's exactly what would happen.

We don't expect users to update `diff.algorithm` in config to run a minimal diff: using the `--diff-algorithm=` option on the command line overrides the config. And the same philosophy applies with fetch: `remote.<name>.partialclonefilter` provides the default filter for fetches, and a user can override it via `git fetch --filter=`. To me this is how Git commands are expected to work.

Partial clones are still relatively new and advanced, and I don't believe we should try and over-predict too much what the correct behaviour is for a user.

I'd be happy adding something to the documentation for the `remote.<name>.partialclonefilter` config setting to explain that changing or removing the filter won't backfill the local object DB and the user would need `fetch --refetch` for that.

Thanks, Rob :)

Hi Robert,

Documentation for the config setting is an acceptable solution! Apologies for the late response -- wanted to wait and see if anyone else on the list had any last thoughts. Also I noticed you were hoping that Jonathan Tan could take a look at your patch on the What's Cooking thread. Before I sent my first review out, I discussed your patch with him so he's been briefed.

Reviewed-by: Calvin Wan <calvinwan@google.com>

On Thu, Mar 10, 2022 at 6:29 AM Robert Coup <robert@coup.net.nz> wrote:

Hi Calvin

On Mon, 21 Mar 2022 at 17:58, Calvin Wan <calvinwan@google.com> wrote:

Great :)

Thank you.

@Junio - I'm on leave for the remainder of this week, so expect the re-roll sometime next week.

Rob :)

If a filter is changed on a partial clone repository, for example from blob:none to blob:limit=1m, there is currently no straightforward way to bulk-refetch the objects that match the new filter for existing local commits. This is because the client will report commits as "have" during fetch negotiation and any dependent objects won't be included in the transferred pack. Another use case is discussed at [1].

This patch series introduces a --refetch option to fetch & fetch-pack to enable doing a full fetch without performing any commit negotiation with the remote, as a fresh clone does. It builds upon cbe566a071 ("negotiator/noop: add noop fetch negotiator", 2020-08-18).

 * Using --refetch will produce duplicated objects between the existing and
   newly fetched packs, but maintenance will clean them up when it runs
   automatically post-fetch (if enabled).
 * If a user fetches with --refetch applying a more restrictive partial
   clone filter than previously (eg: blob:limit=1m then blob:limit=1k) the
   eventual state is a no-op, since any referenced object already in the
   local repository is never removed. More advanced repacking which could
   improve this scenario is currently proposed at [2].

[1] https://lore.kernel.org/git/aa7b89ee-08aa-7943-6a00-28dcf344426e@syntevo.com/ [2] https://lore.kernel.org/git/21ED346B-A906-4905-B061-EDE53691C586@gmail.com/

Changes since v3:

 * Mention fetch --refetch in the remote.<name>.partialclonefilter
   documentation.

Changes since v2:

 * Changed the name from "repair" to "refetch". While it's conceivable to
   use it in some object DB repair situations that's not the focus of these
   changes.
 * Pass config options to maintenance via GIT_CONFIG_PARAMETERS
 * Split out auto-maintenance to a separate & more robust test
 * Minor fixes/improvements from reviews by Junio & Ævar

Changes since RFC (v1):

 * Changed the name from "refilter" to "repair"
 * Removed dependency between server-side support for filtering and repair
 * Added a test case for a shallow clone
 * Post-fetch auto maintenance now strongly encourages
   repacking/consolidation

Reviewed-by: Calvin Wan calvinwan@google.com

Robert Coup (7):
  fetch-negotiator: add specific noop initializer
  fetch-pack: add refetch
  builtin/fetch-pack: add --refetch option
  fetch: add --refetch option
  t5615-partial-clone: add test for fetch --refetch
  fetch: after refetch, encourage auto gc repacking
  docs: mention --refetch fetch option

 Documentation/config/remote.txt           |  6 +-
 Documentation/fetch-options.txt           | 10 +++
 Documentation/git-fetch-pack.txt          |  4 ++
 Documentation/technical/partial-clone.txt |  3 +
 builtin/fetch-pack.c                      |  4 ++
 builtin/fetch.c                           | 34 +++++++++-
 fetch-negotiator.c                        |  5 ++
 fetch-negotiator.h                        |  8 +++
 fetch-pack.c                              | 46 ++++++++-----
 fetch-pack.h                              |  1 +
 remote-curl.c                             |  6 ++
 t/t5616-partial-clone.sh                  | 81 ++++++++++++++++++++++-
 transport-helper.c                        |  3 +
 transport.c                               |  4 ++
 transport.h                               |  4 ++
 15 files changed, 197 insertions(+), 22 deletions(-)

base-commit: abf474a5dd901f28013c52155411a48fd4c09922
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1138%2Frcoup%2Frc-partial-clone-refilter-v4
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1138/rcoup/rc-partial-clone-refilter-v4
Pull-Request: https://github.com/gitgitgadget/git/pull/1138

Range-diff vs v3:

 1:  96a75be3d8a = 1:  6cd6d4a59f6 fetch-negotiator: add specific noop initializer
 2:  04ca6a07f85 = 2:  03f0de3d28c fetch-pack: add refetch
 3:  879d30c4473 = 3:  f7942344ff8 builtin/fetch-pack: add --refetch option
 4:  a503b98f333 = 4:  78501bbf281 fetch: add --refetch option
 5:  01f22e784a5 = 5:  6c17167ac1e t5615-partial-clone: add test for fetch --refetch
 6:  31046625987 = 6:  28c07219fd8 fetch: after refetch, encourage auto gc repacking
 7:  f923a06aab5 ! 7:  da1e6de7a9f doc/partial-clone: mention --refetch fetch option
     @@ Metadata
      Author: Robert Coup <robert@coup.net.nz>
      
       ## Commit message ##
     -    doc/partial-clone: mention --refetch fetch option
     +    docs: mention --refetch fetch option
      
     -    Document it for partial clones as a means to apply a new filter.
     +    Document it for partial clones as a means to apply a new filter, and
     +    reference it from the remote.<name>.partialclonefilter config parameter.
      
          Signed-off-by: Robert Coup <robert@coup.net.nz>
      
     + ## Documentation/config/remote.txt ##
     +@@ Documentation/config/remote.txt: remote.<name>.promisor::
     + 	objects.
     + 
     + remote.<name>.partialclonefilter::
     +-	The filter that will be applied when fetching from this
     +-	promisor remote.
     ++	The filter that will be applied when fetching from this	promisor remote.
     ++	Changing or clearing this value will only affect fetches for new commits.
     ++	To fetch associated objects for commits already present in the local object
     ++	database, use the `--refetch` option of linkgit:git-fetch[1].
     +
       ## Documentation/technical/partial-clone.txt ##
      @@ Documentation/technical/partial-clone.txt: Fetching Missing Objects
         currently fetches all objects referred to by the requested objects, even

From: Robert Coup <robert@coup.net.nz>

Add a specific initializer for the noop fetch negotiator. This is introduced to support allowing partial clones to skip commit negotiation when performing a "refetch".

Signed-off-by: Robert Coup <robert@coup.net.nz>
---
 fetch-negotiator.c | 5 +++++
 fetch-negotiator.h | 8 ++++++++
 2 files changed, 13 insertions(+)

From: Robert Coup <robert@coup.net.nz>

Allow a "refetch" where the contents of the local object store are ignored and a full fetch is performed, not attempting to find or negotiate common commits with the remote.

A key use case is to apply a new partial clone blob/tree filter and refetch all the associated matching content, which would otherwise not be transferred when the commit objects are already present locally.

Signed-off-by: Robert Coup <robert@coup.net.nz>
---
 fetch-pack.c | 46 +++++++++++++++++++++++++++++-----------------
 fetch-pack.h |  1 +
 2 files changed, 30 insertions(+), 17 deletions(-)

On Mon, Mar 28 2022, Robert Coup via GitGitGadget wrote:

FWIW it's not clear to me re earlier comments on earlier iterations whether the "noop fetch" is per-se wanted for this feature for some reason, or if it's just much easier to implement than doing what I suggested in: https://lore.kernel.org/git/220228.86ee3m39jf.gmgdl@evledraar.gmail.com/

I don't think such a thing should hold this series up, but as it would be a bit kinder to servers I think it's worth at least noting in the commit message what's desired per-se here, v.s. what's just needed for the convenience of implementation.

I.e. when this series was in an earlier iteration the scope was to repair repository corruption, which I pointed out to you it really couldn't do without more wider changes to the object store management, and at that point having it be NOOP definitely makes sense. The object lookups etc. take shortcuts that "fsck" wouldn't do, so we could be negotiating on the basis of corrupt content.

But now that it's a "fetch what's missing" wouldn't it make more sense to descend from our otherwise-negotiated tips, and find the OIDs that are "complete", if any, and negotiate with those?

Which again, I think it's fine to say "yeah, that would be ideal, but this is easier". I'm just checking if I'm missing some subtlety here...

nit: remove the {} here nit: this double-indented if can just be one if-statement nit: don't compare against NULL, use ! instead.

nit: This function has only two callers, perhaps it's clearer to do do this "early abort" in those calls?

More needless {}

Here everything_local() is doing what I suggested for mark_complete_and_common_ref() above, i.e. we check args->refetch first.

This one doesn't have braces (good), unlike do_fetch_pack()

Hi Ævar,

< just when I was thinking I'm done... ;-) >

On Thu, 31 Mar 2022 at 16:17, Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

The noop-fetch is an implementation detail IMO, and can be improved if/when someone is motivated later.

Yes, doing some per-commit negotiation is conceptually nicer. The user-facing alternative at this point is basically running "clone" in some form; or temporarily moving aside the object DB and running fetch. Both those (and the new approach) all end up putting the same load on the server.

I can note it in the commit message.

That was never _my_ motivation, I was a bit eager in reflecting some comments I received suggesting it would be useful for that as well.

Yeah, that would be ideal, but this is easier. AFAICS I'm not putting in place any barriers to making it smarter later.

Is the common interpretation for nits "Do another re-roll"; "If you're doing another re-roll"; or "For future reference"?

I discussed similar in https://lore.kernel.org/git/CACf-nVePhtm_HAzAKzcap0E8kiyyEJPY_+N+bbPcYPVUkjweFg@mail.gmail.com/ but yes, I think you're right about mark_complete_and_common_ref(). Will see what it looks like.

Thanks, Rob.

From: Robert Coup <robert@coup.net.nz>

Add a refetch option to fetch-pack to force a full fetch. Use when applying a new partial clone filter to refetch all matching objects.

Signed-off-by: Robert Coup <robert@coup.net.nz>
---
 Documentation/git-fetch-pack.txt | 4 ++++
 builtin/fetch-pack.c             | 4 ++++
 remote-curl.c                    | 6 ++++++
 3 files changed, 14 insertions(+)

From: Robert Coup <robert@coup.net.nz>

Add a test for doing a refetch to apply a changed partial clone filter under protocol v0 and v2.

Signed-off-by: Robert Coup <robert@coup.net.nz>
---
 t/t5616-partial-clone.sh | 52 +++++++++++++++++++++++++++++++++++++++-
 1 file changed, 51 insertions(+), 1 deletion(-)

On Mon, Mar 28 2022, Robert Coup via GitGitGadget wrote:

Is 19999 just "arbitrary big number" here?

Does this test_line_count *really* want to be = 0, or does this mean test_must_be_empty?

I.e. are we expecting content here, just not ending in a \n, or nothing at all?

Why the GIT_TRACE=1? Seems to not be used.

Hi,

On Thu, 31 Mar 2022 at 16:22, Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

A selected big number so we can change it and have different objects match.

I'm expecting no missing objects after a refetch with a new filter that matches all the objects in the repo.

Ah, extraneous debugging on my part.

Thanks, Rob :)

From: Robert Coup <robert@coup.net.nz>

Teach fetch and transports the --refetch option to force a full fetch without negotiating common commits with the remote. Use when applying a new partial clone filter to refetch all matching objects.

Signed-off-by: Robert Coup <robert@coup.net.nz>
---
 Documentation/fetch-options.txt |  9 +++++++++
 builtin/fetch.c                 | 15 ++++++++++++++-
 transport-helper.c              |  3 +++
 transport.c                     |  4 ++++
 transport.h                     |  4 ++++
 5 files changed, 34 insertions(+), 1 deletion(-)

On Mon, Mar 28 2022, Robert Coup via GitGitGadget wrote:

Re my comment on negotiation specifics in 2/7, this documentation is really over-promising depending on what the answer to that is: https://lore.kernel.org/git/220331.86o81mp2w1.gmgdl@evledraar.gmail.com/

I.e. instead of saying that we WILL fetch all objects "just like a clone" shouldn't we have less focus on implementation details here, and assure the user that we'll make their object store "complete" as though --filter hadn't been used, without going into the specifics of what'll happen over the wire?

One too many \n here.

Hi Ævar,

On Thu, 31 Mar 2022 at 16:20, Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

That's not quite the case though: we'll make their object db complete as if a fresh clone with any specified --filter (including none) from that remote has been done.

IMO explaining what will happen and to expect (particularly as this is _not_ transferring some sort of object-delta-set, clone transfers can be big) is a good thing. If it improves later; then change the docs.

Alternatively, rewording along the lines of "it will fix up your object db to match a changed filter" is implying that only the missing bits will be fetched.

Thanks, Rob.

From: Robert Coup <robert@coup.net.nz>

After invoking `fetch --refetch`, the object db will likely contain many duplicate objects. If auto-maintenance is enabled, invoke it with appropriate settings to encourage repacking/consolidation.

* gc.autoPackLimit: unless this is set to 0 (disabled), override the
  value to 1 to force pack consolidation.
* maintenance.incremental-repack.auto: unless this is set to 0, override
  the value to -1 to force incremental repacking.

Signed-off-by: Robert Coup <robert@coup.net.nz>
---
 Documentation/fetch-options.txt |  3 ++-
 builtin/fetch.c                 | 19 ++++++++++++++++++-
 t/t5616-partial-clone.sh        | 29 +++++++++++++++++++++++++++++
 3 files changed, 49 insertions(+), 2 deletions(-)

On Mon, Mar 28 2022, Robert Coup via GitGitGadget wrote:

nit: add a \n after this.

nit: don't compare against 0 or null,  just !opt_val

Isn't this whole thing also clearer as:

	int &forget;

        if (git_conf...(..., &forget))
		git_config_push_parameter("gc.autoPackLimit=1");

Maybe I haven't eyeballed this enough, but aren't you ignoring explicit gc.autoPackLimit=0 configuration? Whereas what you seem to want is "set this config unlress the user has it set", for which we only need to check the git_config...(...) return value, no?

hrm, do we really need to set both of these these days (not saying we don't, just surprised). I.e. both gc.* an maintenance.* config.

*skims the code*

Urgh, yes? too_many_packs() seems to check gc.* only, but incremental_repack_auto_condition() check this variable... :(

Nit: Can we use GIT_CONFIG_KEY_* et al for this these days, or do we
still need this trace2 thingy?

Hi Ævar,

On Thu, 31 Mar 2022 at 16:33, Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

I did this since 0 has a specific meaning ("Setting this to 0 disables"), it's not just false-y in this context. Tomayto, tomahto?

What I'm trying to achieve: if the user has not disabled auto-packing (autoPackLimit=0), then pass autoPackLimit=1 to the subprocess to encourage repacking. Context/why: so we don't 2x the object store size and not even attempt to repack it now, rather than at some unspecified point in the future. Maybe.

How the code achieves it:
  load autoPackLimit into opt_val
  if autoPackLimit is not specified in config: set opt_val to -1
  if opt_val is not 0: pass autoPackLimit=1 to the subprocess

AFAICT if we just if(git_config_get_int()) then if they haven't set it at all in config, we wouldn't encourage repacking in the subprocess. Which isn't what I'm trying to achieve.

Yes.

I copied a pattern existing tests are using.

Thanks, Rob.

From: Robert Coup <robert@coup.net.nz>

Document it for partial clones as a means to apply a new filter, and reference it from the remote.<name>.partialclonefilter config parameter.

Signed-off-by: Robert Coup <robert@coup.net.nz>
---
 Documentation/config/remote.txt           | 6 ++++--
 Documentation/technical/partial-clone.txt | 3 +++
 2 files changed, 7 insertions(+), 2 deletions(-)

"Robert Coup via GitGitGadget" <gitgitgadget@gmail.com> writes:

Good advice to add.

Will replace. I think we've seen this topic enough times and it looked reasonably well done. Let's mark it for 'next' unless we hear objections soonish.