Re: [PATCH v2] Doc: document push.recurseSubmodules=only

From: Glen Choo <chooglen@google.com>
Date: Nov 16, 2022, 21:21 UTC
Message-ID: <kl6lpmdmvbnk.fsf@chooglen-macbookpro.roam.corp.google.com>
In-Reply-To: <20221115184702.169258-1-jonathantanmy@google.com>

Jonathan Tan <jonathantanmy@google.com> writes:

Show 13 quoted lines

>> When we pass this magic, undocumented value, "git push" will warn about
>> about "only" and override it with "on-demand". We always pass it when we
>> recurse into submodules, and we assume that no user will pass it, thus
>> we get the warning iff we are recursing into submodules.
>> 
>> In that case, it sounds like "--recurse-submodules=only-is-on-demand" is
>> a synonym for "this is a submodule that is being recursed into". In that
>> case, wouldn't it be more self-documenting to have a hidden CLI flag
>> that expresses exactly that ? e.g. we could add a PARSE_OPT_HIDDEN flag
>> called "--is-recursing" and check that boolean value. This seems clearer
>> to me at least.
>
> Hmm...--recurse-submodules=only-is-on-demand is hidden too, right?

I suppose so, but as a matter of personal taste, when encountering a hidden CLI option, I'd prefer to see the option's documentation when I grep:

  OPT_HIDDEN_BOOL(0, "is-recursing", &is_recursing, "internal only,
    override recurseSubmodules = only")

than:

  if (!strcmp(arg, "only-is-on-demand")) {
    /* etc */
  }

>                                                                    One
> advantage of doing this instead of a separate arg is that neither the
> caller nor "git push" needs to think about what happens if both --recurse-
> submodules=something and --is-recursing are both specified.

That's true, and I suppose it prevents the temptation to reuse --is-recursing for things it wasn't meant for. But in that case, couldn't we say the same thing if we renamed "--is-recursing" to "--recurse-only-is-on-demand"?

At any rate, do treat this as just a suggestion. I won't block this if both you and Taylor find this clear enough :)