Re: [GIT PULL] sh updates for 2.6.25

Re: [GIT PULL] sh updates for 2.6.25

[Linus Torvalds]

On Wed, 16 Apr 2008, Paul Mundt wrote:
>
> Please pull from:
> 
> 	git://git.kernel.org/pub/scm/linux/kernel/git/lethal/sh-2.6.25.git

Paul, your git tree is odd. Not quite corrupt, but it doesn't really 
follow the rules either.

In particular, it has empty lines at the top of those commits, and I 
wonder how you created them. 

Doing things like "git log" will ignore the spurious empty lines, but they 
can be seen with things like "git cat-file", eg

	git cat-file commit fd785d6b18b930b76ad5076eed6e9af43195b281 

and I wonder if you used a buggy version of git, or whether you perhaps 
have some scripts that import these commits from the outside and uses some 
low-level commands that can generate these kinds of subtly bogus commits.

The reason I noticed is that it screws up the git merge summary, which 
will take the first line of each commit it merges (_without_ the "skip 
empty lines" logic) to generate the summary of the merge.

I think we should fix that git merge summary code to allow for this bad 
behaviour, but I also want to know why such corrupt commits exist in the 
first place. What toolchain do you use to create that commit? We should 
fix that too!

Junio? Something like this for the merge summary code? (It also turns an 
empty commit message with just whitespace in the commit message into the 
SHA1 hex string)

		Linus

----
 builtin-fmt-merge-msg.c |   10 +++++++++-
 1 files changed, 9 insertions(+), 1 deletions(-)

diff --git a/builtin-fmt-merge-msg.c b/builtin-fmt-merge-msg.c
index ebb3f37..7077d52 100644
--- a/builtin-fmt-merge-msg.c
+++ b/builtin-fmt-merge-msg.c
@@ -201,6 +201,15 @@ static void shortlog(const char *name, unsigned char *sha1,
 			continue;
 
 		bol = strstr(commit->buffer, "\n\n");
+		if (bol) {
+			unsigned char c;
+			do {
+				c = *++bol;
+			} while (isspace(c));
+			if (!c)
+				bol = NULL;
+		}
+
 		if (!bol) {
 			append_to_list(&subjects, xstrdup(sha1_to_hex(
 							commit->object.sha1)),
@@ -208,7 +217,6 @@ static void shortlog(const char *name, unsigned char *sha1,
 			continue;
 		}
 
-		bol += 2;
 		eol = strchr(bol, '\n');
 		if (eol) {
 			oneline = xmemdupz(bol, eol - bol);

Re: [GIT PULL] sh updates for 2.6.25

[Linus Torvalds]

On Tue, 15 Apr 2008, Linus Torvalds wrote:
> 
> Paul, your git tree is odd. Not quite corrupt, but it doesn't really 
> follow the rules either.

Anyway, I pulled the result (with my version of git that removes empty 
space at the beginning for the merge summary), and pushed it out. So it's 
merged, but I'd still like to know what tool actually created those extra 
spaces in the commit..

A regular "git commit" or "git am" should always strip whitespace. I 
assume it's something like stgit or other?

				Linus

Re: [GIT PULL] sh updates for 2.6.25

[Paul Mundt]

On Tue, Apr 15, 2008 at 11:01:36AM -0700, Linus Torvalds wrote:
> On Wed, 16 Apr 2008, Paul Mundt wrote:
> >
> > Please pull from:
> > 
> > 	git://git.kernel.org/pub/scm/linux/kernel/git/lethal/sh-2.6.25.git
> 
> Paul, your git tree is odd. Not quite corrupt, but it doesn't really 
> follow the rules either.
> 
> In particular, it has empty lines at the top of those commits, and I 
> wonder how you created them. 
> 
> Doing things like "git log" will ignore the spurious empty lines, but they 
> can be seen with things like "git cat-file", eg
> 
> 	git cat-file commit fd785d6b18b930b76ad5076eed6e9af43195b281 
> 
> and I wonder if you used a buggy version of git, or whether you perhaps 
> have some scripts that import these commits from the outside and uses some 
> low-level commands that can generate these kinds of subtly bogus commits.

It was a combination of mbox munging and git-am, I checked with git log
and thought things were ok, but I wasn't aware that it stripped out empty
lines. cat-file shows that it was just the 2 patches from Andrew that had
this particular problem. I had stripped out the subject and thought the
first line would be used for the merge summary, but it looks like git-am
simply wrote out an empty line and inserted one after that before the
rest of the summary.

I've pushed out updated patches that have this corrected, so please pull
again.

Re: [GIT PULL] sh updates for 2.6.25

[Linus Torvalds]

On Wed, 16 Apr 2008, Paul Mundt wrote:
> 
> It was a combination of mbox munging and git-am, I checked with git log
> and thought things were ok, but I wasn't aware that it stripped out empty
> lines. cat-file shows that it was just the 2 patches from Andrew that had
> this particular problem. I had stripped out the subject and thought the
> first line would be used for the merge summary, but it looks like git-am
> simply wrote out an empty line and inserted one after that before the
> rest of the summary.

Ahh, looks like a git-am buglet then. It will indeed turn an empty subject 
line into an empty first line.

We should run "git stripspace" on the whole thing, so maybe a patch 
something like the appended will help.

NOTE! Totally untested! Beware the patch!

> I've pushed out updated patches that have this corrected, so please pull
> again.

Well, since I pulled your previous one anyway, and since we should fix 
git for any fallout like this _anyway_, I didn't so much worry about this 
one-time event, as about avoiding this happening a lot in the future.

We've had other workflows generate empty lines in commits, so we already 
support stripping them out for other reasons.

			Linus
---
 git-am.sh |   23 +++++++++--------------
 1 files changed, 9 insertions(+), 14 deletions(-)

diff --git a/git-am.sh b/git-am.sh
index ac5c388..432d9fe 100755
--- a/git-am.sh
+++ b/git-am.sh
@@ -107,7 +107,7 @@ It does not apply to blobs recorded in its index."
     # patch did not touch, so recursive ends up canceling them,
     # saying that we reverted all those changes.
 
-    eval GITHEAD_$his_tree='"$SUBJECT"'
+    eval GITHEAD_$his_tree='"$FIRSTLINE"'
     export GITHEAD_$his_tree
     git-merge-recursive $orig_tree -- HEAD $his_tree || {
 	    git rerere
@@ -117,10 +117,6 @@ It does not apply to blobs recorded in its index."
     unset GITHEAD_$his_tree
 }
 
-reread_subject () {
-	git stripspace <"$1" | sed -e 1q
-}
-
 prec=4
 dotest=".dotest"
 sign= utf8=t keep= skip= interactive= resolved= binary= rebasing=
@@ -331,7 +327,11 @@ do
 			echo "Patch is empty.  Was it split wrong?"
 			stop_here $this
 		}
-		git stripspace < "$dotest/msg" > "$dotest/msg-clean"
+		SUBJECT="$(sed -n '/^Subject/ s/Subject: //p' "$dotest/info")"
+		case "$keep_subject" in -k)  SUBJECT="[PATCH] $SUBJECT" ;; esac
+
+		(echo "$SUBJECT" ; echo ; cat "$dotest/msg") |
+			git stripspace > "$dotest/msg-clean"
 		;;
 	esac
 
@@ -347,9 +347,6 @@ do
 
 	export GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL GIT_AUTHOR_DATE
 
-	SUBJECT="$(sed -n '/^Subject/ s/Subject: //p' "$dotest/info")"
-	case "$keep_subject" in -k)  SUBJECT="[PATCH] $SUBJECT" ;; esac
-
 	case "$resume" in
 	'')
 	    if test '' != "$SIGNOFF"
@@ -368,10 +365,8 @@ do
 		ADD_SIGNOFF=
 	    fi
 	    {
-		printf '%s\n' "$SUBJECT"
 		if test -s "$dotest/msg-clean"
 		then
-			echo
 			cat "$dotest/msg-clean"
 		fi
 		if test '' != "$ADD_SIGNOFF"
@@ -388,6 +383,7 @@ do
 			;;
 		esac
 	esac
+	FIRSTLINE=$(head -1 "$dotest/final-commit")
 
 	resume=
 	if test "$interactive" = t
@@ -408,7 +404,6 @@ do
 		[aA]*) action=yes interactive= ;;
 		[nN]*) action=skip ;;
 		[eE]*) git_editor "$dotest/final-commit"
