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

[Jeff King]

On Thu, Jun 01, 2017 at 01:15:57PM +0200, Johannes Schindelin wrote:

> On Wed, 31 May 2017, Jeff King wrote:
> 
> > 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.
> 
> When working with huge repositories with a large number of branches, it is
> all too easy to pick the wrong branch to rebase to. In that case, it can
> take a long time for the `git rebase` call to even generate the mbox.
> 
> Kevin's patch is a visual indication when this is happening.

Right, sorry if it wasn't clear from the rest of my message. My
statement was one of surprise, not objection. I understand his use case.

-Peff

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

[Johannes Schindelin]

Hi Peff,

On Wed, 31 May 2017, Jeff King wrote:

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

When working with huge repositories with a large number of branches, it is
all too easy to pick the wrong branch to rebase to. In that case, it can
take a long time for the `git rebase` call to even generate the mbox.

Kevin's patch is a visual indication when this is happening.

Instead of being puzzled by `git rebase` "hanging", the user will now see
that (.../21957) patch is generated and will have a chance to say "wait a
minute, *that* many patches? I think I meant a different branch".

And since format-patch generates the patches for `git rebase` (actually,
it generates an mbox that then has to be split again, but you get the
gist), it is format-patch that needs to output the progress.

Ciao,
dscho

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

[Junio C Hamano]

Jeff King  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.

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

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

[Jeff King]

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

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

> @@ -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.

> @@ -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

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

[Kevin Willford]

> -Original Message-
> From: git-ow...@vger.kernel.org [mailto:git-ow...@vger.kernel.org] On
> Behalf Of Stefan Beller
> Sent: Wednesday, May 31, 2017 12:40 PM
> To: Kevin Willford <kcwillf...@gmail.com>
> Cc: git@vger.kernel.org; Junio C Hamano <gits...@pobox.com>; Kevin
> Willford <kewi...@microsoft.com>
> Subject: Re: [PATCH 1/2] format-patch: have progress option while
> generating patches
> 
> On Wed, May 31, 2017 at 8:04 AM, Kevin Willford <kcwillf...@gmail.com>
> wrote:
> > 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.
> 
> After reading the code, I was looking for some explanation on the underlying
> assumptions, such as:
> 
>   The progress meter as presented in this patch assumes the thousands of
>   patches to have a fine granularity as well as assuming to require all the
>   same amount of work/time for each, such that a steady progress bar
>   is achieved.
> 
>   We do not want to estimate the time for each patch based e.g.
>   on their size or number of touched files (or parents) as that is too
>   expensive for just a progress meter.
> 

Sounds good.  I will add some explanation to the commit message.

