All of lore.kernel.org
 help / color / mirror / Atom feed
* [REGRESSION] uninitialized value $address in git send-email when given multiple recipients separated by commas
@ 2023-09-22  9:27 Bagas Sanjaya
  2023-09-24  3:36 ` Jeff King
  0 siblings, 1 reply; 47+ messages in thread
From: Bagas Sanjaya @ 2023-09-22  9:27 UTC (permalink / raw)
  To: Michael Strawbridge, Junio C Hamano, Luben Tuikov,
	Ævar Arnfjörð Bjarmason, Taylor Blau, Jeff King
  Cc: Git Mailing List

[-- Attachment #1: Type: text/plain, Size: 4711 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

^ permalink raw reply	[flat|nested] 47+ messages in thread

* Re: [REGRESSION] uninitialized value $address in git send-email when given multiple recipients separated by commas
  2023-09-22  9:27 [REGRESSION] uninitialized value $address in git send-email when given multiple recipients separated by commas Bagas Sanjaya
@ 2023-09-24  3:36 ` Jeff King
  2023-09-25  7:45   ` Bagas Sanjaya
  0 siblings, 1 reply; 47+ messages in thread
From: Jeff King @ 2023-09-24  3:36 UTC (permalink / raw)
  To: Bagas Sanjaya
  Cc: Michael Strawbridge, Junio C Hamano, Luben Tuikov,
	Ævar Arnfjörð Bjarmason, Taylor Blau,
	Git Mailing List

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:

> 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.

> 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.

> 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

^ permalink raw reply	[flat|nested] 47+ messages in thread

* Re: [REGRESSION] uninitialized value $address in git send-email when given multiple recipients separated by commas
  2023-09-24  3:36 ` Jeff King
@ 2023-09-25  7:45   ` Bagas Sanjaya
  2023-09-25  8:00     ` Jeff King
  0 siblings, 1 reply; 47+ messages in thread
From: Bagas Sanjaya @ 2023-09-25  7:45 UTC (permalink / raw)
  To: Jeff King
  Cc: Michael Strawbridge, Junio C Hamano, Luben Tuikov,
	Ævar Arnfjörð Bjarmason, Taylor Blau,
	Git Mailing List