-		       SUBJECT=$(reread_subject "$dotest/final-commit")
 		       action=again ;;
 		[vV]*) action=again
 		       LESS=-S ${PAGER:-less} "$dotest/patch" ;;
@@ -431,7 +426,7 @@ do
 		stop_here $this
 	fi
 
-	printf 'Applying %s\n' "$SUBJECT"
+	printf 'Applying %s\n' "$FIRSTLINE"
 
 	case "$resolved" in
 	'')
@@ -489,7 +484,7 @@ do
 	tree=$(git write-tree) &&
 	parent=$(git rev-parse --verify HEAD) &&
 	commit=$(git commit-tree $tree -p $parent <"$dotest/final-commit") &&
-	git update-ref -m "$GIT_REFLOG_ACTION: $SUBJECT" HEAD $commit $parent ||
+	git update-ref -m "$GIT_REFLOG_ACTION: $FIRSTLINE" HEAD $commit $parent ||
 	stop_here $this
 
 	if test -x "$GIT_DIR"/hooks/post-applypatch

Re: [GIT PULL] sh updates for 2.6.25

[Alex Riesen]

Linus Torvalds, Tue, Apr 15, 2008 21:56:50 +0200:
> 
> 
> On Wed, 16 Apr 2008, Paul Mundt wrote:
> > 
> > It was a combination of mbox munging and git-am, I checked with git log
> > and thought things were ok, but I wasn't aware that it stripped out empty
> > lines. cat-file shows that it was just the 2 patches from Andrew that had
> > this particular problem. I had stripped out the subject and thought the
> > first line would be used for the merge summary, but it looks like git-am
> > simply wrote out an empty line and inserted one after that before the
> > rest of the summary.
> 
> Ahh, looks like a git-am buglet then. It will indeed turn an empty subject 
> line into an empty first line.
> 
> We should run "git stripspace" on the whole thing, so maybe a patch 
> something like the appended will help.
> 
> NOTE! Totally untested! Beware the patch!
> 

