| Message ID | 20211011041033.20004-1-tbperrotta@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | send-email: shell completion improvements | expand |
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/
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 - do not reuse the options variable, for improved readability. 2/3: One extra improvement: - deduplicate send-email and format-patch common options (for example, now --from appears only once) 3/3: - updated git-send-email.perl USAGE to be consistent with git-send-email(1) - still haven't touched anything else, waiting on Carlos's RFC patch ongoing discussion to settle. Thiago Perrotta (3): send-email: terminate --git-completion-helper with LF send-email: programmatically generate bash completions send-email docs: add format-patch options Documentation/git-send-email.txt | 6 +- contrib/completion/git-completion.bash | 11 +- git-send-email.perl | 156 ++++++++++++++----------- t/t9902-completion.sh | 3 + 4 files changed, 97 insertions(+), 79 deletions(-)