Re: [PATCH v3 2/3] bundle-create: progress output control

From: Jeff King <peff@peff.net>
Date: Nov 11, 2019, 04:07 UTC
Message-ID: <20191111040750.GB6379@sigill.intra.peff.net>
In-Reply-To: <20191110204126.30553-2-robbat2@gentoo.org>

On Sun, Nov 10, 2019 at 12:41:25PM -0800, Robin H. Johnson wrote:

Show 5 quoted lines

> Support the progress output options from pack-objects in git-bundle's
> create subcommand. Most notably, this provides --quiet as requested on
> the git mailing list per [1]
> 
> Reference: https://www.mail-archive.com/git@vger.kernel.org/msg182844.html <robbat2-20190806T191156-796782357Z@orbis-terrarum.net>

I'm glad you included the message-id here, since "182844" is useless if mail-archive.com ever goes away. We usually just cite public-inbox for that reason, since its URLs just use the message-id anyway:

  https://public-inbox.org/git/robbat2-20190806T191156-796782357Z@orbis-terrarum.net

Show 26 quoted lines

> +--progress::
> +	Progress status is reported on the standard error stream
> +	by default when it is attached to a terminal, unless -q
> +	is specified. This flag forces progress status even if
> +	the standard error stream is not directed to a terminal.
> +
> +--all-progress::
> +	When --stdout is specified then progress report is
> +	displayed during the object count and compression phases
> +	but inhibited during the write-out phase. The reason is
> +	that in some cases the output stream is directly linked
> +	to another command which may wish to display progress
> +	status of its own as it processes incoming pack data.
> +	This flag is like --progress except that it forces progress
> +	report for the write-out phase as well even if --stdout is
> +	used.
> +
> +--all-progress-implied::
> +	This is used to imply --all-progress whenever progress display
> +	is activated.  Unlike --all-progress this flag doesn't actually
> +	force any progress display by itself.
> +
> +-q::
> +--quiet::
> +	This flag makes the command not to report its progress
> +	on the standard error stream.

Do we need all four of these?

Just saying "--no-progress" would do what you want right now. I could understand the desire for a general "--quiet" flag that implies "--no-progress", and shuts off any other non-progress chatter as well. There isn't any now, but it could be a future proofing thing (plus having a "-q" option is standard). But I think we should document it that way from the outset (though I notice you probably just lifted this from pack-objects, IMHO it should be more clear, too).

The "all-progress" thing doesn't seem useful at this level. pack-objects needs it so that it can do the right thing when being driven by upload-pack versus send-pack. But for a bundle, we're always writing to a file. We'd always want "all-progress" (and that's what the current code does).

Likewise, "all-progress-implied" is about setting the "all-progress" bit but still letting pack-objects decide whether to show progress based on isatty(2). I don't think we'd need that here at all (we check isatty ourselves, and we'd always want all-progress).

So could we perhaps simplify this to:

  1. Set show_progress to isatty(2).

  2. Make --progress a parseopt bool, setting show_progress to 1 (or if
     we see "--no-progress").

  3. Pass "--no-progress" or "--all-progress" to pack-objects, based on
     show_progress.

  4. (Optional) Make "--quiet" a synonym for "--no-progress", with the
     documentation that it may later encompass other messages.

-Peff