Re: [PATCH v8 0/2] send-email: shell completion improvements

On Mon, Oct 25 2021, Thiago Perrotta wrote:

quoted

...I think that re-indentation is better left alone for the patch
readability.

Reverted the `GetOptions` indentation. Noise is now gone :-)

Thanks.

quoted

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.

Makes sense I think, will take a look.

quoted

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.

Digging a bit more: It's for folding away options that are negated, not
for completing "-<TAB>". See the examples at b221b5ab9b9 (completion:
collapse extra --no-.. options, 2018-06-06).

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.

Right, because some --no-whatever we define because we've got a
--whatever and it's boolean, but for others we've got a --no-whatever as
the primary, as in th case of --no-template..

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.

Yeah sorry about the confusion so far, it's also been a voyage of
discovery for me :)

This time around I tested with:

diff --git a/parse-options.c b/parse-options.c
index 6e0535bdaad..d659309c5e7 100644
--- a/parse-options.c
+++ b/parse-options.c

@@ -515,8 +515,6 @@ void parse_options_start(struct parse_opt_ctx_t *ctx,
 static void show_negated_gitcomp(const struct option *opts, int show_all,
                                 int nr_noopts)
 {
-       int printed_dashdash = 0;
-
        for (; opts->type != OPTION_END; opts++) {
                int has_unset_form = 0;
                const char *name;

@@ -551,10 +549,6 @@ static void show_negated_gitcomp(const struct option *opts, int show_all,
                        if (nr_noopts < 0)
                                printf(" --%s", name);
                } else if (nr_noopts >= 0) {
-                       if (nr_noopts && !printed_dashdash) {
-                               printf(" --");
-                               printed_dashdash = 1;
-                       }
                        printf(" --no-%s", opts->long_name);
                        nr_noopts++;
                }

Which will fail a test in t/t9902-completion.sh showing this specific
behavior.