Re: [PATCH v2 4/4] completion: be nicer with zsh

From: Felipe Contreras <felipe.contreras@gmail.com>
Date: Jan 30, 2012, 18:10 UTC
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:

Show 13 quoted lines

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

Show 6 quoted lines

> 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