[-- Attachment #1: Type: text/plain, Size: 11172 bytes --]

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:

```
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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

^ permalink raw reply	[flat|nested] 47+ messages in thread

* Re: [REGRESSION] uninitialized value $address in git send-email when given multiple recipients separated by commas
  2023-09-25  7:45   ` Bagas Sanjaya
@ 2023-09-25  8:00     ` Jeff King
  2023-09-25 14:48       ` Todd Zullinger
  2023-09-26 11:33       ` [REGRESSION] uninitialized value $address in git send-email when given multiple recipients separated by commas Bagas Sanjaya
  0 siblings, 2 replies; 47+ messages in thread
From: Jeff King @ 2023-09-25  8:00 UTC (permalink / raw)
  To: Bagas Sanjaya
  Cc: Michael Strawbridge, Junio C Hamano, Luben Tuikov,
	Ævar Arnfjörð Bjarmason, Taylor Blau,
	Git Mailing List

On Mon, Sep 25, 2023 at 02:45:47PM +0700, Bagas Sanjaya wrote:

> 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).

> 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

^ permalink raw reply	[flat|nested] 47+ messages in thread

* Re: [REGRESSION] uninitialized value $address in git send-email when given multiple recipients separated by commas
  2023-09-25  8:00     ` Jeff King
@ 2023-09-25 14:48       ` Todd Zullinger
  2023-09-25 16:17         ` Jeff King
  2023-09-26 11:33       ` [REGRESSION] uninitialized value $address in git send-email when given multiple recipients separated by commas Bagas Sanjaya
  1 sibling, 1 reply; 47+ messages in thread
From: Todd Zullinger @ 2023-09-25 14:48 UTC (permalink / raw)
  To: Jeff King
  Cc: Bagas Sanjaya, Michael Strawbridge, Junio C Hamano, Luben Tuikov,
	Ævar Arnfjörð Bjarmason, Taylor Blau,
	Git Mailing List

Hi,

Jeff King wrote:
> 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

^ permalink raw reply	[flat|nested] 47+ messages in thread

* Re: [REGRESSION] uninitialized value $address in git send-email when given multiple recipients separated by commas
  2023-09-25 14:48       ` Todd Zullinger
@ 2023-09-25 16:17         ` Jeff King
  2023-10-11 13:41           ` Bagas Sanjaya
  0 siblings, 1 reply; 47+ messages in thread
From: Jeff King @ 2023-09-25 16:17 UTC (permalink / raw)
  To: Todd Zullinger
  Cc: Bagas Sanjaya, Michael Strawbridge, Junio C Hamano, Luben Tuikov,
	Ævar Arnfjörð Bjarmason, Taylor Blau,
	Git Mailing List

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

^ permalink raw reply	[flat|nested] 47+ messages in thread

* Re: [REGRESSION] uninitialized value $address in git send-email when given multiple recipients separated by commas
  2023-09-25  8:00     ` Jeff King
  2023-09-25 14:48       ` Todd Zullinger
@ 2023-09-26 11:33       ` Bagas Sanjaya
  1 sibling, 0 replies; 47+ messages in thread
From: Bagas Sanjaya @ 2023-09-26 11:33 UTC (permalink / raw)
  To: Jeff King
  Cc: Michael Strawbridge, Junio C Hamano, Luben Tuikov,
	Ævar Arnfjörð Bjarmason, Taylor Blau,
	Git Mailing List

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


^ permalink raw reply	[flat|nested] 47+ messages in thread

* Re: [REGRESSION] uninitialized value $address in git send-email when given multiple recipients separated by commas
  2023-09-25 16:17         ` Jeff King
@ 2023-10-11 13:41           ` Bagas Sanjaya
  2023-10-11 19:27             ` Michael Strawbridge
  0 siblings, 1 reply; 47+ messages in thread
From: Bagas Sanjaya @ 2023-10-11 13:41 UTC (permalink / raw)
  To: Jeff King, Todd Zullinger
  Cc: Michael Strawbridge, Junio C Hamano, Luben Tuikov,
	Ævar Arnfjörð Bjarmason, Taylor Blau,
	Git Mailing List

[-- Attachment #1: Type: text/plain, Size: 3282 bytes --]

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?

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

^ permalink raw reply	[flat|nested] 47+ messages in thread

* Re: [REGRESSION] uninitialized value $address in git send-email when given multiple recipients separated by commas
  2023-10-11 13:41           ` Bagas Sanjaya
@ 2023-10-11 19:27             ` Michael Strawbridge
  2023-10-11 20:22               ` [PATCH] send-email: move process_address_list earlier to avoid, uninitialized address error Michael Strawbridge
  0 siblings, 1 reply; 47+ messages in thread
From: Michael Strawbridge @ 2023-10-11 19:27 UTC (permalink / raw)
  To: Bagas Sanjaya, Jeff King, Todd Zullinger
  Cc: Junio C Hamano, Luben Tuikov,
	Ævar Arnfjörð Bjarmason, Taylor Blau,
	Git Mailing List


On 10/11/23 09:41, Bagas Sanjaya wrote:
> 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.


^ permalink raw reply	[flat|nested] 47+ messages in thread

* [PATCH] send-email: move process_address_list earlier to avoid, uninitialized address error
  2023-10-11 19:27             ` Michael Strawbridge
@ 2023-10-11 20:22               ` Michael Strawbridge
  2023-10-11 20:25                 ` Michael Strawbridge
                                   ` (2 more replies)
  0 siblings, 3 replies; 47+ messages in thread
From: Michael Strawbridge @ 2023-10-11 20:22 UTC (permalink / raw)
  To: Bagas Sanjaya, Jeff King, Todd Zullinger
  Cc: Junio C Hamano, Luben Tuikov,
	Ævar Arnfjörð Bjarmason, Taylor Blau,
	Git Mailing List

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

^ permalink raw reply related	[flat|nested] 47+ messages in thread

* Re: [PATCH] send-email: move process_address_list earlier to avoid, uninitialized address error
  2023-10-11 20:22               ` [PATCH] send-email: move process_address_list earlier to avoid, uninitialized address error Michael Strawbridge
@ 2023-10-11 20:25                 ` Michael Strawbridge
  2023-10-11 21:27                 ` Junio C Hamano
  2023-10-20  2:50                 ` [PATCH] send-email: move process_address_list earlier to avoid, uninitialized address error Bagas Sanjaya
  2 siblings, 0 replies; 47+ messages in thread
From: Michael Strawbridge @ 2023-10-11 20:25 UTC (permalink / raw)
  To: Bagas Sanjaya, Jeff King, Todd Zullinger
  Cc: Junio C Hamano, Luben Tuikov,
	Ævar Arnfjörð Bjarmason, Taylor Blau,
	Git Mailing List

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:
> 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)? "),

^ permalink raw reply	[flat|nested] 47+ messages in thread

* Re: [PATCH] send-email: move process_address_list earlier to avoid, uninitialized address error
  2023-10-11 20:22               ` [PATCH] send-email: move process_address_list earlier to avoid, uninitialized address error Michael Strawbridge
  2023-10-11 20:25                 ` Michael Strawbridge
@ 2023-10-11 21:27                 ` Junio C Hamano
  2023-10-11 22:18                   ` Jeff King
  2023-10-20  2:50                 ` [PATCH] send-email: move process_address_list earlier to avoid, uninitialized address error Bagas Sanjaya
  2 siblings, 1 reply; 47+ messages in thread
From: Junio C Hamano @ 2023-10-11 21:27 UTC (permalink / raw)
  To: Michael Strawbridge
  Cc: Bagas Sanjaya, Jeff King, Todd Zullinger, Luben Tuikov,
	Ævar Arnfjörð Bjarmason, Taylor Blau,
	Git Mailing List

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?

>  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 ...

> @@ -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)? "),

^ permalink raw reply	[flat|nested] 47+ messages in thread

* Re: [PATCH] send-email: move process_address_list earlier to avoid, uninitialized address error
  2023-10-11 21:27                 ` Junio C Hamano
@ 2023-10-11 22:18                   ` Jeff King
  2023-10-11 22:37                     ` Junio C Hamano
  0 siblings, 1 reply; 47+ messages in thread
From: Jeff King @ 2023-10-11 22:18 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: Michael Strawbridge, Bagas Sanjaya, Todd Zullinger, Luben Tuikov,
	Ævar Arnfjörð Bjarmason, Taylor Blau,
	Git Mailing List

On Wed, Oct 11, 2023 at 02:27:49PM -0700, Junio C Hamano wrote:

> 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.

> >  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

^ permalink raw reply	[flat|nested] 47+ messages in thread

* Re: [PATCH] send-email: move process_address_list earlier to avoid, uninitialized address error
  2023-10-11 22:18                   ` Jeff King
@ 2023-10-11 22:37                     ` Junio C Hamano
  2023-10-11 22:47                       ` Jeff King
  0 siblings, 1 reply; 47+ messages in thread
From: Junio C Hamano @ 2023-10-11 22:37 UTC (permalink / raw)
  To: Jeff King
  Cc: Michael Strawbridge, Bagas Sanjaya, Todd Zullinger, Luben Tuikov,
	Ævar Arnfjörð Bjarmason, Taylor Blau,
	Git Mailing List

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.

> 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).

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.


