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

[Jeff King <peff@peff.net>]

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