threads / patch / 40196

patchstash: Add stash.showFlag config variable

Subject: [PATCH] stash: Add stash.showFlag config variable

## tl;dr

9 messages between Aug 27, 2015 and Aug 29, 2015. Diffs are folded; open one to read it.

replies: 8people: 4as markdown or json

Namhyung Kim· Aug 27, 2015, 13:52 UTC · lore

Some users might want to see diff (patch) output always rather than diffstat when [s]he runs 'git stash show'. Although this can be done with adding -p option, it'd be better to provide a config option to control this behavior IMHO.

Signed-off-by: Namhyung Kim <namhyung@gmail.com>
---
 Documentation/config.txt    | 5 +++++
 Documentation/git-stash.txt | 1 +
 git-stash.sh                | 8 +++++++-
 3 files changed, 13 insertions(+), 1 deletion(-)
Show changes to 3 files +13 −1

Documentation/config.txt, Documentation/git-stash.txt, git-stash.sh

diff --git a/Documentation/config.txt b/Documentation/config.txt
index f5d15ff..bbadae6 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2567,6 +2567,11 @@ status.submoduleSummary::
 	submodule summary' command, which shows a similar output but does
 	not honor these settings.
 
+stash.showFlag::
+	The default option to pass to `git stash show` when no option is
+	given. The default is '--stat'.  See description of 'show' command
+	in linkgit:git-stash[1].
+
 submodule.<name>.path::
 submodule.<name>.url::
 	The path within this project and URL for a submodule. These
diff --git a/Documentation/git-stash.txt b/Documentation/git-stash.txt
index 375213f..e00f67e 100644
--- a/Documentation/git-stash.txt
+++ b/Documentation/git-stash.txt
@@ -95,6 +95,7 @@ show [<stash>]::
 	shows the latest one. By default, the command shows the diffstat, but
 	it will accept any format known to 'git diff' (e.g., `git stash show
 	-p stash@{1}` to view the second most recent stash in patch form).
+	You can use stash.showflag config variable to change this behavior.
 
 pop [--index] [-q|--quiet] [<stash>]::
 
diff --git a/git-stash.sh b/git-stash.sh
index 1d5ba7a..8432435 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -33,6 +33,12 @@ else
        reset_color=
 fi
 
