[PATCH] stash: Add stash.showFlag config variable

[PATCH] stash: Add stash.showFlag config variable

[Namhyung Kim]

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

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

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

[SZEDER Gábor]

Hi,

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

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


>  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

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

[Namhyung Kim]

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!

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

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


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

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

[Eric Sunshine]

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

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

[Namhyung Kim]

Hi,

On Thu, Aug 27, 2015 at 08:16:35PM -0400, Eric Sunshine wrote:
> 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

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

[Junio C Hamano]

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

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

[Namhyung Kim]

Hi,

On Thu, Aug 27, 2015 at 06:08:39PM -0700, Junio C Hamano wrote:
> 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.


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

:)

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

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

[Junio C Hamano]

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?

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

[Namhyung Kim]

Hi,

On Sat, Aug 29, 2015 at 3:10 AM, Junio C Hamano <gitster@pobox.com> wrote:
> 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