Re: [PATCH v2 2/8] fetch-pack: add repairing

[Robert Coup <robert@coup.net.nz>]

Hi Junio,

On Fri, 25 Feb 2022 at 06:46, Junio C Hamano <gitster@pobox.com> wrote:
>
> "Robert Coup via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > @@ -694,6 +696,9 @@ static void mark_complete_and_common_ref(struct fetch_negotiator *negotiator,
> >
> >       save_commit_buffer = 0;
> >
> > +     if (args->repair)
> > +             return;
> > +
>
> 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?

Good spotting, thank you.

>
> > @@ -1027,9 +1032,6 @@ static struct ref *do_fetch_pack(struct fetch_pack_args *args,
> >       struct fetch_negotiator negotiator_alloc;
> >       struct fetch_negotiator *negotiator;
> >
> > -     negotiator = &negotiator_alloc;
> > -     fetch_negotiator_init(r, negotiator);
>
> 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.

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

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

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?

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

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