^ permalink raw reply	[flat|nested] 47+ messages in thread

* Re: [PATCH] send-email: move process_address_list earlier to avoid, uninitialized address error
  2023-10-11 22:37                     ` Junio C Hamano
@ 2023-10-11 22:47                       ` Jeff King
  2023-10-13 20:25                         ` Michael Strawbridge
  0 siblings, 1 reply; 47+ messages in thread
From: Jeff King @ 2023-10-11 22:47 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: Michael Strawbridge, Bagas Sanjaya, Todd Zullinger, Luben Tuikov,
	Ævar Arnfjörð Bjarmason, Taylor Blau,
	Git Mailing List

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. ;)

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

^ permalink raw reply	[flat|nested] 47+ messages in thread

* Re: [PATCH] send-email: move process_address_list earlier to avoid, uninitialized address error
  2023-10-11 22:47                       ` Jeff King
@ 2023-10-13 20:25                         ` Michael Strawbridge
  2023-10-20  6:45                           ` Jeff King
  0 siblings, 1 reply; 47+ messages in thread
From: Michael Strawbridge @ 2023-10-13 20:25 UTC (permalink / raw)
  To: Jeff King, Junio C Hamano
  Cc: Bagas Sanjaya, Todd Zullinger, Luben Tuikov,
	Ævar Arnfjörð Bjarmason, Taylor Blau,
	Git Mailing List, Uwe Kleine-König


On 10/11/23 18:47, Jeff King wrote:
> 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.
>
> 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.

^ permalink raw reply	[flat|nested] 47+ messages in thread

* Re: [PATCH] send-email: move process_address_list earlier to avoid, uninitialized address error
  2023-10-11 20:22               ` [PATCH] send-email: move process_address_list earlier to avoid, uninitialized address error Michael Strawbridge
  2023-10-11 20:25                 ` Michael Strawbridge
  2023-10-11 21:27                 ` Junio C Hamano
@ 2023-10-20  2:50                 ` Bagas Sanjaya
  2 siblings, 0 replies; 47+ messages in thread
From: Bagas Sanjaya @ 2023-10-20  2:50 UTC (permalink / raw)
  To: Michael Strawbridge, Jeff King, Todd Zullinger
  Cc: Junio C Hamano, Luben Tuikov,
	Ævar Arnfjörð Bjarmason, Taylor Blau,
	Git Mailing List

