[PATCH v1 0/2] speed up git reset

[PATCH v1 0/2] speed up git reset

[Ben Peart]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=UTF-8, Size: 1671 bytes --]

From: Ben Peart <benpeart@microsoft.com>

The reset (mixed) command unstages the specified file(s) and then shows you
the remaining unstaged changes.  This can make the command slow on larger
repos because at the end it calls refresh_index() which has a single thread
that loops through all the entries calling lstat() for every file.

If the user passes the --quiet switch, reset doesn’t display the remaining
unstaged changes but it still does all the work to find them, it just
doesn’t print them out so passing "--quiet" doesn’t help performance.

This patch series will:

1) change the behavior of "git reset --quiet" so that it no longer computes
   the remaining unstaged changes.
   
2) add a new config setting so that "--quiet" can be configured as the default
   so that the default performance of "git reset" is improved.
   
The performance benefit of this can be significant.  In a repo with 200K files
"git reset foo" performance drops from 7.16 seconds to 0.32 seconds for a
savings of 96%.  Even with the small git repo, reset times drop from 0.191
seconds to 0.043 seconds for a savings of 77%.

Base Ref: master
Web-Diff: https://github.com/benpeart/git/commit/2295a310d0
Checkout: git fetch https://github.com/benpeart/git reset-refresh-index-v1 && git checkout 2295a310d0

Ben Peart (2):
  reset: don't compute unstaged changes after reset when --quiet
  reset: add new reset.quietDefault config setting

 Documentation/config.txt    | 6 ++++++
 Documentation/git-reset.txt | 4 +++-
 builtin/reset.c             | 3 ++-
 3 files changed, 11 insertions(+), 2 deletions(-)


base-commit: a4b8ab5363a32f283a61ef3a962853556d136c0e
-- 
2.18.0.windows.1

[PATCH v1 1/2] reset: don't compute unstaged changes after reset when --quiet

[Ben Peart]

From: Ben Peart <benpeart@microsoft.com>

When git reset is run with the --quiet flag, don't bother finding any
additional unstaged changes as they won't be output anyway.  This speeds up
the git reset command by avoiding having to lstat() every file looking for
changes that aren't going to be reported anyway.

The savings can be significant.  In a repo with 200K files "git reset"
drops from 7.16 seconds to 0.32 seconds for a savings of 96%.

Signed-off-by: Ben Peart <benpeart@microsoft.com>
---
 Documentation/git-reset.txt | 4 +++-
 builtin/reset.c             | 2 +-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-reset.txt b/Documentation/git-reset.txt
index 1d697d9962..8610309b55 100644
--- a/Documentation/git-reset.txt
+++ b/Documentation/git-reset.txt
@@ -95,7 +95,9 @@ OPTIONS
 
 -q::
 --quiet::
-	Be quiet, only report errors.
+	Be quiet, only report errors.  Can optimize the performance of reset
+	by avoiding scaning all files in the repo looking for additional
+	unstaged changes.
 
 
 EXAMPLES
diff --git a/builtin/reset.c b/builtin/reset.c
index 11cd0dcb8c..0822798616 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -376,7 +376,7 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
 			if (read_from_tree(&pathspec, &oid, intent_to_add))
 				return 1;
 			if (get_git_work_tree())
-				refresh_index(&the_index, flags, NULL, NULL,
+				refresh_index(&the_index, flags, quiet ? &pathspec : NULL, NULL,
 					      _("Unstaged changes after reset:"));
 		} else {
 			int err = reset_index(&oid, reset_type, quiet);
-- 
2.18.0.windows.1

Re: [PATCH v1 1/2] reset: don't compute unstaged changes after reset when --quiet

[Eric Sunshine]

On Wed, Oct 17, 2018 at 12:40 PM Ben Peart <peartben@gmail.com> wrote:
> When git reset is run with the --quiet flag, don't bother finding any
> additional unstaged changes as they won't be output anyway.  This speeds up
> the git reset command by avoiding having to lstat() every file looking for
> changes that aren't going to be reported anyway.
>
> Signed-off-by: Ben Peart <benpeart@microsoft.com>
> ---
> diff --git a/Documentation/git-reset.txt b/Documentation/git-reset.txt
> @@ -95,7 +95,9 @@ OPTIONS
>  --quiet::
> -       Be quiet, only report errors.
> +       Be quiet, only report errors.  Can optimize the performance of reset
> +       by avoiding scaning all files in the repo looking for additional
> +       unstaged changes.

s/scaning/scanning/

However, I'm not convinced that this should be documented here or at
least in this fashion. When I read this new documentation before
reading the commit message, I was baffled by what it was trying to say
since --quiet'ness is a superficial quality, not an optimizer. My
knee-jerk reaction is that it doesn't belong in end-user documentation
at all since it's an implementation detail, however, I can see that
such knowledge could be handy for people in situations which would be
helped by this. That said, if you do document it, this doesn't feel
like the correct place to do so; it should be in a "Discussion"
section or something. (Who would expect to find --quiet documentation
talking about optimizations? Likely, nobody.)

Re: [PATCH v1 1/2] reset: don't compute unstaged changes after reset when --quiet

[Jeff King]

On Wed, Oct 17, 2018 at 02:14:32PM -0400, Eric Sunshine wrote:

> > diff --git a/Documentation/git-reset.txt b/Documentation/git-reset.txt
> > @@ -95,7 +95,9 @@ OPTIONS
> >  --quiet::
> > -       Be quiet, only report errors.
> > +       Be quiet, only report errors.  Can optimize the performance of reset
> > +       by avoiding scaning all files in the repo looking for additional
> > +       unstaged changes.
> 
> s/scaning/scanning/
> 
> However, I'm not convinced that this should be documented here or at
> least in this fashion. When I read this new documentation before
> reading the commit message, I was baffled by what it was trying to say
> since --quiet'ness is a superficial quality, not an optimizer. My
> knee-jerk reaction is that it doesn't belong in end-user documentation
> at all since it's an implementation detail, however, I can see that
> such knowledge could be handy for people in situations which would be
> helped by this. That said, if you do document it, this doesn't feel
> like the correct place to do so; it should be in a "Discussion"
> section or something. (Who would expect to find --quiet documentation
> talking about optimizations? Likely, nobody.)

Yeah, I had the same thought. You'd probably choose --quiet because you
want it, you know, quiet.

Whereas for the new config variable, you'd probably set it not because
you want it quiet all the time, but because you want to get some time
savings. So there it does make sense to me to explain.

Other than that, this seems like an obvious and easy win. It does feel a
little hacky (you're really losing something in the output, and ideally
we'd just be able to give that answer quickly), but this may be OK as a
hack in the interim.

The sad thing is just that it's user-facing, so we have to respect it
forever. I almost wonder if there should be a global core.optimizeMessages
or something that tries to tradeoff less information for speed in all
commands, but makes no promises about which. Then a user with a big repo
who sets it once will get the benefit as more areas are identified (I
think "status" already has a similar case with ahead/behind)? And vice
versa, as some messages get faster to produce, they can be dropped from
that option.

I dunno. Maybe that is a stupid idea, and people really do want to
control it on a per-message basis.

-Peff

Re: [PATCH v1 1/2] reset: don't compute unstaged changes after reset when --quiet

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Whereas for the new config variable, you'd probably set it not because
> you want it quiet all the time, but because you want to get some time
> savings. So there it does make sense to me to explain.
>
> Other than that, this seems like an obvious and easy win. It does feel a
> little hacky (you're really losing something in the output, and ideally
> we'd just be able to give that answer quickly), but this may be OK as a
> hack in the interim.

After "git reset --quiet -- this/area/" with this change, any
operation you'd do next that needs to learn if working tree files
are different from what is recorded in the index outside that area
will have to spend more cycles, because the refresh done by "reset"
is now limited to the area.  So if your final goal is "make 'reset'
as fast as possible", this is an obvious and easy win.  For other
goals, i.e. "make the overall experience of using Git feel faster",
it is not so obvious to me, though.

If we somehow know that it is much less important in your setup that
the cached stat bits in the index is kept up to date (e.g. perhaps
you are more heavily relying on fsmonitor and are happy with it),
then I suspect that we could even skip the refreshing altogether and
gain more performance, without sacrificing the "overall experience
of using Git" at all, which would be even better.

> The sad thing is just that it's user-facing, so we have to respect it
> forever. I almost wonder if there should be a global core.optimizeMessages
> or something that tries to tradeoff less information for speed in all
> commands, but makes no promises about which. Then a user with a big repo
> who sets it once will get the benefit as more areas are identified (I
> think "status" already has a similar case with ahead/behind)? And vice
> versa, as some messages get faster to produce, they can be dropped from
> that option.
>
> I dunno. Maybe that is a stupid idea, and people really do want to
> control it on a per-message basis.
>
> -Peff

Re: [PATCH v1 1/2] reset: don't compute unstaged changes after reset when --quiet

[Jeff King]

On Thu, Oct 18, 2018 at 12:40:48PM +0900, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > Whereas for the new config variable, you'd probably set it not because
> > you want it quiet all the time, but because you want to get some time
> > savings. So there it does make sense to me to explain.
> >
> > Other than that, this seems like an obvious and easy win. It does feel a
> > little hacky (you're really losing something in the output, and ideally
> > we'd just be able to give that answer quickly), but this may be OK as a
> > hack in the interim.
> 
> After "git reset --quiet -- this/area/" with this change, any
> operation you'd do next that needs to learn if working tree files
> are different from what is recorded in the index outside that area
> will have to spend more cycles, because the refresh done by "reset"
> is now limited to the area.  So if your final goal is "make 'reset'
> as fast as possible", this is an obvious and easy win.  For other
> goals, i.e. "make the overall experience of using Git feel faster",
> it is not so obvious to me, though.
> 
> If we somehow know that it is much less important in your setup that
> the cached stat bits in the index is kept up to date (e.g. perhaps
> you are more heavily relying on fsmonitor and are happy with it),
> then I suspect that we could even skip the refreshing altogether and
> gain more performance, without sacrificing the "overall experience
> of using Git" at all, which would be even better.

Yeah, I assumed that Ben was using fsmonitor. I agree if we can just use
that to make this output faster, that would be the ideal. This is the
"later the message would get faster to produce" I hinted at in my
earlier message.

So I think we are in agreement. It just isn't clear to me how much work
it would take to get to the "ideal". If it's long enough, then this kind
of hackery may be useful in the meantime.

-Peff

Re: [PATCH v1 1/2] reset: don't compute unstaged changes after reset when --quiet

[Ben Peart]

On 10/18/2018 2:36 AM, Jeff King wrote:
> On Thu, Oct 18, 2018 at 12:40:48PM +0900, Junio C Hamano wrote:
> 
>> Jeff King <peff@peff.net> writes:
>>
>>> Whereas for the new config variable, you'd probably set it not because
>>> you want it quiet all the time, but because you want to get some time
>>> savings. So there it does make sense to me to explain.
>>>
>>> Other than that, this seems like an obvious and easy win. It does feel a
>>> little hacky (you're really losing something in the output, and ideally
>>> we'd just be able to give that answer quickly), but this may be OK as a
>>> hack in the interim.
>>
>> After "git reset --quiet -- this/area/" with this change, any
>> operation you'd do next that needs to learn if working tree files
>> are different from what is recorded in the index outside that area
>> will have to spend more cycles, because the refresh done by "reset"
>> is now limited to the area.  So if your final goal is "make 'reset'
>> as fast as possible", this is an obvious and easy win.  For other
>> goals, i.e. "make the overall experience of using Git feel faster",
>> it is not so obvious to me, though.

The final goal is to make git faster (especially on larger repos) and 
this proposal accomplishes that.  Let's look at why that is.

By scoping down (or eliminating) what refresh_index() has to lstat() at 
the end of the reset command, clearly the reset command is faster.  Yes, 
the index isn't as "fresh" because not everything was updated but that 
doesn't typically impact the performance of subsequent commands.

On the next command, git still has to lstat() every file because it 
isn't sure what changes could have happened in the file system.  As a 
result, the overall impact is that we have had to lstat() every file one 
fewer times between the two commands.  A net win overall.

In addition, the preload_index() code that does the lstat() command is 
highly optimized across multiple threads (and on Windows takes advantage 
of the fscache).  This means that it can lstat() every file _much_ 
faster than the single threaded loop in refresh_index().  This also 
makes the overall performance of the pair of git commands faster as we 
got rid of the slow lstat() loop and kept the fast one.

Here are some numbers to demonstrate that.  These are hot cache numbers 
as they are easier to generate.  Cold cache numbers make the net perf 
win significantly better as the cost for the reset jumps from 2.43 
seconds to 7.16 seconds.

0.32 git add asdf
0.31 git -c reset.quiet=true reset asdf
1.34 git status
1.97 Total


0.32 git add asdf
2.43 git -c reset.quiet=false reset asdf
1.32 git status
4.07 Total

Note the status command after the reset doesn't really change as it 
still must lstat() every file (the 0.02 difference is well within the 
variability of run to run differences).

FWIW, none of these numbers are using fsmonitor.



There was additional discussion about whether this should be tied to the 
"--quiet" option and how it should be documented.

One option would be to change the default behavior of reset so that it 
doesn't do the refresh_index() call at all.  This speeds up reset by 
default so there are no user discoverability issues but changes the 
default behavior which is an issue.

Another option that was suggested was to add a separate flag that could 
be passed to reset so that the "quiet" and "fast" options don't get 
conflated.  I don't care for that option because the two options (at 
this point and for the foreseeable future) would be identical in 
behavior from the end users perspective.

It was also suggested that there be a single "fast and quiet" option for 
all of git instead of separate options for each command.  I worry about 
that because now we're forcing users to choose between the "fast and 
quiet" version of git and the "slow and noisy" version.  How do we help 
them decide which they want?  That seems difficult to explain so that 
they can make a rational choice and also hard to discover.  I also have 
to wonder who would say "give me the slow and noisy version please." :)

I'd prefer we systematically move towards a model where the default 
values that are chosen for various settings throughout the code are all 
configurable via settings.  All defaults by necessity make certain 
assumptions about user preference, data shape, machine performance, etc 
and if those assumptions don't match the user's environment then the 
hard coded defaults aren't appropriate.  We do our best but its going to 
be hit or miss.

A consistent way to be able to change those defaults would be very 
useful in those circumstances.  To be clear, I'm not proposing we do a 
wholesale update of our preferences model at this point in time - that 
seems like a significant undertaking and I don't want to tie this 
specific optimization to a potential change in how default settings work.


To move this forward, here is what I propose:

1) If the '--quiet' flag is passed, we silently take advantage of the 
fact we can avoid having to do an "extra" lstat() of every file and 
scope the refresh_index() call to those paths that we know have changed.

2) I can remove the note in the documentation of --quiet which I only 
added to facilitate discoverability.

3) I can also edit the documentation for reset.quietDefault (maybe I 
should rename that to "reset.quiet"?) so that it does not discuss the 
potential performance impact.

