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

From: Jeff King <peff@peff.net>
Date: May 31, 2017, 22:01 UTC
Message-ID: <20170531220100.t27w3w642sn33h7s@sigill.intra.peff.net>
In-Reply-To: <20170531150427.7820-2-kewillf@microsoft.com>

On Wed, May 31, 2017 at 08:04:26AM -0700, Kevin Willford wrote:

Show 9 quoted lines

> When generating patches for the rebase command if the user does
> not realize the branch they are rebasing onto is thousands of
> commits different there is no progress indication after initial
> rewinding message.
> 
> This patch allows a progress option to be passed to format-patch
> so that the user can be informed the progress of generating the
> patch.  This option will then be used by the rebase command when
> calling format-patch.

I'm generally in favor of progress meters, though it does seem a little funny to me that we'd need one on format-patch. It's basically the same operation as "git log -p", after all. I guess one difference is that usually the output of "log" would stream to the pager, so the user would see immediate output. That's true of format-patch, too, but in the instance you care about we're probably doing something more like:

  git format-patch "$@" >patches

and the user sees nothing.

That makes me wonder two things:

  1. Should this really be tied to isatty(2), as the documentation
     claims?  It seems like you'd really only want it to kick in for
     certain cases. Arguably whenever stdout is _not_ going to a tty
     you'd want progress, but I think probably the caller should
     probably just decide whether to ask for it or not.

  2. Should this apply to other commands in the log family besides
     format-patch? E.g., should "git log --progress -p >commits" work?

     I added a progress meter to rev-list a while ago (for connectivity
     checks). I don't think we could push this down as far as the
     revision traversal code, because only its callers really understand
     what's the right unit to be counting.

     It may not be worth the trouble, though. Only "format-patch" does a
     pre-pass to find the total number of commits. So "git log
     --progress" would inherently just be counting up, with no clue what
     the final number is.

Show 9 quoted lines

> @@ -283,6 +285,12 @@ you can use `--suffix=-patch` to get `0001-description-of-my-change-patch`.
>  	range are always formatted as creation patches, independently
>  	of this flag.
>  
> +--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.

Checking whether stderr is a tty would match our usual progress meters. But I don't actually see any code in the patch implementing this.

As I said above, I think I'd prefer it to require "--progress", as format-patch is quite often used as plumbing. But we'd need to fix the documentation here to match.

Show 7 quoted lines

> @@ -1739,8 +1744,12 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
>  		start_number--;
>  	}
>  	rev.add_signoff = do_signoff;
> +
> +	if (show_progress && !quiet)
> +		progress = start_progress(_("Generating patch"), total);

The meter code here all looks correct, but let me bikeshed for a moment. :)

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.

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()).

-Peff