Re: [PATCH v3] submodule: port subcommand 'set-branch' from shell to C

From: Denton Liu <liu.denton@gmail.com>
Date: May 21, 2020, 19:03 UTC
Message-ID: <20200521190329.GB615266@generichostname>
In-Reply-To: <xmqqk115ruux.fsf@gitster.c.googlers.com>

On Thu, May 21, 2020 at 11:44:22AM -0700, Junio C Hamano wrote:

Show 27 quoted lines

> Shourya Shukla <shouryashukla.oo@gmail.com> writes:
> 
> > Convert submodule subcommand 'set-branch' to a builtin and call it via
> > 'git-submodule.sh'.
> >
> > Mentored-by: Christian Couder <chriscool@tuxfamily.org>
> > Mentored-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>
> > Helped-by: Denton Liu <liu.denton@gmail.com>
> > Helped-by: Eric Sunshine <sunshine@sunshineco.com>
> > Signed-off-by: Shourya Shukla <shouryashukla.oo@gmail.com>
> > ---
> > Thank you for the review Eric. I have changed the commit message,
> > and the error prompts. Also, I have added a brief comment about
> > the `quiet` option.
> 
> Sorry, I may have missed the previous rounds of discussion, but the
> comment adds more puzzles than it helps readers.  "is currently not
> used" can be seen from the code, but it is totally unclear why it is
> not used.  Is that a design decision to always keep quiet or always
> talkative (if so, "suppress output..." is not a good description)?
> Is that that this is a WIP patch that the behaviour the option aims
> to achieve hasn't been implemented?  Is it that no existing callers
> pass "-q" to the scripted version, so there is no need to support
> it (if so, why do we even accept it in the first place)?  Is it that
> all existing callers pass "-q" so we need to accept it, but there is
> nothing we need to make verbose so the variable is not passed around
> in the codepath?

As the original author of the shell code, I had it accept -q because, with the other subcommmands, you can pass -q either before or after the subcommand such as

	$ git submodule -q sync

or
	$ git submodule sync -q

and I wanted set-branch to retain that behaviour even though -q ultimately doesn't affect set-branch at all since it's already a quiet command.

Perhaps as a follow-up to this patch, we could stop accepting -q in set-branch. I highly doubt that anyone is using it anyway.