4) To improve the discoverability of the enhanced performance, I could 
add logic similar to what exists for "status --uno" and if 
refresh_index() takes > x seconds, prompt the user with something like:

"It took %.2f seconds to enumerate unstaged changes after reset. 'reset 
--quiet' may speed it up. Set the config setting reset.quiet to true to 
make this the default."


>>
>> If we somehow know that it is much less important in your setup that
>> the cached stat bits in the index is kept up to date (e.g. perhaps
>> you are more heavily relying on fsmonitor and are happy with it),
>> then I suspect that we could even skip the refreshing altogether and
>> gain more performance, without sacrificing the "overall experience
>> of using Git" at all, which would be even better.
> 
> Yeah, I assumed that Ben was using fsmonitor. I agree if we can just use
> that to make this output faster, that would be the ideal. This is the
> "later the message would get faster to produce" I hinted at in my
> earlier message.
> 
> So I think we are in agreement. It just isn't clear to me how much work
> it would take to get to the "ideal". If it's long enough, then this kind
> of hackery may be useful in the meantime.
> 

I actually started my effort to speed up reset by attempting to 
multi-thread refresh_index().  You can see a work in progress at:

https://github.com/benpeart/git/pull/new/refresh-index-multithread-gvfs

The patch doesn't always work as it is still not thread safe.  When it 
works, it's great but I ran into to many difficulties trying to debug 
the remaining threading issues (even adding print statements would 
change the timing and the repro would disappear).  It will take a lot of 
code review to discover and fix the remaining non-thread safe code paths.

In addition, the optimized code path that takes advantage of fsmonitor, 
uses multiple threads, fscache, etc _already exists_ in preload_index(). 
  Trying to recreate all those optimizations in refresh_index() is (as I 
discovered) a daunting task.

This patch was tiny/trivial in comparison and provided all the 
performance benefits so seems like a much better option at this point in 
time.  For now, I suggest we just use that existing path as it provides 
the benefits without the significant additional work and complexity.

> -Peff
> 

Re: [PATCH v1 1/2] reset: don't compute unstaged changes after reset when --quiet

[Duy Nguyen]

On Thu, Oct 18, 2018 at 8:18 PM Ben Peart <peartben@gmail.com> wrote:
> I actually started my effort to speed up reset by attempting to
> multi-thread refresh_index().  You can see a work in progress at:
>
> https://github.com/benpeart/git/pull/new/refresh-index-multithread-gvfs
>
> The patch doesn't always work as it is still not thread safe.  When it
> works, it's great but I ran into to many difficulties trying to debug
> the remaining threading issues (even adding print statements would
> change the timing and the repro would disappear).  It will take a lot of
> code review to discover and fix the remaining non-thread safe code paths.
>
> In addition, the optimized code path that takes advantage of fsmonitor,
> uses multiple threads, fscache, etc _already exists_ in preload_index().
>   Trying to recreate all those optimizations in refresh_index() is (as I
> discovered) a daunting task.

Why not make refresh_index() run preload_index() first (or the
parallel lstat part to be precise), and only do the heavy
content-based refresh in single thread mode?
-- 
Duy

Re: [PATCH v1 1/2] reset: don't compute unstaged changes after reset when --quiet

[Ben Peart]

On 10/18/2018 2:26 PM, Duy Nguyen wrote:
> On Thu, Oct 18, 2018 at 8:18 PM Ben Peart <peartben@gmail.com> wrote:
>> I actually started my effort to speed up reset by attempting to
>> multi-thread refresh_index().  You can see a work in progress at:
>>
>> https://github.com/benpeart/git/pull/new/refresh-index-multithread-gvfs
>>
>> The patch doesn't always work as it is still not thread safe.  When it
>> works, it's great but I ran into to many difficulties trying to debug
>> the remaining threading issues (even adding print statements would
>> change the timing and the repro would disappear).  It will take a lot of
>> code review to discover and fix the remaining non-thread safe code paths.
>>
>> In addition, the optimized code path that takes advantage of fsmonitor,
>> uses multiple threads, fscache, etc _already exists_ in preload_index().
>>    Trying to recreate all those optimizations in refresh_index() is (as I
>> discovered) a daunting task.
> 
> Why not make refresh_index() run preload_index() first (or the
> parallel lstat part to be precise), and only do the heavy
> content-based refresh in single thread mode?
> 

Head smack! Why didn't I think of that?

That is a terrific suggestion.  Calling preload_index() right before the 
big for loop in refresh_index() is a trivial and effective way to do the 
bulk of the updating with the optimized code.  After doing that, most of 
the cache entries can bail out quickly down in refresh_cache_ent() when 
it tests ce_uptodate(ce).

Here are the numbers using that optimization (hot cache, averaged across 
3 runs):

0.32 git add asdf
1.67 git reset asdf
1.68 git status
3.67 Total

vs without it:

0.32 git add asdf
2.48 git reset asdf
1.50 git status
4.30 Total

For a savings in the reset command of 32% and 15% overall.

Clearly doing the refresh_index() faster is not as much savings as not 
doing it at all.  Given how simple this patch is, I think it makes sense 
to do both so that we have optimized each path to is fullest.

Re: [PATCH v1 1/2] reset: don't compute unstaged changes after reset when --quiet

[Junio C Hamano]

Ben Peart <peartben@gmail.com> writes:

> Note the status command after the reset doesn't really change as it
> still must lstat() every file (the 0.02 difference is well within the
> variability of run to run differences).

Of course, it would not make an iota of difference, whether reset
refreshes the cached stat index fully, to the cost of later lstat().
What the refreshing saves is having to scan the contents to find that
the file is unchanged at runtime.

If your lstat() is not significantly faster than opening and
scanning the file, the optimization based on the cached-stat
information becomes moot.  In a working tree full of unmodified
files, stale cached-stat info in the index will cause us to compare
the contents and waste a lot of time, and that is what refreshing
avoids.  If the "status" in your test sequence do not have to do
that (e.g. the cached-stat information is already up-to-date and
there is no point running refresh in reset), then I'd expect no
difference between these two tests.

> To move this forward, here is what I propose:
>
> 1) If the '--quiet' flag is passed, we silently take advantage of the
> fact we can avoid having to do an "extra" lstat() of every file and
> scope the refresh_index() call to those paths that we know have
> changed.

That's pretty much what the patch under discussion does.

> 2) I can remove the note in the documentation of --quiet which I only
> added to facilitate discoverability.

Quite honestly, I am not sure if this (meaning #1 above) alone need
to be even discoverable.  Those who want --quiet output would use
it, those who want to be told which paths are modified would not,
and those who want to quickly be told which paths are modified would
not be helped by the limited refresh anyway, so "with --quiet you
can make it go faster" would not help anybody.

> 3) I can also edit the documentation for reset.quietDefault (maybe I
> should rename that to "reset.quiet"?) so that it does not discuss the
> potential performance impact.

I think reset.quiet (or reset.verbosity) is a good thing to have
regardless.

[PATCH v1 2/2] reset: add new reset.quietDefault config setting

[Ben Peart]

From: Ben Peart <benpeart@microsoft.com>

Add a reset.quietDefault config setting that sets the default value of the
--quiet flag when running the reset command.  This enables users to change
the default behavior to take advantage of the performance advantages of
avoiding the scan for unstaged changes after reset.  Defaults to false.

Signed-off-by: Ben Peart <benpeart@microsoft.com>
---
 Documentation/config.txt | 6 ++++++
 builtin/reset.c          | 1 +
 2 files changed, 7 insertions(+)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index f6f4c21a54..a5cf4c019b 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2728,6 +2728,12 @@ rerere.enabled::
 	`$GIT_DIR`, e.g. if "rerere" was previously used in the
 	repository.
 
+reset.quietDefault::
+	Sets the default value of the "quiet" option for the reset command.
+	Choosing "quiet" can optimize the performance of the reset command
+	by avoiding the scan of all files in the repo looking for additional
+	unstaged changes. Defaults to false.
+
 include::sendemail-config.txt[]
 
 sequence.editor::
diff --git a/builtin/reset.c b/builtin/reset.c
index 0822798616..7d151d48a0 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -306,6 +306,7 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
 	};
 
 	git_config(git_reset_config, NULL);
+	git_config_get_bool("reset.quietDefault", &quiet);
 
 	argc = parse_options(argc, argv, prefix, options, git_reset_usage,
 						PARSE_OPT_KEEP_DASHDASH);
-- 
2.18.0.windows.1

Re: [PATCH v1 2/2] reset: add new reset.quietDefault config setting

[Eric Sunshine]

On Wed, Oct 17, 2018 at 12:40 PM Ben Peart <peartben@gmail.com> wrote:
> Add a reset.quietDefault config setting that sets the default value of the
> --quiet flag when running the reset command.  This enables users to change
> the default behavior to take advantage of the performance advantages of
> avoiding the scan for unstaged changes after reset.  Defaults to false.

As with the previous patch, my knee-jerk reaction is that this really
feels wrong being tied to --quiet. It's particularly unintuitive.

What I _could_ see, and what would feel more natural is if you add a
new option (say, --optimize) which is more general, incorporating
whatever optimizations become available in the future, not just this
one special-case. A side-effect of --optimize is that it implies
--quiet, and that is something which can and should be documented.

> Signed-off-by: Ben Peart <benpeart@microsoft.com>

Re: [PATCH v1 2/2] reset: add new reset.quietDefault config setting

[Jeff King]

On Wed, Oct 17, 2018 at 02:19:59PM -0400, Eric Sunshine wrote:

