Re: [PATCH] usage: clarify --recurse-submodules as a boolean

[Emily Shaffer <nasamuffin@google.com>]

On Fri, Apr 07, 2023 at 04:47:02PM -0700, Junio C Hamano wrote:
> 
> Emily Shaffer <nasamuffin@google.com> writes:
> 
> > `git switch` `git checkout`, `git reset`, and `git read-tree` allow a user to choose to
> > recurse into submodules. All three of these commands' short usage seems
> > to indicate that `--recurse-submodules` should take an argument. In
> > practice, ...
> 
> Did you add 'git switch' at the last minute in so much of a hurry
> that you forgot to put a comma after it, or rewrap the paragraph?
> ;-)

It was 'git checkout', if you must know ;) and in such a hurry that I
also neglected to s/three/four/g. Will fix it with the reroll.

> 
> I do agree with you that "git checkout -h" and "git reset -h" that
> list
> 
> 	--recurse-submodules[=<checkout>]
> 	--recurse-submodules[=<reset>]
> 
> are being unnecessarily confusing by not saying anything about what
> these placeholders are to be filled with.  
> 
> This however is a breaking change.  Even though there is no hint
> that <checkout> and <reset> placeholders above take either Boolean
> true or false in the documentation, they may have picked up a habit
> to use the undocumented form from some random website.

Ah, yeah, I see what you mean, from my locally-built version:

  g checkout --recurse-submodules=false master
  error: option `recurse-submodules' takes no value

> I am not
> sure it is safe to change the behaviour right under them, like this
> patch does, and I wonder if we should do this in two steps, with its
> first step doing:
> 
>  * "--[no-]recurse-submodules" from the command line gets no
>    warning, as that is the way we recommend users to use the
>    feature.
> 
>  * "--recurse-submodules=$true" and "--recurse-submodules=$false"
>    (for various ways to spell true and false) get warning that tells
>    the users that versions of Git in a year or more in the future
>    will stop supporting the Boolean argument form of the option and
>    instructs them to use "--[no-]recurse-submodules" instead.
> 
> We may have to also mention in the documentation that historically
> the code accepted a Boolean value as an optional argument for the
> option by mistake, but we are deprecating that form.
> 
> And after the second step, the code will end up looking like what
> this patch shows.

I'd be happy to do so with a reroll, probably on Monday. It's true that
while these are user-facing commands which we don't guarantee backwards
compatibility for, there's not a reason to subject users to that kind of
pain unnecessarily.

Thanks for the quick response.

 - Emily