[PATCH v8 0/2] send-email: shell completion improvements
From: Thiago Perrotta <hidden>
Date: 2021-10-25 21:27:26
Possibly related (same subject, not in this thread)
- 2021-10-28 · Re: [PATCH v8 0/2] send-email: shell completion improvements · Junio C Hamano <hidden>
...I think that re-indentation is better left alone for the patch readability.
Reverted the `GetOptions` indentation. Noise is now gone :-)
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 don't quite see how this would fit into the commit message. A comment in the code seems to fit better to account for this detail. That's what I did, but if you still disagree, please elaborate where in the commit message this sentence should be added.
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).
I interpreted that standalone `--` as an extraneous / useless token. If it's there intentionally, then I am reverting my stripping of it. At the end of the day whether to include it or not is a small detail, but FYI, when I do: $ git clone -<TAB> in bash, nothing happens. I would have expected it to be expanded to "--" because of the explicit "--", but it doesn't. Therefore my conclusion is that "--" in the output of "--git-completion-helper" is useless. Am I missing something? What would be the function of the standalone "--" then? From my local testing, whether the options are sorted or not, whether they are repeated or not, whether they follow a specific order with respect to "--" or not, all of those details seem not to matter. Bash completion seems to handle all of those cases just fine interactively. In fact, here's another example: $ git init --git-completion-helper | tr ' ' '\n' | grep -C1 '^--$' --no-template -- --no-bare ...there are --no-* options both _before_ and _after_ the --. I really cannot see the point of the -- in the output, it seems to be just noise. I readded -- to the output anyway since you requested it, but if it needs to follow a certain spec w.r.t. ordering, we should have tests for it. This specific part (the -- and the --no- order thing) of the commit is something I am not keen to doing though, at least not in this patch series; sorry, it already goes far beyond the scope of my original intent. Anything else you ask for that is inline with the original intent (like generating options programatically instead of hard-coding them) I am fine with though, and in fact I believe I have addressed all comments so far, if there's anything else I may have missed let me know.
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.
OK, I see. This is a valid point, I updated the code to reflect this desired behavior. PTAL. If needed we could make it more DRY, but as it is now it seems quite readable.
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.
OK, done. Introduced a "not_for_completion" hash. Doesn't seem to make a huge difference compared to just using `grep` though. Differences from V7: As per Ævar's comments: - completely drop 1/3 since we'now using split instead of print to incorporate format-patch options. Side effect: then-extraneous '\n' is now gone. - revert indentation (tabs) on GetOptions to eliminate diff noise (as per Ævar's interpretation of original Carlos's comment) - re-add "--" to the output of "git send-email --git-completion-helper" - introduce a "not_for_completion" hash to remove undesired options from the output - add trailing = to send-email options that expect a string or an integer argument, consistently with format-patch options Thiago Perrotta (2): 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 | 56 +++++++++++++++++++++----- t/t9902-completion.sh | 3 ++ 4 files changed, 54 insertions(+), 22 deletions(-) -- 2.33.1