Re: [PATCH v5 0/3] send-email: shell completion improvements

From: Thiago Perrotta <tbperrotta@gmail.com>
Date: Sep 30, 2021, 03:10 UTC
Message-ID: <CABOtWuqXS_kJk2md=kgg-ReaWtKermpUW_Dk_bc0pMXQL+xMeA@mail.gmail.com>
In-Reply-To: <87bl4h3fgv.fsf@evledraar.gmail.com>

On Fri, 24 Sept 2021 at 16:07, Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

Show 11 quoted lines

> I meant something like the below patch, feel free to incorporate it if
> you'd like with my signed-off-by, i.e. there's no reason to parse the
> usage message, or hardcode another set of options, we've got it right
> there as structured program data being fed to the GetOptions() function.
>
> All we need to do is to assign that to a hash, and use it both for
> emitting the help and to call GetOptions().
>
> What I have doesn't *quite* work, i.e. the --git-completion-helper
> expects "--foo=" I think for things that are "foo=s" in perl, so the
> regex needs adjusting, but that should be an easy addition on top.

1) Thanks Ævar, I get the gist of it. Your approach revealed a few issues with the current usage string:

The following options exist in GetOptions but not in the usage string:

--git-completion-helper --no-signed-off-cc --sender --signed-off-cc

Out of these, I'd argue --git-completion-helper is intentionally omitted, however --sender and --signed-off-cc were overlooked.

2) Also, your patch misses --dump-aliases and --identity; that's because they are in other GetOptions functions in the file.

The two obvious possibilities here are either (i) hard-code them directly, i.e.:

-my @options = sort @gse_options, @fpa_options; +my @options = sort @gse_options, @fpa_options, "--dump-aliases", "--sender";

or (ii) refactor the other two GetOptions like you did in your patch, so that `sub completion_helper` ends up receiving all three hashes (or maybe a single hash as a result of all three merged).

Any preference between (i) or (ii)? I am leaning towards (i).

3) Finally, I noticed that "sort @gse_options, @fpa_options" doesn't really sort fpa_options.

If sorting is really intended, it would be better to modify the source of format-patch to emit sorted output.

Otherwise, we may as well leave it untouched. AFAIK from a completion perspective it seems that it doesn't matter: both bash and zsh emit `git format-patch --<TAB>` sorted today, even though the output of `git format-patch --git-completion-helper` isn't sorted. The only benefit of sorting I see would be to deduplicate ('uniq') flags.

Do you agree with this rationale? Either way, let me know whether or not it's preferable to sort. I'll probably sort `send-email` options anyway just to deduplicate a few flags such as --to-cover, but `format-patch` could remain as is.

I'll wait for replies before sending another patch (on top of your original one).