Re: [PATCH v2] git-send-email.perl: fix In-Reply-To for second and subsequent patches
On Tue, 19 Oct 2010 11:26:33 -0700 Junio C Hamano <gitster@pobox.com> wrote:
Thanks for commenting Junio.
Show 10 quoted lines
> 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.
>
Show 7 quoted lines
> 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.
Show 13 quoted lines
> 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.
Show 6 quoted lines
> 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?
Show 21 quoted lines
> > 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.
Show 8 quoted lines
> > + # 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?
Antonio Ospite
