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

From: Glen Choo <chooglen@google.com>
Date: Dec 20, 2021, 20:54 UTC
Message-ID: <kl6lo85byp3b.fsf@chooglen-macbookpro.roam.corp.google.com>
In-Reply-To: <xmqq4k73vymp.fsf@gitster.g>

Junio C Hamano <gitster@pobox.com> writes:

Show 18 quoted lines

> Glen Choo <chooglen@google.com> 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.

Noted, thanks!

Show 8 quoted lines

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

Hm, I suppose that is true, though I have some reservations. In particular, I'm not sure if the argument you are making is intended to be specific to this combination of options, or all incompatible options in general.

I think it might make sense to check for this specific combination of "--negotiate-only" and "--recurse-submodules" because where the stakes are low and behavior is reasonably simple to understand, though the payoff is also pretty low.

In the more general case of "Does it *always* make sense to check for an invalid combination of options?", I find your argument a bit troubling because it seems to imply that anything we add to the Git CLI should be treated as if users will depend on it. This seems like a needless burden (or even an antipattern) if we need to fork a Git process purely as an implementation detail, but we have to treat these internal CLI options as user-facing.

But maybe I am misunderstanding how Git treats CLI options in general? We don't ever really hide CLI options, even if they are only internal. There is PARSE_OPT_HIDDEN, but those still show up in usage and documentation [1]. So we actually do want users to know about these implementation details?

Presumably, the thought process goes something like this: if we add an option, a user *could* find a use for it (or it might accidentally conflict with a user's flags), even if we never intended it for end user consumption. Thus we need to treat all CLI options with care.

Is this a more accurate description of how we think about CLI options?

[1] This statement doesn't apply to the -- commands (like submodule--helper). Those are 'truly' hidden because they don't have public docs AFAIK.