+if git config --get stash.showflag > /dev/null 2> /dev/null; then
+	show_flag=$(git config --get stash.showflag)
+else
+	show_flag=--stat
+fi
+
 no_changes () {
 	git diff-index --quiet --cached HEAD --ignore-submodules -- &&
 	git diff-files --quiet --ignore-submodules &&
@@ -305,7 +311,7 @@ show_stash () {
 	ALLOW_UNKNOWN_FLAGS=t
 	assert_stash_like "$@"
 
-	git diff ${FLAGS:---stat} $b_commit $w_commit
+	git diff ${FLAGS:-${show_flag}} $b_commit $w_commit
 }
 
 show_help () {
-- 
2.5.0
SZEDER Gábor· Aug 27, 2015, 15:20 UTC · re: Namhyung Kim · lore

Re: [PATCH] stash: Add stash.showFlag config variable

Hi,

I haven't made up my mind about this feature yet, but have a few comments about its implementation.

Show 14 quoted lines
> diff --git a/git-stash.sh b/git-stash.sh
> index 1d5ba7a..8432435 100755
> --- a/git-stash.sh
> +++ b/git-stash.sh
> @@ -33,6 +33,12 @@ else
>         reset_color=
>  fi
>  
> +if git config --get stash.showflag > /dev/null 2> /dev/null; then
> +	show_flag=$(git config --get stash.showflag)
> +else
> +	show_flag=--stat
> +fi
> +

Forking and executing processes are costly on some important platforms we care about, so we should strive to avoid them whenever possible.

 - This hunk runs the the exact same 'git config' command twice.  Run it
   only once, perhaps something like this:
     show_flag=$(git config --get stash.showflag || echo --stat)
   (I hope there are no obscure crazy 'echo' implemtations out there
   that might barf on the unknown option '--stat'...)
 - It runs 'git config' in the main code path, i.e. even for subcommands
   other than 'show'.  Run it only for 'git stash show'.
 - This config setting is not relevant if there were options given on the
   command line.  Run it only if there are no options given, i.e. when
   $FLAGS is empty.
Show 14 quoted lines
>  no_changes () {
>  	git diff-index --quiet --cached HEAD --ignore-submodules -- &&
>  	git diff-files --quiet --ignore-submodules &&
> @@ -305,7 +311,7 @@ show_stash () {
>  	ALLOW_UNKNOWN_FLAGS=t
>  	assert_stash_like "$@"
>  
> -	git diff ${FLAGS:---stat} $b_commit $w_commit
> +	git diff ${FLAGS:-${show_flag}} $b_commit $w_commit
>  }
>  
>  show_help () {
> -- 
> 2.5.0
Namhyung Kim· Aug 27, 2015, 15:36 UTC · re: SZEDER Gábor · lore

Re: [PATCH] stash: Add stash.showFlag config variable

Hi,
On Fri, Aug 28, 2015 at 12:20 AM, SZEDER Gábor <szeder@ira.uka.de> wrote:
> Hi,
>
> I haven't made up my mind about this feature yet, but have a few
> comments about its implementation.
Thanks for taking your time!
Show 26 quoted lines
>
>> diff --git a/git-stash.sh b/git-stash.sh
>> index 1d5ba7a..8432435 100755
>> --- a/git-stash.sh
>> +++ b/git-stash.sh
>> @@ -33,6 +33,12 @@ else
>>         reset_color=
>>  fi
>>
>> +if git config --get stash.showflag > /dev/null 2> /dev/null; then
>> +     show_flag=$(git config --get stash.showflag)
>> +else
>> +     show_flag=--stat
>> +fi
>> +
>
> Forking and executing processes are costly on some important platforms
> we care about, so we should strive to avoid them whenever possible.
>
>  - This hunk runs the the exact same 'git config' command twice.  Run it
>    only once, perhaps something like this:
>
>      show_flag=$(git config --get stash.showflag || echo --stat)
>
>    (I hope there are no obscure crazy 'echo' implemtations out there
>    that might barf on the unknown option '--stat'...)
What about `echo "--stat"` then?
Show 7 quoted lines
>
>  - It runs 'git config' in the main code path, i.e. even for subcommands
>    other than 'show'.  Run it only for 'git stash show'.
>
>  - This config setting is not relevant if there were options given on the
>    command line.  Run it only if there are no options given, i.e. when
>    $FLAGS is empty.
Fair enough.  I'll resend v2.

Thanks, Namhyung

Show 16 quoted lines
>
>
>>  no_changes () {
>>       git diff-index --quiet --cached HEAD --ignore-submodules -- &&
>>       git diff-files --quiet --ignore-submodules &&
>> @@ -305,7 +311,7 @@ show_stash () {
>>       ALLOW_UNKNOWN_FLAGS=t
>>       assert_stash_like "$@"
>>
>> -     git diff ${FLAGS:---stat} $b_commit $w_commit
>> +     git diff ${FLAGS:-${show_flag}} $b_commit $w_commit
>>  }
>>
>>  show_help () {
>> --
>> 2.5.0
Eric Sunshine· Aug 28, 2015, 00:16 UTC · re: Namhyung Kim · lore

Re: [PATCH] stash: Add stash.showFlag config variable

On Thu, Aug 27, 2015 at 11:36 AM, Namhyung Kim <namhyung@gmail.com> wrote:
Show 10 quoted lines
> On Fri, Aug 28, 2015 at 12:20 AM, SZEDER Gábor <szeder@ira.uka.de> wrote:
>>  - This hunk runs the the exact same 'git config' command twice.  Run it
>>    only once, perhaps something like this:
>>
>>      show_flag=$(git config --get stash.showflag || echo --stat)
>>
>>    (I hope there are no obscure crazy 'echo' implemtations out there
>>    that might barf on the unknown option '--stat'...)
>
> What about `echo "--stat"` then?

Adding quotes around --stat won't buy you anything since the shell will have removed the quotes by the time the argument is passed to echo, so an "obscure crazy" 'echo' will still see --stat as an option.

POSIX states that printf should take no options, so:
    printf --stat

should be safe, but some implementations do process options (and will complain about the unknown --stat option), therefore, best would be:

    printf '%s' --stat
Namhyung Kim· Aug 28, 2015, 01:47 UTC · re: Eric Sunshine · lore

Re: [PATCH] stash: Add stash.showFlag config variable

Hi,
On Thu, Aug 27, 2015 at 08:16:35PM -0400, Eric Sunshine wrote:
Show 24 quoted lines
> On Thu, Aug 27, 2015 at 11:36 AM, Namhyung Kim <namhyung@gmail.com> wrote:
> > On Fri, Aug 28, 2015 at 12:20 AM, SZEDER Gábor <szeder@ira.uka.de> wrote:
> >>  - This hunk runs the the exact same 'git config' command twice.  Run it
> >>    only once, perhaps something like this:
> >>
> >>      show_flag=$(git config --get stash.showflag || echo --stat)
> >>
> >>    (I hope there are no obscure crazy 'echo' implemtations out there
> >>    that might barf on the unknown option '--stat'...)
> >
> > What about `echo "--stat"` then?
> 
> Adding quotes around --stat won't buy you anything since the shell
> will have removed the quotes by the time the argument is passed to
> echo, so an "obscure crazy" 'echo' will still see --stat as an option.
> 
> POSIX states that printf should take no options, so:
> 
>     printf --stat
> 
> should be safe, but some implementations do process options (and will
> complain about the unknown --stat option), therefore, best would be:
> 
>     printf '%s' --stat
That's good to know.  I'll change it that way.

Thanks for your review! Namhyung

Junio C Hamano· Aug 28, 2015, 01:08 UTC · re: Namhyung Kim · lore

Re: [PATCH] stash: Add stash.showFlag config variable

Namhyung Kim <namhyung@gmail.com> writes:
> +stash.showFlag::
> +	The default option to pass to `git stash show` when no option is
> +	given. The default is '--stat'.  See description of 'show' command
> +	in linkgit:git-stash[1].

Doesn't the same discussion in $gmane/275752 apply here? By designing the configuration variable in a sloppy way, this change will force us to spawn "git diff" via the shell forever, even after somebody ports "git stash" to C.

Which is not great.
Perhaps a pair of new booleans
 - stash.showStat (defaults to true but you can turn it off)
 - stash.showPatch (defaults to false but you can turn it on)
or something along that line might be sufficient and more palatable.
I dunno.
[Footnote]

*1* Besides, showFlag is a strange configuration variable name. I thought that by setting it to true, you are making "git stash" command to somehow show some kind of a flag when it does its operation ;-).

Namhyung Kim· Aug 28, 2015, 01:54 UTC · re: Junio C Hamano · lore

Re: [PATCH] stash: Add stash.showFlag config variable

Hi,
On Thu, Aug 27, 2015 at 06:08:39PM -0700, Junio C Hamano wrote:
Show 13 quoted lines
> Namhyung Kim <namhyung@gmail.com> writes:
> 
> > +stash.showFlag::
> > +	The default option to pass to `git stash show` when no option is
> > +	given. The default is '--stat'.  See description of 'show' command
> > +	in linkgit:git-stash[1].
> 
> Doesn't the same discussion in $gmane/275752 apply here?  By
> designing the configuration variable in a sloppy way, this change
> will force us to spawn "git diff" via the shell forever, even after
> somebody ports "git stash" to C.
> 
> Which is not great.
I see.
Show 7 quoted lines
> 
> Perhaps a pair of new booleans
> 
>  - stash.showStat (defaults to true but you can turn it off)
>  - stash.showPatch (defaults to false but you can turn it on)
> 
> or something along that line might be sufficient and more palatable.

Hmm.. I agree with you, but I don't know what we should do if both of the options were off. Just run 'git diff' with no option is ok to you?

> 
> I dunno.
:)
Show 8 quoted lines
> 
> 
> [Footnote]
> 
> *1* Besides, showFlag is a strange configuration variable name.  I
> thought that by setting it to true, you are making "git stash"
> command to somehow show some kind of a flag when it does its
> operation ;-).
I admit that it's a bad name.  My naming sense is always horrible.. ;-p

Thanks for the review! Namhyung

Junio C Hamano· Aug 28, 2015, 18:10 UTC · re: Namhyung Kim · lore

Re: [PATCH] stash: Add stash.showFlag config variable

Namhyung Kim <namhyung@gmail.com> writes:
Show 9 quoted lines
>> Perhaps a pair of new booleans
>> 
>>  - stash.showStat (defaults to true but you can turn it off)
>>  - stash.showPatch (defaults to false but you can turn it on)
>> 
>> or something along that line might be sufficient and more palatable.
>
> Hmm.. I agree with you, but I don't know what we should do if both of
> the options were off.  Just run 'git diff' with no option is ok to you?

If the user does not want stat or patch, then not running anything would be more appropriate, don't you think?

Namhyung Kim· Aug 29, 2015, 15:19 UTC · re: Junio C Hamano · lore

Re: [PATCH] stash: Add stash.showFlag config variable

Hi,
On Sat, Aug 29, 2015 at 3:10 AM, Junio C Hamano <gitster@pobox.com> wrote:
Show 14 quoted lines
> Namhyung Kim <namhyung@gmail.com> writes:
>
>>> Perhaps a pair of new booleans
>>>
>>>  - stash.showStat (defaults to true but you can turn it off)
>>>  - stash.showPatch (defaults to false but you can turn it on)
>>>
>>> or something along that line might be sufficient and more palatable.
>>
>> Hmm.. I agree with you, but I don't know what we should do if both of
>> the options were off.  Just run 'git diff' with no option is ok to you?
>
> If the user does not want stat or patch, then not running anything
> would be more appropriate, don't you think?
Ah, ok. :)  I'll change that way and send v3 soon!

Thanks, Namhyung

← back to recent threads