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

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 11, 2023, 22:37 UTC
Message-ID: <xmqqzg0oiy4s.fsf@gitster.g>
In-Reply-To: <20231011221844.GB518221@coredump.intra.peff.net>

Jeff King <peff@peff.net> writes:

> Which of course implies that we're not (and cannot) validate what
> they're typing at this step, but I think that's OK because we feed it
> through extract_valid_address_or_die().

OK, let's queue it then.

Show 10 quoted lines

> IOW, I think there are actually two distinct validation steps
> hidden here:
>
>   1. We want to validate that the patch files we were fed are OK.
>
>   2. We want to validate that the addresses, etc, fed by the user are
>      OK.
>
> And after Michael's original patch, we are accidentally hitting some of
> that validation code for (2) while doing (1).

Show 15 quoted lines

> This is actually a weird split if you think about it. We are feeding to
> the validate hook in (1), so surely it would want to see the full set of
> inputs from the user, too? Which argues for pushing the "if ($validate)"
> down as you suggest. And then either:
>
>   a. We accept that the user experience is a little worse if validation
>      fails after the user typed.
>
>   b. We split (1) into "early" validation that just checks if the files
>      are OK, but doesn't call the hook. And then later on we do the full
>      validation.
>
> I don't have a strong opinion myself (I don't even use send-email
> myself, and if I did, I'd probably mostly be feeding it with "--to" etc
> on the command line, rather than interactively).

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

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

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