> On Wed, Oct 17, 2018 at 12:40 PM Ben Peart <peartben@gmail.com> wrote:
> > Add a reset.quietDefault config setting that sets the default value of the
> > --quiet flag when running the reset command.  This enables users to change
> > the default behavior to take advantage of the performance advantages of
> > avoiding the scan for unstaged changes after reset.  Defaults to false.
> 
> As with the previous patch, my knee-jerk reaction is that this really
> feels wrong being tied to --quiet. It's particularly unintuitive.
> 
> What I _could_ see, and what would feel more natural is if you add a
> new option (say, --optimize) which is more general, incorporating
> whatever optimizations become available in the future, not just this
> one special-case. A side-effect of --optimize is that it implies
> --quiet, and that is something which can and should be documented.

Heh, I just wrote something very similar elsewhere in the thread. I'm
still not sure if it's a dumb idea, but at least we can be dumb
together.

-Peff

Re: [PATCH v1 2/2] reset: add new reset.quietDefault config setting

[Ævar Arnfjörð Bjarmason]

On Wed, Oct 17 2018, Jeff King wrote:

> On Wed, Oct 17, 2018 at 02:19:59PM -0400, Eric Sunshine wrote:
>
>> On Wed, Oct 17, 2018 at 12:40 PM Ben Peart <peartben@gmail.com> wrote:
>> > Add a reset.quietDefault config setting that sets the default value of the
>> > --quiet flag when running the reset command.  This enables users to change
>> > the default behavior to take advantage of the performance advantages of
>> > avoiding the scan for unstaged changes after reset.  Defaults to false.
>>
>> As with the previous patch, my knee-jerk reaction is that this really
>> feels wrong being tied to --quiet. It's particularly unintuitive.
>>
>> What I _could_ see, and what would feel more natural is if you add a
>> new option (say, --optimize) which is more general, incorporating
>> whatever optimizations become available in the future, not just this
>> one special-case. A side-effect of --optimize is that it implies
>> --quiet, and that is something which can and should be documented.
>
> Heh, I just wrote something very similar elsewhere in the thread. I'm
> still not sure if it's a dumb idea, but at least we can be dumb
> together.

Same here. I'm in general if favor of having the ability to configure
porcelain command-line options, but in this case it seems like it would
be more logical to head for something like:

    core.uiMessaging=[default,exhaustive,lossyButFaster,quiet]

Where default would be our current "exhaustive", and this --quiet case
would be covered by lossyButFaster, but also things like the
"--no-ahead-behind" flag for git-status.

Just on this implementation: The usual idiom for flags as config is
command.flag=xyz, not command.flagDefault=xyz, so this should be
reset.quiet.

Re: [PATCH v1 2/2] reset: add new reset.quietDefault config setting

[Ben Peart]

On 10/23/2018 5:13 AM, Ævar Arnfjörð Bjarmason wrote:
> 
> On Wed, Oct 17 2018, Jeff King wrote:
> 
>> On Wed, Oct 17, 2018 at 02:19:59PM -0400, Eric Sunshine wrote:
>>
>>> On Wed, Oct 17, 2018 at 12:40 PM Ben Peart <peartben@gmail.com> wrote:
>>>> Add a reset.quietDefault config setting that sets the default value of the
>>>> --quiet flag when running the reset command.  This enables users to change
>>>> the default behavior to take advantage of the performance advantages of
>>>> avoiding the scan for unstaged changes after reset.  Defaults to false.
>>>
>>> As with the previous patch, my knee-jerk reaction is that this really
>>> feels wrong being tied to --quiet. It's particularly unintuitive.
>>>
>>> What I _could_ see, and what would feel more natural is if you add a
>>> new option (say, --optimize) which is more general, incorporating
>>> whatever optimizations become available in the future, not just this
>>> one special-case. A side-effect of --optimize is that it implies
>>> --quiet, and that is something which can and should be documented.
>>
>> Heh, I just wrote something very similar elsewhere in the thread. I'm
>> still not sure if it's a dumb idea, but at least we can be dumb
>> together.
> 
> Same here. I'm in general if favor of having the ability to configure
> porcelain command-line options, but in this case it seems like it would
> be more logical to head for something like:
> 
>      core.uiMessaging=[default,exhaustive,lossyButFaster,quiet]
> 
> Where default would be our current "exhaustive", and this --quiet case
> would be covered by lossyButFaster, but also things like the
> "--no-ahead-behind" flag for git-status.
> 

This sounds like an easy way to choose a set of default values that we 
think make sense to get bundled together. That could be a way for users 
to quickly choose a set of good defaults but I still think you would 
want find grained control over the individual settings.

Coming up with the set of values to bundle together, figuring out the 
hierarchy of precedence for this new global config->individual 
config->individual command line, updating the code to make it all work 
is outside the scope of this particular patch series.

> Just on this implementation: The usual idiom for flags as config is
> command.flag=xyz, not command.flagDefault=xyz, so this should be
> reset.quiet.
> 

Thanks, I agree and fixed that in later iterations.

Re: [PATCH v1 2/2] reset: add new reset.quietDefault config setting

[Jeff King]

On Tue, Oct 23, 2018 at 02:11:01PM -0400, Ben Peart wrote:

> This sounds like an easy way to choose a set of default values that we think
> make sense to get bundled together. That could be a way for users to quickly
> choose a set of good defaults but I still think you would want find grained
> control over the individual settings.
> 
> Coming up with the set of values to bundle together, figuring out the
> hierarchy of precedence for this new global config->individual
> config->individual command line, updating the code to make it all work is
> outside the scope of this particular patch series.

True, it probably does make sense to give individual defaults. Having a
unifying option may help with the discoverability issue you were
thinking of elsewhere, though.

-Peff

Re: [PATCH v1 2/2] reset: add new reset.quietDefault config setting

[Ævar Arnfjörð Bjarmason]

On Tue, Oct 23 2018, Ben Peart wrote:

> On 10/23/2018 5:13 AM, Ævar Arnfjörð Bjarmason wrote:
>>
>> On Wed, Oct 17 2018, Jeff King wrote:
>>
>>> On Wed, Oct 17, 2018 at 02:19:59PM -0400, Eric Sunshine wrote:
>>>
>>>> On Wed, Oct 17, 2018 at 12:40 PM Ben Peart <peartben@gmail.com> wrote:
>>>>> Add a reset.quietDefault config setting that sets the default value of the
>>>>> --quiet flag when running the reset command.  This enables users to change
>>>>> the default behavior to take advantage of the performance advantages of
>>>>> avoiding the scan for unstaged changes after reset.  Defaults to false.
>>>>
>>>> As with the previous patch, my knee-jerk reaction is that this really
>>>> feels wrong being tied to --quiet. It's particularly unintuitive.
>>>>
>>>> What I _could_ see, and what would feel more natural is if you add a
>>>> new option (say, --optimize) which is more general, incorporating
>>>> whatever optimizations become available in the future, not just this
>>>> one special-case. A side-effect of --optimize is that it implies
>>>> --quiet, and that is something which can and should be documented.
>>>
>>> Heh, I just wrote something very similar elsewhere in the thread. I'm
>>> still not sure if it's a dumb idea, but at least we can be dumb
>>> together.
>>
>> Same here. I'm in general if favor of having the ability to configure
>> porcelain command-line options, but in this case it seems like it would
>> be more logical to head for something like:
>>
>>      core.uiMessaging=[default,exhaustive,lossyButFaster,quiet]
>>
>> Where default would be our current "exhaustive", and this --quiet case
>> would be covered by lossyButFaster, but also things like the
>> "--no-ahead-behind" flag for git-status.
>>
>
> This sounds like an easy way to choose a set of default values that we
> think make sense to get bundled together. That could be a way for
> users to quickly choose a set of good defaults but I still think you
> would want find grained control over the individual settings.

Would you? It seems wanting to configure reset's --quiet in particular
is purely a proxy goal for wanting to toggle off slow things in the
UI. Otherwise why focus on it, and not the plethora of other --quiet
options we have?

    # Including (but probably not limited to):
    $ git grep -e OPT__QUIET -e '(OPT|option).*"quiet"' -- '*.[ch]' | wc -l
    34

> Coming up with the set of values to bundle together, figuring out the
> hierarchy of precedence for this new global config->individual
> config->individual command line[...]

If we'd still want reset.quiet & whatever the global "turn off slow
stuff" UI option is then this part is easy and
e.g. {transfer,fetch,receive}.fsckObjects can be used as a template for
how to do it.

    https://github.com/git/git/blob/v2.19.0/fetch-pack.c#L1432-L1443
    https://github.com/git/git/blob/v2.19.0/fetch-pack.c#L859-L863

I.e. the more specific option always overrides the less specific one.

> [...]updating the code to make it all work is outside the scope of
> this particular patch series.

Is that a Jedi mind trick to get out of patch review? :)

I understand that it's not the patch you wrote, but sometimes feedback
is "maybe we shouldn't do this, but this other thing".

The --ahead-behind config setting stalled on-list before:
https://public-inbox.org/git/36e3a9c3-f7e2-4100-1bfc-647b809a09d0@jeffhostetler.com/

Now we have this similarly themed thing.

I think we need to be mindful of how changes like this can add up to
very confusing UI. I.e. in this case I can see a "how take make git fast
on large repos" post on stackoverflow in our future where the answer is
setting a bunch of seemingly irrelevant config options like reset.quiet
and status.aheadbehind=false etc.

So maybe we should take a step back and consider if the real thing we
want is just some way for the user to tell git "don't work so hard at
coming up with these values".

That can also be smart, e.g. some "auto" setting that tweaks it based on
estimated repo size so even with the same config your tiny dotfiles.git
will get "ahead/behind" reporting, but not when you cd into windows.git.

>> Just on this implementation: The usual idiom for flags as config is
>> command.flag=xyz, not command.flagDefault=xyz, so this should be
>> reset.quiet.
>>
>
> Thanks, I agree and fixed that in later iterations.

Recommended configurations (was Re: [PATCH v1 2/2] reset: add new reset.quietDefault config setting)

[Derrick Stolee]

On 10/23/2018 4:03 PM, Ævar Arnfjörð Bjarmason wrote:
> [snip]
> The --ahead-behind config setting stalled on-list before:
> https://public-inbox.org/git/36e3a9c3-f7e2-4100-1bfc-647b809a09d0@jeffhostetler.com/
>
> Now we have this similarly themed thing.
>
> I think we need to be mindful of how changes like this can add up to
> very confusing UI. I.e. in this case I can see a "how take make git fast
> on large repos" post on stackoverflow in our future where the answer is
> setting a bunch of seemingly irrelevant config options like reset.quiet
> and status.aheadbehind=false etc.
>
> So maybe we should take a step back and consider if the real thing we
> want is just some way for the user to tell git "don't work so hard at
> coming up with these values".
>
> That can also be smart, e.g. some "auto" setting that tweaks it based on
> estimated repo size so even with the same config your tiny dotfiles.git
> will get "ahead/behind" reporting, but not when you cd into windows.git.

Generally, there are a lot of config settings that are likely in the "if 
you have a big repo, then you should use this" category. However, there 
is rarely a one-size-fits-all solution to these problems, just like 
there are different ways a repo can be "big" (working directory? number 
of commits? submodules?).

I would typically expect that users with _really_ big repos have the 
resources to have an expert tweak the settings that are best for that 
data shape and share those settings with all the users. In VFS for Git, 
we turn certain config settings on by default when mounting the repo 
[1], but others are either signaled through warning messages (like the 
status.aheadBehind config setting [2]).

We never upstreamed the status.aheadBehind config setting [2], but we 
_did_ upstream the command-line option as fd9b544 "status: add 
--[no-]ahead-behind to status and commit for V2 format". We didn't want 
to change the expected output permanently, so we didn't add the config 
setting to our list of "required" settings, but instead created a list 
of optional settings [3]; these settings don't override the existing 
settings so users can opt-out. (Now that we have the commit-graph 
enabled and kept up-to-date, it may be time to revisit the importance of 
this setting.)

All of this is to say: it is probably a good idea to have some 
"recommended configuration" for big repos, but there will always be 
power users who want to tweak each and every one of these settings. I'm 
open to design ideas of how to store a list of recommended 
configurations and how to set a group of config settings with one 
command (say, a "git recommended-config [small|large|submodules]" 
builtin that fills the local config with the important settings).

