Krzysztof Mazur <krzysiek@xxxxxxxxxxxx> writes:
> Some addresses are passed twice to unique_email_list() and invalid addresses
> may be reported twice per send_message. Now we warn about them earlier
> and we also remove invalid addresses.
>
> This also removes using of undefined values for string comparison
> for invalid addresses in cc list processing.
>
> Signed-off-by: Krzysztof Mazur <krzysiek@xxxxxxxxxxxx>
> ---
I think there are three kinds of address-looking things we deal
with:
* Possibly invalid but meant for human consumption, e.g.
Cc: Stable Kernel <stable@xxxxx> # for v3.5 and upwards
in the commit log message trailer.
* Meant to be fed to our MSA, without losing the human readable
part, e.g.
Cc: Stable Kernel <stable@xxxxx>
in the header of the outgoing message.
* Without the human-readable part, e.g.
stable@xxxxx
that is returned by extract_valid_address.
My understanding is that our input typically comes from the first
kind and sanitize_address() is meant to massage it into the second
kind. The last kind is to be used to drive the underlying sendmail
machinery and meant to go in the envelope (this includes message-id
generation).
I do not think send-email adds the first kind (invalid ones) in its
output, even though it reads them from its input and copy them to
its output in the e-mail body part of the payload, but I think it
adds new addresses to the e-mail header part of the payload (that is
what $from, @initial_to, @initial_cc and @bcclist are all about).
We would want to feed them in the third form (i.e. output from
extract-valid-address on them) when driving the underlying sendmail
machinery to place them in the envelope part, but they should be in
the second form when we place them on e-mail header lines. As far
as I can tell, the resulting code looks correct in this regard. The
addresses are sanitized into the second form upfront and validated
before they are placed in @initial_to and friends, and we carry the
second form around most of the time, until we call unique_email_list
in send_message to pass them through extract_valid_address to turn
them into the third form to drive the underlying sendmail.
I however found it a bit confusing while reading the callers of
validate_address{,_list} functions, which not just validate (and
warns) but return the ones that pass the test. Perhaps we would
want a brief comment before validate_address, validate_address_list,
and extract_valid_address{,_or_die} to clarify what they are doing
(especially what they return)?
The result still feels somewhat yucky (the yuckiness comes primarily
from the current code, not from the patch but I am mostly focused on
the result after applying the patch), in that extract-valid-address
that has problem with invalid email addresses will still die when
fed an address that is not "sanitized" first, so any future patch
that adds a new address source may still have to suffer the same
problem the part that dealt with the Cc list suffered (which your
1/5 fixed earlier), but I do not offhand think of a good way to
reorganize them. We could of course make validate_address() call
sanitize_address(), but that would be mostly redundant since the new
code sanitizes the input upfront.
So overall, looks good to me. Thanks.
> git-send-email.perl | 52 +++++++++++++++++++++++++++++++++++++++-------------
> 1 file changed, 39 insertions(+), 13 deletions(-)
>
> diff --git a/git-send-email.perl b/git-send-email.perl
> index 356f99d..5056fdc 100755
> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -786,9 +786,11 @@ sub expand_one_alias {
> }
>
> @initial_to = expand_aliases(@initial_to);
> -@initial_to = (map { sanitize_address($_) } @initial_to);
> +@initial_to = validate_address_list(sanitize_address_list(@initial_to));
> @initial_cc = expand_aliases(@initial_cc);
> +@initial_cc = validate_address_list(sanitize_address_list(@initial_cc));
> @bcclist = expand_aliases(@bcclist);
> +@bcclist = validate_address_list(sanitize_address_list(@bcclist));
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html