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

[Glen Choo <chooglen@google.com>]

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

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

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