Junio C Hamano <gitster@xxxxxxxxx> writes: > Glen Choo <chooglen@xxxxxxxxxx> 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.