[-- Attachment #1: Type: text/plain, Size: 1802 bytes --]

On Wed, Oct 11, 2023 at 04:22:18PM -0400, Michael Strawbridge wrote:
> 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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

^ permalink raw reply	[flat|nested] 47+ messages in thread

* Re: [PATCH] send-email: move process_address_list earlier to avoid, uninitialized address error
  2023-10-13 20:25                         ` Michael Strawbridge
@ 2023-10-20  6:45                           ` Jeff King
  2023-10-20  7:14                             ` Jeff King
  0 siblings, 1 reply; 47+ messages in thread
From: Jeff King @ 2023-10-20  6:45 UTC (permalink / raw)
  To: Michael Strawbridge
  Cc: Junio C Hamano, Bagas Sanjaya, Todd Zullinger, Luben Tuikov,
	Ævar Arnfjörð Bjarmason, Taylor Blau,
	Git Mailing List, Uwe Kleine-König

On Fri, Oct 13, 2023 at 04:25:57PM -0400, Michael Strawbridge wrote:

> > 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.

> 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

^ permalink raw reply related	[flat|nested] 47+ messages in thread

* Re: [PATCH] send-email: move process_address_list earlier to avoid, uninitialized address error
  2023-10-20  6:45                           ` Jeff King
@ 2023-10-20  7:14                             ` Jeff King
  2023-10-20 10:03                               ` [PATCH 0/3] some send-email --compose fixes Jeff King
  0 siblings, 1 reply; 47+ messages in thread
From: Jeff King @ 2023-10-20  7:14 UTC (permalink / raw)
  To: Michael Strawbridge
  Cc: Junio C Hamano, Bagas Sanjaya, Todd Zullinger, Luben Tuikov,
	Ævar Arnfjörð Bjarmason, Taylor Blau,
	Git Mailing List, Uwe Kleine-König

On Fri, Oct 20, 2023 at 02:45:25AM -0400, Jeff King wrote:

> > 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

^ permalink raw reply related	[flat|nested] 47+ messages in thread

* [PATCH 0/3] some send-email --compose fixes
  2023-10-20  7:14                             ` Jeff King
@ 2023-10-20 10:03                               ` Jeff King
  2023-10-20 10:09                                 ` [PATCH 1/3] doc/send-email: mention handling of "reply-to" with --compose Jeff King
                                                   ` (3 more replies)
  0 siblings, 4 replies; 47+ messages in thread
From: Jeff King @ 2023-10-20 10:03 UTC (permalink / raw)
  To: Michael Strawbridge; +Cc: Junio C Hamano, Bagas Sanjaya, Git Mailing List

[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

 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

^ permalink raw reply	[flat|nested] 47+ messages in thread

* [PATCH 1/3] doc/send-email: mention handling of "reply-to" with --compose
  2023-10-20 10:03                               ` [PATCH 0/3] some send-email --compose fixes Jeff King
@ 2023-10-20 10:09                                 ` Jeff King
  2023-10-20 10:13                                 ` [PATCH 2/3] Revert "send-email: extract email-parsing code into a subroutine" Jeff King
                                                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 47+ messages in thread
From: Jeff King @ 2023-10-20 10:09 UTC (permalink / raw)
  To: Michael Strawbridge; +Cc: Junio C Hamano, Bagas Sanjaya, Git Mailing List

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


^ permalink raw reply related	[flat|nested] 47+ messages in thread

* [PATCH 2/3] Revert "send-email: extract email-parsing code into a subroutine"
  2023-10-20 10:03                               ` [PATCH 0/3] some send-email --compose fixes Jeff King
  2023-10-20 10:09                                 ` [PATCH 1/3] doc/send-email: mention handling of "reply-to" with --compose Jeff King
@ 2023-10-20 10:13                                 ` Jeff King
  2023-10-20 10:45                                   ` Oswald Buddenhagen
  2023-10-20 21:45                                   ` Junio C Hamano
  2023-10-20 10:15                                 ` [PATCH 3/3] send-email: handle to/cc/bcc from --compose message Jeff King
  2023-10-20 21:42                                 ` [PATCH 0/3] some send-email --compose fixes Junio C Hamano
  3 siblings, 2 replies; 47+ messages in thread
From: Jeff King @ 2023-10-20 10:13 UTC (permalink / raw)
  To: Michael Strawbridge; +Cc: Junio C Hamano, Bagas Sanjaya, Git Mailing List

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


^ permalink raw reply related	[flat|nested] 47+ messages in thread

* [PATCH 3/3] send-email: handle to/cc/bcc from --compose message
  2023-10-20 10:03                               ` [PATCH 0/3] some send-email --compose fixes Jeff King
  2023-10-20 10:09                                 ` [PATCH 1/3] doc/send-email: mention handling of "reply-to" with --compose Jeff King
  2023-10-20 10:13                                 ` [PATCH 2/3] Revert "send-email: extract email-parsing code into a subroutine" Jeff King
@ 2023-10-20 10:15                                 ` Jeff King
  2023-10-20 17:30                                   ` Eric Sunshine
  2023-10-20 21:42                                 ` [PATCH 0/3] some send-email --compose fixes Junio C Hamano
  3 siblings, 1 reply; 47+ messages in thread
From: Jeff King @ 2023-10-20 10:15 UTC (permalink / raw)
  To: Michael Strawbridge; +Cc: Junio C Hamano, Bagas Sanjaya, Git Mailing List

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

^ permalink raw reply related	[flat|nested] 47+ messages in thread

* Re: [PATCH 2/3] Revert "send-email: extract email-parsing code into a subroutine"
  2023-10-20 10:13                                 ` [PATCH 2/3] Revert "send-email: extract email-parsing code into a subroutine" Jeff King
@ 2023-10-20 10:45                                   ` Oswald Buddenhagen
  2023-10-23 18:40                                     ` Jeff King
  2023-10-20 21:45                                   ` Junio C Hamano
  1 sibling, 1 reply; 47+ messages in thread
From: Oswald Buddenhagen @ 2023-10-20 10:45 UTC (permalink / raw)
  To: Jeff King
  Cc: Michael Strawbridge, Junio C Hamano, Bagas Sanjaya, Git Mailing List

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?

>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

^ permalink raw reply	[flat|nested] 47+ messages in thread

* Re: [PATCH 3/3] send-email: handle to/cc/bcc from --compose message
  2023-10-20 10:15                                 ` [PATCH 3/3] send-email: handle to/cc/bcc from --compose message Jeff King
@ 2023-10-20 17:30                                   ` Eric Sunshine
  0 siblings, 0 replies; 47+ messages in thread
From: Eric Sunshine @ 2023-10-20 17:30 UTC (permalink / raw)
  To: Jeff King
  Cc: Michael Strawbridge, Junio C Hamano, Bagas Sanjaya, Git Mailing List

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/

> 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>

^ permalink raw reply	[flat|nested] 47+ messages in thread

* Re: [PATCH 0/3] some send-email --compose fixes
  2023-10-20 10:03                               ` [PATCH 0/3] some send-email --compose fixes Jeff King
                                                   ` (2 preceding siblings ...)
  2023-10-20 10:15                                 ` [PATCH 3/3] send-email: handle to/cc/bcc from --compose message Jeff King
@ 2023-10-20 21:42                                 ` Junio C Hamano
  2023-10-23 18:51                                   ` Jeff King
  3 siblings, 1 reply; 47+ messages in thread
From: Junio C Hamano @ 2023-10-20 21:42 UTC (permalink / raw)
  To: Jeff King; +Cc: Michael Strawbridge, Bagas Sanjaya, Git Mailing List

Jeff King <peff@peff.net> writes:

> [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.

^ permalink raw reply	[flat|nested] 47+ messages in thread

* Re: [PATCH 2/3] Revert "send-email: extract email-parsing code into a subroutine"
  2023-10-20 10:13                                 ` [PATCH 2/3] Revert "send-email: extract email-parsing code into a subroutine" Jeff King
  2023-10-20 10:45                                   ` Oswald Buddenhagen
@ 2023-10-20 21:45                                   ` Junio C Hamano
  2023-10-23 18:47                                     ` Jeff King
  1 sibling, 1 reply; 47+ messages in thread
From: Junio C Hamano @ 2023-10-20 21:45 UTC (permalink / raw)
  To: Jeff King; +Cc: Michael Strawbridge, Bagas Sanjaya, Git Mailing List

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" ;-)

^ permalink raw reply	[flat|nested] 47+ messages in thread

* Re: [PATCH 2/3] Revert "send-email: extract email-parsing code into a subroutine"
  2023-10-20 10:45                                   ` Oswald Buddenhagen
@ 2023-10-23 18:40                                     ` Jeff King
  2023-10-23 19:50                                       ` Oswald Buddenhagen
  0 siblings, 1 reply; 47+ messages in thread
From: Jeff King @ 2023-10-23 18:40 UTC (permalink / raw)
  To: Oswald Buddenhagen
  Cc: Michael Strawbridge, Junio C Hamano, Bagas Sanjaya, Git Mailing List

On Fri, Oct 20, 2023 at 12:45:43PM +0200, Oswald Buddenhagen wrote:

> 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).

> > (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

^ permalink raw reply	[flat|nested] 47+ messages in thread

* Re: [PATCH 2/3] Revert "send-email: extract email-parsing code into a subroutine"
  2023-10-20 21:45                                   ` Junio C Hamano
@ 2023-10-23 18:47                                     ` Jeff King
  0 siblings, 0 replies; 47+ messages in thread
From: Jeff King @ 2023-10-23 18:47 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Michael Strawbridge, Bagas Sanjaya, Git Mailing List

On Fri, Oct 20, 2023 at 02:45:30PM -0700, Junio C Hamano wrote:

> 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

^ permalink raw reply	[flat|nested] 47+ messages in thread

* Re: [PATCH 0/3] some send-email --compose fixes
  2023-10-20 21:42                                 ` [PATCH 0/3] some send-email --compose fixes Junio C Hamano
@ 2023-10-23 18:51                                   ` Jeff King
  2023-10-24 20:12                                     ` Michael Strawbridge
  0 siblings, 1 reply; 47+ messages in thread
From: Jeff King @ 2023-10-23 18:51 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Michael Strawbridge, Bagas Sanjaya, Git Mailing List

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.

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

^ permalink raw reply	[flat|nested] 47+ messages in thread

* Re: [PATCH 2/3] Revert "send-email: extract email-parsing code into a subroutine"
  2023-10-23 18:40                                     ` Jeff King
@ 2023-10-23 19:50                                       ` Oswald Buddenhagen
  2023-10-25  6:11                                         ` Jeff King
  0 siblings, 1 reply; 47+ messages in thread
From: Oswald Buddenhagen @ 2023-10-23 19:50 UTC (permalink / raw)
  To: Jeff King
  Cc: Michael Strawbridge, Junio C Hamano, Bagas Sanjaya, Git Mailing List

On Mon, Oct 23, 2023 at 02:40:10PM -0400, Jeff King wrote:
>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.

>> > 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.

>> > 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

^ permalink raw reply	[flat|nested] 47+ messages in thread

* Re: [PATCH 0/3] some send-email --compose fixes
  2023-10-23 18:51                                   ` Jeff King
@ 2023-10-24 20:12                                     ` Michael Strawbridge
  2023-10-24 20:19                                       ` [PATCH] send-email: move validation code below process_address_list Michael Strawbridge
  0 siblings, 1 reply; 47+ messages in thread
From: Michael Strawbridge @ 2023-10-24 20:12 UTC (permalink / raw)
  To: Jeff King, Junio C Hamano; +Cc: Bagas Sanjaya, Git Mailing List



On 10/23/23 14:51, Jeff King wrote:
> 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.
> 
> 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.

>> 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

^ permalink raw reply	[flat|nested] 47+ messages in thread

* [PATCH] send-email: move validation code below process_address_list
  2023-10-24 20:12                                     ` Michael Strawbridge
@ 2023-10-24 20:19                                       ` Michael Strawbridge
  2023-10-24 21:55                                         ` Junio C Hamano
                                                           ` (2 more replies)
  0 siblings, 3 replies; 47+ messages in thread
From: Michael Strawbridge @ 2023-10-24 20:19 UTC (permalink / raw)
  To: Jeff King, Junio C Hamano; +Cc: Bagas Sanjaya, Git Mailing 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

^ permalink raw reply related	[flat|nested] 47+ messages in thread

* Re: [PATCH] send-email: move validation code below process_address_list
  2023-10-24 20:19                                       ` [PATCH] send-email: move validation code below process_address_list Michael Strawbridge
@ 2023-10-24 21:55                                         ` Junio C Hamano
  2023-10-24 22:03                                           ` Junio C Hamano
  2023-10-25  6:50                                         ` [PATCH] " Jeff King
  2023-10-25  7:43                                         ` Uwe Kleine-König
  2 siblings, 1 reply; 47+ messages in thread
From: Junio C Hamano @ 2023-10-24 21:55 UTC (permalink / raw)
  To: Michael Strawbridge; +Cc: Jeff King, Bagas Sanjaya, Git Mailing 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


^ permalink raw reply	[flat|nested] 47+ messages in thread

* Re: [PATCH] send-email: move validation code below process_address_list
  2023-10-24 21:55                                         ` Junio C Hamano
@ 2023-10-24 22:03                                           ` Junio C Hamano
  2023-10-25 18:48                                             ` Michael Strawbridge
  2023-10-25 18:51                                             ` [PATCH v2] " Michael Strawbridge
  0 siblings, 2 replies; 47+ messages in thread
From: Junio C Hamano @ 2023-10-24 22:03 UTC (permalink / raw)
  To: Michael Strawbridge; +Cc: Jeff King, Bagas Sanjaya, Git Mailing List

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.

^ permalink raw reply	[flat|nested] 47+ messages in thread

* Re: [PATCH 2/3] Revert "send-email: extract email-parsing code into a subroutine"
  2023-10-23 19:50                                       ` Oswald Buddenhagen
@ 2023-10-25  6:11                                         ` Jeff King
  2023-10-25  9:23                                           ` Oswald Buddenhagen
  0 siblings, 1 reply; 47+ messages in thread
From: Jeff King @ 2023-10-25  6:11 UTC (permalink / raw)
  To: Oswald Buddenhagen
  Cc: Michael Strawbridge, Junio C Hamano, Bagas Sanjaya, Git Mailing List

On Mon, Oct 23, 2023 at 09:50:54PM +0200, Oswald Buddenhagen wrote:

> > 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

^ permalink raw reply	[flat|nested] 47+ messages in thread

* Re: [PATCH] send-email: move validation code below process_address_list
  2023-10-24 20:19                                       ` [PATCH] send-email: move validation code below process_address_list Michael Strawbridge
  2023-10-24 21:55                                         ` Junio C Hamano
@ 2023-10-25  6:50                                         ` Jeff King
  2023-10-25 18:47                                           ` Michael Strawbridge
  2023-10-25  7:43                                         ` Uwe Kleine-König
  2 siblings, 1 reply; 47+ messages in thread
From: Jeff King @ 2023-10-25  6:50 UTC (permalink / raw)
  To: Michael Strawbridge; +Cc: Junio C Hamano, Bagas Sanjaya, Git Mailing List

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!

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

^ permalink raw reply related	[flat|nested] 47+ messages in thread

* Re: [PATCH] send-email: move validation code below process_address_list
  2023-10-24 20:19                                       ` [PATCH] send-email: move validation code below process_address_list Michael Strawbridge
  2023-10-24 21:55                                         ` Junio C Hamano
  2023-10-25  6:50                                         ` [PATCH] " Jeff King
@ 2023-10-25  7:43                                         ` Uwe Kleine-König
  2023-10-27 13:04                                           ` Junio C Hamano
  2 siblings, 1 reply; 47+ messages in thread
From: Uwe Kleine-König @ 2023-10-25  7:43 UTC (permalink / raw)
  To: Michael Strawbridge
  Cc: Jeff King, Junio C Hamano, Bagas Sanjaya, Git Mailing List

[-- Attachment #1: Type: text/plain, Size: 1386 bytes --]

Hello,

On Tue, Oct 24, 2023 at 04:19:43PM -0400, Michael Strawbridge wrote:
> >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/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 47+ messages in thread

* Re: [PATCH 2/3] Revert "send-email: extract email-parsing code into a subroutine"
  2023-10-25  6:11                                         ` Jeff King
@ 2023-10-25  9:23                                           ` Oswald Buddenhagen
  2023-10-27 22:31                                             ` Junio C Hamano
  2023-10-30  9:13                                             ` Jeff King
  0 siblings, 2 replies; 47+ messages in thread
From: Oswald Buddenhagen @ 2023-10-25  9:23 UTC (permalink / raw)
  To: Jeff King
  Cc: Michael Strawbridge, Junio C Hamano, Bagas Sanjaya, Git Mailing List

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

^ permalink raw reply	[flat|nested] 47+ messages in thread

* Re: [PATCH] send-email: move validation code below process_address_list
  2023-10-25  6:50                                         ` [PATCH] " Jeff King
@ 2023-10-25 18:47                                           ` Michael Strawbridge
  0 siblings, 0 replies; 47+ messages in thread
From: Michael Strawbridge @ 2023-10-25 18:47 UTC (permalink / raw)
  To: Jeff King; +Cc: Junio C Hamano, Bagas Sanjaya, Git Mailing List



On 10/25/23 02:50, Jeff King wrote:
> 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.
> 
> 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.
> 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

^ permalink raw reply	[flat|nested] 47+ messages in thread

* Re: [PATCH] send-email: move validation code below process_address_list
  2023-10-24 22:03                                           ` Junio C Hamano
@ 2023-10-25 18:48                                             ` Michael Strawbridge
  2023-10-25 18:51                                             ` [PATCH v2] " Michael Strawbridge
  1 sibling, 0 replies; 47+ messages in thread
From: Michael Strawbridge @ 2023-10-25 18:48 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Jeff King, Bagas Sanjaya, Git Mailing List



On 10/24/23 18:03, Junio C Hamano wrote:
> 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.

^ permalink raw reply	[flat|nested] 47+ messages in thread

* [PATCH v2] send-email: move validation code below process_address_list
  2023-10-24 22:03                                           ` Junio C Hamano
  2023-10-25 18:48                                             ` Michael Strawbridge
@ 2023-10-25 18:51                                             ` Michael Strawbridge
  2023-10-26 12:44                                               ` Junio C Hamano
  1 sibling, 1 reply; 47+ messages in thread
From: Michael Strawbridge @ 2023-10-25 18:51 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Jeff King, Bagas Sanjaya, Git Mailing 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

^ permalink raw reply related	[flat|nested] 47+ messages in thread

* Re: [PATCH v2] send-email: move validation code below process_address_list
  2023-10-25 18:51                                             ` [PATCH v2] " Michael Strawbridge
@ 2023-10-26 12:44                                               ` Junio C Hamano
  2023-10-26 13:11                                                 ` Michael Strawbridge
  0 siblings, 1 reply; 47+ messages in thread
From: Junio C Hamano @ 2023-10-26 12:44 UTC (permalink / raw)
  To: Michael Strawbridge; +Cc: Jeff King, Bagas Sanjaya, Git Mailing List

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?

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

^ permalink raw reply	[flat|nested] 47+ messages in thread

* Re: [PATCH v2] send-email: move validation code below process_address_list
  2023-10-26 12:44                                               ` Junio C Hamano
@ 2023-10-26 13:11                                                 ` Michael Strawbridge
  0 siblings, 0 replies; 47+ messages in thread
From: Michael Strawbridge @ 2023-10-26 13:11 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Jeff King, Bagas Sanjaya, Git Mailing List



On 10/26/23 08:44, Junio C Hamano wrote:
> 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).

^ permalink raw reply	[flat|nested] 47+ messages in thread

* Re: [PATCH] send-email: move validation code below process_address_list
  2023-10-25  7:43                                         ` Uwe Kleine-König
@ 2023-10-27 13:04                                           ` Junio C Hamano
  0 siblings, 0 replies; 47+ messages in thread
From: Junio C Hamano @ 2023-10-27 13:04 UTC (permalink / raw)
  To: Uwe Kleine-König
  Cc: Michael Strawbridge, Jeff King, Bagas Sanjaya, Git Mailing List

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

>> 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.

^ permalink raw reply	[flat|nested] 47+ messages in thread

* Re: [PATCH 2/3] Revert "send-email: extract email-parsing code into a subroutine"
  2023-10-25  9:23                                           ` Oswald Buddenhagen
@ 2023-10-27 22:31                                             ` Junio C Hamano
  2023-10-30  9:13                                             ` Jeff King
  1 sibling, 0 replies; 47+ messages in thread
From: Junio C Hamano @ 2023-10-27 22:31 UTC (permalink / raw)
  To: Oswald Buddenhagen
  Cc: Jeff King, Michael Strawbridge, Bagas Sanjaya, Git Mailing List

Oswald Buddenhagen <oswald.buddenhagen@gmx.de> writes:

> 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.

^ permalink raw reply	[flat|nested] 47+ messages in thread

* Re: [PATCH 2/3] Revert "send-email: extract email-parsing code into a subroutine"
  2023-10-25  9:23                                           ` Oswald Buddenhagen
  2023-10-27 22:31                                             ` Junio C Hamano
@ 2023-10-30  9:13                                             ` Jeff King
  1 sibling, 0 replies; 47+ messages in thread
From: Jeff King @ 2023-10-30  9:13 UTC (permalink / raw)
  To: Oswald Buddenhagen
  Cc: Michael Strawbridge, Junio C Hamano, Bagas Sanjaya, Git Mailing List

On Wed, Oct 25, 2023 at 11:23:01AM +0200, Oswald Buddenhagen wrote:

> 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

^ permalink raw reply	[flat|nested] 47+ messages in thread

end of thread, other threads:[~2023-10-30  9:13 UTC | newest]

Thread overview: 47+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-09-22  9:27 [REGRESSION] uninitialized value $address in git send-email when given multiple recipients separated by commas Bagas Sanjaya
2023-09-24  3:36 ` Jeff King
2023-09-25  7:45   ` Bagas Sanjaya
2023-09-25  8:00     ` Jeff King
2023-09-25 14:48       ` Todd Zullinger
2023-09-25 16:17         ` Jeff King
2023-10-11 13:41           ` Bagas Sanjaya
2023-10-11 19:27             ` Michael Strawbridge
2023-10-11 20:22               ` [PATCH] send-email: move process_address_list earlier to avoid, uninitialized address error Michael Strawbridge
2023-10-11 20:25                 ` Michael Strawbridge
2023-10-11 21:27                 ` Junio C Hamano
2023-10-11 22:18                   ` Jeff King
2023-10-11 22:37                     ` Junio C Hamano
2023-10-11 22:47                       ` Jeff King
2023-10-13 20:25                         ` Michael Strawbridge
2023-10-20  6:45                           ` Jeff King
2023-10-20  7:14                             ` Jeff King
2023-10-20 10:03                               ` [PATCH 0/3] some send-email --compose fixes Jeff King
2023-10-20 10:09                                 ` [PATCH 1/3] doc/send-email: mention handling of "reply-to" with --compose Jeff King
2023-10-20 10:13                                 ` [PATCH 2/3] Revert "send-email: extract email-parsing code into a subroutine" Jeff King
2023-10-20 10:45                                   ` Oswald Buddenhagen
2023-10-23 18:40                                     ` Jeff King
2023-10-23 19:50                                       ` Oswald Buddenhagen
2023-10-25  6:11                                         ` Jeff King
2023-10-25  9:23                                           ` Oswald Buddenhagen
2023-10-27 22:31                                             ` Junio C Hamano
2023-10-30  9:13                                             ` Jeff King
2023-10-20 21:45                                   ` Junio C Hamano
2023-10-23 18:47                                     ` Jeff King
2023-10-20 10:15                                 ` [PATCH 3/3] send-email: handle to/cc/bcc from --compose message Jeff King
2023-10-20 17:30                                   ` Eric Sunshine
2023-10-20 21:42                                 ` [PATCH 0/3] some send-email --compose fixes Junio C Hamano
2023-10-23 18:51                                   ` Jeff King
2023-10-24 20:12                                     ` Michael Strawbridge
2023-10-24 20:19                                       ` [PATCH] send-email: move validation code below process_address_list Michael Strawbridge
2023-10-24 21:55                                         ` Junio C Hamano
2023-10-24 22:03                                           ` Junio C Hamano
2023-10-25 18:48                                             ` Michael Strawbridge
2023-10-25 18:51                                             ` [PATCH v2] " Michael Strawbridge
2023-10-26 12:44                                               ` Junio C Hamano
2023-10-26 13:11                                                 ` Michael Strawbridge
2023-10-25  6:50                                         ` [PATCH] " Jeff King
2023-10-25 18:47                                           ` Michael Strawbridge
2023-10-25  7:43                                         ` Uwe Kleine-König
2023-10-27 13:04                                           ` Junio C Hamano
2023-10-20  2:50                 ` [PATCH] send-email: move process_address_list earlier to avoid, uninitialized address error Bagas Sanjaya
2023-09-26 11:33       ` [REGRESSION] uninitialized value $address in git send-email when given multiple recipients separated by commas Bagas Sanjaya

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.