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@xxxxxxxxxxxxxxxxxxx/