Thanks,
-Stolee

[1] 
https://github.com/Microsoft/VFSForGit/blob/7daa9f1133764a4e4bd87014833fc2091e6702c1/GVFS/GVFS/CommandLine/GVFSVerb.cs#L79-L104
     Code in VFS for Git that enables "required" config settings.

[2] 
https://github.com/Microsoft/git/commit/0cbe9e6b23e4d9008d4a1676e1dd6a87bdcd6ed5
     status: add status.aheadbehind setting

[3] 
https://github.com/Microsoft/VFSForGit/blob/7daa9f1133764a4e4bd87014833fc2091e6702c1/GVFS/GVFS/CommandLine/GVFSVerb.cs#L120-L123
     Code in VFS for Git that enables "optional" config settings.

Re: Recommended configurations (was Re: [PATCH v1 2/2] reset: add new reset.quietDefault config setting)

[Jeff King]

On Wed, Oct 24, 2018 at 11:48:20AM -0400, Derrick Stolee wrote:

> Generally, there are a lot of config settings that are likely in the "if you
> have a big repo, then you should use this" category. However, there is
> rarely a one-size-fits-all solution to these problems, just like there are
> different ways a repo can be "big" (working directory? number of commits?
> submodules?).
> [...]
> All of this is to say: it is probably a good idea to have some "recommended
> configuration" for big repos, but there will always be power users who want
> to tweak each and every one of these settings. I'm open to design ideas of
> how to store a list of recommended configurations and how to set a group of
> config settings with one command (say, a "git recommended-config
> [small|large|submodules]" builtin that fills the local config with the
> important settings).

Maybe it would be useful to teach git-sizer[1] to recommend particular
settings based on the actual definitions of "big" that it measures.

I do hope that some options will just be no-brainers to enable always,
though (e.g., I think in the long run commit-graph should just default
to "on"; it's cheap to keep up to date and helps proportionally to the
repo size).

[1] https://github.com/github/git-sizer

Re: Recommended configurations (was Re: [PATCH v1 2/2] reset: add new reset.quietDefault config setting)

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I do hope that some options will just be no-brainers to enable always,
> though (e.g., I think in the long run commit-graph should just default
> to "on"; it's cheap to keep up to date and helps proportionally to the
> repo size).

Same here.

We should strive to make any feature to optimize for repositories
with a particular trait not to hurt too much to repositories without
that trait, so that we can start such a feature as opt-in but later
can make it the default for everybody.  Sometimes it may not be
possible, but my gut feeling is that features aiming for optimizing
big repositories should fundamentally need only very small overhead
when enabled in a small repository.

So I view them not as a set of million "if your repository matches
this criterion, turn it on" knobs.  Rather, they are "we haven't
tested it fully, but you can opt into the experiment a new way to do
the same operation, which is designed to optimize for repositories
with this trait. Enabling it even when your repository does not have
that trait and reporting regression is also very welcome, as it is a
good indication that the new way has rough edges at its corners".

Thanks.

[PATCH v2 0/3] speed up git reset

[Ben Peart]

From: Ben Peart <benpeart@microsoft.com>

This itteration avoids the refresh_index() call completely if 'quiet'.
The advantage of this is that "git refresh" without any pathspec is also
significantly sped up.

Also added a notification if finding unstaged changes after reset takes
longer than 2 seconds to make users aware of the option to speed it up if
they don't need the unstaged changes after reset to be output.

It also renames the new config setting reset.quietDefault to reset.quiet.

Base Ref: 
Web-Diff: https://github.com/benpeart/git/commit/50d3415ef1
Checkout: git fetch https://github.com/benpeart/git reset-refresh-index-v2 && git checkout 50d3415ef1


### Interdiff (v1..v2):

diff --git a/Documentation/config.txt b/Documentation/config.txt
index a5cf4c019b..a2d1b8b116 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2728,11 +2728,8 @@ rerere.enabled::
 	`$GIT_DIR`, e.g. if "rerere" was previously used in the
 	repository.
 
-reset.quietDefault::
-	Sets the default value of the "quiet" option for the reset command.
-	Choosing "quiet" can optimize the performance of the reset command
-	by avoiding the scan of all files in the repo looking for additional
-	unstaged changes. Defaults to false.
+reset.quiet::
+	When set to true, 'git reset' will default to the '--quiet' option.
 
 include::sendemail-config.txt[]
 
diff --git a/Documentation/git-reset.txt b/Documentation/git-reset.txt
index 8610309b55..1d697d9962 100644
--- a/Documentation/git-reset.txt
+++ b/Documentation/git-reset.txt
@@ -95,9 +95,7 @@ OPTIONS
 
 -q::
 --quiet::
-	Be quiet, only report errors.  Can optimize the performance of reset
-	by avoiding scaning all files in the repo looking for additional
-	unstaged changes.
+	Be quiet, only report errors.
 
 
 EXAMPLES
diff --git a/builtin/reset.c b/builtin/reset.c
index 7d151d48a0..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>..."),
@@ -306,7 +308,7 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
 	};
 
 	git_config(git_reset_config, NULL);
-	git_config_get_bool("reset.quietDefault", &quiet);
+	git_config_get_bool("reset.quiet", &quiet);
 
 	argc = parse_options(argc, argv, prefix, options, git_reset_usage,
 						PARSE_OPT_KEEP_DASHDASH);
@@ -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 (get_git_work_tree())
-				refresh_index(&the_index, flags, quiet ? &pathspec : NULL, NULL,
+			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)


### Patches

Ben Peart (3):
  reset: don't compute unstaged changes after reset when --quiet
  reset: add new reset.quiet config setting
  reset: warn when refresh_index() takes more than 2 seconds

 Documentation/config.txt |  3 +++
 builtin/reset.c          | 15 ++++++++++++++-
 2 files changed, 17 insertions(+), 1 deletion(-)


base-commit: ca63497355222acefcca02b9cbb540a4768f3286
-- 
2.18.0.windows.1

[PATCH v2 1/3] reset: don't compute unstaged changes after reset when --quiet

[Ben Peart]

From: Ben Peart <benpeart@microsoft.com>

When git reset is run with the --quiet flag, don't bother finding any
additional unstaged changes as they won't be output anyway.  This speeds up
the git reset command by avoiding having to lstat() every file looking for
changes that aren't going to be reported anyway.

The savings can be significant.  In a repo with 200K files "git reset"
drops from 7.16 seconds to 0.32 seconds for a savings of 96%.

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

diff --git a/builtin/reset.c b/builtin/reset.c
index 11cd0dcb8c..04f0d9b4f5 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -375,7 +375,7 @@ 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 (get_git_work_tree())
+			if (!quiet && get_git_work_tree())
 				refresh_index(&the_index, flags, NULL, NULL,
 					      _("Unstaged changes after reset:"));
 		} else {
-- 
2.18.0.windows.1

[PATCH v2 2/3] reset: add new reset.quiet config setting

[Ben Peart]

From: Ben Peart <benpeart@microsoft.com>

Add a reset.quiet config setting that sets the default value of the --quiet
flag when running the reset command.  This enables users to change the
default behavior to take advantage of the performance advantages of
avoiding the scan for unstaged changes after reset.  Defaults to false.

Signed-off-by: Ben Peart <benpeart@microsoft.com>
---
 Documentation/config.txt | 3 +++
 builtin/reset.c          | 1 +
 2 files changed, 4 insertions(+)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index f6f4c21a54..a2d1b8b116 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2728,6 +2728,9 @@ rerere.enabled::
 	`$GIT_DIR`, e.g. if "rerere" was previously used in the
 	repository.
 
+reset.quiet::
+	When set to true, 'git reset' will default to the '--quiet' option.
+
 include::sendemail-config.txt[]
 
 sequence.editor::
diff --git a/builtin/reset.c b/builtin/reset.c
index 04f0d9b4f5..3b43aee544 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -306,6 +306,7 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
 	};
 
 	git_config(git_reset_config, NULL);
+	git_config_get_bool("reset.quiet", &quiet);
 
 	argc = parse_options(argc, argv, prefix, options, git_reset_usage,
 						PARSE_OPT_KEEP_DASHDASH);
-- 
2.18.0.windows.1

Re: [PATCH v2 2/3] reset: add new reset.quiet config setting

[Eric Sunshine]

On Fri, Oct 19, 2018 at 12:12 PM Ben Peart <peartben@gmail.com> wrote:
> Add a reset.quiet config setting that sets the default value of the --quiet
> flag when running the reset command.  This enables users to change the
> default behavior to take advantage of the performance advantages of
> avoiding the scan for unstaged changes after reset.  Defaults to false.
>
> Signed-off-by: Ben Peart <benpeart@microsoft.com>
> ---
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> @@ -2728,6 +2728,9 @@ rerere.enabled::
> +reset.quiet::
> +       When set to true, 'git reset' will default to the '--quiet' option.

How does the user reverse this for a particular git-reset invocation?
There is no --no-quiet or --verbose option.

Perhaps you want to use OPT__VERBOSITY() instead of OPT__QUIET() in
builtin/reset.c and document that --verbose overrides --quiet and
reset.quiet (or something like that).

Re: [PATCH v2 2/3] reset: add new reset.quiet config setting

[Jeff King]

On Fri, Oct 19, 2018 at 12:36:44PM -0400, Eric Sunshine wrote:

> On Fri, Oct 19, 2018 at 12:12 PM Ben Peart <peartben@gmail.com> wrote:
> > Add a reset.quiet config setting that sets the default value of the --quiet
> > flag when running the reset command.  This enables users to change the
> > default behavior to take advantage of the performance advantages of
> > avoiding the scan for unstaged changes after reset.  Defaults to false.
> >
> > Signed-off-by: Ben Peart <benpeart@microsoft.com>
> > ---
> > diff --git a/Documentation/config.txt b/Documentation/config.txt
> > @@ -2728,6 +2728,9 @@ rerere.enabled::
> > +reset.quiet::
> > +       When set to true, 'git reset' will default to the '--quiet' option.
> 
> How does the user reverse this for a particular git-reset invocation?
> There is no --no-quiet or --verbose option.
> 
> Perhaps you want to use OPT__VERBOSITY() instead of OPT__QUIET() in
> builtin/reset.c and document that --verbose overrides --quiet and
> reset.quiet (or something like that).

I think OPT__QUIET() provides --no-quiet, since it's really an
OPT_COUNTUP() under the hood. Saying "--no-quiet" should reset it back
to 0.

-Peff

Re: [PATCH v2 2/3] reset: add new reset.quiet config setting

[Eric Sunshine]

On Fri, Oct 19, 2018 at 12:46 PM Jeff King <peff@peff.net> wrote:
> On Fri, Oct 19, 2018 at 12:36:44PM -0400, Eric Sunshine wrote:
> > How does the user reverse this for a particular git-reset invocation?
> > There is no --no-quiet or --verbose option.
> >
> > Perhaps you want to use OPT__VERBOSITY() instead of OPT__QUIET() in
> > builtin/reset.c and document that --verbose overrides --quiet and
> > reset.quiet (or something like that).
>
> I think OPT__QUIET() provides --no-quiet, since it's really an
> OPT_COUNTUP() under the hood. Saying "--no-quiet" should reset it back
> to 0.

Okay. In any case, --no-quiet probably ought to be mentioned alongside
the "reset.quiet" option (and perhaps in git-reset.txt to as a way to
reverse "reset.quiet").

Re: [PATCH v2 2/3] reset: add new reset.quiet config setting

[Jeff King]

On Fri, Oct 19, 2018 at 01:10:34PM -0400, Eric Sunshine wrote:

> On Fri, Oct 19, 2018 at 12:46 PM Jeff King <peff@peff.net> wrote:
> > On Fri, Oct 19, 2018 at 12:36:44PM -0400, Eric Sunshine wrote:
> > > How does the user reverse this for a particular git-reset invocation?
> > > There is no --no-quiet or --verbose option.
> > >
> > > Perhaps you want to use OPT__VERBOSITY() instead of OPT__QUIET() in
> > > builtin/reset.c and document that --verbose overrides --quiet and
> > > reset.quiet (or something like that).
> >
> > I think OPT__QUIET() provides --no-quiet, since it's really an
> > OPT_COUNTUP() under the hood. Saying "--no-quiet" should reset it back
> > to 0.
> 
> Okay. In any case, --no-quiet probably ought to be mentioned alongside
> the "reset.quiet" option (and perhaps in git-reset.txt to as a way to
> reverse "reset.quiet").

