Re: [PATCH] include/asm-arm/: Spelling fixes

Re: [PATCH] include/asm-arm/: Spelling fixes

[J. Bruce Fields]

Hope you don't mind my cc'ing the git list:

On Mon, Dec 17, 2007 at 12:04:07PM -0800, Joe Perches wrote:
> On Mon, 2007-12-17 at 14:56 -0500, J. Bruce Fields wrote:
> > My mail client seems to be flagging all your messages as duplicates of each
> > other.
> > 
> > Hm.  It may be that your headers have two Message-Id's...
> > Message-Id: <1197919875-5288-3-git-send-email-joe@perches.com>
> > X-Mailer: git-send-email 1.5.3.7.949.g2221a6
> > Message-Id:
> >         <5703e57f925f31fc0eb38873bd7f10fc44f99cb4.1197918889.git.joe@perches.com
> >         >
> > 
> > ... the second of which is pretty weird (and is the same for every message.  Is
> > that a git-send-email bug or something else?
> 
> Dunno.  I did a git-format-patch, then git-send-email.

Weird.  Anyone know how that could happen?

--b.

Re: [PATCH] include/asm-arm/: Spelling fixes

[Joe Perches]

On Mon, 2007-12-17 at 15:12 -0500, J. Bruce Fields wrote:
> Hope you don't mind my cc'ing the git list:

Nope.  Not a bit.

I had patches that were committed in word order that
I wanted to aggregate by subsystem.

I took all the patches, created a branch, committed
all the patches at once, then used these commands:
 
(patch_order is a directory list)

$ let count=0 ; for line in $(cat patch_order) ; do ((count++)); \
	printf -v tmp "%04u-Spelling-%s" $count $line ; \
	tmp=${tmp//\//-} ; tmp=${tmp// /}; \
	git-format-patch -N --thread --start-number $count -s \
	-o patch3 \
	--subject-prefix="[PATCH] Spelling: $line" master $line  ; done
[converted the subjects appropriately]
$ git-send-email --smtp-server <foo> --smtp-user <bar> \
	--from "Joe Perches <joe@perches.com>" \
	--to "linux-kernel@vger.kernel.org" \
	--cc "Andrew Morton <akpm@linux-foundation.org>" \
	--cc-cmd "../get_maintainer.pl --nogit" \
	--no-thread --suppress-from patch3

I probably just reversed the "--thread" on
git-format-patch and send-email.

cheers, Joe

Re: [PATCH] include/asm-arm/: Spelling fixes

[Jeff King]

On Mon, Dec 17, 2007 at 12:22:51PM -0800, Joe Perches wrote:

> $ let count=0 ; for line in $(cat patch_order) ; do ((count++)); \
> 	printf -v tmp "%04u-Spelling-%s" $count $line ; \
> 	tmp=${tmp//\//-} ; tmp=${tmp// /}; \
> 	git-format-patch -N --thread --start-number $count -s \
> 	-o patch3 \
> 	--subject-prefix="[PATCH] Spelling: $line" master $line  ; done
> [converted the subjects appropriately]
> $ git-send-email --smtp-server <foo> --smtp-user <bar> \
> 	--from "Joe Perches <joe@perches.com>" \
> 	--to "linux-kernel@vger.kernel.org" \
> 	--cc "Andrew Morton <akpm@linux-foundation.org>" \
> 	--cc-cmd "../get_maintainer.pl --nogit" \
> 	--no-thread --suppress-from patch3

Ah. The problem is that git-send-email unconditionally adds a
message-id. Usually git-format-patch doesn't add one, but for obvious
reasons, it must when doing --thread. Here is a fix.

-- >8 --
git-send-email: avoid duplicate message-ids

We used to unconditionally add a message-id to the outgoing
email without bothering to check if it already had one.
Instead, let's use the existing one.

Signed-off-by: Jeff King <peff@peff.net>
---
It will also happily add duplicate --in-reply-to and --references
headers, but those can be suppressed with --no-thread. Arguably, it
should detect this case and turn on --no-thread, but looking at
git-send-email makes me want to claw my eyes out.

 git-send-email.perl |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/git-send-email.perl b/git-send-email.perl
index 1d6f466..083466a 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -580,7 +580,7 @@ sub send_message
 		$ccline = "\nCc: $cc";
 	}
 	my $sanitized_sender = sanitize_address($sender);
-	make_message_id();
+	make_message_id() unless defined($message_id);
 
 	my $header = "From: $sanitized_sender
 To: $to${ccline}
@@ -718,6 +718,9 @@ foreach my $t (@files) {
 					}
 					push @xh, $_;
 				}
+				elsif (/^Message-Id: (.*)/i) {
+					$message_id = $1;
+				}
 				elsif (!/^Date:\s/ && /^[-A-Za-z]+:\s+\S/) {
 					push @xh, $_;
 				}
-- 
1.5.4.rc0.1146.gc97f-dirty

Re: [PATCH] include/asm-arm/: Spelling fixes

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Ah. The problem is that git-send-email unconditionally adds a
> message-id. Usually git-format-patch doesn't add one, but for obvious
> reasons, it must when doing --thread. Here is a fix.

> diff --git a/git-send-email.perl b/git-send-email.perl
> index 1d6f466..083466a 100755
> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -580,7 +580,7 @@ sub send_message
>  		$ccline = "\nCc: $cc";
>  	}
>  	my $sanitized_sender = sanitize_address($sender);
> -	make_message_id();
> +	make_message_id() unless defined($message_id);