t4014-format-patch.sh broke. It probably has to be updated

Re: [GIT PULL] sh updates for 2.6.25

[Junio C Hamano]

Linus Torvalds <torvalds@linux-foundation.org> writes:

> Ahh, looks like a git-am buglet then. It will indeed turn an empty subject 
> line into an empty first line.
>
> We should run "git stripspace" on the whole thing, so maybe a patch 
> something like the appended will help.
>
> NOTE! Totally untested! Beware the patch!

The basic idea is very sound and as usual your patch deletes more lines
than it adds, which always impresses me.  I wish all our patches are like
this.

> @@ -388,6 +383,7 @@ do
>  			;;
>  		esac
>  	esac
> +	FIRSTLINE=$(head -1 "$dotest/final-commit")
>  
>  	resume=
>  	if test "$interactive" = t
> @@ -408,7 +404,6 @@ do
>  		[aA]*) action=yes interactive= ;;
>  		[nN]*) action=skip ;;
>  		[eE]*) git_editor "$dotest/final-commit"
> -		       SUBJECT=$(reread_subject "$dotest/final-commit")

This needs to be replaced with re-assignment to FIRSTLINE, as the user may
have fixed the title in the editor; otherwise...

> @@ -431,7 +426,7 @@ do
>  		stop_here $this
>  	fi
>  
> -	printf 'Applying %s\n' "$SUBJECT"
> +	printf 'Applying %s\n' "$FIRSTLINE"

... this would surprise the user who expects us to give the report with
the updated title.

Re: [GIT PULL] sh updates for 2.6.25

[Linus Torvalds]

On Wed, 16 Apr 2008, Junio C Hamano wrote:
> 
> > @@ -388,6 +383,7 @@ do
> >  			;;
> >  		esac
> >  	esac
> > +	FIRSTLINE=$(head -1 "$dotest/final-commit")
> >  
> >  	resume=
> >  	if test "$interactive" = t
> > @@ -408,7 +404,6 @@ do
> >  		[aA]*) action=yes interactive= ;;
> >  		[nN]*) action=skip ;;
> >  		[eE]*) git_editor "$dotest/final-commit"
> > -		       SUBJECT=$(reread_subject "$dotest/final-commit")
> 
> This needs to be replaced with re-assignment to FIRSTLINE, as the user may
> have fixed the title in the editor; otherwise...

Hmm. I think we could just have moved the assignment of FIRSTLINE it down, 
and had it in just one place. I see you already fixed it up, but maybe a 
patch like this is still a worthy cleanup.