Yes, I'd agree with that.

-Peff

Re: [PATCH v2 2/3] reset: add new reset.quiet config setting

[Ben Peart]

On 10/19/2018 1:11 PM, Jeff King wrote:
> On Fri, Oct 19, 2018 at 01:10:34PM -0400, Eric Sunshine wrote:
> 
>> On Fri, Oct 19, 2018 at 12:46 PM Jeff King <peff@peff.net> wrote:
>>> On Fri, Oct 19, 2018 at 12:36:44PM -0400, Eric Sunshine wrote:
>>>> How does the user reverse this for a particular git-reset invocation?
>>>> There is no --no-quiet or --verbose option.
>>>>
>>>> Perhaps you want to use OPT__VERBOSITY() instead of OPT__QUIET() in
>>>> builtin/reset.c and document that --verbose overrides --quiet and
>>>> reset.quiet (or something like that).
>>>
>>> I think OPT__QUIET() provides --no-quiet, since it's really an
>>> OPT_COUNTUP() under the hood. Saying "--no-quiet" should reset it back
>>> to 0.
>>
>> Okay. In any case, --no-quiet probably ought to be mentioned alongside
>> the "reset.quiet" option (and perhaps in git-reset.txt to as a way to
>> reverse "reset.quiet").
> 
> Yes, I'd agree with that.
> 
> -Peff
> 

Makes sense.  I'll update the docs to say:

-q::
--quiet::
--no-quiet::
	Be quiet, only report errors.
+
With --no-quiet report errors and unstaged changes after reset.

Re: [PATCH v2 2/3] reset: add new reset.quiet config setting

[Jeff King]

On Fri, Oct 19, 2018 at 01:23:06PM -0400, Ben Peart wrote:

> > > Okay. In any case, --no-quiet probably ought to be mentioned alongside
> > > the "reset.quiet" option (and perhaps in git-reset.txt to as a way to
> > > reverse "reset.quiet").
> [...]
> Makes sense.  I'll update the docs to say:
> 
> -q::
> --quiet::
> --no-quiet::
> 	Be quiet, only report errors.
> +
> With --no-quiet report errors and unstaged changes after reset.

I think we should be explicit that "--no-quiet" is already the default,
which makes it easy to mention the config option. Something like:

  -q::
  --quiet::
  --no-quiet::
	Be quiet, only report errors. The default behavior respects the
	`reset.quiet` config option, or `--no-quiet` if that is not set.

I don't know if we need to mention the "unstaged changes" thing. We may
grow other non-error messages (or may even have some now, I didn't
check). But I'm OK including it, too.

-Peff

Re: [PATCH v2 2/3] reset: add new reset.quiet config setting

[Junio C Hamano]

Ben Peart <peartben@gmail.com> writes:

> On 10/19/2018 1:11 PM, Jeff King wrote:
>> On Fri, Oct 19, 2018 at 01:10:34PM -0400, Eric Sunshine wrote:
>>
>>> On Fri, Oct 19, 2018 at 12:46 PM Jeff King <peff@peff.net> wrote:
>>>> On Fri, Oct 19, 2018 at 12:36:44PM -0400, Eric Sunshine wrote:
>>>>> How does the user reverse this for a particular git-reset invocation?
>>>>> There is no --no-quiet or --verbose option.
>>>>>
>>>>> Perhaps you want to use OPT__VERBOSITY() instead of OPT__QUIET() in
>>>>> builtin/reset.c and document that --verbose overrides --quiet and
>>>>> reset.quiet (or something like that).
>>>>
>>>> I think OPT__QUIET() provides --no-quiet, since it's really an
>>>> OPT_COUNTUP() under the hood. Saying "--no-quiet" should reset it back
>>>> to 0.
>>>
>>> Okay. In any case, --no-quiet probably ought to be mentioned alongside
>>> the "reset.quiet" option (and perhaps in git-reset.txt to as a way to
>>> reverse "reset.quiet").
>>
>> Yes, I'd agree with that.
>>
>> -Peff
>>
>
> Makes sense.  I'll update the docs to say:
>
> -q::
> --quiet::
> --no-quiet::
> 	Be quiet, only report errors.
> +
> With --no-quiet report errors and unstaged changes after reset.

Sounds good.  Thanks all.

Re: [PATCH v2 2/3] reset: add new reset.quiet config setting

[Ben Peart]

On 10/19/2018 12:46 PM, Jeff King wrote:
> On Fri, Oct 19, 2018 at 12:36:44PM -0400, Eric Sunshine wrote:
> 
>> On Fri, Oct 19, 2018 at 12:12 PM Ben Peart <peartben@gmail.com> wrote:
>>> Add a reset.quiet config setting that sets the default value of the --quiet
>>> flag when running the reset command.  This enables users to change the
>>> default behavior to take advantage of the performance advantages of
>>> avoiding the scan for unstaged changes after reset.  Defaults to false.
>>>
>>> Signed-off-by: Ben Peart <benpeart@microsoft.com>
>>> ---
>>> diff --git a/Documentation/config.txt b/Documentation/config.txt
>>> @@ -2728,6 +2728,9 @@ rerere.enabled::
>>> +reset.quiet::
>>> +       When set to true, 'git reset' will default to the '--quiet' option.
>>
>> How does the user reverse this for a particular git-reset invocation?
>> There is no --no-quiet or --verbose option.
>>
>> Perhaps you want to use OPT__VERBOSITY() instead of OPT__QUIET() in
>> builtin/reset.c and document that --verbose overrides --quiet and
>> reset.quiet (or something like that).
> 
> I think OPT__QUIET() provides --no-quiet, since it's really an
> OPT_COUNTUP() under the hood. Saying "--no-quiet" should reset it back
> to 0.
> 

Thanks Peff.  That is correct as confirmed by:


C:\Repos\VSO\src>git reset --no-quiet
Unstaged changes after reset:
M       init.ps1

It took 6.74 seconds to enumerate unstaged changes after reset.  You can
use '--quiet' to avoid this.  Set the config setting reset.quiet to true
to make this the default.


> -Peff
> 

[PATCH v2 3/3] reset: warn when refresh_index() takes more than 2 seconds

[Ben Peart]

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(-)

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)
-- 
2.18.0.windows.1

[PATCH v3 0/3] speed up git reset

[Ben Peart]

From: Ben Peart <benpeart@microsoft.com>

Reworded the documentation for git-reset per review feedback.

Base Ref: 
Web-Diff: https://github.com/benpeart/git/commit/1228898917
Checkout: git fetch https://github.com/benpeart/git reset-refresh-index-v3 && git checkout 1228898917


### Interdiff (v2..v3):

diff --git a/Documentation/git-reset.txt b/Documentation/git-reset.txt
index 1d697d9962..51a427a34a 100644
--- a/Documentation/git-reset.txt
+++ b/Documentation/git-reset.txt
@@ -95,7 +95,9 @@ OPTIONS
 
 -q::
 --quiet::
-	Be quiet, only report errors.
+--no-quiet::
+	Be quiet, only report errors. The default behavior respects the
+	`reset.quiet` config option, or `--no-quiet` if that is not set.
 
 
 EXAMPLES


### Patches

Ben Peart (3):
  reset: don't compute unstaged changes after reset when --quiet
  reset: add new reset.quiet config setting
  reset: warn when refresh_index() takes more than 2 seconds

 Documentation/config.txt    |  3 +++
 Documentation/git-reset.txt |  4 +++-
 builtin/reset.c             | 15 ++++++++++++++-
 3 files changed, 20 insertions(+), 2 deletions(-)


base-commit: ca63497355222acefcca02b9cbb540a4768f3286
-- 
2.18.0.windows.1

[PATCH v3 1/3] reset: don't compute unstaged changes after reset when --quiet

[Ben Peart]

From: Ben Peart <benpeart@microsoft.com>

When git reset is run with the --quiet flag, don't bother finding any
additional unstaged changes as they won't be output anyway.  This speeds up
the git reset command by avoiding having to lstat() every file looking for
changes that aren't going to be reported anyway.

The savings can be significant.  In a repo with 200K files "git reset"
drops from 7.16 seconds to 0.32 seconds for a savings of 96%.

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

