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

[Junio C Hamano]

Jeff King  writes:

> On Mon, Aug 14, 2017 at 03:42:14PM -0700, Junio C Hamano wrote:
>
>> Jeff King  writes:
>> 
>> > If it's smooth, the (50,1) case is slightly nicer in that it puts the
>> > progress in front of the user more quickly. I'm not sure if that's
>> > actually worth pushing an additional decision onto the person writing
>> > the calling code, though (especially when we are just now puzzling out
>> > the method for making such a decision from first principles).
>> >
>> > So I'd vote to drop that parameter entirely. And if 1 second seems
>> > noticeably snappier, then we should probably just move everything to a 1
>> > second delay (I don't have a strong feeling either way).
>> 
>> Sounds like a good idea to me.  
>> 
>> I've already locally tweaked Kevin's patch to use (0,2) instead of
>> (0,1) without introducing the simpler wrapper.  It should be trivial
>> to do a wrapper to catch and migrate all the (0,2) users to a
>> start_delayed_progress() that takes neither percentage or time with
>> mechanical replacement.
>
> I was actually proposing to move (50,1) cases to the simpler wrapper,
> too. IOW, drop the delayed_percent_treshold code entirely.

I should have mentioned that (50,1) should also be treated just like
(0,2) case--they should mean the same thing.  Other oddness like 95
might merit individual consideration, and I didn't want to add (0,1)
as another oddball to the mix.

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

[Jeff King]

On Mon, Aug 14, 2017 at 03:42:14PM -0700, Junio C Hamano wrote:

> Jeff King  writes:
> 
> > If it's smooth, the (50,1) case is slightly nicer in that it puts the
> > progress in front of the user more quickly. I'm not sure if that's
> > actually worth pushing an additional decision onto the person writing
> > the calling code, though (especially when we are just now puzzling out
> > the method for making such a decision from first principles).
> >
> > So I'd vote to drop that parameter entirely. And if 1 second seems
> > noticeably snappier, then we should probably just move everything to a 1
> > second delay (I don't have a strong feeling either way).
> 
> Sounds like a good idea to me.  
> 
> I've already locally tweaked Kevin's patch to use (0,2) instead of
> (0,1) without introducing the simpler wrapper.  It should be trivial
> to do a wrapper to catch and migrate all the (0,2) users to a
> start_delayed_progress() that takes neither percentage or time with
> mechanical replacement.

I was actually proposing to move (50,1) cases to the simpler wrapper,
too. IOW, drop the delayed_percent_treshold code entirely.

-Peff

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

[Junio C Hamano]

Jeff King  writes:

> If it's smooth, the (50,1) case is slightly nicer in that it puts the
> progress in front of the user more quickly. I'm not sure if that's
> actually worth pushing an additional decision onto the person writing
> the calling code, though (especially when we are just now puzzling out
> the method for making such a decision from first principles).
>
> So I'd vote to drop that parameter entirely. And if 1 second seems
> noticeably snappier, then we should probably just move everything to a 1
> second delay (I don't have a strong feeling either way).

Sounds like a good idea to me.  

I've already locally tweaked Kevin's patch to use (0,2) instead of
(0,1) without introducing the simpler wrapper.  It should be trivial
to do a wrapper to catch and migrate all the (0,2) users to a
start_delayed_progress() that takes neither percentage or time with
mechanical replacement.

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

[Jeff King]

On Mon, Aug 14, 2017 at 11:35:33AM -0700, Junio C Hamano wrote:

> Junio C Hamano  writes:
> 
> > Perhaps we may want to replace the calls to progress_delay() with a
> > call to a simpler wrapper that does not let the callers give their
> > own delay threashold to simplify the API.
> 
> ... which does not look too bad, but because it makes me wonder if
> we even need to make the delay-threshold customizable per callers,
> I'll wait further discussions before committing to the approach.

For what it's worth, I had a similar "why is this even part of the API"
thought when writing earlier in the thread.

> For example, I do not quite understand why 95 is a good value for
> prune-packed while 0 is a good value for prune.

And I also wondered this same thing. I do not have a good answer.

> The rename detection in diffcore-rename, delay in blame, and
> checkout (via unpack-trees) all of which use 50-percent threshold
> with 1 second delay, sort of make sense to me in that if we
> completed 50 percent within 1 second, it is likely we will finish
> all in 2 seconds (which is the norm for everybody else), perhaps as
> a better version of 0-percent 2 seconds rule.

Just thinking what could go wrong for a moment.

If we reach 51% in one second, would we then never show progress? That
seems like a misfeature when the counter is spiky. E.g., consider
something like object transfer (which doesn't do a delayed progress, but
pretend for a moment as it makes a simple example). Imagine we have 100
objects, 99 of which are 10KB and one of which is 100MB. And that the
big object comes at slot 75.

No matter how the delay works, the counter is going to jump quickly to
75% as we send the small objects, and then appear to stall on the large
one. That's unavoidable without feeding the progress code more data
about the items. But what does the user see?

With (0,2), we start the progress meter at 2 seconds, the user sees it
stall at 75%, and then eventually it unclogs.

With (50,1), we check the percentage after 1 second and see we are
already at 75%. We then disable the meter totally. After 2 seconds, we
get the same stall but the user sees nothing.

So in that case we should always base our decision only on time taken.

And that gives me an answer for your question above: the difference is
whether we expect the counter to move smoothly, or to be spiky.

If it's smooth, the (50,1) case is slightly nicer in that it puts the
progress in front of the user more quickly. I'm not sure if that's
actually worth pushing an additional decision onto the person writing
the calling code, though (especially when we are just now puzzling out
the method for making such a decision from first principles).

So I'd vote to drop that parameter entirely. And if 1 second seems
noticeably snappier, then we should probably just move everything to a 1
second delay (I don't have a strong feeling either way).

-Peff

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

[Junio C Hamano]

Junio C Hamano  writes:

> Perhaps we may want to replace the calls to progress_delay() with a
> call to a simpler wrapper that does not let the callers give their
> own delay threashold to simplify the API.

... which does not look too bad, but because it makes me wonder if
we even need to make the delay-threshold customizable per callers,
I'll wait further discussions before committing to the approach.

For example, I do not quite understand why 95 is a good value for
prune-packed while 0 is a good value for prune.  

The rename detection in diffcore-rename, delay in blame, and
checkout (via unpack-trees) all of which use 50-percent threshold
with 1 second delay, sort of make sense to me in that if we
completed 50 percent within 1 second, it is likely we will finish
all in 2 seconds (which is the norm for everybody else), perhaps as
a better version of 0-percent 2 seconds rule.

 builtin/blame.c|  3 +--
 builtin/fsck.c |  2 +-
 builtin/prune-packed.c |  4 ++--
 builtin/prune.c|  2 +-
 builtin/rev-list.c |  2 +-
 diffcore-rename.c  |  4 ++--
 progress.c | 10 --
 progress.h |  4 ++--
 unpack-trees.c |  3 +--
 9 files changed, 19 insertions(+), 15 deletions(-)

diff --git a/builtin/blame.c b/builtin/blame.c
index bda1a78726..05115900f3 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -925,8 +925,7 @@ int cmd_blame(int argc, const char **argv, const char 
*prefix)
sb.found_guilty_entry = _guilty_entry;
sb.found_guilty_entry_data = 
if (show_progress)
-   pi.progress = start_progress_delay(_("Blaming lines"),
-  sb.num_lines, 50, 1);
+   pi.progress = start_delayed_progress(_("Blaming lines"), 
sb.num_lines, 50);
 
assign_blame(, opt);
 
diff --git a/builtin/fsck.c b/builtin/fsck.c
index 99dea7adf6..6a26079ebc 100644
--- a/builtin/fsck.c
+++ b/builtin/fsck.c
@@ -188,7 +188,7 @@ static int traverse_reachable(void)
unsigned int nr = 0;
int result = 0;
if (show_progress)
-   progress = start_progress_delay(_("Checking connectivity"), 0, 
0, 2);
+   progress = start_delayed_progress(_("Checking connectivity"), 
0, 0);
while (pending.nr) {
struct object_array_entry *entry;
struct object *obj;
diff --git a/builtin/prune-packed.c b/builtin/prune-packed.c
index ac978ad401..4fc6b175de 100644
--- a/builtin/prune-packed.c
+++ b/builtin/prune-packed.c
@@ -37,8 +37,8 @@ static int prune_object(const struct object_id *oid, const 
char *path,
 void prune_packed_objects(int opts)
 {
if (opts & PRUNE_PACKED_VERBOSE)
-   progress = start_progress_delay(_("Removing duplicate objects"),
-   256, 95, 2);
+   progress = start_delayed_progress(_("Removing duplicate 
objects"),
+ 256, 95);
 
for_each_loose_file_in_objdir(get_object_directory(),
  prune_object, NULL, prune_subdir, );
diff --git a/builtin/prune.c b/builtin/prune.c
index c378690545..f32adaa0e9 100644
--- a/builtin/prune.c
+++ b/builtin/prune.c
@@ -138,7 +138,7 @@ int cmd_prune(int argc, const char **argv, const char 
*prefix)
if (show_progress == -1)
show_progress = isatty(2);
if (show_progress)
-   progress = start_progress_delay(_("Checking connectivity"), 0, 
0, 2);
+   progress = start_delayed_progress(_("Checking connectivity"), 
0, 0);
 
mark_reachable_objects(, 1, expire, progress);
stop_progress();
diff --git a/builtin/rev-list.c b/builtin/rev-list.c
index 95d84d5cda..5cfc4b35f4 100644
--- a/builtin/rev-list.c
+++ b/builtin/rev-list.c
@@ -364,7 +364,7 @@ int cmd_rev_list(int argc, const char **argv, const char 
*prefix)
revs.limited = 1;
 
if (show_progress)
-   progress = start_progress_delay(show_progress, 0, 0, 2);
+   progress = start_delayed_progress(show_progress, 0, 0);
 
if (use_bitmap_index && !revs.prune) {
if (revs.count && !revs.left_right && !revs.cherry_mark) {
diff --git a/diffcore-rename.c b/diffcore-rename.c
index 786f389498..0eafa43618 100644
--- a/diffcore-rename.c
+++ b/diffcore-rename.c
@@ -532,9 +532,9 @@ void diffcore_rename(struct diff_options *options)
}
 
if (options->show_rename_progress) {
-   progress = start_progress_delay(
+   progress = start_delayed_progress(
_("Performing inexact rename detection"),
-   rename_dst_nr * rename_src_nr, 50, 1);
+   rename_dst_nr * rename_src_nr, 50);
}
 
mx = xcalloc(st_mult(NUM_CANDIDATE_PER_DST, num_create), sizeof(*mx));
diff --git a/progress.c b/progress.c
index 73e36d4a42..eeebe15b6f 

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

[Junio C Hamano]

Jeff King  writes:

> On Sat, Aug 12, 2017 at 09:06:18AM +0100, Philip Oakley wrote:
>
>> > > > + progress = start_progress_delay(_("Generating patches"), total, 0, 
>> > > > 1);
>> > > 
>> > > I don't really have an opinion on a 1 second delay versus 2. I thought
>> > > we used 2 pretty consistently, though grepping around I do see a couple
>> > > of 1's. It probably doesn't matter, but just a curiosity.
>> > 
> ...
> Here we're just talking about calls to start_progress_delay(), and how
> long it waits before deciding that the operation is slow enough to show
> progress. Blame, rename detection, and checkout use 1 second. Prune,
> prune-packed, and connectivity checks all use 2 seconds. I doubt it
> matters all that much, but presumably the purpose in all is "how long
> before a user starts to wonder if things are actually happening", which
> is probably command-independent.

I feel comfortable moving forward, basing our decisions on the
assumption that the "delay before the user wonders is independent of
the command" without anybody actually proving it.

Even though I think that it is natural for people to expect longer
delay from some operation than others (due to some chicken-and-egg
reasons), trying to scientifically measure and to come up with
different delay value that suited for each and every operation is
waste of our brain cycles.

Perhaps we may want to replace the calls to progress_delay() with a
call to a simpler wrapper that does not let the callers give their
own delay threashold to simplify the API.

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

[Jeff King]

On Sat, Aug 12, 2017 at 09:06:18AM +0100, Philip Oakley wrote:

> > > > + progress = start_progress_delay(_("Generating patches"), total, 0, 1);
> > > 
> > > I don't really have an opinion on a 1 second delay versus 2. I thought
> > > we used 2 pretty consistently, though grepping around I do see a couple
> > > of 1's. It probably doesn't matter, but just a curiosity.
> > 
> > Yeah, I also thought 2-second was what we used by default.  Perhaps
> > we would want to bring others in line?
> 
>  Surely the choice should depend on the purpose of the delay. IIRC
> the 1 second between patches on the formal 'sent' time was simply to ensure
> the patches had the right sequence. Delays for warnings and progress have
> different purposes, so I think it's more about the purpose than
> standardising the time (along with expectations [least surprise] if other
> messages are displayed).

I think you're confusing two unrelated things. There's a 1-second fake
time-advance on patches, but that's done by send-email, not
format-patch.

Here we're just talking about calls to start_progress_delay(), and how
long it waits before deciding that the operation is slow enough to show
progress. Blame, rename detection, and checkout use 1 second. Prune,
prune-packed, and connectivity checks all use 2 seconds. I doubt it
matters all that much, but presumably the purpose in all is "how long
before a user starts to wonder if things are actually happening", which
is probably command-independent.

-Peff

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

[Philip Oakley]

From: "Junio C Hamano" 

Jeff King  writes:


On Thu, Aug 10, 2017 at 02:32:55PM -0400, Kevin Willford wrote:

@@ -1493,6 +1496,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 while generating patches")),


Earlier I suggested allowing --progress="custom text" since this may be
driven as plumbing for other commands. But I don't think there's any
need to worry about it now. It can be added seamlessly later if we find
such a caller.

@@ -1752,8 +1757,12 @@ int cmd_format_patch(int argc, const char **argv, 
const char *prefix)

 start_number--;
 }
 rev.add_signoff = do_signoff;
+
+ if (show_progress)
+ progress = start_progress_delay(_("Generating patches"), total, 0, 1);


I don't really have an opinion on a 1 second delay versus 2. I thought
we used 2 pretty consistently, though grepping around I do see a couple
of 1's. It probably doesn't matter, but just a curiosity.


Yeah, I also thought 2-second was what we used by default.  Perhaps
we would want to bring others in line?


 Surely the choice should depend on the purpose of the delay. IIRC 
the 1 second between patches on the formal 'sent' time was simply to ensure 
the patches had the right sequence. Delays for warnings and progress have 
different purposes, so I think it's more about the purpose than 
standardising the time (along with expectations [least surprise] if other 
messages are displayed).

--
Philip 

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

[Junio C Hamano]

Jeff King  writes:

> On Thu, Aug 10, 2017 at 02:32:55PM -0400, Kevin Willford wrote:
>
>> @@ -1493,6 +1496,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 while generating patches")),
>
> Earlier I suggested allowing --progress="custom text" since this may be
> driven as plumbing for other commands. But I don't think there's any
> need to worry about it now. It can be added seamlessly later if we find
> such a caller.
>
>> @@ -1752,8 +1757,12 @@ int cmd_format_patch(int argc, const char **argv, 
>> const char *prefix)
>>  start_number--;
>>  }
>>  rev.add_signoff = do_signoff;
>> +
>> +if (show_progress)
>> +progress = start_progress_delay(_("Generating patches"), total, 
>> 0, 1);
>
> I don't really have an opinion on a 1 second delay versus 2. I thought
> we used 2 pretty consistently, though grepping around I do see a couple
> of 1's. It probably doesn't matter, but just a curiosity.

Yeah, I also thought 2-second was what we used by default.  Perhaps
we would want to bring others in line?

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

[Jeff King]

On Thu, Aug 10, 2017 at 02:32:55PM -0400, Kevin Willford wrote:

> @@ -1493,6 +1496,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 while generating patches")),

Earlier I suggested allowing --progress="custom text" since this may be
driven as plumbing for other commands. But I don't think there's any
need to worry about it now. It can be added seamlessly later if we find
such a caller.

> @@ -1752,8 +1757,12 @@ int cmd_format_patch(int argc, const char **argv, 
> const char *prefix)
>   start_number--;
>   }
>   rev.add_signoff = do_signoff;
> +
> + if (show_progress)
> + progress = start_progress_delay(_("Generating patches"), total, 
> 0, 1);

I don't really have an opinion on a 1 second delay versus 2. I thought
we used 2 pretty consistently, though grepping around I do see a couple
of 1's. It probably doesn't matter, but just a curiosity.

-Peff

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

[Kevin Willford]

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.

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.

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 is then used by the rebase command when
calling format-patch.

Signed-off-by: Kevin Willford 
---
 Documentation/git-format-patch.txt |  4 
 builtin/log.c  | 10 ++
 2 files changed, 14 insertions(+)

diff --git a/Documentation/git-format-patch.txt 
b/Documentation/git-format-patch.txt
index c890328b02..6cbe462a77 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]
   []
   [  |  ]
 
@@ -283,6 +284,9 @@ 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::
+   Show progress reports on stderr as patches are generated.
+
 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 725c7b8a1a..b07a5529c2 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -27,6 +27,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;
@@ -1422,6 +1423,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,
@@ -1493,6 +1496,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 while generating patches")),
OPT_END()
};
 
@@ -1752,8 +1757,12 @@ int cmd_format_patch(int argc, const char **argv, const 
char *prefix)
start_number--;
}
rev.add_signoff = do_signoff;
+
+   if (show_progress)
+   progress = start_progress_delay(_("Generating patches"), total, 
0, 1);
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 */
@@ -1818,6 +1827,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.14.0.rc0.286.g44127d70e4