Isn't this called inside a loop?  If the outgoing message does not
originally have "Message-Id:", does the loop correctly reinitialize
$message_id to undef, or does this change make everybody reuse the same
$message_id over and over again?

I have a feeling that --thread to format-patch is a misfeature.  Why is
it needed if you are feeding the output to send-email?

I wonder if stripping existing "Message-Id:" away just like we strip
away "Date:" from @xh would be a much saner fix.

Re: [PATCH] include/asm-arm/: Spelling fixes

[Jeff King]

On Mon, Dec 17, 2007 at 01:27:20PM -0800, Junio C Hamano wrote:

> >  	my $sanitized_sender = sanitize_address($sender);
> > -	make_message_id();
> > +	make_message_id() unless defined($message_id);
> 
> Isn't this called inside a loop?  If the outgoing message does not
> originally have "Message-Id:", does the loop correctly reinitialize
> $message_id to undef, or does this change make everybody reuse the same
> $message_id over and over again?

Yes, sorry. I realized it right after I sent the other out, but then a
repairman showed up to fix my non-working furnace. :)

The following needs to be squashed in (alternatively, the message_id
doesn't need to be a loop variable, so it could be cleaned up. But part
of me says that git-send-email is beyond hope for being clean).

diff --git a/git-send-email.perl b/git-send-email.perl
index 083466a..248d035 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -808,6 +808,7 @@ foreach my $t (@files) {
 			$references = "$message_id";
 		}
 	}
