[PATCH] git-send-email.perl: fix In-Reply-To for second and subsequent patches

[PATCH] git-send-email.perl: fix In-Reply-To for second and subsequent patches

[Antonio Ospite]

Make second and subsequent patches appear as replies to the first patch,
even when an initial In-Reply-To is supplied; this is the typical
behaviour we want when we send a series with cover letter in reply to
some discussion, and this is also what the man page says about
--in-reply-to.

In order to achieve the old behaviour of a flat structure in reply to
something the user can always use "--no-thread --in-reply-to <...>".

Signed-off-by: Antonio Ospite <ospite@studenti.unina.it>

---

Hi,

Right now _all_ the patches appear as reply to the message indicated as
initial In-Reply-To, and I think this is not right, the behaviour this
patch introduces can be debatable of course, but there are quite some
arguments supporting it:

  - When $initial_reply_to is asked to the user, it is asked as the
    "Message-ID to be used as In-Reply-To for the _first_ email", this
    makes me think that the second and subsequent patches are not using
    it and will be considered as reply to the first message or chained
    according to the --[no-]chain-reply-to setting.

  - git-format-patch states that clearly in the man page, and I think
    git-send-email should behave the same way, and this is explained
    also in the git-send-email man page, look at
    --in-reply-to=<identifier> explanation.

Please keep CCing me on this as I am not subscribed to git@vger.kernel.org.

Thanks,
   Antonio Ospite
   http://ao2.it


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