That said - I didn't check that there isn't some subtle intermediate user 
or a break out of the loop or something. So while this patch _looks_ 
obvious and passes the tests, I'm not going to guarantee that there isn't 
some special case. I doubt any of the tests really check things like the 
reflog comments after git-am etc..

		Linus

---
 git-am.sh |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/git-am.sh b/git-am.sh
index 245e1db..9a865cc 100755
--- a/git-am.sh
+++ b/git-am.sh
@@ -383,7 +383,6 @@ do
 			;;
 		esac
 	esac
-	FIRSTLINE=$(head -1 "$dotest/final-commit")
 
 	resume=
 	if test "$interactive" = t
@@ -404,7 +403,6 @@ do
 		[aA]*) action=yes interactive= ;;
 		[nN]*) action=skip ;;
 		[eE]*) git_editor "$dotest/final-commit"
-		       FIRSTLINE=$(head -1 "$dotest/final-commit")
 		       action=again ;;
 		[vV]*) action=again
 		       LESS=-S ${PAGER:-less} "$dotest/patch" ;;
@@ -427,6 +425,7 @@ do
 		stop_here $this
 	fi
 
+	FIRSTLINE=$(head -1 "$dotest/final-commit")
 	printf 'Applying %s\n' "$FIRSTLINE"
 
 	case "$resolved" in

Re: [GIT PULL] sh updates for 2.6.25

[Junio C Hamano]

Linus Torvalds <torvalds@linux-foundation.org> writes:

> Junio? Something like this for the merge summary code? (It also turns an 
> empty commit message with just whitespace in the commit message into the 
> SHA1 hex string)

Yeah, your patch makes sense, but it also makes me wonder if we should fix
the code in pretty.c::parse_commit_header() that grabs the "subject" line,
and use format_commit_message() with "%s" format here.  Interested people
then can enhance it to take custom format string.