+	$message_id = undef;
 }
 
 if ($compose) {

> I have a feeling that --thread to format-patch is a misfeature.  Why is
> it needed if you are feeding the output to send-email?

I think it is a case of --thread being added for people not using
send-email, and then getting it misused. I am just trying to add a
sanity check to send-email in case the user does something silly (though
one could certainly argue that it is already hopelessly tied to
git-format-patch, and fixing git-format-patch is the right way to go).

> I wonder if stripping existing "Message-Id:" away just like we strip
> away "Date:" from @xh would be a much saner fix.

That is definitely wrong if we expect to re-use the in-reply-to and
references headers that already exist (though obviously we could strip
out all three of those headers and re-add our own).

I don't have a strong opinion. I never use git-send-email myself, but
was just trying to fix a reported bug.

-Peff

Re: [PATCH] include/asm-arm/: Spelling fixes

[Jeff King]

On Mon, Dec 17, 2007 at 06:05:58PM -0500, Jeff King wrote:

> > I wonder if stripping existing "Message-Id:" away just like we strip
> > away "Date:" from @xh would be a much saner fix.
> 
> That is definitely wrong if we expect to re-use the in-reply-to and
> references headers that already exist (though obviously we could strip
> out all three of those headers and re-add our own).
> 
> I don't have a strong opinion. I never use git-send-email myself, but
> was just trying to fix a reported bug.

Actually, I don't think stripping the message-id is ever the right
thing. The user put it in there for some purpose, and "ours not to
reason why" (ours but to do and die). IOW, it is not possible for us to
know what we are breaking by changing the message-id. It could simply be
the reply-to header in the following messages, or it could be the
reply-to in some message we have not and will not ever see.

Even if we assume git-send-email only ever gets the unmunged output of
git-format-patch, we do not necessarily know it has been fed all of the
patches during a single run.

-Peff

Re: [PATCH] include/asm-arm/: Spelling fixes

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>> I wonder if stripping existing "Message-Id:" away just like we strip
>> away "Date:" from @xh would be a much saner fix.
>
> That is definitely wrong if we expect to re-use the in-reply-to and
> references headers that already exist (though obviously we could strip
> out all three of those headers and re-add our own).

Ah, you are right.

And undef $message_id you squashed in definitely belongs there --
"prepare for the next round" section.

Re: [PATCH] include/asm-arm/: Spelling fixes

[Josh Triplett]

Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>> Ah. The problem is that git-send-email unconditionally adds a
>> message-id. Usually git-format-patch doesn't add one, but for obvious
>> reasons, it must when doing --thread. Here is a fix.
> 
>> diff --git a/git-send-email.perl b/git-send-email.perl
>> index 1d6f466..083466a 100755
>> --- a/git-send-email.perl
>> +++ b/git-send-email.perl
>> @@ -580,7 +580,7 @@ sub send_message
>>  		$ccline = "\nCc: $cc";
>>  	}
>>  	my $sanitized_sender = sanitize_address($sender);
>> -	make_message_id();
>> +	make_message_id() unless defined($message_id);
> 
> Isn't this called inside a loop?  If the outgoing message does not
> originally have "Message-Id:", does the loop correctly reinitialize
> $message_id to undef, or does this change make everybody reuse the same
> $message_id over and over again?
> 
> I have a feeling that --thread to format-patch is a misfeature.  Why is
> it needed if you are feeding the output to send-email?

I added that option; see (d1566f7883f727f38bf442af3fdb69d36e6fcea2,
cc35de8470541e389b7d2bdda4c901574720fa81, and
da56645dd7c1175fc2ed1628ac35fdd35e705641).  I use git-imap-send, not
git-send-email, and I wanted to thread my patches.

- Josh Triplett

Re: [PATCH] include/asm-arm/: Spelling fixes

[Josh Triplett]

Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>> Ah. The problem is that git-send-email unconditionally adds a
>> message-id. Usually git-format-patch doesn't add one, but for obvious
>> reasons, it must when doing --thread. Here is a fix.
> 
>> diff --git a/git-send-email.perl b/git-send-email.perl
>> index 1d6f466..083466a 100755
>> --- a/git-send-email.perl
>> +++ b/git-send-email.perl
>> @@ -580,7 +580,7 @@ sub send_message
>>  		$ccline = "\nCc: $cc";
>>  	}
>>  	my $sanitized_sender = sanitize_address($sender);
>> -	make_message_id();
>> +	make_message_id() unless defined($message_id);
> 
> Isn't this called inside a loop?  If the outgoing message does not
> originally have "Message-Id:", does the loop correctly reinitialize
> $message_id to undef, or does this change make everybody reuse the same
> $message_id over and over again?
> 
> I have a feeling that --thread to format-patch is a misfeature.  Why is
> it needed if you are feeding the output to send-email?

I added that option; see (d1566f7883f727f38bf442af3fdb69d36e6fcea2,
cc35de8470541e389b7d2bdda4c901574720fa81, and
da56645dd7c1175fc2ed1628ac35fdd35e705641).  I use git-imap-send, not
git-send-email, and I wanted to thread my patches.

- Josh Triplett