From: Felipe Contreras <felipe.contreras@gmail.com>
Date: Mon, 30 Jan 2012 18:10:01 GMT
Subject: Re: [PATCH v2 4/4] completion: be nicer with zsh
Message-ID: <CAMP44s0ACC+AnwHGtBLe8C1S_sxWj6SbMbawDThvLQAA0pKMYQ@mail.gmail.com>
In-Reply-To: <20120130175324.GH10618@burratino>

On Mon, Jan 30, 2012 at 7:53 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Felipe Contreras wrote:
>
>> --- a/contrib/completion/git-completion.bash
>> +++ b/contrib/completion/git-completion.bash
>> @@ -657,7 +657,8 @@ __git_merge_strategies=
>>  # is needed.
>>  __git_compute_merge_strategies ()
>>  {
>> -     : ${__git_merge_strategies:=$(__git_list_merge_strategies)}
>> +     test "$__git_merge_strategies" && return
>> +     __git_merge_strategies=$(__git_list_merge_strategies 2> /dev/null)
>
> Why the new redirect?

It's not new, it was in the original code that your change to the ':'
stuff (eaa4e6e) replaced.

And the reason is explained right above, in the comment:

# 'git merge -s help' (and thus detection of the merge strategy
# list) fails, unfortunately, if run outside of any git working
# tree.  __git_merge_strategies is set to the empty string in
# that case, and the detection will be repeated the next time it
# is needed.

The commands might fail, that's why '2> /dev/null' was used before,
and ':' is used right now.

> If I add debugging output to __git_list_merge_strategies that writes to stderr, I want to see it.

Well, you wouldn't see it right now, so that out of scope of this patch.

> Why the 'test "$foo"' form instead of [[ -n which is more common in
> this completion script?  Why use "return" instead of
>
>        [[ -n $var ]] || var=$(...)
>
> which feels a little simpler?

Because this is _huge_:

[[ "$__git_merge_strategies" ]] ||
__git_merge_strategies=$(__git_list_merge_strategies 2> /dev/null)

And IMO harder to read. But you are correct that most of the code uses
[[]], which I think is a shame. But I guess people want to keep using
that.

So, how about?

[[ "$__git_merge_strategies" ]] && return
__git_merge_strategies=$(__git_list_merge_strategies 2> /dev/null)

-- 
Felipe Contreras