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

[Namhyung Kim <namhyung@gmail.com>]

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