threads / patch / 29481

patch, 4 partscompletion: trivial cleanups

Subject: [PATCH 0/4] completion: trivial cleanups

## tl;dr

17 messages between Jan 30, 2012 and Jan 31, 2012. Diffs are folded; open one to read it.

replies: 16people: 5as markdown or json

Felipe Contreras· Jan 30, 2012, 17:23 UTC · lore
From: Felipe Contreras <felipe.contreras@gmail.com>
And an improvement for zsh.
Felipe Contreras (4):
  completion: simplify __git_remotes
  completion: remove unused code
  completion: cleanup __gitcomp*
  completion: be nicer with zsh
 contrib/completion/git-completion.bash |   66 +++++---------------------------
 1 files changed, 10 insertions(+), 56 deletions(-)
-- 
1.7.8
Felipe Contreras· Jan 30, 2012, 17:23 UTC · re: Felipe Contreras · lore

[PATCH v2 2/4] completion: remove unused code

From: Felipe Contreras <felipe.contreras@gmail.com>
No need for this rather complicated piece of code anymore :)
Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 contrib/completion/git-completion.bash |   30 ------------------------------
 1 files changed, 0 insertions(+), 30 deletions(-)
Show changes to contrib/completion/git-completion.bash +0 −30
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 086e38d..4f68f0a 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -2728,33 +2728,3 @@ if [ Cygwin = "$(uname -o 2>/dev/null)" ]; then
 complete -o bashdefault -o default -o nospace -F _git git.exe 2>/dev/null \
 	|| complete -o default -o nospace -F _git git.exe
 fi
-
-if [[ -n ${ZSH_VERSION-} ]]; then
-	__git_shopt () {
-		local option
-		if [ $# -ne 2 ]; then
-			echo "USAGE: $0 (-q|-s|-u) <option>" >&2
-			return 1
-		fi
-		case "$2" in
-		nullglob)
-			option="$2"
-			;;
-		*)
-			echo "$0: invalid option: $2" >&2
-			return 1
-		esac
-		case "$1" in
-		-q)	setopt | grep -q "$option" ;;
-		-u)	unsetopt "$option" ;;
-		-s)	setopt "$option" ;;
-		*)
-			echo "$0: invalid flag: $1" >&2
-			return 1
-		esac
-	}
-else
-	__git_shopt () {
-		shopt "$@"
-	}
-fi

-- 
1.7.8
Jonathan Nieder· Jan 30, 2012, 17:34 UTC · lore

Re: [PATCH v2 1/4] completion: simplify __git_remotes

Felipe Contreras wrote:
Show 7 quoted lines
> From: Felipe Contreras <felipe.contreras@gmail.com>
>
> There's no need for all that complicated code that requires nullglob,
> and the complexities related to such option.
>
> As an advantage, this would allow us to get rid of __git_shopt, which is
> used only in this fuction to enable 'nullglob' in zsh.

That is all a longwinded way to say "zsh doesn't support the same interface as bash for setting the nullglob option, so let's avoid it and use 'ls' which is simpler", right?

There's a potential speed regression involved --- using "ls" involves an extra fork/exec. I believe you have thought about this and done a little to mitigate it; could you explain this in the commit message so future coders know what features of your code need to be preserved?

Please consider squashing this with patch 2/4. It will make both patches way easier to understand on their own.

Cc-ing Gábor, who I imagine is more familiar with this code than I am.
Show 26 quoted lines
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
>  contrib/completion/git-completion.bash |    7 +------
>  1 files changed, 1 insertions(+), 6 deletions(-)
>
> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> index 1496c6d..086e38d 100755
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -644,12 +644,7 @@ __git_refs_remotes ()
>  __git_remotes ()
>  {
>  	local i ngoff IFS=$'\n' d="$(__gitdir)"
> -	__git_shopt -q nullglob || ngoff=1
> -	__git_shopt -s nullglob
> -	for i in "$d/remotes"/*; do
> -		echo ${i#$d/remotes/}
> -	done
> -	[ "$ngoff" ] && __git_shopt -u nullglob
> +	test -d "$d/remotes" && ls -1 "$d/remotes"
>  	for i in $(git --git-dir="$d" config --get-regexp 'remote\..*\.url' 2>/dev/null); do
>  		i="${i#remote.}"
>  		echo "${i/.url*/}"
> -- 
> 1.7.8
>
Junio C Hamano· Jan 30, 2012, 18:27 UTC · re: Jonathan Nieder · lore

