Re: [PATCH 1/2] format-patch: have progress option while generating patches

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 1, 2017, 04:10 UTC
Message-ID: <xmqqpoeo73g7.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20170531220100.t27w3w642sn33h7s@sigill.intra.peff.net>

Jeff King <peff@peff.net> writes:

> As I said above, I think I'd prefer it to require "--progress", as
> format-patch is quite often used as plumbing.

Yes, that sounds sensible.

Initially, my reaction was "Why do we even need --progress for format-patch, when it gives one-line per patch output to show the progress anyway?", but if that output is redirected to a file, of course you'd need --progress independently.

> Should this use start_progress_delay()? In most cases the command will
> complete very quickly, and the progress report is just noise. For many
> commands (e.g., checkout) we wait 1-2 seconds before bothering to show
> progress output.

It is better to use the "delay" version for progress meters for commands that may or may not last very long, and this may be a good candidate to heed that principle.

The subcommands that use start_progress() tend to be older and more batch oriented operations, e.g. fsck, pack-objects, etc., that are expected to last longer. It may be a good idea to convert them to the "delay" variant, but obviously that is outside the scope of this patch.

Show 11 quoted lines

> I would have expected this to say "Generating patches"; most of our
> other progress messages are pluralized. You could use Q_() to handle the
> mono/plural case, but I think it's fine to just always say "patches"
> (that's what other messages do).
>
> One final thought is whether callers would want to customize this
> message, since it will often be used as plumbing. E.g., would rebase
> want to say something besides "Generating patches". I'm not sure.
> Anyway, if you're interested in that direction, there's prior art in
> the way rev-list handles "--progress" (and its sole caller,
> check_connected()).

These are all good suggestions.