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

On Mon, Oct 11 2021, Thiago Perrotta wrote:

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

...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/