Re: [PATCH v2 1/4] completion: simplify __git_remotes

Jonathan Nieder <jrnieder@gmail.com> writes:
Show 13 quoted lines
> Felipe Contreras wrote:
>
>> From: Felipe Contreras <felipe.contreras@gmail.com>
>>
>> There's no need for all that complicated code that requires nullglob,
>> and the complexities related to such option.
>>
>> As an advantage, this would allow us to get rid of __git_shopt, which is
>> used only in this fuction to enable 'nullglob' in zsh.
>
> That is all a longwinded way to say "zsh doesn't support the same
> interface as bash for setting the nullglob option, so let's avoid
> it and use 'ls' which is simpler", right?
;-)
Show 15 quoted lines
>> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
>> index 1496c6d..086e38d 100755
>> --- a/contrib/completion/git-completion.bash
>> +++ b/contrib/completion/git-completion.bash
>> @@ -644,12 +644,7 @@ __git_refs_remotes ()
>>  __git_remotes ()
>>  {
>>  	local i ngoff IFS=$'\n' d="$(__gitdir)"
>> -	__git_shopt -q nullglob || ngoff=1
>> -	__git_shopt -s nullglob
>> -	for i in "$d/remotes"/*; do
>> -		echo ${i#$d/remotes/}
>> -	done
>> -	[ "$ngoff" ] && __git_shopt -u nullglob
>> +	test -d "$d/remotes" && ls -1 "$d/remotes"
Yeah, very nice reduction of unnecessary code.

The original loop might have been justifiable if it were doing something more meaningful inside (e.g. making sure the file really describes a remote), but as far as I can tell, it merely is a poor-man's emulation of "ls -1".

You updated it to make the code say what it wanted to say in the way it should have said from day one ;-).

Jonathan Nieder· Jan 30, 2012, 17:50 UTC · lore

Re: [PATCH v2 3/4] completion: cleanup __gitcomp*

Felipe Contreras wrote:
> I don't know why there's so much code; these functions don't seem to be
> doing much:

Unless you mean "This patch has had inadequate review and I don't understand the code I'm patching, so do not trust it", please drop this commentary or place it after the three dashes.

Show 9 quoted lines
>  * no need to check $#, ${3:-$cur} is much easier
>  * __gitcomp_nl doesn't seem to using the initial IFS
>
> This makes the code much simpler.
>
> Eventually it would be nice to wrap everything that touches compgen and
> COMPREPLY in one function for the zsh wrapper.
>
> Comments by Jonathan Nieder.

I don't want this acknowledgement. Who should care that I commented on something?

> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
>  contrib/completion/git-completion.bash |   20 +++-----------------
>  1 files changed, 3 insertions(+), 17 deletions(-)

This diffstat tells me more of what I wanted to know about the patch than the description did.

I imagine it would have been enough to say something along the lines of "The __gitcomp and __gitcomp_nl functions are unnecessarily verbose. __gitcomp_nl sets IFS to " \t\n" unnecessarily before setting it to "\n" by mistake. Both functions use 'if' statements to read parameters with defaults, where the ${parameter:-default} idiom would be just as clear. By fixing these, we can make each function almost a one-liner."

By the way, the subject ("clean up __gitcomp*") tells me almost as little as something like "fix __gitcomp*". A person reading the shortlog would like to know _how_ you are fixing it, or what the impact of the change will be --- e.g., something like "simplify __gitcomp and __gitcomp_nl" would be clearer.

[...]
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
[...]
Show 18 quoted lines
> @@ -524,18 +520,8 @@ __gitcomp ()
>  #    appended.
>  __gitcomp_nl ()
>  {
> -	local s=$'\n' IFS=' '$'\t'$'\n'
> -	local cur_="$cur" suffix=" "
> -
> -	if [ $# -gt 2 ]; then
> -		cur_="$3"
> -		if [ $# -gt 3 ]; then
> -			suffix="$4"
> -		fi
> -	fi
> -
> -	IFS=$s
> -	COMPREPLY=($(compgen -P "${2-}" -S "$suffix" -W "$1" -- "$cur_"))
> +	local IFS=$'\n'
> +	COMPREPLY=($(compgen -P "${2-}" -S "${4:- }" -W "$1" -- "${3:-$cur}"))

This loses the nice name $suffix for the -S argument. Not a problem, just noticing.

Junio C Hamano· Jan 30, 2012, 19:03 UTC · re: Jonathan Nieder · lore

Re: [PATCH v2 3/4] completion: cleanup __gitcomp*

Jonathan Nieder <jrnieder@gmail.com> writes:
Show 12 quoted lines
> I imagine it would have been enough to say something along the lines of
> "The __gitcomp and __gitcomp_nl functions are unnecessarily verbose.
> __gitcomp_nl sets IFS to " \t\n" unnecessarily before setting it to "\n"
> by mistake.  Both functions use 'if' statements to read parameters
> with defaults, where the ${parameter:-default} idiom would be just as
> clear.  By fixing these, we can make each function almost a one-liner."
>
> By the way, the subject ("clean up __gitcomp*") tells me almost as
> little as something like "fix __gitcomp*".  A person reading the
> shortlog would like to know _how_ you are fixing it, or what the
> impact of the change will be --- e.g., something like "simplify
> __gitcomp and __gitcomp_nl" would be clearer.
I love both of the above two paragraphs.  Thanks.
Show 25 quoted lines
> [...]
>> --- a/contrib/completion/git-completion.bash
>> +++ b/contrib/completion/git-completion.bash
> [...]
>> @@ -524,18 +520,8 @@ __gitcomp ()
>>  #    appended.
>>  __gitcomp_nl ()
>>  {
>> -	local s=$'\n' IFS=' '$'\t'$'\n'
>> -	local cur_="$cur" suffix=" "
>> -
>> -	if [ $# -gt 2 ]; then
>> -		cur_="$3"
>> -		if [ $# -gt 3 ]; then
>> -			suffix="$4"
>> -		fi
>> -	fi
>> -
>> -	IFS=$s
>> -	COMPREPLY=($(compgen -P "${2-}" -S "$suffix" -W "$1" -- "$cur_"))
>> +	local IFS=$'\n'
>> +	COMPREPLY=($(compgen -P "${2-}" -S "${4:- }" -W "$1" -- "${3:-$cur}"))
>
> This loses the nice name $suffix for the -S argument.  Not a problem,
> just noticing.
The patch looks good, including the localness that is kept for IFS.
Junio C Hamano· Jan 30, 2012, 21:25 UTC · re: Junio C Hamano · lore

Re: [PATCH v2 3/4] completion: cleanup __gitcomp*

I managed to pick up 1 & 2:
 1. be nicer with zsh (aka avoid default value assignment on : true
    command);
 2. simplify __git_remotes (aka use ls -1 instead of rolling a loop to do
    that ourselves).

But I do not see your patch for 3 & 4 either on gmane archive nor in my mailbox (via vger, not direct delivery from you to me). The threads for these two patches both begin with Jonathan's review for me.

Care to resend 3 & 4?
Thanks.
SZEDER Gábor· Jan 31, 2012, 00:15 UTC · re: Jonathan Nieder · lore

Re: [PATCH v2 3/4] completion: cleanup __gitcomp*

Hi,
On Mon, Jan 30, 2012 at 11:50:04AM -0600, Jonathan Nieder wrote:
Show 33 quoted lines
> Felipe Contreras wrote:
> 
> > I don't know why there's so much code; these functions don't seem to be
> > doing much:
> 
> Unless you mean "This patch has had inadequate review and I don't
> understand the code I'm patching, so do not trust it", please drop
> this commentary or place it after the three dashes.
> 
> >  * no need to check $#, ${3:-$cur} is much easier
> >  * __gitcomp_nl doesn't seem to using the initial IFS
> >
> > This makes the code much simpler.
> >
> > Eventually it would be nice to wrap everything that touches compgen and
> > COMPREPLY in one function for the zsh wrapper.
> >
> > Comments by Jonathan Nieder.
> 
> I don't want this acknowledgement.  Who should care that I commented
> on something?
> 
> > Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> > ---
> >  contrib/completion/git-completion.bash |   20 +++-----------------
> >  1 files changed, 3 insertions(+), 17 deletions(-)
> 
> This diffstat tells me more of what I wanted to know about the patch
> than the description did.
> 
> I imagine it would have been enough to say something along the lines of
> "The __gitcomp and __gitcomp_nl functions are unnecessarily verbose.
> __gitcomp_nl sets IFS to " \t\n" unnecessarily

Yeah, that's unnecessary. I'm not sure why I did that, perhaps just blindly followed suit of gitcomp_1(), without realizing that I don't do any word-splitting in __gitcomp_nl() except when invoking compgen.

> before setting it to "\n"
> by mistake.

But that is deliberate, that's why it's called __gitcomp_nl(), see a31e6262 (completion: optimize refs completion, 2011-10-15), third paragraph.

Show 35 quoted lines
>  Both functions use 'if' statements to read parameters
> with defaults, where the ${parameter:-default} idiom would be just as
> clear.  By fixing these, we can make each function almost a one-liner."
> 
> By the way, the subject ("clean up __gitcomp*") tells me almost as
> little as something like "fix __gitcomp*".  A person reading the
> shortlog would like to know _how_ you are fixing it, or what the
> impact of the change will be --- e.g., something like "simplify
> __gitcomp and __gitcomp_nl" would be clearer.
> 
> [...]
> > --- a/contrib/completion/git-completion.bash
> > +++ b/contrib/completion/git-completion.bash
> [...]
> > @@ -524,18 +520,8 @@ __gitcomp ()
> >  #    appended.
> >  __gitcomp_nl ()
> >  {
> > -	local s=$'\n' IFS=' '$'\t'$'\n'
> > -	local cur_="$cur" suffix=" "
> > -
> > -	if [ $# -gt 2 ]; then
> > -		cur_="$3"
> > -		if [ $# -gt 3 ]; then
> > -			suffix="$4"
> > -		fi
> > -	fi
> > -
> > -	IFS=$s
> > -	COMPREPLY=($(compgen -P "${2-}" -S "$suffix" -W "$1" -- "$cur_"))
> > +	local IFS=$'\n'
> > +	COMPREPLY=($(compgen -P "${2-}" -S "${4:- }" -W "$1" -- "${3:-$cur}"))
> 
> This loses the nice name $suffix for the -S argument.  Not a problem,
> just noticing.

I think loosing the name of $suffix would be OK, because the comment above the function explains what the fourth parameter is about.

However, that comment also says that "If [the 4. argument is] specified but empty, nothing is appended.", but this patch changes this behavior, because "${4:- }" is substituted by a SP when $4 is an empty string. You have to drop the colon and use "${4- }" there:

$ foo="" $ echo ,${foo:- }, , , $ echo ,${foo- }, ,,

Best, Gábor

Jonathan Nieder· Jan 31, 2012, 00:25 UTC · re: SZEDER Gábor · lore

Re: [PATCH v2 3/4] completion: cleanup __gitcomp*

SZEDER Gábor wrote:
> On Mon, Jan 30, 2012 at 11:50:04AM -0600, Jonathan Nieder wrote:
Show 14 quoted lines
>> I imagine it would have been enough to say something along the lines of
>> "The __gitcomp and __gitcomp_nl functions are unnecessarily verbose.
>> __gitcomp_nl sets IFS to " \t\n" unnecessarily
>
> Yeah, that's unnecessary.  I'm not sure why I did that, perhaps just
> blindly followed suit of gitcomp_1(), without realizing that I don't
> do any word-splitting in __gitcomp_nl() except when invoking compgen.
>
>> before setting it to "\n"
>> by mistake.
>
> But that is deliberate, that's why it's called __gitcomp_nl(), see
> a31e6262 (completion: optimize refs completion, 2011-10-15), third
> paragraph.

Yep, sorry for the ambiguity. I meant that setting IFS to " \t\n" (before setting it to "\n") was not done for any serious reason. The explanation is definitely clearer with "by mistake" dropped.

Thanks, Jonathan

Jonathan Nieder· Jan 30, 2012, 17:53 UTC · lore

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

Felipe Contreras wrote:
Show 9 quoted lines
> --- 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? 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?
Felipe Contreras· Jan 30, 2012, 18:10 UTC · re: Jonathan Nieder · lore

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

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
Jonathan Nieder· Jan 30, 2012, 18:25 UTC · re: Felipe Contreras · lore

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

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.

Felipe Contreras· Jan 30, 2012, 18:56 UTC · re: Jonathan Nieder · lore

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

On Mon, Jan 30, 2012 at 8:25 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
Show 9 quoted lines
> 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.
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.

Show 10 quoted lines
> [...]
>> 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.
Maybe '[' then.
-- 
Felipe Contreras
Jonathan Nieder· Jan 30, 2012, 19:03 UTC · re: Felipe Contreras · lore

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

Felipe Contreras wrote:
> On Mon, Jan 30, 2012 at 8:25 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
Show 6 quoted lines
>> : 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

Junio C Hamano· Jan 30, 2012, 19:09 UTC · re: Jonathan Nieder · lore

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

Jonathan Nieder <jrnieder@gmail.com> writes:
Show 9 quoted lines
> 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.
s/no-op/true/ ;-)
> ..., so I suppose consistency is the only reason to
> prefer one over another.
Yes. And the script may probably use [[ very heavily.
Early return after || i.e.
	A || return
        B

simply looks ugly and misleading, especially when the remainder B is just a single line. But I stopped caring about the styles in this particular script long time ago, so...

Felipe Contreras· Jan 30, 2012, 19:22 UTC · re: Junio C Hamano · lore

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

2012/1/30 Junio C Hamano <gitster@pobox.com>:
Show 15 quoted lines
> Jonathan Nieder <jrnieder@gmail.com> writes:
>> Felipe Contreras wrote:
>> ..., so I suppose consistency is the only reason to
>> prefer one over another.
>
> Yes. And the script may probably use [[ very heavily.
>
> Early return after || i.e.
>
>        A || return
>        B
>
> simply looks ugly and misleading, especially when the remainder B is just
> a single line.  But I stopped caring about the styles in this particular
> script long time ago, so...
What would you rather use?

[ "$__git_merge_strategies" ] && __git_merge_strategies=$(__git_list_merge_strategies)

That's 90 characters long. Although much better without the 2>/dev/null.
if [ "$__git_merge_strategies" ]; then
  __git_merge_strategies=$(__git_list_merge_strategies)
fi
That's even more lines =/
-- 
Felipe Contreras
Jonathan Nieder· Jan 30, 2012, 19:28 UTC · re: Felipe Contreras · lore

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

Felipe Contreras wrote:
Show 10 quoted lines
> What would you rather use?
>
> [ "$__git_merge_strategies" ] &&
> __git_merge_strategies=$(__git_list_merge_strategies)
>
> That's 90 characters long. Although much better without the 2>/dev/null.
>
> if [ "$__git_merge_strategies" ]; then
>   __git_merge_strategies=$(__git_list_merge_strategies)
> fi

Neither, since they both have the test inverted. But I prefer to explicitly use the "-n" operator.

	[[ -n $__git_merge_strategies ]] ||
	__git_merge_strategies=$(__git_list_merge_strategies)

seems as fine as any other spelling to me. It means "either this value has already been computed and cached, or we need to compute it".

← back to recent threads