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

On Mon, Jan 30, 2012 at 7:53 PM, Jonathan Nieder [off-list ref] wrote:

Felipe Contreras wrote:

quoted

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