git-svn rebase screwing up commit messages

git-svn rebase screwing up commit messages

[Benoit SIGOURE]

[-- Attachment #1: Type: text/plain, Size: 418 bytes --]

Hello,
git version 1.5.2.3 over here, I've googled and searched in the ML  
archives but did not find this:  when I git-svn rebase, my commits in  
Git (that are temporarily removed and then re-applied) get their  
commit message flattened on a single line (needless to say this is  
utterly annoying :D).

Is this a bug or something?

Thanks.

-- 
Benoit Sigoure aka Tsuna
EPITA Research and Development Laboratory



[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 186 bytes --]

Re: git-svn rebase screwing up commit messages

[Junio C Hamano]

Benoit SIGOURE <tsuna@lrde.epita.fr> writes:

> git version 1.5.2.3 over here, I've googled and searched in the ML
> archives but did not find this:  when I git-svn rebase, my commits in
> Git (that are temporarily removed and then re-applied) get their
> commit message flattened on a single line...

Do you mean by "my commits in Git" a commit you created with git
in your git repository?

Re: git-svn rebase screwing up commit messages

[Sean]

On Sat, 28 Jul 2007 05:14:26 -0700
Junio C Hamano <gitster@pobox.com> wrote:

Hi,

> Benoit SIGOURE <tsuna@lrde.epita.fr> writes:
> 
> > git version 1.5.2.3 over here, I've googled and searched in the ML
> > archives but did not find this:  when I git-svn rebase, my commits in
> > Git (that are temporarily removed and then re-applied) get their
> > commit message flattened on a single line...
> 
> Do you mean by "my commits in Git" a commit you created with git
> in your git repository?
> 

Tested this here (rc3.24.g83b3d) and can confirm the reported problem.
After making a commit in git and then running "git svn rebase" to
receive updates from the svn repo, the rebased commit has a borked
description (multi-lined commit message appears all on one line).

Sean

Re: git-svn rebase screwing up commit messages

[Junio C Hamano]

Sean <seanlkml@sympatico.ca> writes:

>> Do you mean by "my commits in Git" a commit you created with git
>> in your git repository?
>
> Tested this here (rc3.24.g83b3d) and can confirm the reported problem.
> After making a commit in git and then running "git svn rebase" to
> receive updates from the svn repo, the rebased commit has a borked
> description (multi-lined commit message appears all on one line).

In short, your original commit log message is broken.

The recommended convention for commit messages is to start it
with a single line that describes what it does, followed by a
blank line (i.e. the first paragraph consists of a single line),
followed by a longer explanation of why the change brought by
the commit is a good thing.

Following this convention is recommended to make other peoples'
lives more pleasant, and git assumes you follow this convention
at several places.  For example, "git log --pretty=oneline",
"git reflog", and "git show-branch" are ways to get concise
listing of commits; git-shortlog gives the list of such commit
titles in its output, omitting the longer explanation.  Patches
prepared for e-mail exchange ("git format-patch", and
--pretty=email) use the title on the Subject: line of the
message.  

The behaviour of these tools are _not_ the primarly reasons why
you are better off following the commit message convention.  The
reason behind the behaviour of these tools is to help readers of
your commit messages.  The punch line comes at the beginning,
single line short-and-sweet, and when readers want to get the
birds-eye view, that is the only thing they see.

Try them on a commit that has several lines in its first
paragraph and you will see why this is a good convention.

Until recently, we chomped a multi-line first paragraph at the
first line, and used the resulting partial sentence as the
title.  This had an interesting effect on badly formatted commit
like this:

        tree 2a003266ee6fda9305833a4f6e6dc7194018805a
        parent fef221460121c8a1b9e242422d8b521fbd0f6dc0
        author Junio C Hamano <gitster@pobox.com> 1185621830 -0700
        committer Junio C Hamano <gitster@pobox.com> 1185621830 -0700

        A long and unsightly commit
        log message that has more than
        one lines in the first paragraph

        Such a first paragraph is flattened by --pretty=email

When this was formatted for e-mail, it resulted in something
like this:

        From: Junio C Hamano <gitster@pobox.com>
        Date: Sat, 28 Jul 2007 04:23:50 -0700
        Subject: [PATCH] A long and unsightly commit
        log message that has more than
        one lines in the first paragraph

        Such a first paragraph is flattened by --pretty=email

As rebase/am uses the same machinery as e-mailed patch
acceptance, the paragraph was chomped at the first line, while
normalizing it for other git commands' use, like this:

        tree 842c159f584fd4f970b6aceed21f479c1b62e333
        parent 57887443c24e5a2b4b04e7db69b44b53d8e87b44
        author Junio C Hamano <gitster@pobox.com> 1185621830 -0700
        committer Junio C Hamano <gitster@pobox.com> 1185626445 -0700

        A long and unsightly commit

        log message that has more than
        one lines in the first paragraph

        Such a first paragraph is flattened by --pretty=email

This would mean that "oneline" format will see only the initial
part of the sentence.  If your message is properly formatted,
it is not a problem.

Recent enough git instead uses RFC2822 line-folding like this,
to help noncomforming messages somewhat:

        From 4c04a94...
        From: Junio C Hamano <gitster@pobox.com>
        Date: Sat, 28 Jul 2007 04:23:50 -0700
        Subject: [PATCH] A long and unsightly commit
         log message that has more than
         one lines in the first paragraph

        Such a first paragraph is flattened by --pretty=email

But this has an interesting side effect itself.  Such a folded
line is logically treated as a single line, and rebase/am makes
a commit like this out of such a message:

        tree 842c159f584fd4f970b6aceed21f479c1b62e333
        parent 57887443c24e5a2b4b04e7db69b44b53d8e87b44
        author Junio C Hamano <gitster@pobox.com> 1185621830 -0700
        committer Junio C Hamano <gitster@pobox.com> 1185626469 -0700

        A long and unsightly commit log message that has more than one lines in the first paragraph

        Such a first paragraph is flattened by --pretty=email

which is still unsightly (but the original message is unfriendly
to oneline summary format to begin with).  At least, this is an
improvement in that the new formatting does not lose information
when viewed in "oneline" format.

Having said all that, so that the readers understand the
background, here is a not-so-heavily-tested patch, which might
help.  It passes all the test suite as before, but that tells
how existing git-svn tests do not test many things.

I am not considering this for inclusion right now, by the way.

-- >8 --
Rebase/am: preserve multi-line first paragraph better.

This is actually three patches folded into one; as such it
should not be applied as-is.  It needs to be split into:

 * Changes to refs.c::log_ref_write() to sanitize embedded
   newlines from the log message, instead of chomping it at the
   first newline;

 * Changes to symbolic-ref and update-ref, so that they do not
   refuse a reflog message that has embedded newlines.  They
   have no business in dictating what the reflog message should
   look like.

 * Changes to builtin-mailinfo.c to preserve LF in Subject:
   header, when it is folded using RFC2822 header folding.

With this, the first paragraph of a commit message that has
multiple lines is reproduced by "am" without being squashed into
a single line.

---

 builtin-mailinfo.c     |   29 +++++++++++++++++++++++------
 builtin-symbolic-ref.c |    2 --
 builtin-update-ref.c   |    2 --
 refs.c                 |   39 +++++++++++++++++++++++----------------
 4 files changed, 46 insertions(+), 26 deletions(-)

diff --git a/builtin-mailinfo.c b/builtin-mailinfo.c
index b4f6e91..9d2064a 100644
--- a/builtin-mailinfo.c
+++ b/builtin-mailinfo.c
@@ -272,15 +272,15 @@ static char *cleanup_subject(char *subject)
 	}
 }
 