diff --git a/builtin/reset.c b/builtin/reset.c
index 11cd0dcb8c..04f0d9b4f5 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -375,7 +375,7 @@ 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 (get_git_work_tree())
+			if (!quiet && get_git_work_tree())
 				refresh_index(&the_index, flags, NULL, NULL,
 					      _("Unstaged changes after reset:"));
 		} else {
-- 
2.18.0.windows.1

Re: [PATCH v3 1/3] reset: don't compute unstaged changes after reset when --quiet

[Johannes Schindelin]

Hi Ben,

On Mon, 22 Oct 2018, Ben Peart wrote:

> From: Ben Peart <benpeart@microsoft.com>
> 
> When git reset is run with the --quiet flag, don't bother finding any
> additional unstaged changes as they won't be output anyway.  This speeds up
> the git reset command by avoiding having to lstat() every file looking for
> changes that aren't going to be reported anyway.
> 
> The savings can be significant.  In a repo with 200K files "git reset"
> drops from 7.16 seconds to 0.32 seconds for a savings of 96%.

That's very nice!

Those numbers, just out of curiosity, are they on Windows? Or on Linux?

Ciao,
Dscho

RE: [PATCH v3 1/3] reset: don't compute unstaged changes after reset when --quiet

[Ben Peart]

> -----Original Message-----
> From: Johannes Schindelin <Johannes.Schindelin@gmx.de>
> Sent: Monday, October 22, 2018 4:45 PM
> To: Ben Peart <peartben@gmail.com>
> Cc: git@vger.kernel.org; gitster@pobox.com; Ben Peart
> <Ben.Peart@microsoft.com>; peff@peff.net; sunshine@sunshineco.com
> Subject: Re: [PATCH v3 1/3] reset: don't compute unstaged changes after
> reset when --quiet
> 
> Hi Ben,
> 
> On Mon, 22 Oct 2018, Ben Peart wrote:
> 
> > From: Ben Peart <benpeart@microsoft.com>
> >
> > When git reset is run with the --quiet flag, don't bother finding any
> > additional unstaged changes as they won't be output anyway.  This speeds
> up
> > the git reset command by avoiding having to lstat() every file looking for
> > changes that aren't going to be reported anyway.
> >
> > The savings can be significant.  In a repo with 200K files "git reset"
> > drops from 7.16 seconds to 0.32 seconds for a savings of 96%.
> 
> That's very nice!
> 
> Those numbers, just out of curiosity, are they on Windows? Or on Linux?
> 

It's safe to assume all my numbers are on Windows. :-)

> Ciao,
> Dscho

RE: [PATCH v3 1/3] reset: don't compute unstaged changes after reset when --quiet

[Johannes Schindelin]

Hi Ben,

On Mon, 22 Oct 2018, Ben Peart wrote:

> > From: Johannes Schindelin <Johannes.Schindelin@gmx.de>
> > 
> > On Mon, 22 Oct 2018, Ben Peart wrote:
> > 
> > > When git reset is run with the --quiet flag, don't bother finding
> > > any additional unstaged changes as they won't be output anyway.
> > > This speeds up the git reset command by avoiding having to lstat()
> > > every file looking for changes that aren't going to be reported
> > > anyway.
> > >
> > > The savings can be significant.  In a repo with 200K files "git
> > > reset" drops from 7.16 seconds to 0.32 seconds for a savings of 96%.
> > 
> > That's very nice!
> > 
> > Those numbers, just out of curiosity, are they on Windows? Or on
> > Linux?
> > 
> 
> It's safe to assume all my numbers are on Windows. :-)

Excellent. These speed-ups will really help our users.

Thanks!
Dscho

Re: [PATCH v3 1/3] reset: don't compute unstaged changes after reset when --quiet

[Duy Nguyen]

On Tue, Oct 23, 2018 at 1:01 AM Ben Peart <Ben.Peart@microsoft.com> wrote:
>
> > -----Original Message-----
> > From: Johannes Schindelin <Johannes.Schindelin@gmx.de>
> > Sent: Monday, October 22, 2018 4:45 PM
> > To: Ben Peart <peartben@gmail.com>
> > Cc: git@vger.kernel.org; gitster@pobox.com; Ben Peart
> > <Ben.Peart@microsoft.com>; peff@peff.net; sunshine@sunshineco.com
> > Subject: Re: [PATCH v3 1/3] reset: don't compute unstaged changes after
> > reset when --quiet
> >
> > Hi Ben,
> >
> > On Mon, 22 Oct 2018, Ben Peart wrote:
> >
> > > From: Ben Peart <benpeart@microsoft.com>
> > >
> > > When git reset is run with the --quiet flag, don't bother finding any
> > > additional unstaged changes as they won't be output anyway.  This speeds
> > up
> > > the git reset command by avoiding having to lstat() every file looking for
> > > changes that aren't going to be reported anyway.
> > >
> > > The savings can be significant.  In a repo with 200K files "git reset"
> > > drops from 7.16 seconds to 0.32 seconds for a savings of 96%.
> >
> > That's very nice!
> >
> > Those numbers, just out of curiosity, are they on Windows? Or on Linux?
> >
>
> It's safe to assume all my numbers are on Windows. :-)

It does bug me about this. Next time please mention the platform you
tested on in the commit message. Not all platforms behave the same way
especially when it comes to performance.

>
> > Ciao,
> > Dscho
>
>


-- 
Duy

Re: [PATCH v3 1/3] reset: don't compute unstaged changes after reset when --quiet

[Johannes Schindelin]

Hi Duy,

On Tue, 23 Oct 2018, Duy Nguyen wrote:

> On Tue, Oct 23, 2018 at 1:01 AM Ben Peart <Ben.Peart@microsoft.com> wrote:
> >
> > > -----Original Message-----
> > > From: Johannes Schindelin <Johannes.Schindelin@gmx.de>
> > > Sent: Monday, October 22, 2018 4:45 PM
> > > To: Ben Peart <peartben@gmail.com>
> > > Cc: git@vger.kernel.org; gitster@pobox.com; Ben Peart
> > > <Ben.Peart@microsoft.com>; peff@peff.net; sunshine@sunshineco.com
> > > Subject: Re: [PATCH v3 1/3] reset: don't compute unstaged changes after
> > > reset when --quiet
> > >
> > > Hi Ben,
> > >
> > > On Mon, 22 Oct 2018, Ben Peart wrote:
> > >
> > > > From: Ben Peart <benpeart@microsoft.com>
> > > >
> > > > When git reset is run with the --quiet flag, don't bother finding any
> > > > additional unstaged changes as they won't be output anyway.  This speeds
> > > up
> > > > the git reset command by avoiding having to lstat() every file looking for
> > > > changes that aren't going to be reported anyway.
> > > >
> > > > The savings can be significant.  In a repo with 200K files "git reset"
> > > > drops from 7.16 seconds to 0.32 seconds for a savings of 96%.
> > >
> > > That's very nice!
> > >
> > > Those numbers, just out of curiosity, are they on Windows? Or on Linux?
> > >
> >
> > It's safe to assume all my numbers are on Windows. :-)
> 
> It does bug me about this. Next time please mention the platform you
> tested on in the commit message. Not all platforms behave the same way
> especially when it comes to performance.

And pretty much all different testing scenarios behave differently, too.
And at some stage, we're asking for too many fries.

In other words: we always accepted performance improvements when it could
be demonstrated that they improved a certain not too uncommon scenario,
and I do not think it would make sense to change this stance now. Not
unless you can demonstrate a good reason why we should.

Ciao,
Johannes

[PATCH v3 2/3] reset: add new reset.quiet config setting

[Ben Peart]

From: Ben Peart <benpeart@microsoft.com>

Add a reset.quiet config setting that sets the default value of the --quiet
flag when running the reset command.  This enables users to change the
default behavior to take advantage of the performance advantages of
avoiding the scan for unstaged changes after reset.  Defaults to false.

Signed-off-by: Ben Peart <benpeart@microsoft.com>
---
 Documentation/config.txt    | 3 +++
 Documentation/git-reset.txt | 4 +++-
 builtin/reset.c             | 1 +
 3 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index f6f4c21a54..a2d1b8b116 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2728,6 +2728,9 @@ rerere.enabled::
 	`$GIT_DIR`, e.g. if "rerere" was previously used in the
 	repository.
 
+reset.quiet::
+	When set to true, 'git reset' will default to the '--quiet' option.
+
 include::sendemail-config.txt[]
 
 sequence.editor::
diff --git a/Documentation/git-reset.txt b/Documentation/git-reset.txt
index 1d697d9962..51a427a34a 100644
--- a/Documentation/git-reset.txt
+++ b/Documentation/git-reset.txt
@@ -95,7 +95,9 @@ OPTIONS
 
 -q::
 --quiet::
-	Be quiet, only report errors.
+--no-quiet::
+	Be quiet, only report errors. The default behavior respects the
+	`reset.quiet` config option, or `--no-quiet` if that is not set.
 
 
 EXAMPLES
diff --git a/builtin/reset.c b/builtin/reset.c
index 04f0d9b4f5..3b43aee544 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -306,6 +306,7 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
 	};
 
 	git_config(git_reset_config, NULL);
+	git_config_get_bool("reset.quiet", &quiet);
 
 	argc = parse_options(argc, argv, prefix, options, git_reset_usage,
 						PARSE_OPT_KEEP_DASHDASH);
-- 
2.18.0.windows.1

Re: [PATCH v3 2/3] reset: add new reset.quiet config setting

[Duy Nguyen]

On Mon, Oct 22, 2018 at 3:38 PM Ben Peart <peartben@gmail.com> wrote:
>
> From: Ben Peart <benpeart@microsoft.com>
>
> Add a reset.quiet config setting that sets the default value of the --quiet
> flag when running the reset command.  This enables users to change the
> default behavior to take advantage of the performance advantages of
> avoiding the scan for unstaged changes after reset.  Defaults to false.
>
> Signed-off-by: Ben Peart <benpeart@microsoft.com>
> ---
>  Documentation/config.txt    | 3 +++
>  Documentation/git-reset.txt | 4 +++-
>  builtin/reset.c             | 1 +
>  3 files changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index f6f4c21a54..a2d1b8b116 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -2728,6 +2728,9 @@ rerere.enabled::
>         `$GIT_DIR`, e.g. if "rerere" was previously used in the
>         repository.
>
> +reset.quiet::
> +       When set to true, 'git reset' will default to the '--quiet' option.
> +

With 'nd/config-split' topic moving pretty much all config keys out of
config.txt, you probably want to do the same for this series: add this
in a new file called Documentation/reset-config.txt then include the
file here like the sendemail one below.

>  include::sendemail-config.txt[]
>
>  sequence.editor::
-- 
Duy

Re: [PATCH v3 2/3] reset: add new reset.quiet config setting

[Ben Peart]

On 10/22/2018 10:45 AM, Duy Nguyen wrote:
> On Mon, Oct 22, 2018 at 3:38 PM Ben Peart <peartben@gmail.com> wrote:
>>
>> From: Ben Peart <benpeart@microsoft.com>
>>
>> Add a reset.quiet config setting that sets the default value of the --quiet
>> flag when running the reset command.  This enables users to change the
>> default behavior to take advantage of the performance advantages of
>> avoiding the scan for unstaged changes after reset.  Defaults to false.
>>
>> Signed-off-by: Ben Peart <benpeart@microsoft.com>
>> ---
>>   Documentation/config.txt    | 3 +++
>>   Documentation/git-reset.txt | 4 +++-
>>   builtin/reset.c             | 1 +
>>   3 files changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/config.txt b/Documentation/config.txt
>> index f6f4c21a54..a2d1b8b116 100644
>> --- a/Documentation/config.txt
>> +++ b/Documentation/config.txt
>> @@ -2728,6 +2728,9 @@ rerere.enabled::
>>          `$GIT_DIR`, e.g. if "rerere" was previously used in the
>>          repository.
>>
>> +reset.quiet::
>> +       When set to true, 'git reset' will default to the '--quiet' option.
>> +
> 
> With 'nd/config-split' topic moving pretty much all config keys out of
> config.txt, you probably want to do the same for this series: add this
> in a new file called Documentation/reset-config.txt then include the
> file here like the sendemail one below.
> 

Seems a bit overkill to pull a line of documentation into a separate 
file and replace it with a line of 'import' logic.  Perhaps if/when 
there is more documentation to pull out that would make more sense.

>>   include::sendemail-config.txt[]
>>
>>   sequence.editor::

Re: [PATCH v3 2/3] reset: add new reset.quiet config setting

[Junio C Hamano]

Ben Peart <peartben@gmail.com> writes:

>>> diff --git a/Documentation/config.txt b/Documentation/config.txt
>>> index f6f4c21a54..a2d1b8b116 100644
>>> --- a/Documentation/config.txt
>>> +++ b/Documentation/config.txt
>>> @@ -2728,6 +2728,9 @@ rerere.enabled::
>>>          `$GIT_DIR`, e.g. if "rerere" was previously used in the
>>>          repository.
>>>
>>> +reset.quiet::
>>> +       When set to true, 'git reset' will default to the '--quiet' option.
>>> +
>>
>> With 'nd/config-split' topic moving pretty much all config keys out of
>> config.txt, you probably want to do the same for this series: add this
>> in a new file called Documentation/reset-config.txt then include the
>> file here like the sendemail one below.
>>
>
> Seems a bit overkill to pull a line of documentation into a separate
> file and replace it with a line of 'import' logic.  Perhaps if/when
> there is more documentation to pull out that would make more sense.

This change (ehh, rather, perhaps nd/config-split topic) came at an
unfortunate moment.  Until I actually did one integration cycle to
rebuild 'pu' and merge this patch and the other topic, I had exactly
the same reaction as yours above to Duy's comment.  But seeing the
tree at the tip of 'pu' today, I do think the end result with a
single liner file that has configuration for the "reset" command
that is included in config.txt would make sense, and I also think
you would agree with it if you see the same tree.

How we should get there is a different story.  I think Duy's series
needs at least another update to move the split pieces into its own
subdirectory of Documentation/, and it is not all that urgent, while
this three-patch series (with the advice.* bit added) for "reset" is
pretty much ready to go 'next', so my gut feeling is that it is best
to keep the description here, and to ask Duy to base the updated
version of config-split topic on top.

Re: [PATCH v3 2/3] reset: add new reset.quiet config setting

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

>> Seems a bit overkill to pull a line of documentation into a separate
>> file and replace it with a line of 'import' logic.  Perhaps if/when
>> there is more documentation to pull out that would make more sense.
>
> This change (ehh, rather, perhaps nd/config-split topic) came at an
> unfortunate moment.  Until I actually did one integration cycle to
> rebuild 'pu' and merge this patch and the other topic, I had exactly
> the same reaction as yours above to Duy's comment.  But seeing the
> tree at the tip of 'pu' today, I do think the end result with a
> single liner file that has configuration for the "reset" command
> that is included in config.txt would make sense, and I also think
> you would agree with it if you see the same tree.
>
> How we should get there is a different story.  I think Duy's series
> needs at least another update to move the split pieces into its own
> subdirectory of Documentation/, and it is not all that urgent, while
> this three-patch series (with the advice.* bit added) for "reset" is
> pretty much ready to go 'next', so my gut feeling is that it is best
> to keep the description here, and to ask Duy to base the updated
> version of config-split topic on top.

I'll take the "it is not all that urgent" bit (and only that bit)
back, even though the conclusion would be the same.  It is quite
painful having to keep this topic while a few topics that touch the
huge Documentation/config.txt is in flight.  The monolithic file is
large enough that it does not cause much pain while many topics are
in flight, but the single step of spliting it into million pieces
done by nd/config-split topic is a pain to merge.

Anybody interested may fetch, and try

    $ git checkout 5c2d198e8e
    $ git merge b2358ceaca

and imagine that you'd have to redo that every time somebody adds or
touches up a byte in the description of an individual entry in the
Documentation/config.txt file.  It is not pretty until we finish the
split.

Re: [PATCH v3 2/3] reset: add new reset.quiet config setting

[Duy Nguyen]

