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

From: Felipe Contreras <felipe.contreras@gmail.com>
Date: Apr 14, 2012, 22:54 UTC
Message-ID: <CAMP44s3qXnONLh_=gO4_9DK2=bx0tLBTTAb4WenYh5k2QPQL-g@mail.gmail.com>
In-Reply-To: <87obqueyas.fsf@thomas.inf.ethz.ch>

On Sun, Apr 15, 2012 at 1:36 AM, Thomas Rast <trast@inf.ethz.ch> wrote:

Show 27 quoted lines

> 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?

That doesn't make any sense to me. If you want that, you should do 'eval "echo $foo"', and even if you do 'eval "echo \"$foo\""', that would be avoided.

> Also, I can't help but complain about your commit messages (again).

Well, we can start a discussion about how "simplify __gitcomp_1" does not explain sufficiently that this is indeed a simplification of __gitcomp_1, but from previous experiences you don't really want to discuss, you just want to be right, and me to follow orders.

Personally, I don't see why every modified line needs an ode.

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

Or

1. simplifying __gitcomp_1

> 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

This is a *bash* completion script.

If you *need* an essay for a commit message for a patch that essentially does nothing but simplify some code, I'll just drop it; it's not worth my effort.

Cheers.

-- 
Felipe Contreras