Re: [PATCH v2 01/41] parse-options: support --git-completion-helper

From: SZEDER Gábor <szeder.dev@gmail.com>
Date: Feb 7, 2018, 23:41 UTC
Message-ID: <20180207234152.5087-1-szeder.dev@gmail.com>
In-Reply-To: <CAPig+cSxxEn5q_5gv9JDXprnnocTTaLaugnhYmpHJz5tzOAkEg@mail.gmail.com>

Show 13 quoted lines

> > OK how about some thing like this fixup patch? __gitcomp_builtin now
> > allows to add extra options as well as remove some.
> >
> > -- 8< --
> >  __gitcomp_builtin ()
> >  {
> > +       local incl="$2"
> > +       local excl="$3"
> > +               options="$(__git ${cmd/_/ } --git-completion-helper) $incl "
> > +               for i in $excl; do
> > +                       options="${options/$i /}"
> 
> Is 'options' guaranteed to end with a space?

It is, note the space before the closing double quote in:

  options="$(__git ${cmd/_/ } --git-completion-helper) $incl "

> If not, then this
> expulsion will fail for the very last option. I'd think you can get by
> fine with just "${options/$i}".

I would prefer a space both at the beginning and at the end of the pattern. Please excuse the contrived corner case, but it could still fail if the option to be excluded is a suffix of an other option:

  $ o="$(echo --foo--bar --baz --bar) "
  $ echo "'${o/--bar /}'"       # exclude '--bar'
  '--foo--baz --bar '

Maybe we'll never have --opt--ions with a doubledash in them[1], but still...

  $ o=" $(echo --foo--bar --baz --bar) "
  $ echo "'${o/ --bar / }'"
  ' --foo--bar --baz '

[1] - Interestingly, grep shows that the German translation does
      contain a '--reset--author'.