login
Search
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) {
...(+cc: some send-email people) Hi, 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 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 --
On Thu, 14 Oct 2010 13:22:50 -0500 For the new recipients, the original mail is here btw: http://permalink.gmane.org/gmane.comp.version-control.git/159039 Ok, I can add this in the commit message, I am waiting some days for I like the OR chain on the same line, but I can split it anyways if 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?
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 --- ...
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
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.
--
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" ...
On Tue, 19 Oct 2010 11:26:33 -0700 Junio C Hamano <gitster@pobox.com> wrote: Sorry, I meant the man page part about --[no-]chain-reply-to, I mistyped 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 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. 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 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?
On Wed, 20 Oct 2010 00:45:33 +0200 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?
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 ...
+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/ --
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. --
On Tue, 09 Nov 2010 13:23:07 -0800
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
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?
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.
--
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. --
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.
--
On Fri, 12 Nov 2010 13:44:13 -0800 [...] 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?
Hi Antonio, 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 --
On Fri, 5 Nov 2010 16:41:59 -0500 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?