diff --git a/git-send-email.perl b/git-send-email.perl
index 8cc4161..615a40d 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -1313,7 +1313,7 @@ foreach my $t (@files) {
 
 	# set up for the next message
 	if ($thread && $message_was_sent &&
-		(chain_reply_to() || !defined $reply_to || length($reply_to) == 0)) {
+		($message_num == 1 || chain_reply_to() || !defined $reply_to || length($reply_to) == 0)) {
 		$reply_to = $message_id;
 		if (length $references > 0) {
 			$references .= "\n $message_id";
-- 
1.7.1

Re: [PATCH] git-send-email.perl: fix In-Reply-To for second and subsequent patches

[Jonathan Nieder]

(+cc: some send-email people)

Hi,

Antonio Ospite wrote:

> Make second and subsequent patches appear as replies to the first patch,
> even when an initial In-Reply-To is supplied
[...]
> Signed-off-by: Antonio Ospite <ospite@studenti.unina.it>

Thanks.

>   - When $initial_reply_to is asked to the user, it is asked as the
>     "Message-ID to be used as In-Reply-To for the _first_ email", this
>     makes me think that the second and subsequent patches are not using
>     it

This kind of justification belongs in the commit message, no?
That way, we can save future readers the trouble of figuring out
the rationale all over again when considering future changes to this
code.

> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -1313,7 +1313,7 @@ foreach my $t (@files) {
>  
>  	# set up for the next message
>  	if ($thread && $message_was_sent &&
> -		(chain_reply_to() || !defined $reply_to || length($reply_to) == 0)) {
> +		($message_num == 1 || chain_reply_to() || !defined $reply_to || length($reply_to) == 0)) {
>  		$reply_to = $message_id;

Would it be possible to break this long line?

If you're feeling particularly adventurous, it would be nice to add a
test for the changed functionality to t/t9001-send-email.sh, so we
don't break it with other changes in the future.

I haven't looked too deeply or even tried running applying the patch,
but generally it looks good to me.

Ciao,
Jonathan

Re: [PATCH] git-send-email.perl: fix In-Reply-To for second and subsequent patches

[Antonio Ospite]

[-- Attachment #1: Type: text/plain, Size: 2347 bytes --]

On Thu, 14 Oct 2010 13:22:50 -0500
Jonathan Nieder <jrnieder@gmail.com> wrote:

> (+cc: some send-email people)
>

For the new recipients, the original mail is here btw:
http://permalink.gmane.org/gmane.comp.version-control.git/159039

More comments below.

> Hi,
> 
> Antonio Ospite wrote:
> 
> > Make second and subsequent patches appear as replies to the first patch,
> > even when an initial In-Reply-To is supplied
> [...]
> > Signed-off-by: Antonio Ospite <ospite@studenti.unina.it>
> 
> Thanks.
>

Thanks for commenting Jonathan.

> >   - When $initial_reply_to is asked to the user, it is asked as the
> >     "Message-ID to be used as In-Reply-To for the _first_ email", this
> >     makes me think that the second and subsequent patches are not using
> >     it
> 
> This kind of justification belongs in the commit message, no?
> That way, we can save future readers the trouble of figuring out
> the rationale all over again when considering future changes to this
> code.
>

Ok, I can add this in the commit message, I am waiting some days for
v2, in case someone else has more to say.

> > --- a/git-send-email.perl
> > +++ b/git-send-email.perl
> > @@ -1313,7 +1313,7 @@ foreach my $t (@files) {
> >  
> >  	# set up for the next message
> >  	if ($thread && $message_was_sent &&
> > -		(chain_reply_to() || !defined $reply_to || length($reply_to) == 0)) {
> > +		($message_num == 1 || chain_reply_to() || !defined $reply_to || length($reply_to) == 0)) {
> >  		$reply_to = $message_id;
> 
> Would it be possible to break this long line?
>

I like the OR chain on the same line, but I can split it anyways if
that's the preference.

> If you're feeling particularly adventurous, it would be nice to add a
> test for the changed functionality to t/t9001-send-email.sh, so we
> don't break it with other changes in the future.
>

No promises, but I might give that a try.

> I haven't looked too deeply or even tried running applying the patch,
> but generally it looks good to me.
> 
> Ciao,
> Jonathan
> 

Thanks,
   Antonio

-- 
Antonio Ospite
http://ao2.it

PGP public key ID: 0x4553B001

A: Because it messes up the order in which people normally read text.
   See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?

[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]

[PATCH v2] git-send-email.perl: fix In-Reply-To for second and subsequent patches

[Antonio Ospite]

Make second and subsequent patches appear as replies to the first patch,
even when an initial In-Reply-To is supplied; this is the typical
behaviour we want when we send a series with cover letter in reply to
some discussion, and this is also what the man page says about
the --in-reply-to option.

When $initial_reply_to is asked to the user interactively it is asked as
the "Message-ID to be used as In-Reply-To for the _first_ email", this
makes the user think that the second and subsequent patches are not
using it but are considered as replies to the first message or chained
according to the --[no-]chain-reply setting.

In order to achieve the old behaviour of a flat structure in reply to
something the user can always use "--no-thread --in-reply-to <...>".

Signed-off-by: Antonio Ospite <ospite@studenti.unina.it>
---

Changes since v1:
 - add more details about the interactive case in the commit message
 - split long line as requested by Jonathan Nieder
 - add a test case in t/t9001-send-email.sh, please check that, I am not
   comparing the strings inside '<>' is it necessary to be so strict?
   Note to self: remember to run 'make' before running t9001-send-email.sh :)

Thanks,
   Antonio Ospite
   http://ao2.it

 git-send-email.perl   |    3 ++-
 t/t9001-send-email.sh |   14 ++++++++++++++
 2 files changed, 16 insertions(+), 1 deletions(-)

diff --git a/git-send-email.perl b/git-send-email.perl
index 8cc4161..bc4e318 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -1313,7 +1313,8 @@ foreach my $t (@files) {
 
 	# set up for the next message
 	if ($thread && $message_was_sent &&
-		(chain_reply_to() || !defined $reply_to || length($reply_to) == 0)) {
+		(chain_reply_to() || !defined $reply_to || length($reply_to) == 0 ||
+		$message_num == 1)) {
 		$reply_to = $message_id;
 		if (length $references > 0) {
 			$references .= "\n $message_id";
diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh
index a298eb0..410b85f 100755
--- a/t/t9001-send-email.sh
+++ b/t/t9001-send-email.sh
@@ -295,6 +295,20 @@ test_expect_success $PREREQ 'Valid In-Reply-To when prompting' '
 	! grep "^In-Reply-To: < *>" msgtxt1
 '
 
+test_expect_success $PREREQ 'In-Reply-To in second patch with --thread' '
+	clean_fake_sendmail &&
+	git send-email \
+		--from="Example <nobody@example.com>" \
+		--to=nobody@example.com \
+		--thread \
+		--in-reply-to="<unique-message-id@example.com>" \
+		--smtp-server="$(pwd)/fake.sendmail" \
+		$patches $patches \
+		2>errors
+        # The second patch should be seen as reply to the first one
+        test $(sed -n -e "s/^In-Reply-To:\(.*\)/\1/p" msgtxt2) = $(sed -n -e "s/^Message-Id:\(.*\)/\1/p" msgtxt1)
+'
+
 test_expect_success $PREREQ 'setup fake editor' '
 	(echo "#!$SHELL_PATH" &&
 	 echo "echo fake edit >>\"\$1\""
-- 
1.7.2.3

Re: [PATCH v2] git-send-email.perl: fix In-Reply-To for second and subsequent patches

[Junio C Hamano]

Antonio Ospite <ospite@studenti.unina.it> writes:

> Make second and subsequent patches appear as replies to the first patch,
> even when an initial In-Reply-To is supplied; this is the typical
> behaviour we want when we send a series with cover letter in reply to
> some discussion, and this is also what the man page says about
> the --in-reply-to option.

I am not so sure if that is what the documentation says.

 1. When --in-reply-to gives $reply_to, the first one becomes a reply to
    that message, with or without --chain-reply-to.

 2. When --chain-reply-to is in effect, all the messages are strung
    together to form a single chain.  The first message may be in reply to
    the $reply_to given by --in-reply-to command line option (see
    previous), or the root of the discussion thread.  The second one is a
    response to the first one, and the third one is a response to the
    second one, etc.

 3. When --chain-reply-to is not in effect:

    a. When --in-reply-to is used, too, the second and the subsequent ones
       become replies to $reply_to.  Together with the first rule, all
       messages become replies to $reply_to given by --in-reply-to.

    b. When --in-reply-to is not used, presumably the second and
       subsequent ones become replies to the first one, which would be the
       root.

The documentation is reasonably clear about the 1., 2. and 3a. above, I
think, even though I do not think 3b. is clearly specified.

If you are changing 3a. above so that the first message becomes a response
to $reply_to, and the second one becomes a response to the first message
(and the third and subsequent ones too when --chain-reply-to is not in
effect), you would need to update the documentation as well.  Even if it
might be of good kind, it would be a change of the established behaviour.

> diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh
> index a298eb0..410b85f 100755
> --- a/t/t9001-send-email.sh
> +++ b/t/t9001-send-email.sh
> @@ -295,6 +295,20 @@ test_expect_success $PREREQ 'Valid In-Reply-To when prompting' '
>  	! grep "^In-Reply-To: < *>" msgtxt1
>  '
>  
> +test_expect_success $PREREQ 'In-Reply-To in second patch with --thread' '
> +	clean_fake_sendmail &&
> +	git send-email \
> +		--from="Example <nobody@example.com>" \
> +		--to=nobody@example.com \
> +		--thread \
> +		--in-reply-to="<unique-message-id@example.com>" \
> +		--smtp-server="$(pwd)/fake.sendmail" \
> +		$patches $patches \
> +		2>errors

You are breaking the && chain here.

> +        # The second patch should be seen as reply to the first one
> +        test $(sed -n -e "s/^In-Reply-To:\(.*\)/\1/p" msgtxt2) = $(sed -n -e "s/^Message-Id:\(.*\)/\1/p" msgtxt1)
> +'

You would need to test the interaction with --chain-reply-to as well, so
there should be another test, and you would probably need three messages
fed to send-email not just two to see the effect of the interaction.

Thanks.

Re: [PATCH v2] git-send-email.perl: fix In-Reply-To for second and subsequent patches

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

>> +		$patches $patches \
>> +		2>errors
>
> You are breaking the && chain here.
>
>> +        # The second patch should be seen as reply to the first one
>> +        test $(sed -n -e "s/^In-Reply-To:\(.*\)/\1/p" msgtxt2) = $(sed -n -e "s/^Message-Id:\(.*\)/\1/p" msgtxt1)
>> +'
>
> You would need to test the interaction with --chain-reply-to as well, so
> there should be another test, and you would probably need three messages
> fed to send-email not just two to see the effect of the interaction.

IOW, the test part of the patch should look something like this.

Note that the below uses the current semantics (3a. in the previous
message), not your version of the definition.

I would suggest using this one as the first patch in your series, perhaps
with a documentation update to clarify the semantics of 3b. in my previous
message.  Then change git-send-email and update the test, as your change
will break the expected behaviour of the first test added here, and
document the change of semantics 3a. in the second patch in your series.

 t/t9001-send-email.sh |   41 +++++++++++++++++++++++++++++++++++++++++
 1 files changed, 41 insertions(+), 0 deletions(-)

diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh
index 07c50c7..c7e5c93 100755
--- a/t/t9001-send-email.sh
+++ b/t/t9001-send-email.sh
@@ -295,6 +295,47 @@ test_expect_success $PREREQ 'Valid In-Reply-To when prompting' '
 	! grep "^In-Reply-To: < *>" msgtxt1
 '
 
+test_expect_success $PREREQ 'In-Reply-To without --chain-reply-to' '
+	clean_fake_sendmail &&
+	echo "<unique-message-id@example.com>" >expect &&
+	git send-email \
+		--from="Example <nobody@example.com>" \
+		--to=nobody@example.com \
+		--no-chain-reply-to \
+		--in-reply-to="$(cat expect)" \
+		--smtp-server="$(pwd)/fake.sendmail" \
+		$patches $patches $patches \
+		2>errors &&
+	# All the messages are replies to --in-reply-to
+	sed -n -e "s/^In-Reply-To: *\(.*\)/\1/p" msgtxt1 >actual &&
+	test_cmp expect actual &&
+	sed -n -e "s/^In-Reply-To: *\(.*\)/\1/p" msgtxt2 >actual &&
+	test_cmp expect actual &&
+	sed -n -e "s/^In-Reply-To: *\(.*\)/\1/p" msgtxt3 >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success $PREREQ 'In-Reply-To with --chain-reply-to' '
+	clean_fake_sendmail &&
+	echo "<unique-message-id@example.com>" >expect &&
+	git send-email \
+		--from="Example <nobody@example.com>" \
+		--to=nobody@example.com \
+		--chain-reply-to \
+		--in-reply-to="$(cat expect)" \
+		--smtp-server="$(pwd)/fake.sendmail" \
+		$patches $patches $patches \
+		2>errors &&
+	sed -n -e "s/^In-Reply-To: *\(.*\)/\1/p" msgtxt1 >actual &&
+	test_cmp expect actual &&
+	sed -n -e "s/^Message-Id: *\(.*\)/\1/p" msgtxt1 >expect &&
+	sed -n -e "s/^In-Reply-To: *\(.*\)/\1/p" msgtxt2 >actual &&
+	test_cmp expect actual &&
+	sed -n -e "s/^Message-Id: *\(.*\)/\1/p" msgtxt2 >expect &&
+	sed -n -e "s/^In-Reply-To: *\(.*\)/\1/p" msgtxt3 >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success $PREREQ 'setup fake editor' '
 	(echo "#!$SHELL_PATH" &&
 	 echo "echo fake edit >>\"\$1\""

Re: [PATCH v2] git-send-email.perl: fix In-Reply-To for second and subsequent patches

[Antonio Ospite]

[-- Attachment #1: Type: text/plain, Size: 5128 bytes --]

On Tue, 19 Oct 2010 11:26:33 -0700
Junio C Hamano <gitster@pobox.com> wrote:

Thanks for commenting Junio.

> Antonio Ospite <ospite@studenti.unina.it> writes:
> 
> > Make second and subsequent patches appear as replies to the first patch,
> > even when an initial In-Reply-To is supplied; this is the typical
> > behaviour we want when we send a series with cover letter in reply to
> > some discussion, and this is also what the man page says about
> > the --in-reply-to option.
> 
> I am not so sure if that is what the documentation says.
>

Sorry, I meant the man page part about --[no-]chain-reply-to, I mistyped
that and generated more confusion than was "necessary".

>  1. When --in-reply-to gives $reply_to, the first one becomes a reply to
>     that message, with or without --chain-reply-to.
>

No doubts on that.

>  2. When --chain-reply-to is in effect, all the messages are strung
>     together to form a single chain.  The first message may be in reply to
>     the $reply_to given by --in-reply-to command line option (see
>     previous), or the root of the discussion thread.  The second one is a
>     response to the first one, and the third one is a response to the
>     second one, etc.
>

This is pretty clear as well.

>  3. When --chain-reply-to is not in effect:
>
>     a. When --in-reply-to is used, too, the second and the subsequent ones
>        become replies to $reply_to.  Together with the first rule, all
>        messages become replies to $reply_to given by --in-reply-to.
> 
>     b. When --in-reply-to is not used, presumably the second and
>        subsequent ones become replies to the first one, which would be the
>        root.
> 
> The documentation is reasonably clear about the 1., 2. and 3a. above, I
> think, even though I do not think 3b. is clearly specified.
>

In general, 3b. is specified in --[no-]chain-reply-to section, the
"problem" with 3a. is that --in-reply-to _overrides_ the behavior
specified by --no-chain-reply-to.

So I think that the whole issue really boils down to the question:
Should --in-reply-to apply _only_ to the first email?
The doc for the corresponding git-format-patch option gives _one_
answer, and you know that :)

By answering to this question with a YES also in git-send-email, we are
making --in-reply-to *independent* from --[no-]chain-reply-to, hence
the very simple test.

> If you are changing 3a. above so that the first message becomes a response
> to $reply_to, and the second one becomes a response to the first message
> (and the third and subsequent ones too when --chain-reply-to is not in
> effect), you would need to update the documentation as well.  Even if it
> might be of good kind, it would be a change of the established behaviour.
>

Right, the documentation needs be updated as well, thanks for pointing
this out. I think I am going to copy from the git-format-patch man
page.
So, do you agree to this change of behavior as long as it is documented?

> > diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh
> > index a298eb0..410b85f 100755
> > --- a/t/t9001-send-email.sh
> > +++ b/t/t9001-send-email.sh
> > @@ -295,6 +295,20 @@ test_expect_success $PREREQ 'Valid In-Reply-To when prompting' '
> >  	! grep "^In-Reply-To: < *>" msgtxt1
> >  '
> >  
> > +test_expect_success $PREREQ 'In-Reply-To in second patch with --thread' '
> > +	clean_fake_sendmail &&
> > +	git send-email \
> > +		--from="Example <nobody@example.com>" \
> > +		--to=nobody@example.com \
> > +		--thread \
> > +		--in-reply-to="<unique-message-id@example.com>" \
> > +		--smtp-server="$(pwd)/fake.sendmail" \
> > +		$patches $patches \
> > +		2>errors
> 
> You are breaking the && chain here.
>

Some other tests do that as well, the last line is a command by
itself not and-chained with the git-send-email invocation. I guess the
logic behind this is that the test succeeds if the _last_ command
succeeds. If this is wrong then some other tests are affected too.

> > +        # The second patch should be seen as reply to the first one
> > +        test $(sed -n -e "s/^In-Reply-To:\(.*\)/\1/p" msgtxt2) = $(sed -n -e "s/^Message-Id:\(.*\)/\1/p" msgtxt1)
> > +'
> 
> You would need to test the interaction with --chain-reply-to as well, so
> there should be another test, and you would probably need three messages
> fed to send-email not just two to see the effect of the interaction.
> 

I saw the tests in the other mail, but under my interpretation
we should just ensure --in-reply-to is applying to the first
message only (so we check the second one), if so from the third one on
--[no-]chain-reply-to is totally unrelated to --in-reply-to.

I think I can make the test more explicit tho, like:
("In-Reply-To" of second message) != $initial_reply_to

Thanks,
   Antonio

-- 
Antonio Ospite
http://ao2.it

PGP public key ID: 0x4553B001

A: Because it messes up the order in which people normally read text.
   See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?

[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH v2] git-send-email.perl: fix In-Reply-To for second and subsequent patches

[Antonio Ospite]

[-- Attachment #1: Type: text/plain, Size: 730 bytes --]

On Wed, 20 Oct 2010 00:45:33 +0200
Antonio Ospite <ospite@studenti.unina.it> wrote:

> On Tue, 19 Oct 2010 11:26:33 -0700
> Junio C Hamano <gitster@pobox.com> wrote:
> 
> Thanks for commenting Junio.
>

Junio, did you see the comments inlined in the previous message? Maybe
the "Thanks" line above made you think that there were no more comments?

If you saw them and agree with what I wrote then I am sending a v3 in
some days adding just the doc updates.

Thanks,
   Antonio

-- 
Antonio Ospite
http://ao2.it

PGP public key ID: 0x4553B001

A: Because it messes up the order in which people normally read text.
   See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?

[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]

[PATCH v3] git-send-email.perl: make initial In-Reply-To apply only to first email

[Antonio Ospite]

When an initial In-Reply-To is supplied it should apply only to the
first email, second and subsequent messages should behave just according
to the --[no-]chain-reply-to setting; this is the typical behaviour we
want when we send a series with cover letter in reply to some
discussion, this is what the man page says about the
--[no-]chain-reply-to option and this is also how the --in-reply-to
option behaves in git-format-patch.

Moreover, when $initial_reply_to is asked to the user interactively it
is asked as the "Message-ID to be used as In-Reply-To for the _first_
email", this makes the user think that the second and subsequent patches
are not using it but are considered as replies to the first message or
chained according to the --[no-]chain-reply setting.

Adjust also the documentation about --in-reply-to to avoid ambiguities.

NOTE: This patch changes the current behaviour and brings it to be what
I think was the intentions stated in the documentation, also aligning it
to how git-format-patch behaves; in order to achieve the old behaviour
of a flat structure in reply to something the user can always use
"--no-thread --in-reply-to <...>".

Signed-off-by: Antonio Ospite <ospite@studenti.unina.it>
---

Changes since v2:
 - Make the purpose of the patch more explicit
 - Adjust the documentation
 - Make the test narrower and more explicit as well

I am CCing some of the latest contributors to git-send-email.perl

Juno, there are still some unanswered questions (one about the
and-chains in tests) in one of previous mails in this thread.

With Best Regards,
   Antonio

 Documentation/git-send-email.txt |    8 +++++---
 git-send-email.perl              |    3 ++-
 t/t9001-send-email.sh            |   14 ++++++++++++++
 3 files changed, 21 insertions(+), 4 deletions(-)

diff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt
index 05904e0..acbff9b 100644
--- a/Documentation/git-send-email.txt
+++ b/Documentation/git-send-email.txt
@@ -82,9 +82,11 @@ See the CONFIGURATION section for 'sendemail.multiedit'.
 	set, as returned by "git var -l".
 
 --in-reply-to=<identifier>::
-	Specify the contents of the first In-Reply-To header.
-	Subsequent emails will refer to the previous email
-	instead of this if --chain-reply-to is set.
+	Make the first mail (or all the mails with `--no-thread`) appear as a
+	reply to the given Message-Id, which avoids breaking threads to
+	provide a new patch series.
+	The second and subsequent emails will be sent as replies according to
+	the --[no]-chain-reply-to setting.
 	Only necessary if --compose is also set.  If --compose
 	is not set, this will be prompted for.
 
diff --git a/git-send-email.perl b/git-send-email.perl
index f68ed5a..fe6b848 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -1319,7 +1319,8 @@ foreach my $t (@files) {
 
 	# set up for the next message
 	if ($thread && $message_was_sent &&
-		(chain_reply_to() || !defined $reply_to || length($reply_to) == 0)) {
+		(chain_reply_to() || !defined $reply_to || length($reply_to) == 0 ||
+		$message_num == 1)) {
 		$reply_to = $message_id;
 		if (length $references > 0) {
 			$references .= "\n $message_id";
diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh
index d1ba252..c85be0f 100755
--- a/t/t9001-send-email.sh
+++ b/t/t9001-send-email.sh
@@ -313,6 +313,20 @@ test_expect_success $PREREQ 'Valid In-Reply-To when prompting' '
 	! grep "^In-Reply-To: < *>" msgtxt1
 '
 
+test_expect_success $PREREQ 'Apply initial In-Reply-To only to first patch with --thread' '
+	clean_fake_sendmail &&
+	git send-email \
+		--from="Example <nobody@example.com>" \
+		--to=nobody@example.com \
+		--thread \
+		--in-reply-to="<unique-message-id@example.com>" \
+		--smtp-server="$(pwd)/fake.sendmail" \
+		$patches $patches \
+		2>errors
+        # The second message should not have the initial In-Reply-To
+        test $(sed -n -e "s/^In-Reply-To: \(.*\)/\1/p" msgtxt2) != "<unique-message-id@example.com>"
+'
+
 test_expect_success $PREREQ 'setup fake editor' '
 	(echo "#!$SHELL_PATH" &&
 	 echo "echo fake edit >>\"\$1\""
-- 
1.7.2.3

Re: [PATCH v2] git-send-email.perl: fix In-Reply-To for second and subsequent patches

[Jonathan Nieder]

Hi Antonio,

Antonio Ospite wrote:
> Junio C Hamano <gitster@pobox.com> wrote:

>> You are breaking the && chain here.
>
> Some other tests do that as well, the last line is a command by
> itself not and-chained with the git-send-email invocation. I guess the
> logic behind this is that the test succeeds if the _last_ command
> succeeds. If this is wrong then some other tests are affected too.

Yes, breaking the && chain is never a good thing.

See:

 - t/README: "Chain your test assertions"
 - v1.5.4~20 (t9001: add missing && operators, 2008-01-21)
 - git log --grep=&&

Hope that helps,
Jonathan

Re: [PATCH v3] git-send-email.perl: make initial In-Reply-To apply only to first email

[Matthieu Moy]

Antonio Ospite <ospite@studenti.unina.it> writes:

> When an initial In-Reply-To is supplied it should apply only to the
> first email, second and subsequent messages should behave just according
> to the --[no-]chain-reply-to setting; this is the typical behaviour we
> want when we send a series with cover letter in reply to some
> discussion,

+1 on this. I've been biten by this behavior sending the v2 of
a patch serie --in-reply-to the cover letter for the v1. The two
versions of each patch appear as reply to the original cover letter,
it's kind of a mess. I was really expecting the patch serie to appear
as a separate subtree in the discussion.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCH v2] git-send-email.perl: fix In-Reply-To for second and subsequent patches

[Antonio Ospite]

[-- Attachment #1: Type: text/plain, Size: 1415 bytes --]

On Fri, 5 Nov 2010 16:41:59 -0500
Jonathan Nieder <jrnieder@gmail.com> wrote:

> Hi Antonio,
> 
> Antonio Ospite wrote:
> > Junio C Hamano <gitster@pobox.com> wrote:
> 
> >> You are breaking the && chain here.
> >
> > Some other tests do that as well, the last line is a command by
> > itself not and-chained with the git-send-email invocation. I guess the
> > logic behind this is that the test succeeds if the _last_ command
> > succeeds. If this is wrong then some other tests are affected too.
> 
> Yes, breaking the && chain is never a good thing.
> 
> See:
> 
>  - t/README: "Chain your test assertions"
>  - v1.5.4~20 (t9001: add missing && operators, 2008-01-21)
>  - git log --grep=&&
> 

Thanks Jonathan, I am fixing that also to some other tests in t9001
right now.

Let me know if the v3 in this series is going to be applied as is, so I
can fix the newly added test too. If a v4 is needed than I'll fix my
test there.

I would also like to point your attention on tests like
"confirm by default (due to cc)" and following in t9001, which are
storing return value of an intermediate command, how to fix those?

Thanks,
   Antonio

-- 
Antonio Ospite
http://ao2.it

PGP public key ID: 0x4553B001

A: Because it messes up the order in which people normally read text.
   See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?

[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH v3] git-send-email.perl: make initial In-Reply-To apply only to first email

[Junio C Hamano]

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:

> I've been biten by this behavior sending the v2 of
> a patch serie --in-reply-to the cover letter for the v1. The two
> versions of each patch appear as reply to the original cover letter,
> it's kind of a mess. I was really expecting the patch serie to appear
> as a separate subtree in the discussion.

The above is much better description of what issue the patch is trying to
address; something like that should go to the description.

Antonio, I've already queued a few tests that document the established
behaviour on ao/send-email-irt branch (54aae5e1), so could you rebase your
patch on it, perhaps with an updated explanation in the log (and in the
documentation)?

Thanks, both.

Re: [PATCH v3] git-send-email.perl: make initial In-Reply-To apply only to first email

[Antonio Ospite]

[-- Attachment #1: Type: text/plain, Size: 2820 bytes --]

On Tue, 09 Nov 2010 13:23:07 -0800
Junio C Hamano <gitster@pobox.com> wrote:

> Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
> 
> > I've been biten by this behavior sending the v2 of
> > a patch serie --in-reply-to the cover letter for the v1. The two
> > versions of each patch appear as reply to the original cover letter,
> > it's kind of a mess. I was really expecting the patch serie to appear
> > as a separate subtree in the discussion.
> 
> The above is much better description of what issue the patch is trying to
> address; something like that should go to the description.
>

Alright, I'll try mentioning the actual use case too.

> Antonio, I've already queued a few tests that document the established
> behaviour on ao/send-email-irt branch (54aae5e1), so could you rebase your
> patch on it, perhaps with an updated explanation in the log (and in the
> documentation)?
>

Junio, ao/send-email-irt seems to have been merged into origin/next, so
I am rebasing on that. About the tests, I am going to modify one of your
tests instead of adding another one, is that OK? This is a change of the
established behavior after all, so the relative test have to change too,
something along these lines:

diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh
index 66e4852..c56787f 100755
--- a/t/t9001-send-email.sh
+++ b/t/t9001-send-email.sh
@@ -324,9 +324,11 @@ test_expect_success $PREREQ 'In-Reply-To without --chain-reply-to' '
                --smtp-server="$(pwd)/fake.sendmail" \
                $patches $patches $patches \
                2>errors &&
-       # All the messages are replies to --in-reply-to
+       # The first message is a reply to --in-reply-to
        sed -n -e "s/^In-Reply-To: *\(.*\)/\1/p" msgtxt1 >actual &&
        test_cmp expect actual &&
+        # Second and subsequent messages are replies to the first one
+       sed -n -e "s/^Message-Id: *\(.*\)/\1/p" msgtxt1 >expect &&
        sed -n -e "s/^In-Reply-To: *\(.*\)/\1/p" msgtxt2 >actual &&
        test_cmp expect actual &&
        sed -n -e "s/^In-Reply-To: *\(.*\)/\1/p" msgtxt3 >actual &&


Let me just stress out that 3a. as in 54aae5e1 is not well specified
either, that's what all this fuss is about. I notice you didn't comment
about my view of the "independence" of the --in-reply-to setting wrt.
--[no-]chain-reply-to but I guess that falls into the implicit/explicit
debate, so I am not pushing it and just follow your directions about
explicitly relating the two.

> Thanks, both.
> 

Regards,
   Antonio

-- 
Antonio Ospite
http://ao2.it

PGP public key ID: 0x4553B001

A: Because it messes up the order in which people normally read text.
   See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?

[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH v3] git-send-email.perl: make initial In-Reply-To apply only to first email

[Junio C Hamano]

Antonio Ospite <ospite@studenti.unina.it> writes:

> ... About the tests, I am going to modify one of your
> tests instead of adding another one, is that OK?

It is not just Ok but is preferred; that is a good way to document where
the behaviour changed how, and for what reason ;-).

Perhaps an illustration in the documentation may help.  Until I read what
Matthieu wrote in his message, I didn't quite get why anybody wanted this
new behaviour---my understanding of which is to get something like this:

    [PATCH 0/2] Here is what I did...
     [PATCH 1/2] Clean up and tests
     [PATCH 2/2] Implementation
     [PATCH v2 0/3] Here is a reroll
      [PATCH v2 1/3] Clean up
      [PATCH v2 2/3] New tests
      [PATCH v2 3/3] Implementation

when sending the re-rolled series, with the --in-reply-to for [v2 0/3] set
to [0/2] of the original.  If you illustrate the current behaviour in a
similar way in your commit log message, perhaps side-by-side to save
vertical space, it would help make it clear why people would want the new
behaviour.

[PATCHi v4] git-send-email.perl: make initial In-Reply-To apply only to first email

[Antonio Ospite]

When an initial In-Reply-To is supplied it should apply only to the
first email, second and subsequent messages should behave just according
to the --[no-]chain-reply-to setting; this is also what the man page
says about the --[no-]chain-reply-to option and this is also how the
correspondent git-format-patch option behaves.

This is the typical behaviour we    |
want when we send a series with     | [PATCH 0/2] Here is what I did...
cover letter in reply to some       |   [PATCH 1/2] Clean up and tests
discussion, the new patch series    |   [PATCH 2/2] Implementation
should appear as a separate subtree |   [PATCH v2 0/3] Here is a reroll
in the discussion, look at the v2   |     [PATCH v2 1/3] Clean up
series in the illustration on the   |     [PATCH v2 2/3] New tests
right to see what the new behaviour |     [PATCH v2 3/3] Implementation
ensures.                            |

Moreover, when $initial_reply_to is asked to the user interactively it
is asked as the "Message-ID to be used as In-Reply-To for the _first_
email", this makes the user think that the second and subsequent patches
are not using it but are considered as replies to the first message or
chained according to the --[no-]chain-reply setting.

Fix also the documentation about --in-reply-to to avoid ambiguities.

NOTE: This patch changes the current behaviour and brings it to be what
I think were the intentions stated in the documentation, also aligning it
to how git-format-patch behaves; in order to achieve the old behaviour
of a flat structure in reply to something the user can always use
"--no-thread --in-reply-to <...>".

Signed-off-by: Antonio Ospite <ospite@studenti.unina.it>
---

Hoping we are there. Patch is on top of origin/next.

Changes since v3:
 - Change the test about 'In-Reply-To without --chain-reply-to' instead of
   providing  a new one.
 - Illustrate an actual use case when describing the new behaviour, both in
   the commit message and in the documentation.

It is cool to see how such a small change in the code requires quite some
"communication overhead", not a big surprise in general but in this case I
wonder if I've been overly verbose.

Junio, if you feel that the documentation and the commit message can be slimmed
down feel free to do it when committing the patch.

Matthieu, maybe we can have a Tested-by: you, can't we?

Thanks a lot,
   Antonio

 Documentation/git-send-email.txt |   25 ++++++++++++++++++++-----
 git-send-email.perl              |    3 ++-
 t/t9001-send-email.sh            |    4 +++-
 3 files changed, 25 insertions(+), 7 deletions(-)

diff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt
index 05904e0..ebc024a 100644
--- a/Documentation/git-send-email.txt
+++ b/Documentation/git-send-email.txt
@@ -82,11 +82,26 @@ See the CONFIGURATION section for 'sendemail.multiedit'.
 	set, as returned by "git var -l".
 
 --in-reply-to=<identifier>::
-	Specify the contents of the first In-Reply-To header.
-	Subsequent emails will refer to the previous email
-	instead of this if --chain-reply-to is set.
-	Only necessary if --compose is also set.  If --compose
-	is not set, this will be prompted for.
+	Make the first mail (or all the mails with `--no-thread`) appear as a
+	reply to the given Message-Id, which avoids breaking threads to
+	provide a new patch series.
+	The second and subsequent emails will be sent as replies according to
+	the `--[no]-chain-reply-to` setting.
++
+So for example when `--thread` and `--no-chain-reply-to` are specified, the
+second and subsequent patches will be replies to the first one like in the
+illustration below where `[PATCH v2 0/3]` is in reply to `[PATCH 0/2]`:
++
+  [PATCH 0/2] Here is what I did...
+    [PATCH 1/2] Clean up and tests
+    [PATCH 2/2] Implementation
+    [PATCH v2 0/3] Here is a reroll
+      [PATCH v2 1/3] Clean up
+      [PATCH v2 2/3] New tests
+      [PATCH v2 3/3] Implementation
++
+Only necessary if --compose is also set.  If --compose
+is not set, this will be prompted for.
 
 --subject=<string>::
 	Specify the initial subject of the email thread.
diff --git a/git-send-email.perl b/git-send-email.perl
index f68ed5a..fe6b848 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -1319,7 +1319,8 @@ foreach my $t (@files) {
 
 	# set up for the next message
 	if ($thread && $message_was_sent &&
-		(chain_reply_to() || !defined $reply_to || length($reply_to) == 0)) {
+		(chain_reply_to() || !defined $reply_to || length($reply_to) == 0 ||
+		$message_num == 1)) {
 		$reply_to = $message_id;
 		if (length $references > 0) {
 			$references .= "\n $message_id";
diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh
index 26c2e93..5e48318 100755
--- a/t/t9001-send-email.sh
+++ b/t/t9001-send-email.sh
@@ -324,9 +324,11 @@ test_expect_success $PREREQ 'In-Reply-To without --chain-reply-to' '
 		--smtp-server="$(pwd)/fake.sendmail" \
 		$patches $patches $patches \
 		2>errors &&
-	# All the messages are replies to --in-reply-to
+	# The first message is a reply to --in-reply-to
 	sed -n -e "s/^In-Reply-To: *\(.*\)/\1/p" msgtxt1 >actual &&
 	test_cmp expect actual &&
+	# Second and subsequent messages are replies to the first one
+	sed -n -e "s/^Message-Id: *\(.*\)/\1/p" msgtxt1 >expect &&
 	sed -n -e "s/^In-Reply-To: *\(.*\)/\1/p" msgtxt2 >actual &&
 	test_cmp expect actual &&
 	sed -n -e "s/^In-Reply-To: *\(.*\)/\1/p" msgtxt3 >actual &&
-- 
1.7.2.3

Re: [PATCHi v4] git-send-email.perl: make initial In-Reply-To apply only to first email

[Junio C Hamano]

Antonio Ospite <ospite@studenti.unina.it> writes:

> When an initial In-Reply-To is supplied it should apply only to the
> first email, second and subsequent messages should behave just according
> to the --[no-]chain-reply-to setting; this is also what the man page
> says about the --[no-]chain-reply-to option and this is also how the
> correspondent git-format-patch option behaves.
>
> This is the typical behaviour we    |
> want when we send a series with     | [PATCH 0/2] Here is what I did...
> cover letter in reply to some       |   [PATCH 1/2] Clean up and tests
> discussion, the new patch series    |   [PATCH 2/2] Implementation
> should appear as a separate subtree |   [PATCH v2 0/3] Here is a reroll
> in the discussion, look at the v2   |     [PATCH v2 1/3] Clean up
> series in the illustration on the   |     [PATCH v2 2/3] New tests
> right to see what the new behaviour |     [PATCH v2 3/3] Implementation
> ensures.                            |

Yuck.

It is a common trap to think that everybody already knows what you have
been suffering from.  It certainly is sufficient to show the output after
your patch to convince them that your change is a good thing.

IOW, if you do two-column, please do it right ;-).  The LHS should show
how the output _used to_ look like, so that even people who didn't
particulary care (because they never hit the issue) can see why the
updated behaviour is desirable.

Re: [PATCHi v4] git-send-email.perl: make initial In-Reply-To apply only to first email

[Junio C Hamano]

Antonio Ospite <ospite@studenti.unina.it> writes:

> diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh
> index 26c2e93..5e48318 100755
> --- a/t/t9001-send-email.sh
> +++ b/t/t9001-send-email.sh
> @@ -324,9 +324,11 @@ test_expect_success $PREREQ 'In-Reply-To without --chain-reply-to' '
>  		--smtp-server="$(pwd)/fake.sendmail" \
>  		$patches $patches $patches \
>  		2>errors &&
> -	# All the messages are replies to --in-reply-to
> +	# The first message is a reply to --in-reply-to
>  	sed -n -e "s/^In-Reply-To: *\(.*\)/\1/p" msgtxt1 >actual &&
>  	test_cmp expect actual &&
> +	# Second and subsequent messages are replies to the first one
> +	sed -n -e "s/^Message-Id: *\(.*\)/\1/p" msgtxt1 >expect &&
>  	sed -n -e "s/^In-Reply-To: *\(.*\)/\1/p" msgtxt2 >actual &&
>  	test_cmp expect actual &&
>  	sed -n -e "s/^In-Reply-To: *\(.*\)/\1/p" msgtxt3 >actual &&

Looks good ;-)

I'll do the obvious fix-up (below) at my end, so if there is nothing else
there is no need to resend.

    Look at the v2 series in the illustration to see what the new behavior
    ensures:

           (before the patch)          |      (after the patch)
     [PATCH 0/2] Here is what I did... | [PATCH 0/2] Here is what I did...
       [PATCH 1/2] Clean up and tests  |   [PATCH 1/2] Clean up and tests
       [PATCH 2/2] Implementation      |   [PATCH 2/2] Implementation
       [PATCH v2 0/3] Here is a reroll |   [PATCH v2 0/3] Here is a reroll
       [PATCH v2 1/3] Clean up         |     [PATCH v2 1/3] Clean up
       [PATCH v2 2/3] New tests        |     [PATCH v2 2/3] New tests
       [PATCH v2 3/3] Implementation   |     [PATCH v2 3/3] Implementation

    This is the typical behaviour we want when we send a series with cover
    letter in reply to some discussion, the new patch series should appear
    as a separate subtree in the discussion.

Thanks.

Re: [PATCHi v4] git-send-email.perl: make initial In-Reply-To apply only to first email

[Antonio Ospite]

[-- Attachment #1: Type: text/plain, Size: 1283 bytes --]

On Fri, 12 Nov 2010 13:44:13 -0800
Junio C Hamano <gitster@pobox.com> wrote:

> Antonio Ospite <ospite@studenti.unina.it> writes:
> 
[...]
> I'll do the obvious fix-up (below) at my end, so if there is nothing else
> there is no need to resend.
>
>     Look at the v2 series in the illustration to see what the new behavior
>     ensures:
> 
>            (before the patch)          |      (after the patch)
>      [PATCH 0/2] Here is what I did... | [PATCH 0/2] Here is what I did...
>        [PATCH 1/2] Clean up and tests  |   [PATCH 1/2] Clean up and tests
>        [PATCH 2/2] Implementation      |   [PATCH 2/2] Implementation
>        [PATCH v2 0/3] Here is a reroll |   [PATCH v2 0/3] Here is a reroll
>        [PATCH v2 1/3] Clean up         |     [PATCH v2 1/3] Clean up
>        [PATCH v2 2/3] New tests        |     [PATCH v2 2/3] New tests
>        [PATCH v2 3/3] Implementation   |     [PATCH v2 3/3] Implementation
> 
[...]

Thanks for taking care of that, I won't forget the lesson.

Regards,
   Antonio

-- 
Antonio Ospite
http://ao2.it

PGP public key ID: 0x4553B001

A: Because it messes up the order in which people normally read text.
   See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?

[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]