@@ -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_strategies2>/dev/null)
Why the new redirect? If I add debugging output to
__git_list_merge_strategies that writes to stderr, I want to see it.
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?
# 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
From: Jonathan Nieder <hidden> Date: 2016-06-15 22:52:53
Felipe Contreras wrote:
The commands might fail, that's why '2> /dev/null' was used before,
and ':' is used right now.
Wait, what?
: is a no-op command. It does not redirect stderr automatically or
do any other magical thing.
[...]
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.
[[ has simpler syntax wrt quoting and other details. But now that I
check, the code uses [ a lot, too (which, like "test", is a plain
built-in command), so I suppose consistency is the only reason to
prefer one over another. "git log --grep='if \['" tells me the use of
'[' instead of 'test' here is deliberate.
From: Felipe Contreras <hidden> Date: 2016-06-15 22:52:53
On Mon, Jan 30, 2012 at 8:25 PM, Jonathan Nieder [off-list ref] wrote:
Felipe Contreras wrote:
quoted
The commands might fail, that's why '2> /dev/null' was used before,
and ':' is used right now.
Wait, what?
: is a no-op command. It does not redirect stderr automatically or
do any other magical thing.
Why don't you go ahead and try it?
bash -c ': echo "err" > /dev/stderr'
I don't see anything here.
But actually, if I use $(echo "err" > /dev/stderr); _then_ I get
something. Smells a lot like a bug to me.
In any case, if you expected ':' to print errors, now I understand why
you removed 2>/dev/null in eaa4e6e.
[...]
quoted
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.
[[ has simpler syntax wrt quoting and other details. But now that I
check, the code uses [ a lot, too (which, like "test", is a plain
built-in command), so I suppose consistency is the only reason to
prefer one over another. "git log --grep='if \['" tells me the use of
'[' instead of 'test' here is deliberate.
From: Jonathan Nieder <hidden> Date: 2016-06-15 22:52:53
Felipe Contreras wrote:
On Mon, Jan 30, 2012 at 8:25 PM, Jonathan Nieder [off-list ref] wrote:
quoted
: is a no-op command. It does not redirect stderr automatically or
do any other magical thing.
Why don't you go ahead and try it?
bash -c ': echo "err" > /dev/stderr'
: is a no-op command. If you have any questions after reading about
it in your manual or online help system of choice, I'll be happy to
answer them.
[...]
Maybe '[' then.
Honestly, I don't care. :)
(If I had to choose a convention for scripts specific to ksh-style
shells, in order of preference, I would rank them:
1. Always use [[.
2. Use "test", spelled out, like the portable shell code in git does.
3. Use [.
If you have arguments for one convention or another that are
compelling enough that the codebase won't be flipping back and forth
and a patch to go along with them, I imagine no one will mind.)
By the way, since I forget to say enough: thanks for taking care about
this code. Simpler code is definitely a good thing.
Regards,
Jonathan