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

[Antonio Ospite <ospite@studenti.unina.it>]

[-- 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 --]