Re: [PATCH v2 0/8] fetch: add repair: full refetch without negotiation (was: "refiltering")

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Feb 28, 2022, 22:20 UTC
Message-ID: <220228.86ee3m39jf.gmgdl@evledraar.gmail.com>
In-Reply-To: <CACf-nVcy8xsf+STJoE5vwcsUauHRcR5wmwmCfnUnSW=4jDcgYQ@mail.gmail.com>

On Mon, Feb 28 2022, Robert Coup wrote:

Show 17 quoted lines

> 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:
>> I realize this was probably based on feedback on v1 (I didn't go back
>> and re-read it, sorry).
>
> 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.

*nod*

Show 11 quoted lines

>> 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?
>
> 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

*nod*

Show 13 quoted lines

>> 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?
>
> 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.

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.

Show 7 quoted lines

>> 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".
>
> 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
> :-)

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.

Show 13 quoted lines

>> 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.
>
> 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.

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.

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