{"thread":{"id":"58850","subject":"[PATCH] send-email: disable option auto-abbreviation","startedAt":"2022-11-24T02:01:10Z","lastAt":"2022-11-28T12:40:08Z","messageCount":7,"participants":["Kyle Meyer","Junio C Hamano","Ævar Arnfjörð Bjarmason"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"467900","messageId":"20221124020056.242185-1-kyle@kyleam.com","threadId":"58850","inReplyTo":null,"subject":"[PATCH] send-email: disable option auto-abbreviation","fromName":"Kyle Meyer","fromEmail":"kyle@kyleam.com","sentAt":"2022-11-24T02:00:56Z","receivedAt":"2022-11-24T02:01:10Z","isPatch":true,"sender":{"key":"kyle@kyleam.com","avatar":"https://avatars.githubusercontent.com/u/1297788?v=4"},"body":"send-email supports specifying format-patch options.  However, some\nvalid format-patch short options trigger an error because Getopt's\ndefault auto-abbreviation is enabled.  For example, with\n\n  git send-email -v 3 @{u}\n\nthe -v is consumed as send-email's --validate, and 3 is passed on to\nthe format-patch call, leading to\n\n  fatal: ambiguous argument '3': unknown revision or path not in the\n  working tree.  [...]\n\nDisable Getopt's auto-abbreviation feature so that such options are\nproperly relayed to format-patch.  With this change, there is some\nrisk of breaking external scripts that rely on the abbreviation, but\nthat is hopefully unlikely given that Git does not advertise support\nfor auto-abbreviation and most subcommands do not support it.\n\nSigned-off-by: Kyle Meyer <kyle@kyleam.com>\n---\n git-send-email.perl   | 2 +-\n t/t9001-send-email.sh | 6 ++++++\n 2 files changed, 7 insertions(+), 1 deletion(-)\n\ndiff --git a/git-send-email.perl b/git-send-email.perl\nindex 5861e99a6e..1e6d5d7677 100755\n--- a/git-send-email.perl\n+++ b/git-send-email.perl\n@@ -24,7 +24,7 @@\n use Git;\n use Git::I18N;\n \n-Getopt::Long::Configure qw/ pass_through /;\n+Getopt::Long::Configure qw/ pass_through no_auto_abbrev /;\n \n package FakeTerm;\n sub new {\ndiff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh\nindex 01c74b8b07..c2ebf19ec6 100755\n--- a/t/t9001-send-email.sh\n+++ b/t/t9001-send-email.sh\n@@ -2334,6 +2334,12 @@ test_expect_success $PREREQ 'test that send-email works outside a repo' '\n \t\t\"$(pwd)/0001-add-main.patch\"\n '\n \n+test_expect_success $PREREQ 'send-email relays -v 4' '\n+\ttest_when_finished \"rm -f out\" &&\n+\tgit send-email --dry-run -v 4 -1 >out &&\n+\tgrep \"PATCH v4\" out\n+'\n+\n test_expect_success $PREREQ 'test that sendmail config is rejected' '\n \ttest_config sendmail.program sendmail &&\n \ttest_must_fail git send-email \\\n\nbase-commit: e7e5c6f715b2de7bea0d39c7d2ba887335b40aa0\n-- \n2.38.1\n\n"},{"id":"467976","messageId":"xmqqv8n3cxv9.fsf@gitster.g","threadId":"58850","inReplyTo":"20221124020056.242185-1-kyle@kyleam.com","subject":"Re: [PATCH] send-email: disable option auto-abbreviation","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-11-25T07:11:06Z","receivedAt":"2022-11-25T07:11:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Kyle Meyer <kyle@kyleam.com> writes:\n\n> send-email supports specifying format-patch options.  However, some\n> valid format-patch short options trigger an error because Getopt's\n> default auto-abbreviation is enabled.  For example, with\n>\n>   git send-email -v 3 @{u}\n>\n> the -v is consumed as send-email's --validate, and 3 is passed on to\n> the format-patch call, leading to\n>\n>   fatal: ambiguous argument '3': unknown revision or path not in the\n>   working tree.  [...]\n>\n> Disable Getopt's auto-abbreviation feature so that such options are\n> properly relayed to format-patch.  With this change, there is some\n> risk of breaking external scripts that rely on the abbreviation, but\n> that is hopefully unlikely given that Git does not advertise support\n> for auto-abbreviation and most subcommands do not support it.\n\nI personally have no sympathy to those who drive \"format-patch\" from\ninside \"send-email\".\n\nHaving said that.\n\nMany subcommands of \"git\" do take uniquely abbreviated double-dashed\noption names, but it is true that we do not allow --vanything to be\ngiven as -v even when there is no other double-dashed option that\nbegins with 'v', so \"git send-email -v\" that stands for \"git\nsend-email --validate\" indeed is an odd thing.\n\nBut robbing \"git send-email --val\" that expands to \"--validate\" from\nthe users is going a bit too far, I am afraid.  The right solution\nfor allowing \"-v 3\" given to \"format-patch\" I think is to make\nsend-email understand it and pass that through.  The presence of\nboth (\"validate\" => \\$validate) and (\"v\" => \\$reroll_count) in the\nGetOptions() argument would prevent \"-v\" to be taken as \"--validate\"\nwhile still allowing \"--val\" to be used as an abbrevatiion, no?\n\nBy the way, do we advertise support for any and all options to\nformat-patch when the feature to drive it from send-email is used?\nSome of the options (e.g. \"-o <directory>\") do not make any sense in\nthe context I would suspect.\n"},{"id":"468005","messageId":"87k03j54aj.fsf@kyleam.com","threadId":"58850","inReplyTo":"xmqqv8n3cxv9.fsf@gitster.g","subject":"Re: [PATCH] send-email: disable option auto-abbreviation","fromName":"Kyle Meyer","fromEmail":"kyle@kyleam.com","sentAt":"2022-11-25T17:31:48Z","receivedAt":"2022-11-25T17:38:15Z","isPatch":true,"sender":{"key":"kyle@kyleam.com","avatar":"https://avatars.githubusercontent.com/u/1297788?v=4"},"body":"Junio C Hamano writes:\n\n> Kyle Meyer <kyle@kyleam.com> writes:\n[...]\n>>   fatal: ambiguous argument '3': unknown revision or path not in the\n>>   working tree.  [...]\n>>\n>> Disable Getopt's auto-abbreviation feature so that such options are\n>> properly relayed to format-patch.  With this change, there is some\n>> risk of breaking external scripts that rely on the abbreviation, but\n>> that is hopefully unlikely given that Git does not advertise support\n>> for auto-abbreviation and most subcommands do not support it.\n>\n> I personally have no sympathy to those who drive \"format-patch\" from\n> inside \"send-email\".\n\nI'm not one of those users myself, but I was prompted to look into this\nby a report of the above error on another mailing list [*].  I do\nsympathize with \"skip the explicit format-patch\" users that find that\nerror confusing.\n\n  [*] https://yhetil.org/guix-patches/20221123190710.26517-1-paren@disroot.org\n\n> Many subcommands of \"git\" do take uniquely abbreviated double-dashed\n> option names, but it is true that we do not allow --vanything to be\n> given as -v even when there is no other double-dashed option that\n> begins with 'v', so \"git send-email -v\" that stands for \"git\n> send-email --validate\" indeed is an odd thing.\n\nThanks for the correction. I didn't realize that many subcommands\nsupported abbreviated options.  I expected it to be, at most, the\nremaining ones written in Perl.  When I tried out a couple of commands,\nI convinced myself that auto-abbreviation wasn't generally supported:\n\n  $ git log --onelin\n  fatal: unrecognized argument: --onelin\n  $ git diff --histog\n  error: invalid option: --histog\n\nBut I didn't look hard enough.  Trying again, I stumbled onto a few\ncounterexamples (e.g., `git status --shor` works and so does `git\nrange-diff --le ...`).\n\nAnd my claim in the commit message that \"Git does not advertise support\nfor auto-abbreviation\" is wrong.  I've now found this bit in gitcli(7):\n\n  Abbreviating long options\n  ~~~~~~~~~~~~~~~~~~~~~~~~~\n  Commands that support the enhanced option parser accepts unique\n  prefix of a long option as if it is fully spelled out, but use this\n  with a caution.  For example, `git commit --amen` behaves as if you\n  typed `git commit --amend`, but that is true only until a later version\n  of Git introduces another option that shares the same prefix,\n  e.g. `git commit --amenity` option.\n\n> But robbing \"git send-email --val\" that expands to \"--validate\" from\n> the users is going a bit too far, I am afraid.\n\nFair enough.  For the reasons above, the last sentence I wrote in the\ncommit message is invalid and can't justify the change.\n\n> The right solution for allowing \"-v 3\" given to \"format-patch\" I think\n> is to make send-email understand it and pass that through.  The\n> presence of both (\"validate\" => \\$validate) and (\"v\" =>\n> \\$reroll_count) in the GetOptions() argument would prevent \"-v\" to be\n> taken as \"--validate\" while still allowing \"--val\" to be used as an\n> abbrevatiion, no?\n\nI'd think that would work, yes.  I'll look more into going this route.\n\nWith that approach, there are other cases of abbreviation intercepting\nvalid format patch options.  For example, send-email doesn't have the\nshort option -n while format-patch does, but that doesn't make it\nthrough to format-patch:\n\n  $ git send-email --dry-run -n @{u} | grep Subj\n  Subject: [PATCH] send-email: disable option auto-abbreviation\n\n  $ git send-email --dry-run --numbered @{u} | grep Subj\n  Subject: [PATCH 1/1] send-email: disable option auto-abbreviation\n\n> By the way, do we advertise support for any and all options to\n> format-patch when the feature to drive it from send-email is used?\n> Some of the options (e.g. \"-o <directory>\") do not make any sense in\n> the context I would suspect.\n\nPassing an -o to send-email would cause its format-patch call to fail\nbecause send-email uses -o internally:\n\n  $ git send-email --dry-run -o . @{u}\n  fatal: two output directories?\n  format-patch -o /tmp/W1ZGCr0hwv -o @{u}: command returned error: 128\n\nIn any case, here's the only relevant part I spot from\ngit-send-email(1):\n\n  Patches can be specified as files, directories (which will send all\n  files in the directory), or directly as a revision list.  In the last\n  case, any format accepted by linkgit:git-format-patch[1] can be passed\n  to git send-email, as well as options understood by\n  linkgit:git-format-patch[1].\n\nSo, there's no mention that some options like -o do not make sense in\nthe send-email context, but perhaps that's obvious enough (at least in\nmy view it's much more obvious than '-v 3' and -n not being valid).\n\nThanks for the review.\n"},{"id":"468018","messageId":"87edtp5uws.fsf@kyleam.com","threadId":"58850","inReplyTo":"87k03j54aj.fsf@kyleam.com","subject":"[PATCH v2] send-email: relay '-v N' to format-patch","fromName":"Kyle Meyer","fromEmail":"kyle@kyleam.com","sentAt":"2022-11-26T20:21:23Z","receivedAt":"2022-11-26T20:21:33Z","isPatch":true,"sender":{"key":"kyle@kyleam.com","avatar":"https://avatars.githubusercontent.com/u/1297788?v=4"},"body":"Kyle Meyer writes:\n\n> Junio C Hamano writes:\n>\n>> The right solution for allowing \"-v 3\" given to \"format-patch\" I think\n>> is to make send-email understand it and pass that through.  The\n>> presence of both (\"validate\" => \\$validate) and (\"v\" =>\n>> \\$reroll_count) in the GetOptions() argument would prevent \"-v\" to be\n>> taken as \"--validate\" while still allowing \"--val\" to be used as an\n>> abbrevatiion, no?\n>\n> I'd think that would work, yes.  I'll look more into going this route.\n>\n> With that approach, there are other cases of abbreviation intercepting\n> valid format patch options.  [...]\n\nHere's a patch handling the -v case.  I don't plan on working on a more\ncomplete fix for the other cases (as I mentioned before, I don't use\nsend-email to drive format-patch), but in my opinion the -v fix by\nitself is still valuable.\n\n-- >8 --\nSubject: [PATCH v2] send-email: relay '-v N' to format-patch\n\nsend-email relays unrecognized arguments to its format-patch call.\nPassing '-v N' leads to an error because -v is consumed as\nsend-email's --validate.  For example,\n\n  git send-email -v 3 @{u}\n\nfails with\n\n  fatal: ambiguous argument '3': unknown revision or path not in the\n  working tree.  [...]\n\nTo prevent this, add the short --reroll-count option to send-email's\nmain option list and explicitly provide it to the format-patch call.\n\nThere other format-patch options that send-email doesn't relay\nproperly, including at least -n, -N, and the diff option -D.  Punt on\nthese because dealing with them is more complicated:\n\n * they would require configuring send-email to not ignore option case\n\n * send-email makes three GetOptions() calls with different sets of\n   options, the last being the main set of options.  Unlike -v, which\n   is consumed by the last GetOptions call, the -n, -N, and -D options\n   are consumed as abbreviations by the earlier calls.\n\nSigned-off-by: Kyle Meyer <kyle@kyleam.com>\n---\n git-send-email.perl   | 9 ++++++++-\n t/t9001-send-email.sh | 6 ++++++\n 2 files changed, 14 insertions(+), 1 deletion(-)\n\ndiff --git a/git-send-email.perl b/git-send-email.perl\nindex 5861e99a6e..07f2a0cbea 100755\n--- a/git-send-email.perl\n+++ b/git-send-email.perl\n@@ -220,6 +220,10 @@ sub format_2822_time {\n my $force = 0;\n my $dump_aliases = 0;\n \n+# Variables to prevent short format-patch options from being captured\n+# as abbreviated send-email options\n+my $reroll_count;\n+\n # Handle interactive edition of files.\n my $multiedit;\n my $editor;\n@@ -542,6 +546,7 @@ sub config_regexp {\n \t\t    \"batch-size=i\" => \\$batch_size,\n \t\t    \"relogin-delay=i\" => \\$relogin_delay,\n \t\t    \"git-completion-helper\" => \\$git_completion_helper,\n+\t\t    \"v=s\" => \\$reroll_count,\n );\n $rc = GetOptions(%options);\n \n@@ -782,7 +787,9 @@ sub is_format_patch_arg {\n \tdie __(\"Cannot run git format-patch from outside a repository\\n\")\n \t\tunless $repo;\n \trequire File::Temp;\n-\tpush @files, $repo->command('format-patch', '-o', File::Temp::tempdir(CLEANUP => 1), @rev_list_opts);\n+\tpush @files, $repo->command('format-patch', '-o', File::Temp::tempdir(CLEANUP => 1),\n+\t\t\t\t    defined $reroll_count ? ('-v', $reroll_count) : (),\n+\t\t\t\t    @rev_list_opts);\n }\n \n @files = handle_backup_files(@files);\ndiff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh\nindex 01c74b8b07..152bd2c697 100755\n--- a/t/t9001-send-email.sh\n+++ b/t/t9001-send-email.sh\n@@ -2334,6 +2334,12 @@ test_expect_success $PREREQ 'test that send-email works outside a repo' '\n \t\t\"$(pwd)/0001-add-main.patch\"\n '\n \n+test_expect_success $PREREQ 'send-email relays -v 3 to format-patch' '\n+\ttest_when_finished \"rm -f out\" &&\n+\tgit send-email --dry-run -v 3 -1 >out &&\n+\tgrep \"PATCH v3\" out\n+'\n+\n test_expect_success $PREREQ 'test that sendmail config is rejected' '\n \ttest_config sendmail.program sendmail &&\n \ttest_must_fail git send-email \\\n\nbase-commit: e7e5c6f715b2de7bea0d39c7d2ba887335b40aa0\n-- \n2.38.1\n\n"},{"id":"468031","messageId":"xmqqzgcd9ok2.fsf@gitster.g","threadId":"58850","inReplyTo":"87edtp5uws.fsf@kyleam.com","subject":"Re: [PATCH v2] send-email: relay '-v N' to format-patch","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-11-27T01:25:01Z","receivedAt":"2022-11-27T01:25:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Kyle Meyer <kyle@kyleam.com> writes:\n\n> Here's a patch handling the -v case.  I don't plan on working on a more\n> complete fix for the other cases (as I mentioned before, I don't use\n> send-email to drive format-patch), but in my opinion the -v fix by\n> itself is still valuable.\n\nYup, I think it is a good place to stop for the first patch.  Other\npeople can add more when they discover the need, and anything more\ncomplex [*] is probably not worth the effort, I would think.\n\n    Side note: [*] we could imagine running \"git format-patch -h\"\n    (or a new variant of it), parse its output and populate the\n    %options dynamically, for example.\n\nWill queue.  Thanks.\n"},{"id":"468066","messageId":"xmqqzgcb5scv.fsf@gitster.g","threadId":"58850","inReplyTo":"87edtp5uws.fsf@kyleam.com","subject":"Re: [PATCH v2] send-email: relay '-v N' to format-patch","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-11-28T09:41:04Z","receivedAt":"2022-11-28T09:41:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Kyle Meyer <kyle@kyleam.com> writes:\n\n> diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh\n> index 01c74b8b07..152bd2c697 100755\n> --- a/t/t9001-send-email.sh\n> +++ b/t/t9001-send-email.sh\n> @@ -2334,6 +2334,12 @@ test_expect_success $PREREQ 'test that send-email works outside a repo' '\n>  \t\t\"$(pwd)/0001-add-main.patch\"\n>  '\n>  \n> +test_expect_success $PREREQ 'send-email relays -v 3 to format-patch' '\n> +\ttest_when_finished \"rm -f out\" &&\n> +\tgit send-email --dry-run -v 3 -1 >out &&\n> +\tgrep \"PATCH v3\" out\n> +'\n> +\n>  test_expect_success $PREREQ 'test that sendmail config is rejected' '\n>  \ttest_config sendmail.program sendmail &&\n>  \ttest_must_fail git send-email \\\n>\n> base-commit: e7e5c6f715b2de7bea0d39c7d2ba887335b40aa0\n\nIt seems that this new test, by invoking format-patch, makes a leaks\ncheck at GitHub CI fail.\n\n  https://github.com/git/git/actions/runs/3562362890/jobs/5984036422\n\nDropping PASSES_SANITIZE_LEAK from the test script would certainly\nbe a short-term workaround, though, but it is a rather broad\nmechanism.  There should be a better way to control the leak\nchecker, but that is what we currently have X-<.\n\n"},{"id":"468080","messageId":"221128.86v8mzl0bh.gmgdl@evledraar.gmail.com","threadId":"58850","inReplyTo":"xmqqzgcd9ok2.fsf@gitster.g","subject":"Re: [PATCH v2] send-email: relay '-v N' to format-patch","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-11-28T12:34:32Z","receivedAt":"2022-11-28T12:40:08Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Sun, Nov 27 2022, Junio C Hamano wrote:\n\n> Kyle Meyer <kyle@kyleam.com> writes:\n>\n>> Here's a patch handling the -v case.  I don't plan on working on a more\n>> complete fix for the other cases (as I mentioned before, I don't use\n>> send-email to drive format-patch), but in my opinion the -v fix by\n>> itself is still valuable.\n>\n> Yup, I think it is a good place to stop for the first patch.  Other\n> people can add more when they discover the need, and anything more\n> complex [*] is probably not worth the effort, I would think.\n>\n>     Side note: [*] we could imagine running \"git format-patch -h\"\n>     (or a new variant of it), parse its output and populate the\n>     %options dynamically, for example.\n>\n> Will queue.  Thanks.\n\nThis is just a comment on the #leftoverbits: I've looked at this option\nparsing in \"git-send-email\" before, and IMO the right long-term fix is\nto split out the *.perl code into a \"git send-email--helper\", and do the\noption parsing in C using our parse_options().\n\nSome of it will be a bit of a hassle, but it should be much easier after\n8de2e2e41b2 (Merge branch 'ab/send-email-optim', 2021-07-22) (and the\nsubsquent regression fix).\n\n\n"}]}