patch, 2 partsspeed up git reset

> 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.

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

> 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

> 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.

> 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.
> 

> 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.

> 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?
> 

> 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.

> 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.

> 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.

> 
> 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.

> 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.

>> 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.

> [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?).
> [...]
> 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).

> 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.

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

> 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.

> 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").

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

> > > 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.

> 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.

> 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.
> 

> 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%.

> -----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?
> 

> > 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. :-)

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

> 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.

>
> 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.
> +

> 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.
> 

>>> 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.

>> 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.

> 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.

>
>
>
> 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.

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

> >  -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?

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

> 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.

> 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.

> 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.
> 

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

>> 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'?

>>  -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-<.

> 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?
> 

> 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?