> 		Linus
>
> ----
>  builtin-fmt-merge-msg.c |   10 +++++++++-
>  1 files changed, 9 insertions(+), 1 deletions(-)
>
> diff --git a/builtin-fmt-merge-msg.c b/builtin-fmt-merge-msg.c
> index ebb3f37..7077d52 100644
> --- a/builtin-fmt-merge-msg.c
> +++ b/builtin-fmt-merge-msg.c
> @@ -201,6 +201,15 @@ static void shortlog(const char *name, unsigned char *sha1,
>  			continue;
>  
>  		bol = strstr(commit->buffer, "\n\n");
> +		if (bol) {
> +			unsigned char c;
> +			do {
> +				c = *++bol;
> +			} while (isspace(c));
> +			if (!c)
> +				bol = NULL;
> +		}
> +
>  		if (!bol) {
>  			append_to_list(&subjects, xstrdup(sha1_to_hex(
>  							commit->object.sha1)),
> @@ -208,7 +217,6 @@ static void shortlog(const char *name, unsigned char *sha1,
>  			continue;
>  		}
>  
> -		bol += 2;
>  		eol = strchr(bol, '\n');
>  		if (eol) {
>  			oneline = xmemdupz(bol, eol - bol);

Re: [GIT PULL] sh updates for 2.6.25

[Jakub Narebski]

Linus Torvalds <torvalds@linux-foundation.org> writes:

> Paul, your git tree is odd. Not quite corrupt, but it doesn't really 
> follow the rules either.
> 
> In particular, it has empty lines at the top of those commits, and I 
> wonder how you created them. 

> The reason I noticed is that it screws up the git merge summary, which 
> will take the first line of each commit it merges (_without_ the "skip 
> empty lines" logic) to generate the summary of the merge.
> 
> I think we should fix that git merge summary code to allow for this bad 
> behaviour, but I also want to know why such corrupt commits exist in the 
> first place. What toolchain do you use to create that commit? We should 
> fix that too!

I seem to remember (but I might be mistaken) that this issue was
corrected by some patch on git mailing list already...

-- 
Jakub Narebski
Poland
ShadeHawk on #git

Re: [GIT PULL] sh updates for 2.6.25

[Miklos Vajna]

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

On Tue, Apr 15, 2008 at 11:43:24AM -0700, Jakub Narebski <jnareb@gmail.com> wrote:
> I seem to remember (but I might be mistaken) that this issue was
> corrected by some patch on git mailing list already...

If we are at it, I had a similar bugreport: If one doesn't use an empty
line after the first line in the commit message, a git-format-patch +
git-am combo will strip newlines from the commit message:

http://article.gmane.org/gmane.comp.version-control.git/73755

There, you suggested to modify git-format-patch, but I haven't come up
with such a patch nor anybody else.

Actually I recently tried to make one but I got lost in pretty.c and
log-tree.c. :-)

What I would like to do is just to change the current:

----
line1
 line2
 line3
----

output to:

----
line1

line2
line3
----

[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]

[PATCH] format-patch: Make sure the subject is always a one-liner

[Miklos Vajna]

If the commit message has no empty line after the first line, we need to
insert a newline after the first, so that the newlines won't be removed
from the commit message for example when they are applied using git-am.

Signed-off-by: Miklos Vajna <vmiklos@frugalware.org>
---

On Wed, Apr 16, 2008 at 02:37:25AM +0200, Miklos Vajna <vmiklos@frugalware.org> wrote:
> If we are at it, I had a similar bugreport: If one doesn't use an
> empty
> line after the first line in the commit message, a git-format-patch +
> git-am combo will strip newlines from the commit message:
>
> http://article.gmane.org/gmane.comp.version-control.git/73755
>
> There, you suggested to modify git-format-patch, but I haven't come up
> with such a patch nor anybody else.
>
> Actually I recently tried to make one but I got lost in pretty.c and
> log-tree.c. :-)

Ok, here is a try. It does the trick for me, but this it the first time
I touch pretty.c so feel free to point out if I did something wrong ;-)

Thanks.

 pretty.c |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/pretty.c b/pretty.c
index 6c04176..45a5679 100644
--- a/pretty.c
+++ b/pretty.c
@@ -653,6 +653,7 @@ void pp_title_line(enum cmit_fmt fmt,
 
 	strbuf_init(&title, 80);
 
+	int check_empty = 1;
 	for (;;) {
 		const char *line = *msg_p;
 		int linelen = get_one_line(line);
@@ -666,7 +667,10 @@ void pp_title_line(enum cmit_fmt fmt,
 			if (fmt == CMIT_FMT_EMAIL) {
 				strbuf_addch(&title, '\n');
 			}
-			strbuf_addch(&title, ' ');
+			if (check_empty && strcmp(line, "\n")) {
+				check_empty = 0;
+				strbuf_addch(&title, '\n');
+			}
 		}
 		strbuf_add(&title, line, linelen);
 	}
-- 
1.5.5

Re: [GIT PULL] sh updates for 2.6.25

[Junio C Hamano]

Miklos Vajna <vmiklos@frugalware.org> writes:

> On Tue, Apr 15, 2008 at 11:43:24AM -0700, Jakub Narebski <jnareb@gmail.com> wrote:
>> I seem to remember (but I might be mistaken) that this issue was
>> corrected by some patch on git mailing list already...
>
> If we are at it, I had a similar bugreport: If one doesn't use an empty
> line after the first line in the commit message, a git-format-patch +
> git-am combo will strip newlines from the commit message:

That's not a bug but an intended behaviour.  You are triggering "RFC 2822
line folding" of Subject: header.

Re: [GIT PULL] sh updates for 2.6.25

[Miklos Vajna]

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

On Tue, Apr 15, 2008 at 08:25:28PM -0700, Junio C Hamano <gitster@pobox.com> wrote:
> > If we are at it, I had a similar bugreport: If one doesn't use an empty
> > line after the first line in the commit message, a git-format-patch +
> > git-am combo will strip newlines from the commit message:
> 
> That's not a bug but an intended behaviour.  You are triggering "RFC 2822
> line folding" of Subject: header.

Hm, then is it git-am that would have to be fixed up to properly unfold
such a subject? (I mean not stripping newlines.)

[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]

Re* [GIT PULL] sh updates for 2.6.25

[Junio C Hamano]

Miklos Vajna <vmiklos@frugalware.org> writes:

> On Tue, Apr 15, 2008 at 08:25:28PM -0700, Junio C Hamano <gitster@pobox.com> wrote:
>> > If we are at it, I had a similar bugreport: If one doesn't use an empty
>> > line after the first line in the commit message, a git-format-patch +
>> > git-am combo will strip newlines from the commit message:
>> 
>> That's not a bug but an intended behaviour.  You are triggering "RFC 2822
>> line folding" of Subject: header.
>
> Hm, then is it git-am that would have to be fixed up to properly unfold
> such a subject? (I mean not stripping newlines.)

"Properly unfolding" by definition means remove LF before WSP at the
beginning of the continued lines, so it does not solve it.  "git am" is
primarily an email acceptance tool, and the mail pathway between the
sender and the recipient can splice the "Subject: " at any point, so the
receiver cannot assume that the lines were folded at the place the
originator intended them to be folded (often, the sender wanted a single
line).

But let's step back a bit.

First of all, if you are using git as "fast filesystem that lets you build
your own SCM on top of", aka "plumbing", you can have your commit log
message in any way you want.  "git-commit-tree" allows you to record
arbitrary commit log message without whitespace cleansing (it might eat
NUL, but that would be a bug if it did so), and "git-cat-file commit"
would give you what you recorded literally.

However, if you are using git as a full featured SCM, aka "Porcelain", you
have to work within the rules of how the world works, which were not set
arbitrarily but came from real world constraints.  "git-format-patch" and
"git-am" are two examples of such Porcelain programs.

Unlike olden days when people used CVS and recorded a two-week's worth of
work as a single huge commit, we encourage working with many small
commits, and we have tools to give you overview of the history without
drowning you in the sea of information.  "git reflog", "git branch -v",
"git-log --pretty=oneline", "git show-branch", "git fmt-merge-msg",
"gitk", and "git shortlog" all are built around the notion that the first
line of the commit is _special_ in that by reading only that line, there
should be enough information for the reader to tell what the commit is
about.  Also, emailed patch has the "Subject: " line to serve the same
purpose and by definition that is a single liner.

When one adopts the notion of "a single line at the top summarizes what
the commit is about", it is very natural to call that a "title", and
having a blank line between the title and the body to separate them also
becomes natural, and it matches how a patch is presented in email, as a
bonus, so it matches people's expectation.

So this format is merely a convention when viewed at the "plumbing" level,
but it is more important than just a convention if you are living at the
"Porcelain" level; if you deviate from that, "Porcelain" would not work
very well for you.

We can do two things about "would not work very well" part above when you
do have a commit whose first paragraph has more then one lines, and
dealing with such a commit gracefully is important.

People who are used to other systems without a good history summarization
tools can and do write such log messages.  People who make commits on such
systems whose commits are imported to git (perhaps even without them
knowing about it) do not have an incentive to use a short-and-clear single
line summary in each of their commits, as their system may not give a good
way to make use of the result of such a practice.

Very old git literally took "the first line is the summary" approach,
which meant that the first line of such a multi-line paragraph at the
beginning became "Subject: ", and the message body started in the mid
sentence.  "git log --pretty=oneline" appeared to chomp a sentence in the
middle (while in fact the guilty party who chomped the sentence in the
middle was the committer). People who migrated from CVS hated the loss of
information. Worse yet, because "rebase" is implemented in terms of
"format-patch piped to am" (which we can eventually change to use
git-sequencer), if you rebase such a commit, you will get an extra empty
line between the first line and the subsequent lines.

These days, format-patch was taught to use "the first paragraph" as the
summarizing first line to avoid chomping a sentence in the middle.  This
change did not hurt people who use git "Porcelain", as the commit log
message for them is always "a single line summary, a blank line, and the
body".  The first paragraph is the same as the first line for them.  But
for commits that have a multi-line paragraph at the beginning, information
lossage is avoided this way.  Now the first chunk of the message, even if
it is splattered over two physical lines, is used as the summary.

The tools that allocate only one display line for each commit (again,
format-patch is one of them because there is only one "Subject: " line)
still need to cope with this, as they have only one line to work with.
The way format-patch and friends do so is to take it as a logically single
line folded into multiple lines.  format-patch (and --pretty=email)
happens to know that the output medium (i.e. rfc 2822 messages) allows to
express the logically single line as multiple physical lines, so its
output "preserves" the original line breaks, but "git am" is in no
position to honor it, at least without an extra option to tell it that it
is safe and meaningful to do so.

So in short, when you use "am", it by design unfolds the "Subject: " line
and there is no bug there.  "rebase" being implemented in terms of
"format-patch piped to am" does mangle the message because of this, but
if anything that is a bug in rebase, and not "am".

And this is a potential fix to the issue, which was made possible only
because recently "rebase" started passing an extra option to "am".

-- >8 --
From: Junio C Hamano <gitster@pobox.com>
Date: Wed, 16 Apr 2008 12:50:48 -0700
Subject: [PATCH] rebase: do not munge commit log message

Traditionally git-rebase was implemented in terms of "format-patch" piped
to "am -3", to strike balance between speed (because it avoids a rather
expensive read-tree/merge-recursive machinery most of the time) and
flexibility (the magic "-3" allows it to fall back to 3-way merge as
necessary).  However, this combination has one flaw when dealing with a
nonstandard commit log message format that has more than one lines in the
first paragraph, because such a "first line" is formatted as logically a
single line, and unfolded at the applying end.

This teaches "git am --rebasing" to take advantage of the fact that the
mbox message "git rebase" prepares for it records the original commit
object name, and that such a commit _is_ available locally.  It reads the
log message from the original commit object instead.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 git-am.sh                    |   19 ++++++++++++++-----
 t/t3408-rebase-multi-line.sh |   41 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 55 insertions(+), 5 deletions(-)
 create mode 100755 t/t3408-rebase-multi-line.sh

diff --git a/git-am.sh b/git-am.sh
index 245e1db..5a7695e 100755
--- a/git-am.sh
+++ b/git-am.sh
@@ -327,11 +327,20 @@ do
 			echo "Patch is empty.  Was it split wrong?"
 			stop_here $this
 		}
-		SUBJECT="$(sed -n '/^Subject/ s/Subject: //p' "$dotest/info")"
-		case "$keep_subject" in -k)  SUBJECT="[PATCH] $SUBJECT" ;; esac
-
-		(echo "$SUBJECT" ; echo ; cat "$dotest/msg") |
-			git stripspace > "$dotest/msg-clean"
+		if test -f "$dotest/rebasing" &&
+			commit=$(sed -e 's/^From \([0-9a-f]*\) .*/\1/' \
+				-e q "$dotest/$msgnum") &&
+			test "$(git cat-file -t "$commit")" = commit
+		then
+			git cat-file commit "$commit" |
+			sed -e '1,/^$/d' >"$dotest/msg-clean"
+		else
+			SUBJECT="$(sed -n '/^Subject/ s/Subject: //p' "$dotest/info")"
+			case "$keep_subject" in -k)  SUBJECT="[PATCH] $SUBJECT" ;; esac
+
+			(echo "$SUBJECT" ; echo ; cat "$dotest/msg") |
+				git stripspace > "$dotest/msg-clean"
+		fi
 		;;
 	esac
 
