threads / discuss / 60255

[REGRESSION] uninitialized value $address in git send-email when given multiple recipients separated by commas

Subject: [REGRESSION] uninitialized value $address in git send-email when given multiple recipients separated by commas

## tl;dr

47 messages between Sep 22, 2023 and Oct 30, 2023.

replies: 46people: 8as markdown or json

Bagas Sanjaya· Sep 22, 2023, 09:27 UTC · lore
Hi,

This regression is similar to one I reported earlier [1], with the same error message but with slightly different reproducer (and as continuation to the former report).

I use git-send-email(1) to submit patches (occasional doc fixes) to LKML. Rather than having to type multiple --to/--cc addresses, I use the undocumented behavior of listing multiple addresses separated by comma in a single --to/--cc option. i.e.:

```
$ git send-email \
  --to="foo <foo@acme.com>,bar <bar@acme.com>" \
  --cc="main list <main-list@acme.com>, sub list <sub-list@acme.com>" \
  /path/to/series/*.patch
```
[This is the same behavior as Thunderbird.]

In my linux kernel tree (linux.git) used for development, I add sendemail-validate hook that adds DKIM-like attestation with patatt:

``` #!/bin/sh # installed by patatt install-hook patatt sign --hook "${1}" ```

Starting from Git v2.41.0, when I try to use git-send-email(1), I got perl-related error:

``` Use of uninitialized value $address in sprintf at /home/bagas/.app/git/dist/v2.42.0/libexec/git-core/git-send-email line 1172. error: unable to extract a valid address from: ```

It looks like git-send-email(1) trips on cover letter since there is no recipient addresses there, and also on patches without Signed-off-by: trailer.

