Re: [PATCH resend] git-send-email: Use sanitized address when reading mbox body

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 21, 2024, 17:27 UTC
Message-ID: <xmqqr0cqmck5.fsf@gitster.g>
In-Reply-To: <20240621092721.2980939-2-csokas.bence@prolan.hu>

"Csókás, Bence" <csokas.bence@prolan.hu> writes:

Show 26 quoted lines

> Commas and other punctuation marks in 'Cc: ', 'Signed-off-by: '
> etc. lines mess with git-send-email. In parsing the mbox headers,
> this is handled by calling `sanitize_address()`. This function
> is called when parsing the message body as well, but was only
> used for comparing it to $author. Now we add it to @cc too.
>
> Signed-off-by: Csókás, Bence <csokas.bence@prolan.hu>
> ---
>  git-send-email.perl | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/git-send-email.perl b/git-send-email.perl
> index f0be4b4560..72044e5ef3 100755
> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -1847,9 +1847,9 @@ sub pre_process_file {
>  					$what, $_) unless $quiet;
>  				next;
>  			}
> -			push @cc, $c;
> +			push @cc, $sc;
>  			printf(__("(body) Adding cc: %s from line '%s'\n"),
> -				$c, $_) unless $quiet;
> +				$sc, $_) unless $quiet;
>  		}
>  	}

Hmph, there is this piece of code before the block:

			if ($c !~ /.+@.+|<.+>/) {
				printf("(body) Ignoring %s from line '%s'\n",
					$what, $_) unless $quiet;
				next;
			}

This is to reject strings that do not even look like an e-mail address, but we called sanitize_address() call on $c way before this check. I wonder if we should move this block way up, even before the call to santize_address()?

That was a relatively unrelated tangent.

In the same function, there is this snippet about Cc: (I didn't check if the same issue is shared with other header fields):

			elsif (/^Cc:\s+(.*)$/i) {
				foreach my $addr (parse_address_line($1)) {
					my $qaddr = unquote_rfc2047($addr);
					my $saddr = sanitize_address($qaddr);
					if ($saddr eq $sender) {
						next if ($suppress_cc{'self'});
					} else {
						next if ($suppress_cc{'cc'});
					}
					printf(__("(mbox) Adding cc: %s from line '%s'\n"),
						$addr, $_) unless $quiet;
					push @cc, $addr;
				}
			}

We seem to use sanitized address only for the purpose of suppressing
Cc, and use the original address given in the input for e-mail
purposes (@cc is the same variable as the patch under discussion
sends the "fixed" address to, which holds the data to formulate the
Cc: header of the outgoing message, I presume?).  So in that way, we
seem to be very consistent.

Possibly we are being consistent in a broken way, but I am not yet convinced that we are.

It looks to me that there are many other places that we try to be as faithful to the original as possible. In the same block as the one that handles "Cc:" I quoted above, an address on "From:" is also sent intact into @cc and addresses on "To:" are handled the same way.

The patch under discussion singles out the addresses on the trailers in the message body and treat them differently from others, which I am not sure is what we want to do.