diff --git a/t/t3408-rebase-multi-line.sh b/t/t3408-rebase-multi-line.sh
new file mode 100755
index 0000000..e12cd57
--- /dev/null
+++ b/t/t3408-rebase-multi-line.sh
@@ -0,0 +1,41 @@
+#!/bin/sh
+
+test_description='rebasing a commit with multi-line first paragraph.'
+
+. ./test-lib.sh
+
+test_expect_success setup '
+
+	>file &&
+	git add file &&
+	test_tick &&
+	git commit -m initial &&
+
+	echo hello >file &&
+	test_tick &&
+	git commit -a -m "A sample commit log message that has a long
+summary that spills over multiple lines.
+
+But otherwise with a sane description."
+
+	git branch side &&
+
+	git reset --hard HEAD^ &&
+	>elif &&
+	git add elif &&
+	test_tick &&
+	git commit -m second
+
+'
+
+test_expect_success rebase '
+
+	git checkout side &&
+	git rebase master &&
+	git cat-file commit HEAD | sed -e "1,/^$/d" >actual &&
+	git cat-file commit side@{1} | sed -e "1,/^$/d" >expect &&
+	test_cmp expect actual
+
+'
+
+test_done
-- 
1.5.5.120.gea9a0

Re: Re* [GIT PULL] sh updates for 2.6.25