Bisecting between v2.40.0 and v2.41.0, the culprit is commit a8022c5f7b67 (send-email: expose header information to git-send-email's sendemail-validate hook, 2023-04-19). The perl error should have been reduced by [2], but this address splitting (parsing) is still not addressed.

The full bisection log is:

``` git bisect start '--term-good=ok' '--term-bad=oops' # status: waiting for both good and bad commits # ok: [73876f4861cd3d187a4682290ab75c9dccadbc56] Git 2.40 git bisect ok 73876f4861cd3d187a4682290ab75c9dccadbc56 # status: waiting for bad commit, 1 good commit known # oops: [fe86abd7511a9a6862d5706c6fa1d9b57a63ba09] Git 2.41 git bisect oops fe86abd7511a9a6862d5706c6fa1d9b57a63ba09 # ok: [b64894c2063e5875bfd95b537eafcb3e1abf46ff] Merge branch 'ow/ref-filter-omit-empty' git bisect ok b64894c2063e5875bfd95b537eafcb3e1abf46ff # ok: [ccd12a3d6cc62f51b746654ae56e26d92f89ba92] Merge branch 'en/header-split-cache-h-part-2' git bisect ok ccd12a3d6cc62f51b746654ae56e26d92f89ba92 # oops: [1e1dcb2a423cad350e8f20fdcc957064e5cff528] Merge branch 'jc/dirstat-plug-leaks' git bisect oops 1e1dcb2a423cad350e8f20fdcc957064e5cff528 # oops: [40a5d2b79b57378cc36d43d3b30e704100dc1492] Merge branch 'fc/doc-man-lift-title-length-limit' git bisect oops 40a5d2b79b57378cc36d43d3b30e704100dc1492 # ok: [07ac32fff94b245aec3e2b80efad0b5dada629cb] Merge branch 'ma/gittutorial-fixes' git bisect ok 07ac32fff94b245aec3e2b80efad0b5dada629cb # oops: [7f3cc51b284d696fdb8dfbd8c9f9d0c014019d93] Merge branch 'ar/test-cleanup-unused-file-creation-part2' git bisect oops 7f3cc51b284d696fdb8dfbd8c9f9d0c014019d93 # oops: [b6e9521956b752be4c666efedd7b91bdd05f9756] Merge branch 'ms/send-email-feed-header-to-validate-hook' git bisect oops b6e9521956b752be4c666efedd7b91bdd05f9756 # ok: [e2abfa7212525daa24a52d9f53c45b736abb5dfe] Merge branch 'hx/negotiator-non-recursive' git bisect ok e2abfa7212525daa24a52d9f53c45b736abb5dfe # oops: [a8022c5f7b678189135b6caa3fadb3d8ec0c0d48] send-email: expose header information to git-send-email's sendemail-validate hook git bisect oops a8022c5f7b678189135b6caa3fadb3d8ec0c0d48 # ok: [56adddaa06d376f3977ee91e8a769cd85439d21c] send-email: refactor header generation functions git bisect ok 56adddaa06d376f3977ee91e8a769cd85439d21c # first oops commit: [a8022c5f7b678189135b6caa3fadb3d8ec0c0d48] send-email: expose header information to git-send-email's sendemail-validate hook ```

To reproduce this regression:
1. Clone git.git repo, then branch off:
   ```
   $ git clone https://github.com/git/git.git && cd git
   $ git checkout -b test
   ```
2. Make two dummy signed-off commits:
   ```
   $ echo test > test && git add test && git commit -s -m "test"
   $ echo "test test" >> test && git commit -a -s -m "test test"
   ```
3. Generate patch series:
   ```
   $ mkdir /tmp/test
   $ git format-patch -o /tmp/test --cover-letter main
   ```
4. Send the series to dummy address:
   ```
   $ git send-email --to="foo <foo@acme.com>,bar <bar@acme.com>" /tmp/test/*.patch
   ```
My system runs Debian testing (trixie/sid) with perl 5.36.0.
Thanks.

[1]: https://lore.kernel.org/git/ZQhI5fMhDE82awpE@debian.me/ [2]: https://lore.kernel.org/git/545729b619308c6f3397b9aa1747f26ddc58f461.1695054945.git.me@ttaylorr.com/

-- 
An old man doll... just what I always wanted! - Clara
Jeff King· Sep 24, 2023, 03:36 UTC · re: Bagas Sanjaya · lore

Re: [REGRESSION] uninitialized value $address in git send-email when given multiple recipients separated by commas

On Fri, Sep 22, 2023 at 04:27:55PM +0700, Bagas Sanjaya wrote:
> To reproduce this regression:
I couldn't reproduce the problem here.
I had to modify your instructions slightly:
Show 13 quoted lines
> 1. Clone git.git repo, then branch off:
> 
>    ```
>    $ git clone https://github.com/git/git.git && cd git
>    $ git checkout -b test
>    ```
> 
> 2. Make two dummy signed-off commits:
> 
>    ```
>    $ echo test > test && git add test && git commit -s -m "test"
>    $ echo "test test" >> test && git commit -a -s -m "test test"
>    ```
This all worked.
Show 6 quoted lines
> 3. Generate patch series:
> 
>    ```
>    $ mkdir /tmp/test
>    $ git format-patch -o /tmp/test --cover-letter main
>    ```

This should be s/main/master/, since the git.git repo from step 1 does not have a "main" branch.

Show 5 quoted lines
> 4. Send the series to dummy address:
> 
>    ```
>    $ git send-email --to="foo <foo@acme.com>,bar <bar@acme.com>" /tmp/test/*.patch
>    ```

This did not produce an error for me. I switched out acme.com for some addresses I control, and confirmed that the mail was all delivered fine.

Your report also mentions a validation hook, so I tried installing one like:

	cat >.git/hooks/sendemail-validate <<-\EOF
	#!/bin/sh
	echo >&2 running validate hook
	exit 0
	EOF
	chmod +x .git/hooks/sendemail-validate

and confirmed that the hook runs (three times, as expected). But still no error. I'm using v2.41.0 to test against.

-Peff
Bagas Sanjaya· Sep 25, 2023, 07:45 UTC · re: Jeff King · lore

Re: [REGRESSION] uninitialized value $address in git send-email when given multiple recipients separated by commas

On Sat, Sep 23, 2023 at 11:36:25PM -0400, Jeff King wrote:
Show 13 quoted lines
> Your report also mentions a validation hook, so I tried installing one
> like:
> 
> 	cat >.git/hooks/sendemail-validate <<-\EOF
> 	#!/bin/sh
> 	echo >&2 running validate hook
> 	exit 0
> 	EOF
> 	chmod +x .git/hooks/sendemail-validate
> 
> and confirmed that the hook runs (three times, as expected). But still
> no error. I'm using v2.41.0 to test against.
> 
Hi Jeff,

I think you missed perl version. As stated earlier, I'm on Debian testing with perl v5.36.0. On there, `perl -V` outputs:

```
Summary of my perl5 (revision 5 version 36 subversion 0) configuration:
   
  Platform:
    osname=linux
    osvers=4.19.0
    archname=x86_64-linux-gnu-thread-multi
    uname='linux localhost 4.19.0 #1 smp debian 4.19.0 x86_64 gnulinux '
    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'
    hint=recommended
    useposix=true
    d_sigaction=define
    useithreads=define
    usemultiplicity=define
    use64bitint=define
    use64bitall=define
    uselongdouble=undef
    usemymalloc=n
    default_inc_excludes_dot=define
  Compiler:
    cc='x86_64-linux-gnu-gcc'
    ccflags ='-D_REENTRANT -D_GNU_SOURCE -DDEBIAN -fwrapv -fno-strict-aliasing -pipe -I/usr/local/include -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64'
    optimize='-O2 -g'
    cppflags='-D_REENTRANT -D_GNU_SOURCE -DDEBIAN -fwrapv -fno-strict-aliasing -pipe -I/usr/local/include'
    ccversion=''
    gccversion='13.2.0'
    gccosandvers=''
    intsize=4
    longsize=8
    ptrsize=8
    doublesize=8
    byteorder=12345678
    doublekind=3
    d_longlong=define
    longlongsize=8
    d_longdbl=define
    longdblsize=16
    longdblkind=3
    ivtype='long'
    ivsize=8
    nvtype='double'
    nvsize=8
    Off_t='off_t'
    lseeksize=8
    alignbytes=8
    prototype=define
  Linker and Libraries:
    ld='x86_64-linux-gnu-gcc'
    ldflags =' -fstack-protector-strong -L/usr/local/lib'
    libpth=/usr/local/lib /usr/lib/x86_64-linux-gnu /usr/lib /lib/x86_64-linux-gnu /lib
    libs=-lgdbm -lgdbm_compat -ldb -ldl -lm -lpthread -lc -lcrypt
    perllibs=-ldl -lm -lpthread -lc -lcrypt
    libc=/lib/x86_64-linux-gnu/libc.so.6
    so=so
    useshrplib=true
    libperl=libperl.so.5.36
    gnulibc_version='2.37'
  Dynamic Linking:
    dlsrc=dl_dlopen.xs
    dlext=so
    d_dlsymun=undef
    ccdlflags='-Wl,-E'
    cccdlflags='-fPIC'
    lddlflags='-shared -L/usr/local/lib -fstack-protector-strong'
Characteristics of this binary (from libperl): 
  Compile-time options:
    HAS_TIMES
    MULTIPLICITY
    PERLIO_LAYERS
    PERL_COPY_ON_WRITE
    PERL_DONT_CREATE_GVSV
    PERL_MALLOC_WRAP
    PERL_OP_PARENT
    PERL_PRESERVE_IVUV
    USE_64_BIT_ALL
    USE_64_BIT_INT
    USE_ITHREADS
    USE_LARGE_FILES
    USE_LOCALE
    USE_LOCALE_COLLATE
    USE_LOCALE_CTYPE
    USE_LOCALE_NUMERIC
    USE_LOCALE_TIME
    USE_PERLIO
    USE_PERL_ATOF
    USE_REENTRANT_API
    USE_THREAD_SAFE_LOCALE
  Locally applied patches:
    DEBPKG:debian/cpan_definstalldirs - Provide a sensible INSTALLDIRS default for modules installed from CPAN.
    DEBPKG:debian/db_file_ver - https://bugs.debian.org/340047 Remove overly restrictive DB_File version check.
    DEBPKG:debian/doc_info - Replace generic man(1) instructions with Debian-specific information.
    DEBPKG:debian/enc2xs_inc - https://bugs.debian.org/290336 Tweak enc2xs to follow symlinks and ignore missing @INC directories.
    DEBPKG:debian/errno_ver - https://bugs.debian.org/343351 Remove Errno version check due to upgrade problems with long-running processes.
    DEBPKG:debian/libperl_embed_doc - https://bugs.debian.org/186778 Note that libperl-dev package is required for embedded linking
    DEBPKG:fixes/respect_umask - Respect umask during installation
    DEBPKG:debian/writable_site_dirs - Set umask approproately for site install directories
    DEBPKG:debian/extutils_set_libperl_path - EU:MM: set location of libperl.a under /usr/lib
    DEBPKG:debian/no_packlist_perllocal - Don't install .packlist or perllocal.pod for perl or vendor
    DEBPKG:debian/fakeroot - Postpone LD_LIBRARY_PATH evaluation to the binary targets.
    DEBPKG:debian/instmodsh_doc - Debian policy doesn't install .packlist files for core or vendor.
    DEBPKG:debian/ld_run_path - Remove standard libs from LD_RUN_PATH as per Debian policy.
    DEBPKG:debian/libnet_config_path - Set location of libnet.cfg to /etc/perl/Net as /usr may not be writable.
    DEBPKG:debian/perlivp - https://bugs.debian.org/510895 Make perlivp skip include directories in /usr/local
    DEBPKG:debian/squelch-locale-warnings - https://bugs.debian.org/508764 Squelch locale warnings in Debian package maintainer scripts
    DEBPKG:debian/patchlevel - https://bugs.debian.org/567489 List packaged patches for 5.36.0-9 in patchlevel.h
    DEBPKG:fixes/document_makemaker_ccflags - https://bugs.debian.org/628522 [rt.cpan.org #68613] Document that CCFLAGS should include $Config{ccflags}
    DEBPKG:debian/find_html2text - https://bugs.debian.org/640479 Configure CPAN::Distribution with correct name of html2text
    DEBPKG:debian/perl5db-x-terminal-emulator.patch - https://bugs.debian.org/668490 Invoke x-terminal-emulator rather than xterm in perl5db.pl
    DEBPKG:debian/cpan-missing-site-dirs - https://bugs.debian.org/688842 Fix CPAN::FirstTime defaults with nonexisting site dirs if a parent is writable
    DEBPKG:fixes/memoize_storable_nstore - [rt.cpan.org #77790] https://bugs.debian.org/587650 Memoize::Storable: respect 'nstore' option not respected
    DEBPKG:debian/makemaker-pasthru - https://bugs.debian.org/758471 Pass LD settings through to subdirectories
    DEBPKG:debian/makemaker-manext - https://bugs.debian.org/247370 Make EU::MakeMaker honour MANnEXT settings in generated manpage headers
    DEBPKG:debian/kfreebsd-softupdates - https://bugs.debian.org/796798 Work around Debian Bug#796798
    DEBPKG:fixes/memoize-pod - [rt.cpan.org #89441] Fix POD errors in Memoize
    DEBPKG:debian/hurd-softupdates - https://bugs.debian.org/822735 Fix t/op/stat.t failures on hurd
    DEBPKG:fixes/math_complex_doc_great_circle - https://bugs.debian.org/697567 [rt.cpan.org #114104] Math::Trig: clarify definition of great_circle_midpoint
    DEBPKG:fixes/math_complex_doc_see_also - https://bugs.debian.org/697568 [rt.cpan.org #114105] Math::Trig: add missing SEE ALSO
    DEBPKG:fixes/math_complex_doc_angle_units - https://bugs.debian.org/731505 [rt.cpan.org #114106] Math::Trig: document angle units
    DEBPKG:fixes/cpan_web_link - https://bugs.debian.org/367291 CPAN: Add link to main CPAN web site
    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
    DEBPKG:debian/installman-utf8 - https://bugs.debian.org/840211 Generate man pages with UTF-8 characters
    DEBPKG:debian/hppa_opmini_optimize_workaround - https://bugs.debian.org/869122 Lower the optimization level of opmini.c on hppa
    DEBPKG:debian/sh4_op_optimize_workaround - https://bugs.debian.org/869373 Also lower the optimization level of op.c and opmini.c on sh4
    DEBPKG:debian/perldoc-pager - https://bugs.debian.org/870340 [rt.cpan.org #120229] Fix perldoc terminal escapes when sensible-pager is less
    DEBPKG:debian/prune_libs - https://bugs.debian.org/128355 Prune the list of libraries wanted to what we actually need.
    DEBPKG:debian/mod_paths - Tweak @INC ordering for Debian
    DEBPKG:debian/deprecate-with-apt - https://bugs.debian.org/747628 Point users to Debian packages of deprecated core modules
    DEBPKG:debian/disable-stack-check - https://bugs.debian.org/902779 [GH #16607] Disable debugperl stack extension checks for binary compatibility with perl
    DEBPKG:debian/perlbug-editor - https://bugs.debian.org/922609 Use "editor" as the default perlbug editor, as per Debian policy
    DEBPKG:debian/eu-mm-perl-base - https://bugs.debian.org/962138 Suppress an ExtUtils::MakeMaker warning about our non-default @INC
    DEBPKG:fixes/io_socket_ip_ipv6 - Disable getaddrinfo(3) AI_ADDRCONFIG for localhost and IPv4 numeric addresses
    DEBPKG:debian/usrmerge-lib64 - https://bugs.debian.org/914128 Configure / libpth.U: Do not adjust glibpth when /usr/lib64 is present.
    DEBPKG:debian/usrmerge-realpath - https://bugs.debian.org/914128 Configure / libpth.U: use realpath --no-symlinks on Debian
    DEBPKG:debian/configure-regen - https://bugs.debian.org/762638 Regenerate Configure et al. after probe unit changes
    DEBPKG:fixes/x32-io-msg-skip - https://bugs.debian.org/922609 Skip io/msg.t on x32 due to broken System V message queues
    DEBPKG:debian/hurd-eumm-workaround - https://bugs.debian.org/1018289 Work around a MakeMaker regression breaking GNU/Hurd hint files
    DEBPKG:fixes/json-pp-warnings - https://bugs.debian.org/1019757 Call unimport first to silence warnings
    DEBPKG:fixes/readline-stream-errors - [80c1f1e] [GH #6799] https://bugs.debian.org/1016369 only clear the stream error state in readline() for glob()
    DEBPKG:fixes/readline-stream-errors-test - [0b60216] [GH #6799] https://bugs.debian.org/1016369 test that <> doesn't clear the stream error state
    DEBPKG:fixes/lto-test-fix - [69b4fa3] [GH #20518] https://bugs.debian.org/1015579 skip checking categorization of libperl symbols for LTO builds
  Built under linux
  Compiled at Sep  9 2023 16:19:46
  @INC:
    /etc/perl
    /usr/local/lib/x86_64-linux-gnu/perl/5.36.0
    /usr/local/share/perl/5.36.0
    /usr/lib/x86_64-linux-gnu/perl5/5.36
    /usr/share/perl5
    /usr/lib/x86_64-linux-gnu/perl-base
    /usr/lib/x86_64-linux-gnu/perl/5.36
    /usr/share/perl/5.36
    /usr/local/lib/site_perl
```
What are yours?

For the sendemail-validate hook itself, I managed to trigger this regression with simple helloworld script:

``` #!/bin/bash

echo "patching..." && exit 0 ```

Thanks.
-- 
An old man doll... just what I always wanted! - Clara
Jeff King· Sep 25, 2023, 08:00 UTC · re: Bagas Sanjaya · lore

Re: [REGRESSION] uninitialized value $address in git send-email when given multiple recipients separated by commas

On Mon, Sep 25, 2023 at 02:45:47PM +0700, Bagas Sanjaya wrote:
Show 19 quoted lines
> On Sat, Sep 23, 2023 at 11:36:25PM -0400, Jeff King wrote:
> > Your report also mentions a validation hook, so I tried installing one
> > like:
> > 
> > 	cat >.git/hooks/sendemail-validate <<-\EOF
> > 	#!/bin/sh
> > 	echo >&2 running validate hook
> > 	exit 0
> > 	EOF
> > 	chmod +x .git/hooks/sendemail-validate
> > 
> > and confirmed that the hook runs (three times, as expected). But still
> > no error. I'm using v2.41.0 to test against.
> > 
> 
> Hi Jeff,
> 
> I think you missed perl version. As stated earlier, I'm on Debian testing
> with perl v5.36.0. On there, `perl -V` outputs:

Mine is the same (I'm on Debian unstable, but the version is currently the same as the one on testing).

Show 8 quoted lines
> For the sendemail-validate hook itself, I managed to trigger this regression
> with simple helloworld script:
> 
> ```
> #!/bin/bash
> 
> echo "patching..." && exit 0
> ```

I think that's equivalent to what I was using (and certainly using yours verbatim does not change anything on my end).

Do you have any other send-email related config? Can you show us the output of "git config --list"?

-Peff
Todd Zullinger· Sep 25, 2023, 14:48 UTC · re: Jeff King · lore

Re: [REGRESSION] uninitialized value $address in git send-email when given multiple recipients separated by commas

Hi,
Jeff King wrote:
Show 6 quoted lines
> On Mon, Sep 25, 2023 at 02:45:47PM +0700, Bagas Sanjaya wrote:
>> I think you missed perl version. As stated earlier, I'm on Debian testing
>> with perl v5.36.0. On there, `perl -V` outputs:
> 
> Mine is the same (I'm on Debian unstable, but the version is currently
> the same as the one on testing).
[...]
> Do you have any other send-email related config? Can you show us the
> output of "git config --list"?

From the peanut gallery... could the presence or lack of the Email::Valid perl module be a factor?

-- 
Todd
Jeff King· Sep 25, 2023, 16:17 UTC · re: Todd Zullinger · lore

Re: [REGRESSION] uninitialized value $address in git send-email when given multiple recipients separated by commas

On Mon, Sep 25, 2023 at 10:48:29AM -0400, Todd Zullinger wrote:
> From the peanut gallery... could the presence or lack of the
> Email::Valid perl module be a factor?

Ah, thanks! The thought of differing modules even occurred to me, since I know we have a few optimistic dependencies, but when I looked I didn't manage to find that one (somehow I thought Mail::Address was the interesting one here; I think I might be getting senile).

With Email::Valid installed, I can reproduce with just (in git.git, but I think it would work in any repo):

  $ echo "exit 0" >.git/hooks/sendemail-validate
  $ chmod +x .git/hooks/sendemail-validate
  $ git send-email --dry-run -1 --to=foo@example.com,bar@example.com
  error: unable to extract a valid address from: foo@example.com,bar@example.com

Disabling the hook with "chmod -x" makes the problem go away (and this is with current "master", hence the more readable error message).

I think the issue is that a8022c5f7b ends up in extract_valid_address() via this call stack:

  $ = main::extract_valid_address('foo@example.com,bar@example.com') called from file '/home/peff/compile/git/git-send-email' line 1161
  $ = main::extract_valid_address_or_die('foo@example.com,bar@example.com') called from file '/home/peff/compile/git/git-send-email' line 2087
  @ = main::unique_email_list('foo@example.com,bar@example.com') called from file '/home/peff/compile/git/git-send-email' line 1507
  @ = main::gen_header() called from file '/home/peff/compile/git/git-send-email' line 2113
  . = main::validate_patch('/tmp/WfoPQSKCUa/0001-The-twelfth-batch.patch', 'auto') called from file '/home/peff/compile/git/git-send-email' line 815
whereas prior to that commit, we hit it later:
  $ = main::extract_valid_address('foo@example.com') called from file '/home/peff/compile/git/git-send-email' line 1166
  @ = main::validate_address('foo@example.com') called from file '/home/peff/compile/git/git-send-email' line 1189
  @ = main::validate_address_list('foo@example.com', 'bar@example.com') called from file '/home/peff/compile/git/git-send-email' line 1348
  @ = main::process_address_list('foo@example.com,bar@example.com') called from file '/home/peff/compile/git/git-send-email' line 1091

So the issue is the call to gen_header() added in validate_patch(). We won't yet have processed the address lists by that point. We can move those calls up, but it requires moving a bit of extra code, too (like the parts prompting for the "to" list if it isn't filled in).

Possibly the validation checks need to be moved down, if they want to see a more complete view of the emails. But now we're doing more work (like asking the user to write the cover letter!) before we do validation, which is probably bad.

So I dunno. Maybe gen_header() should be lazily doing this process_address_list() stuff? I'm not very familiar with the send-email code, so I'm not sure what secondary effects that could have.

-Peff
Bagas Sanjaya· Oct 11, 2023, 13:41 UTC · re: Jeff King · lore

Re: [REGRESSION] uninitialized value $address in git send-email when given multiple recipients separated by commas

On Mon, Sep 25, 2023 at 12:17:48PM -0400, Jeff King wrote:
Show 51 quoted lines
> On Mon, Sep 25, 2023 at 10:48:29AM -0400, Todd Zullinger wrote:
> 
> > From the peanut gallery... could the presence or lack of the
> > Email::Valid perl module be a factor?
> 
> Ah, thanks! The thought of differing modules even occurred to me, since
> I know we have a few optimistic dependencies, but when I looked I didn't
> manage to find that one (somehow I thought Mail::Address was the
> interesting one here; I think I might be getting senile).
> 
> With Email::Valid installed, I can reproduce with just (in git.git, but
> I think it would work in any repo):
> 
>   $ echo "exit 0" >.git/hooks/sendemail-validate
>   $ chmod +x .git/hooks/sendemail-validate
>   $ git send-email --dry-run -1 --to=foo@example.com,bar@example.com
>   error: unable to extract a valid address from: foo@example.com,bar@example.com
> 
> Disabling the hook with "chmod -x" makes the problem go away (and this
> is with current "master", hence the more readable error message).
> 
> I think the issue is that a8022c5f7b ends up in extract_valid_address()
> via this call stack:
> 
>   $ = main::extract_valid_address('foo@example.com,bar@example.com') called from file '/home/peff/compile/git/git-send-email' line 1161
>   $ = main::extract_valid_address_or_die('foo@example.com,bar@example.com') called from file '/home/peff/compile/git/git-send-email' line 2087
>   @ = main::unique_email_list('foo@example.com,bar@example.com') called from file '/home/peff/compile/git/git-send-email' line 1507
>   @ = main::gen_header() called from file '/home/peff/compile/git/git-send-email' line 2113
>   . = main::validate_patch('/tmp/WfoPQSKCUa/0001-The-twelfth-batch.patch', 'auto') called from file '/home/peff/compile/git/git-send-email' line 815
> 
> whereas prior to that commit, we hit it later:
> 
>   $ = main::extract_valid_address('foo@example.com') called from file '/home/peff/compile/git/git-send-email' line 1166
>   @ = main::validate_address('foo@example.com') called from file '/home/peff/compile/git/git-send-email' line 1189
>   @ = main::validate_address_list('foo@example.com', 'bar@example.com') called from file '/home/peff/compile/git/git-send-email' line 1348
>   @ = main::process_address_list('foo@example.com,bar@example.com') called from file '/home/peff/compile/git/git-send-email' line 1091
> 
> So the issue is the call to gen_header() added in validate_patch(). We
> won't yet have processed the address lists by that point. We can move
> those calls up, but it requires moving a bit of extra code, too (like
> the parts prompting for the "to" list if it isn't filled in).
> 
> Possibly the validation checks need to be moved down, if they want to
> see a more complete view of the emails. But now we're doing more work
> (like asking the user to write the cover letter!) before we do
> validation, which is probably bad.
> 
> So I dunno. Maybe gen_header() should be lazily doing this
> process_address_list() stuff? I'm not very familiar with the send-email
> code, so I'm not sure what secondary effects that could have.
> 
Michael, did you look into this since you authored the culprit?
-- 
An old man doll... just what I always wanted! - Clara
Michael Strawbridge· Oct 11, 2023, 19:27 UTC · re: Bagas Sanjaya · lore

Re: [REGRESSION] uninitialized value $address in git send-email when given multiple recipients separated by commas

On 10/11/23 09:41, Bagas Sanjaya wrote:
Show 53 quoted lines
> On Mon, Sep 25, 2023 at 12:17:48PM -0400, Jeff King wrote:
>> On Mon, Sep 25, 2023 at 10:48:29AM -0400, Todd Zullinger wrote:
>>
>>> From the peanut gallery... could the presence or lack of the
>>> Email::Valid perl module be a factor?
>> Ah, thanks! The thought of differing modules even occurred to me, since
>> I know we have a few optimistic dependencies, but when I looked I didn't
>> manage to find that one (somehow I thought Mail::Address was the
>> interesting one here; I think I might be getting senile).
>>
>> With Email::Valid installed, I can reproduce with just (in git.git, but
>> I think it would work in any repo):
>>
>>   $ echo "exit 0" >.git/hooks/sendemail-validate
>>   $ chmod +x .git/hooks/sendemail-validate
>>   $ git send-email --dry-run -1 --to=foo@example.com,bar@example.com
>>   error: unable to extract a valid address from: foo@example.com,bar@example.com
>>
>> Disabling the hook with "chmod -x" makes the problem go away (and this
>> is with current "master", hence the more readable error message).
>>
>> I think the issue is that a8022c5f7b ends up in extract_valid_address()
>> via this call stack:
>>
>>   $ = main::extract_valid_address('foo@example.com,bar@example.com') called from file '/home/peff/compile/git/git-send-email' line 1161
>>   $ = main::extract_valid_address_or_die('foo@example.com,bar@example.com') called from file '/home/peff/compile/git/git-send-email' line 2087
>>   @ = main::unique_email_list('foo@example.com,bar@example.com') called from file '/home/peff/compile/git/git-send-email' line 1507
>>   @ = main::gen_header() called from file '/home/peff/compile/git/git-send-email' line 2113
>>   . = main::validate_patch('/tmp/WfoPQSKCUa/0001-The-twelfth-batch.patch', 'auto') called from file '/home/peff/compile/git/git-send-email' line 815
>>
>> whereas prior to that commit, we hit it later:
>>
>>   $ = main::extract_valid_address('foo@example.com') called from file '/home/peff/compile/git/git-send-email' line 1166
>>   @ = main::validate_address('foo@example.com') called from file '/home/peff/compile/git/git-send-email' line 1189
>>   @ = main::validate_address_list('foo@example.com', 'bar@example.com') called from file '/home/peff/compile/git/git-send-email' line 1348
>>   @ = main::process_address_list('foo@example.com,bar@example.com') called from file '/home/peff/compile/git/git-send-email' line 1091
>>
>> So the issue is the call to gen_header() added in validate_patch(). We
>> won't yet have processed the address lists by that point. We can move
>> those calls up, but it requires moving a bit of extra code, too (like
>> the parts prompting for the "to" list if it isn't filled in).
>>
>> Possibly the validation checks need to be moved down, if they want to
>> see a more complete view of the emails. But now we're doing more work
>> (like asking the user to write the cover letter!) before we do
>> validation, which is probably bad.
>>
>> So I dunno. Maybe gen_header() should be lazily doing this
>> process_address_list() stuff? I'm not very familiar with the send-email
>> code, so I'm not sure what secondary effects that could have.
>>
> Michael, did you look into this since you authored the culprit?
>
I 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.
The 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.
Michael Strawbridge· Oct 11, 2023, 20:22 UTC · re: Michael Strawbridge · lore

[PATCH] send-email: move process_address_list earlier to avoid, uninitialized address error

Move processing of email address lists before the sendemail-validate
hook code.  This fixes email address validation errors when the optional
perl module Email::Valid is installed and multiple addresses are passed
in on a single to/cc argument like --to=foo@example.com,bar@example.com.
---
 git-send-email.perl | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/git-send-email.perl b/git-send-email.perl
index 288ea1ae80..cfd80c9d8b 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -799,6 +799,10 @@ sub is_format_patch_arg {
 
 $time = time - scalar $#files;
 
+@initial_to = process_address_list(@initial_to);
+@initial_cc = process_address_list(@initial_cc);
+@initial_bcc = process_address_list(@initial_bcc);
+
 if ($validate) {
        # FIFOs can only be read once, exclude them from validation.
        my @real_files = ();
@@ -1099,10 +1103,6 @@ sub expand_one_alias {
        return $aliases{$alias} ? expand_aliases(@{$aliases{$alias}}) : $alias;
 }
 
-@initial_to = process_address_list(@initial_to);
-@initial_cc = process_address_list(@initial_cc);
-@initial_bcc = process_address_list(@initial_bcc);
-
 if ($thread && !defined $initial_in_reply_to && $prompting) {
        $initial_in_reply_to = ask(
                __("Message-ID to be used as In-Reply-To for the first email (if any)? "),
-- 
2.34.1
Michael Strawbridge· Oct 11, 2023, 20:25 UTC · re: Michael Strawbridge · lore

Re: [PATCH] send-email: move process_address_list earlier to avoid, uninitialized address error

Hi Bagas,
If possible, please try the patch I just sent out and let me know if it works for your situation.
Thanks,
Michael
On 10/11/23 16:22, Michael Strawbridge wrote:
Show 34 quoted lines
> Move processing of email address lists before the sendemail-validate
> hook code.  This fixes email address validation errors when the optional
> perl module Email::Valid is installed and multiple addresses are passed
> in on a single to/cc argument like --to=foo@example.com,bar@example.com.
> ---
>  git-send-email.perl | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/git-send-email.perl b/git-send-email.perl
> index 288ea1ae80..cfd80c9d8b 100755
> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -799,6 +799,10 @@ sub is_format_patch_arg {
>  
>  $time = time - scalar $#files;
>  
> +@initial_to = process_address_list(@initial_to);
> +@initial_cc = process_address_list(@initial_cc);
> +@initial_bcc = process_address_list(@initial_bcc);
> +
>  if ($validate) {
>         # FIFOs can only be read once, exclude them from validation.
>         my @real_files = ();
> @@ -1099,10 +1103,6 @@ sub expand_one_alias {
>         return $aliases{$alias} ? expand_aliases(@{$aliases{$alias}}) : $alias;
>  }
>  
> -@initial_to = process_address_list(@initial_to);
> -@initial_cc = process_address_list(@initial_cc);
> -@initial_bcc = process_address_list(@initial_bcc);
> -
>  if ($thread && !defined $initial_in_reply_to && $prompting) {
>         $initial_in_reply_to = ask(
>                 __("Message-ID to be used as In-Reply-To for the first email (if any)? "),
Junio C Hamano· Oct 11, 2023, 21:27 UTC · re: Michael Strawbridge · lore

Re: [PATCH] send-email: move process_address_list earlier to avoid, uninitialized address error

Michael Strawbridge <michael.strawbridge@amd.com> writes:
Show 8 quoted lines
> @@ -799,6 +799,10 @@ sub is_format_patch_arg {
>  
>  $time = time - scalar $#files;
>  
> +@initial_to = process_address_list(@initial_to);
> +@initial_cc = process_address_list(@initial_cc);
> +@initial_bcc = process_address_list(@initial_bcc);
> +
This does not look OK.  If we trace how @initial_to gets its value,
 - it first gets its value from @getopt_to and @config_to
 - if that is empty, and there is no $to_cmd, the end-user is
   interactively asked.
 - then process_address_list() is applied.

But this patch just swapped the second one and the third one, so process_address_list() does not process what the end-user gave interactively, no?

>  if ($validate) {
>         # FIFOs can only be read once, exclude them from validation.
>         my @real_files = ();

It almost feels like what need to move is not the setting of these address lists, but the code that calls int validation callchain that needs access to these address lists---the block that begins with the above "if ($validate) {" needs to move below ...

Show 8 quoted lines
> @@ -1099,10 +1103,6 @@ sub expand_one_alias {
>         return $aliases{$alias} ? expand_aliases(@{$aliases{$alias}}) : $alias;
>  }
>  
> -@initial_to = process_address_list(@initial_to);
> -@initial_cc = process_address_list(@initial_cc);
> -@initial_bcc = process_address_list(@initial_bcc);
> -
... this point, or something, perhaps?
>  if ($thread && !defined $initial_in_reply_to && $prompting) {
>         $initial_in_reply_to = ask(
>                 __("Message-ID to be used as In-Reply-To for the first email (if any)? "),
Jeff King· Oct 11, 2023, 22:18 UTC · re: Junio C Hamano · lore

Re: [PATCH] send-email: move process_address_list earlier to avoid, uninitialized address error

On Wed, Oct 11, 2023 at 02:27:49PM -0700, Junio C Hamano wrote:
Show 23 quoted lines
> Michael Strawbridge <michael.strawbridge@amd.com> writes:
> 
> > @@ -799,6 +799,10 @@ sub is_format_patch_arg {
> >  
> >  $time = time - scalar $#files;
> >  
> > +@initial_to = process_address_list(@initial_to);
> > +@initial_cc = process_address_list(@initial_cc);
> > +@initial_bcc = process_address_list(@initial_bcc);
> > +
> 
> This does not look OK.  If we trace how @initial_to gets its value,
> 
>  - it first gets its value from @getopt_to and @config_to
> 
>  - if that is empty, and there is no $to_cmd, the end-user is
>    interactively asked.
> 
>  - then process_address_list() is applied.
> 
> But this patch just swapped the second one and the third one, so
> process_address_list() does not process what the end-user gave
> interactively, no?
Yep, that is the issue I found when I dug into this earlier.
Show 8 quoted lines
> >  if ($validate) {
> >         # FIFOs can only be read once, exclude them from validation.
> >         my @real_files = ();
> 
> It almost feels like what need to move is not the setting of these
> address lists, but the code that calls int validation callchain that
> needs access to these address lists---the block that begins with the
> above "if ($validate) {" needs to move below ...

Yes, though then we have the problem that we've asked the user some interactive questions before validating if the input files are bogus or not. Which would be annoying if they aren't valid, because when we barf they've wasted time typing.

Which of course implies that we're not (and cannot) validate what they're typing at this step, but I think that's OK because we feed it through extract_valid_address_or_die(). IOW, I think there are actually two distinct validation steps hidden here:

  1. We want to validate that the patch files we were fed are OK.
  2. We want to validate that the addresses, etc, fed by the user are
     OK.

And after Michael's original patch, we are accidentally hitting some of that validation code for (2) while doing (1).

This is actually a weird split if you think about it. We are feeding to the validate hook in (1), so surely it would want to see the full set of inputs from the user, too? Which argues for pushing the "if ($validate)" down as you suggest. And then either:

  a. We accept that the user experience is a little worse if validation
     fails after the user typed.
  b. We split (1) into "early" validation that just checks if the files
     are OK, but doesn't call the hook. And then later on we do the full
     validation.

I don't have a strong opinion myself (I don't even use send-email myself, and if I did, I'd probably mostly be feeding it with "--to" etc on the command line, rather than interactively).

-Peff
Junio C Hamano· Oct 11, 2023, 22:37 UTC · re: Jeff King · lore

Re: [PATCH] send-email: move process_address_list earlier to avoid, uninitialized address error

Jeff King <peff@peff.net> writes:
> Which of course implies that we're not (and cannot) validate what
> they're typing at this step, but I think that's OK because we feed it
> through extract_valid_address_or_die().
OK, let's queue it then.
Show 10 quoted lines
> IOW, I think there are actually two distinct validation steps
> hidden here:
>
>   1. We want to validate that the patch files we were fed are OK.
>
>   2. We want to validate that the addresses, etc, fed by the user are
>      OK.
>
> And after Michael's original patch, we are accidentally hitting some of
> that validation code for (2) while doing (1).
Show 15 quoted lines
> This is actually a weird split if you think about it. We are feeding to
> the validate hook in (1), so surely it would want to see the full set of
> inputs from the user, too? Which argues for pushing the "if ($validate)"
> down as you suggest. And then either:
>
>   a. We accept that the user experience is a little worse if validation
>      fails after the user typed.
>
>   b. We split (1) into "early" validation that just checks if the files
>      are OK, but doesn't call the hook. And then later on we do the full
>      validation.
>
> I don't have a strong opinion myself (I don't even use send-email
> myself, and if I did, I'd probably mostly be feeding it with "--to" etc
> on the command line, rather than interactively).

I am not affected, either, and do not have a strong opinion either way. As long as the end-user input is validated separately, it would be OK, but if the end-user supplied validation hook cares about what addresses the messages are going to be sent to, not knowing the set of recipients mean the validation hook is not getting the whole picture, which does smell bad.

On the other hand, I am not sure what is wrong with "after the user typed", actually. As you said, anybody sane would be using --to (or an equivalent configuration variable in the repository) to send their patches to the project address instead of typing, and to them it is not a problem. After getting the recipient address from the end user, the validation may fail due to a wrong address, in which case it is a good thing. If the validation failed due to wrong contents of the patch (perhaps it included a change to the file with trade secret that appeared in the context lines), as long as the reason why the validation hook rejected the patches is clear enough (e.g., "it's the patches, not the recipients"), such "a rejection after typing" would be only once per a patch series, so it does not sound too bad, either.

But perhaps I am not seeing the reason why "fail after the user typed" is so disliked and being unnecessarily unsympathetic. I dunno.

Jeff King· Oct 11, 2023, 22:47 UTC · re: Junio C Hamano · lore

Re: [PATCH] send-email: move process_address_list earlier to avoid, uninitialized address error

On Wed, Oct 11, 2023 at 03:37:39PM -0700, Junio C Hamano wrote:
Show 16 quoted lines
> On the other hand, I am not sure what is wrong with "after the user
> typed", actually.  As you said, anybody sane would be using --to (or
> an equivalent configuration variable in the repository) to send
> their patches to the project address instead of typing, and to them
> it is not a problem.  After getting the recipient address from the
> end user, the validation may fail due to a wrong address, in which
> case it is a good thing.  If the validation failed due to wrong
> contents of the patch (perhaps it included a change to the file with
> trade secret that appeared in the context lines), as long as the
> reason why the validation hook rejected the patches is clear enough
> (e.g., "it's the patches, not the recipients"), such "a rejection
> after typing" would be only once per a patch series, so it does not
> sound too bad, either.
> 
> But perhaps I am not seeing the reason why "fail after the user typed"
> is so disliked and being unnecessarily unsympathetic.  I dunno.

I did not look carefully at the flow of send-email, so this may or may not be an issue. But what I think would be _really_ annoying is if you asked to write a cover letter, went through the trouble of writing it, and then send-email bailed due to some validation failure that could have been checked earlier.

There is probably a way to recover your work (presumably we leave it in a temporary file somewhere), but it may not be entirely trivial, especially for users who are not comfortable with advanced usage of their editor. ;)

I seem to remember we had one or two such problems in the early days with "git commit", where you would go to the trouble to type a commit message only to bail on some condition which _could_ have been checked earlier. You can recover the message from .git/COMMIT_EDITMSG, but you need to remember to do so before re-invoking "git commit", otherwise it gets obliterated.

Now for send-email, if your flow is to generate the patches with "format-patch", then edit the cover letter separately, and then finally ship it all out with "send-email", that might not be an issue. But some workflows use the --compose option instead.

-Peff
Michael Strawbridge· Oct 13, 2023, 20:25 UTC · re: Jeff King · lore

Re: [PATCH] send-email: move process_address_list earlier to avoid, uninitialized address error

On 10/11/23 18:47, Jeff King wrote:
Show 28 quoted lines
> On Wed, Oct 11, 2023 at 03:37:39PM -0700, Junio C Hamano wrote:
>
>> On the other hand, I am not sure what is wrong with "after the user
>> typed", actually.  As you said, anybody sane would be using --to (or
>> an equivalent configuration variable in the repository) to send
>> their patches to the project address instead of typing, and to them
>> it is not a problem.  After getting the recipient address from the
>> end user, the validation may fail due to a wrong address, in which
>> case it is a good thing.  If the validation failed due to wrong
>> contents of the patch (perhaps it included a change to the file with
>> trade secret that appeared in the context lines), as long as the
>> reason why the validation hook rejected the patches is clear enough
>> (e.g., "it's the patches, not the recipients"), such "a rejection
>> after typing" would be only once per a patch series, so it does not
>> sound too bad, either.
>>
>> But perhaps I am not seeing the reason why "fail after the user typed"
>> is so disliked and being unnecessarily unsympathetic.  I dunno.
> I did not look carefully at the flow of send-email, so this may or may
> not be an issue. But what I think would be _really_ annoying is if you
> asked to write a cover letter, went through the trouble of writing it,
> and then send-email bailed due to some validation failure that could
> have been checked earlier.
>
> There is probably a way to recover your work (presumably we leave it in
> a temporary file somewhere), but it may not be entirely trivial,
> especially for users who are not comfortable with advanced usage of
> their editor. ;)
As 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.
Show 14 quoted lines
>
> I seem to remember we had one or two such problems in the early days
> with "git commit", where you would go to the trouble to type a commit
> message only to bail on some condition which _could_ have been checked
> earlier. You can recover the message from .git/COMMIT_EDITMSG, but you
> need to remember to do so before re-invoking "git commit", otherwise it
> gets obliterated.
>
> Now for send-email, if your flow is to generate the patches with
> "format-patch", then edit the cover letter separately, and then finally
> ship it all out with "send-email", that might not be an issue. But some
> workflows use the --compose option instead.
>
> -Peff
I 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.
Jeff King· Oct 20, 2023, 06:45 UTC · re: Michael Strawbridge · lore

Re: [PATCH] send-email: move process_address_list earlier to avoid, uninitialized address error

On Fri, Oct 13, 2023 at 04:25:57PM -0400, Michael Strawbridge wrote:
Show 18 quoted lines
> > I did not look carefully at the flow of send-email, so this may or may
> > not be an issue. But what I think would be _really_ annoying is if you
> > asked to write a cover letter, went through the trouble of writing it,
> > and then send-email bailed due to some validation failure that could
> > have been checked earlier.
> >
> > There is probably a way to recover your work (presumably we leave it in
> > a temporary file somewhere), but it may not be entirely trivial,
> > especially for users who are not comfortable with advanced usage of
> > their editor. ;)
>
> As 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.

OK, that makes me feel better about just moving the validation code. A more complicated solution could be do to do _some_ basic checks up front, and then more complete validation later. But even if we wanted to do that, moving the bulk of the validation (as discussed in this thread) would probably be the first step anyway (and if nobody complains, maybe we can avoid doing the extra work).

I do wonder if we might find other interesting corner cases where the validation code (or somebody's hook) isn't happy with seeing the more "full" picture (i.e., with the extra addresses from interactive and command-line input). But arguably any such case would be indicative of a bug, and smoking it out would be a good thing.

Show 6 quoted lines
> I 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.

Sounds like something is making a perl ref that shouldn't (or something that should be dereferencing it not doing so). If you post your patch and a reproduction command, I might be able to help debug.

But just blindly moving the validation code down, like:
diff --git a/git-send-email.perl b/git-send-email.perl
index 288ea1ae80..76589c7827 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -799,30 +799,6 @@ sub is_format_patch_arg {
 
 $time = time - scalar $#files;
 
-if ($validate) {
-	# FIFOs can only be read once, exclude them from validation.
-	my @real_files = ();
-	foreach my $f (@files) {
-		unless (-p $f) {
-			push(@real_files, $f);
-		}
-	}
-
-	# Run the loop once again to avoid gaps in the counter due to FIFO
-	# arguments provided by the user.
-	my $num = 1;
-	my $num_files = scalar @real_files;
-	$ENV{GIT_SENDEMAIL_FILE_TOTAL} = "$num_files";
-	foreach my $r (@real_files) {
-		$ENV{GIT_SENDEMAIL_FILE_COUNTER} = "$num";
-		pre_process_file($r, 1);
-		validate_patch($r, $target_xfer_encoding);
-		$num += 1;
-	}
-	delete $ENV{GIT_SENDEMAIL_FILE_COUNTER};
-	delete $ENV{GIT_SENDEMAIL_FILE_TOTAL};
-}
-
 @files = handle_backup_files(@files);
 
 if (@files) {
@@ -1121,6 +1097,30 @@ sub expand_one_alias {
 	$reply_to = sanitize_address($reply_to);
 }
 
+if ($validate) {
+	# FIFOs can only be read once, exclude them from validation.
+	my @real_files = ();
+	foreach my $f (@files) {
+		unless (-p $f) {
+			push(@real_files, $f);
+		}
+	}
+
+	# Run the loop once again to avoid gaps in the counter due to FIFO
+	# arguments provided by the user.
+	my $num = 1;
+	my $num_files = scalar @real_files;
+	$ENV{GIT_SENDEMAIL_FILE_TOTAL} = "$num_files";
+	foreach my $r (@real_files) {
+		$ENV{GIT_SENDEMAIL_FILE_COUNTER} = "$num";
+		pre_process_file($r, 1);
+		validate_patch($r, $target_xfer_encoding);
+		$num += 1;
+	}
+	delete $ENV{GIT_SENDEMAIL_FILE_COUNTER};
+	delete $ENV{GIT_SENDEMAIL_FILE_TOTAL};
+}
+
 if (!defined $sendmail_cmd && !defined $smtp_server) {
 	my @sendmail_paths = qw( /usr/sbin/sendmail /usr/lib/sendmail );
 	push @sendmail_paths, map {"$_/sendmail"} split /:/, $ENV{PATH};

seems to fix the problem from this thread and passes the existing tests.
Manually inspecting the result (and what's fed to the validation hook) I
don't see anything odd (like "ARRAY (...)").

-Peff
Jeff King· Oct 20, 2023, 07:14 UTC · re: Jeff King · lore

Re: [PATCH] send-email: move process_address_list earlier to avoid, uninitialized address error

On Fri, Oct 20, 2023 at 02:45:25AM -0400, Jeff King wrote:
Show 10 quoted lines
> > I 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.
> 
> Sounds like something is making a perl ref that shouldn't (or something
> that should be dereferencing it not doing so). If you post your patch
> and a reproduction command, I might be able to help debug.
Ah, your "address I wrote in the compose buffer" was the clue I needed.
I think this is actually an existing bug. If I use --compose and write:
  To: foo@example.com

in the editor, we read that back in and handle it in parse_header_line() like:

        my $addr_pat = join "|", qw(To Cc Bcc);
        foreach (split(/\n/, $lines)) {
                if (/^($addr_pat):\s*(.+)$/i) {
                        $parsed_line->{$1} = [ parse_address_line($2) ];
                } elsif (/^([^:]*):\s*(.+)\s*$/i) {
                        $parsed_line->{$1} = $2;
                }
        }

and there's your perl array ref (from the square brackets, which are necessary because we're sticking it in a hash value). But even before your patch, this seems to end up as garbage. The code which reads $parsed_line does not dereference the array.

The patch to fix it is only a few lines (well, more than that with some light editorializing in the comments):

diff --git a/git-send-email.perl b/git-send-email.perl
index 76589c7827..46a30088c9 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -918,7 +918,28 @@ sub get_patch_subject {
 	# Preserve unknown headers
 	foreach my $key (keys %parsed_email) {
 		next if $key eq 'body';
-		print $c2 "$key: $parsed_email{$key}";
+
+		# it seems like it would be easier to just look for
+		# $parsed_email{'To'} and so on. But we actually match
+		# these case-insenstively and preserve the user's spelling, so
+		# we might see $parsed_email{'to'}. Of course, the same bug
+		# exists for Subject, etc, above. Anyway, a "/i" regex here
+		# handles all cases.
+		#
+		# It kind of feels like all of this code would be much simpler
+		# if we just handled all of the headers while reading back the
+		# input, rather than stuffing them all into $parsed_email and
+		# then picking them out of it.
+		#
+		# It also really feels like these to/cc/bcc lines should be
+		# added to the regular ones? It is silly to make a cover letter
+		# that goes to some addresses, and then not send the patches to
+		# them, too.
+		if ($key =~ /^(To|Cc|Bcc)$/i) {
+			print $c2 "$key: ", join(', ', @{$parsed_email{$key}});
+		} else {
+			print $c2 "$key: $parsed_email{$key}";
+		}
 	}
 
 	if ($parsed_email{'body'}) {

I don't really think your patch makes things worse here. But it is
probably worth fixing it while we are here.

-Peff
Jeff King· Oct 20, 2023, 10:03 UTC · re: Jeff King · lore

[PATCH 0/3] some send-email --compose fixes

[culling the rather large cc, as we moving off the original topic]
On Fri, Oct 20, 2023 at 03:14:03AM -0400, Jeff King wrote:
Show 7 quoted lines
> and there's your perl array ref (from the square brackets, which are
> necessary because we're sticking it in a hash value). But even before
> your patch, this seems to end up as garbage. The code which reads
> $parsed_line does not dereference the array.
> 
> The patch to fix it is only a few lines (well, more than that with some
> light editorializing in the comments):

So here's the fix in a cleaned up form, guided by my own comments from earlier. ;) I think this is actually all orthogonal to the patch you are working on, so yours could either go on top or just be applied separately.

  [1/3]: doc/send-email: mention handling of "reply-to" with --compose
  [2/3]: Revert "send-email: extract email-parsing code into a subroutine"
  [3/3]: send-email: handle to/cc/bcc from --compose message
 Documentation/git-send-email.txt |  10 +--
 git-send-email.perl              | 132 ++++++++++++-------------------
 t/t9001-send-email.sh            |  41 ++++++++++
 3 files changed, 98 insertions(+), 85 deletions(-)
-Peff
Jeff King· Oct 20, 2023, 10:09 UTC · re: Jeff King · lore

[PATCH 1/3] doc/send-email: mention handling of "reply-to" with --compose

The documentation for git-send-email lists the headers handled specially by --compose in a way that implies that this is the complete set of headers that are special. But one more was added by d11c943c78 (send-email: support separate Reply-To address, 2018-03-04) and never documented.

Let's add it, and reword the documentation slightly to avoid having to specify the list of headers twice (as it is growing and will continue to do so as we add new features).

If you read the code, you may notice that we also handle MIME-Version specially, in that we'll avoid over-writing user-provided MIME headers. I don't think this is worth mentioning, as it's what you'd expect to happen (as opposed to the other headers, which are picked up to be used in later emails). And certainly this feature existed when the documentation was expanded in 01d3861217 (git-send-email.txt: describe --compose better, 2009-03-16), and we chose not to mention it then.

Signed-off-by: Jeff King <peff@peff.net>
---
Just something I noticed since a later commit touches the same list.
 Documentation/git-send-email.txt | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt
index 492a82323d..021276329c 100644
--- a/Documentation/git-send-email.txt
+++ b/Documentation/git-send-email.txt
@@ -68,11 +68,11 @@ This option may be specified multiple times.
 	Invoke a text editor (see GIT_EDITOR in linkgit:git-var[1])
 	to edit an introductory message for the patch series.
 +
-When `--compose` is used, git send-email will use the From, Subject, and
-In-Reply-To headers specified in the message. If the body of the message
-(what you type after the headers and a blank line) only contains blank
-(or Git: prefixed) lines, the summary won't be sent, but From, Subject,
-and In-Reply-To headers will be used unless they are removed.
+When `--compose` is used, git send-email will use the From, Subject,
+Reply-To, and In-Reply-To headers specified in the message. If the body
+of the message (what you type after the headers and a blank line) only
+contains blank (or Git: prefixed) lines, the summary won't be sent, but
+the headers mentioned above will be used unless they are removed.
 +
 Missing From or In-Reply-To headers will be prompted for.
 +
-- 
2.42.0.980.g8b5f6199be
Jeff King· Oct 20, 2023, 10:13 UTC · re: Jeff King · lore

[PATCH 2/3] Revert "send-email: extract email-parsing code into a subroutine"

This reverts commit b6049542b97e7b135e0e82bf996084d461224d32.

Prior to that commit, we read the results of the user editing the "--compose" message in a loop, picking out parts we cared about, and streaming the result out to a ".final" file. That commit split the reading/interpreting into two phases; we'd now read into a hash, and then pick things out of the hash.

The goal was making the code more readable. And in some ways it did, because the ugly regexes are confined to the reading phase. But it also introduced several bugs, because now the two phases need to match each other. In particular:

  - we pick out headers like "Subject: foo" with a case-insensitive
    regex, and then use the user-provided header name as the key in a
    case-sensitive hash. So if the user wrote "subject: foo", we'd no
    longer recognize it as a subject.
  - the namespace for the hash keys conflates header names with meta
    information like "body". If you put "body: foo" in your message, it
    would be misinterpreted as the actual message body (nobody is likely
    to do that in practice, but it seems like an unnecessary danger).
  - the handling for to/cc/bcc is totally broken. The behavior before
    that commit is to recognize and skip those headers, with a note to
    the user that they are not yet handled. Not great, but OK. But
    after the patch, the reading side now splits the addresses into a
    perl array-ref. But the interpreting side doesn't handle this at
    all, and blindly prints the stringified array-ref value. This leads
    to garbage like:
      (mbox) Adding to: ARRAY (0x555b4345c428) from line 'To: ARRAY(0x555b4345c428)'
      error: unable to extract a valid address from: ARRAY (0x555b4345c428)
      What to do with this address? ([q]uit|[d]rop|[e]dit):
    Probably not a huge deal, since nobody should even try to use those
    headers in the first place (since they were not implemented). But
    the new behavior is worse, and indicative of the sorts of problems
    that come from having the two layers.

The revert had a few conflicts, due to later work in this area from 15dc3b9161 (send-email: rename variable for clarity, 2018-03-04) and d11c943c78 (send-email: support separate Reply-To address, 2018-03-04). I've ported the changes from those commits over as part of the conflict resolution.

The new tests show the bugs. Note the use of GIT_SEND_EMAIL_NOTTY in the second one. Without it, the test is happy to reach outside the test harness to the developer's actual terminal (when run with the buggy state before this patch).

Signed-off-by: Jeff King <peff@peff.net>
---
I guess "readable" is up for debate here, but I find the inline handling
a lot easier to follow (and it's half as many lines; most of the
diffstat is the new tests).

But one thing that gives me pause is that the neither before or after this patch do we handle continuation lines like:

  Subject: this is the beginning
    and this is more subject

And it would probably be a lot easier to add when storing the headers in a hash (it's not impossible to do it the other way, but you basically have to delay processing each line with a small state machine).

So another option is to just fix the individual bugs separately.
 git-send-email.perl   | 120 ++++++++++++++----------------------------
 t/t9001-send-email.sh |  35 ++++++++++++
 2 files changed, 75 insertions(+), 80 deletions(-)
diff --git a/git-send-email.perl b/git-send-email.perl
index 288ea1ae80..bbda2a931b 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -888,73 +888,59 @@ sub get_patch_subject {
 		do_edit($compose_filename);
 	}
 
+	open my $c2, ">", $compose_filename . ".final"
+		or die sprintf(__("Failed to open %s.final: %s"), $compose_filename, $!);
+
 	open $c, "<", $compose_filename
 		or die sprintf(__("Failed to open %s: %s"), $compose_filename, $!);
 
+	my $need_8bit_cte = file_has_nonascii($compose_filename);
+	my $in_body = 0;
+	my $summary_empty = 1;
 	if (!defined $compose_encoding) {
 		$compose_encoding = "UTF-8";
 	}
-
-	my %parsed_email;
-	while (my $line = <$c>) {
-		next if $line =~ m/^GIT:/;
-		parse_header_line($line, \%parsed_email);
-		if ($line =~ /^$/) {
-			$parsed_email{'body'} = filter_body($c);
+	while(<$c>) {
+		next if m/^GIT:/;
+		if ($in_body) {
+			$summary_empty = 0 unless (/^\n$/);
+		} elsif (/^\n$/) {
+			$in_body = 1;
+			if ($need_8bit_cte) {
+				print $c2 "MIME-Version: 1.0\n",
+					 "Content-Type: text/plain; ",
+					   "charset=$compose_encoding\n",
+					 "Content-Transfer-Encoding: 8bit\n";
+			}
+		} elsif (/^MIME-Version:/i) {
+			$need_8bit_cte = 0;
+		} elsif (/^Subject:\s*(.+)\s*$/i) {
+			$initial_subject = $1;
+			my $subject = $initial_subject;
+			$_ = "Subject: " .
+				quote_subject($subject, $compose_encoding) .
+				"\n";
+		} elsif (/^In-Reply-To:\s*(.+)\s*$/i) {
+			$initial_in_reply_to = $1;
+			next;
+		} elsif (/^Reply-To:\s*(.+)\s*$/i) {
+			$reply_to = $1;
+		} elsif (/^From:\s*(.+)\s*$/i) {
+			$sender = $1;
+			next;
+		} elsif (/^(?:To|Cc|Bcc):/i) {
+			print __("To/Cc/Bcc fields are not interpreted yet, they have been ignored\n");
+			next;
 		}
+		print $c2 $_;
 	}
 	close $c;
+	close $c2;
 
-	open my $c2, ">", $compose_filename . ".final"
-	or die sprintf(__("Failed to open %s.final: %s"), $compose_filename, $!);
-
-
-	if ($parsed_email{'From'}) {
-		$sender = delete($parsed_email{'From'});
-	}
-	if ($parsed_email{'In-Reply-To'}) {
-		$initial_in_reply_to = delete($parsed_email{'In-Reply-To'});
-	}
-	if ($parsed_email{'Reply-To'}) {
-		$reply_to = delete($parsed_email{'Reply-To'});
-	}
-	if ($parsed_email{'Subject'}) {
-		$initial_subject = delete($parsed_email{'Subject'});
-		print $c2 "Subject: " .
-			quote_subject($initial_subject, $compose_encoding) .
-			"\n";
-	}
-
-	if ($parsed_email{'MIME-Version'}) {
-		print $c2 "MIME-Version: $parsed_email{'MIME-Version'}\n",
-				"Content-Type: $parsed_email{'Content-Type'};\n",
-				"Content-Transfer-Encoding: $parsed_email{'Content-Transfer-Encoding'}\n";
-		delete($parsed_email{'MIME-Version'});
-		delete($parsed_email{'Content-Type'});
-		delete($parsed_email{'Content-Transfer-Encoding'});
-	} elsif (file_has_nonascii($compose_filename)) {
-		my $content_type = (delete($parsed_email{'Content-Type'}) or
-			"text/plain; charset=$compose_encoding");
-		print $c2 "MIME-Version: 1.0\n",
-			"Content-Type: $content_type\n",
-			"Content-Transfer-Encoding: 8bit\n";
-	}
-	# Preserve unknown headers
-	foreach my $key (keys %parsed_email) {
-		next if $key eq 'body';
-		print $c2 "$key: $parsed_email{$key}";
-	}
-
-	if ($parsed_email{'body'}) {
-		print $c2 "\n$parsed_email{'body'}\n";
-		delete($parsed_email{'body'});
-	} else {
+	if ($summary_empty) {
 		print __("Summary email is empty, skipping it\n");
 		$compose = -1;
 	}
-
-	close $c2;
-
 } elsif ($annotate) {
 	do_edit(@files);
 }
@@ -1009,32 +995,6 @@ sub ask {
 	return;
 }
 
-sub parse_header_line {
-	my $lines = shift;
-	my $parsed_line = shift;
-	my $addr_pat = join "|", qw(To Cc Bcc);
-
-	foreach (split(/\n/, $lines)) {
-		if (/^($addr_pat):\s*(.+)$/i) {
-		        $parsed_line->{$1} = [ parse_address_line($2) ];
-		} elsif (/^([^:]*):\s*(.+)\s*$/i) {
-		        $parsed_line->{$1} = $2;
-		}
-	}
-}
-
-sub filter_body {
-	my $c = shift;
-	my $body = "";
-	while (my $body_line = <$c>) {
-		if ($body_line !~ m/^GIT:/) {
-			$body .= $body_line;
-		}
-	}
-	return $body;
-}
-
-
 my %broken_encoding;
 
 sub file_declares_8bit_cte {
diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh
index 263db3ad17..9644ff5793 100755
--- a/t/t9001-send-email.sh
+++ b/t/t9001-send-email.sh
@@ -2505,4 +2505,39 @@ test_expect_success $PREREQ 'test forbidSendmailVariables behavior override' '
 		HEAD^
 '
 
+test_expect_success $PREREQ '--compose handles lowercase headers' '
+	write_script fake-editor <<-\EOF &&
+	sed "s/^From:.*/from: edited-from@example.com/i" "$1" >"$1.tmp" &&
+	mv "$1.tmp" "$1"
+	EOF
+	clean_fake_sendmail &&
+	git send-email \
+		--compose \
+		--from="Example <from@example.com>" \
+		--to=nobody@example.com \
+		--smtp-server="$(pwd)/fake.sendmail" \
+		HEAD^ &&
+	grep "From: edited-from@example.com" msgtxt1
+'
+
+test_expect_success $PREREQ '--compose handles to headers' '
+	write_script fake-editor <<-\EOF &&
+	sed "s/^$/To: edited-to@example.com\n/" <"$1" >"$1.tmp" &&
+	echo this is the body >>"$1.tmp" &&
+	mv "$1.tmp" "$1"
+	EOF
+	clean_fake_sendmail &&
+	GIT_SEND_EMAIL_NOTTY=1 \
+	git send-email \
+		--compose \
+		--from="Example <from@example.com>" \
+		--to=nobody@example.com \
+		--smtp-server="$(pwd)/fake.sendmail" \
+		HEAD^ &&
+	# Ideally the "to" header we specified would be used,
+	# but the program explicitly warns that these are
+	# ignored. For now, just make sure we did not abort.
+	grep "To:" msgtxt1
+'
+
 test_done
-- 
2.42.0.980.g8b5f6199be
Oswald Buddenhagen· Oct 20, 2023, 10:45 UTC · re: Jeff King · lore

Re: [PATCH 2/3] Revert "send-email: extract email-parsing code into a subroutine"

On Fri, Oct 20, 2023 at 06:13:10AM -0400, Jeff King wrote:
Show 10 quoted lines
>But one thing that gives me pause is that the neither before or after
>this patch do we handle continuation lines like:
>
>  Subject: this is the beginning
>    and this is more subject
>
>And it would probably be a lot easier to add when storing the headers in
>a hash (it's not impossible to do it the other way, but you basically
>have to delay processing each line with a small state machine).
>
that seems like a rather significant point, doesn't it?
>So another option is to just fix the individual bugs separately.
>

... so that seems preferable to me, given that the necessary fixes seem rather trivial.

> I guess "readable" is up for debate here, but I find the inline handling
> a lot easier to follow
>
any particular reason for that?
> (and it's half as many lines; most of the diffstat is the new tests).
>-	if ($parsed_email{'From'}) {
>-		$sender = delete($parsed_email{'From'});
>-	}
this verbosity could be cut down somewhat using just
   $sender = delete($parsed_email{'From'});
and if the value can be pre-set and needs to be preserved,
   $sender = delete($parsed_email{'From'}) // $sender;
but this seems kind of counter-productive legibility-wise.
regards
Jeff King· Oct 23, 2023, 18:40 UTC · re: Oswald Buddenhagen · lore

Re: [PATCH 2/3] Revert "send-email: extract email-parsing code into a subroutine"

On Fri, Oct 20, 2023 at 12:45:43PM +0200, Oswald Buddenhagen wrote:
Show 12 quoted lines
> On Fri, Oct 20, 2023 at 06:13:10AM -0400, Jeff King wrote:
> > But one thing that gives me pause is that the neither before or after
> > this patch do we handle continuation lines like:
> > 
> >  Subject: this is the beginning
> >    and this is more subject
> > 
> > And it would probably be a lot easier to add when storing the headers in
> > a hash (it's not impossible to do it the other way, but you basically
> > have to delay processing each line with a small state machine).
> > 
> that seems like a rather significant point, doesn't it?

Maybe. It depends on whether anybody is interested in adding continuation support. Nobody has in the previous 18 years, and nobody has asked for it.

> > So another option is to just fix the individual bugs separately.
> > 
> ... so that seems preferable to me, given that the necessary fixes seem
> rather trivial.
They're not too bad. Probably:
  1. lc() the keys we put into the hash
  2. match to/cc/bcc and dereference their arrays
  3. maybe handle 'body' separately from headers to avoid confusion

But there may be other similar bugs lurking. One I didn't mention: the hash-based version randomly reorders headers!

> > I guess "readable" is up for debate here, but I find the inline handling
> > a lot easier to follow
> > 
> any particular reason for that?

For the reasons I gave in the commit message: namely that the matching and logic is in one place and doesn't need to be duplicated (e.g., the special handling of to/cc/bcc, which caused a bug here).

Show 15 quoted lines
> > (and it's half as many lines; most of the diffstat is the new tests).
> 
> > -	if ($parsed_email{'From'}) {
> > -		$sender = delete($parsed_email{'From'});
> > -	}
> 
> this verbosity could be cut down somewhat using just
> 
>   $sender = delete($parsed_email{'From'});
> 
> and if the value can be pre-set and needs to be preserved,
> 
>   $sender = delete($parsed_email{'From'}) // $sender;
> 
> but this seems kind of counter-productive legibility-wise.

We do need to avoid overwriting the pre-set value. The "//" one would work, but we support perl versions old enough that they don't have it.

-Peff
Oswald Buddenhagen· Oct 23, 2023, 19:50 UTC · re: Jeff King · lore

Re: [PATCH 2/3] Revert "send-email: extract email-parsing code into a subroutine"

On Mon, Oct 23, 2023 at 02:40:10PM -0400, Jeff King wrote:
Show 7 quoted lines
>On Fri, Oct 20, 2023 at 12:45:43PM +0200, Oswald Buddenhagen wrote:
>> that seems like a rather significant point, doesn't it?
>
>Maybe. It depends on whether anybody is interested in adding
>continuation support. Nobody has in the previous 18 years, and nobody
>has asked for it.
>

dunno, it seems like a bug to me. so if i cared at all about this functionality, i'd fix it just because. so at least it doesn't seem nice to make it harder for a potential volunteer.

Show 14 quoted lines
>> > So another option is to just fix the individual bugs separately.
>> > 
>> ... so that seems preferable to me, given that the necessary fixes 
>> seem
>> rather trivial.
>
>They're not too bad. Probably:
>
>  1. lc() the keys we put into the hash
>
>  2. match to/cc/bcc and dereference their arrays
>
>  3. maybe handle 'body' separately from headers to avoid confusion
>

with the header keys lowercased, one could simply use BODY as the key and be done with it.

>But there may be other similar bugs lurking.
>One I didn't mention: the
>hash-based version randomly reorders headers!
>

hmm, yeah, that would mean using Tie::IxHash if one wanted to do it elegantly, at the cost of depending on another non-core module.

also, it means that another hash with non-lowercased versions of the keys would have to be kept.

ok, that's stupid. it would be easier to just keep an additional array of the original keys for iteration, and check the hash before emitting them.

Show 9 quoted lines
>> > I guess "readable" is up for debate here, but I find the inline handling
>> > a lot easier to follow
>> > 
>> any particular reason for that?
>
>For the reasons I gave in the commit message: namely that the matching
>and logic is in one place and doesn't need to be duplicated (e.g., the
>special handling of to/cc/bcc, which caused a bug here).
>

from what i can see, there isn't really anything to "match", apart from agreeing on the data structure (which the code partially failed to do, but that's trivial enough). and layering/abstracting things is usually considered a good thing, unless the cost/benefit ratio is completely backwards.

>The "//" one would
>work, but we support perl versions old enough that they don't have it.
>

according to my grepping, that ship has sailed. also, why _would_ you support such ancient perl versions? that makes even less sense to me than supporting ancient c compilers.

regards
Jeff King· Oct 25, 2023, 06:11 UTC · re: Oswald Buddenhagen · lore

Re: [PATCH 2/3] Revert "send-email: extract email-parsing code into a subroutine"

On Mon, Oct 23, 2023 at 09:50:54PM +0200, Oswald Buddenhagen wrote:
Show 6 quoted lines
> > The "//" one would
> > work, but we support perl versions old enough that they don't have it.
> > 
> according to my grepping, that ship has sailed.
> also, why _would_ you support such ancient perl versions? that makes even
> less sense to me than supporting ancient c compilers.

It may be reasonable to bump the default perl version for the script. But that would require somebody digging into what tends to ship these days (which can be sometimes be surprising; witness macos using old versions of bash due to license issues), and then updating the "use 5.008" in the script.

The "//" operator was added in perl 5.10. I'm not sure what you found that makes you think the ship has sailed. The only hits for "//" I see look like the end of substitution regexes ("s/foo//" and similar). But if we are not consistent with the "use" claim, that is worth fixing.

-Peff
Oswald Buddenhagen· Oct 25, 2023, 09:23 UTC · re: Jeff King · lore

Re: [PATCH 2/3] Revert "send-email: extract email-parsing code into a subroutine"

On Wed, Oct 25, 2023 at 02:11:20AM -0400, Jeff King wrote:
>The "//" operator was added in perl 5.10. I'm not sure what you found
>that makes you think the ship has sailed. The only hits for "//" I see
>look like the end of substitution regexes ("s/foo//" and similar).
>

grep with spaces around the operator, then you can see the instance in git-credential-netrc.perl easily.

regards
Junio C Hamano· Oct 27, 2023, 22:31 UTC · re: Oswald Buddenhagen · lore

Re: [PATCH 2/3] Revert "send-email: extract email-parsing code into a subroutine"

Oswald Buddenhagen <oswald.buddenhagen@gmx.de> writes:
Show 7 quoted lines
> On Wed, Oct 25, 2023 at 02:11:20AM -0400, Jeff King wrote:
>>The "//" operator was added in perl 5.10. I'm not sure what you found
>>that makes you think the ship has sailed. The only hits for "//" I see
>>look like the end of substitution regexes ("s/foo//" and similar).
>>
> grep with spaces around the operator, then you can see the instance in
> git-credential-netrc.perl easily.

Good find, but given the relative prevalence in use between netrc helper and send-email, my conclusion is rather opposite. It seems to indicate that avoiding "//" would still be prudent if the only tool we can find find broken on 5.008 is the netrc helper.

Jeff King· Oct 30, 2023, 09:13 UTC · re: Oswald Buddenhagen · lore

Re: [PATCH 2/3] Revert "send-email: extract email-parsing code into a subroutine"

On Wed, Oct 25, 2023 at 11:23:01AM +0200, Oswald Buddenhagen wrote:
Show 7 quoted lines
> On Wed, Oct 25, 2023 at 02:11:20AM -0400, Jeff King wrote:
> > The "//" operator was added in perl 5.10. I'm not sure what you found
> > that makes you think the ship has sailed. The only hits for "//" I see
> > look like the end of substitution regexes ("s/foo//" and similar).
> > 
> grep with spaces around the operator, then you can see the instance in
> git-credential-netrc.perl easily.

Ah, yeah, there is one instance there. That script does not have a "use" marker, though, and we do not necessarily need or want to be as strict with contrib/ scripts, which are quite optional compared to core functionality like send-email.

That said, I do suspect that requiring 5.10 or later would not be too burdensome these days. If we want to do so, then the first step would be updating the text in INSTALL, along with the "use" directives in most files. Probably d48b284183 (perl: bump the required Perl version to 5.8 from 5.6.[21], 2010-09-24) could serve as a template.

-Peff
Junio C Hamano· Oct 20, 2023, 21:45 UTC · re: Jeff King · lore

Re: [PATCH 2/3] Revert "send-email: extract email-parsing code into a subroutine"

Jeff King <peff@peff.net> writes:
>   - the handling for to/cc/bcc is totally broken.

It is good to see another evidence that "--compose" is probably not as often as used as we thought. With enough bugs discovered, perhaps someday we can declare "it cannot be that the feature is used in the wild, without anybody getting hit by these bugs---let's deprecate and eventually remove it" ;-)

Jeff King· Oct 23, 2023, 18:47 UTC · re: Junio C Hamano · lore

Re: [PATCH 2/3] Revert "send-email: extract email-parsing code into a subroutine"

On Fri, Oct 20, 2023 at 02:45:30PM -0700, Junio C Hamano wrote:
Show 9 quoted lines
> Jeff King <peff@peff.net> writes:
> 
> >   - the handling for to/cc/bcc is totally broken.
> 
> It is good to see another evidence that "--compose" is probably not
> as often as used as we thought.  With enough bugs discovered,
> perhaps someday we can declare "it cannot be that the feature is
> used in the wild, without anybody getting hit by these bugs---let's
> deprecate and eventually remove it" ;-)

I'm not sure if that is evidence or not. The to/cc/bcc feature was just never implemented. The commit from 2017 made it more broken than saying "not yet implemented", but that may only be an indication that nobody wants it or tried to use it.

I dunno. As I noted, the same feature exists when reading the cover-letter from a set of format-patch files. And of course it is implemented using totally separate code (in pre_process_file). One possible cleanup would be to unify those two, but I'm sure there would be behavior changes. Some of them perhaps good (e.g., it looks like pre_process_file is more careful about rfc2047 handling) and some of them I'm not so sure of (e.g., support for --header-cmd in the --compose letter).

I think an interested person could champion such changes, but I am not that interested in send-email (I don't use it, and some of its code is pretty ancient and gross). My goal was to fix the bug I saw with minimal regression (I waffled even on my patch 2).

-Peff
Jeff King· Oct 20, 2023, 10:15 UTC · re: Jeff King · lore

[PATCH 3/3] send-email: handle to/cc/bcc from --compose message

If the user writes a message via --compose, send-email will pick up varius headers like "From", "Subject", etc and use them for other patches as if they were specified on the command-line. But we don't handle "To", "Cc", or "Bcc" this way; we just tell the user "those aren't interpeted yet" and ignore them.

But it seems like an obvious thing to want, especially as the same feature exists when the cover letter is generated separately by format-patch. There it is gated behind the --to-cover option, but I don't think we'd need the same control here; since we generate the --compose template ourselves based on the existing input, if the user leaves the lines unchanged then the behavior remains the same.

So let's fill in the implementation; like those other headers we already handle, we just need to assign to the initial_* variables. The only difference in this case is that they are arrays, so we'll feed them through parse_address_line() to split them (just like we would when reading a single string via prompting).

Signed-off-by: Jeff King <peff@peff.net>
---
 Documentation/git-send-email.txt | 11 ++++++-----
 git-send-email.perl              | 16 ++++++++++++++--
 t/t9001-send-email.sh            | 16 +++++++++++-----
 3 files changed, 31 insertions(+), 12 deletions(-)
diff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt
index 021276329c..f4d7166275 100644
--- a/Documentation/git-send-email.txt
+++ b/Documentation/git-send-email.txt
@@ -68,11 +68,12 @@ This option may be specified multiple times.
 	Invoke a text editor (see GIT_EDITOR in linkgit:git-var[1])
 	to edit an introductory message for the patch series.
 +
-When `--compose` is used, git send-email will use the From, Subject,
-Reply-To, and In-Reply-To headers specified in the message. If the body
-of the message (what you type after the headers and a blank line) only
-contains blank (or Git: prefixed) lines, the summary won't be sent, but
-the headers mentioned above will be used unless they are removed.
+When `--compose` is used, git send-email will use the From, To, Cc, Bcc,
+Subject, Reply-To, and In-Reply-To headers specified in the message. If
+the body of the message (what you type after the headers and a blank
+line) only contains blank (or Git: prefixed) lines, the summary won't be
+sent, but the headers mentioned above will be used unless they are
+removed.
 +
 Missing From or In-Reply-To headers will be prompted for.
 +
diff --git a/git-send-email.perl b/git-send-email.perl
index bbda2a931b..9e21b0b3f4 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -861,6 +861,9 @@ sub get_patch_subject {
 	my $tpl_subject = $initial_subject || '';
 	my $tpl_in_reply_to = $initial_in_reply_to || '';
 	my $tpl_reply_to = $reply_to || '';
+	my $tpl_to = join(',', @initial_to);
+	my $tpl_cc = join(',', @initial_cc);
+	my $tpl_bcc = join(', ', @initial_bcc);
 
 	print $c <<EOT1, Git::prefix_lines("GIT: ", __(<<EOT2)), <<EOT3;
 From $tpl_sender # This line is ignored.
@@ -872,6 +875,9 @@ sub get_patch_subject {
 Clear the body content if you don't wish to send a summary.
 EOT2
 From: $tpl_sender
+To: $tpl_to
+Cc: $tpl_cc
+Bcc: $tpl_bcc
 Reply-To: $tpl_reply_to
 Subject: $tpl_subject
 In-Reply-To: $tpl_in_reply_to
@@ -928,8 +934,14 @@ sub get_patch_subject {
 		} elsif (/^From:\s*(.+)\s*$/i) {
 			$sender = $1;
 			next;
-		} elsif (/^(?:To|Cc|Bcc):/i) {
-			print __("To/Cc/Bcc fields are not interpreted yet, they have been ignored\n");
+		} elsif (/^To:\s*(.+)\s*$/i) {
+			@initial_to = parse_address_line($1);
+			next;
+		} elsif (/^Cc:\s*(.+)\s*$/i) {
+			@initial_cc = parse_address_line($1);
+			next;
+		} elsif (/^Bcc:/i) {
+			@initial_bcc = parse_address_line($1);
 			next;
 		}
 		print $c2 $_;
diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh
index 9644ff5793..2e8e8137fb 100755
--- a/t/t9001-send-email.sh
+++ b/t/t9001-send-email.sh
@@ -2522,7 +2522,7 @@ test_expect_success $PREREQ '--compose handles lowercase headers' '
 
 test_expect_success $PREREQ '--compose handles to headers' '
 	write_script fake-editor <<-\EOF &&
-	sed "s/^$/To: edited-to@example.com\n/" <"$1" >"$1.tmp" &&
+	sed "s/^To: .*/&, edited-to@example.com/" <"$1" >"$1.tmp" &&
 	echo this is the body >>"$1.tmp" &&
 	mv "$1.tmp" "$1"
 	EOF
@@ -2534,10 +2534,16 @@ test_expect_success $PREREQ '--compose handles to headers' '
 		--to=nobody@example.com \
 		--smtp-server="$(pwd)/fake.sendmail" \
 		HEAD^ &&
-	# Ideally the "to" header we specified would be used,
-	# but the program explicitly warns that these are
-	# ignored. For now, just make sure we did not abort.
-	grep "To:" msgtxt1
+	# Check both that the cover letter used our modified "to" line,
+	# but also that it was picked up for the patch.
+	q_to_tab >expect <<-\EOF &&
+	To: nobody@example.com,
+	Qedited-to@example.com
+	EOF
+	grep -A1 "^To:" msgtxt1 >msgtxt1.to &&
+	test_cmp expect msgtxt1.to &&
+	grep -A1 "^To:" msgtxt2 >msgtxt2.to &&
+	test_cmp expect msgtxt2.to
 '
 
 test_done
-- 
2.42.0.980.g8b5f6199be
Eric Sunshine· Oct 20, 2023, 17:30 UTC · re: Jeff King · lore

Re: [PATCH 3/3] send-email: handle to/cc/bcc from --compose message

On Fri, Oct 20, 2023 at 6:15 AM Jeff King <peff@peff.net> wrote:
> If the user writes a message via --compose, send-email will pick up
> varius headers like "From", "Subject", etc and use them for other
s/varius/various/
Show 18 quoted lines
> patches as if they were specified on the command-line. But we don't
> handle "To", "Cc", or "Bcc" this way; we just tell the user "those
> aren't interpeted yet" and ignore them.
>
> But it seems like an obvious thing to want, especially as the same
> feature exists when the cover letter is generated separately by
> format-patch. There it is gated behind the --to-cover option, but I
> don't think we'd need the same control here; since we generate the
> --compose template ourselves based on the existing input, if the user
> leaves the lines unchanged then the behavior remains the same.
>
> So let's fill in the implementation; like those other headers we already
> handle, we just need to assign to the initial_* variables. The only
> difference in this case is that they are arrays, so we'll feed them
> through parse_address_line() to split them (just like we would when
> reading a single string via prompting).
>
> Signed-off-by: Jeff King <peff@peff.net>
Junio C Hamano· Oct 20, 2023, 21:42 UTC · re: Jeff King · lore

Re: [PATCH 0/3] some send-email --compose fixes

Jeff King <peff@peff.net> writes:
Show 20 quoted lines
> [culling the rather large cc, as we moving off the original topic]
>
> On Fri, Oct 20, 2023 at 03:14:03AM -0400, Jeff King wrote:
>
>> and there's your perl array ref (from the square brackets, which are
>> necessary because we're sticking it in a hash value). But even before
>> your patch, this seems to end up as garbage. The code which reads
>> $parsed_line does not dereference the array.
>> 
>> The patch to fix it is only a few lines (well, more than that with some
>> light editorializing in the comments):
>
> So here's the fix in a cleaned up form, guided by my own comments from
> earlier. ;) I think this is actually all orthogonal to the patch you are
> working on, so yours could either go on top or just be applied
> separately.
>
>   [1/3]: doc/send-email: mention handling of "reply-to" with --compose
>   [2/3]: Revert "send-email: extract email-parsing code into a subroutine"
>   [3/3]: send-email: handle to/cc/bcc from --compose message
Nice.

With the approach suggested to move the validation down to where the necessary addresses are already all defined, Michael observed "whoa, why am I getting stringified array ref?". If that is the only issue in the approach, queuing these three patches first and then have Michael's fix on top of them sounds like the cleanest thing to do.

Will queue on top of v2.42.0 to help those who may want to backport these to the maintenance track.

Thanks.
Jeff King· Oct 23, 2023, 18:51 UTC · re: Junio C Hamano · lore

Re: [PATCH 0/3] some send-email --compose fixes

On Fri, Oct 20, 2023 at 02:42:13PM -0700, Junio C Hamano wrote:
Show 16 quoted lines
> > So here's the fix in a cleaned up form, guided by my own comments from
> > earlier. ;) I think this is actually all orthogonal to the patch you are
> > working on, so yours could either go on top or just be applied
> > separately.
> >
> >   [1/3]: doc/send-email: mention handling of "reply-to" with --compose
> >   [2/3]: Revert "send-email: extract email-parsing code into a subroutine"
> >   [3/3]: send-email: handle to/cc/bcc from --compose message
> 
> Nice.
> 
> With the approach suggested to move the validation down to where the
> necessary addresses are already all defined, Michael observed "whoa,
> why am I getting stringified array ref?".  If that is the only issue
> in the approach, queuing these three patches first and then have
> Michael's fix on top of them sounds like the cleanest thing to do.

I don't think it is even an issue in Michael's approach. I'd have to see his patch and how he tested it to be sure, but I suspect he was simply being extra careful to test nearby behavior and stumbled upon the ARRAY() bug. But the bug was there long before either of his patches.

> Will queue on top of v2.42.0 to help those who may want to backport
> these to the maintenance track.

So I think you could take my series on top of master (or 2.42.0), and eventually target 'master'. The bug it fixes is from 2017, so not urgent. The reading of "to" headers is a new feature.

But the fix to move the validation around should probably go directly onto a8022c5f7b (send-email: expose header information to git-send-email's sendemail-validate hook, 2023-04-19) for use on maint. I guess maybe it is not that urgent anymore, as that regression is in v2.41, and we would not release anything along that maint track anymore, though.

-Peff
Michael Strawbridge· Oct 24, 2023, 20:12 UTC · re: Jeff King · lore

Re: [PATCH 0/3] some send-email --compose fixes

On 10/23/23 14:51, Jeff King wrote:
Show 18 quoted lines
> On Fri, Oct 20, 2023 at 02:42:13PM -0700, Junio C Hamano wrote:
> 
>>> So here's the fix in a cleaned up form, guided by my own comments from
>>> earlier. ;) I think this is actually all orthogonal to the patch you are
>>> working on, so yours could either go on top or just be applied
>>> separately.
>>>
>>>   [1/3]: doc/send-email: mention handling of "reply-to" with --compose
>>>   [2/3]: Revert "send-email: extract email-parsing code into a subroutine"
>>>   [3/3]: send-email: handle to/cc/bcc from --compose message
>>
>> Nice.
>>
>> With the approach suggested to move the validation down to where the
>> necessary addresses are already all defined, Michael observed "whoa,
>> why am I getting stringified array ref?".  If that is the only issue
>> in the approach, queuing these three patches first and then have
>> Michael's fix on top of them sounds like the cleanest thing to do.
Patch coming soon.
Show 6 quoted lines
> 
> I don't think it is even an issue in Michael's approach. I'd have to see
> his patch and how he tested it to be sure, but I suspect he was simply
> being extra careful to test nearby behavior and stumbled upon the
> ARRAY() bug. But the bug was there long before either of his patches.
> 

Thank you for your patches Peff! I think it fixes the issue I was seeing. I was trying to be extra careful with my testing. I had missed testing --compose and also the multiple --to/cc/bcc examples before.

Show 15 quoted lines
>> Will queue on top of v2.42.0 to help those who may want to backport
>> these to the maintenance track.
> 
> So I think you could take my series on top of master (or 2.42.0), and
> eventually target 'master'. The bug it fixes is from 2017, so not
> urgent. The reading of "to" headers is a new feature.
> 
> But the fix to move the validation around should probably go directly
> onto a8022c5f7b (send-email: expose header information to
> git-send-email's sendemail-validate hook, 2023-04-19) for use on maint.
> I guess maybe it is not that urgent anymore, as that regression is in
> v2.41, and we would not release anything along that maint track anymore,
> though.
> 
> -Peff
Michael Strawbridge· Oct 24, 2023, 20:19 UTC · re: Michael Strawbridge · lore

[PATCH] send-email: move validation code below process_address_list

From 09ea51d63cebdf9ff0c073ef86e21b4b09c268e5 Mon Sep 17 00:00:00 2001
From: Michael Strawbridge <michael.strawbridge@amd.com>
Date: Wed, 11 Oct 2023 16:13:13 -0400
Subject: [PATCH] send-email: move validation code below process_address_list

Move validation logic below processing of email address lists so that email validation gets the proper email addresses.

This fixes email address validation errors when the optional perl module Email::Valid is installed and multiple addresses are passed in on a single to/cc argument like --to=foo@example.com,bar@example.com.

Reported-by: Bagas Sanjaya <bagasdotme@gmail.com>
Signed-off-by: Michael Strawbridge <michael.strawbridge@amd.com>
---
 git-send-email.perl | 48 ++++++++++++++++++++++-----------------------
 1 file changed, 24 insertions(+), 24 deletions(-)
diff --git a/git-send-email.perl b/git-send-email.perl
index 288ea1ae80..a898dbc76e 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -799,30 +799,6 @@ sub is_format_patch_arg {
 
 $time = time - scalar $#files;
 
-if ($validate) {
-	# FIFOs can only be read once, exclude them from validation.
-	my @real_files = ();
-	foreach my $f (@files) {
-		unless (-p $f) {
-			push(@real_files, $f);
-		}
-	}
-
-	# Run the loop once again to avoid gaps in the counter due to FIFO
-	# arguments provided by the user.
-	my $num = 1;
-	my $num_files = scalar @real_files;
-	$ENV{GIT_SENDEMAIL_FILE_TOTAL} = "$num_files";
-	foreach my $r (@real_files) {
-		$ENV{GIT_SENDEMAIL_FILE_COUNTER} = "$num";
-		pre_process_file($r, 1);
-		validate_patch($r, $target_xfer_encoding);
-		$num += 1;
-	}
-	delete $ENV{GIT_SENDEMAIL_FILE_COUNTER};
-	delete $ENV{GIT_SENDEMAIL_FILE_TOTAL};
-}
-
 @files = handle_backup_files(@files);
 
 if (@files) {
@@ -2023,6 +1999,30 @@ sub process_file {
 	return 1;
 }
 
+if ($validate) {
+	# FIFOs can only be read once, exclude them from validation.
+	my @real_files = ();
+	foreach my $f (@files) {
+		unless (-p $f) {
+			push(@real_files, $f);
+		}
+	}
+
+	# Run the loop once again to avoid gaps in the counter due to FIFO
+	# arguments provided by the user.
+	my $num = 1;
+	my $num_files = scalar @real_files;
+	$ENV{GIT_SENDEMAIL_FILE_TOTAL} = "$num_files";
+	foreach my $r (@real_files) {
+		$ENV{GIT_SENDEMAIL_FILE_COUNTER} = "$num";
+		pre_process_file($r, 1);
+		validate_patch($r, $target_xfer_encoding);
+		$num += 1;
+	}
+	delete $ENV{GIT_SENDEMAIL_FILE_COUNTER};
+	delete $ENV{GIT_SENDEMAIL_FILE_TOTAL};
+}
+
 foreach my $t (@files) {
 	while (!process_file($t)) {
 		# user edited the file
-- 
2.42.0
Junio C Hamano· Oct 24, 2023, 21:55 UTC · re: Michael Strawbridge · lore

Re: [PATCH] send-email: move validation code below process_address_list

Michael Strawbridge <michael.strawbridge@amd.com> writes:
> Subject: [PATCH] send-email: move validation code below process_address_list
>
> Move validation logic below processing of email address lists so that
> email validation gets the proper email addresses.

Hmph, without this patch, the tip of 'seen' passes t9001 on my box, but with it, it claims that it failed 58, 87, and 89.

Here is how #58 fails (the last part of "cd t && sh t9001-*.sh -i -v").
expecting success of 9001.58 'In-Reply-To without --chain-reply-to':
        clean_fake_sendmail &&
        echo "<unique-message-id@example.com>" >expect &&
        git send-email \
                --from="Example <nobody@example.com>" \
                --to=nobody@example.com \
                --no-chain-reply-to \
                --in-reply-to="$(cat expect)" \
                --smtp-server="$(pwd)/fake.sendmail" \
                $patches $patches $patches \
                2>errors &&
        # The first message is a reply to --in-reply-to
        sed -n -e "s/^In-Reply-To: *\(.*\)/\1/p" msgtxt1 >actual &&
        test_cmp expect actual &&
        # Second and subsequent messages are replies to the first one
        sed -n -e "s/^Message-ID: *\(.*\)/\1/p" msgtxt1 >expect &&
        sed -n -e "s/^In-Reply-To: *\(.*\)/\1/p" msgtxt2 >actual &&
        test_cmp expect actual &&
        sed -n -e "s/^In-Reply-To: *\(.*\)/\1/p" msgtxt3 >actual &&
        test_cmp expect actual
0001-Second.patch
0001-Second.patch
0001-Second.patch
(mbox) Adding cc: A <author@example.com> from line 'From: A <author@example.com>'
(mbox) Adding cc: One <one@example.com> from line 'Cc: One <one@example.com>, two@example.com'
(mbox) Adding cc: two@example.com from line 'Cc: One <one@example.com>, two@example.com'
(body) Adding cc: C O Mitter <committer@example.com> from line 'Signed-off-by: C O Mitter <committer@example.com>'
OK. Log says:
Sendmail: /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
From: Example <nobody@example.com>
To: nobody@example.com
Cc: A <author@example.com>,
        One <one@example.com>,
        two@example.com,
        C O Mitter <committer@example.com>
Subject: [PATCH 1/1] Second.
Date: Tue, 24 Oct 2023 21:52:27 +0000
Message-ID: <20231024215229.1787922-1-nobody@example.com>
X-Mailer: git-send-email 2.42.0-705-g1a1f985ecc
In-Reply-To: <unique-message-id@example.com>
References: <unique-message-id@example.com>
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Result: OK
(mbox) Adding cc: A <author@example.com> from line 'From: A <author@example.com>'
(mbox) Adding cc: One <one@example.com> from line 'Cc: One <one@example.com>, two@example.com'
(mbox) Adding cc: two@example.com from line 'Cc: One <one@example.com>, two@example.com'
(body) Adding cc: C O Mitter <committer@example.com> from line 'Signed-off-by: C O Mitter <committer@example.com>'
OK. Log says:
Sendmail: /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
From: Example <nobody@example.com>
To: nobody@example.com
Cc: A <author@example.com>,
        One <one@example.com>,
        two@example.com,
        C O Mitter <committer@example.com>
Subject: [PATCH 1/1] Second.
Date: Tue, 24 Oct 2023 21:52:28 +0000
Message-ID: <20231024215229.1787922-2-nobody@example.com>
X-Mailer: git-send-email 2.42.0-705-g1a1f985ecc
In-Reply-To: <unique-message-id@example.com>
References: <unique-message-id@example.com>
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Result: OK
(mbox) Adding cc: A <author@example.com> from line 'From: A <author@example.com>'
(mbox) Adding cc: One <one@example.com> from line 'Cc: One <one@example.com>, two@example.com'
(mbox) Adding cc: two@example.com from line 'Cc: One <one@example.com>, two@example.com'
(body) Adding cc: C O Mitter <committer@example.com> from line 'Signed-off-by: C O Mitter <committer@example.com>'
OK. Log says:
Sendmail: /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
From: Example <nobody@example.com>
To: nobody@example.com
Cc: A <author@example.com>,
        One <one@example.com>,
        two@example.com,
        C O Mitter <committer@example.com>
Subject: [PATCH 1/1] Second.
Date: Tue, 24 Oct 2023 21:52:29 +0000
Message-ID: <20231024215229.1787922-3-nobody@example.com>
X-Mailer: git-send-email 2.42.0-705-g1a1f985ecc
In-Reply-To: <unique-message-id@example.com>
References: <unique-message-id@example.com>
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Result: OK
--- expect      2023-10-24 21:52:29.115044899 +0000
+++ actual      2023-10-24 21:52:29.119045306 +0000
@@ -1 +1 @@
-<20231024215229.1787922-1-nobody@example.com>
+<unique-message-id@example.com>
not ok 58 - In-Reply-To without --chain-reply-to
#
#               clean_fake_sendmail &&
#               echo "<unique-message-id@example.com>" >expect &&
#               git send-email \
#                       --from="Example <nobody@example.com>" \
#                       --to=nobody@example.com \
#                       --no-chain-reply-to \
#                       --in-reply-to="$(cat expect)" \
#                       --smtp-server="$(pwd)/fake.sendmail" \
#                       $patches $patches $patches \
#                       2>errors &&
#               # The first message is a reply to --in-reply-to
#               sed -n -e "s/^In-Reply-To: *\(.*\)/\1/p" msgtxt1 >actual &&
#               test_cmp expect actual &&
#               # Second and subsequent messages are replies to the first one
#               sed -n -e "s/^Message-ID: *\(.*\)/\1/p" msgtxt1 >expect &&
#               sed -n -e "s/^In-Reply-To: *\(.*\)/\1/p" msgtxt2 >actual &&
#               test_cmp expect actual &&
#               sed -n -e "s/^In-Reply-To: *\(.*\)/\1/p" msgtxt3 >actual &&
#               test_cmp expect actual
#
1..58
Junio C Hamano· Oct 24, 2023, 22:03 UTC · re: Junio C Hamano · lore

Re: [PATCH] send-email: move validation code below process_address_list

Junio C Hamano <gitster@pobox.com> writes:
Show 9 quoted lines
> Michael Strawbridge <michael.strawbridge@amd.com> writes:
>
>> Subject: [PATCH] send-email: move validation code below process_address_list
>>
>> Move validation logic below processing of email address lists so that
>> email validation gets the proper email addresses.
>
> Hmph, without this patch, the tip of 'seen' passes t9001 on my box,
> but with it, it claims that it failed 58, 87, and 89.

FWIW, when this patch is used with 'master' (not 'seen'), t9001 claims the same three tests failed. The way #58 fails seems to be identical to the way 'seen' with this patch failed, shown in the message I am responding to.

Michael Strawbridge· Oct 25, 2023, 18:48 UTC · re: Junio C Hamano · lore

Re: [PATCH] send-email: move validation code below process_address_list

On 10/24/23 18:03, Junio C Hamano wrote:
Show 16 quoted lines
> Junio C Hamano <gitster@pobox.com> writes:
> 
>> Michael Strawbridge <michael.strawbridge@amd.com> writes:
>>
>>> Subject: [PATCH] send-email: move validation code below process_address_list
>>>
>>> Move validation logic below processing of email address lists so that
>>> email validation gets the proper email addresses.
>>
>> Hmph, without this patch, the tip of 'seen' passes t9001 on my box,
>> but with it, it claims that it failed 58, 87, and 89.
> 
> FWIW, when this patch is used with 'master' (not 'seen'), t9001
> claims the same three tests failed.  The way #58 fails seems to be
> identical to the way 'seen' with this patch failed, shown in the
> message I am responding to.

I'm sorry to have wasted your time with patch 1. I had done the other manual tests but ended up forgetting the automated ones.

Michael Strawbridge· Oct 25, 2023, 18:51 UTC · re: Junio C Hamano · lore

[PATCH v2] send-email: move validation code below process_address_list

From 67223238d9b1977d20b1286055d7f197e4d746e9 Mon Sep 17 00:00:00 2001
From: Michael Strawbridge <michael.strawbridge@amd.com>
Date: Wed, 11 Oct 2023 16:13:13 -0400
Subject: [PATCH v2] send-email: move validation code below
 process_address_list

Move validation logic below processing of email address lists so that email validation gets the proper email addresses. As a side effect, some initialization needed to be moved down. In order for validation and the actual email sending to have the same initial state, the initialized variables that get modified by pre_process_file are encapsulated in a new function.

This fixes email address validation errors when the optional perl module Email::Valid is installed and multiple addresses are passed in on a single to/cc argument like --to=foo@example.com,bar@example.com. A new test was added to t9001 to expose failures with this case in the future.

Fixes: a8022c5f7b67 ("send-email: expose header information to git-send-email's sendemail-validate hook")
Reported-by: Bagas Sanjaya <bagasdotme@gmail.com>
Signed-off-by: Michael Strawbridge <michael.strawbridge@amd.com>
---
 git-send-email.perl   | 60 +++++++++++++++++++++++--------------------
 t/t9001-send-email.sh | 19 ++++++++++++++
 2 files changed, 51 insertions(+), 28 deletions(-)
diff --git a/git-send-email.perl b/git-send-email.perl
index 288ea1ae80..ce22a5e06d 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -799,30 +799,6 @@ sub is_format_patch_arg {
 
 $time = time - scalar $#files;
 
-if ($validate) {
-	# FIFOs can only be read once, exclude them from validation.
-	my @real_files = ();
-	foreach my $f (@files) {
-		unless (-p $f) {
-			push(@real_files, $f);
-		}
-	}
-
-	# Run the loop once again to avoid gaps in the counter due to FIFO
-	# arguments provided by the user.
-	my $num = 1;
-	my $num_files = scalar @real_files;
-	$ENV{GIT_SENDEMAIL_FILE_TOTAL} = "$num_files";
-	foreach my $r (@real_files) {
-		$ENV{GIT_SENDEMAIL_FILE_COUNTER} = "$num";
-		pre_process_file($r, 1);
-		validate_patch($r, $target_xfer_encoding);
-		$num += 1;
-	}
-	delete $ENV{GIT_SENDEMAIL_FILE_COUNTER};
-	delete $ENV{GIT_SENDEMAIL_FILE_TOTAL};
-}
-
 @files = handle_backup_files(@files);
 
 if (@files) {
@@ -1754,10 +1730,6 @@ sub send_message {
 	return 1;
 }
 
-$in_reply_to = $initial_in_reply_to;
-$references = $initial_in_reply_to || '';
-$message_num = 0;
-
 sub pre_process_file {
 	my ($t, $quiet) = @_;
 
@@ -2023,6 +1995,38 @@ sub process_file {
 	return 1;
 }
 
+sub initialize_modified_loop_vars {
+	$in_reply_to = $initial_in_reply_to;
+	$references = $initial_in_reply_to || '';
+	$message_num = 0;
+}
+
+if ($validate) {
+	# FIFOs can only be read once, exclude them from validation.
+	my @real_files = ();
+	foreach my $f (@files) {
+		unless (-p $f) {
+			push(@real_files, $f);
+		}
+	}
+
+	# Run the loop once again to avoid gaps in the counter due to FIFO
+	# arguments provided by the user.
+	my $num = 1;
+	my $num_files = scalar @real_files;
+	$ENV{GIT_SENDEMAIL_FILE_TOTAL} = "$num_files";
+	initialize_modified_loop_vars();
+	foreach my $r (@real_files) {
+		$ENV{GIT_SENDEMAIL_FILE_COUNTER} = "$num";
+		pre_process_file($r, 1);
+		validate_patch($r, $target_xfer_encoding);
+		$num += 1;
+	}
+	delete $ENV{GIT_SENDEMAIL_FILE_COUNTER};
+	delete $ENV{GIT_SENDEMAIL_FILE_TOTAL};
+}
+
+initialize_modified_loop_vars();
 foreach my $t (@files) {
 	while (!process_file($t)) {
 		# user edited the file
diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh
index 263db3ad17..ccff2ad647 100755
--- a/t/t9001-send-email.sh
+++ b/t/t9001-send-email.sh
@@ -633,6 +633,25 @@ test_expect_success $PREREQ "--validate respects absolute core.hooksPath path" '
 	test_cmp expect actual
 '
 
+test_expect_success $PREREQ "--validate hook supports multiple addresses in arguments" '
+	hooks_path="$(pwd)/my-hooks" &&
+	test_config core.hooksPath "$hooks_path" &&
+	test_when_finished "rm my-hooks.ran" &&
+	test_must_fail git send-email \
+		--from="Example <nobody@example.com>" \
+		--to=nobody@example.com,abc@example.com \
+		--smtp-server="$(pwd)/fake.sendmail" \
+		--validate \
+		longline.patch 2>actual &&
+	test_path_is_file my-hooks.ran &&
+	cat >expect <<-EOF &&
+	fatal: longline.patch: rejected by sendemail-validate hook
+	fatal: command '"'"'git hook run --ignore-missing sendemail-validate -- <patch> <header>'"'"' died with exit code 1
+	warning: no patches were sent
+	EOF
+	test_cmp expect actual
+'
+
 test_expect_success $PREREQ "--validate hook supports header argument" '
 	write_script my-hooks/sendemail-validate <<-\EOF &&
 	if test "$#" -ge 2
-- 
2.42.GIT
Junio C Hamano· Oct 26, 2023, 12:44 UTC · re: Michael Strawbridge · lore

Re: [PATCH v2] send-email: move validation code below process_address_list

Michael Strawbridge <michael.strawbridge@amd.com> writes:
Show 5 quoted lines
> From 67223238d9b1977d20b1286055d7f197e4d746e9 Mon Sep 17 00:00:00 2001
> From: Michael Strawbridge <michael.strawbridge@amd.com>
> Date: Wed, 11 Oct 2023 16:13:13 -0400
> Subject: [PATCH v2] send-email: move validation code below
>  process_address_list
Why do these in-body headers to lie about the author date?

By the way, the in-body header does seem to support the header line folding (see the "subject" one here).

Michael Strawbridge· Oct 26, 2023, 13:11 UTC · re: Junio C Hamano · lore

Re: [PATCH v2] send-email: move validation code below process_address_list

On 10/26/23 08:44, Junio C Hamano wrote:
Show 9 quoted lines
> Michael Strawbridge <michael.strawbridge@amd.com> writes:
> 
>> From 67223238d9b1977d20b1286055d7f197e4d746e9 Mon Sep 17 00:00:00 2001
>> From: Michael Strawbridge <michael.strawbridge@amd.com>
>> Date: Wed, 11 Oct 2023 16:13:13 -0400
>> Subject: [PATCH v2] send-email: move validation code below
>>  process_address_list
> 
> Why do these in-body headers to lie about the author date?

Sorry. They weren't meant to. I have been amending my local commit rather than making new ones for every version. It seems that when I do that, the author date stays at the first time the commit was created. I wasn't aware of that unintended side effect.

> 
> By the way, the in-body header does seem to support the header line
> folding (see the "subject" one here).
Jeff King· Oct 25, 2023, 06:50 UTC · re: Michael Strawbridge · lore

Re: [PATCH] send-email: move validation code below process_address_list

On Tue, Oct 24, 2023 at 04:19:43PM -0400, Michael Strawbridge wrote:
Show 6 quoted lines
> Move validation logic below processing of email address lists so that
> email validation gets the proper email addresses.
> 
> This fixes email address validation errors when the optional
> perl module Email::Valid is installed and multiple addresses are passed
> in on a single to/cc argument like --to=foo@example.com,bar@example.com.
Is there a test we can include here?
Show 5 quoted lines
> @@ -2023,6 +1999,30 @@ sub process_file {
>  	return 1;
>  }
>  
> +if ($validate) {

So the new spot is right before we call process_file() on each of the input files. It is a little hard to follow because of the number of functions defined inline in the middle of the script, but I think that is a reasonable spot. It is after we have called process_address_list() on to/cc/bcc, which I think fixes the regression. But it is also after we sanitize $reply_to, etc, which seems like a good idea.

But I think putting it down that far is the source of the test failures.

The culprit seems not to be the call to validate_patch() in the loop you moved, but rather pre_process_file(), which was added in your earlier a8022c5f7b (send-email: expose header information to git-send-email's sendemail-validate hook, 2023-04-19).

It looks like the issue is the global $message_num variable which is incremented by pre_process_file(). On line 1755 (on the current tip of master), we set it to 0. And your patch moves the validation across there (from line ~799 to ~2023).

And that's why the extra increments didn't matter when you added the calls to pre_process_file() in your earlier patch; they all happened before we reset $message_num to 0. But now they happen after.

To be fair, this is absolutely horrific perl code. There's over a thousand lines of function definitions, and then hidden in the middle are some global variable assignments!

So we have a few options, I think:
  1. Reset $message_num to 0 after validating (maybe we also need
     to reset $in_reply_to, etc, set at the same time? I didn't check).
     This feels like a hack.
  2. Move the validation down, but not so far down. Like maybe right
     after we set up the @files list with the $compose.final name. This
     is the smallest diff, but feels like we haven't really made the
     world a better place.
  3. Move the $message_num, etc, initialization to right before we call
     the process_file() loop, which is what expects to use them. Like
     this (squashed into your patch):
diff --git a/git-send-email.perl b/git-send-email.perl
index a898dbc76e..d44db14223 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -1730,10 +1730,6 @@ sub send_message {
 	return 1;
 }
 
-$in_reply_to = $initial_in_reply_to;
-$references = $initial_in_reply_to || '';
-$message_num = 0;
-
 sub pre_process_file {
 	my ($t, $quiet) = @_;
 
@@ -2023,6 +2019,10 @@ sub process_file {
 	delete $ENV{GIT_SENDEMAIL_FILE_TOTAL};
 }
 
+$in_reply_to = $initial_in_reply_to;
+$references = $initial_in_reply_to || '';
+$message_num = 0;
+
 foreach my $t (@files) {
 	while (!process_file($t)) {
 		# user edited the file

That seems to make the test failures go away. It is still weird that the
validation code is calling pre_process_file(), which increments
$message_num, without us having set it up in any meaningful way. I'm not
sure if there are bugs lurking there or not. I'm not impressed by the
general quality of this code, and I'm kind of afraid to keep looking
deeper.

-Peff
Michael Strawbridge· Oct 25, 2023, 18:47 UTC · re: Jeff King · lore

Re: [PATCH] send-email: move validation code below process_address_list

On 10/25/23 02:50, Jeff King wrote:
Show 43 quoted lines
> On Tue, Oct 24, 2023 at 04:19:43PM -0400, Michael Strawbridge wrote:
> 
>> Move validation logic below processing of email address lists so that
>> email validation gets the proper email addresses.
>>
>> This fixes email address validation errors when the optional
>> perl module Email::Valid is installed and multiple addresses are passed
>> in on a single to/cc argument like --to=foo@example.com,bar@example.com.
> 
> Is there a test we can include here?
> 
>> @@ -2023,6 +1999,30 @@ sub process_file {
>>  	return 1;
>>  }
>>  
>> +if ($validate) {
> 
> So the new spot is right before we call process_file() on each of the
> input files. It is a little hard to follow because of the number of
> functions defined inline in the middle of the script, but I think that
> is a reasonable spot. It is after we have called process_address_list()
> on to/cc/bcc, which I think fixes the regression. But it is also after
> we sanitize $reply_to, etc, which seems like a good idea.
> 
> But I think putting it down that far is the source of the test failures.
> 
> The culprit seems not to be the call to validate_patch() in the loop you
> moved, but rather pre_process_file(), which was added in your earlier
> a8022c5f7b (send-email: expose header information to git-send-email's
> sendemail-validate hook, 2023-04-19).
> 
> It looks like the issue is the global $message_num variable which is
> incremented by pre_process_file(). On line 1755 (on the current tip of
> master), we set it to 0. And your patch moves the validation across
> there (from line ~799 to ~2023).
> 
> And that's why the extra increments didn't matter when you added the
> calls to pre_process_file() in your earlier patch; they all happened
> before we reset $message_num to 0. But now they happen after.
> 
> To be fair, this is absolutely horrific perl code. There's over a
> thousand lines of function definitions, and then hidden in the middle
> are some global variable assignments!
I agree. Following where things are initialized seems to be especially troublesome.
Show 43 quoted lines
> 
> So we have a few options, I think:
> 
>   1. Reset $message_num to 0 after validating (maybe we also need
>      to reset $in_reply_to, etc, set at the same time? I didn't check).
>      This feels like a hack.
> 
>   2. Move the validation down, but not so far down. Like maybe right
>      after we set up the @files list with the $compose.final name. This
>      is the smallest diff, but feels like we haven't really made the
>      world a better place.
> 
>   3. Move the $message_num, etc, initialization to right before we call
>      the process_file() loop, which is what expects to use them. Like
>      this (squashed into your patch):
> 
> diff --git a/git-send-email.perl b/git-send-email.perl
> index a898dbc76e..d44db14223 100755
> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -1730,10 +1730,6 @@ sub send_message {
>  	return 1;
>  }
>  
> -$in_reply_to = $initial_in_reply_to;
> -$references = $initial_in_reply_to || '';
> -$message_num = 0;
> -
>  sub pre_process_file {
>  	my ($t, $quiet) = @_;
>  
> @@ -2023,6 +2019,10 @@ sub process_file {
>  	delete $ENV{GIT_SENDEMAIL_FILE_TOTAL};
>  }
>  
> +$in_reply_to = $initial_in_reply_to;
> +$references = $initial_in_reply_to || '';
> +$message_num = 0;
> +
>  foreach my $t (@files) {
>  	while (!process_file($t)) {
>  		# user edited the file
> 

The above patch was a great place to start. Thank you! In order to address the fact that validation and actually sending the emails should have the same initial conditions I created a new function to set the variables and call it instead.

Show 8 quoted lines
> That seems to make the test failures go away. It is still weird that the
> validation code is calling pre_process_file(), which increments
> $message_num, without us having set it up in any meaningful way. I'm not
> sure if there are bugs lurking there or not. I'm not impressed by the
> general quality of this code, and I'm kind of afraid to keep looking
> deeper.
> 
> -Peff
Uwe Kleine-König· Oct 25, 2023, 07:43 UTC · re: Michael Strawbridge · lore

Re: [PATCH] send-email: move validation code below process_address_list

Hello,
On Tue, Oct 24, 2023 at 04:19:43PM -0400, Michael Strawbridge wrote:
Show 14 quoted lines
> >From 09ea51d63cebdf9ff0c073ef86e21b4b09c268e5 Mon Sep 17 00:00:00 2001
> From: Michael Strawbridge <michael.strawbridge@amd.com>
> Date: Wed, 11 Oct 2023 16:13:13 -0400
> Subject: [PATCH] send-email: move validation code below process_address_list
> 
> Move validation logic below processing of email address lists so that
> email validation gets the proper email addresses.
> 
> This fixes email address validation errors when the optional
> perl module Email::Valid is installed and multiple addresses are passed
> in on a single to/cc argument like --to=foo@example.com,bar@example.com.
> 
> Reported-by: Bagas Sanjaya <bagasdotme@gmail.com>
> Signed-off-by: Michael Strawbridge <michael.strawbridge@amd.com>
If you do Fixes: trailers as the kernel does, this could get:
Fixes: a8022c5f7b67 ("send-email: expose header information to git-send-email's sendemail-validate hook")

I tested this patch on top of main (2e8e77cbac8a) and it fixes the regression I reported in a separate thread (where Jeff pointed out this patch as fixing it).

Tested-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Thanks Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |
Junio C Hamano· Oct 27, 2023, 13:04 UTC · re: Uwe Kleine-König · lore

Re: [PATCH] send-email: move validation code below process_address_list

Uwe Kleine-König <u.kleine-koenig@pengutronix.de> writes:
Show 10 quoted lines
>> This fixes email address validation errors when the optional
>> perl module Email::Valid is installed and multiple addresses are passed
>> in on a single to/cc argument like --to=foo@example.com,bar@example.com.
>> 
>> Reported-by: Bagas Sanjaya <bagasdotme@gmail.com>
>> Signed-off-by: Michael Strawbridge <michael.strawbridge@amd.com>
>
> If you do Fixes: trailers as the kernel does, this could get:
>
> Fixes: a8022c5f7b67 ("send-email: expose header information to git-send-email's sendemail-validate hook")

While referring to a concrete commit object name is great, we tend not to use that particular trailer in this project; rather, we prefer to see description on how and in what way the culprit change was undesirable.

> I tested this patch on top of main (2e8e77cbac8a) and it fixes the
> regression I reported in a separate thread (where Jeff pointed out this
> patch as fixing it).
Great.  Thanks.
Bagas Sanjaya· Oct 20, 2023, 02:50 UTC · re: Michael Strawbridge · lore

Re: [PATCH] send-email: move process_address_list earlier to avoid, uninitialized address error

On Wed, Oct 11, 2023 at 04:22:18PM -0400, Michael Strawbridge wrote:
Show 34 quoted lines
> Move processing of email address lists before the sendemail-validate
> hook code.  This fixes email address validation errors when the optional
> perl module Email::Valid is installed and multiple addresses are passed
> in on a single to/cc argument like --to=foo@example.com,bar@example.com.
> ---
>  git-send-email.perl | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/git-send-email.perl b/git-send-email.perl
> index 288ea1ae80..cfd80c9d8b 100755
> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -799,6 +799,10 @@ sub is_format_patch_arg {
>  
>  $time = time - scalar $#files;
>  
> +@initial_to = process_address_list(@initial_to);
> +@initial_cc = process_address_list(@initial_cc);
> +@initial_bcc = process_address_list(@initial_bcc);
> +
>  if ($validate) {
>         # FIFOs can only be read once, exclude them from validation.
>         my @real_files = ();
> @@ -1099,10 +1103,6 @@ sub expand_one_alias {
>         return $aliases{$alias} ? expand_aliases(@{$aliases{$alias}}) : $alias;
>  }
>  
> -@initial_to = process_address_list(@initial_to);
> -@initial_cc = process_address_list(@initial_cc);
> -@initial_bcc = process_address_list(@initial_bcc);
> -
>  if ($thread && !defined $initial_in_reply_to && $prompting) {
>         $initial_in_reply_to = ask(
>                 __("Message-ID to be used as In-Reply-To for the first email (if any)? "),

Thanks for the fixup! The patch itself is whitespace-damaged, though, so I have to manually apply it. Regardless,

Tested-by: Bagas Sanjaya <bagasdotme@gmail.com>
-- 
An old man doll... just what I always wanted! - Clara
Bagas Sanjaya· Sep 26, 2023, 11:33 UTC · re: Jeff King · lore

Re: [REGRESSION] uninitialized value $address in git send-email when given multiple recipients separated by commas

On 25/09/2023 15:00, Jeff King wrote:
> Do you have any other send-email related config? Can you show us the
> output of "git config --list"?
> 
Hi Jeff, sure here is the output from my git.git clone:

``` $ git config --list core.abbrev=10 alias.am-failed=am --show-current-patch=diff alias.fixwhat=show -s --pretty=fixes user.name=Bagas Sanjaya user.email=bagasdotme@gmail.com user.signingkey=D0A10F5A447A37A42A63419FD7B55A665D5C863D format.signature=An old man doll... just what I always wanted! - Clara diff.algorithm=histogram sendemail.smtpencryption=tls sendemail.smtpserver=smtp.gmail.com sendemail.smtpserverport=587 sendemail.smtpuser=bagasdotme@gmail.com sendemail.smtppass=<app password> pack.deltacachesize=225M pack.window=13 pack.windowmemory=460M pack.threads=2 pretty.fixes=Fixes: %h ("%s") pretty.fixup=fixup for "%s" pretty.kreference=%h ("%s") pretty.upstream=commit %H upstream. pretty.upstreamsasha=[ Upstream commit %H ] merge.conflictstyle=diff3 tar.xz.command=xz -c tar.zst.command=zstd -c diff.algorithm=histogram filter.lfs.clean=git-lfs clean -- %f filter.lfs.smudge=git-lfs smudge -- %f filter.lfs.process=git-lfs filter-process filter.lfs.required=true core.repositoryformatversion=0 core.filemode=true core.bare=false core.logallrefupdates=true tag.sort=creatordate remote.origin.url=https://git.kernel.org/pub/scm/git/git.git remote.origin.fetch=+refs/heads/master:refs/remotes/origin/master remote.origin.fetch=+refs/heads/main:refs/remotes/origin/main remote.origin.fetch=+refs/heads/next:refs/remotes/origin/next remote.origin.fetch=+refs/heads/seen:refs/remotes/origin/seen remote.origin.fetch=+refs/heads/maint:refs/remotes/origin/maint remote.origin.fetch=+refs/heads/todo:refs/remotes/origin/todo sendemail.sendmailcmd=/usr/sbin/sendmail sendemail.envelopesender=auto branch.master.remote=origin branch.master.merge=refs/heads/master branch.next.remote=origin branch.next.merge=refs/heads/next branch.seen.remote=origin branch.seen.merge=refs/heads/seen branch.maint.remote=origin branch.maint.merge=refs/heads/maint branch.todo.remote=origin branch.todo.merge=refs/heads/todo sendemail.sendmailcmd=/usr/sbin/sendmail sendemail.envelopesender=auto branch.main.remote=origin branch.main.merge=refs/heads/main remote.gitnode.url=git@gitnode.io:bagas/git.git remote.gitnode.fetch=+refs/heads/*:refs/remotes/gitnode/* ```

Thanks.
-- 
An old man doll... just what I always wanted! - Clara

← back to recent threads