[PATCH] git-format-patch: Make the second and subsequent mails replies to the first

[PATCH] git-format-patch: Make the second and subsequent mails replies to the first

[Josh Triplett]

Add message_id and ref_message_id fields to struct rev_info, used in show_log
with CMIT_FMT_EMAIL to set Message-Id and In-Reply-To/References respectively.
Use these in git-format-patch to make the second and subsequent patch mails
replies to the first patch mail.

Signed-off-by: Josh Triplett <josh@freedesktop.org>
---
git-send-email already does this, but this change helps people who use
things like git-imap-send or similar to send the patch emails by other
means.

 builtin-log.c |   23 +++++++++++++++++++++++
 log-tree.c    |    5 +++++
 revision.h    |    2 ++
 3 files changed, 30 insertions(+), 0 deletions(-)

diff --git a/builtin-log.c b/builtin-log.c
index 864c6cd..9d0cae1 100644
--- a/builtin-log.c
+++ b/builtin-log.c
@@ -220,6 +220,17 @@ static void get_patch_ids(struct rev_inf
 	o2->flags = flags2;
 }
 
+static void gen_message_id(char *dest, unsigned int length, char *base)
+{
+	const char *committer = git_committer_info(1);
+	const char *email_start = strrchr(committer, '<');
+	const char *email_end = strrchr(committer, '>');
+	if(!email_start || !email_end || email_start > email_end - 1)
+		die("Could not extract email from committer identity.");
+	snprintf(dest, length, "%s.%u.git.%.*s", base, time(NULL),
+		 email_end - email_start - 1, email_start + 1);
+}
+
 int cmd_format_patch(int argc, const char **argv, char **envp)
 {
 	struct commit *commit;
@@ -233,6 +244,8 @@ int cmd_format_patch(int argc, const cha
 	int ignore_if_in_upstream = 0;
 	struct diff_options patch_id_opts;
 	char *add_signoff = NULL;
+	char message_id[1024];
+	char ref_message_id[1024];
 
 	init_revisions(&rev);
 	rev.commit_format = CMIT_FMT_EMAIL;
@@ -359,6 +372,16 @@ int cmd_format_patch(int argc, const cha
 		int shown;
 		commit = list[nr];
 		rev.nr = total - nr + (start_number - 1);
+		/* Make the second and subsequent mails replies to the first */
+		if (nr == (total - 2)) {
+			strncpy(ref_message_id, message_id,
+				sizeof(ref_message_id));
+			ref_message_id[sizeof(ref_message_id)-1] = '\0';
+			rev.ref_message_id = ref_message_id;
+		}
+		gen_message_id(message_id, sizeof(message_id),
+			       sha1_to_hex(commit->object.sha1));
+		rev.message_id = message_id;
 		if (!use_stdout)
 			reopen_stdout(commit, rev.nr, keep_subject);
 		shown = log_tree_commit(&rev, commit);
diff --git a/log-tree.c b/log-tree.c
index 9d8d46f..4971988 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -97,6 +97,11 @@ void show_log(struct rev_info *opt, cons
 			subject = "Subject: ";
 
 		printf("From %s Mon Sep 17 00:00:00 2001\n", sha1);
+		if (opt->message_id)
+			printf("Message-Id: <%s>\n", opt->message_id);
+		if (opt->ref_message_id)
+			printf("In-Reply-To: <%s>\nReferences: <%s>\n",
+			       opt->ref_message_id, opt->ref_message_id);
 		if (opt->mime_boundary) {
 			static char subject_buffer[1024];
 			static char buffer[1024];
diff --git a/revision.h b/revision.h
index c010a08..e23ec8f 100644
--- a/revision.h
+++ b/revision.h
@@ -61,6 +61,8 @@ struct rev_info {
 	struct log_info *loginfo;
 	int		nr, total;
 	const char	*mime_boundary;
+	const char	*message_id;
+	const char	*ref_message_id;
 	const char	*add_signoff;
 	const char	*extra_headers;
 
-- 
1.4.1.gbe4c7-dirty

Re: [PATCH] git-format-patch: Make the second and subsequent mails replies to the first

[Johannes Schindelin]

Hi,

please make that behaviour optional.

Ciao,
Dscho

Re: [PATCH] git-format-patch: Make the second and subsequent mails replies to the first

[Erik Mouw]

On Mon, Jul 10, 2006 at 06:01:48PM +0200, Johannes Schindelin wrote:
> please make that behaviour optional.

Rather make it consistent with git-send-email. Principle of least
surprise.


Erik

-- 
+-- Erik Mouw -- www.harddisk-recovery.com -- +31 70 370 12 90 --
| Lab address: Delftechpark 26, 2628 XH, Delft, The Netherlands

Re: [PATCH] git-format-patch: Make the second and subsequent mails replies to the first

[Josh Triplett]

On Mon, 2006-07-10 at 18:29 +0200, Erik Mouw wrote:
> On Mon, Jul 10, 2006 at 06:01:48PM +0200, Johannes Schindelin wrote:
> > please make that behaviour optional.
> 
> Rather make it consistent with git-send-email. Principle of least
> surprise.

Well, git-send-email does not include an option to disable the threading
headers, so consistency with git-send-email would imply not including
any such option.  I can, however, implement a --no-thread option to omit
the headers, as well as git-send-email's --in-reply-to option to set an
initial In-Reply-To/References.  New patch series shortly.

- Josh Triplett

[PATCH 1/3] git-format-patch: Make the second and subsequent mails replies to the first

[Josh Triplett]

Add message_id and ref_message_id fields to struct rev_info, used in show_log
with CMIT_FMT_EMAIL to set Message-Id and In-Reply-To/References respectively.
Use these in git-format-patch to make the second and subsequent patch mails
replies to the first patch mail.

Signed-off-by: Josh Triplett <josh@freedesktop.org>
---
Resend of previous patch as part of new patch series.

 builtin-log.c |   23 +++++++++++++++++++++++
 log-tree.c    |    5 +++++
 revision.h    |    2 ++
 3 files changed, 30 insertions(+), 0 deletions(-)

diff --git a/builtin-log.c b/builtin-log.c
index 864c6cd..9d0cae1 100644
--- a/builtin-log.c
+++ b/builtin-log.c
@@ -220,6 +220,17 @@ static void get_patch_ids(struct rev_inf
 	o2->flags = flags2;
 }
 
+static void gen_message_id(char *dest, unsigned int length, char *base)
+{
+	const char *committer = git_committer_info(1);
+	const char *email_start = strrchr(committer, '<');
+	const char *email_end = strrchr(committer, '>');
+	if(!email_start || !email_end || email_start > email_end - 1)
+		die("Could not extract email from committer identity.");
+	snprintf(dest, length, "%s.%u.git.%.*s", base, time(NULL),
+		 email_end - email_start - 1, email_start + 1);
+}
+
 int cmd_format_patch(int argc, const char **argv, char **envp)
 {
 	struct commit *commit;
@@ -233,6 +244,8 @@ int cmd_format_patch(int argc, const cha
 	int ignore_if_in_upstream = 0;
 	struct diff_options patch_id_opts;
 	char *add_signoff = NULL;
+	char message_id[1024];
+	char ref_message_id[1024];
 
 	init_revisions(&rev);
 	rev.commit_format = CMIT_FMT_EMAIL;
@@ -359,6 +372,16 @@ int cmd_format_patch(int argc, const cha
 		int shown;
 		commit = list[nr];
 		rev.nr = total - nr + (start_number - 1);
+		/* Make the second and subsequent mails replies to the first */
+		if (nr == (total - 2)) {
+			strncpy(ref_message_id, message_id,
+				sizeof(ref_message_id));
+			ref_message_id[sizeof(ref_message_id)-1] = '\0';
+			rev.ref_message_id = ref_message_id;
+		}
+		gen_message_id(message_id, sizeof(message_id),
+			       sha1_to_hex(commit->object.sha1));
+		rev.message_id = message_id;
 		if (!use_stdout)
 			reopen_stdout(commit, rev.nr, keep_subject);
 		shown = log_tree_commit(&rev, commit);
diff --git a/log-tree.c b/log-tree.c
index 9d8d46f..4971988 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -97,6 +97,11 @@ void show_log(struct rev_info *opt, cons
 			subject = "Subject: ";
 
 		printf("From %s Mon Sep 17 00:00:00 2001\n", sha1);
+		if (opt->message_id)
+			printf("Message-Id: <%s>\n", opt->message_id);
+		if (opt->ref_message_id)
+			printf("In-Reply-To: <%s>\nReferences: <%s>\n",
+			       opt->ref_message_id, opt->ref_message_id);
 		if (opt->mime_boundary) {
 			static char subject_buffer[1024];
 			static char buffer[1024];
diff --git a/revision.h b/revision.h
index c010a08..e23ec8f 100644
--- a/revision.h
+++ b/revision.h
@@ -61,6 +61,8 @@ struct rev_info {
 	struct log_info *loginfo;
 	int		nr, total;
 	const char	*mime_boundary;
+	const char	*message_id;
+	const char	*ref_message_id;
 	const char	*add_signoff;
 	const char	*extra_headers;
 
-- 
1.4.1.gf029

Re: [PATCH] git-format-patch: Make the second and subsequent mails replies to the first

[Jakub Narebski]

Josh Triplett wrote:

> On Mon, 2006-07-10 at 18:29 +0200, Erik Mouw wrote:
>> On Mon, Jul 10, 2006 at 06:01:48PM +0200, Johannes Schindelin wrote:
>> > please make that behaviour optional.
>> 
>> Rather make it consistent with git-send-email. Principle of least
>> surprise.
> 
> Well, git-send-email does not include an option to disable the threading
> headers, so consistency with git-send-email would imply not including
> any such option.  I can, however, implement a --no-thread option to omit
> the headers, as well as git-send-email's --in-reply-to option to set an
> initial In-Reply-To/References.  New patch series shortly.

git-send-email has three ways of sending files:
 1. Chain Reply-To:, where every patch refers to earlier in series.
    Ugly in threaded mail/news readers, harder to comment, but there is
    no way to loose the order (e.g. if patches are not numbered *blush*)
 2. No chain reply-to, with cover letter introducing patch series.
    IMHO nicest format... provided there are no errors nor mistakes.
 3. No chain reply-to, without cover letter. I presonally don't like 
    this format, YMMV.
-- 
Jakub Narebski
Warsaw, Poland
ShadeHawk on #git

Re: [PATCH 1/3] git-format-patch: Make the second and subsequent mails replies to the first

[Junio C Hamano]

Josh Triplett <josht@us.ibm.com> writes:

> Add message_id and ref_message_id fields to struct rev_info, used in show_log
> with CMIT_FMT_EMAIL to set Message-Id and In-Reply-To/References respectively.
> Use these in git-format-patch to make the second and subsequent patch mails
> replies to the first patch mail.
>
> Signed-off-by: Josh Triplett <josh@freedesktop.org>
> ---
> Resend of previous patch as part of new patch series.

While I understand what you said about imap-send, I really would
feel better if this was optional.  Do not change the default
output format, please.

Re: [PATCH 1/3] git-format-patch: Make the second and subsequent mails replies to the first

[Josh Triplett]

On Mon, 2006-07-10 at 14:44 -0700, Junio C Hamano wrote:
> Josh Triplett <josht@us.ibm.com> writes:
> 
> > Add message_id and ref_message_id fields to struct rev_info, used in show_log
> > with CMIT_FMT_EMAIL to set Message-Id and In-Reply-To/References respectively.
> > Use these in git-format-patch to make the second and subsequent patch mails
> > replies to the first patch mail.
> >
> > Signed-off-by: Josh Triplett <josh@freedesktop.org>
> > ---
> > Resend of previous patch as part of new patch series.
> 
> While I understand what you said about imap-send, I really would
> feel better if this was optional.  Do not change the default
> output format, please.

So rather than the --no-thread option provided in the second patch of
this series, you'd prefer a --thread option to enable setting the
In-Reply-To/References headers?

Note that I based the direction of the --no-thread switch on
git-send-email's inclusion of these headers by default with no way to
turn them off, figuring that having an option to do so gave it an
advantage over git-send-email while remaining consistent with it.  I
also figured that most people would not mess with the defaults, and thus
the default should make a patch series more readable on mailing lists.

- Josh Triplett

Re: [PATCH 1/3] git-format-patch: Make the second and subsequent mails replies to the first

[Junio C Hamano]

Josh Triplett <josht@us.ibm.com> writes:

>> While I understand what you said about imap-send, I really would
>> feel better if this was optional.  Do not change the default
>> output format, please.
>
> So rather than the --no-thread option provided in the second patch of
> this series, you'd prefer a --thread option to enable setting the
> In-Reply-To/References headers?

Eh, that's not what I meant.

I do not mind the code you added to log-tree.c and revision.h,
and honestly I do not care which of threading or non-threading
mode is the default, although I think your explanation that it
parallels what send-email does makes a lot of sense.

But I do mind that the code added by the first patch to
cmd_format_patch runs by default, and worse yet, there is no
option turn it off.  Setting message_id and ref_message_id in
rev_info struct should be something the end user should ask for
explicitly by invoking the command with an option, perhaps
--with-message-id, which you probably would also want to turn on
when any of --no-thread, --thread or --initial-reply-to options
are given.
-

Re: [PATCH 1/3] git-format-patch: Make the second and subsequent mails replies to the first

[Josh Triplett]

On Fri, 2006-07-14 at 11:23 -0700, Junio C Hamano wrote:
> Josh Triplett <josht@us.ibm.com> writes:
> 
> >> While I understand what you said about imap-send, I really would
> >> feel better if this was optional.  Do not change the default
> >> output format, please.
> >
> > So rather than the --no-thread option provided in the second patch of
> > this series, you'd prefer a --thread option to enable setting the
> > In-Reply-To/References headers?
> 
> Eh, that's not what I meant.
> 
> I do not mind the code you added to log-tree.c and revision.h,
> and honestly I do not care which of threading or non-threading
> mode is the default, although I think your explanation that it
> parallels what send-email does makes a lot of sense.
> 
> But I do mind that the code added by the first patch to
> cmd_format_patch runs by default, and worse yet, there is no
> option turn it off.  Setting message_id and ref_message_id in
> rev_info struct should be something the end user should ask for
> explicitly by invoking the command with an option, perhaps
> --with-message-id, which you probably would also want to turn on
> when any of --no-thread, --thread or --initial-reply-to options
> are given.

How would that work though?  Threading requires a Message-Id on at least
the first message, so to avoid Message-IDs by default would require
turning off threading by default; I can do that if you like, but you
suggested that you didn't mind having threading as the default.  I
could, however, avoid generating Message-Id on the subsequent messages,
and avoid generating that Message-Id if you give --no-thread.  Would
that work?

- Josh Triplett

Re: [PATCH 1/3] git-format-patch: Make the second and subsequent mails replies to the first

[Junio C Hamano]

Josh Triplett <josht@us.ibm.com> writes:

> ..., but you
> suggested that you didn't mind having threading as the default.

Did I? ... then that was either a mistake or miscommunication.

I do mind changing the default output.  I do not mind threading
as the default ONLY IF user asks for output with these extra
headers.

Re: [PATCH 1/3] git-format-patch: Make the second and subsequent mails replies to the first

[Petr Baudis]

Dear diary, on Fri, Jul 14, 2006 at 09:32:27PM CEST, I got a letter
where Junio C Hamano <junkio@cox.net> said that...
> Josh Triplett <josht@us.ibm.com> writes:
> 
> > ..., but you
> > suggested that you didn't mind having threading as the default.
> 
> Did I? ... then that was either a mistake or miscommunication.
> 
> I do mind changing the default output.  I do not mind threading
> as the default ONLY IF user asks for output with these extra
> headers.

What's the big deal? It's not like we didn't change those things in the
past if it doesn't horribly break everything and the new behaviour is
clearly more sensible.

It would be good to know what the general policy on this is nowadays.

-- 
				Petr "Pasky" Baudis
Stuff: http://pasky.or.cz/
Snow falling on Perl. White noise covering line noise.
Hides all the bugs too. -- J. Putnam

Re: [PATCH 1/3] git-format-patch: Make the second and subsequent mails replies to the first

[Junio C Hamano]

Petr Baudis <pasky@suse.cz> writes:

> Dear diary, on Fri, Jul 14, 2006 at 09:32:27PM CEST, I got a letter
> where Junio C Hamano <junkio@cox.net> said that...
>> Josh Triplett <josht@us.ibm.com> writes:
>> 
>> > ..., but you
>> > suggested that you didn't mind having threading as the default.
>> 
>> Did I? ... then that was either a mistake or miscommunication.
>> 
>> I do mind changing the default output.  I do not mind threading
>> as the default ONLY IF user asks for output with these extra
>> headers.
>
> What's the big deal? It's not like we didn't change those things in the
> past if it doesn't horribly break everything and the new behaviour is
> clearly more sensible.

While I agree to the whole three lines, I do not think adding the
Message-Id and In-Reply-To header lines by default is more
sensible at all.

Adding phoney Message-Id to format-patch output makes some sense
only when you are sending messages, and if I recall original
"motive" message correctly only with git-imap-send.  We do not
need this for git-send-email, since it can do its own threading.

Although I've already accepted the series to "next", now after
you brought up the issue, I started to suspect that it might
even make sense not to do this in format-patch but make it a
responsibility for MUA-looking commands instead.

More importantly, format-patch is used to extract patches into
separate files (I do that myself often, and I think Andrew
Morten uses it to extract stuff from git-maintained trees).  In
such a case having phoney Message-Id is simply a waste.  Running
"head -n X 0*.txt" now needs one or two larger X to view the
same information, and fewer patches fit on the screen than
before.  So the new behaviour, if it were not optional, is
clearly less useful for such purpose.

It could even be confusing and inviting mistakes.  When quoting
a change from somebody that was sent in an e-mail to the list,
giving its Message-Id is often helpful to others who want to go
to the source themselves, but if a file that was generated by
format-patch by default carries a phoney Message-Id, it can be
mistakenly used in such a quote.