[RFC] format-patch: Ensure that author and commit time are not lost

[RFC] format-patch: Ensure that author and commit time are not lost

[Björn Gustavsson]

'git format-patch' encodes the author of the commit in the From:
header and the time of the commit in the Date: header.
Depending on how the email is sent, however, those headers can be
lost before the email reaches its destination.

Therefore, if the sender of the email (i.e. the configuration
options user.name and user.email) are different from the author
of a commit, insert From: and Date: headers at the beginning of
the body of the commit message.

Signed-off-by: Björn Gustavsson <bgustavsson@gmail.com>
---
The code works, but is not appropriate for 'pu' because a fair
number of test cases will fail.

Some additional work is needed before this functionality can
be considered ready for 'pu'. Since I am rather busy at the
moment, I don't want to spend any more time on this unless I'll
get some indication that this functionality is useful for
someone else.

I would also want some input on whether it would be OK
to make this functionality unconditionally turned on so that
there will always From: and Date: headers in the commit
message.

I suspect that the answer is no, because there might be scripts
that parse the output of format-patch (perhaps to interface
with other source-code control systems).

Assuming that we need an option, should the default be to
produce the extra headers (to make sure that the original
author is not lost) or to not produce any extra headers
(for backwards compatibility)?

I suspect that the answer is that the default should be not
to generate any extra headers not to break any existing
scripts.

To implement an option is not difficult, but will need
changes in a fair number of source files in order to propagate
a boolean down to pretty_print_commit() (either as an
additional argument or possibly as an additional field
in the pretty_print_context struct).

 pretty.c |   45 +++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 45 insertions(+), 0 deletions(-)

diff --git a/pretty.c b/pretty.c
index da15cf2..63268a1 100644
--- a/pretty.c
+++ b/pretty.c
@@ -916,6 +916,36 @@ char *reencode_commit_message(const struct commit *commit, const char **encoding
 	return logmsg_reencode(commit, encoding);
 }
 
+static int sender_is_not_author(const char *message)
+{
+	const char **msg_p = &message;
+
+	for (;;) {
+		const char *line = *msg_p;
+		int linelen = get_one_line(*msg_p);
+		*msg_p += linelen;
+
+		/* Get out of here if the commit has no author. */
+		if (linelen <= 1)
+			return 0;
+
+		if (!memcmp(line, "author ", 7)) {
+			size_t name_len = strlen(git_default_name);
+			size_t email_len;
+			line += 7;
+			if (strncmp(line, git_default_name, name_len))
+				return 1;
+			line += name_len;
+			if (line[0] != ' ' || line[1] != '<')
+				return 1;
+			line += 2;
+			email_len = strlen(git_default_email);
+			return strncmp(line, git_default_email, email_len) ||
+				line[email_len] != '>';
+		}
+	}
+}
+
 void pretty_print_commit(enum cmit_fmt fmt, const struct commit *commit,
 			 struct strbuf *sb,
 			 const struct pretty_print_context *context)
