On Mon, Jan 30, 2012 at 7:53 PM, Jonathan Nieder <jrnieder@xxxxxxxxx> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html