On Monday 2009 January 12 21:14:40 Junio C Hamano wrote:
>Ted Pavlic <ted@xxxxxxxxxxxxx> writes:
>> A vim modeline has also been added for consistency.
>
>Yuck.
While I dislike emacs and use vim pretty much exclusively, I don't really see
the need for a vim modeline. On top of that, fdm=marker is a bit silly since
there aren't any markers. ff=unix and ft=sh are redundant, as any vim should
detect these properly, given the filename.
So, I'm slightly negative on the modeline hunk.
>> @@ -111,7 +115,7 @@ __git_ps1 ()
>> fi
>> fi
>>
>> - if [ -n "$1" ]; then
>> + if [ $# -gt 0 ] && [ -n "$1" ]; then
>
>I found the previous round's [ -n "${1-}" ] much easier to read, if we were
> to do this. If -n "${1-}", then "$1" is definitely set so nothing need to
> change in the then ... else part.
I found "${1-}" ugly, and this a bit better, but I'll defer to Junio.
>> @@ -131,11 +136,22 @@ __gitcomp_1 ()
>> done
>> }
>>
>> +# __gitcomp accepts 1, 2, 3, or 4 arguments
>> +# generates completion reply with compgen
>> __gitcomp ()
>> {
>> - local cur="${COMP_WORDS[COMP_CWORD]}"
>> - if [ $# -gt 2 ]; then
>> - cur="$3"
>> + local one two cur="${COMP_WORDS[COMP_CWORD]}" four
>> + if [ $# -gt 0 ]; then
>> + one="$1"
>> + if [ $# -gt 1 ]; then
>> + two="$2"
>> + if [ $# -gt 2 ]; then
>> + cur="$3"
>> + if [ $# -gt 3 ]; then
>> + four="$4"
>> + fi
>> + fi
>> + fi
>> fi
>
>Yuck.
Definitely agreeing with Junio here. This is far less ascetic than the old
patch. Truth be told, this whole thread would probably have been more
productive without me chiming in. Sorry about that.
>If you are taking advantage of the fact that "local one"
>will bind one to emptiness anyway, can't you do something like:
>
> local one=${1-} two=${2-} cur=${3-} four=${4-}
Even better to use variable names that match the usage, if possible.
--
Boyd Stephen Smith Jr. ,= ,-_-. =.
bss@xxxxxxxxxxxxxxxxx ((_/)o o(\_))
ICQ: 514984 YM/AIM: DaTwinkDaddy `-'(. .)`-'
http://iguanasuicide.net/ \_/
Attachment:
signature.asc
Description: This is a digitally signed message part.