On Wed, Oct 24, 2018 at 4:56 AM Junio C Hamano <gitster@pobox.com> wrote:
> How we should get there is a different story.  I think Duy's series
> needs at least another update to move the split pieces into its own
> subdirectory of Documentation/, and it is not all that urgent, while
> this three-patch series (with the advice.* bit added) for "reset" is
> pretty much ready to go 'next', so my gut feeling is that it is best
> to keep the description here, and to ask Duy to base the updated
> version of config-split topic on top.

OK. Just to be sure we're on the same page. Am I waiting for all
config changes to land in 'master', or do I rebase my series on
'next'? I usually base on 'master' but the mention of 'next' here
confuses me a bit.
-- 
Duy

Re: [PATCH v3 2/3] reset: add new reset.quiet config setting

[Junio C Hamano]

Duy Nguyen <pclouds@gmail.com> writes:

> OK. Just to be sure we're on the same page. Am I waiting for all
> config changes to land in 'master', or do I rebase my series on
> 'next'? I usually base on 'master' but the mention of 'next' here
> confuses me a bit.

I was hoping that you can do something like:

  $ git fetch https://github.com/gitster/git \
            --refmap=refs/heads/*:refs/remotes/broken-out/* \
	    bp/reset-quiet master
  $ git checkout broken-out/master^0
  $ git merge broekn-out/bp/reset-quiet
  $ git rebase HEAD np/config-split

once it is clear to everybody that Ben's reset series is ready to be
merged to 'next' (or it actually hits 'next').

Re: [PATCH v3 2/3] reset: add new reset.quiet config setting

[Duy Nguyen]

On Tue, Oct 23, 2018 at 8:47 PM Ben Peart <peartben@gmail.com> wrote:
>
>
>
> On 10/22/2018 10:45 AM, Duy Nguyen wrote:
> > On Mon, Oct 22, 2018 at 3:38 PM Ben Peart <peartben@gmail.com> wrote:
> >>
> >> From: Ben Peart <benpeart@microsoft.com>
> >>
> >> Add a reset.quiet config setting that sets the default value of the --quiet
> >> flag when running the reset command.  This enables users to change the
> >> default behavior to take advantage of the performance advantages of
> >> avoiding the scan for unstaged changes after reset.  Defaults to false.
> >>
> >> Signed-off-by: Ben Peart <benpeart@microsoft.com>
> >> ---
> >>   Documentation/config.txt    | 3 +++
> >>   Documentation/git-reset.txt | 4 +++-
> >>   builtin/reset.c             | 1 +
> >>   3 files changed, 7 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/Documentation/config.txt b/Documentation/config.txt
> >> index f6f4c21a54..a2d1b8b116 100644
> >> --- a/Documentation/config.txt
> >> +++ b/Documentation/config.txt
> >> @@ -2728,6 +2728,9 @@ rerere.enabled::
> >>          `$GIT_DIR`, e.g. if "rerere" was previously used in the
> >>          repository.
> >>
> >> +reset.quiet::
> >> +       When set to true, 'git reset' will default to the '--quiet' option.
> >> +
> >
> > With 'nd/config-split' topic moving pretty much all config keys out of
> > config.txt, you probably want to do the same for this series: add this
> > in a new file called Documentation/reset-config.txt then include the
> > file here like the sendemail one below.
> >
>
> Seems a bit overkill to pull a line of documentation into a separate
> file and replace it with a line of 'import' logic.  Perhaps if/when
> there is more documentation to pull out that would make more sense.

There are a couple benefits of having all config keys stored in the
same way (i.e. in separate files). Searching will be easier, you
_know_ reset.stuff will be in reset-config.txt. If you mix both ways,
you may need to look in config.txt as well as searching
reset-config.txt. This single config key also stands out when you look
at the end result of nd/config-split. The config split also opens up
an opportunity to include command-specific config in individual
command man page if done consistently.
-- 
Duy

Re: [PATCH v3 2/3] reset: add new reset.quiet config setting

[Ramsay Jones]

On 22/10/2018 14:18, Ben Peart wrote:
> From: Ben Peart <benpeart@microsoft.com>
> 
> Add a reset.quiet config setting that sets the default value of the --quiet
> flag when running the reset command.  This enables users to change the
> default behavior to take advantage of the performance advantages of
> avoiding the scan for unstaged changes after reset.  Defaults to false.
> 
> Signed-off-by: Ben Peart <benpeart@microsoft.com>
> ---
>  Documentation/config.txt    | 3 +++
>  Documentation/git-reset.txt | 4 +++-
>  builtin/reset.c             | 1 +
>  3 files changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index f6f4c21a54..a2d1b8b116 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -2728,6 +2728,9 @@ rerere.enabled::
>  	`$GIT_DIR`, e.g. if "rerere" was previously used in the
>  	repository.
>  
> +reset.quiet::
> +	When set to true, 'git reset' will default to the '--quiet' option.
> +
>  include::sendemail-config.txt[]
>  
>  sequence.editor::
> diff --git a/Documentation/git-reset.txt b/Documentation/git-reset.txt
> index 1d697d9962..51a427a34a 100644
> --- a/Documentation/git-reset.txt
> +++ b/Documentation/git-reset.txt
> @@ -95,7 +95,9 @@ OPTIONS
>  
>  -q::
>  --quiet::
> -	Be quiet, only report errors.
> +--no-quiet::
> +	Be quiet, only report errors. The default behavior respects the
> +	`reset.quiet` config option, or `--no-quiet` if that is not set.

Sorry, I can't quite parse this; -q,--quiet and --no-quiet on the
command line (should) trump whatever rest.quiet is set to in the
configuration. Is that not the case?

ATB,
Ramsay Jones

>  
>  
>  EXAMPLES
> diff --git a/builtin/reset.c b/builtin/reset.c
> index 04f0d9b4f5..3b43aee544 100644
> --- a/builtin/reset.c
> +++ b/builtin/reset.c
> @@ -306,6 +306,7 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
>  	};
>  
>  	git_config(git_reset_config, NULL);
> +	git_config_get_bool("reset.quiet", &quiet);
>  
>  	argc = parse_options(argc, argv, prefix, options, git_reset_usage,
>  						PARSE_OPT_KEEP_DASHDASH);
> 

Re: [PATCH v3 2/3] reset: add new reset.quiet config setting

[Jeff King]

On Mon, Oct 22, 2018 at 08:13:32PM +0100, Ramsay Jones wrote:

> >  -q::
> >  --quiet::
> > -	Be quiet, only report errors.
> > +--no-quiet::
> > +	Be quiet, only report errors. The default behavior respects the
> > +	`reset.quiet` config option, or `--no-quiet` if that is not set.
> 
> Sorry, I can't quite parse this; -q,--quiet and --no-quiet on the
> command line (should) trump whatever rest.quiet is set to in the
> configuration. Is that not the case?

That is the case, and what was meant by "the default behavior" (i.e.,
the behavior when none of these is used). Maybe there's a more clear way
of saying that.

-Peff

Re: [PATCH v3 2/3] reset: add new reset.quiet config setting

[Ben Peart]

On 10/22/2018 4:06 PM, Jeff King wrote:
> On Mon, Oct 22, 2018 at 08:13:32PM +0100, Ramsay Jones wrote:
> 
>>>   -q::
>>>   --quiet::
>>> -	Be quiet, only report errors.
>>> +--no-quiet::
>>> +	Be quiet, only report errors. The default behavior respects the
>>> +	`reset.quiet` config option, or `--no-quiet` if that is not set.
>>
>> Sorry, I can't quite parse this; -q,--quiet and --no-quiet on the
>> command line (should) trump whatever rest.quiet is set to in the
>> configuration. Is that not the case?
> 
> That is the case, and what was meant by "the default behavior" (i.e.,
> the behavior when none of these is used). Maybe there's a more clear way
> of saying that.
> 
> -Peff
> 

Is this more clear?

-q::
--quiet::
--no-quiet::
	Be quiet, only report errors. The default behavior is set by the
	`reset.quiet` config option. `--quiet` and `--no-quiet` will
	overwrite the default behavior.

Re: [PATCH v3 2/3] reset: add new reset.quiet config setting

[Jeff King]

On Tue, Oct 23, 2018 at 01:31:58PM -0400, Ben Peart wrote:

> On 10/22/2018 4:06 PM, Jeff King wrote:
> > On Mon, Oct 22, 2018 at 08:13:32PM +0100, Ramsay Jones wrote:
> > 
> > > >   -q::
> > > >   --quiet::
> > > > -	Be quiet, only report errors.
> > > > +--no-quiet::
> > > > +	Be quiet, only report errors. The default behavior respects the
> > > > +	`reset.quiet` config option, or `--no-quiet` if that is not set.
> > > 
> > > Sorry, I can't quite parse this; -q,--quiet and --no-quiet on the
> > > command line (should) trump whatever rest.quiet is set to in the
> > > configuration. Is that not the case?
> > 
> > That is the case, and what was meant by "the default behavior" (i.e.,
> > the behavior when none of these is used). Maybe there's a more clear way
> > of saying that.
> > 
> 
> Is this more clear?
> 
> -q::
> --quiet::
> --no-quiet::
> 	Be quiet, only report errors. The default behavior is set by the
> 	`reset.quiet` config option. `--quiet` and `--no-quiet` will
> 	overwrite the default behavior.

That looks OK to me (but then so did the earlier one ;) ).

I'd probably s/overwrite/override/.

-Peff

[PATCH v3 3/3] reset: warn when refresh_index() takes more than 2 seconds

[Ben Peart]

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(-)

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)
-- 
2.18.0.windows.1

Re: [PATCH v3 3/3] reset: warn when refresh_index() takes more than 2 seconds

[Junio C Hamano]

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.

Re: [PATCH v3 3/3] reset: warn when refresh_index() takes more than 2 seconds

[Ben Peart]

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 v4 0/3] speed up git reset

[Ben Peart]

From: Ben Peart <benpeart@microsoft.com>

Updated the wording in the documentation and commit messages to (hopefully)
make it clearer. Added the warning about 'reset --quiet' to the advice
system so that it can be turned off.

Base Ref: 
Web-Diff: https://github.com/benpeart/git/commit/8a2fef45d4
Checkout: git fetch https://github.com/benpeart/git reset-refresh-index-v4 && git checkout 8a2fef45d4


### Patches

Ben Peart (3):
  reset: don't compute unstaged changes after reset when --quiet
  reset: add new reset.quiet config setting
  reset: warn when refresh_index() takes more than 2 seconds

 Documentation/config.txt    |  7 +++++++
 Documentation/git-reset.txt |  5 ++++-
 advice.c                    |  2 ++
 advice.h                    |  1 +
 builtin/reset.c             | 15 ++++++++++++++-
 5 files changed, 28 insertions(+), 2 deletions(-)


base-commit: ca63497355222acefcca02b9cbb540a4768f3286
-- 
2.18.0.windows.1

[PATCH v4 1/3] reset: don't compute unstaged changes after reset when --quiet

[Ben Peart]

From: Ben Peart <benpeart@microsoft.com>

When git reset is run with the --quiet flag, don't bother finding any
additional unstaged changes as they won't be output anyway.  This speeds up
the git reset command by avoiding having to lstat() every file looking for
changes that aren't going to be reported anyway.

The savings can be significant.  In a repo on Windows with 200K files
"git reset" drops from 7.16 seconds to 0.32 seconds for a savings of 96%.

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

diff --git a/builtin/reset.c b/builtin/reset.c
index 11cd0dcb8c..04f0d9b4f5 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -375,7 +375,7 @@ 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 (get_git_work_tree())
+			if (!quiet && get_git_work_tree())
 				refresh_index(&the_index, flags, NULL, NULL,
 					      _("Unstaged changes after reset:"));
 		} else {
-- 
2.18.0.windows.1

[PATCH v4 2/3] reset: add new reset.quiet config setting

[Ben Peart]

From: Ben Peart <benpeart@microsoft.com>

Add a reset.quiet config setting that sets the default value of the --quiet
flag when running the reset command.  This enables users to change the
default behavior to take advantage of the performance advantages of
avoiding the scan for unstaged changes after reset.  Defaults to false.

Signed-off-by: Ben Peart <benpeart@microsoft.com>
---
 Documentation/config.txt    | 3 +++
 Documentation/git-reset.txt | 5 ++++-
 builtin/reset.c             | 1 +
 3 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index f6f4c21a54..a2d1b8b116 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2728,6 +2728,9 @@ rerere.enabled::
 	`$GIT_DIR`, e.g. if "rerere" was previously used in the
 	repository.
 
+reset.quiet::
+	When set to true, 'git reset' will default to the '--quiet' option.
+
 include::sendemail-config.txt[]
 
 sequence.editor::
diff --git a/Documentation/git-reset.txt b/Documentation/git-reset.txt
index 1d697d9962..2dac95c71a 100644
--- a/Documentation/git-reset.txt
+++ b/Documentation/git-reset.txt
@@ -95,7 +95,10 @@ OPTIONS
 
 -q::
 --quiet::
-	Be quiet, only report errors.
+--no-quiet::
+	Be quiet, only report errors. The default behavior is set by the
+	`reset.quiet` config option. `--quiet` and `--no-quiet` will
+	override the default behavior.
 
 
 EXAMPLES
diff --git a/builtin/reset.c b/builtin/reset.c
index 04f0d9b4f5..3b43aee544 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -306,6 +306,7 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
 	};
 
 	git_config(git_reset_config, NULL);
+	git_config_get_bool("reset.quiet", &quiet);
 
 	argc = parse_options(argc, argv, prefix, options, git_reset_usage,
 						PARSE_OPT_KEEP_DASHDASH);
-- 
2.18.0.windows.1

Re: [PATCH v4 2/3] reset: add new reset.quiet config setting

[Ramsay Jones]

On 23/10/2018 20:04, Ben Peart wrote:
> From: Ben Peart <benpeart@microsoft.com>

Sorry for the late reply, ... I've been away from email - I am
still trying to catch up.

> 
> Add a reset.quiet config setting that sets the default value of the --quiet
> flag when running the reset command.  This enables users to change the
> default behavior to take advantage of the performance advantages of
> avoiding the scan for unstaged changes after reset.  Defaults to false.
> 
> Signed-off-by: Ben Peart <benpeart@microsoft.com>
> ---
>  Documentation/config.txt    | 3 +++
>  Documentation/git-reset.txt | 5 ++++-
>  builtin/reset.c             | 1 +
>  3 files changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index f6f4c21a54..a2d1b8b116 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -2728,6 +2728,9 @@ rerere.enabled::
>  	`$GIT_DIR`, e.g. if "rerere" was previously used in the
>  	repository.
>  
> +reset.quiet::
> +	When set to true, 'git reset' will default to the '--quiet' option.

Mention that this 'Defaults to false'?

> +
>  include::sendemail-config.txt[]
>  
>  sequence.editor::
> diff --git a/Documentation/git-reset.txt b/Documentation/git-reset.txt
> index 1d697d9962..2dac95c71a 100644
> --- a/Documentation/git-reset.txt
> +++ b/Documentation/git-reset.txt
> @@ -95,7 +95,10 @@ OPTIONS
>  
>  -q::
>  --quiet::
> -	Be quiet, only report errors.
> +--no-quiet::
> +	Be quiet, only report errors. The default behavior is set by the
> +	`reset.quiet` config option. `--quiet` and `--no-quiet` will
> +	override the default behavior.

Better than last time, but how about something like:

 -q::
 --quiet::
 --no-quiet::
      Be quiet, only report errors. The default behaviour of the
      command, which is to not be quiet, can be specified by the
      `reset.quiet` configuration variable. The `--quiet` and
      `--no-quiet` options can be used to override any configured
      default.

Hmm, I am not sure that is any better! :-D

Also, note that the --no-option is often described separately to
the --option (in a separate paragraph). I don't know if that would
help here.

[The default behaviour is _not_ set by the configuration, if no
configuration is specified. :-P ]

Not sure if that helps!

ATB,
Ramsay Jones

>  
>  
>  EXAMPLES
> diff --git a/builtin/reset.c b/builtin/reset.c
> index 04f0d9b4f5..3b43aee544 100644
> --- a/builtin/reset.c
> +++ b/builtin/reset.c
> @@ -306,6 +306,7 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
>  	};
>  
>  	git_config(git_reset_config, NULL);
> +	git_config_get_bool("reset.quiet", &quiet);
>  
>  	argc = parse_options(argc, argv, prefix, options, git_reset_usage,
>  						PARSE_OPT_KEEP_DASHDASH);
> 

Re: [PATCH v4 2/3] reset: add new reset.quiet config setting

[Junio C Hamano]

Ramsay Jones <ramsay@ramsayjones.plus.com> writes:

>> diff --git a/Documentation/config.txt b/Documentation/config.txt
>> index f6f4c21a54..a2d1b8b116 100644
>> --- a/Documentation/config.txt
>> +++ b/Documentation/config.txt
>> @@ -2728,6 +2728,9 @@ rerere.enabled::
>>  	`$GIT_DIR`, e.g. if "rerere" was previously used in the
>>  	repository.
>>  
>> +reset.quiet::
>> +	When set to true, 'git reset' will default to the '--quiet' option.
>
> Mention that this 'Defaults to false'?

Perhaps.

>>  -q::
>>  --quiet::
>> -	Be quiet, only report errors.
>> +--no-quiet::
>> +	Be quiet, only report errors. The default behavior is set by the
>> +	`reset.quiet` config option. `--quiet` and `--no-quiet` will
>> +	override the default behavior.
>
> Better than last time, but how about something like:
>
>  -q::
>  --quiet::
>  --no-quiet::
>       Be quiet, only report errors. The default behaviour of the
>       command, which is to not be quiet, can be specified by the
>       `reset.quiet` configuration variable. The `--quiet` and
>       `--no-quiet` options can be used to override any configured
>       default.
>
> Hmm, I am not sure that is any better! :-D

To be honest, I find the second sentence in your rewrite even more
confusing.  It reads as if `reset.quiet` configuration variable 
can be used to restore the "show what is yet to be added"
behaviour, due to the parenthetical mention of the default behaviour
without any configuration.

	The command reports what is yet to be added to the index
	after `reset` by default.  It can be made to only report
	errors with the `--quiet` option, or setting `reset.quiet`
	configuration variable to `true` (the latter can be
	overriden with `--no-quiet`).

That may not be much better, though X-<.

Re: [PATCH v4 2/3] reset: add new reset.quiet config setting

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> To be honest, I find the second sentence in your rewrite even more
> confusing.  It reads as if `reset.quiet` configuration variable 
> can be used to restore the "show what is yet to be added"
> behaviour, due to the parenthetical mention of the default behaviour
> without any configuration.
>
> 	The command reports what is yet to be added to the index
> 	after `reset` by default.  It can be made to only report
> 	errors with the `--quiet` option, or setting `reset.quiet`
> 	configuration variable to `true` (the latter can be
> 	overriden with `--no-quiet`).
>
> That may not be much better, though X-<.

In any case, the comments are getting closer to the bikeshedding
territory, that can be easily addressed incrementally.  I am getting
the impression that everbody agrees that the change is desirable,
sufficiently documented and properly implemented.  

Shall we mark it for "Will merge to 'next'" in the what's cooking
report and leave further refinements to incremental updates as
needed?

Re: [PATCH v4 2/3] reset: add new reset.quiet config setting

[Ben Peart]

On 10/25/2018 5:26 AM, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
> 
>> To be honest, I find the second sentence in your rewrite even more
>> confusing.  It reads as if `reset.quiet` configuration variable
>> can be used to restore the "show what is yet to be added"
>> behaviour, due to the parenthetical mention of the default behaviour
>> without any configuration.
>>
>> 	The command reports what is yet to be added to the index
>> 	after `reset` by default.  It can be made to only report
>> 	errors with the `--quiet` option, or setting `reset.quiet`
>> 	configuration variable to `true` (the latter can be
>> 	overriden with `--no-quiet`).
>>
>> That may not be much better, though X-<.
> 
> In any case, the comments are getting closer to the bikeshedding
> territory, that can be easily addressed incrementally.  I am getting
> the impression that everbody agrees that the change is desirable,
> sufficiently documented and properly implemented.
> 
> Shall we mark it for "Will merge to 'next'" in the what's cooking
> report and leave further refinements to incremental updates as
> needed?
> 

While not great, I think it is good enough.  I don't think either of the 
last couple of rewrite attempts were clearly better than what is in the 
latest patch. I'd agree we should merge to 'next' and if someone comes 
up with something great, we can update it then.

Re: [PATCH v4 2/3] reset: add new reset.quiet config setting

[Ramsay Jones]

On 25/10/2018 10:26, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
> 
>> To be honest, I find the second sentence in your rewrite even more
>> confusing.  It reads as if `reset.quiet` configuration variable 
>> can be used to restore the "show what is yet to be added"
>> behaviour, due to the parenthetical mention of the default behaviour
>> without any configuration.
>>
>> 	The command reports what is yet to be added to the index
>> 	after `reset` by default.  It can be made to only report
>> 	errors with the `--quiet` option, or setting `reset.quiet`
>> 	configuration variable to `true` (the latter can be
>> 	overriden with `--no-quiet`).
>>
>> That may not be much better, though X-<.
> 
> In any case, the comments are getting closer to the bikeshedding
> territory, that can be easily addressed incrementally.  I am getting
> the impression that everbody agrees that the change is desirable,
> sufficiently documented and properly implemented.  
> 
> Shall we mark it for "Will merge to 'next'" in the what's cooking
> report and leave further refinements to incremental updates as
> needed?

Yeah, the first version gave me a 'huh?' moment (hence the
comment), the last version was better and, as you can see,
I am no great shakes at wordsmith-ing documentation! ;-)

Thanks!

ATB,
Ramsay Jones

[PATCH v4 3/3] reset: warn when refresh_index() takes more than 2 seconds

[Ben Peart]

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>
---
 Documentation/config.txt |  4 ++++
 advice.c                 |  2 ++
 advice.h                 |  1 +
 builtin/reset.c          | 14 +++++++++++++-
 4 files changed, 20 insertions(+), 1 deletion(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index a2d1b8b116..415db31def 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -333,6 +333,10 @@ advice.*::
 	commitBeforeMerge::
 		Advice shown when linkgit:git-merge[1] refuses to
 		merge to avoid overwriting local changes.
+	resetQuiet::
+		Advice to consider using the `--quiet` option to linkgit:git-reset[1]
+		when the command takes more than 2 seconds to enumerate unstaged
+		changes after reset.
 	resolveConflict::
 		Advice shown by various commands when conflicts
 		prevent the operation from being performed.
diff --git a/advice.c b/advice.c
index 3561cd64e9..5f35656409 100644
--- a/advice.c
+++ b/advice.c
@@ -12,6 +12,7 @@ int advice_push_needs_force = 1;
 int advice_status_hints = 1;
 int advice_status_u_option = 1;
 int advice_commit_before_merge = 1;
+int advice_reset_quiet_warning = 1;
 int advice_resolve_conflict = 1;
 int advice_implicit_identity = 1;
 int advice_detached_head = 1;
@@ -65,6 +66,7 @@ static struct {
 	{ "statusHints", &advice_status_hints },
 	{ "statusUoption", &advice_status_u_option },
 	{ "commitBeforeMerge", &advice_commit_before_merge },
+	{ "resetQuiet", &advice_reset_quiet_warning },
 	{ "resolveConflict", &advice_resolve_conflict },
 	{ "implicitIdentity", &advice_implicit_identity },
 	{ "detachedHead", &advice_detached_head },
diff --git a/advice.h b/advice.h
index ab24df0fd0..696bf0e7d2 100644
--- a/advice.h
+++ b/advice.h
@@ -12,6 +12,7 @@ extern int advice_push_needs_force;
 extern int advice_status_hints;
 extern int advice_status_u_option;
 extern int advice_commit_before_merge;
+extern int advice_reset_quiet_warning;
 extern int advice_resolve_conflict;
 extern int advice_implicit_identity;
 extern int advice_detached_head;
diff --git a/builtin/reset.c b/builtin/reset.c
index 3b43aee544..b31a0eae8a 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 (advice_reset_quiet_warning && 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.\n"), t_delta_in_ms / 1000.0);
+				}
+			}
 		} else {
 			int err = reset_index(&oid, reset_type, quiet);
 			if (reset_type == KEEP && !err)
-- 
2.18.0.windows.1