Thread (44 messages) flat view 44 messages, 5 authors, 2021-10-26
STALE1792d

Revision v8 of 7 in this series.

Revisions (7)
  1. v2 [diff vs current]
  2. v3 [diff vs current]
  3. v4 [diff vs current]
  4. v5 [diff vs current]
  5. v6 [diff vs current]
  6. v7 [diff vs current]
  7. v8 current

[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)

...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
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help