Re: [PATCH v3 1/5] completion: simplify __gitcomp_1

From: Thomas Rast <trast@inf.ethz.ch>
Date: Apr 14, 2012, 22:36 UTC
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:

Show 16 quoted lines

>  # __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