From: Thomas Rast <trast@inf.ethz.ch>
Date: Sat, 14 Apr 2012 22:36:11 GMT
Subject: Re: [PATCH v3 1/5] completion: simplify __gitcomp_1
Message-ID: <87obqueyas.fsf@thomas.inf.ethz.ch>
In-Reply-To: <1334439784-6460-2-git-send-email-felipe.contreras@gmail.com>

Felipe Contreras <felipe.contreras@gmail.com> writes:

>  # __gitcomp_1 requires 2 arguments
>  __gitcomp_1 ()
>  {
> -	local c IFS=' '$'\t'$'\n'
> +	local c s IFS=' '$'\t'$'\n'
>  	for c in $1; do
>  		case "$c$2" in
> -		--*=*) printf %s$'\n' "$c$2" ;;
> -		*.)    printf %s$'\n' "$c$2" ;;
> -		*)     printf %s$'\n' "$c$2 " ;;
> +		--*=* | *.) s="" ;;
> +		*)          s=" " ;;
>  		esac
> +		echo "$c$2$s"
>  	done
>  }

Sorry for not noticing earlier, but...

I did a double take at the change to 'echo'.  I'm guessing from the
patch that $c$2$s is never just '-e' or some other option taken by the
bash version[1] of echo.  But can you be sure?  Do you know off hand
whether '-nbogus' complains, treats the -n as usual and prints 'bogus',
or echoes '-nbogus'[2]?  Are you sure future bash versions won't break
this?


Also, I can't help but complain about your commit messages (again).
Compare with Jonathan's in 4/5.  His patch is all of a one-line(!)
change

 			--exec-path
+			--exec-path=

yet his commit message is two paragraphs worth of explanations why the
changed behavior is more helpful than what we had before.

On the other hand, your commit message for the above says only

  completion: simplify __gitcomp_1

However, your patch is actually two different changes:

1. the refactoring of the partial command: printf %s$'\n' "$c$2
2. the change to echo

The latter is not in any way explained or justified by your (total
absence of a) commit message.


Footnotes: 
[1]  POSIX states that echo "shall not support any options" and "shall
not recognize the -- argument", but we have printfs all over the code
base because option support is extremely inconsistent

[2]  Spoiler: it prints -nbogus literally

-- 
Thomas Rast
trast@{inf,student}.ethz.ch