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

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Oct 25, 2021, 22:44 UTC
Message-ID: <211026.86o87c1zry.gmgdl@evledraar.gmail.com>
In-Reply-To: <20211025212707.188151-1-tbperrotta@gmail.com>

On Mon, Oct 25 2021, Thiago Perrotta wrote:

>> ...I think that re-indentation is better left alone for the patch
>> readability.
>
> Reverted the `GetOptions` indentation. Noise is now gone :-)

Thanks.

Show 8 quoted lines

>> 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.

Show 20 quoted lines

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

Show 9 quoted lines

> 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..

Show 9 quoted lines

> 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.