Re: [PATCH] git-send-email: Delete additional From message body

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 26, 2015, 22:38 UTC
Message-ID: <xmqqk2sh1xql.fsf@gitster.dls.corp.google.com>
In-Reply-To: <1440581120-14156-1-git-send-email-brilliantov@inbox.ru>

brilliantov@inbox.ru writes:

Show 18 quoted lines

> Additional From added to message body if git-send-email run
> with --from parameters
>
> Signed-off-by: Brilliantov Kirill Vladimirovich <brilliantov@inbox.ru>
> ---
>  git-send-email.perl | 25 +++++++++++--------------
>  1 file changed, 11 insertions(+), 14 deletions(-)
>
> diff --git a/git-send-email.perl b/git-send-email.perl
> index b660cc2..92ec74a 100755
> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -1522,24 +1522,21 @@ foreach my $t (@files) {
>  		$subject = quote_subject($subject, $auto_8bit_encoding);
>  	}
>  
> -	if (defined $sauthor and $sauthor ne $sender) {
> -		$message = "From: $author\n\n$message";

This condition attempts to say "Look at the author taken from the
From: line of the message, and if that is different from the sender
(which will be recorded in the header of the resulting e-mail), then
we do need to add an additional in-body From: line to record the
author of the commit, who may be different from the name that would
appear in the e-mail."

In other words, the "Additional From" is very much deliberate when the "--from" option is used and is different from the patch author. And I do not see this "add in-body header if the author and the sender are different" you are removing from here appear anywhere in the added text of your patch. So one consequence of this patch is to _never_ use the in-body From: line.

That needs a serious justification.

Now, it is entirely possible that what you wanted to do (and what you are doing) in this patch may be different from "unconditionally, remove the in-body From: line". Perhaps the code mistakenly declares that the sender given via "--from" and the author are different, possibly due to a bug in rfc2047 unquoting code or something, and you are only trying to make sure that the code notices they are really the same. But I somehow have a feeling that that is not what is going on, because in such a patch, there still will be some code that adds "From: $author" at the very beginning of outgoing message body; such a fix might update the "if ()" condition used to guard it (e.g. $sauthor and $sender may have to get further cleansed before comparing), but I do not see anything of that sort, either. Also all the changes below do not have anything to do with the additional in-body "From: " line.

So, I dunno. I cannot tell what you are trying to do, and the only immediate effect of the change I can read is that in-body "From:" line is unconditionally disabled, which is certainly not something we would want to do.

Puzzled...

Show 30 quoted lines

> -		if (defined $author_encoding) {
> -			if ($has_content_type) {
> -				if ($body_encoding eq $author_encoding) {
> -					# ok, we already have the right encoding
> -				}
> -				else {
> -					# uh oh, we should re-encode
> -				}
> +	if (defined $author_encoding) {
> +		if ($has_content_type) {
> +			if ($body_encoding eq $author_encoding) {
> +				# ok, we already have the right encoding
>  			}
>  			else {
> -				$xfer_encoding = '8bit' if not defined $xfer_encoding;
> -				$has_content_type = 1;
> -				push @xh,
> -				  "Content-Type: text/plain; charset=$author_encoding";
> +				# uh oh, we should re-encode
>  			}
>  		}
> +		else {
> +			$xfer_encoding = '8bit' if not defined $xfer_encoding;
> +			$has_content_type = 1;
> +			push @xh,
> +			  "Content-Type: text/plain; charset=$author_encoding";
> +		}
>  	}
>  	if (defined $target_xfer_encoding) {
>  		$xfer_encoding = '8bit' if not defined $xfer_encoding;