Felipe Contreras <felipe.contreras@xxxxxxxxx> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html