-static void cleanup_space(char *buf)
+static void cleanup_space(char *buf, int keep_LF)
 {
 	unsigned char c;
 	while ((c = *buf) != 0) {
 		buf++;
-		if (isspace(c)) {
+		if (isspace(c) && (!keep_LF || c != '\n')) {
 			buf[-1] = ' ';
 			c = *buf;
-			while (isspace(c)) {
+			while (isspace(c) && (!keep_LF || c != '\n')) {
 				int len = strlen(buf);
 				memmove(buf, buf+1, len);
 				c = *buf;
@@ -425,6 +425,7 @@ static int read_one_header_line(char *line, int sz, FILE *in)
 			if (addlen >= sz - len)
 				addlen = sz - len - 1;
 			memcpy(line + len, continuation, addlen);
+			line[len] = '\n';
 			len += addlen;
 		}
 	}
@@ -846,6 +847,22 @@ static void handle_body(void)
 	return;
 }
 
+static void output_header_lines(FILE *fout, const char *hdr, char *data)
+{
+	while (1) {
+		char *ep = strchr(data, '\n');
+		int len;
+		if (!ep)
+			len = strlen(data);
+		else
+			len = ep - data;
+		fprintf(fout, "%s: %.*s\n", hdr, len, data);
+		if (!ep)
+			break;
+		data = ep + 1;
+	}
+}
+
 static void handle_info(void)
 {
 	char *sub;
@@ -864,14 +881,14 @@ static void handle_info(void)
 
 		if (!memcmp(header[i], "Subject", 7)) {
 			sub = cleanup_subject(hdr);
-			cleanup_space(sub);
-			fprintf(fout, "Subject: %s\n", sub);
+			cleanup_space(sub, 1);
+			output_header_lines(fout, "Subject", sub);
 		} else if (!memcmp(header[i], "From", 4)) {
 			handle_from(hdr);
 			fprintf(fout, "Author: %s\n", name);
 			fprintf(fout, "Email: %s\n", email);
 		} else {
-			cleanup_space(hdr);
+			cleanup_space(hdr, 0);
 			fprintf(fout, "%s: %s\n", header[i], hdr);
 		}
 	}
diff --git a/builtin-symbolic-ref.c b/builtin-symbolic-ref.c
index d41b406..9eb95e5 100644
--- a/builtin-symbolic-ref.c
+++ b/builtin-symbolic-ref.c
@@ -43,8 +43,6 @@ int cmd_symbolic_ref(int argc, const char **argv, const char *prefix)
 			msg = argv[1];
 			if (!*msg)
 				die("Refusing to perform update with empty message");
-			if (strchr(msg, '\n'))
-				die("Refusing to perform update with \\n in message");
 		}
 		else if (!strcmp("--", arg)) {
 			argc--;
diff --git a/builtin-update-ref.c b/builtin-update-ref.c
index feac2ed..8339cf1 100644
--- a/builtin-update-ref.c
+++ b/builtin-update-ref.c
@@ -23,8 +23,6 @@ int cmd_update_ref(int argc, const char **argv, const char *prefix)
 			msg = argv[++i];
 			if (!*msg)
 				die("Refusing to perform update with empty message.");
-			if (strchr(msg, '\n'))
-				die("Refusing to perform update with \\n in message.");
 			continue;
 		}
 		if (!strcmp("-d", argv[i])) {
diff --git a/refs.c b/refs.c
index 2694e70..98e3202 100644
--- a/refs.c
+++ b/refs.c
@@ -1036,6 +1036,27 @@ void unlock_ref(struct ref_lock *lock)
 	free(lock);
 }
 
+static int copy_msg(char *buf, const char *msg)
+{
+	char *cp = buf;
+	char c;
+	int wasspace = 1;
+
+	*cp++ = '\t';
+	while ((c = *msg++)) {
+		if (wasspace && isspace(c))
+			continue;
+		wasspace = isspace(c);
+		if (wasspace)
+			c = ' ';
+		*cp++ = c;
+	}
+	while (buf < cp && isspace(cp[-1]))
+		cp--;
+	*cp++ = '\n';
+	return cp - buf;
+}
+
 static int log_ref_write(const char *ref_name, const unsigned char *old_sha1,
 			 const unsigned char *new_sha1, const char *msg)
 {
@@ -1080,21 +1101,7 @@ static int log_ref_write(const char *ref_name, const unsigned char *old_sha1,
 
 	adjust_shared_perm(log_file);
 
-	msglen = 0;
-	if (msg) {
-		/* clean up the message and make sure it is a single line */
-		for ( ; *msg; msg++)
-			if (!isspace(*msg))
-				break;
-		if (*msg) {
-			const char *ep = strchr(msg, '\n');
-			if (ep)
-				msglen = ep - msg;
-			else
-				msglen = strlen(msg);
-		}
-	}
-
+	msglen = msg ? strlen(msg) : 0;
 	committer = git_committer_info(-1);
 	maxlen = strlen(committer) + msglen + 100;
 	logrec = xmalloc(maxlen);
@@ -1103,7 +1110,7 @@ static int log_ref_write(const char *ref_name, const unsigned char *old_sha1,
 		      sha1_to_hex(new_sha1),
 		      committer);
 	if (msglen)
-		len += sprintf(logrec + len - 1, "\t%.*s\n", msglen, msg) - 1;
+		len += copy_msg(logrec + len - 1, msg) - 1;
 	written = len <= maxlen ? write_in_full(logfd, logrec, len) : -1;
 	free(logrec);
 	if (close(logfd) != 0 || written != len)

Re: git-svn rebase screwing up commit messages

[Sean]

On Sat, 28 Jul 2007 06:10:49 -0700
Junio C Hamano <gitster@pobox.com> wrote:

> In short, your original commit log message is broken.
> 
> The recommended convention for commit messages is to start it
> with a single line that describes what it does, followed by a
> blank line (i.e. the first paragraph consists of a single line),
> followed by a longer explanation of why the change brought by
> the commit is a good thing.

That explains why I hadn't seen the problem before, since I
usually follow the commit message convention.  For testing
purposes I had simply mashed the home row to add five or
six lines without thinking...

> Following this convention is recommended to make other peoples'
> lives more pleasant, and git assumes you follow this convention
> at several places.  For example, "git log --pretty=oneline",
> "git reflog", and "git show-branch" are ways to get concise
> listing of commits; git-shortlog gives the list of such commit
> titles in its output, omitting the longer explanation.  Patches
> prepared for e-mail exchange ("git format-patch", and
> --pretty=email) use the title on the Subject: line of the
> message.  

Yes, Bisecting shows this "problem" was introduced in your
commit 4234a76167 which mentions that commit messages following
the normal convention would be unaffected.

...[SNIPPED]...
> Having said all that, so that the readers understand the
> background, here is a not-so-heavily-tested patch, which might
> help.  It passes all the test suite as before, but that tells
> how existing git-svn tests do not test many things.
> 
> I am not considering this for inclusion right now, by the way.

FWIW your patch fixed my test case here.

Cheers,
Sean

Re: git-svn rebase screwing up commit messages

[Junio C Hamano]

Sean <seanlkml@sympatico.ca> writes:

>> Having said all that, so that the readers understand the
>> background, here is a not-so-heavily-tested patch, which might
>> help.  It passes all the test suite as before, but that tells
>> how existing git-svn tests do not test many things.
>> 
>> I am not considering this for inclusion right now, by the way.
>
> FWIW your patch fixed my test case here.

Actually the patched behaviour actively encourages a bad (not in
the sense that those oneline tools will not work well, but in
the sense that these messages are reader unfriendly) practice; I
do not think what the patch did deserves to be called "fixed".

And that is one of the reasons, other than that we are in -rc
freeze that we do not add anything but unarguable fixes, that I
am not considering the patch for inclusion right now.

Re: git-svn rebase screwing up commit messages

[Sean]

On Sat, 28 Jul 2007 06:38:19 -0700
Junio C Hamano <gitster@pobox.com> wrote:


> Actually the patched behaviour actively encourages a bad (not in
> the sense that those oneline tools will not work well, but in
> the sense that these messages are reader unfriendly) practice; I
> do not think what the patch did deserves to be called "fixed".
> And that is one of the reasons, other than that we are in -rc
> freeze that we do not add anything but unarguable fixes, that I
> am not considering the patch for inclusion right now.
> 

First, i didn't read the patch and i have no stake at all in
non-conforming commit messages; i always follow the convention.
Having said that, the current behavior of Git crosses the line
from advocating the common commit message format into the realm
of not-working properly with non-conforming commit messages.

I would argue that you shouldn't try to have it both ways.  Either
Git supports non-conforming message formats, in which case the
current behavior seems buggy.  Or Git does not support commit
messages that deviate from the standard, in which case the
documentation should be updated to state so bluntly.

If handling email-mangled commit messages means that non-conforming
formats can no longer be fully supported (while discouraged) then
perhaps the time has come to explicitly state that people should
not expect Git to handle anything but the 1+empty+description
commit message format.

Sean

Re: git-svn rebase screwing up commit messages

[Junio C Hamano]

Sean <seanlkml@sympatico.ca> writes:

> I would argue that you shouldn't try to have it both ways.

You cannot have it both ways as-is, but this is solvable.  The
invocation of am from rebase needs an extra (internal to
implementation) option to use the code it patches, and the
regular am can fold what are found on Subject: lines it used
to.

The patch as-is breaks the more important case of e-mail
acceptance codepath, because MUAs are free to fold the Subject:
line when the original line is long, and what the user (the
original patch submitter) expects to happen is that a
single-line-ness of the original Subject: of the message to be
kept.  The patch breaks such a line at the place MUA happens to
fold such a long, single line, for comforming messages.
 

Re: git-svn rebase screwing up commit messages

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Sean <seanlkml@sympatico.ca> writes:
>
>> I would argue that you shouldn't try to have it both ways.
>
> You cannot have it both ways as-is, but this is solvable.  The
> invocation of am from rebase needs an extra (internal to
> implementation) option to use the code it patches, and the
> regular am can fold what are found on Subject: lines it used
> to.
>
> The patch as-is breaks the more important case of e-mail
> acceptance codepath, because MUAs are free to fold the Subject:
> line when the original line is long, and what the user (the
> original patch submitter) expects to happen is that a
> single-line-ness of the original Subject: of the message to be
> kept.  The patch breaks such a line at the place MUA happens to
> fold such a long, single line, for comforming messages.

So here comes an updated series, properly split along the
logical lines.

  0001-log_ref_write-do-not-chomp-reflog-message-at-the.txt
  0002-symbolic-ref-update-ref-do-not-refuse-reflog-message.txt
  0003-rebase-try-not-to-munge-commit-log-message.txt

I am not absolutely sure about the implications of mailinfo
change.  I tried to be careful by making sure that:

 - the updated code kicks in when "format-patch piped to am"
   toolchain uses the -k option on both ends, as rebase/am
   does.  This is the case where we would want to keep the
   way original commit log message was formatted.

 - otherwise the original "line folding" clean-up is used, so
   that usual e-mailed patch acceptance codepath cleanses the
   oneline summary obtained from the "Subject:" header.

Because it would be a serious regression to break the latter,
while the former is just "an improvement" [*1], somebody really
should double check the change for me.

[Footnote]

*1* It does not mean that improved support for foreign SCM
interoperation does not matter.  It is "merely an improvement"
in the sense that people do not like what the code currently
does anyway, and if the updated code is still not liked, then
not having such "an improvement" does not matter.

[PATCH 1/3] log_ref_write() -- do not chomp reflog message at the first LF

[Junio C Hamano]

A reflog file is organized as one-line-per-entry records, and we
enforced the file format integrity by chomping the given message
at the first LF.  This changes it to convert them to SP, which
is more in line with the --pretty=oneline format.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 refs.c |   44 ++++++++++++++++++++++++++++----------------
 1 files changed, 28 insertions(+), 16 deletions(-)

diff --git a/refs.c b/refs.c
index 2694e70..fac6548 100644
--- a/refs.c
+++ b/refs.c
@@ -1036,6 +1036,32 @@ void unlock_ref(struct ref_lock *lock)
 	free(lock);
 }
 
+/*
+ * copy the reflog message msg to buf, which has been allocated sufficiently
+ * large, while cleaning up the whitespaces.  Especially, convert LF to space,
+ * because reflog file is one line per entry.
+ */
+static int copy_msg(char *buf, const char *msg)
+{
+	char *cp = buf;
+	char c;
+	int wasspace = 1;
+
+	*cp++ = '\t';
+	while ((c = *msg++)) {
+		if (wasspace && isspace(c))
+			continue;
+		wasspace = isspace(c);
+		if (wasspace)
+			c = ' ';
+		*cp++ = c;
+	}
+	while (buf < cp && isspace(cp[-1]))
+		cp--;
+	*cp++ = '\n';
+	return cp - buf;
+}
+
 static int log_ref_write(const char *ref_name, const unsigned char *old_sha1,
 			 const unsigned char *new_sha1, const char *msg)
 {
@@ -1080,21 +1106,7 @@ static int log_ref_write(const char *ref_name, const unsigned char *old_sha1,
 
 	adjust_shared_perm(log_file);
 
-	msglen = 0;
-	if (msg) {
-		/* clean up the message and make sure it is a single line */
-		for ( ; *msg; msg++)
-			if (!isspace(*msg))
-				break;
-		if (*msg) {
-			const char *ep = strchr(msg, '\n');
-			if (ep)
-				msglen = ep - msg;
-			else
-				msglen = strlen(msg);
-		}
-	}
-
+	msglen = msg ? strlen(msg) : 0;
 	committer = git_committer_info(-1);
 	maxlen = strlen(committer) + msglen + 100;
 	logrec = xmalloc(maxlen);
@@ -1103,7 +1115,7 @@ static int log_ref_write(const char *ref_name, const unsigned char *old_sha1,
 		      sha1_to_hex(new_sha1),
 		      committer);
 	if (msglen)
-		len += sprintf(logrec + len - 1, "\t%.*s\n", msglen, msg) - 1;
+		len += copy_msg(logrec + len - 1, msg) - 1;
 	written = len <= maxlen ? write_in_full(logfd, logrec, len) : -1;
 	free(logrec);
 	if (close(logfd) != 0 || written != len)
-- 
1.5.3.rc3.24.g83b3d

Re: [PATCH 1/3] log_ref_write() -- do not chomp reflog message at the first LF

[Johannes Schindelin]

Hi,

On Sat, 28 Jul 2007, Junio C Hamano wrote:

> A reflog file is organized as one-line-per-entry records, and we
> enforced the file format integrity by chomping the given message
> at the first LF.  This changes it to convert them to SP, which
> is more in line with the --pretty=oneline format.

Would it not be better to chop off before the first "\n", and just append 
"..."?  IOW something like

-- snip --
 refs.c |   13 +++++++++----
 1 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/refs.c b/refs.c
index 2694e70..554436b 100644
--- a/refs.c
+++ b/refs.c
@@ -1043,7 +1043,7 @@ static int log_ref_write(const char *ref_name, const unsigned char *old_sha1,
 	unsigned maxlen, len;
 	int msglen;
 	char *log_file, *logrec;
-	const char *committer;
+	const char *committer, *postmsg;
 
 	if (log_all_ref_updates < 0)
 		log_all_ref_updates = !is_bare_repository();
@@ -1088,15 +1088,16 @@ static int log_ref_write(const char *ref_name, const unsigned char *old_sha1,
 				break;
 		if (*msg) {
 			const char *ep = strchr(msg, '\n');
-			if (ep)
+			if (ep) {
 				msglen = ep - msg;
-			else
+				postmsg = (ep[1] && !isspace(ep[1])) ? "..." : NULL;
+			} else
 				msglen = strlen(msg);
 		}
 	}
 
 	committer = git_committer_info(-1);
-	maxlen = strlen(committer) + msglen + 100;
+	maxlen = strlen(committer) + msglen + 100 + 3;
 	logrec = xmalloc(maxlen);
 	len = sprintf(logrec, "%s %s %s\n",
 		      sha1_to_hex(old_sha1),
@@ -1104,6 +1105,10 @@ static int log_ref_write(const char *ref_name, const unsigned char *old_sha1,
 		      committer);
 	if (msglen)
 		len += sprintf(logrec + len - 1, "\t%.*s\n", msglen, msg) - 1;
+	if (postmsg) {
+		len += strlen(postmsg);
+		strcat(logrec + len - 1, postmsg);
+	}
 	written = len <= maxlen ? write_in_full(logfd, logrec, len) : -1;
 	free(logrec);
 	if (close(logfd) != 0 || written != len)
-- snap --

It is not like the reflog messages have to be very verbose; they only have 
to give a hint what the commit was about, and the commit name is the 
important information.

What do you think?

Ciao,
Dscho

Re: [PATCH 1/3] log_ref_write() -- do not chomp reflog message at the first LF

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> It is not like the reflog messages have to be very verbose; they only have 
> to give a hint what the commit was about, and the commit name is the 
> important information.

I've considered it, but decided against it because:

 (1) I did not like the information lossage;

 (2) this is solely to help log messages coming from outside the
     recommended convention, and having excess will not hurt the
     normal usage;

 (3) it is not known if messages from outside the recommended
     convention have enough information on its first line of the
     first paragraph; the result of s/\n.*/.../ may not be
     sufficient "hint";

 (4) log output are by default piped to "less -S" so extra
     output gives the user chance to inspect more if he chosses
     to, without hurting the end user display; and

 (5) the code was simpler and less error prone to go wrong.

Re: [PATCH 1/3] log_ref_write() -- do not chomp reflog message at the first LF

[Johannes Schindelin]

Hi,

On Sun, 29 Jul 2007, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > It is not like the reflog messages have to be very verbose; they only have 
> > to give a hint what the commit was about, and the commit name is the 
> > important information.
> 
> I've considered it, but decided against it because:
> 
>  (1) I did not like the information lossage;
> 
>  (2) this is solely to help log messages coming from outside the
>      recommended convention, and having excess will not hurt the
>      normal usage;
> 
>  (3) it is not known if messages from outside the recommended
>      convention have enough information on its first line of the
>      first paragraph; the result of s/\n.*/.../ may not be
>      sufficient "hint";

Good catch.  Didn't think of that.

Ciao,
Dscho

[PATCH 2/3] symbolic-ref, update-ref: do not refuse reflog message with LF

[Junio C Hamano]

Earlier these tools refused to create a reflog entry when the
message given by the calling Porcelain had a LF in it, partially
to keep the file format integrity of reflog file, which is
one-entry-per-line.  These tools should not be dictating such a
policy.

Instead, let the codepath to write out the reflog entry worry
about the format integrity and allow messages with LF in them.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin-symbolic-ref.c |    2 --
 builtin-update-ref.c   |    2 --
 2 files changed, 0 insertions(+), 4 deletions(-)

diff --git a/builtin-symbolic-ref.c b/builtin-symbolic-ref.c
index d41b406..9eb95e5 100644
--- a/builtin-symbolic-ref.c
+++ b/builtin-symbolic-ref.c
@@ -43,8 +43,6 @@ int cmd_symbolic_ref(int argc, const char **argv, const char *prefix)
 			msg = argv[1];
 			if (!*msg)
 				die("Refusing to perform update with empty message");
-			if (strchr(msg, '\n'))
-				die("Refusing to perform update with \\n in message");
 		}
 		else if (!strcmp("--", arg)) {
 			argc--;
diff --git a/builtin-update-ref.c b/builtin-update-ref.c
index feac2ed..8339cf1 100644
--- a/builtin-update-ref.c
+++ b/builtin-update-ref.c
@@ -23,8 +23,6 @@ int cmd_update_ref(int argc, const char **argv, const char *prefix)
 			msg = argv[++i];
 			if (!*msg)
 				die("Refusing to perform update with empty message.");
-			if (strchr(msg, '\n'))
-				die("Refusing to perform update with \\n in message.");
 			continue;
 		}
 		if (!strcmp("-d", argv[i])) {
-- 
1.5.3.rc3.24.g83b3d

[PATCH 3/3] rebase: try not to munge commit log message

[Junio C Hamano]

This makes rebase/am keep the original commit log message
better, even when it does not conform to "single line paragraph
to say what it does, then explain and defend why it is a good
change in later paragraphs" convention.

This change is a two-edged sword.  While the earlier behaviour
would make such commit log messages more friendly to readers who
expect to get the birds-eye view with oneline summary formats,
users who primarily use git as a way to interact with foreign
SCM systems would not care much about the convenience of oneline
git log tools, but care more about preserving their own
convention.  This changes their commits less useful to readers
who read them with git tools while keeping them more consistent
with the foreign SCM systems they interact with.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin-mailinfo.c          |   29 +++++++++++++++++++++----
 t/t3405-rebase-malformed.sh |   48 +++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 72 insertions(+), 5 deletions(-)
 create mode 100755 t/t3405-rebase-malformed.sh

diff --git a/builtin-mailinfo.c b/builtin-mailinfo.c
index b4f6e91..b558754 100644
--- a/builtin-mailinfo.c
+++ b/builtin-mailinfo.c
@@ -237,8 +237,6 @@ static int eatspace(char *line)
 
 static char *cleanup_subject(char *subject)
 {
-	if (keep_subject)
-		return subject;
 	for (;;) {
 		char *p;
 		int len, remove;
@@ -425,6 +423,7 @@ static int read_one_header_line(char *line, int sz, FILE *in)
 			if (addlen >= sz - len)
 				addlen = sz - len - 1;
 			memcpy(line + len, continuation, addlen);
+			line[len] = '\n';
 			len += addlen;
 		}
 	}
@@ -846,6 +845,22 @@ static void handle_body(void)
 	return;
 }
 
+static void output_header_lines(FILE *fout, const char *hdr, char *data)
+{
+	while (1) {
+		char *ep = strchr(data, '\n');
+		int len;
+		if (!ep)
+			len = strlen(data);
+		else
+			len = ep - data;
+		fprintf(fout, "%s: %.*s\n", hdr, len, data);
+		if (!ep)
+			break;
+		data = ep + 1;
+	}
+}
+
 static void handle_info(void)
 {
 	char *sub;
@@ -863,9 +878,13 @@ static void handle_info(void)
 			continue;
 
 		if (!memcmp(header[i], "Subject", 7)) {
-			sub = cleanup_subject(hdr);
-			cleanup_space(sub);
-			fprintf(fout, "Subject: %s\n", sub);
+			if (keep_subject)
+				sub = hdr;
+			else {
+				sub = cleanup_subject(hdr);
+				cleanup_space(sub);
+			}
+			output_header_lines(fout, "Subject", sub);
 		} else if (!memcmp(header[i], "From", 4)) {
 			handle_from(hdr);
 			fprintf(fout, "Author: %s\n", name);
diff --git a/t/t3405-rebase-malformed.sh b/t/t3405-rebase-malformed.sh
new file mode 100755
index 0000000..e4e2e64
--- /dev/null
+++ b/t/t3405-rebase-malformed.sh
@@ -0,0 +1,48 @@
+#!/bin/sh
+
+test_description='rebase should not insist on git message convention'
+
+. ./test-lib.sh
+
+cat >F <<\EOF
+This is an example of a commit log message
+that does not  conform to git commit convention.
+
+It has two paragraphs, but its first paragraph is not friendly
+to oneline summary format.
+EOF
+
+test_expect_success setup '
+
+	>file1 &&
+	>file2 &&
+	git add file1 file2 &&
+	test_tick &&
+	git commit -m "Initial commit" &&
+
+	git checkout -b side &&
+	cat F >file2 &&
+	git add file2 &&
+	test_tick &&
+	git commit -F F &&
+
+	git cat-file commit HEAD | sed -e "1,/^\$/d" >F0 &&
+
+	git checkout master &&
+
+	echo One >file1 &&
+	test_tick &&
+	git add file1 &&
+	git commit -m "Second commit"
+'
+
+test_expect_success rebase '
+
+	git rebase master side &&
+	git cat-file commit HEAD | sed -e "1,/^\$/d" >F1 &&
+
+	diff -u F0 F1 &&
+	diff -u F F0
+'
+
+test_done
-- 
1.5.3.rc3.24.g83b3d

Re: git-svn rebase screwing up commit messages

[Jakub Narebski]

Junio C Hamano wrote:

> Sean <seanlkml@sympatico.ca> writes:
> 
>>> Having said all that, so that the readers understand the
>>> background, here is a not-so-heavily-tested patch, which might
>>> help.  It passes all the test suite as before, but that tells
>>> how existing git-svn tests do not test many things.
>>> 
>>> I am not considering this for inclusion right now, by the way.
>>
>> FWIW your patch fixed my test case here.
> 
> Actually the patched behaviour actively encourages a bad (not in
> the sense that those oneline tools will not work well, but in
> the sense that these messages are reader unfriendly) practice; I
> do not think what the patch did deserves to be called "fixed".
> 
> And that is one of the reasons, other than that we are in -rc
> freeze that we do not add anything but unarguable fixes, that I
> am not considering the patch for inclusion right now.

I think that git should not enforce this policy. Think import
and exchange with foreign SCMs which do not follow this, argueably
very reasonable, one-line summary policy.

-- 
Jakub Narebski
Warsaw, Poland
ShadeHawk on #git

Re: git-svn rebase screwing up commit messages

[Robin Rosenberg]

lördag 28 juli 2007 skrev Junio C Hamano:
> Sean <seanlkml@sympatico.ca> writes:
> 
> >> Having said all that, so that the readers understand the
> >> background, here is a not-so-heavily-tested patch, which might
> >> help.  It passes all the test suite as before, but that tells
> >> how existing git-svn tests do not test many things.
> >> 
> >> I am not considering this for inclusion right now, by the way.
> >
> > FWIW your patch fixed my test case here.
> 
> Actually the patched behaviour actively encourages a bad (not in
> the sense that those oneline tools will not work well, but in
> the sense that these messages are reader unfriendly) practice; I
> do not think what the patch did deserves to be called "fixed".

git-svn should not enforce git conventions when managing commits
intended for svn. Those commits should obviously follow the conventions
for the target (svn) repo. 

-- robin

Re: git-svn rebase screwing up commit messages

[Benoit SIGOURE]

[-- Attachment #1: Type: text/plain, Size: 1655 bytes --]

On Jul 28, 2007, at 3:10 PM, Junio C Hamano wrote:

> Sean <seanlkml@sympatico.ca> writes:
>
>>> Do you mean by "my commits in Git" a commit you created with git
>>> in your git repository?
>>
>> Tested this here (rc3.24.g83b3d) and can confirm the reported  
>> problem.
>> After making a commit in git and then running "git svn rebase" to
>> receive updates from the svn repo, the rebased commit has a borked
>> description (multi-lined commit message appears all on one line).
>
> In short, your original commit log message is broken.
>
> The recommended convention for commit messages is to start it
> with a single line that describes what it does, followed by a
> blank line (i.e. the first paragraph consists of a single line),
> followed by a longer explanation of why the change brought by
> the commit is a good thing.
>

Hi people,
thanks for the quick and complete replies.  As a user, I do not  
expect git-rebase to change my commit message.  I was aware of this  
convention in Git, but it looks like it's more than just a  
convention.  This trap should either be fixed (your patch works for  
me) or it should be clearly stated in the documentation that commit  
messages must really be formatted according to the convention.

Personally I expected Git to keep the 1st line of my commit message  
as the "short version" (which is what git log and the like do ATM)  
and although I don't leave a blank line between the 1st line and the  
rest of the message, the 1st line is always the sentence that can be  
used as a short version of the commit message.

Cheers,

-- 
Benoit Sigoure aka Tsuna
EPITA Research and Development Laboratory



[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 186 bytes --]