Hi Junio, On Fri, 25 Feb 2022 at 06:46, Junio C Hamano <gitster@xxxxxxxxx> wrote: > > "Robert Coup via GitGitGadget" <gitgitgadget@xxxxxxxxx> 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 :)
