| Message ID | 20181022131828.21348-4-peartben@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3,1/3] reset: don't compute unstaged changes after reset when --quiet | expand |
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.
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.
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)