Re: [PATCH v7 0/3] send-email: shell completion improvements

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Mon, Oct 11 2021, Thiago Perrotta wrote:

> Differences from V6:
>
> 2/3: Addresses all of Carlos's comments:
>   - make indentation consistent (tabs): note that there's a giant diff
>     for the largest GetOptions now, it adds a bit of noise to the patch

I took Carlo's suggestion to mean to indent your uniq function, not to
re-indent a bunch of existing code while at it...

>   - do not reuse the options variable, for improved readability.

...I think that re-indentation is better left alone for the patch
readability.

Anyway, sorry about not looking at this sooner after my off-the-cuff
[1]; I think this looks mostly good-ish, but there's a few broken things
here:

First, in your 1/3 you're adding a \n, but in 2/3 we end up with \n\n. I
think you can just skip 1/3, maybe mention "how it also has a "\n" in
the commit message.

I.e. you start implicitly picking up the newline because you changed
from a "print" to a "split", and latter imposes Perl's scalar context on
its argument, but the former doesn't. That's a combination of some Perl
trivia and our own Git.pm being overly clever about wantarray(), but
there you go.

More importantly in [1] I meant that last paragraph as a "and as an
excercise for the reader..".

I.e. we should not simply strip the trailing "=" etc., we need to parse
those out of the Perl GetOptions arguments, and come up with mapping to
what we do in parse-options.c. I think that's basically adding a "=" at
the end of all options that end with "=s", ":i", "=d", ":s" etc.

You then strip out "--" arguments from the combined list, but isn't this
something we do need to emit? I.e. it's used as syntax by the bash
completion isn't it? (I just skimmed the relevant C code in
parse-options.c).
    
    $ git clone --git-completion-helper | tr ' ' '\n' | grep -C2 -- ^--$
    --hardlinks
    --tags
    --
    --no-verbose
    --no-quiet

For --no-foo arguments we emit both a --foo and --no-foo in it, that
sort of (maybe entirely) works in your version because some/all (I
haven't checked all) options have corresponding "foo" arguments for
"no-foo", so maybe it sort of works out, but does the ordering
before/after the "--", and that we strip out the "--" but e.g. "git
clone" will emit it?

We then don't want to emit "-h", but you strip out "--h", first we
mapped "h" to "--h" in the loop above, so we should do that there. But
better yet we have a "%dump_aliases_options" already, maybe it +
"git-completion-helper" can be moved to another "%not_for_completion"
hash or something.

The map/map/keys loop also will silently do something odd if it starts
getting input data it didn't expect, i.e. it would be more reliable as a
regex check, and then die() if we start getting somethnig we don't
expect, i.e. if someone adds an option not covered by our regex.

That it's a map/map/keys is just some off-the-cuff Perl hacking on my
part, I think for validation etc. it's usually better to just turn it
into a plain old boring for-loop.

1. https://lore.kernel.org/git/87bl4h3fgv.fsf@evledraar.gmail.com/