[v3,2/3] reset: add new reset.quiet config setting
diff mbox series

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

Commit Message

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

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

Comments

Duy Nguyen Oct. 22, 2018, 2:45 p.m. UTC | #1
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::
Ramsay Jones Oct. 22, 2018, 7:13 p.m. UTC | #2
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);
>
Jeff King Oct. 22, 2018, 8:06 p.m. UTC | #3
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
Ben Peart Oct. 23, 2018, 5:31 p.m. UTC | #4
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.
Jeff King Oct. 23, 2018, 5:35 p.m. UTC | #5
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
Ben Peart Oct. 23, 2018, 6:47 p.m. UTC | #6
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::
Junio C Hamano Oct. 24, 2018, 2:56 a.m. UTC | #7
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.
Junio C Hamano Oct. 24, 2018, 7:21 a.m. UTC | #8
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.
Duy Nguyen Oct. 24, 2018, 2:49 p.m. UTC | #9
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 Nguyen Oct. 24, 2018, 2:54 p.m. UTC | #10
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.
Junio C Hamano Oct. 25, 2018, 1:12 a.m. UTC | #11
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').

Patch
diff mbox series

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