Re: [PATCH v2 1/2] format-patch: have progress option while generating patches
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
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
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
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 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 = &found_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(&sb, 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, &opts); 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(&revs, 1, expire, progress); stop_progress(&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..eee
Re: [PATCH v2 1/2] format-patch: have progress option while generating patches
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
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
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", &signature_file, N_("add a signature from a file")), OPT__QUIET(&quiet, N_("don't print the patch filenames")), + OPT_BOOL(0, "progress", &show_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
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", &signature_file, >> N_("add a signature from a file")), >> OPT__QUIET(&quiet, N_("don't print the patch filenames")), >> +OPT_BOOL(0, "progress", &show_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
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", &signature_file, > N_("add a signature from a file")), > OPT__QUIET(&quiet, N_("don't print the patch filenames")), > + OPT_BOOL(0, "progress", &show_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
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", &numbered, NULL, @@ -1493,6 +1496,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix) OPT_FILENAME(0, "signature-file", &signature_file, N_("add a signature from a file")), OPT__QUIET(&quiet, N_("don't print the patch filenames")), + OPT_BOOL(0, "progress", &show_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(&progress); free(list); free(branch_name); string_list_clear(&extra_to, 0); -- 2.14.0.rc0.286.g44127d70e4