Subject: Re: [PATCH v2] builtin/fetch: skip unnecessary tasks when using --negotiate-only

From: Junio C Hamano <[email-masked]>
Date: Mon, 20 Dec 2021 11:56:30 -0800

Glen Choo <[email-masked]> writes:

>> By the way, do not move the check about the number of negotiation
>> tips from the original location.  That check, or its location, have
>> nothing to do with what you want to do in this patch, which is "do
>> not gc or update the graph file if we are not fetching".  It is
>> better to leave unrelated changes out of the patch.
>
> Ah, I see that it's not easy to tell whether or not the behavior is
> correct after that line is moved. I'll avoid doing this in the future.
>
> I still think that it is cleaner to move the negotiation_tip.nr check.
> Should I do this in a follow-up patch?

I am not seeing what makes it cleaner to have it early or at the
current position, but with "It is cleaner to do tip.nr check early
because ...", in a separate patch, it may become obvious.  But let's
not do it in this patch.

> I hope Jonathan can chime in to confirm whether or not users want/need
> to invoke "--negotiate-only".

Yeah, I knew that this was "internal implementation detail", but
then perhaps we know that a sane developer who knows what they are
doing will never write combination of it with recurse-submodule
option?  If so, we'd catch a notice developer breaking the system by
having a sanity check by detecting it as an error, no?