| Message ID | 20210924024606.20542-1-tbperrotta@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | send-email: shell completion improvements | expand |
On Thu, Sep 23 2021, Thiago Perrotta wrote: > Thanks for the heads up, I am slowly getting used to this email > workflow, this is my first contribution. Hopefully I got it right this > time. > >> Isn't this just: > >> my @params = <getopts list>; >> GetOptions(@params); > > Let me know if the regex approach I went with is OK. You seem to have > suggested me to do something with the `GetOptions` module, but I'm > afraid I only know the basics of Perl. > > I tried to do something like `GetOptions($USAGE)` but it didn't quite > work (clearly I have no idea how to do that :P). If you have something > specific in mind, I'd appreciate if you could send a small patch back > that I can incorporate. Otherwise, either way, the current regex > approach isn't too horrible and seems to be reasonably reliable. 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. -- >8 -- diff --git a/git-send-email.perl b/git-send-email.perl index 5262d88ee32..221115fbbdd 100755 --- a/git-send-email.perl +++ b/git-send-email.perl @@ -114,8 +114,18 @@ sub usage { } sub completion_helper { - print Git::command('format-patch', '--git-completion-helper'); - exit(0); + my ($options) = @_; + + my @gse_options = sort map { + "--$_" + } map { + s/(?:[:=][si]|!)$//; + split /\|/, $_; + } keys %$options; + my @fpa_options = Git::command('format-patch', '--git-completion-helper'); + my @options = sort @gse_options, @fpa_options; + print "@options\n"; + exit(0); } # most mail servers generate the Date: header, but not all... @@ -449,7 +459,7 @@ sub config_regexp { usage() unless $rc; die __("--dump-aliases incompatible with other options\n") if !$help and $dump_aliases and @ARGV; -$rc = GetOptions( +my %options = ( "sender|from=s" => \$sender, "in-reply-to=s" => \$initial_in_reply_to, "reply-to=s" => \$reply_to, @@ -508,7 +518,8 @@ sub config_regexp { "batch-size=i" => \$batch_size, "relogin-delay=i" => \$relogin_delay, "git-completion-helper" => \$git_completion_helper, - ); +); +$rc = GetOptions(%options); # Munge any "either config or getopt, not both" variables my @initial_to = @getopt_to ? @getopt_to : ($no_to ? () : @config_to); @@ -516,7 +527,7 @@ sub config_regexp { my @initial_bcc = @getopt_bcc ? @getopt_bcc : ($no_bcc ? () : @config_bcc); usage() if $help; -completion_helper() if $git_completion_helper; +completion_helper(\%options) if $git_completion_helper; unless ($rc) { usage(); }
On Fri, 24 Sept 2021 at 16:07, Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote: > 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).