[prev in list] [next in list] [prev in thread] [next in thread] 

List:       git
Subject:    Re: [PATCH v2 0/8] fetch: add repair: full refetch without negotiation (was: "refiltering")
From:       Ævar Arnfjörð Bjarmason <avarab () gmail ! com>
Date:       2022-02-28 22:20:59
Message-ID: 220228.86ee3m39jf.gmgdl () evledraar ! gmail ! com
[Download RAW message or body]


On Mon, Feb 28 2022, Robert Coup wrote:

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

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

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

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

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

[prev in list] [next in list] [prev in thread] [next in thread]