@@ -977,6 +1007,21 @@ void pretty_print_commit(enum cmit_fmt fmt, const struct commit *commit,
 		pp_title_line(fmt, &msg, sb, context->subject,
 			      context->after_subject, encoding, need_8bit_cte);
 
+	/*
+	 * If we are formatting an email and if the sender is different
+	 * from the author of the commit, include the From: and Date:
+	 * headers in the body of the commit message to make sure they
+	 * are not lost.
+	 */
+	if (fmt == CMIT_FMT_EMAIL) {
+		const char *p = reencoded ? reencoded : commit->buffer;
+		if (sender_is_not_author(commit->buffer)) {
+			pp_header(fmt, context->abbrev, context->date_mode, encoding,
+				  commit, &p, sb);
+			strbuf_addch(sb, '\n');
+		}
+	}
+
 	beginning_of_body = sb->len;
 	if (fmt != CMIT_FMT_ONELINE)
 		pp_remainder(fmt, &msg, sb, indent);
-- 
1.6.5.2.186.geb718e

Re: [RFC] format-patch: Ensure that author and commit time are not lost

[Junio C Hamano]

Björn Gustavsson <bgustavsson@gmail.com> writes:

> 'git format-patch' encodes the author of the commit in the From: header
> and the time of the commit in the Date: header.  Depending on how the
> email is sent, however, those headers can be lost before the email
> reaches its destination.
>
> Therefore, if the sender of the email (i.e. the configuration options
> user.name and user.email) are different from the author of a commit,
> insert From: and Date: headers at the beginning of the body of the
> commit message.

I think you are addressing a very valid issue, but I suspect that you are
doing so at a wrong place in the "patch mail-out" workflow.

> +static int sender_is_not_author(const char *message)
> +{
> +	const char **msg_p = &message;
> +
> +	for (;;) {
> ...
> +	}
> +}

This new function is not about "Is the _sender_ the same as the author?",
but is about "Is the person who is running format-patch the same as the
author?".  There is a big difference.

What you want to catch is really "Does the MUA that sends out the final
message have the name of the author on its 'From: ' header?", and that 
depends on how the output from format-patch command is processed in the
downstream of the workflow.

You may read the file into your MUA edit session.  You would typically
edit the first three lines out and move Subject: to the MUA's subject line.
You can choose to keep From:/Date: when you do so.  This happens to be the
way I work, by the way.

The output may not even be used by a MUA; you may upload it to web based
thingy like Bugzilla or FrySpray.  The recipient will download the whole
thing and there is no need to edit.

I would expect the right solution would be to give send-email an ability
to either (1) use "Sender:" to record the operator of the MUA while
keeping "From: " taken from the payload, or (2) duplicate "From: " as an
in-body header when it sends out.

It is a separate issue if that ability should be on by default or
controlled by an option, of course.  But I do not think it should be in
the format-patch.

Re: [RFC] format-patch: Ensure that author and commit time are not lost

[Björn Gustavsson]

2009/11/15 Junio C Hamano <gitster@pobox.com>:

> I think you are addressing a very valid issue, but I suspect that you are
> doing so at a wrong place in the "patch mail-out" workflow.
[...]
> This new function is not about "Is the _sender_ the same as the author?",
> but is about "Is the person who is running format-patch the same as the
> author?".  There is a big difference.

Yes, I kind of suspected that my solution would not
be general enough to suit all users and workflows.

Thanks for the feedback.

/Björn

-- 
Björn Gustavsson, Erlang/OTP, Ericsson AB

Re: [RFC] format-patch: Ensure that author and commit time are not lost

[Jeff King]

On Sun, Nov 15, 2009 at 01:03:42PM -0800, Junio C Hamano wrote:

> I would expect the right solution would be to give send-email an ability
> to either (1) use "Sender:" to record the operator of the MUA while
> keeping "From: " taken from the payload, or (2) duplicate "From: " as an
> in-body header when it sends out.

I agree that send-email is the "right" place for this functionality in
the git toolchain. And indeed, it already does (2):

  $ sed -ne '/$author ne $sender/,+1p' git-send-email.perl
          if (defined $author and $author ne $sender) {
                          $message = "From: $author\n\n$message";

That being said, like you, I usually pull the patches directly from
format-patch into my MUA, and I fix up the headers manually. I suspect
there are many others who do the same thing. And each of us has to
either handle this case manually, or write our own munging code
ourselves for our particular setup.

So in that sense, even though format-patch is not the right place, it
may be useful for it to give tool support to people who do not use the
"format-patch to send-email" workflow. In other words, I would be
happy if my short glue shell-script became:

  git format-patch --stdout --sender-is-me "$@" >mbox &&
  mutt -f mbox

and this just handled the case properly, without me having to parse the
From header of each message and munge the messages in my script.

The arguments against it are:

  1. It is polluting format-patch with MUA cruft.

  2. --sender-is-me (besides being a terrible name) may not be
     expressive enough. You might want --sender=... depending on the
     setup of the calling script.

Honestly, though, I send few enough patches made by other people that I
have never found it to be a huge burden. This would be a minor
convenience to have.

-Peff