[Jakub Narebski]

Junio C Hamano wrote:
[cut]

> So in short, when you use "am", it by design unfolds the "Subject: " line
> and there is no bug there.  "rebase" being implemented in terms of
> "format-patch piped to am" does mangle the message because of this, but
> if anything that is a bug in rebase, and not "am".
> 
> And this is a potential fix to the issue, which was made possible only
> because recently "rebase" started passing an extra option to "am".
> 
> -- >8 --
> From: Junio C Hamano <gitster@pobox.com>
> Date: Wed, 16 Apr 2008 12:50:48 -0700
> Subject: [PATCH] rebase: do not munge commit log message
> 
> Traditionally git-rebase was implemented in terms of "format-patch" piped
> to "am -3", to strike balance between speed (because it avoids a rather
> expensive read-tree/merge-recursive machinery most of the time) and
> flexibility (the magic "-3" allows it to fall back to 3-way merge as
> necessary).  However, this combination has one flaw when dealing with a
> nonstandard commit log message format that has more than one lines in the
> first paragraph, because such a "first line" is formatted as logically a
> single line, and unfolded at the applying end.
> 
> This teaches "git am --rebasing" to take advantage of the fact that the
> mbox message "git rebase" prepares for it records the original commit
> object name, and that such a commit _is_ available locally.  It reads the
> log message from the original commit object instead.

