{"thread":{"id":"60255","subject":"[REGRESSION] uninitialized value $address in git send-email when given multiple recipients separated by commas","startedAt":"2023-09-22T09:28:02Z","lastAt":"2023-10-30T09:13:30Z","messageCount":47,"participants":["Bagas Sanjaya","Jeff King","Todd Zullinger","Michael Strawbridge","Junio C Hamano","Oswald Buddenhagen","Eric Sunshine","Uwe Kleine-König"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"482126","messageId":"ZQ1eGzqfyoeeTBUq@debian.me","threadId":"60255","inReplyTo":null,"subject":"[REGRESSION] uninitialized value $address in git send-email when given multiple recipients separated by commas","fromName":"Bagas Sanjaya","fromEmail":"bagasdotme@gmail.com","sentAt":"2023-09-22T09:27:55Z","receivedAt":"2023-09-22T09:28:02Z","isPatch":false,"sender":{"key":"bagasdotme@gmail.com","avatar":"https://avatars.githubusercontent.com/u/40219486?v=4"},"body":"Hi,\n\nThis regression is similar to one I reported earlier [1], with the same\nerror message but with slightly different reproducer (and as continuation\nto the former report).\n\nI use git-send-email(1) to submit patches (occasional doc fixes) to LKML.\nRather than having to type multiple --to/--cc addresses, I use the undocumented\nbehavior of listing multiple addresses separated by comma in a single --to/--cc\noption. i.e.:\n\n```\n$ git send-email \\\n --to=\"foo ,bar \" \\\n --cc=\"main list , sub list \" \\\n /path/to/series/*.patch\n```\n\n[This is the same behavior as Thunderbird.]\n\nIn my linux kernel tree (linux.git) used for development, I add\nsendemail-validate hook that adds DKIM-like attestation with patatt:\n\n```\n#!/bin/sh\n# installed by patatt install-hook\npatatt sign --hook \"${1}\"\n```\n\nStarting from Git v2.41.0, when I try to use git-send-email(1), I got\nperl-related error:\n\n```\nUse of uninitialized value $address in sprintf at /home/bagas/.app/git/dist/v2.42.0/libexec/git-core/git-send-email line 1172.\nerror: unable to extract a valid address from:\n```\n\nIt looks like git-send-email(1) trips on cover letter since there is no\nrecipient addresses there, and also on patches without Signed-off-by: trailer.\n\nBisecting between v2.40.0 and v2.41.0, the culprit is commit a8022c5f7b67\n(send-email: expose header information to git-send-email's sendemail-validate\nhook, 2023-04-19). The perl error should have been reduced by [2], but this\naddress splitting (parsing) is still not addressed.\n\nThe full bisection log is:\n\n```\ngit bisect start '--term-good=ok' '--term-bad=oops'\n# status: waiting for both good and bad commits\n# ok: [73876f4861cd3d187a4682290ab75c9dccadbc56] Git 2.40\ngit bisect ok 73876f4861cd3d187a4682290ab75c9dccadbc56\n# status: waiting for bad commit, 1 good commit known\n# oops: [fe86abd7511a9a6862d5706c6fa1d9b57a63ba09] Git 2.41\ngit bisect oops fe86abd7511a9a6862d5706c6fa1d9b57a63ba09\n# ok: [b64894c2063e5875bfd95b537eafcb3e1abf46ff] Merge branch 'ow/ref-filter-omit-empty'\ngit bisect ok b64894c2063e5875bfd95b537eafcb3e1abf46ff\n# ok: [ccd12a3d6cc62f51b746654ae56e26d92f89ba92] Merge branch 'en/header-split-cache-h-part-2'\ngit bisect ok ccd12a3d6cc62f51b746654ae56e26d92f89ba92\n# oops: [1e1dcb2a423cad350e8f20fdcc957064e5cff528] Merge branch 'jc/dirstat-plug-leaks'\ngit bisect oops 1e1dcb2a423cad350e8f20fdcc957064e5cff528\n# oops: [40a5d2b79b57378cc36d43d3b30e704100dc1492] Merge branch 'fc/doc-man-lift-title-length-limit'\ngit bisect oops 40a5d2b79b57378cc36d43d3b30e704100dc1492\n# ok: [07ac32fff94b245aec3e2b80efad0b5dada629cb] Merge branch 'ma/gittutorial-fixes'\ngit bisect ok 07ac32fff94b245aec3e2b80efad0b5dada629cb\n# oops: [7f3cc51b284d696fdb8dfbd8c9f9d0c014019d93] Merge branch 'ar/test-cleanup-unused-file-creation-part2'\ngit bisect oops 7f3cc51b284d696fdb8dfbd8c9f9d0c014019d93\n# oops: [b6e9521956b752be4c666efedd7b91bdd05f9756] Merge branch 'ms/send-email-feed-header-to-validate-hook'\ngit bisect oops b6e9521956b752be4c666efedd7b91bdd05f9756\n# ok: [e2abfa7212525daa24a52d9f53c45b736abb5dfe] Merge branch 'hx/negotiator-non-recursive'\ngit bisect ok e2abfa7212525daa24a52d9f53c45b736abb5dfe\n# oops: [a8022c5f7b678189135b6caa3fadb3d8ec0c0d48] send-email: expose header information to git-send-email's sendemail-validate hook\ngit bisect oops a8022c5f7b678189135b6caa3fadb3d8ec0c0d48\n# ok: [56adddaa06d376f3977ee91e8a769cd85439d21c] send-email: refactor header generation functions\ngit bisect ok 56adddaa06d376f3977ee91e8a769cd85439d21c\n# first oops commit: [a8022c5f7b678189135b6caa3fadb3d8ec0c0d48] send-email: expose header information to git-send-email's sendemail-validate hook\n```\n\nTo reproduce this regression:\n\n1. Clone git.git repo, then branch off:\n\n ```\n $ git clone https://github.com/git/git.git && cd git\n $ git checkout -b test\n ```\n\n2. Make two dummy signed-off commits:\n\n ```\n $ echo test > test && git add test && git commit -s -m \"test\"\n $ echo \"test test\" >> test && git commit -a -s -m \"test test\"\n ```\n\n3. Generate patch series:\n\n ```\n $ mkdir /tmp/test\n $ git format-patch -o /tmp/test --cover-letter main\n ```\n\n4. Send the series to dummy address:\n\n ```\n $ git send-email --to=\"foo ,bar \" /tmp/test/*.patch\n ```\n\nMy system runs Debian testing (trixie/sid) with perl 5.36.0.\n\nThanks.\n\n[1]: https://lore.kernel.org/git/ZQhI5fMhDE82awpE@debian.me/\n[2]: https://lore.kernel.org/git/545729b619308c6f3397b9aa1747f26ddc58f461.1695054945.git.me@ttaylorr.com/\n\n-- \nAn old man doll... just what I always wanted! - Clara\n"},{"id":"482217","messageId":"20230924033625.GA1492190@coredump.intra.peff.net","threadId":"60255","inReplyTo":"ZQ1eGzqfyoeeTBUq@debian.me","subject":"Re: [REGRESSION] uninitialized value $address in git send-email when given multiple recipients separated by commas","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-09-24T03:36:25Z","receivedAt":"2023-09-24T03:38:25Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Sep 22, 2023 at 04:27:55PM +0700, Bagas Sanjaya wrote:\n\n> To reproduce this regression:\n\nI couldn't reproduce the problem here.\n\nI had to modify your instructions slightly:\n\n> 1. Clone git.git repo, then branch off:\n> \n> ```\n> $ git clone https://github.com/git/git.git && cd git\n> $ git checkout -b test\n> ```\n> \n> 2. Make two dummy signed-off commits:\n> \n> ```\n> $ echo test > test && git add test && git commit -s -m \"test\"\n> $ echo \"test test\" >> test && git commit -a -s -m \"test test\"\n> ```\n\nThis all worked.\n\n> 3. Generate patch series:\n> \n> ```\n> $ mkdir /tmp/test\n> $ git format-patch -o /tmp/test --cover-letter main\n> ```\n\nThis should be s/main/master/, since the git.git repo from step 1 does\nnot have a \"main\" branch.\n\n> 4. Send the series to dummy address:\n> \n> ```\n> $ git send-email --to=\"foo ,bar \" /tmp/test/*.patch\n> ```\n\nThis did not produce an error for me. I switched out acme.com for some\naddresses I control, and confirmed that the mail was all delivered fine.\n\nYour report also mentions a validation hook, so I tried installing one\nlike:\n\n\tcat >.git/hooks/sendemail-validate <<-\\EOF\n\t#!/bin/sh\n\techo >&2 running validate hook\n\texit 0\n\tEOF\n\tchmod +x .git/hooks/sendemail-validate\n\nand confirmed that the hook runs (three times, as expected). But still\nno error. I'm using v2.41.0 to test against.\n\n-Peff\n"},{"id":"482238","messageId":"ZRE6q8dHPFRIQezX@debian.me","threadId":"60255","inReplyTo":"20230924033625.GA1492190@coredump.intra.peff.net","subject":"Re: [REGRESSION] uninitialized value $address in git send-email when given multiple recipients separated by commas","fromName":"Bagas Sanjaya","fromEmail":"bagasdotme@gmail.com","sentAt":"2023-09-25T07:45:47Z","receivedAt":"2023-09-25T07:46:46Z","isPatch":false,"sender":{"key":"bagasdotme@gmail.com","avatar":"https://avatars.githubusercontent.com/u/40219486?v=4"},"body":"On Sat, Sep 23, 2023 at 11:36:25PM -0400, Jeff King wrote:\n> Your report also mentions a validation hook, so I tried installing one\n> like:\n> \n> \tcat >.git/hooks/sendemail-validate <<-\\EOF\n> \t#!/bin/sh\n> \techo >&2 running validate hook\n> \texit 0\n> \tEOF\n> \tchmod +x .git/hooks/sendemail-validate\n> \n> and confirmed that the hook runs (three times, as expected). But still\n> no error. I'm using v2.41.0 to test against.\n> \n\nHi Jeff,\n\nI think you missed perl version. As stated earlier, I'm on Debian testing\nwith perl v5.36.0. On there, `perl -V` outputs:\n\n```\nSummary of my perl5 (revision 5 version 36 subversion 0) configuration:\n \n Platform:\n osname=linux\n osvers=4.19.0\n archname=x86_64-linux-gnu-thread-multi\n uname='linux localhost 4.19.0 #1 smp debian 4.19.0 x86_64 gnulinux '\n config_args='-Dmksymlinks -Dusethreads -Duselargefiles -Dcc=x86_64-linux-gnu-gcc -Dcpp=x86_64-linux-gnu-cpp -Dld=x86_64-linux-gnu-gcc -Dccflags=-DDEBIAN -Wdate-time -D_FORTIFY_SOURCE=2 -g -O2 -ffile-prefix-map=/dummy/build/dir=. -fstack-protector-strong -fstack-clash-protection -Wformat -Werror=format-security -fcf-protection -Dldflags= -Wl,-z,relro -Dlddlflags=-shared -Wl,-z,relro -Dcccdlflags=-fPIC -Darchname=x86_64-linux-gnu -Dprefix=/usr -Dprivlib=/usr/share/perl/5.36 -Darchlib=/usr/lib/x86_64-linux-gnu/perl/5.36 -Dvendorprefix=/usr -Dvendorlib=/usr/share/perl5 -Dvendorarch=/usr/lib/x86_64-linux-gnu/perl5/5.36 -Dsiteprefix=/usr/local -Dsitelib=/usr/local/share/perl/5.36.0 -Dsitearch=/usr/local/lib/x86_64-linux-gnu/perl/5.36.0 -Dman1dir=/usr/share/man/man1 -Dman3dir=/usr/share/man/man3 -Dsiteman1dir=/usr/local/man/man1 -Dsiteman3dir=/usr/local/man/man3 -Duse64bitint -Dman1ext=1 -Dman3ext=3perl -Dpager=/usr/bin/sensible-pager -Uafs -Ud_csh -Ud_ualarm -Uusesfio -Uusenm -Ui_libutil -Ui_xlocale -Uversiononly -Ud_strlcpy -Ud_strlcat -DDEBUGGING=-g -Doptimize=-O2 -dEs -Duseshrplib -Dlibperl=libperl.so.5.36.0'\n hint=recommended\n useposix=true\n d_sigaction=define\n useithreads=define\n usemultiplicity=define\n use64bitint=define\n use64bitall=define\n uselongdouble=undef\n usemymalloc=n\n default_inc_excludes_dot=define\n Compiler:\n cc='x86_64-linux-gnu-gcc'\n ccflags ='-D_REENTRANT -D_GNU_SOURCE -DDEBIAN -fwrapv -fno-strict-aliasing -pipe -I/usr/local/include -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64'\n optimize='-O2 -g'\n cppflags='-D_REENTRANT -D_GNU_SOURCE -DDEBIAN -fwrapv -fno-strict-aliasing -pipe -I/usr/local/include'\n ccversion=''\n gccversion='13.2.0'\n gccosandvers=''\n intsize=4\n longsize=8\n ptrsize=8\n doublesize=8\n byteorder=12345678\n doublekind=3\n d_longlong=define\n longlongsize=8\n d_longdbl=define\n longdblsize=16\n longdblkind=3\n ivtype='long'\n ivsize=8\n nvtype='double'\n nvsize=8\n Off_t='off_t'\n lseeksize=8\n alignbytes=8\n prototype=define\n Linker and Libraries:\n ld='x86_64-linux-gnu-gcc'\n ldflags =' -fstack-protector-strong -L/usr/local/lib'\n libpth=/usr/local/lib /usr/lib/x86_64-linux-gnu /usr/lib /lib/x86_64-linux-gnu /lib\n libs=-lgdbm -lgdbm_compat -ldb -ldl -lm -lpthread -lc -lcrypt\n perllibs=-ldl -lm -lpthread -lc -lcrypt\n libc=/lib/x86_64-linux-gnu/libc.so.6\n so=so\n useshrplib=true\n libperl=libperl.so.5.36\n gnulibc_version='2.37'\n Dynamic Linking:\n dlsrc=dl_dlopen.xs\n dlext=so\n d_dlsymun=undef\n ccdlflags='-Wl,-E'\n cccdlflags='-fPIC'\n lddlflags='-shared -L/usr/local/lib -fstack-protector-strong'\n\n\nCharacteristics of this binary (from libperl): \n Compile-time options:\n HAS_TIMES\n MULTIPLICITY\n PERLIO_LAYERS\n PERL_COPY_ON_WRITE\n PERL_DONT_CREATE_GVSV\n PERL_MALLOC_WRAP\n PERL_OP_PARENT\n PERL_PRESERVE_IVUV\n USE_64_BIT_ALL\n USE_64_BIT_INT\n USE_ITHREADS\n USE_LARGE_FILES\n USE_LOCALE\n USE_LOCALE_COLLATE\n USE_LOCALE_CTYPE\n USE_LOCALE_NUMERIC\n USE_LOCALE_TIME\n USE_PERLIO\n USE_PERL_ATOF\n USE_REENTRANT_API\n USE_THREAD_SAFE_LOCALE\n Locally applied patches:\n DEBPKG:debian/cpan_definstalldirs - Provide a sensible INSTALLDIRS default for modules installed from CPAN.\n DEBPKG:debian/db_file_ver - https://bugs.debian.org/340047 Remove overly restrictive DB_File version check.\n DEBPKG:debian/doc_info - Replace generic man(1) instructions with Debian-specific information.\n DEBPKG:debian/enc2xs_inc - https://bugs.debian.org/290336 Tweak enc2xs to follow symlinks and ignore missing @INC directories.\n DEBPKG:debian/errno_ver - https://bugs.debian.org/343351 Remove Errno version check due to upgrade problems with long-running processes.\n DEBPKG:debian/libperl_embed_doc - https://bugs.debian.org/186778 Note that libperl-dev package is required for embedded linking\n DEBPKG:fixes/respect_umask - Respect umask during installation\n DEBPKG:debian/writable_site_dirs - Set umask approproately for site install directories\n DEBPKG:debian/extutils_set_libperl_path - EU:MM: set location of libperl.a under /usr/lib\n DEBPKG:debian/no_packlist_perllocal - Don't install .packlist or perllocal.pod for perl or vendor\n DEBPKG:debian/fakeroot - Postpone LD_LIBRARY_PATH evaluation to the binary targets.\n DEBPKG:debian/instmodsh_doc - Debian policy doesn't install .packlist files for core or vendor.\n DEBPKG:debian/ld_run_path - Remove standard libs from LD_RUN_PATH as per Debian policy.\n DEBPKG:debian/libnet_config_path - Set location of libnet.cfg to /etc/perl/Net as /usr may not be writable.\n DEBPKG:debian/perlivp - https://bugs.debian.org/510895 Make perlivp skip include directories in /usr/local\n DEBPKG:debian/squelch-locale-warnings - https://bugs.debian.org/508764 Squelch locale warnings in Debian package maintainer scripts\n DEBPKG:debian/patchlevel - https://bugs.debian.org/567489 List packaged patches for 5.36.0-9 in patchlevel.h\n DEBPKG:fixes/document_makemaker_ccflags - https://bugs.debian.org/628522 [rt.cpan.org #68613] Document that CCFLAGS should include $Config{ccflags}\n DEBPKG:debian/find_html2text - https://bugs.debian.org/640479 Configure CPAN::Distribution with correct name of html2text\n DEBPKG:debian/perl5db-x-terminal-emulator.patch - https://bugs.debian.org/668490 Invoke x-terminal-emulator rather than xterm in perl5db.pl\n DEBPKG:debian/cpan-missing-site-dirs - https://bugs.debian.org/688842 Fix CPAN::FirstTime defaults with nonexisting site dirs if a parent is writable\n DEBPKG:fixes/memoize_storable_nstore - [rt.cpan.org #77790] https://bugs.debian.org/587650 Memoize::Storable: respect 'nstore' option not respected\n DEBPKG:debian/makemaker-pasthru - https://bugs.debian.org/758471 Pass LD settings through to subdirectories\n DEBPKG:debian/makemaker-manext - https://bugs.debian.org/247370 Make EU::MakeMaker honour MANnEXT settings in generated manpage headers\n DEBPKG:debian/kfreebsd-softupdates - https://bugs.debian.org/796798 Work around Debian Bug#796798\n DEBPKG:fixes/memoize-pod - [rt.cpan.org #89441] Fix POD errors in Memoize\n DEBPKG:debian/hurd-softupdates - https://bugs.debian.org/822735 Fix t/op/stat.t failures on hurd\n DEBPKG:fixes/math_complex_doc_great_circle - https://bugs.debian.org/697567 [rt.cpan.org #114104] Math::Trig: clarify definition of great_circle_midpoint\n DEBPKG:fixes/math_complex_doc_see_also - https://bugs.debian.org/697568 [rt.cpan.org #114105] Math::Trig: add missing SEE ALSO\n DEBPKG:fixes/math_complex_doc_angle_units - https://bugs.debian.org/731505 [rt.cpan.org #114106] Math::Trig: document angle units\n DEBPKG:fixes/cpan_web_link - https://bugs.debian.org/367291 CPAN: Add link to main CPAN web site\n DEBPKG:debian/hppa_op_optimize_workaround - https://bugs.debian.org/838613 Temporarily lower the optimization of op.c on hppa due to gcc-6 problems\n DEBPKG:debian/installman-utf8 - https://bugs.debian.org/840211 Generate man pages with UTF-8 characters\n DEBPKG:debian/hppa_opmini_optimize_workaround - https://bugs.debian.org/869122 Lower the optimization level of opmini.c on hppa\n DEBPKG:debian/sh4_op_optimize_workaround - https://bugs.debian.org/869373 Also lower the optimization level of op.c and opmini.c on sh4\n DEBPKG:debian/perldoc-pager - https://bugs.debian.org/870340 [rt.cpan.org #120229] Fix perldoc terminal escapes when sensible-pager is less\n DEBPKG:debian/prune_libs - https://bugs.debian.org/128355 Prune the list of libraries wanted to what we actually need.\n DEBPKG:debian/mod_paths - Tweak @INC ordering for Debian\n DEBPKG:debian/deprecate-with-apt - https://bugs.debian.org/747628 Point users to Debian packages of deprecated core modules\n DEBPKG:debian/disable-stack-check - https://bugs.debian.org/902779 [GH #16607] Disable debugperl stack extension checks for binary compatibility with perl\n DEBPKG:debian/perlbug-editor - https://bugs.debian.org/922609 Use \"editor\" as the default perlbug editor, as per Debian policy\n DEBPKG:debian/eu-mm-perl-base - https://bugs.debian.org/962138 Suppress an ExtUtils::MakeMaker warning about our non-default @INC\n DEBPKG:fixes/io_socket_ip_ipv6 - Disable getaddrinfo(3) AI_ADDRCONFIG for localhost and IPv4 numeric addresses\n DEBPKG:debian/usrmerge-lib64 - https://bugs.debian.org/914128 Configure / libpth.U: Do not adjust glibpth when /usr/lib64 is present.\n DEBPKG:debian/usrmerge-realpath - https://bugs.debian.org/914128 Configure / libpth.U: use realpath --no-symlinks on Debian\n DEBPKG:debian/configure-regen - https://bugs.debian.org/762638 Regenerate Configure et al. after probe unit changes\n DEBPKG:fixes/x32-io-msg-skip - https://bugs.debian.org/922609 Skip io/msg.t on x32 due to broken System V message queues\n DEBPKG:debian/hurd-eumm-workaround - https://bugs.debian.org/1018289 Work around a MakeMaker regression breaking GNU/Hurd hint files\n DEBPKG:fixes/json-pp-warnings - https://bugs.debian.org/1019757 Call unimport first to silence warnings\n DEBPKG:fixes/readline-stream-errors - [80c1f1e] [GH #6799] https://bugs.debian.org/1016369 only clear the stream error state in readline() for glob()\n DEBPKG:fixes/readline-stream-errors-test - [0b60216] [GH #6799] https://bugs.debian.org/1016369 test that <> doesn't clear the stream error state\n DEBPKG:fixes/lto-test-fix - [69b4fa3] [GH #20518] https://bugs.debian.org/1015579 skip checking categorization of libperl symbols for LTO builds\n Built under linux\n Compiled at Sep 9 2023 16:19:46\n @INC:\n /etc/perl\n /usr/local/lib/x86_64-linux-gnu/perl/5.36.0\n /usr/local/share/perl/5.36.0\n /usr/lib/x86_64-linux-gnu/perl5/5.36\n /usr/share/perl5\n /usr/lib/x86_64-linux-gnu/perl-base\n /usr/lib/x86_64-linux-gnu/perl/5.36\n /usr/share/perl/5.36\n /usr/local/lib/site_perl\n```\n\nWhat are yours?\n\nFor the sendemail-validate hook itself, I managed to trigger this regression\nwith simple helloworld script:\n\n```\n#!/bin/bash\n\necho \"patching...\" && exit 0\n```\n\nThanks.\n\n-- \nAn old man doll... just what I always wanted! - Clara\n"},{"id":"482239","messageId":"20230925080010.GA1534025@coredump.intra.peff.net","threadId":"60255","inReplyTo":"ZRE6q8dHPFRIQezX@debian.me","subject":"Re: [REGRESSION] uninitialized value $address in git send-email when given multiple recipients separated by commas","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-09-25T08:00:10Z","receivedAt":"2023-09-25T08:00:21Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Sep 25, 2023 at 02:45:47PM +0700, Bagas Sanjaya wrote:\n\n> On Sat, Sep 23, 2023 at 11:36:25PM -0400, Jeff King wrote:\n> > Your report also mentions a validation hook, so I tried installing one\n> > like:\n> > \n> > \tcat >.git/hooks/sendemail-validate <<-\\EOF\n> > \t#!/bin/sh\n> > \techo >&2 running validate hook\n> > \texit 0\n> > \tEOF\n> > \tchmod +x .git/hooks/sendemail-validate\n> > \n> > and confirmed that the hook runs (three times, as expected). But still\n> > no error. I'm using v2.41.0 to test against.\n> > \n> \n> Hi Jeff,\n> \n> I think you missed perl version. As stated earlier, I'm on Debian testing\n> with perl v5.36.0. On there, `perl -V` outputs:\n\nMine is the same (I'm on Debian unstable, but the version is currently\nthe same as the one on testing).\n\n> For the sendemail-validate hook itself, I managed to trigger this regression\n> with simple helloworld script:\n> \n> ```\n> #!/bin/bash\n> \n> echo \"patching...\" && exit 0\n> ```\n\nI think that's equivalent to what I was using (and certainly using yours\nverbatim does not change anything on my end).\n\nDo you have any other send-email related config? Can you show us the\noutput of \"git config --list\"?\n\n-Peff\n"},{"id":"482270","messageId":"ZRGdvRQuj4zllGnm@pobox.com","threadId":"60255","inReplyTo":"20230925080010.GA1534025@coredump.intra.peff.net","subject":"Re: [REGRESSION] uninitialized value $address in git send-email when given multiple recipients separated by commas","fromName":"Todd Zullinger","fromEmail":"tmz@pobox.com","sentAt":"2023-09-25T14:48:29Z","receivedAt":"2023-09-25T14:48:42Z","isPatch":false,"sender":{"key":"tmz@pobox.com","avatar":"https://avatars.githubusercontent.com/u/806319?v=4"},"body":"Hi,\n\nJeff King wrote:\n> On Mon, Sep 25, 2023 at 02:45:47PM +0700, Bagas Sanjaya wrote:\n>> I think you missed perl version. As stated earlier, I'm on Debian testing\n>> with perl v5.36.0. On there, `perl -V` outputs:\n> \n> Mine is the same (I'm on Debian unstable, but the version is currently\n> the same as the one on testing).\n[...]\n> Do you have any other send-email related config? Can you show us the\n> output of \"git config --list\"?\n\nFrom the peanut gallery... could the presence or lack of the\nEmail::Valid perl module be a factor?\n\n-- \nTodd\n"},{"id":"482286","messageId":"20230925161748.GA2149383@coredump.intra.peff.net","threadId":"60255","inReplyTo":"ZRGdvRQuj4zllGnm@pobox.com","subject":"Re: [REGRESSION] uninitialized value $address in git send-email when given multiple recipients separated by commas","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-09-25T16:17:48Z","receivedAt":"2023-09-25T16:17:53Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Sep 25, 2023 at 10:48:29AM -0400, Todd Zullinger wrote:\n\n> From the peanut gallery... could the presence or lack of the\n> Email::Valid perl module be a factor?\n\nAh, thanks! The thought of differing modules even occurred to me, since\nI know we have a few optimistic dependencies, but when I looked I didn't\nmanage to find that one (somehow I thought Mail::Address was the\ninteresting one here; I think I might be getting senile).\n\nWith Email::Valid installed, I can reproduce with just (in git.git, but\nI think it would work in any repo):\n\n $ echo \"exit 0\" >.git/hooks/sendemail-validate\n $ chmod +x .git/hooks/sendemail-validate\n $ git send-email --dry-run -1 --to=foo@example.com,bar@example.com\n error: unable to extract a valid address from: foo@example.com,bar@example.com\n\nDisabling the hook with \"chmod -x\" makes the problem go away (and this\nis with current \"master\", hence the more readable error message).\n\nI think the issue is that a8022c5f7b ends up in extract_valid_address()\nvia this call stack:\n\n $ = main::extract_valid_address('foo@example.com,bar@example.com') called from file '/home/peff/compile/git/git-send-email' line 1161\n $ = main::extract_valid_address_or_die('foo@example.com,bar@example.com') called from file '/home/peff/compile/git/git-send-email' line 2087\n @ = main::unique_email_list('foo@example.com,bar@example.com') called from file '/home/peff/compile/git/git-send-email' line 1507\n @ = main::gen_header() called from file '/home/peff/compile/git/git-send-email' line 2113\n . = main::validate_patch('/tmp/WfoPQSKCUa/0001-The-twelfth-batch.patch', 'auto') called from file '/home/peff/compile/git/git-send-email' line 815\n\nwhereas prior to that commit, we hit it later:\n\n $ = main::extract_valid_address('foo@example.com') called from file '/home/peff/compile/git/git-send-email' line 1166\n @ = main::validate_address('foo@example.com') called from file '/home/peff/compile/git/git-send-email' line 1189\n @ = main::validate_address_list('foo@example.com', 'bar@example.com') called from file '/home/peff/compile/git/git-send-email' line 1348\n @ = main::process_address_list('foo@example.com,bar@example.com') called from file '/home/peff/compile/git/git-send-email' line 1091\n\nSo the issue is the call to gen_header() added in validate_patch(). We\nwon't yet have processed the address lists by that point. We can move\nthose calls up, but it requires moving a bit of extra code, too (like\nthe parts prompting for the \"to\" list if it isn't filled in).\n\nPossibly the validation checks need to be moved down, if they want to\nsee a more complete view of the emails. But now we're doing more work\n(like asking the user to write the cover letter!) before we do\nvalidation, which is probably bad.\n\nSo I dunno. Maybe gen_header() should be lazily doing this\nprocess_address_list() stuff? I'm not very familiar with the send-email\ncode, so I'm not sure what secondary effects that could have.\n\n-Peff\n"},{"id":"482334","messageId":"f5c6a72b-f888-4d43-8be8-3ce2c878c669@gmail.com","threadId":"60255","inReplyTo":"20230925080010.GA1534025@coredump.intra.peff.net","subject":"Re: [REGRESSION] uninitialized value $address in git send-email when given multiple recipients separated by commas","fromName":"Bagas Sanjaya","fromEmail":"bagasdotme@gmail.com","sentAt":"2023-09-26T11:33:02Z","receivedAt":"2023-09-26T11:33:18Z","isPatch":false,"sender":{"key":"bagasdotme@gmail.com","avatar":"https://avatars.githubusercontent.com/u/40219486?v=4"},"body":"On 25/09/2023 15:00, Jeff King wrote:\n> Do you have any other send-email related config? Can you show us the\n> output of \"git config --list\"?\n> \n\nHi Jeff, sure here is the output from my git.git clone:\n\n```\n$ git config --list\ncore.abbrev=10\nalias.am-failed=am --show-current-patch=diff\nalias.fixwhat=show -s --pretty=fixes\nuser.name=Bagas Sanjaya\nuser.email=bagasdotme@gmail.com\nuser.signingkey=D0A10F5A447A37A42A63419FD7B55A665D5C863D\nformat.signature=An old man doll... just what I always wanted! - Clara\ndiff.algorithm=histogram\nsendemail.smtpencryption=tls\nsendemail.smtpserver=smtp.gmail.com\nsendemail.smtpserverport=587\nsendemail.smtpuser=bagasdotme@gmail.com\nsendemail.smtppass=\npack.deltacachesize=225M\npack.window=13\npack.windowmemory=460M\npack.threads=2\npretty.fixes=Fixes: %h (\"%s\")\npretty.fixup=fixup for \"%s\"\npretty.kreference=%h (\"%s\")\npretty.upstream=commit %H upstream.\npretty.upstreamsasha=[ Upstream commit %H ]\nmerge.conflictstyle=diff3\ntar.xz.command=xz -c\ntar.zst.command=zstd -c\ndiff.algorithm=histogram\nfilter.lfs.clean=git-lfs clean -- %f\nfilter.lfs.smudge=git-lfs smudge -- %f\nfilter.lfs.process=git-lfs filter-process\nfilter.lfs.required=true\ncore.repositoryformatversion=0\ncore.filemode=true\ncore.bare=false\ncore.logallrefupdates=true\ntag.sort=creatordate\nremote.origin.url=https://git.kernel.org/pub/scm/git/git.git\nremote.origin.fetch=+refs/heads/master:refs/remotes/origin/master\nremote.origin.fetch=+refs/heads/main:refs/remotes/origin/main\nremote.origin.fetch=+refs/heads/next:refs/remotes/origin/next\nremote.origin.fetch=+refs/heads/seen:refs/remotes/origin/seen\nremote.origin.fetch=+refs/heads/maint:refs/remotes/origin/maint\nremote.origin.fetch=+refs/heads/todo:refs/remotes/origin/todo\nsendemail.sendmailcmd=/usr/sbin/sendmail\nsendemail.envelopesender=auto\nbranch.master.remote=origin\nbranch.master.merge=refs/heads/master\nbranch.next.remote=origin\nbranch.next.merge=refs/heads/next\nbranch.seen.remote=origin\nbranch.seen.merge=refs/heads/seen\nbranch.maint.remote=origin\nbranch.maint.merge=refs/heads/maint\nbranch.todo.remote=origin\nbranch.todo.merge=refs/heads/todo\nsendemail.sendmailcmd=/usr/sbin/sendmail\nsendemail.envelopesender=auto\nbranch.main.remote=origin\nbranch.main.merge=refs/heads/main\nremote.gitnode.url=git@gitnode.io:bagas/git.git\nremote.gitnode.fetch=+refs/heads/*:refs/remotes/gitnode/*\n```\n\nThanks.\n\n-- \nAn old man doll... just what I always wanted! - Clara\n\n"},{"id":"483045","messageId":"ZSal-mQIZAUBaq6g@debian.me","threadId":"60255","inReplyTo":"20230925161748.GA2149383@coredump.intra.peff.net","subject":"Re: [REGRESSION] uninitialized value $address in git send-email when given multiple recipients separated by commas","fromName":"Bagas Sanjaya","fromEmail":"bagasdotme@gmail.com","sentAt":"2023-10-11T13:41:14Z","receivedAt":"2023-10-11T13:41:40Z","isPatch":false,"sender":{"key":"bagasdotme@gmail.com","avatar":"https://avatars.githubusercontent.com/u/40219486?v=4"},"body":"On Mon, Sep 25, 2023 at 12:17:48PM -0400, Jeff King wrote:\n> On Mon, Sep 25, 2023 at 10:48:29AM -0400, Todd Zullinger wrote:\n> \n> > From the peanut gallery... could the presence or lack of the\n> > Email::Valid perl module be a factor?\n> \n> Ah, thanks! The thought of differing modules even occurred to me, since\n> I know we have a few optimistic dependencies, but when I looked I didn't\n> manage to find that one (somehow I thought Mail::Address was the\n> interesting one here; I think I might be getting senile).\n> \n> With Email::Valid installed, I can reproduce with just (in git.git, but\n> I think it would work in any repo):\n> \n> $ echo \"exit 0\" >.git/hooks/sendemail-validate\n> $ chmod +x .git/hooks/sendemail-validate\n> $ git send-email --dry-run -1 --to=foo@example.com,bar@example.com\n> error: unable to extract a valid address from: foo@example.com,bar@example.com\n> \n> Disabling the hook with \"chmod -x\" makes the problem go away (and this\n> is with current \"master\", hence the more readable error message).\n> \n> I think the issue is that a8022c5f7b ends up in extract_valid_address()\n> via this call stack:\n> \n> $ = main::extract_valid_address('foo@example.com,bar@example.com') called from file '/home/peff/compile/git/git-send-email' line 1161\n> $ = main::extract_valid_address_or_die('foo@example.com,bar@example.com') called from file '/home/peff/compile/git/git-send-email' line 2087\n> @ = main::unique_email_list('foo@example.com,bar@example.com') called from file '/home/peff/compile/git/git-send-email' line 1507\n> @ = main::gen_header() called from file '/home/peff/compile/git/git-send-email' line 2113\n> . = main::validate_patch('/tmp/WfoPQSKCUa/0001-The-twelfth-batch.patch', 'auto') called from file '/home/peff/compile/git/git-send-email' line 815\n> \n> whereas prior to that commit, we hit it later:\n> \n> $ = main::extract_valid_address('foo@example.com') called from file '/home/peff/compile/git/git-send-email' line 1166\n> @ = main::validate_address('foo@example.com') called from file '/home/peff/compile/git/git-send-email' line 1189\n> @ = main::validate_address_list('foo@example.com', 'bar@example.com') called from file '/home/peff/compile/git/git-send-email' line 1348\n> @ = main::process_address_list('foo@example.com,bar@example.com') called from file '/home/peff/compile/git/git-send-email' line 1091\n> \n> So the issue is the call to gen_header() added in validate_patch(). We\n> won't yet have processed the address lists by that point. We can move\n> those calls up, but it requires moving a bit of extra code, too (like\n> the parts prompting for the \"to\" list if it isn't filled in).\n> \n> Possibly the validation checks need to be moved down, if they want to\n> see a more complete view of the emails. But now we're doing more work\n> (like asking the user to write the cover letter!) before we do\n> validation, which is probably bad.\n> \n> So I dunno. Maybe gen_header() should be lazily doing this\n> process_address_list() stuff? I'm not very familiar with the send-email\n> code, so I'm not sure what secondary effects that could have.\n> \n\nMichael, did you look into this since you authored the culprit?\n\n-- \nAn old man doll... just what I always wanted! - Clara\n"},{"id":"483081","messageId":"95b9e5d5-ab07-48a6-b972-af5348f653be@amd.com","threadId":"60255","inReplyTo":"ZSal-mQIZAUBaq6g@debian.me","subject":"Re: [REGRESSION] uninitialized value $address in git send-email when given multiple recipients separated by commas","fromName":"Michael Strawbridge","fromEmail":"michael.strawbridge@amd.com","sentAt":"2023-10-11T19:27:51Z","receivedAt":"2023-10-11T19:28:08Z","isPatch":false,"sender":{"key":"michael.strawbridge@amd.com","avatar":null},"body":"\nOn 10/11/23 09:41, Bagas Sanjaya wrote:\n> On Mon, Sep 25, 2023 at 12:17:48PM -0400, Jeff King wrote:\n>> On Mon, Sep 25, 2023 at 10:48:29AM -0400, Todd Zullinger wrote:\n>>\n>>> From the peanut gallery... could the presence or lack of the\n>>> Email::Valid perl module be a factor?\n>> Ah, thanks! The thought of differing modules even occurred to me, since\n>> I know we have a few optimistic dependencies, but when I looked I didn't\n>> manage to find that one (somehow I thought Mail::Address was the\n>> interesting one here; I think I might be getting senile).\n>>\n>> With Email::Valid installed, I can reproduce with just (in git.git, but\n>> I think it would work in any repo):\n>>\n>> $ echo \"exit 0\" >.git/hooks/sendemail-validate\n>> $ chmod +x .git/hooks/sendemail-validate\n>> $ git send-email --dry-run -1 --to=foo@example.com,bar@example.com\n>> error: unable to extract a valid address from: foo@example.com,bar@example.com\n>>\n>> Disabling the hook with \"chmod -x\" makes the problem go away (and this\n>> is with current \"master\", hence the more readable error message).\n>>\n>> I think the issue is that a8022c5f7b ends up in extract_valid_address()\n>> via this call stack:\n>>\n>> $ = main::extract_valid_address('foo@example.com,bar@example.com') called from file '/home/peff/compile/git/git-send-email' line 1161\n>> $ = main::extract_valid_address_or_die('foo@example.com,bar@example.com') called from file '/home/peff/compile/git/git-send-email' line 2087\n>> @ = main::unique_email_list('foo@example.com,bar@example.com') called from file '/home/peff/compile/git/git-send-email' line 1507\n>> @ = main::gen_header() called from file '/home/peff/compile/git/git-send-email' line 2113\n>> . = main::validate_patch('/tmp/WfoPQSKCUa/0001-The-twelfth-batch.patch', 'auto') called from file '/home/peff/compile/git/git-send-email' line 815\n>>\n>> whereas prior to that commit, we hit it later:\n>>\n>> $ = main::extract_valid_address('foo@example.com') called from file '/home/peff/compile/git/git-send-email' line 1166\n>> @ = main::validate_address('foo@example.com') called from file '/home/peff/compile/git/git-send-email' line 1189\n>> @ = main::validate_address_list('foo@example.com', 'bar@example.com') called from file '/home/peff/compile/git/git-send-email' line 1348\n>> @ = main::process_address_list('foo@example.com,bar@example.com') called from file '/home/peff/compile/git/git-send-email' line 1091\n>>\n>> So the issue is the call to gen_header() added in validate_patch(). We\n>> won't yet have processed the address lists by that point. We can move\n>> those calls up, but it requires moving a bit of extra code, too (like\n>> the parts prompting for the \"to\" list if it isn't filled in).\n>>\n>> Possibly the validation checks need to be moved down, if they want to\n>> see a more complete view of the emails. But now we're doing more work\n>> (like asking the user to write the cover letter!) before we do\n>> validation, which is probably bad.\n>>\n>> So I dunno. Maybe gen_header() should be lazily doing this\n>> process_address_list() stuff? I'm not very familiar with the send-email\n>> code, so I'm not sure what secondary effects that could have.\n>>\n> Michael, did you look into this since you authored the culprit?\n>\nI tried to repro the issue previously but didn't have luck (even with Email::Valid installed). I decided to try again today and realised I forgot to make the test sendemail-validate hook executable before.  Now that I can repro it, I can look further.\n\nThe fix may not be easy for the reasons that Jeff King states.  There is a lot of implicit order in the code because there are several places where code exists outside of any function, which has function definitions scattered through it.  My change attempted to clean a portion of that up and encapsulated it into a reusable function for generating header information so that sendemail-validate could see it.  I'll keep digging into it.\n\n"},{"id":"483085","messageId":"7e2c92ff-b42c-4b3f-a509-9d0785448262@amd.com","threadId":"60255","inReplyTo":"95b9e5d5-ab07-48a6-b972-af5348f653be@amd.com","subject":"[PATCH] send-email: move process_address_list earlier to avoid, uninitialized address error","fromName":"Michael Strawbridge","fromEmail":"michael.strawbridge@amd.com","sentAt":"2023-10-11T20:22:18Z","receivedAt":"2023-10-11T20:22:31Z","isPatch":true,"sender":{"key":"michael.strawbridge@amd.com","avatar":null},"body":"Move processing of email address lists before the sendemail-validate\nhook code.  This fixes email address validation errors when the optional\nperl module Email::Valid is installed and multiple addresses are passed\nin on a single to/cc argument like --to=foo@example.com,bar@example.com.\n---\n git-send-email.perl | 8 ++++----\n 1 file changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/git-send-email.perl b/git-send-email.perl\nindex 288ea1ae80..cfd80c9d8b 100755\n--- a/git-send-email.perl\n+++ b/git-send-email.perl\n@@ -799,6 +799,10 @@ sub is_format_patch_arg {\n \n $time = time - scalar $#files;\n \n+@initial_to = process_address_list(@initial_to);\n+@initial_cc = process_address_list(@initial_cc);\n+@initial_bcc = process_address_list(@initial_bcc);\n+\n if ($validate) {\n        # FIFOs can only be read once, exclude them from validation.\n        my @real_files = ();\n@@ -1099,10 +1103,6 @@ sub expand_one_alias {\n        return $aliases{$alias} ? expand_aliases(@{$aliases{$alias}}) : $alias;\n }\n \n-@initial_to = process_address_list(@initial_to);\n-@initial_cc = process_address_list(@initial_cc);\n-@initial_bcc = process_address_list(@initial_bcc);\n-\n if ($thread && !defined $initial_in_reply_to && $prompting) {\n        $initial_in_reply_to = ask(\n                __(\"Message-ID to be used as In-Reply-To for the first email (if any)? \"),\n-- \n2.34.1\n"},{"id":"483086","messageId":"a62743b2-d50a-4138-bbd5-d70653dfaf09@amd.com","threadId":"60255","inReplyTo":"7e2c92ff-b42c-4b3f-a509-9d0785448262@amd.com","subject":"Re: [PATCH] send-email: move process_address_list earlier to avoid, uninitialized address error","fromName":"Michael Strawbridge","fromEmail":"michael.strawbridge@amd.com","sentAt":"2023-10-11T20:25:31Z","receivedAt":"2023-10-11T20:25:44Z","isPatch":true,"sender":{"key":"michael.strawbridge@amd.com","avatar":null},"body":"Hi Bagas,\n\nIf possible, please try the patch I just sent out and let me know if it works for your situation.\n\nThanks,\n\nMichael\n\nOn 10/11/23 16:22, Michael Strawbridge wrote:\n> Move processing of email address lists before the sendemail-validate\n> hook code.  This fixes email address validation errors when the optional\n> perl module Email::Valid is installed and multiple addresses are passed\n> in on a single to/cc argument like --to=foo@example.com,bar@example.com.\n> ---\n>  git-send-email.perl | 8 ++++----\n>  1 file changed, 4 insertions(+), 4 deletions(-)\n>\n> diff --git a/git-send-email.perl b/git-send-email.perl\n> index 288ea1ae80..cfd80c9d8b 100755\n> --- a/git-send-email.perl\n> +++ b/git-send-email.perl\n> @@ -799,6 +799,10 @@ sub is_format_patch_arg {\n>  \n>  $time = time - scalar $#files;\n>  \n> +@initial_to = process_address_list(@initial_to);\n> +@initial_cc = process_address_list(@initial_cc);\n> +@initial_bcc = process_address_list(@initial_bcc);\n> +\n>  if ($validate) {\n>         # FIFOs can only be read once, exclude them from validation.\n>         my @real_files = ();\n> @@ -1099,10 +1103,6 @@ sub expand_one_alias {\n>         return $aliases{$alias} ? expand_aliases(@{$aliases{$alias}}) : $alias;\n>  }\n>  \n> -@initial_to = process_address_list(@initial_to);\n> -@initial_cc = process_address_list(@initial_cc);\n> -@initial_bcc = process_address_list(@initial_bcc);\n> -\n>  if ($thread && !defined $initial_in_reply_to && $prompting) {\n>         $initial_in_reply_to = ask(\n>                 __(\"Message-ID to be used as In-Reply-To for the first email (if any)? \"),\n"},{"id":"483090","messageId":"xmqq1qe0lui2.fsf@gitster.g","threadId":"60255","inReplyTo":"7e2c92ff-b42c-4b3f-a509-9d0785448262@amd.com","subject":"Re: [PATCH] send-email: move process_address_list earlier to avoid, uninitialized address error","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-10-11T21:27:49Z","receivedAt":"2023-10-11T21:27:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Strawbridge writes:\n\n> @@ -799,6 +799,10 @@ sub is_format_patch_arg {\n> \n> $time = time - scalar $#files;\n> \n> +@initial_to = process_address_list(@initial_to);\n> +@initial_cc = process_address_list(@initial_cc);\n> +@initial_bcc = process_address_list(@initial_bcc);\n> +\n\nThis does not look OK. If we trace how @initial_to gets its value,\n\n - it first gets its value from @getopt_to and @config_to\n\n - if that is empty, and there is no $to_cmd, the end-user is\n interactively asked.\n\n - then process_address_list() is applied.\n\nBut this patch just swapped the second one and the third one, so\nprocess_address_list() does not process what the end-user gave\ninteractively, no?\n\n> if ($validate) {\n> # FIFOs can only be read once, exclude them from validation.\n> my @real_files = ();\n\nIt almost feels like what need to move is not the setting of these\naddress lists, but the code that calls int validation callchain that\nneeds access to these address lists---the block that begins with the\nabove \"if ($validate) {\" needs to move below ...\n\n> @@ -1099,10 +1103,6 @@ sub expand_one_alias {\n> return $aliases{$alias} ? expand_aliases(@{$aliases{$alias}}) : $alias;\n> }\n> \n> -@initial_to = process_address_list(@initial_to);\n> -@initial_cc = process_address_list(@initial_cc);\n> -@initial_bcc = process_address_list(@initial_bcc);\n> -\n\n... this point, or something, perhaps?\n\n> if ($thread && !defined $initial_in_reply_to && $prompting) {\n> $initial_in_reply_to = ask(\n> __(\"Message-ID to be used as In-Reply-To for the first email (if any)? \"),\n"},{"id":"483096","messageId":"20231011221844.GB518221@coredump.intra.peff.net","threadId":"60255","inReplyTo":"xmqq1qe0lui2.fsf@gitster.g","subject":"Re: [PATCH] send-email: move process_address_list earlier to avoid, uninitialized address error","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-10-11T22:18:44Z","receivedAt":"2023-10-11T22:18:49Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Oct 11, 2023 at 02:27:49PM -0700, Junio C Hamano wrote:\n\n> Michael Strawbridge writes:\n> \n> > @@ -799,6 +799,10 @@ sub is_format_patch_arg {\n> > \n> > $time = time - scalar $#files;\n> > \n> > +@initial_to = process_address_list(@initial_to);\n> > +@initial_cc = process_address_list(@initial_cc);\n> > +@initial_bcc = process_address_list(@initial_bcc);\n> > +\n> \n> This does not look OK. If we trace how @initial_to gets its value,\n> \n> - it first gets its value from @getopt_to and @config_to\n> \n> - if that is empty, and there is no $to_cmd, the end-user is\n> interactively asked.\n> \n> - then process_address_list() is applied.\n> \n> But this patch just swapped the second one and the third one, so\n> process_address_list() does not process what the end-user gave\n> interactively, no?\n\nYep, that is the issue I found when I dug into this earlier.\n\n> > if ($validate) {\n> > # FIFOs can only be read once, exclude them from validation.\n> > my @real_files = ();\n> \n> It almost feels like what need to move is not the setting of these\n> address lists, but the code that calls int validation callchain that\n> needs access to these address lists---the block that begins with the\n> above \"if ($validate) {\" needs to move below ...\n\nYes, though then we have the problem that we've asked the user some\ninteractive questions before validating if the input files are bogus or\nnot. Which would be annoying if they aren't valid, because when we barf\nthey've wasted time typing.\n\nWhich of course implies that we're not (and cannot) validate what\nthey're typing at this step, but I think that's OK because we feed it\nthrough extract_valid_address_or_die(). IOW, I think there are actually\ntwo distinct validation steps hidden here:\n\n 1. We want to validate that the patch files we were fed are OK.\n\n 2. We want to validate that the addresses, etc, fed by the user are\n OK.\n\nAnd after Michael's original patch, we are accidentally hitting some of\nthat validation code for (2) while doing (1).\n\nThis is actually a weird split if you think about it. We are feeding to\nthe validate hook in (1), so surely it would want to see the full set of\ninputs from the user, too? Which argues for pushing the \"if ($validate)\"\ndown as you suggest. And then either:\n\n a. We accept that the user experience is a little worse if validation\n fails after the user typed.\n\n b. We split (1) into \"early\" validation that just checks if the files\n are OK, but doesn't call the hook. And then later on we do the full\n validation.\n\nI don't have a strong opinion myself (I don't even use send-email\nmyself, and if I did, I'd probably mostly be feeding it with \"--to\" etc\non the command line, rather than interactively).\n\n-Peff\n"},{"id":"483104","messageId":"xmqqzg0oiy4s.fsf@gitster.g","threadId":"60255","inReplyTo":"20231011221844.GB518221@coredump.intra.peff.net","subject":"Re: [PATCH] send-email: move process_address_list earlier to avoid, uninitialized address error","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-10-11T22:37:39Z","receivedAt":"2023-10-11T22:37:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Which of course implies that we're not (and cannot) validate what\n> they're typing at this step, but I think that's OK because we feed it\n> through extract_valid_address_or_die().\n\nOK, let's queue it then.\n\n> IOW, I think there are actually two distinct validation steps\n> hidden here:\n>\n> 1. We want to validate that the patch files we were fed are OK.\n>\n> 2. We want to validate that the addresses, etc, fed by the user are\n> OK.\n>\n> And after Michael's original patch, we are accidentally hitting some of\n> that validation code for (2) while doing (1).\n\n> This is actually a weird split if you think about it. We are feeding to\n> the validate hook in (1), so surely it would want to see the full set of\n> inputs from the user, too? Which argues for pushing the \"if ($validate)\"\n> down as you suggest. And then either:\n>\n> a. We accept that the user experience is a little worse if validation\n> fails after the user typed.\n>\n> b. We split (1) into \"early\" validation that just checks if the files\n> are OK, but doesn't call the hook. And then later on we do the full\n> validation.\n>\n> I don't have a strong opinion myself (I don't even use send-email\n> myself, and if I did, I'd probably mostly be feeding it with \"--to\" etc\n> on the command line, rather than interactively).\n\nI am not affected, either, and do not have a strong opinion either\nway. As long as the end-user input is validated separately, it\nwould be OK, but if the end-user supplied validation hook cares\nabout what addresses the messages are going to be sent to, not\nknowing the set of recipients mean the validation hook is not\ngetting the whole picture, which does smell bad.\n\nOn the other hand, I am not sure what is wrong with \"after the user\ntyped\", actually. As you said, anybody sane would be using --to (or\nan equivalent configuration variable in the repository) to send\ntheir patches to the project address instead of typing, and to them\nit is not a problem. After getting the recipient address from the\nend user, the validation may fail due to a wrong address, in which\ncase it is a good thing. If the validation failed due to wrong\ncontents of the patch (perhaps it included a change to the file with\ntrade secret that appeared in the context lines), as long as the\nreason why the validation hook rejected the patches is clear enough\n(e.g., \"it's the patches, not the recipients\"), such \"a rejection\nafter typing\" would be only once per a patch series, so it does not\nsound too bad, either.\n\nBut perhaps I am not seeing the reason why \"fail after the user typed\"\nis so disliked and being unnecessarily unsympathetic. I dunno.\n\n"},{"id":"483106","messageId":"20231011224753.GE518221@coredump.intra.peff.net","threadId":"60255","inReplyTo":"xmqqzg0oiy4s.fsf@gitster.g","subject":"Re: [PATCH] send-email: move process_address_list earlier to avoid, uninitialized address error","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-10-11T22:47:53Z","receivedAt":"2023-10-11T22:47:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Oct 11, 2023 at 03:37:39PM -0700, Junio C Hamano wrote:\n\n> On the other hand, I am not sure what is wrong with \"after the user\n> typed\", actually. As you said, anybody sane would be using --to (or\n> an equivalent configuration variable in the repository) to send\n> their patches to the project address instead of typing, and to them\n> it is not a problem. After getting the recipient address from the\n> end user, the validation may fail due to a wrong address, in which\n> case it is a good thing. If the validation failed due to wrong\n> contents of the patch (perhaps it included a change to the file with\n> trade secret that appeared in the context lines), as long as the\n> reason why the validation hook rejected the patches is clear enough\n> (e.g., \"it's the patches, not the recipients\"), such \"a rejection\n> after typing\" would be only once per a patch series, so it does not\n> sound too bad, either.\n> \n> But perhaps I am not seeing the reason why \"fail after the user typed\"\n> is so disliked and being unnecessarily unsympathetic. I dunno.\n\nI did not look carefully at the flow of send-email, so this may or may\nnot be an issue. But what I think would be _really_ annoying is if you\nasked to write a cover letter, went through the trouble of writing it,\nand then send-email bailed due to some validation failure that could\nhave been checked earlier.\n\nThere is probably a way to recover your work (presumably we leave it in\na temporary file somewhere), but it may not be entirely trivial,\nespecially for users who are not comfortable with advanced usage of\ntheir editor. ;)\n\nI seem to remember we had one or two such problems in the early days\nwith \"git commit\", where you would go to the trouble to type a commit\nmessage only to bail on some condition which _could_ have been checked\nearlier. You can recover the message from .git/COMMIT_EDITMSG, but you\nneed to remember to do so before re-invoking \"git commit\", otherwise it\ngets obliterated.\n\nNow for send-email, if your flow is to generate the patches with\n\"format-patch\", then edit the cover letter separately, and then finally\nship it all out with \"send-email\", that might not be an issue. But some\nworkflows use the --compose option instead.\n\n-Peff\n"},{"id":"483230","messageId":"b4385543-bee0-473b-ab2d-df0d7847ddf3@amd.com","threadId":"60255","inReplyTo":"20231011224753.GE518221@coredump.intra.peff.net","subject":"Re: [PATCH] send-email: move process_address_list earlier to avoid, uninitialized address error","fromName":"Michael Strawbridge","fromEmail":"michael.strawbridge@amd.com","sentAt":"2023-10-13T20:25:57Z","receivedAt":"2023-10-13T20:26:07Z","isPatch":true,"sender":{"key":"michael.strawbridge@amd.com","avatar":null},"body":"\nOn 10/11/23 18:47, Jeff King wrote:\n> On Wed, Oct 11, 2023 at 03:37:39PM -0700, Junio C Hamano wrote:\n>\n>> On the other hand, I am not sure what is wrong with \"after the user\n>> typed\", actually. As you said, anybody sane would be using --to (or\n>> an equivalent configuration variable in the repository) to send\n>> their patches to the project address instead of typing, and to them\n>> it is not a problem. After getting the recipient address from the\n>> end user, the validation may fail due to a wrong address, in which\n>> case it is a good thing. If the validation failed due to wrong\n>> contents of the patch (perhaps it included a change to the file with\n>> trade secret that appeared in the context lines), as long as the\n>> reason why the validation hook rejected the patches is clear enough\n>> (e.g., \"it's the patches, not the recipients\"), such \"a rejection\n>> after typing\" would be only once per a patch series, so it does not\n>> sound too bad, either.\n>>\n>> But perhaps I am not seeing the reason why \"fail after the user typed\"\n>> is so disliked and being unnecessarily unsympathetic. I dunno.\n> I did not look carefully at the flow of send-email, so this may or may\n> not be an issue. But what I think would be _really_ annoying is if you\n> asked to write a cover letter, went through the trouble of writing it,\n> and then send-email bailed due to some validation failure that could\n> have been checked earlier.\n>\n> There is probably a way to recover your work (presumably we leave it in\n> a temporary file somewhere), but it may not be entirely trivial,\n> especially for users who are not comfortable with advanced usage of\n> their editor. ;)\nAs I was looking at covering the case of interactive input (--compose) to the fix I noticed that this seems to be at least partly handled by the $compose_filename code.  There is a nice output message telling you exactly where the intermediate version of the email you are composing is located if there are errors.  I took a quick look inside and can verify that any lost work should be minimal as long as someone knows how to edit files with their editor of choice.\n>\n> I seem to remember we had one or two such problems in the early days\n> with \"git commit\", where you would go to the trouble to type a commit\n> message only to bail on some condition which _could_ have been checked\n> earlier. You can recover the message from .git/COMMIT_EDITMSG, but you\n> need to remember to do so before re-invoking \"git commit\", otherwise it\n> gets obliterated.\n>\n> Now for send-email, if your flow is to generate the patches with\n> \"format-patch\", then edit the cover letter separately, and then finally\n> ship it all out with \"send-email\", that might not be an issue. But some\n> workflows use the --compose option instead.\n>\n> -Peff\nI have been looking into handling the interactive input cases while solving this issue, but have yet to make a breakthrough.  Simply moving the validation code below the original process_address_list code results in a a scenario where I get the email address being seen as something like \"ARRAY (0x55ddb951d768)\" rather than the email address I wrote in the compose buffer.\n"},{"id":"483538","messageId":"ZTHq4GHpeGq7D7zZ@debian.me","threadId":"60255","inReplyTo":"7e2c92ff-b42c-4b3f-a509-9d0785448262@amd.com","subject":"Re: [PATCH] send-email: move process_address_list earlier to avoid, uninitialized address error","fromName":"Bagas Sanjaya","fromEmail":"bagasdotme@gmail.com","sentAt":"2023-10-20T02:50:08Z","receivedAt":"2023-10-20T02:50:18Z","isPatch":true,"sender":{"key":"bagasdotme@gmail.com","avatar":"https://avatars.githubusercontent.com/u/40219486?v=4"},"body":"On Wed, Oct 11, 2023 at 04:22:18PM -0400, Michael Strawbridge wrote:\n> Move processing of email address lists before the sendemail-validate\n> hook code.  This fixes email address validation errors when the optional\n> perl module Email::Valid is installed and multiple addresses are passed\n> in on a single to/cc argument like --to=foo@example.com,bar@example.com.\n> ---\n>  git-send-email.perl | 8 ++++----\n>  1 file changed, 4 insertions(+), 4 deletions(-)\n> \n> diff --git a/git-send-email.perl b/git-send-email.perl\n> index 288ea1ae80..cfd80c9d8b 100755\n> --- a/git-send-email.perl\n> +++ b/git-send-email.perl\n> @@ -799,6 +799,10 @@ sub is_format_patch_arg {\n>  \n>  $time = time - scalar $#files;\n>  \n> +@initial_to = process_address_list(@initial_to);\n> +@initial_cc = process_address_list(@initial_cc);\n> +@initial_bcc = process_address_list(@initial_bcc);\n> +\n>  if ($validate) {\n>         # FIFOs can only be read once, exclude them from validation.\n>         my @real_files = ();\n> @@ -1099,10 +1103,6 @@ sub expand_one_alias {\n>         return $aliases{$alias} ? expand_aliases(@{$aliases{$alias}}) : $alias;\n>  }\n>  \n> -@initial_to = process_address_list(@initial_to);\n> -@initial_cc = process_address_list(@initial_cc);\n> -@initial_bcc = process_address_list(@initial_bcc);\n> -\n>  if ($thread && !defined $initial_in_reply_to && $prompting) {\n>         $initial_in_reply_to = ask(\n>                 __(\"Message-ID to be used as In-Reply-To for the first email (if any)? \"),\n\nThanks for the fixup! The patch itself is whitespace-damaged, though, so\nI have to manually apply it. Regardless,\n\nTested-by: Bagas Sanjaya \n\n-- \nAn old man doll... just what I always wanted! - Clara\n"},{"id":"483542","messageId":"20231020064525.GB1642714@coredump.intra.peff.net","threadId":"60255","inReplyTo":"b4385543-bee0-473b-ab2d-df0d7847ddf3@amd.com","subject":"Re: [PATCH] send-email: move process_address_list earlier to avoid, uninitialized address error","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-10-20T06:45:25Z","receivedAt":"2023-10-20T06:45:28Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Oct 13, 2023 at 04:25:57PM -0400, Michael Strawbridge wrote:\n\n> > I did not look carefully at the flow of send-email, so this may or may\n> > not be an issue. But what I think would be _really_ annoying is if you\n> > asked to write a cover letter, went through the trouble of writing it,\n> > and then send-email bailed due to some validation failure that could\n> > have been checked earlier.\n> >\n> > There is probably a way to recover your work (presumably we leave it in\n> > a temporary file somewhere), but it may not be entirely trivial,\n> > especially for users who are not comfortable with advanced usage of\n> > their editor. ;)\n>\n> As I was looking at covering the case of interactive input (--compose)\n> to the fix I noticed that this seems to be at least partly handled by\n> the $compose_filename code.  There is a nice output message telling\n> you exactly where the intermediate version of the email you are\n> composing is located if there are errors.  I took a quick look inside\n> and can verify that any lost work should be minimal as long as someone\n> knows how to edit files with their editor of choice.\n\nOK, that makes me feel better about just moving the validation code. A\nmore complicated solution could be do to do _some_ basic checks up\nfront, and then more complete validation later. But even if we wanted to\ndo that, moving the bulk of the validation (as discussed in this thread)\nwould probably be the first step anyway (and if nobody complains, maybe\nwe can avoid doing the extra work).\n\nI do wonder if we might find other interesting corner cases where\nthe validation code (or somebody's hook) isn't happy with seeing the\nmore \"full\" picture (i.e., with the extra addresses from interactive and\ncommand-line input). But arguably any such case would be indicative of a\nbug, and smoking it out would be a good thing.\n\n> I have been looking into handling the interactive input cases while\n> solving this issue, but have yet to make a breakthrough.  Simply\n> moving the validation code below the original process_address_list\n> code results in a a scenario where I get the email address being seen\n> as something like \"ARRAY (0x55ddb951d768)\" rather than the email\n> address I wrote in the compose buffer.\n\nSounds like something is making a perl ref that shouldn't (or something\nthat should be dereferencing it not doing so). If you post your patch\nand a reproduction command, I might be able to help debug.\n\nBut just blindly moving the validation code down, like:\n\ndiff --git a/git-send-email.perl b/git-send-email.perl\nindex 288ea1ae80..76589c7827 100755\n--- a/git-send-email.perl\n+++ b/git-send-email.perl\n@@ -799,30 +799,6 @@ sub is_format_patch_arg {\n \n $time = time - scalar $#files;\n \n-if ($validate) {\n-\t# FIFOs can only be read once, exclude them from validation.\n-\tmy @real_files = ();\n-\tforeach my $f (@files) {\n-\t\tunless (-p $f) {\n-\t\t\tpush(@real_files, $f);\n-\t\t}\n-\t}\n-\n-\t# Run the loop once again to avoid gaps in the counter due to FIFO\n-\t# arguments provided by the user.\n-\tmy $num = 1;\n-\tmy $num_files = scalar @real_files;\n-\t$ENV{GIT_SENDEMAIL_FILE_TOTAL} = \"$num_files\";\n-\tforeach my $r (@real_files) {\n-\t\t$ENV{GIT_SENDEMAIL_FILE_COUNTER} = \"$num\";\n-\t\tpre_process_file($r, 1);\n-\t\tvalidate_patch($r, $target_xfer_encoding);\n-\t\t$num += 1;\n-\t}\n-\tdelete $ENV{GIT_SENDEMAIL_FILE_COUNTER};\n-\tdelete $ENV{GIT_SENDEMAIL_FILE_TOTAL};\n-}\n-\n @files = handle_backup_files(@files);\n \n if (@files) {\n@@ -1121,6 +1097,30 @@ sub expand_one_alias {\n \t$reply_to = sanitize_address($reply_to);\n }\n \n+if ($validate) {\n+\t# FIFOs can only be read once, exclude them from validation.\n+\tmy @real_files = ();\n+\tforeach my $f (@files) {\n+\t\tunless (-p $f) {\n+\t\t\tpush(@real_files, $f);\n+\t\t}\n+\t}\n+\n+\t# Run the loop once again to avoid gaps in the counter due to FIFO\n+\t# arguments provided by the user.\n+\tmy $num = 1;\n+\tmy $num_files = scalar @real_files;\n+\t$ENV{GIT_SENDEMAIL_FILE_TOTAL} = \"$num_files\";\n+\tforeach my $r (@real_files) {\n+\t\t$ENV{GIT_SENDEMAIL_FILE_COUNTER} = \"$num\";\n+\t\tpre_process_file($r, 1);\n+\t\tvalidate_patch($r, $target_xfer_encoding);\n+\t\t$num += 1;\n+\t}\n+\tdelete $ENV{GIT_SENDEMAIL_FILE_COUNTER};\n+\tdelete $ENV{GIT_SENDEMAIL_FILE_TOTAL};\n+}\n+\n if (!defined $sendmail_cmd && !defined $smtp_server) {\n \tmy @sendmail_paths = qw( /usr/sbin/sendmail /usr/lib/sendmail );\n \tpush @sendmail_paths, map {\"$_/sendmail\"} split /:/, $ENV{PATH};\n\nseems to fix the problem from this thread and passes the existing tests.\nManually inspecting the result (and what's fed to the validation hook) I\ndon't see anything odd (like \"ARRAY (...)\").\n\n-Peff\n"},{"id":"483543","messageId":"20231020071402.GC1642714@coredump.intra.peff.net","threadId":"60255","inReplyTo":"20231020064525.GB1642714@coredump.intra.peff.net","subject":"Re: [PATCH] send-email: move process_address_list earlier to avoid, uninitialized address error","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-10-20T07:14:02Z","receivedAt":"2023-10-20T07:14:06Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Oct 20, 2023 at 02:45:25AM -0400, Jeff King wrote:\n\n> > I have been looking into handling the interactive input cases while\n> > solving this issue, but have yet to make a breakthrough.  Simply\n> > moving the validation code below the original process_address_list\n> > code results in a a scenario where I get the email address being seen\n> > as something like \"ARRAY (0x55ddb951d768)\" rather than the email\n> > address I wrote in the compose buffer.\n> \n> Sounds like something is making a perl ref that shouldn't (or something\n> that should be dereferencing it not doing so). If you post your patch\n> and a reproduction command, I might be able to help debug.\n\nAh, your \"address I wrote in the compose buffer\" was the clue I needed.\n\nI think this is actually an existing bug. If I use --compose and write:\n\n To: foo@example.com\n\nin the editor, we read that back in and handle it in parse_header_line()\nlike:\n\n my $addr_pat = join \"|\", qw(To Cc Bcc);\n\n foreach (split(/\\n/, $lines)) {\n if (/^($addr_pat):\\s*(.+)$/i) {\n $parsed_line->{$1} = [ parse_address_line($2) ];\n } elsif (/^([^:]*):\\s*(.+)\\s*$/i) {\n $parsed_line->{$1} = $2;\n }\n }\n\nand there's your perl array ref (from the square brackets, which are\nnecessary because we're sticking it in a hash value). But even before\nyour patch, this seems to end up as garbage. The code which reads\n$parsed_line does not dereference the array.\n\nThe patch to fix it is only a few lines (well, more than that with some\nlight editorializing in the comments):\n\ndiff --git a/git-send-email.perl b/git-send-email.perl\nindex 76589c7827..46a30088c9 100755\n--- a/git-send-email.perl\n+++ b/git-send-email.perl\n@@ -918,7 +918,28 @@ sub get_patch_subject {\n \t# Preserve unknown headers\n \tforeach my $key (keys %parsed_email) {\n \t\tnext if $key eq 'body';\n-\t\tprint $c2 \"$key: $parsed_email{$key}\";\n+\n+\t\t# it seems like it would be easier to just look for\n+\t\t# $parsed_email{'To'} and so on. But we actually match\n+\t\t# these case-insenstively and preserve the user's spelling, so\n+\t\t# we might see $parsed_email{'to'}. Of course, the same bug\n+\t\t# exists for Subject, etc, above. Anyway, a \"/i\" regex here\n+\t\t# handles all cases.\n+\t\t#\n+\t\t# It kind of feels like all of this code would be much simpler\n+\t\t# if we just handled all of the headers while reading back the\n+\t\t# input, rather than stuffing them all into $parsed_email and\n+\t\t# then picking them out of it.\n+\t\t#\n+\t\t# It also really feels like these to/cc/bcc lines should be\n+\t\t# added to the regular ones? It is silly to make a cover letter\n+\t\t# that goes to some addresses, and then not send the patches to\n+\t\t# them, too.\n+\t\tif ($key =~ /^(To|Cc|Bcc)$/i) {\n+\t\t\tprint $c2 \"$key: \", join(', ', @{$parsed_email{$key}});\n+\t\t} else {\n+\t\t\tprint $c2 \"$key: $parsed_email{$key}\";\n+\t\t}\n \t}\n \n \tif ($parsed_email{'body'}) {\n\nI don't really think your patch makes things worse here. But it is\nprobably worth fixing it while we are here.\n\n-Peff\n"},{"id":"483554","messageId":"20231020100343.GA2194322@coredump.intra.peff.net","threadId":"60255","inReplyTo":"20231020071402.GC1642714@coredump.intra.peff.net","subject":"[PATCH 0/3] some send-email --compose fixes","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-10-20T10:03:43Z","receivedAt":"2023-10-20T10:03:47Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"[culling the rather large cc, as we moving off the original topic]\n\nOn Fri, Oct 20, 2023 at 03:14:03AM -0400, Jeff King wrote:\n\n> and there's your perl array ref (from the square brackets, which are\n> necessary because we're sticking it in a hash value). But even before\n> your patch, this seems to end up as garbage. The code which reads\n> $parsed_line does not dereference the array.\n> \n> The patch to fix it is only a few lines (well, more than that with some\n> light editorializing in the comments):\n\nSo here's the fix in a cleaned up form, guided by my own comments from\nearlier. ;) I think this is actually all orthogonal to the patch you are\nworking on, so yours could either go on top or just be applied\nseparately.\n\n [1/3]: doc/send-email: mention handling of \"reply-to\" with --compose\n [2/3]: Revert \"send-email: extract email-parsing code into a subroutine\"\n [3/3]: send-email: handle to/cc/bcc from --compose message\n\n Documentation/git-send-email.txt | 10 +--\n git-send-email.perl | 132 ++++++++++++-------------------\n t/t9001-send-email.sh | 41 ++++++++++\n 3 files changed, 98 insertions(+), 85 deletions(-)\n\n-Peff\n"},{"id":"483556","messageId":"20231020100901.GA2673716@coredump.intra.peff.net","threadId":"60255","inReplyTo":"20231020100343.GA2194322@coredump.intra.peff.net","subject":"[PATCH 1/3] doc/send-email: mention handling of \"reply-to\" with --compose","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-10-20T10:09:01Z","receivedAt":"2023-10-20T10:09:03Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The documentation for git-send-email lists the headers handled specially\nby --compose in a way that implies that this is the complete set of\nheaders that are special. But one more was added by d11c943c78\n(send-email: support separate Reply-To address, 2018-03-04) and never\ndocumented.\n\nLet's add it, and reword the documentation slightly to avoid having to\nspecify the list of headers twice (as it is growing and will continue to\ndo so as we add new features).\n\nIf you read the code, you may notice that we also handle MIME-Version\nspecially, in that we'll avoid over-writing user-provided MIME headers.\nI don't think this is worth mentioning, as it's what you'd expect to\nhappen (as opposed to the other headers, which are picked up to be used\nin later emails). And certainly this feature existed when the\ndocumentation was expanded in 01d3861217 (git-send-email.txt: describe\n--compose better, 2009-03-16), and we chose not to mention it then.\n\nSigned-off-by: Jeff King \n---\nJust something I noticed since a later commit touches the same list.\n\n Documentation/git-send-email.txt | 10 +++++-----\n 1 file changed, 5 insertions(+), 5 deletions(-)\n\ndiff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt\nindex 492a82323d..021276329c 100644\n--- a/Documentation/git-send-email.txt\n+++ b/Documentation/git-send-email.txt\n@@ -68,11 +68,11 @@ This option may be specified multiple times.\n \tInvoke a text editor (see GIT_EDITOR in linkgit:git-var[1])\n \tto edit an introductory message for the patch series.\n +\n-When `--compose` is used, git send-email will use the From, Subject, and\n-In-Reply-To headers specified in the message. If the body of the message\n-(what you type after the headers and a blank line) only contains blank\n-(or Git: prefixed) lines, the summary won't be sent, but From, Subject,\n-and In-Reply-To headers will be used unless they are removed.\n+When `--compose` is used, git send-email will use the From, Subject,\n+Reply-To, and In-Reply-To headers specified in the message. If the body\n+of the message (what you type after the headers and a blank line) only\n+contains blank (or Git: prefixed) lines, the summary won't be sent, but\n+the headers mentioned above will be used unless they are removed.\n +\n Missing From or In-Reply-To headers will be prompted for.\n +\n-- \n2.42.0.980.g8b5f6199be\n\n"},{"id":"483557","messageId":"20231020101310.GB2673716@coredump.intra.peff.net","threadId":"60255","inReplyTo":"20231020100343.GA2194322@coredump.intra.peff.net","subject":"[PATCH 2/3] Revert \"send-email: extract email-parsing code into a subroutine\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-10-20T10:13:10Z","receivedAt":"2023-10-20T10:13:14Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This reverts commit b6049542b97e7b135e0e82bf996084d461224d32.\n\nPrior to that commit, we read the results of the user editing the\n\"--compose\" message in a loop, picking out parts we cared about, and\nstreaming the result out to a \".final\" file. That commit split the\nreading/interpreting into two phases; we'd now read into a hash, and\nthen pick things out of the hash.\n\nThe goal was making the code more readable. And in some ways it did,\nbecause the ugly regexes are confined to the reading phase. But it also\nintroduced several bugs, because now the two phases need to match each\nother. In particular:\n\n - we pick out headers like \"Subject: foo\" with a case-insensitive\n regex, and then use the user-provided header name as the key in a\n case-sensitive hash. So if the user wrote \"subject: foo\", we'd no\n longer recognize it as a subject.\n\n - the namespace for the hash keys conflates header names with meta\n information like \"body\". If you put \"body: foo\" in your message, it\n would be misinterpreted as the actual message body (nobody is likely\n to do that in practice, but it seems like an unnecessary danger).\n\n - the handling for to/cc/bcc is totally broken. The behavior before\n that commit is to recognize and skip those headers, with a note to\n the user that they are not yet handled. Not great, but OK. But\n after the patch, the reading side now splits the addresses into a\n perl array-ref. But the interpreting side doesn't handle this at\n all, and blindly prints the stringified array-ref value. This leads\n to garbage like:\n\n (mbox) Adding to: ARRAY (0x555b4345c428) from line 'To: ARRAY(0x555b4345c428)'\n error: unable to extract a valid address from: ARRAY (0x555b4345c428)\n What to do with this address? ([q]uit|[d]rop|[e]dit):\n\n Probably not a huge deal, since nobody should even try to use those\n headers in the first place (since they were not implemented). But\n the new behavior is worse, and indicative of the sorts of problems\n that come from having the two layers.\n\nThe revert had a few conflicts, due to later work in this area from\n15dc3b9161 (send-email: rename variable for clarity, 2018-03-04) and\nd11c943c78 (send-email: support separate Reply-To address, 2018-03-04).\nI've ported the changes from those commits over as part of the conflict\nresolution.\n\nThe new tests show the bugs. Note the use of GIT_SEND_EMAIL_NOTTY in the\nsecond one. Without it, the test is happy to reach outside the test\nharness to the developer's actual terminal (when run with the buggy\nstate before this patch).\n\nSigned-off-by: Jeff King \n---\nI guess \"readable\" is up for debate here, but I find the inline handling\na lot easier to follow (and it's half as many lines; most of the\ndiffstat is the new tests).\n\nBut one thing that gives me pause is that the neither before or after\nthis patch do we handle continuation lines like:\n\n Subject: this is the beginning\n and this is more subject\n\nAnd it would probably be a lot easier to add when storing the headers in\na hash (it's not impossible to do it the other way, but you basically\nhave to delay processing each line with a small state machine).\n\nSo another option is to just fix the individual bugs separately.\n\n git-send-email.perl | 120 ++++++++++++++----------------------------\n t/t9001-send-email.sh | 35 ++++++++++++\n 2 files changed, 75 insertions(+), 80 deletions(-)\n\ndiff --git a/git-send-email.perl b/git-send-email.perl\nindex 288ea1ae80..bbda2a931b 100755\n--- a/git-send-email.perl\n+++ b/git-send-email.perl\n@@ -888,73 +888,59 @@ sub get_patch_subject {\n \t\tdo_edit($compose_filename);\n \t}\n \n+\topen my $c2, \">\", $compose_filename . \".final\"\n+\t\tor die sprintf(__(\"Failed to open %s.final: %s\"), $compose_filename, $!);\n+\n \topen $c, \"<\", $compose_filename\n \t\tor die sprintf(__(\"Failed to open %s: %s\"), $compose_filename, $!);\n \n+\tmy $need_8bit_cte = file_has_nonascii($compose_filename);\n+\tmy $in_body = 0;\n+\tmy $summary_empty = 1;\n \tif (!defined $compose_encoding) {\n \t\t$compose_encoding = \"UTF-8\";\n \t}\n-\n-\tmy %parsed_email;\n-\twhile (my $line = <$c>) {\n-\t\tnext if $line =~ m/^GIT:/;\n-\t\tparse_header_line($line, \\%parsed_email);\n-\t\tif ($line =~ /^$/) {\n-\t\t\t$parsed_email{'body'} = filter_body($c);\n+\twhile(<$c>) {\n+\t\tnext if m/^GIT:/;\n+\t\tif ($in_body) {\n+\t\t\t$summary_empty = 0 unless (/^\\n$/);\n+\t\t} elsif (/^\\n$/) {\n+\t\t\t$in_body = 1;\n+\t\t\tif ($need_8bit_cte) {\n+\t\t\t\tprint $c2 \"MIME-Version: 1.0\\n\",\n+\t\t\t\t\t \"Content-Type: text/plain; \",\n+\t\t\t\t\t \"charset=$compose_encoding\\n\",\n+\t\t\t\t\t \"Content-Transfer-Encoding: 8bit\\n\";\n+\t\t\t}\n+\t\t} elsif (/^MIME-Version:/i) {\n+\t\t\t$need_8bit_cte = 0;\n+\t\t} elsif (/^Subject:\\s*(.+)\\s*$/i) {\n+\t\t\t$initial_subject = $1;\n+\t\t\tmy $subject = $initial_subject;\n+\t\t\t$_ = \"Subject: \" .\n+\t\t\t\tquote_subject($subject, $compose_encoding) .\n+\t\t\t\t\"\\n\";\n+\t\t} elsif (/^In-Reply-To:\\s*(.+)\\s*$/i) {\n+\t\t\t$initial_in_reply_to = $1;\n+\t\t\tnext;\n+\t\t} elsif (/^Reply-To:\\s*(.+)\\s*$/i) {\n+\t\t\t$reply_to = $1;\n+\t\t} elsif (/^From:\\s*(.+)\\s*$/i) {\n+\t\t\t$sender = $1;\n+\t\t\tnext;\n+\t\t} elsif (/^(?:To|Cc|Bcc):/i) {\n+\t\t\tprint __(\"To/Cc/Bcc fields are not interpreted yet, they have been ignored\\n\");\n+\t\t\tnext;\n \t\t}\n+\t\tprint $c2 $_;\n \t}\n \tclose $c;\n+\tclose $c2;\n \n-\topen my $c2, \">\", $compose_filename . \".final\"\n-\tor die sprintf(__(\"Failed to open %s.final: %s\"), $compose_filename, $!);\n-\n-\n-\tif ($parsed_email{'From'}) {\n-\t\t$sender = delete($parsed_email{'From'});\n-\t}\n-\tif ($parsed_email{'In-Reply-To'}) {\n-\t\t$initial_in_reply_to = delete($parsed_email{'In-Reply-To'});\n-\t}\n-\tif ($parsed_email{'Reply-To'}) {\n-\t\t$reply_to = delete($parsed_email{'Reply-To'});\n-\t}\n-\tif ($parsed_email{'Subject'}) {\n-\t\t$initial_subject = delete($parsed_email{'Subject'});\n-\t\tprint $c2 \"Subject: \" .\n-\t\t\tquote_subject($initial_subject, $compose_encoding) .\n-\t\t\t\"\\n\";\n-\t}\n-\n-\tif ($parsed_email{'MIME-Version'}) {\n-\t\tprint $c2 \"MIME-Version: $parsed_email{'MIME-Version'}\\n\",\n-\t\t\t\t\"Content-Type: $parsed_email{'Content-Type'};\\n\",\n-\t\t\t\t\"Content-Transfer-Encoding: $parsed_email{'Content-Transfer-Encoding'}\\n\";\n-\t\tdelete($parsed_email{'MIME-Version'});\n-\t\tdelete($parsed_email{'Content-Type'});\n-\t\tdelete($parsed_email{'Content-Transfer-Encoding'});\n-\t} elsif (file_has_nonascii($compose_filename)) {\n-\t\tmy $content_type = (delete($parsed_email{'Content-Type'}) or\n-\t\t\t\"text/plain; charset=$compose_encoding\");\n-\t\tprint $c2 \"MIME-Version: 1.0\\n\",\n-\t\t\t\"Content-Type: $content_type\\n\",\n-\t\t\t\"Content-Transfer-Encoding: 8bit\\n\";\n-\t}\n-\t# Preserve unknown headers\n-\tforeach my $key (keys %parsed_email) {\n-\t\tnext if $key eq 'body';\n-\t\tprint $c2 \"$key: $parsed_email{$key}\";\n-\t}\n-\n-\tif ($parsed_email{'body'}) {\n-\t\tprint $c2 \"\\n$parsed_email{'body'}\\n\";\n-\t\tdelete($parsed_email{'body'});\n-\t} else {\n+\tif ($summary_empty) {\n \t\tprint __(\"Summary email is empty, skipping it\\n\");\n \t\t$compose = -1;\n \t}\n-\n-\tclose $c2;\n-\n } elsif ($annotate) {\n \tdo_edit(@files);\n }\n@@ -1009,32 +995,6 @@ sub ask {\n \treturn;\n }\n \n-sub parse_header_line {\n-\tmy $lines = shift;\n-\tmy $parsed_line = shift;\n-\tmy $addr_pat = join \"|\", qw(To Cc Bcc);\n-\n-\tforeach (split(/\\n/, $lines)) {\n-\t\tif (/^($addr_pat):\\s*(.+)$/i) {\n-\t\t $parsed_line->{$1} = [ parse_address_line($2) ];\n-\t\t} elsif (/^([^:]*):\\s*(.+)\\s*$/i) {\n-\t\t $parsed_line->{$1} = $2;\n-\t\t}\n-\t}\n-}\n-\n-sub filter_body {\n-\tmy $c = shift;\n-\tmy $body = \"\";\n-\twhile (my $body_line = <$c>) {\n-\t\tif ($body_line !~ m/^GIT:/) {\n-\t\t\t$body .= $body_line;\n-\t\t}\n-\t}\n-\treturn $body;\n-}\n-\n-\n my %broken_encoding;\n \n sub file_declares_8bit_cte {\ndiff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh\nindex 263db3ad17..9644ff5793 100755\n--- a/t/t9001-send-email.sh\n+++ b/t/t9001-send-email.sh\n@@ -2505,4 +2505,39 @@ test_expect_success $PREREQ 'test forbidSendmailVariables behavior override' '\n \t\tHEAD^\n '\n \n+test_expect_success $PREREQ '--compose handles lowercase headers' '\n+\twrite_script fake-editor <<-\\EOF &&\n+\tsed \"s/^From:.*/from: edited-from@example.com/i\" \"$1\" >\"$1.tmp\" &&\n+\tmv \"$1.tmp\" \"$1\"\n+\tEOF\n+\tclean_fake_sendmail &&\n+\tgit send-email \\\n+\t\t--compose \\\n+\t\t--from=\"Example \" \\\n+\t\t--to=nobody@example.com \\\n+\t\t--smtp-server=\"$(pwd)/fake.sendmail\" \\\n+\t\tHEAD^ &&\n+\tgrep \"From: edited-from@example.com\" msgtxt1\n+'\n+\n+test_expect_success $PREREQ '--compose handles to headers' '\n+\twrite_script fake-editor <<-\\EOF &&\n+\tsed \"s/^$/To: edited-to@example.com\\n/\" <\"$1\" >\"$1.tmp\" &&\n+\techo this is the body >>\"$1.tmp\" &&\n+\tmv \"$1.tmp\" \"$1\"\n+\tEOF\n+\tclean_fake_sendmail &&\n+\tGIT_SEND_EMAIL_NOTTY=1 \\\n+\tgit send-email \\\n+\t\t--compose \\\n+\t\t--from=\"Example \" \\\n+\t\t--to=nobody@example.com \\\n+\t\t--smtp-server=\"$(pwd)/fake.sendmail\" \\\n+\t\tHEAD^ &&\n+\t# Ideally the \"to\" header we specified would be used,\n+\t# but the program explicitly warns that these are\n+\t# ignored. For now, just make sure we did not abort.\n+\tgrep \"To:\" msgtxt1\n+'\n+\n test_done\n-- \n2.42.0.980.g8b5f6199be\n\n"},{"id":"483558","messageId":"20231020101524.GA2673857@coredump.intra.peff.net","threadId":"60255","inReplyTo":"20231020100343.GA2194322@coredump.intra.peff.net","subject":"[PATCH 3/3] send-email: handle to/cc/bcc from --compose message","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-10-20T10:15:24Z","receivedAt":"2023-10-20T10:15:27Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"If the user writes a message via --compose, send-email will pick up\nvarius headers like \"From\", \"Subject\", etc and use them for other\npatches as if they were specified on the command-line. But we don't\nhandle \"To\", \"Cc\", or \"Bcc\" this way; we just tell the user \"those\naren't interpeted yet\" and ignore them.\n\nBut it seems like an obvious thing to want, especially as the same\nfeature exists when the cover letter is generated separately by\nformat-patch. There it is gated behind the --to-cover option, but I\ndon't think we'd need the same control here; since we generate the\n--compose template ourselves based on the existing input, if the user\nleaves the lines unchanged then the behavior remains the same.\n\nSo let's fill in the implementation; like those other headers we already\nhandle, we just need to assign to the initial_* variables. The only\ndifference in this case is that they are arrays, so we'll feed them\nthrough parse_address_line() to split them (just like we would when\nreading a single string via prompting).\n\nSigned-off-by: Jeff King \n---\n Documentation/git-send-email.txt | 11 ++++++-----\n git-send-email.perl | 16 ++++++++++++++--\n t/t9001-send-email.sh | 16 +++++++++++-----\n 3 files changed, 31 insertions(+), 12 deletions(-)\n\ndiff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt\nindex 021276329c..f4d7166275 100644\n--- a/Documentation/git-send-email.txt\n+++ b/Documentation/git-send-email.txt\n@@ -68,11 +68,12 @@ This option may be specified multiple times.\n \tInvoke a text editor (see GIT_EDITOR in linkgit:git-var[1])\n \tto edit an introductory message for the patch series.\n +\n-When `--compose` is used, git send-email will use the From, Subject,\n-Reply-To, and In-Reply-To headers specified in the message. If the body\n-of the message (what you type after the headers and a blank line) only\n-contains blank (or Git: prefixed) lines, the summary won't be sent, but\n-the headers mentioned above will be used unless they are removed.\n+When `--compose` is used, git send-email will use the From, To, Cc, Bcc,\n+Subject, Reply-To, and In-Reply-To headers specified in the message. If\n+the body of the message (what you type after the headers and a blank\n+line) only contains blank (or Git: prefixed) lines, the summary won't be\n+sent, but the headers mentioned above will be used unless they are\n+removed.\n +\n Missing From or In-Reply-To headers will be prompted for.\n +\ndiff --git a/git-send-email.perl b/git-send-email.perl\nindex bbda2a931b..9e21b0b3f4 100755\n--- a/git-send-email.perl\n+++ b/git-send-email.perl\n@@ -861,6 +861,9 @@ sub get_patch_subject {\n \tmy $tpl_subject = $initial_subject || '';\n \tmy $tpl_in_reply_to = $initial_in_reply_to || '';\n \tmy $tpl_reply_to = $reply_to || '';\n+\tmy $tpl_to = join(',', @initial_to);\n+\tmy $tpl_cc = join(',', @initial_cc);\n+\tmy $tpl_bcc = join(', ', @initial_bcc);\n \n \tprint $c <\"$1.tmp\" &&\n+\tsed \"s/^To: .*/&, edited-to@example.com/\" <\"$1\" >\"$1.tmp\" &&\n \techo this is the body >>\"$1.tmp\" &&\n \tmv \"$1.tmp\" \"$1\"\n \tEOF\n@@ -2534,10 +2534,16 @@ test_expect_success $PREREQ '--compose handles to headers' '\n \t\t--to=nobody@example.com \\\n \t\t--smtp-server=\"$(pwd)/fake.sendmail\" \\\n \t\tHEAD^ &&\n-\t# Ideally the \"to\" header we specified would be used,\n-\t# but the program explicitly warns that these are\n-\t# ignored. For now, just make sure we did not abort.\n-\tgrep \"To:\" msgtxt1\n+\t# Check both that the cover letter used our modified \"to\" line,\n+\t# but also that it was picked up for the patch.\n+\tq_to_tab >expect <<-\\EOF &&\n+\tTo: nobody@example.com,\n+\tQedited-to@example.com\n+\tEOF\n+\tgrep -A1 \"^To:\" msgtxt1 >msgtxt1.to &&\n+\ttest_cmp expect msgtxt1.to &&\n+\tgrep -A1 \"^To:\" msgtxt2 >msgtxt2.to &&\n+\ttest_cmp expect msgtxt2.to\n '\n \n test_done\n-- \n2.42.0.980.g8b5f6199be\n"},{"id":"483561","messageId":"ZTJaVzt75r0iHPzR@ugly","threadId":"60255","inReplyTo":"20231020101310.GB2673716@coredump.intra.peff.net","subject":"Re: [PATCH 2/3] Revert \"send-email: extract email-parsing code into a subroutine\"","fromName":"Oswald Buddenhagen","fromEmail":"oswald.buddenhagen@gmx.de","sentAt":"2023-10-20T10:45:43Z","receivedAt":"2023-10-20T10:48:03Z","isPatch":true,"sender":{"key":"oswald.buddenhagen@gmx.de","avatar":"https://avatars.githubusercontent.com/u/812380?v=4"},"body":"On Fri, Oct 20, 2023 at 06:13:10AM -0400, Jeff King wrote:\n>But one thing that gives me pause is that the neither before or after\n>this patch do we handle continuation lines like:\n>\n> Subject: this is the beginning\n> and this is more subject\n>\n>And it would probably be a lot easier to add when storing the headers in\n>a hash (it's not impossible to do it the other way, but you basically\n>have to delay processing each line with a small state machine).\n>\nthat seems like a rather significant point, doesn't it?\n\n>So another option is to just fix the individual bugs separately.\n>\n... so that seems preferable to me, given that the necessary fixes seem \nrather trivial.\n\n> I guess \"readable\" is up for debate here, but I find the inline handling\n> a lot easier to follow\n>\nany particular reason for that?\n\n> (and it's half as many lines; most of the diffstat is the new tests).\n\n>-\tif ($parsed_email{'From'}) {\n>-\t\t$sender = delete($parsed_email{'From'});\n>-\t}\n\nthis verbosity could be cut down somewhat using just\n\n $sender = delete($parsed_email{'From'});\n\nand if the value can be pre-set and needs to be preserved,\n\n $sender = delete($parsed_email{'From'}) // $sender;\n\nbut this seems kind of counter-productive legibility-wise.\n\nregards\n"},{"id":"483577","messageId":"CAPig+cQHOF-aQqqZUQB07HrPAoCg=80P3eLwwSUbbCeHAwX5zg@mail.gmail.com","threadId":"60255","inReplyTo":"20231020101524.GA2673857@coredump.intra.peff.net","subject":"Re: [PATCH 3/3] send-email: handle to/cc/bcc from --compose message","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2023-10-20T17:30:05Z","receivedAt":"2023-10-20T17:30:19Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Oct 20, 2023 at 6:15 AM Jeff King wrote:\n> If the user writes a message via --compose, send-email will pick up\n> varius headers like \"From\", \"Subject\", etc and use them for other\n\ns/varius/various/\n\n> patches as if they were specified on the command-line. But we don't\n> handle \"To\", \"Cc\", or \"Bcc\" this way; we just tell the user \"those\n> aren't interpeted yet\" and ignore them.\n>\n> But it seems like an obvious thing to want, especially as the same\n> feature exists when the cover letter is generated separately by\n> format-patch. There it is gated behind the --to-cover option, but I\n> don't think we'd need the same control here; since we generate the\n> --compose template ourselves based on the existing input, if the user\n> leaves the lines unchanged then the behavior remains the same.\n>\n> So let's fill in the implementation; like those other headers we already\n> handle, we just need to assign to the initial_* variables. The only\n> difference in this case is that they are arrays, so we'll feed them\n> through parse_address_line() to split them (just like we would when\n> reading a single string via prompting).\n>\n> Signed-off-by: Jeff King \n"},{"id":"483603","messageId":"xmqqil71otsa.fsf@gitster.g","threadId":"60255","inReplyTo":"20231020100343.GA2194322@coredump.intra.peff.net","subject":"Re: [PATCH 0/3] some send-email --compose fixes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-10-20T21:42:13Z","receivedAt":"2023-10-20T21:42:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> [culling the rather large cc, as we moving off the original topic]\n>\n> On Fri, Oct 20, 2023 at 03:14:03AM -0400, Jeff King wrote:\n>\n>> and there's your perl array ref (from the square brackets, which are\n>> necessary because we're sticking it in a hash value). But even before\n>> your patch, this seems to end up as garbage. The code which reads\n>> $parsed_line does not dereference the array.\n>> \n>> The patch to fix it is only a few lines (well, more than that with some\n>> light editorializing in the comments):\n>\n> So here's the fix in a cleaned up form, guided by my own comments from\n> earlier. ;) I think this is actually all orthogonal to the patch you are\n> working on, so yours could either go on top or just be applied\n> separately.\n>\n> [1/3]: doc/send-email: mention handling of \"reply-to\" with --compose\n> [2/3]: Revert \"send-email: extract email-parsing code into a subroutine\"\n> [3/3]: send-email: handle to/cc/bcc from --compose message\n\nNice.\n\nWith the approach suggested to move the validation down to where the\nnecessary addresses are already all defined, Michael observed \"whoa,\nwhy am I getting stringified array ref?\". If that is the only issue\nin the approach, queuing these three patches first and then have\nMichael's fix on top of them sounds like the cleanest thing to do.\n\nWill queue on top of v2.42.0 to help those who may want to backport\nthese to the maintenance track.\n\nThanks.\n"},{"id":"483604","messageId":"xmqqedhpotmt.fsf@gitster.g","threadId":"60255","inReplyTo":"20231020101310.GB2673716@coredump.intra.peff.net","subject":"Re: [PATCH 2/3] Revert \"send-email: extract email-parsing code into a subroutine\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-10-20T21:45:30Z","receivedAt":"2023-10-20T21:46:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> - the handling for to/cc/bcc is totally broken.\n\nIt is good to see another evidence that \"--compose\" is probably not\nas often as used as we thought. With enough bugs discovered,\nperhaps someday we can declare \"it cannot be that the feature is\nused in the wild, without anybody getting hit by these bugs---let's\ndeprecate and eventually remove it\" ;-)\n"},{"id":"483697","messageId":"20231023184010.GA1537181@coredump.intra.peff.net","threadId":"60255","inReplyTo":"ZTJaVzt75r0iHPzR@ugly","subject":"Re: [PATCH 2/3] Revert \"send-email: extract email-parsing code into a subroutine\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-10-23T18:40:10Z","receivedAt":"2023-10-23T18:40:14Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Oct 20, 2023 at 12:45:43PM +0200, Oswald Buddenhagen wrote:\n\n> On Fri, Oct 20, 2023 at 06:13:10AM -0400, Jeff King wrote:\n> > But one thing that gives me pause is that the neither before or after\n> > this patch do we handle continuation lines like:\n> > \n> > Subject: this is the beginning\n> > and this is more subject\n> > \n> > And it would probably be a lot easier to add when storing the headers in\n> > a hash (it's not impossible to do it the other way, but you basically\n> > have to delay processing each line with a small state machine).\n> > \n> that seems like a rather significant point, doesn't it?\n\nMaybe. It depends on whether anybody is interested in adding\ncontinuation support. Nobody has in the previous 18 years, and nobody\nhas asked for it.\n\n> > So another option is to just fix the individual bugs separately.\n> > \n> ... so that seems preferable to me, given that the necessary fixes seem\n> rather trivial.\n\nThey're not too bad. Probably:\n\n 1. lc() the keys we put into the hash\n\n 2. match to/cc/bcc and dereference their arrays\n\n 3. maybe handle 'body' separately from headers to avoid confusion\n\nBut there may be other similar bugs lurking. One I didn't mention: the\nhash-based version randomly reorders headers!\n\n> > I guess \"readable\" is up for debate here, but I find the inline handling\n> > a lot easier to follow\n> > \n> any particular reason for that?\n\nFor the reasons I gave in the commit message: namely that the matching\nand logic is in one place and doesn't need to be duplicated (e.g., the\nspecial handling of to/cc/bcc, which caused a bug here).\n\n> > (and it's half as many lines; most of the diffstat is the new tests).\n> \n> > -\tif ($parsed_email{'From'}) {\n> > -\t\t$sender = delete($parsed_email{'From'});\n> > -\t}\n> \n> this verbosity could be cut down somewhat using just\n> \n> $sender = delete($parsed_email{'From'});\n> \n> and if the value can be pre-set and needs to be preserved,\n> \n> $sender = delete($parsed_email{'From'}) // $sender;\n> \n> but this seems kind of counter-productive legibility-wise.\n\nWe do need to avoid overwriting the pre-set value. The \"//\" one would\nwork, but we support perl versions old enough that they don't have it.\n\n-Peff\n"},{"id":"483700","messageId":"20231023184724.GB1537181@coredump.intra.peff.net","threadId":"60255","inReplyTo":"xmqqedhpotmt.fsf@gitster.g","subject":"Re: [PATCH 2/3] Revert \"send-email: extract email-parsing code into a subroutine\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-10-23T18:47:24Z","receivedAt":"2023-10-23T18:47:27Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Oct 20, 2023 at 02:45:30PM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > - the handling for to/cc/bcc is totally broken.\n> \n> It is good to see another evidence that \"--compose\" is probably not\n> as often as used as we thought. With enough bugs discovered,\n> perhaps someday we can declare \"it cannot be that the feature is\n> used in the wild, without anybody getting hit by these bugs---let's\n> deprecate and eventually remove it\" ;-)\n\nI'm not sure if that is evidence or not. The to/cc/bcc feature was just\nnever implemented. The commit from 2017 made it more broken than saying\n\"not yet implemented\", but that may only be an indication that nobody\nwants it or tried to use it.\n\nI dunno. As I noted, the same feature exists when reading the\ncover-letter from a set of format-patch files. And of course it is\nimplemented using totally separate code (in pre_process_file). One\npossible cleanup would be to unify those two, but I'm sure there would\nbe behavior changes. Some of them perhaps good (e.g., it looks like\npre_process_file is more careful about rfc2047 handling) and some of\nthem I'm not so sure of (e.g., support for --header-cmd in the --compose\nletter).\n\nI think an interested person could champion such changes, but I am not\nthat interested in send-email (I don't use it, and some of its code is\npretty ancient and gross). My goal was to fix the bug I saw with minimal\nregression (I waffled even on my patch 2).\n\n-Peff\n"},{"id":"483701","messageId":"20231023185152.GC1537181@coredump.intra.peff.net","threadId":"60255","inReplyTo":"xmqqil71otsa.fsf@gitster.g","subject":"Re: [PATCH 0/3] some send-email --compose fixes","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-10-23T18:51:52Z","receivedAt":"2023-10-23T18:51:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Oct 20, 2023 at 02:42:13PM -0700, Junio C Hamano wrote:\n\n> > So here's the fix in a cleaned up form, guided by my own comments from\n> > earlier. ;) I think this is actually all orthogonal to the patch you are\n> > working on, so yours could either go on top or just be applied\n> > separately.\n> >\n> > [1/3]: doc/send-email: mention handling of \"reply-to\" with --compose\n> > [2/3]: Revert \"send-email: extract email-parsing code into a subroutine\"\n> > [3/3]: send-email: handle to/cc/bcc from --compose message\n> \n> Nice.\n> \n> With the approach suggested to move the validation down to where the\n> necessary addresses are already all defined, Michael observed \"whoa,\n> why am I getting stringified array ref?\". If that is the only issue\n> in the approach, queuing these three patches first and then have\n> Michael's fix on top of them sounds like the cleanest thing to do.\n\nI don't think it is even an issue in Michael's approach. I'd have to see\nhis patch and how he tested it to be sure, but I suspect he was simply\nbeing extra careful to test nearby behavior and stumbled upon the\nARRAY() bug. But the bug was there long before either of his patches.\n\n> Will queue on top of v2.42.0 to help those who may want to backport\n> these to the maintenance track.\n\nSo I think you could take my series on top of master (or 2.42.0), and\neventually target 'master'. The bug it fixes is from 2017, so not\nurgent. The reading of \"to\" headers is a new feature.\n\nBut the fix to move the validation around should probably go directly\nonto a8022c5f7b (send-email: expose header information to\ngit-send-email's sendemail-validate hook, 2023-04-19) for use on maint.\nI guess maybe it is not that urgent anymore, as that regression is in\nv2.41, and we would not release anything along that maint track anymore,\nthough.\n\n-Peff\n"},{"id":"483716","messageId":"ZTbOnsxBFERPLN3F@ugly","threadId":"60255","inReplyTo":"20231023184010.GA1537181@coredump.intra.peff.net","subject":"Re: [PATCH 2/3] Revert \"send-email: extract email-parsing code into a subroutine\"","fromName":"Oswald Buddenhagen","fromEmail":"oswald.buddenhagen@gmx.de","sentAt":"2023-10-23T19:50:54Z","receivedAt":"2023-10-23T19:51:09Z","isPatch":true,"sender":{"key":"oswald.buddenhagen@gmx.de","avatar":"https://avatars.githubusercontent.com/u/812380?v=4"},"body":"On Mon, Oct 23, 2023 at 02:40:10PM -0400, Jeff King wrote:\n>On Fri, Oct 20, 2023 at 12:45:43PM +0200, Oswald Buddenhagen wrote:\n>> that seems like a rather significant point, doesn't it?\n>\n>Maybe. It depends on whether anybody is interested in adding\n>continuation support. Nobody has in the previous 18 years, and nobody\n>has asked for it.\n>\ndunno, it seems like a bug to me. so if i cared at all about this \nfunctionality, i'd fix it just because. so at least it doesn't seem nice \nto make it harder for a potential volunteer.\n\n>> > So another option is to just fix the individual bugs separately.\n>> > \n>> ... so that seems preferable to me, given that the necessary fixes \n>> seem\n>> rather trivial.\n>\n>They're not too bad. Probably:\n>\n> 1. lc() the keys we put into the hash\n>\n> 2. match to/cc/bcc and dereference their arrays\n>\n> 3. maybe handle 'body' separately from headers to avoid confusion\n>\nwith the header keys lowercased, one could simply use BODY as the key \nand be done with it.\n\n>But there may be other similar bugs lurking.\n\n>One I didn't mention: the\n>hash-based version randomly reorders headers!\n>\nhmm, yeah, that would mean using Tie::IxHash if one wanted to do it \nelegantly, at the cost of depending on another non-core module.\n\nalso, it means that another hash with non-lowercased versions of the \nkeys would have to be kept.\n\nok, that's stupid. it would be easier to just keep an additional array \nof the original keys for iteration, and check the hash before emitting \nthem.\n\n>> > I guess \"readable\" is up for debate here, but I find the inline handling\n>> > a lot easier to follow\n>> > \n>> any particular reason for that?\n>\n>For the reasons I gave in the commit message: namely that the matching\n>and logic is in one place and doesn't need to be duplicated (e.g., the\n>special handling of to/cc/bcc, which caused a bug here).\n>\nfrom what i can see, there isn't really anything to \"match\", apart from \nagreeing on the data structure (which the code partially failed to do, \nbut that's trivial enough). and layering/abstracting things is usually \nconsidered a good thing, unless the cost/benefit ratio is completely \nbackwards.\n\n>The \"//\" one would\n>work, but we support perl versions old enough that they don't have it.\n>\naccording to my grepping, that ship has sailed.\nalso, why _would_ you support such ancient perl versions? that makes \neven less sense to me than supporting ancient c compilers.\n\nregards\n"},{"id":"483813","messageId":"393f598e-c7cd-4dc6-a221-9aed7ffcc2b1@amd.com","threadId":"60255","inReplyTo":"20231023185152.GC1537181@coredump.intra.peff.net","subject":"Re: [PATCH 0/3] some send-email --compose fixes","fromName":"Michael Strawbridge","fromEmail":"michael.strawbridge@amd.com","sentAt":"2023-10-24T20:12:03Z","receivedAt":"2023-10-24T20:12:11Z","isPatch":true,"sender":{"key":"michael.strawbridge@amd.com","avatar":null},"body":"\n\nOn 10/23/23 14:51, Jeff King wrote:\n> On Fri, Oct 20, 2023 at 02:42:13PM -0700, Junio C Hamano wrote:\n> \n>>> So here's the fix in a cleaned up form, guided by my own comments from\n>>> earlier. ;) I think this is actually all orthogonal to the patch you are\n>>> working on, so yours could either go on top or just be applied\n>>> separately.\n>>>\n>>> [1/3]: doc/send-email: mention handling of \"reply-to\" with --compose\n>>> [2/3]: Revert \"send-email: extract email-parsing code into a subroutine\"\n>>> [3/3]: send-email: handle to/cc/bcc from --compose message\n>>\n>> Nice.\n>>\n>> With the approach suggested to move the validation down to where the\n>> necessary addresses are already all defined, Michael observed \"whoa,\n>> why am I getting stringified array ref?\". If that is the only issue\n>> in the approach, queuing these three patches first and then have\n>> Michael's fix on top of them sounds like the cleanest thing to do.\n\nPatch coming soon.\n> \n> I don't think it is even an issue in Michael's approach. I'd have to see\n> his patch and how he tested it to be sure, but I suspect he was simply\n> being extra careful to test nearby behavior and stumbled upon the\n> ARRAY() bug. But the bug was there long before either of his patches.\n> \nThank you for your patches Peff! I think it fixes the issue I was seeing.\nI was trying to be extra careful with my testing. I had missed testing\n--compose and also the multiple --to/cc/bcc examples before.\n\n>> Will queue on top of v2.42.0 to help those who may want to backport\n>> these to the maintenance track.\n> \n> So I think you could take my series on top of master (or 2.42.0), and\n> eventually target 'master'. The bug it fixes is from 2017, so not\n> urgent. The reading of \"to\" headers is a new feature.\n> \n> But the fix to move the validation around should probably go directly\n> onto a8022c5f7b (send-email: expose header information to\n> git-send-email's sendemail-validate hook, 2023-04-19) for use on maint.\n> I guess maybe it is not that urgent anymore, as that regression is in\n> v2.41, and we would not release anything along that maint track anymore,\n> though.\n> \n> -Peff\n"},{"id":"483816","messageId":"ee56c4df-e030-45f9-86a9-94fb3540db60@amd.com","threadId":"60255","inReplyTo":"393f598e-c7cd-4dc6-a221-9aed7ffcc2b1@amd.com","subject":"[PATCH] send-email: move validation code below process_address_list","fromName":"Michael Strawbridge","fromEmail":"michael.strawbridge@amd.com","sentAt":"2023-10-24T20:19:43Z","receivedAt":"2023-10-24T20:19:52Z","isPatch":true,"sender":{"key":"michael.strawbridge@amd.com","avatar":null},"body":"From 09ea51d63cebdf9ff0c073ef86e21b4b09c268e5 Mon Sep 17 00:00:00 2001\nFrom: Michael Strawbridge \nDate: Wed, 11 Oct 2023 16:13:13 -0400\nSubject: [PATCH] send-email: move validation code below process_address_list\n\nMove validation logic below processing of email address lists so that\nemail validation gets the proper email addresses.\n\nThis fixes email address validation errors when the optional\nperl module Email::Valid is installed and multiple addresses are passed\nin on a single to/cc argument like --to=foo@example.com,bar@example.com.\n\nReported-by: Bagas Sanjaya \nSigned-off-by: Michael Strawbridge \n---\n git-send-email.perl | 48 ++++++++++++++++++++++-----------------------\n 1 file changed, 24 insertions(+), 24 deletions(-)\n\ndiff --git a/git-send-email.perl b/git-send-email.perl\nindex 288ea1ae80..a898dbc76e 100755\n--- a/git-send-email.perl\n+++ b/git-send-email.perl\n@@ -799,30 +799,6 @@ sub is_format_patch_arg {\n \n $time = time - scalar $#files;\n \n-if ($validate) {\n-\t# FIFOs can only be read once, exclude them from validation.\n-\tmy @real_files = ();\n-\tforeach my $f (@files) {\n-\t\tunless (-p $f) {\n-\t\t\tpush(@real_files, $f);\n-\t\t}\n-\t}\n-\n-\t# Run the loop once again to avoid gaps in the counter due to FIFO\n-\t# arguments provided by the user.\n-\tmy $num = 1;\n-\tmy $num_files = scalar @real_files;\n-\t$ENV{GIT_SENDEMAIL_FILE_TOTAL} = \"$num_files\";\n-\tforeach my $r (@real_files) {\n-\t\t$ENV{GIT_SENDEMAIL_FILE_COUNTER} = \"$num\";\n-\t\tpre_process_file($r, 1);\n-\t\tvalidate_patch($r, $target_xfer_encoding);\n-\t\t$num += 1;\n-\t}\n-\tdelete $ENV{GIT_SENDEMAIL_FILE_COUNTER};\n-\tdelete $ENV{GIT_SENDEMAIL_FILE_TOTAL};\n-}\n-\n @files = handle_backup_files(@files);\n \n if (@files) {\n@@ -2023,6 +1999,30 @@ sub process_file {\n \treturn 1;\n }\n \n+if ($validate) {\n+\t# FIFOs can only be read once, exclude them from validation.\n+\tmy @real_files = ();\n+\tforeach my $f (@files) {\n+\t\tunless (-p $f) {\n+\t\t\tpush(@real_files, $f);\n+\t\t}\n+\t}\n+\n+\t# Run the loop once again to avoid gaps in the counter due to FIFO\n+\t# arguments provided by the user.\n+\tmy $num = 1;\n+\tmy $num_files = scalar @real_files;\n+\t$ENV{GIT_SENDEMAIL_FILE_TOTAL} = \"$num_files\";\n+\tforeach my $r (@real_files) {\n+\t\t$ENV{GIT_SENDEMAIL_FILE_COUNTER} = \"$num\";\n+\t\tpre_process_file($r, 1);\n+\t\tvalidate_patch($r, $target_xfer_encoding);\n+\t\t$num += 1;\n+\t}\n+\tdelete $ENV{GIT_SENDEMAIL_FILE_COUNTER};\n+\tdelete $ENV{GIT_SENDEMAIL_FILE_TOTAL};\n+}\n+\n foreach my $t (@files) {\n \twhile (!process_file($t)) {\n \t\t# user edited the file\n-- \n2.42.0\n"},{"id":"483824","messageId":"xmqqmsw73cua.fsf@gitster.g","threadId":"60255","inReplyTo":"ee56c4df-e030-45f9-86a9-94fb3540db60@amd.com","subject":"Re: [PATCH] send-email: move validation code below process_address_list","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-10-24T21:55:09Z","receivedAt":"2023-10-24T21:55:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Strawbridge writes:\n\n> Subject: [PATCH] send-email: move validation code below process_address_list\n>\n> Move validation logic below processing of email address lists so that\n> email validation gets the proper email addresses.\n\nHmph, without this patch, the tip of 'seen' passes t9001 on my box,\nbut with it, it claims that it failed 58, 87, and 89.\n\nHere is how #58 fails (the last part of \"cd t && sh t9001-*.sh -i -v\").\n\nexpecting success of 9001.58 'In-Reply-To without --chain-reply-to':\n clean_fake_sendmail &&\n echo \"\" >expect &&\n git send-email \\\n --from=\"Example \" \\\n --to=nobody@example.com \\\n --no-chain-reply-to \\\n --in-reply-to=\"$(cat expect)\" \\\n --smtp-server=\"$(pwd)/fake.sendmail\" \\\n $patches $patches $patches \\\n 2>errors &&\n # The first message is a reply to --in-reply-to\n sed -n -e \"s/^In-Reply-To: *\\(.*\\)/\\1/p\" msgtxt1 >actual &&\n test_cmp expect actual &&\n # Second and subsequent messages are replies to the first one\n sed -n -e \"s/^Message-ID: *\\(.*\\)/\\1/p\" msgtxt1 >expect &&\n sed -n -e \"s/^In-Reply-To: *\\(.*\\)/\\1/p\" msgtxt2 >actual &&\n test_cmp expect actual &&\n sed -n -e \"s/^In-Reply-To: *\\(.*\\)/\\1/p\" msgtxt3 >actual &&\n test_cmp expect actual\n\n0001-Second.patch\n0001-Second.patch\n0001-Second.patch\n(mbox) Adding cc: A from line 'From: A '\n(mbox) Adding cc: One from line 'Cc: One , two@example.com'\n(mbox) Adding cc: two@example.com from line 'Cc: One , two@example.com'\n(body) Adding cc: C O Mitter from line 'Signed-off-by: C O Mitter '\nOK. Log says:\nSendmail: /usr/local/google/home/jch/w/git.git/t/trash directory.t9001-send-email/fake.sendmail -i nobody@example.com author@example.com one@example.com two@example.com committer@example.com\nFrom: Example \nTo: nobody@example.com\nCc: A ,\n One ,\n two@example.com,\n C O Mitter \nSubject: [PATCH 1/1] Second.\nDate: Tue, 24 Oct 2023 21:52:27 +0000\nMessage-ID: <20231024215229.1787922-1-nobody@example.com>\nX-Mailer: git-send-email 2.42.0-705-g1a1f985ecc\nIn-Reply-To: \nReferences: \nMIME-Version: 1.0\nContent-Transfer-Encoding: 8bit\n\nResult: OK\n(mbox) Adding cc: A from line 'From: A '\n(mbox) Adding cc: One from line 'Cc: One , two@example.com'\n(mbox) Adding cc: two@example.com from line 'Cc: One , two@example.com'\n(body) Adding cc: C O Mitter from line 'Signed-off-by: C O Mitter '\nOK. Log says:\nSendmail: /usr/local/google/home/jch/w/git.git/t/trash directory.t9001-send-email/fake.sendmail -i nobody@example.com author@example.com one@example.com two@example.com committer@example.com\nFrom: Example \nTo: nobody@example.com\nCc: A ,\n One ,\n two@example.com,\n C O Mitter \nSubject: [PATCH 1/1] Second.\nDate: Tue, 24 Oct 2023 21:52:28 +0000\nMessage-ID: <20231024215229.1787922-2-nobody@example.com>\nX-Mailer: git-send-email 2.42.0-705-g1a1f985ecc\nIn-Reply-To: \nReferences: \nMIME-Version: 1.0\nContent-Transfer-Encoding: 8bit\n\nResult: OK\n(mbox) Adding cc: A from line 'From: A '\n(mbox) Adding cc: One from line 'Cc: One , two@example.com'\n(mbox) Adding cc: two@example.com from line 'Cc: One , two@example.com'\n(body) Adding cc: C O Mitter from line 'Signed-off-by: C O Mitter '\nOK. Log says:\nSendmail: /usr/local/google/home/jch/w/git.git/t/trash directory.t9001-send-email/fake.sendmail -i nobody@example.com author@example.com one@example.com two@example.com committer@example.com\nFrom: Example \nTo: nobody@example.com\nCc: A ,\n One ,\n two@example.com,\n C O Mitter \nSubject: [PATCH 1/1] Second.\nDate: Tue, 24 Oct 2023 21:52:29 +0000\nMessage-ID: <20231024215229.1787922-3-nobody@example.com>\nX-Mailer: git-send-email 2.42.0-705-g1a1f985ecc\nIn-Reply-To: \nReferences: \nMIME-Version: 1.0\nContent-Transfer-Encoding: 8bit\n\nResult: OK\n--- expect 2023-10-24 21:52:29.115044899 +0000\n+++ actual 2023-10-24 21:52:29.119045306 +0000\n@@ -1 +1 @@\n-<20231024215229.1787922-1-nobody@example.com>\n+\nnot ok 58 - In-Reply-To without --chain-reply-to\n#\n# clean_fake_sendmail &&\n# echo \"\" >expect &&\n# git send-email \\\n# --from=\"Example \" \\\n# --to=nobody@example.com \\\n# --no-chain-reply-to \\\n# --in-reply-to=\"$(cat expect)\" \\\n# --smtp-server=\"$(pwd)/fake.sendmail\" \\\n# $patches $patches $patches \\\n# 2>errors &&\n# # The first message is a reply to --in-reply-to\n# sed -n -e \"s/^In-Reply-To: *\\(.*\\)/\\1/p\" msgtxt1 >actual &&\n# test_cmp expect actual &&\n# # Second and subsequent messages are replies to the first one\n# sed -n -e \"s/^Message-ID: *\\(.*\\)/\\1/p\" msgtxt1 >expect &&\n# sed -n -e \"s/^In-Reply-To: *\\(.*\\)/\\1/p\" msgtxt2 >actual &&\n# test_cmp expect actual &&\n# sed -n -e \"s/^In-Reply-To: *\\(.*\\)/\\1/p\" msgtxt3 >actual &&\n# test_cmp expect actual\n#\n1..58\n\n"},{"id":"483825","messageId":"xmqqil6v3cgq.fsf@gitster.g","threadId":"60255","inReplyTo":"xmqqmsw73cua.fsf@gitster.g","subject":"Re: [PATCH] send-email: move validation code below process_address_list","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-10-24T22:03:17Z","receivedAt":"2023-10-24T22:03:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Michael Strawbridge writes:\n>\n>> Subject: [PATCH] send-email: move validation code below process_address_list\n>>\n>> Move validation logic below processing of email address lists so that\n>> email validation gets the proper email addresses.\n>\n> Hmph, without this patch, the tip of 'seen' passes t9001 on my box,\n> but with it, it claims that it failed 58, 87, and 89.\n\nFWIW, when this patch is used with 'master' (not 'seen'), t9001\nclaims the same three tests failed. The way #58 fails seems to be\nidentical to the way 'seen' with this patch failed, shown in the\nmessage I am responding to.\n"},{"id":"483830","messageId":"20231025061120.GA2094463@coredump.intra.peff.net","threadId":"60255","inReplyTo":"ZTbOnsxBFERPLN3F@ugly","subject":"Re: [PATCH 2/3] Revert \"send-email: extract email-parsing code into a subroutine\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-10-25T06:11:20Z","receivedAt":"2023-10-25T06:11:24Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Oct 23, 2023 at 09:50:54PM +0200, Oswald Buddenhagen wrote:\n\n> > The \"//\" one would\n> > work, but we support perl versions old enough that they don't have it.\n> > \n> according to my grepping, that ship has sailed.\n> also, why _would_ you support such ancient perl versions? that makes even\n> less sense to me than supporting ancient c compilers.\n\nIt may be reasonable to bump the default perl version for the script.\nBut that would require somebody digging into what tends to ship these\ndays (which can be sometimes be surprising; witness macos using old\nversions of bash due to license issues), and then updating the \"use\n5.008\" in the script.\n\nThe \"//\" operator was added in perl 5.10. I'm not sure what you found\nthat makes you think the ship has sailed. The only hits for \"//\" I see\nlook like the end of substitution regexes (\"s/foo//\" and similar). But\nif we are not consistent with the \"use\" claim, that is worth fixing.\n\n-Peff\n"},{"id":"483832","messageId":"20231025065033.GB2094463@coredump.intra.peff.net","threadId":"60255","inReplyTo":"ee56c4df-e030-45f9-86a9-94fb3540db60@amd.com","subject":"Re: [PATCH] send-email: move validation code below process_address_list","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-10-25T06:50:33Z","receivedAt":"2023-10-25T06:50:39Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Oct 24, 2023 at 04:19:43PM -0400, Michael Strawbridge wrote:\n\n> Move validation logic below processing of email address lists so that\n> email validation gets the proper email addresses.\n> \n> This fixes email address validation errors when the optional\n> perl module Email::Valid is installed and multiple addresses are passed\n> in on a single to/cc argument like --to=foo@example.com,bar@example.com.\n\nIs there a test we can include here?\n\n> @@ -2023,6 +1999,30 @@ sub process_file {\n> \treturn 1;\n> }\n> \n> +if ($validate) {\n\nSo the new spot is right before we call process_file() on each of the\ninput files. It is a little hard to follow because of the number of\nfunctions defined inline in the middle of the script, but I think that\nis a reasonable spot. It is after we have called process_address_list()\non to/cc/bcc, which I think fixes the regression. But it is also after\nwe sanitize $reply_to, etc, which seems like a good idea.\n\nBut I think putting it down that far is the source of the test failures.\n\nThe culprit seems not to be the call to validate_patch() in the loop you\nmoved, but rather pre_process_file(), which was added in your earlier\na8022c5f7b (send-email: expose header information to git-send-email's\nsendemail-validate hook, 2023-04-19).\n\nIt looks like the issue is the global $message_num variable which is\nincremented by pre_process_file(). On line 1755 (on the current tip of\nmaster), we set it to 0. And your patch moves the validation across\nthere (from line ~799 to ~2023).\n\nAnd that's why the extra increments didn't matter when you added the\ncalls to pre_process_file() in your earlier patch; they all happened\nbefore we reset $message_num to 0. But now they happen after.\n\nTo be fair, this is absolutely horrific perl code. There's over a\nthousand lines of function definitions, and then hidden in the middle\nare some global variable assignments!\n\nSo we have a few options, I think:\n\n 1. Reset $message_num to 0 after validating (maybe we also need\n to reset $in_reply_to, etc, set at the same time? I didn't check).\n This feels like a hack.\n\n 2. Move the validation down, but not so far down. Like maybe right\n after we set up the @files list with the $compose.final name. This\n is the smallest diff, but feels like we haven't really made the\n world a better place.\n\n 3. Move the $message_num, etc, initialization to right before we call\n the process_file() loop, which is what expects to use them. Like\n this (squashed into your patch):\n\ndiff --git a/git-send-email.perl b/git-send-email.perl\nindex a898dbc76e..d44db14223 100755\n--- a/git-send-email.perl\n+++ b/git-send-email.perl\n@@ -1730,10 +1730,6 @@ sub send_message {\n \treturn 1;\n }\n \n-$in_reply_to = $initial_in_reply_to;\n-$references = $initial_in_reply_to || '';\n-$message_num = 0;\n-\n sub pre_process_file {\n \tmy ($t, $quiet) = @_;\n \n@@ -2023,6 +2019,10 @@ sub process_file {\n \tdelete $ENV{GIT_SENDEMAIL_FILE_TOTAL};\n }\n \n+$in_reply_to = $initial_in_reply_to;\n+$references = $initial_in_reply_to || '';\n+$message_num = 0;\n+\n foreach my $t (@files) {\n \twhile (!process_file($t)) {\n \t\t# user edited the file\n\nThat seems to make the test failures go away. It is still weird that the\nvalidation code is calling pre_process_file(), which increments\n$message_num, without us having set it up in any meaningful way. I'm not\nsure if there are bugs lurking there or not. I'm not impressed by the\ngeneral quality of this code, and I'm kind of afraid to keep looking\ndeeper.\n\n-Peff\n"},{"id":"483837","messageId":"20231025074317.r3sydthautjjsf5y@pengutronix.de","threadId":"60255","inReplyTo":"ee56c4df-e030-45f9-86a9-94fb3540db60@amd.com","subject":"Re: [PATCH] send-email: move validation code below process_address_list","fromName":"Uwe Kleine-König","fromEmail":"u.kleine-koenig@pengutronix.de","sentAt":"2023-10-25T07:43:17Z","receivedAt":"2023-10-25T07:43:27Z","isPatch":true,"sender":{"key":"u.kleine-koenig@pengutronix.de","avatar":"https://gravatar.com/avatar/354b5e3ceb2806a2f1e1e382ac29ddbdad18288654da62b61eb13583a857eee7?d=mp&s=160"},"body":"Hello,\n\nOn Tue, Oct 24, 2023 at 04:19:43PM -0400, Michael Strawbridge wrote:\n> >From 09ea51d63cebdf9ff0c073ef86e21b4b09c268e5 Mon Sep 17 00:00:00 2001\n> From: Michael Strawbridge \n> Date: Wed, 11 Oct 2023 16:13:13 -0400\n> Subject: [PATCH] send-email: move validation code below process_address_list\n> \n> Move validation logic below processing of email address lists so that\n> email validation gets the proper email addresses.\n> \n> This fixes email address validation errors when the optional\n> perl module Email::Valid is installed and multiple addresses are passed\n> in on a single to/cc argument like --to=foo@example.com,bar@example.com.\n> \n> Reported-by: Bagas Sanjaya \n> Signed-off-by: Michael Strawbridge \n\nIf you do Fixes: trailers as the kernel does, this could get:\n\nFixes: a8022c5f7b67 (\"send-email: expose header information to git-send-email's sendemail-validate hook\")\n\nI tested this patch on top of main (2e8e77cbac8a) and it fixes the\nregression I reported in a separate thread (where Jeff pointed out this\npatch as fixing it).\n\nTested-by: Uwe Kleine-König \n\nThanks\nUwe\n\n-- \nPengutronix e.K. | Uwe Kleine-König |\nIndustrial Linux Solutions | https://www.pengutronix.de/ |\n"},{"id":"483847","messageId":"ZTjedSluwyrVY+L9@ugly","threadId":"60255","inReplyTo":"20231025061120.GA2094463@coredump.intra.peff.net","subject":"Re: [PATCH 2/3] Revert \"send-email: extract email-parsing code into a subroutine\"","fromName":"Oswald Buddenhagen","fromEmail":"oswald.buddenhagen@gmx.de","sentAt":"2023-10-25T09:23:01Z","receivedAt":"2023-10-25T09:23:08Z","isPatch":true,"sender":{"key":"oswald.buddenhagen@gmx.de","avatar":"https://avatars.githubusercontent.com/u/812380?v=4"},"body":"On Wed, Oct 25, 2023 at 02:11:20AM -0400, Jeff King wrote:\n>The \"//\" operator was added in perl 5.10. I'm not sure what you found\n>that makes you think the ship has sailed. The only hits for \"//\" I see\n>look like the end of substitution regexes (\"s/foo//\" and similar).\n>\ngrep with spaces around the operator, then you can see the instance in \ngit-credential-netrc.perl easily.\n\nregards\n"},{"id":"483863","messageId":"907b2699-5792-4cb0-a727-44e514c2480d@amd.com","threadId":"60255","inReplyTo":"20231025065033.GB2094463@coredump.intra.peff.net","subject":"Re: [PATCH] send-email: move validation code below process_address_list","fromName":"Michael Strawbridge","fromEmail":"michael.strawbridge@amd.com","sentAt":"2023-10-25T18:47:44Z","receivedAt":"2023-10-25T18:47:58Z","isPatch":true,"sender":{"key":"michael.strawbridge@amd.com","avatar":null},"body":"\n\nOn 10/25/23 02:50, Jeff King wrote:\n> On Tue, Oct 24, 2023 at 04:19:43PM -0400, Michael Strawbridge wrote:\n> \n>> Move validation logic below processing of email address lists so that\n>> email validation gets the proper email addresses.\n>>\n>> This fixes email address validation errors when the optional\n>> perl module Email::Valid is installed and multiple addresses are passed\n>> in on a single to/cc argument like --to=foo@example.com,bar@example.com.\n> \n> Is there a test we can include here?\n> \n>> @@ -2023,6 +1999,30 @@ sub process_file {\n>> \treturn 1;\n>> }\n>> \n>> +if ($validate) {\n> \n> So the new spot is right before we call process_file() on each of the\n> input files. It is a little hard to follow because of the number of\n> functions defined inline in the middle of the script, but I think that\n> is a reasonable spot. It is after we have called process_address_list()\n> on to/cc/bcc, which I think fixes the regression. But it is also after\n> we sanitize $reply_to, etc, which seems like a good idea.\n> \n> But I think putting it down that far is the source of the test failures.\n> \n> The culprit seems not to be the call to validate_patch() in the loop you\n> moved, but rather pre_process_file(), which was added in your earlier\n> a8022c5f7b (send-email: expose header information to git-send-email's\n> sendemail-validate hook, 2023-04-19).\n> \n> It looks like the issue is the global $message_num variable which is\n> incremented by pre_process_file(). On line 1755 (on the current tip of\n> master), we set it to 0. And your patch moves the validation across\n> there (from line ~799 to ~2023).\n> \n> And that's why the extra increments didn't matter when you added the\n> calls to pre_process_file() in your earlier patch; they all happened\n> before we reset $message_num to 0. But now they happen after.\n> \n> To be fair, this is absolutely horrific perl code. There's over a\n> thousand lines of function definitions, and then hidden in the middle\n> are some global variable assignments!\n\nI agree. Following where things are initialized seems to be especially troublesome.\n> \n> So we have a few options, I think:\n> \n> 1. Reset $message_num to 0 after validating (maybe we also need\n> to reset $in_reply_to, etc, set at the same time? I didn't check).\n> This feels like a hack.\n> \n> 2. Move the validation down, but not so far down. Like maybe right\n> after we set up the @files list with the $compose.final name. This\n> is the smallest diff, but feels like we haven't really made the\n> world a better place.\n> \n> 3. Move the $message_num, etc, initialization to right before we call\n> the process_file() loop, which is what expects to use them. Like\n> this (squashed into your patch):\n> \n> diff --git a/git-send-email.perl b/git-send-email.perl\n> index a898dbc76e..d44db14223 100755\n> --- a/git-send-email.perl\n> +++ b/git-send-email.perl\n> @@ -1730,10 +1730,6 @@ sub send_message {\n> \treturn 1;\n> }\n> \n> -$in_reply_to = $initial_in_reply_to;\n> -$references = $initial_in_reply_to || '';\n> -$message_num = 0;\n> -\n> sub pre_process_file {\n> \tmy ($t, $quiet) = @_;\n> \n> @@ -2023,6 +2019,10 @@ sub process_file {\n> \tdelete $ENV{GIT_SENDEMAIL_FILE_TOTAL};\n> }\n> \n> +$in_reply_to = $initial_in_reply_to;\n> +$references = $initial_in_reply_to || '';\n> +$message_num = 0;\n> +\n> foreach my $t (@files) {\n> \twhile (!process_file($t)) {\n> \t\t# user edited the file\n> \nThe above patch was a great place to start. Thank you! In order to address\nthe fact that validation and actually sending the emails should have the same\ninitial conditions I created a new function to set the variables and call it\ninstead.\n> That seems to make the test failures go away. It is still weird that the\n> validation code is calling pre_process_file(), which increments\n> $message_num, without us having set it up in any meaningful way. I'm not\n> sure if there are bugs lurking there or not. I'm not impressed by the\n> general quality of this code, and I'm kind of afraid to keep looking\n> deeper.\n> \n> -Peff\n"},{"id":"483864","messageId":"0bea6645-b21f-499d-bfe0-dc6ac56814fa@amd.com","threadId":"60255","inReplyTo":"xmqqil6v3cgq.fsf@gitster.g","subject":"Re: [PATCH] send-email: move validation code below process_address_list","fromName":"Michael Strawbridge","fromEmail":"michael.strawbridge@amd.com","sentAt":"2023-10-25T18:48:45Z","receivedAt":"2023-10-25T18:48:50Z","isPatch":true,"sender":{"key":"michael.strawbridge@amd.com","avatar":null},"body":"\n\nOn 10/24/23 18:03, Junio C Hamano wrote:\n> Junio C Hamano writes:\n> \n>> Michael Strawbridge writes:\n>>\n>>> Subject: [PATCH] send-email: move validation code below process_address_list\n>>>\n>>> Move validation logic below processing of email address lists so that\n>>> email validation gets the proper email addresses.\n>>\n>> Hmph, without this patch, the tip of 'seen' passes t9001 on my box,\n>> but with it, it claims that it failed 58, 87, and 89.\n> \n> FWIW, when this patch is used with 'master' (not 'seen'), t9001\n> claims the same three tests failed. The way #58 fails seems to be\n> identical to the way 'seen' with this patch failed, shown in the\n> message I am responding to.\n\nI'm sorry to have wasted your time with patch 1. I had done the other manual\ntests but ended up forgetting the automated ones.\n"},{"id":"483865","messageId":"ddd4bfdd-ed14-44f4-89d3-192332bbc1c4@amd.com","threadId":"60255","inReplyTo":"xmqqil6v3cgq.fsf@gitster.g","subject":"[PATCH v2] send-email: move validation code below process_address_list","fromName":"Michael Strawbridge","fromEmail":"michael.strawbridge@amd.com","sentAt":"2023-10-25T18:51:29Z","receivedAt":"2023-10-25T18:51:38Z","isPatch":true,"sender":{"key":"michael.strawbridge@amd.com","avatar":null},"body":"From 67223238d9b1977d20b1286055d7f197e4d746e9 Mon Sep 17 00:00:00 2001\nFrom: Michael Strawbridge \nDate: Wed, 11 Oct 2023 16:13:13 -0400\nSubject: [PATCH v2] send-email: move validation code below\n process_address_list\n\nMove validation logic below processing of email address lists so that\nemail validation gets the proper email addresses. As a side effect,\nsome initialization needed to be moved down. In order for validation\nand the actual email sending to have the same initial state, the\ninitialized variables that get modified by pre_process_file are\nencapsulated in a new function.\n\nThis fixes email address validation errors when the optional\nperl module Email::Valid is installed and multiple addresses are passed\nin on a single to/cc argument like --to=foo@example.com,bar@example.com.\nA new test was added to t9001 to expose failures with this case in the\nfuture.\n\nFixes: a8022c5f7b67 (\"send-email: expose header information to git-send-email's sendemail-validate hook\")\nReported-by: Bagas Sanjaya \nSigned-off-by: Michael Strawbridge \n---\n git-send-email.perl | 60 +++++++++++++++++++++++--------------------\n t/t9001-send-email.sh | 19 ++++++++++++++\n 2 files changed, 51 insertions(+), 28 deletions(-)\n\ndiff --git a/git-send-email.perl b/git-send-email.perl\nindex 288ea1ae80..ce22a5e06d 100755\n--- a/git-send-email.perl\n+++ b/git-send-email.perl\n@@ -799,30 +799,6 @@ sub is_format_patch_arg {\n \n $time = time - scalar $#files;\n \n-if ($validate) {\n-\t# FIFOs can only be read once, exclude them from validation.\n-\tmy @real_files = ();\n-\tforeach my $f (@files) {\n-\t\tunless (-p $f) {\n-\t\t\tpush(@real_files, $f);\n-\t\t}\n-\t}\n-\n-\t# Run the loop once again to avoid gaps in the counter due to FIFO\n-\t# arguments provided by the user.\n-\tmy $num = 1;\n-\tmy $num_files = scalar @real_files;\n-\t$ENV{GIT_SENDEMAIL_FILE_TOTAL} = \"$num_files\";\n-\tforeach my $r (@real_files) {\n-\t\t$ENV{GIT_SENDEMAIL_FILE_COUNTER} = \"$num\";\n-\t\tpre_process_file($r, 1);\n-\t\tvalidate_patch($r, $target_xfer_encoding);\n-\t\t$num += 1;\n-\t}\n-\tdelete $ENV{GIT_SENDEMAIL_FILE_COUNTER};\n-\tdelete $ENV{GIT_SENDEMAIL_FILE_TOTAL};\n-}\n-\n @files = handle_backup_files(@files);\n \n if (@files) {\n@@ -1754,10 +1730,6 @@ sub send_message {\n \treturn 1;\n }\n \n-$in_reply_to = $initial_in_reply_to;\n-$references = $initial_in_reply_to || '';\n-$message_num = 0;\n-\n sub pre_process_file {\n \tmy ($t, $quiet) = @_;\n \n@@ -2023,6 +1995,38 @@ sub process_file {\n \treturn 1;\n }\n \n+sub initialize_modified_loop_vars {\n+\t$in_reply_to = $initial_in_reply_to;\n+\t$references = $initial_in_reply_to || '';\n+\t$message_num = 0;\n+}\n+\n+if ($validate) {\n+\t# FIFOs can only be read once, exclude them from validation.\n+\tmy @real_files = ();\n+\tforeach my $f (@files) {\n+\t\tunless (-p $f) {\n+\t\t\tpush(@real_files, $f);\n+\t\t}\n+\t}\n+\n+\t# Run the loop once again to avoid gaps in the counter due to FIFO\n+\t# arguments provided by the user.\n+\tmy $num = 1;\n+\tmy $num_files = scalar @real_files;\n+\t$ENV{GIT_SENDEMAIL_FILE_TOTAL} = \"$num_files\";\n+\tinitialize_modified_loop_vars();\n+\tforeach my $r (@real_files) {\n+\t\t$ENV{GIT_SENDEMAIL_FILE_COUNTER} = \"$num\";\n+\t\tpre_process_file($r, 1);\n+\t\tvalidate_patch($r, $target_xfer_encoding);\n+\t\t$num += 1;\n+\t}\n+\tdelete $ENV{GIT_SENDEMAIL_FILE_COUNTER};\n+\tdelete $ENV{GIT_SENDEMAIL_FILE_TOTAL};\n+}\n+\n+initialize_modified_loop_vars();\n foreach my $t (@files) {\n \twhile (!process_file($t)) {\n \t\t# user edited the file\ndiff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh\nindex 263db3ad17..ccff2ad647 100755\n--- a/t/t9001-send-email.sh\n+++ b/t/t9001-send-email.sh\n@@ -633,6 +633,25 @@ test_expect_success $PREREQ \"--validate respects absolute core.hooksPath path\" '\n \ttest_cmp expect actual\n '\n \n+test_expect_success $PREREQ \"--validate hook supports multiple addresses in arguments\" '\n+\thooks_path=\"$(pwd)/my-hooks\" &&\n+\ttest_config core.hooksPath \"$hooks_path\" &&\n+\ttest_when_finished \"rm my-hooks.ran\" &&\n+\ttest_must_fail git send-email \\\n+\t\t--from=\"Example \" \\\n+\t\t--to=nobody@example.com,abc@example.com \\\n+\t\t--smtp-server=\"$(pwd)/fake.sendmail\" \\\n+\t\t--validate \\\n+\t\tlongline.patch 2>actual &&\n+\ttest_path_is_file my-hooks.ran &&\n+\tcat >expect <<-EOF &&\n+\tfatal: longline.patch: rejected by sendemail-validate hook\n+\tfatal: command '\"'\"'git hook run --ignore-missing sendemail-validate --
'\"'\"' died with exit code 1\n+\twarning: no patches were sent\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success $PREREQ \"--validate hook supports header argument\" '\n \twrite_script my-hooks/sendemail-validate <<-\\EOF &&\n \tif test \"$#\" -ge 2\n-- \n2.42.GIT\n"},{"id":"483918","messageId":"xmqqlebpr1se.fsf@gitster.g","threadId":"60255","inReplyTo":"ddd4bfdd-ed14-44f4-89d3-192332bbc1c4@amd.com","subject":"Re: [PATCH v2] send-email: move validation code below process_address_list","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-10-26T12:44:33Z","receivedAt":"2023-10-26T12:44:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Strawbridge writes:\n\n> From 67223238d9b1977d20b1286055d7f197e4d746e9 Mon Sep 17 00:00:00 2001\n> From: Michael Strawbridge \n> Date: Wed, 11 Oct 2023 16:13:13 -0400\n> Subject: [PATCH v2] send-email: move validation code below\n> process_address_list\n\nWhy do these in-body headers to lie about the author date?\n\nBy the way, the in-body header does seem to support the header line\nfolding (see the \"subject\" one here).\n"},{"id":"483920","messageId":"7f4620d7-69bc-4237-a46e-4ed7249c00d8@amd.com","threadId":"60255","inReplyTo":"xmqqlebpr1se.fsf@gitster.g","subject":"Re: [PATCH v2] send-email: move validation code below process_address_list","fromName":"Michael Strawbridge","fromEmail":"michael.strawbridge@amd.com","sentAt":"2023-10-26T13:11:13Z","receivedAt":"2023-10-26T13:11:20Z","isPatch":true,"sender":{"key":"michael.strawbridge@amd.com","avatar":null},"body":"\n\nOn 10/26/23 08:44, Junio C Hamano wrote:\n> Michael Strawbridge writes:\n> \n>> From 67223238d9b1977d20b1286055d7f197e4d746e9 Mon Sep 17 00:00:00 2001\n>> From: Michael Strawbridge \n>> Date: Wed, 11 Oct 2023 16:13:13 -0400\n>> Subject: [PATCH v2] send-email: move validation code below\n>> process_address_list\n> \n> Why do these in-body headers to lie about the author date?\n\nSorry. They weren't meant to. I have been amending my local commit\nrather than making new ones for every version. It seems that when\nI do that, the author date stays at the first time the commit was\ncreated. I wasn't aware of that unintended side effect.\n> \n> By the way, the in-body header does seem to support the header line\n> folding (see the \"subject\" one here).\n"},{"id":"483983","messageId":"xmqqpm10p67k.fsf@gitster.g","threadId":"60255","inReplyTo":"20231025074317.r3sydthautjjsf5y@pengutronix.de","subject":"Re: [PATCH] send-email: move validation code below process_address_list","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-10-27T13:04:15Z","receivedAt":"2023-10-27T13:04:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Uwe Kleine-König writes:\n\n>> This fixes email address validation errors when the optional\n>> perl module Email::Valid is installed and multiple addresses are passed\n>> in on a single to/cc argument like --to=foo@example.com,bar@example.com.\n>> \n>> Reported-by: Bagas Sanjaya \n>> Signed-off-by: Michael Strawbridge \n>\n> If you do Fixes: trailers as the kernel does, this could get:\n>\n> Fixes: a8022c5f7b67 (\"send-email: expose header information to git-send-email's sendemail-validate hook\")\n\nWhile referring to a concrete commit object name is great, we tend\nnot to use that particular trailer in this project; rather, we\nprefer to see description on how and in what way the culprit change\nwas undesirable.\n\n> I tested this patch on top of main (2e8e77cbac8a) and it fixes the\n> regression I reported in a separate thread (where Jeff pointed out this\n> patch as fixing it).\n\nGreat. Thanks.\n"},{"id":"484006","messageId":"xmqq34xvpuig.fsf@gitster.g","threadId":"60255","inReplyTo":"ZTjedSluwyrVY+L9@ugly","subject":"Re: [PATCH 2/3] Revert \"send-email: extract email-parsing code into a subroutine\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-10-27T22:31:35Z","receivedAt":"2023-10-27T22:31:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Oswald Buddenhagen writes:\n\n> On Wed, Oct 25, 2023 at 02:11:20AM -0400, Jeff King wrote:\n>>The \"//\" operator was added in perl 5.10. I'm not sure what you found\n>>that makes you think the ship has sailed. The only hits for \"//\" I see\n>>look like the end of substitution regexes (\"s/foo//\" and similar).\n>>\n> grep with spaces around the operator, then you can see the instance in\n> git-credential-netrc.perl easily.\n\nGood find, but given the relative prevalence in use between netrc\nhelper and send-email, my conclusion is rather opposite. It seems\nto indicate that avoiding \"//\" would still be prudent if the only\ntool we can find find broken on 5.008 is the netrc helper.\n"},{"id":"484083","messageId":"20231030091327.GC84866@coredump.intra.peff.net","threadId":"60255","inReplyTo":"ZTjedSluwyrVY+L9@ugly","subject":"Re: [PATCH 2/3] Revert \"send-email: extract email-parsing code into a subroutine\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-10-30T09:13:27Z","receivedAt":"2023-10-30T09:13:30Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Oct 25, 2023 at 11:23:01AM +0200, Oswald Buddenhagen wrote:\n\n> On Wed, Oct 25, 2023 at 02:11:20AM -0400, Jeff King wrote:\n> > The \"//\" operator was added in perl 5.10. I'm not sure what you found\n> > that makes you think the ship has sailed. The only hits for \"//\" I see\n> > look like the end of substitution regexes (\"s/foo//\" and similar).\n> > \n> grep with spaces around the operator, then you can see the instance in\n> git-credential-netrc.perl easily.\n\nAh, yeah, there is one instance there. That script does not have a \"use\"\nmarker, though, and we do not necessarily need or want to be as strict\nwith contrib/ scripts, which are quite optional compared to core\nfunctionality like send-email.\n\nThat said, I do suspect that requiring 5.10 or later would not be too\nburdensome these days. If we want to do so, then the first step would be\nupdating the text in INSTALL, along with the \"use\" directives in most\nfiles. Probably d48b284183 (perl: bump the required Perl version to 5.8\nfrom 5.6.[21], 2010-09-24) could serve as a template.\n\n-Peff\n"}]}