> 
> >
> > Signed-off-by: Kevin Willford <kewi...@microsoft.com>
> > ---
> >  Documentation/git-format-patch.txt |  8 
> >  builtin/log.c  | 10 ++
> >  2 files changed, 18 insertions(+)
> >
> > diff --git a/Documentation/git-format-patch.txt
> > b/Documentation/git-format-patch.txt
> > index c890328b02..ee5f99f606 100644
> > --- a/Documentation/git-format-patch.txt
> > +++ b/Documentation/git-format-patch.txt
> > @@ -23,6 +23,7 @@ SYNOPSIS
> >[(--reroll-count|-v) ]
> >[--to=] [--cc=]
> >[--[no-]cover-letter] [--quiet] [--notes[=]]
> > +  [--progress]
> >[]
> >[  |  ]
> >
> > @@ -260,6 +261,7 @@ you can use `--suffix=-patch` to get `0001-
> description-of-my-change-patch`.
> >  -q::
> >  --quiet::
> > Do not print the names of the generated files to standard output.
> > +   Progress is not reported to the standard error stream.
> >
> >  --no-binary::
> > Do not output contents of changes in binary files, instead @@
> > -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.
> > +
> >  CONFIGURATION
> >  -
> >  You can specify extra mail header lines to be added to each message,
> > diff --git a/builtin/log.c b/builtin/log.c index
> > 631fbc984f..02c50431b6 100644
> > --- a/builtin/log.c
> > +++ b/builtin/log.c
> > @@ -26,6 +26,7 @@
> >  #include "version.h"
> >  #include "mailmap.h"
> >  #include "gpg-interface.h"
> > +#include "progress.h"
> >
> >  /* Set a default date-time format for git log ("log.date" config
> > variable) */  static const char *default_date_mode = NULL; @@ -1409,6
> > +1410,8 @@ int cmd_format_patch(int argc, const char **argv, const char
> *prefix)
> > char *branch_name = NULL;
> > char *base_commit = NULL;
> > struct base_tree_info bases;
> > +   int show_progress = 0;
> > +   struct progress *progress = NULL;
> >
> > const struct option builtin_format_patch_options[] = {
> > { OPTION_CALLBACK, 'n', "numbered", , NULL,
> > @@ -1480,6 +1483,8 @@ int cmd_format_patch(int argc, const char **argv,
> const char *prefix)
> > OPT_FILENAME(0, "signature-file", _file,
> > N_("add a signature from a file")),
> >

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

[Stefan Beller]

On Wed, May 31, 2017 at 8:04 AM, Kevin Willford  wrote:
> 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.

After reading the code, I was looking for some explanation
on the underlying assumptions, such as:

  The progress meter as presented in this patch assumes the thousands of
  patches to have a fine granularity as well as assuming to require all the
  same amount of work/time for each, such that a steady progress bar
  is achieved.

  We do not want to estimate the time for each patch based e.g.
  on their size or number of touched files (or parents) as that is too
  expensive for just a progress meter.


>
> Signed-off-by: Kevin Willford 
> ---
>  Documentation/git-format-patch.txt |  8 
>  builtin/log.c  | 10 ++
>  2 files changed, 18 insertions(+)
>
> diff --git a/Documentation/git-format-patch.txt 
> b/Documentation/git-format-patch.txt
> index c890328b02..ee5f99f606 100644
> --- a/Documentation/git-format-patch.txt
> +++ b/Documentation/git-format-patch.txt
> @@ -23,6 +23,7 @@ SYNOPSIS
>[(--reroll-count|-v) ]
>[--to=] [--cc=]
>[--[no-]cover-letter] [--quiet] [--notes[=]]
> +  [--progress]
>[]
>[  |  ]
>
> @@ -260,6 +261,7 @@ you can use `--suffix=-patch` to get 
> `0001-description-of-my-change-patch`.
>  -q::
>  --quiet::
> Do not print the names of the generated files to standard output.
> +   Progress is not reported to the standard error stream.
>
>  --no-binary::
> Do not output contents of changes in binary files, instead
> @@ -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.
> +
>  CONFIGURATION
>  -
>  You can specify extra mail header lines to be added to each message,
> diff --git a/builtin/log.c b/builtin/log.c
> index 631fbc984f..02c50431b6 100644
> --- a/builtin/log.c
> +++ b/builtin/log.c
> @@ -26,6 +26,7 @@
>  #include "version.h"
>  #include "mailmap.h"
>  #include "gpg-interface.h"
> +#include "progress.h"
>
>  /* Set a default date-time format for git log ("log.date" config variable) */
>  static const char *default_date_mode = NULL;
> @@ -1409,6 +1410,8 @@ int cmd_format_patch(int argc, const char **argv, const 
> char *prefix)
> char *branch_name = NULL;
> char *base_commit = NULL;
> struct base_tree_info bases;
> +   int show_progress = 0;
> +   struct progress *progress = NULL;
>
> const struct option builtin_format_patch_options[] = {
> { OPTION_CALLBACK, 'n', "numbered", , NULL,
> @@ -1480,6 +1483,8 @@ int cmd_format_patch(int argc, const char **argv, const 
> char *prefix)
> OPT_FILENAME(0, "signature-file", _file,
> N_("add a signature from a file")),
> OPT__QUIET(, N_("don't print the patch filenames")),
> +   OPT_BOOL(0, "progress", _progress,
> +N_("show progress")),
> OPT_END()
> };
>
> @@ -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);
> while (0 <= --nr) {
> int shown;
> +   display_progress(progress, total - nr);
> commit = list[nr];
> rev.nr = total - nr + (start_number - 1);
> /* Make the second and subsequent mails replies to the first 
> */
> @@ -1805,6 +1814,7 @@ int cmd_format_patch(int argc, const char **argv, const 
> char *prefix)
> if (!use_stdout)
> fclose(rev.diffopt.file);
> }
> +   stop_progress();
> free(list);
> free(branch_name);
> string_list_clear(_to, 0);
> --
> 2.13.0.92.g73a4ce6a77
>