IIRC there was alternate patch which made git-format-patch to add extra
email header meant for git-am to "obey the (encoded) commit message
formatting."

But this solution is simpler, and I think better.

-- 
Jakub Narebski
Poland

Re: Re* [GIT PULL] sh updates for 2.6.25

[Miklos Vajna]

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

On Wed, Apr 16, 2008 at 12:58:54PM -0700, Junio C Hamano <gitster@pobox.com> wrote:
> When one adopts the notion of "a single line at the top summarizes what
> the commit is about", it is very natural to call that a "title", and
> having a blank line between the title and the body to separate them also
> becomes natural, and it matches how a patch is presented in email, as a
> bonus, so it matches people's expectation.
> 
> So this format is merely a convention when viewed at the "plumbing" level,
> but it is more important than just a convention if you are living at the
> "Porcelain" level; if you deviate from that, "Porcelain" would not work
> very well for you.

I understand this, my only problem is that some project does not use an
empty line after the "title". Consider a commit message like:

----
Change foo to bar
- this patch changes foo to bar becase of baz
- ok devel1@, devel2@
----

Given that a project uses such a commit message style, the newlines are
removed when applying with git am, but the commit still has a title.

> People who are used to other systems without a good history summarization
> tools can and do write such log messages.  People who make commits on such
> systems whose commits are imported to git (perhaps even without them
> knowing about it) do not have an incentive to use a short-and-clear single
> line summary in each of their commits, as their system may not give a good
> way to make use of the result of such a practice.

That makes sense, but those commits are unlikely transferred using
format-patch+am. :)

> These days, format-patch was taught to use "the first paragraph" as the
> summarizing first line to avoid chomping a sentence in the middle.  This
> change did not hurt people who use git "Porcelain", as the commit log
> message for them is always "a single line summary, a blank line, and the
> body".  The first paragraph is the same as the first line for them.  But
> for commits that have a multi-line paragraph at the beginning, information
> lossage is avoided this way.  Now the first chunk of the message, even if
> it is splattered over two physical lines, is used as the summary.

I see. If I'm right, then basically the old behaviour is what I want. At
least after a

git reset --hard 4234a76167b12a7669dae0e6386c62e712b9dcf5^

I get the behaviour I wished. :)

(Well, almost. It inserts a newline after the first line but that's far
better than stripping all the newlines.)

Would you accept a patch that would make this configurable?

> So in short, when you use "am", it by design unfolds the "Subject: " line
> and there is no bug there.  "rebase" being implemented in terms of
> "format-patch piped to am" does mangle the message because of this, but
> if anything that is a bug in rebase, and not "am".

Yes, that's an other issue.

Thanks.

[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]

Re: [GIT PULL] sh updates for 2.6.25

[David Woodhouse]

On Tue, 2008-04-15 at 11:01 -0700, Linus Torvalds wrote:
> Paul, your git tree is odd. Not quite corrupt, but it doesn't really 
> follow the rules either.
> 
> In particular, it has empty lines at the top of those commits, and I 
> wonder how you created them. 

Hm, I noticed those go past on the commits list and meant to
investigate, but got distracted before I got round to it. Should I
assume there's nothing to fix in the script which feeds the list, then?

$todo--; :)

-- 
dwmw2