[v3,3/3] reset: warn when refresh_index() takes more than 2 seconds
diff mbox series

Message ID 20181022131828.21348-4-peartben@gmail.com
State New
Headers show
Series
  • [v3,1/3] reset: don't compute unstaged changes after reset when --quiet
Related show

Commit Message

Ben Peart Oct. 22, 2018, 1:18 p.m. UTC
From: Ben Peart <benpeart@microsoft.com>

refresh_index() is done after a reset command as an optimization.  Because
it can be an expensive call, warn the user if it takes more than 2 seconds
and tell them how to avoid it using the --quiet command line option or
reset.quiet config setting.

Signed-off-by: Ben Peart <benpeart@microsoft.com>
---
 builtin/reset.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

Comments

Junio C Hamano Oct. 23, 2018, 12:23 a.m. UTC | #1
Ben Peart <peartben@gmail.com> writes:

> From: Ben Peart <benpeart@microsoft.com>
>
> refresh_index() is done after a reset command as an optimization.  Because
> it can be an expensive call, warn the user if it takes more than 2 seconds
> and tell them how to avoid it using the --quiet command line option or
> reset.quiet config setting.

I am moderately negative on this step.  It will irritate users who
know about and still choose not to use the "--quiet" option, because
they want to gain performance in later real work and/or they want to
know what paths are now dirty.  A working tree that needs long time
to refresh will take long time to instead do "cached stat info says
it may be modified so let's run 'diff' for real---we may discover
that there wasn't any change after all" when a "git diff" is run
after a "reset --quiet" that does not refresh; i.e. there would be
valid reasons to run "reset" without "--quiet".

It feels a bit irresponsible to throw an ad without informing
pros-and-cons and to pretend that we are advising on BCP.  In
general, we do *not* advertise new features randomly like this.

Thanks.  The previous two steps looks quite sensible.
Ben Peart Oct. 23, 2018, 5:12 p.m. UTC | #2
On 10/22/2018 8:23 PM, Junio C Hamano wrote:
> Ben Peart <peartben@gmail.com> writes:
> 
>> From: Ben Peart <benpeart@microsoft.com>
>>
>> refresh_index() is done after a reset command as an optimization.  Because
>> it can be an expensive call, warn the user if it takes more than 2 seconds
>> and tell them how to avoid it using the --quiet command line option or
>> reset.quiet config setting.
> 
> I am moderately negative on this step.  It will irritate users who
> know about and still choose not to use the "--quiet" option, because
> they want to gain performance in later real work and/or they want to
> know what paths are now dirty.  A working tree that needs long time
> to refresh will take long time to instead do "cached stat info says
> it may be modified so let's run 'diff' for real---we may discover
> that there wasn't any change after all" when a "git diff" is run
> after a "reset --quiet" that does not refresh; i.e. there would be
> valid reasons to run "reset" without "--quiet".
> 
> It feels a bit irresponsible to throw an ad without informing
> pros-and-cons and to pretend that we are advising on BCP.  In
> general, we do *not* advertise new features randomly like this.
> 
> Thanks.  The previous two steps looks quite sensible.
> 

The challenge I'm trying to address is the discoverability of this 
significant performance win.  In earlier review feedback, all mention of 
this option speeding up reset was removed.  I added this patch to enable 
users to find out it even exists as an option.

While I modeled this on the untracked files/--uno and ahead/behind 
logic, I missed adding this to the 'advice' logic so that it can be 
turned off and avoid irritating users.  I'll send an updated patch that 
corrects that.

Patch
diff mbox series

diff --git a/builtin/reset.c b/builtin/reset.c
index 3b43aee544..d95a27d52e 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -25,6 +25,8 @@ 
 #include "submodule.h"
 #include "submodule-config.h"
 
+#define REFRESH_INDEX_DELAY_WARNING_IN_MS (2 * 1000)
+
 static const char * const git_reset_usage[] = {
 	N_("git reset [--mixed | --soft | --hard | --merge | --keep] [-q] [<commit>]"),
 	N_("git reset [-q] [<tree-ish>] [--] <paths>..."),
@@ -376,9 +378,19 @@  int cmd_reset(int argc, const char **argv, const char *prefix)
 			int flags = quiet ? REFRESH_QUIET : REFRESH_IN_PORCELAIN;
 			if (read_from_tree(&pathspec, &oid, intent_to_add))
 				return 1;
-			if (!quiet && get_git_work_tree())
+			if (!quiet && get_git_work_tree()) {
+				uint64_t t_begin, t_delta_in_ms;
+
+				t_begin = getnanotime();
 				refresh_index(&the_index, flags, NULL, NULL,
 					      _("Unstaged changes after reset:"));
+				t_delta_in_ms = (getnanotime() - t_begin) / 1000000;
+				if (t_delta_in_ms > REFRESH_INDEX_DELAY_WARNING_IN_MS) {
+					printf(_("\nIt took %.2f seconds to enumerate unstaged changes after reset.  You can\n"
+						"use '--quiet' to avoid this.  Set the config setting reset.quiet to true\n"
+						"to make this the default."), t_delta_in_ms / 1000.0);
+				}
+			}
 		} else {
 			int err = reset_index(&oid, reset_type, quiet);
 			if (reset_type == KEEP && !err)