Re: [PATCH 04/11] builtin rebase: support --quiet

From: Johannes Schindelin <Johannes.Schindelin@gmx.de>
To: Junio C Hamano <gitster@pobox.com>
Cc: Stefan Beller <sbeller@google.com>,
	Pratik Karki <predatoramigo@gmail.com>, git <git@vger.kernel.org>,
	Christian Couder <christian.couder@gmail.com>,
	Alban Gruin <alban.gruin@gmail.com>
Subject: Re: [PATCH 04/11] builtin rebase: support --quiet
Date: Mon, 27 Aug 2018 14:31:04 +0200 (DST)	[thread overview]
Message-ID: <nycvar.QRO.7.76.6.1808271416410.73@tvgsbejvaqbjf.bet> (raw)
In-Reply-To: <xmqqsh3ofy4f.fsf@gitster-ct.c.googlers.com>

Hi Junio,

On Wed, 8 Aug 2018, Junio C Hamano wrote:

> Stefan Beller <sbeller@google.com> writes:
> 
> > On Wed, Aug 8, 2018 at 6:51 AM Pratik Karki <predatoramigo@gmail.com> wrote:
> >>
> >> This commit introduces a rebase option `--quiet`. While `--quiet` is
> >> commonly perceived as opposite to `--verbose`, this is not the case for
> >> the rebase command: both `--quiet` and `--verbose` default to `false` if
> >> neither `--quiet` nor `--verbose` is present.
> >>
> >> This commit goes further and introduces `--no-quiet` which is the
> >> contrary of `--quiet` and it's introduction doesn't modify any
> >> behaviour.
> 
> Why?  Is it for completeness (i.e. does the scripted version take
> such an option and addition of --no-quiet makes the C rewrite behave
> the same)?

Ah. I mentioned that an explanation for this is needed in the commit
message, and I guess that it is a bit too subtle. The part you clipped
from your quoted text says:

	[... `--quiet`] switches off --verbose and --stat, and as
	--verbose switches off --quiet, we use the (negated)
	REBASE_NO_QUIET instead of REBASE_QUIET: this allows us to turn
	off the quiet mode and turn on the verbose and diffstat mode in a
	single OPT_BIT(), and the opposite in a single OPT_NEGBIT().

I agree that this is a pretty convoluted way to express the issue. See
below for an attempt at a clearer commit message.

> >> Note: The `flags` field in `rebase_options` will accumulate more bits in
> >> subsequent commits, in particular a verbose and a diffstat flag. And as
> >> --quoet inthe shell scripted version of the rebase command switches off
> >
> >   --quote in the
> >
> > (in case a resend is needed)
> 
> Meaning --quiet?

Yep. I should have paid more attention in my pre-submission review, sorry.

I changed the commit message to read like this:

	builtin rebase: support --quiet

	This commit introduces a rebase option `--quiet`. While `--quiet` is
	commonly perceived as opposite to `--verbose`, this is not the case for
	the rebase command: both `--quiet` and `--verbose` default to `false` if
	neither `--quiet` nor `--verbose` is present.

	Despite the default being `false` for both verbose and quiet mode,
	passing the `--quiet` option will turn off verbose mode, and `--verbose`
	will turn off quiet mode.

	This patch introduces the `flags` bit field, with `REBASE_NO_QUIET`
	as first user (with many more to come).

	We do *not* use `REBASE_QUIET` here for an important reason: To keep the
	implementation simple, this commit introduces `--no-quiet` instead of
	`--quiet`, so that a single `OPT_NEGBIT()` can turn on quiet mode and
	turn off verbose and diffstat mode at the same time. Likewise, the
	companion commit which will introduce support for `--verbose` will have
	a single `OPT_BIT()` that turns off quiet mode and turns on verbose and
	diffstat mode at the same time.

Ciao,
Dscho