Serious bug with pretty format strings & empty bodies?

Serious bug with pretty format strings & empty bodies?

[Jonathan del Strother]

I'm struggling to come up with a minimal test case that confirms this,
but I seem to be hitting a bug in the pretty string formatter when
trying to print bodies of commits that don't have bodies.

For example, on a private repository, I'm trying to print the subject
& body of a commit.  This particular commit doesn't actually have a
body, so I'm expecting to see this output :

=================
commit 18d2480ab689b483ef1ebbdb3f7420904049ba0b
Try to flush log files before terminating the app

=================

However, when I actually run the command, I get gibberish in place of %b :
=================
$ git rev-list -1- --pretty=format:"%s%n%b"
18d2480ab689b483ef1ebbdb3f7420904049ba0b
commit 18d2480ab689b483ef1ebbdb3f7420904049ba0b
Try to flush log files before terminating the app
tree 57bc7cf30a10aee96251852125cf30fd2c81d7aa
parent 04c833865828538315fcdf6e187da077869ce444
author Jonathan del Strother <jon.delStrother@bestbefore.tv> 1197901755 +0000
committer Jonathan del Strother <jon.delStrother@bestbefore.tv> 1197901755 +0000

Check that ThreadWorker's work method actually returns a value with
method signatures
=================

So here you can see that the full details of a different commit is
appearing in place of the requested commit's (empty) body.  Rerunning
the rev-list produces the same output - the same string appears in
place of %b every time, but it's different for each commit.  Often you
only get a small fragment of another commit's message, rather than the
full commit as seen above.

When I use --pretty=full, it works fine.

This appears in quite a few places in my repository's history.
Repacking the repo semi-fixes it - I still get junk bodies, but not
quite so many.

It doesn't always happen when a commit message body is empty, but I've
never seen it happen where the message body has something in.

This happens with both git 1.5.3.7 and 1.5.4.rc0.1162.g3bfea, though
the output is slightly different - the former has a number of
<unknown> markers in it.  This is on OS X 10.5.1


Any suggestions?
Cheers,
Jon

Re: Serious bug with pretty format strings & empty bodies?

[René Scharfe]

Jonathan del Strother schrieb:
> I'm struggling to come up with a minimal test case that confirms this,
> but I seem to be hitting a bug in the pretty string formatter when
> trying to print bodies of commits that don't have bodies.
> 
> For example, on a private repository, I'm trying to print the subject
> & body of a commit.  This particular commit doesn't actually have a
> body, so I'm expecting to see this output :
> 
> =================
> commit 18d2480ab689b483ef1ebbdb3f7420904049ba0b
> Try to flush log files before terminating the app
> 
> =================
> 
> However, when I actually run the command, I get gibberish in place of %b :
> =================
> $ git rev-list -1- --pretty=format:"%s%n%b"
> 18d2480ab689b483ef1ebbdb3f7420904049ba0b
> commit 18d2480ab689b483ef1ebbdb3f7420904049ba0b
> Try to flush log files before terminating the app
> tree 57bc7cf30a10aee96251852125cf30fd2c81d7aa
> parent 04c833865828538315fcdf6e187da077869ce444
> author Jonathan del Strother <jon.delStrother@bestbefore.tv> 1197901755 +0000
> committer Jonathan del Strother <jon.delStrother@bestbefore.tv> 1197901755 +0000
> 
> Check that ThreadWorker's work method actually returns a value with
> method signatures
> =================
> 
> So here you can see that the full details of a different commit is
> appearing in place of the requested commit's (empty) body.  Rerunning
> the rev-list produces the same output - the same string appears in
> place of %b every time, but it's different for each commit.  Often you
> only get a small fragment of another commit's message, rather than the
> full commit as seen above.
> 
> When I use --pretty=full, it works fine.
> 
> This appears in quite a few places in my repository's history.
> Repacking the repo semi-fixes it - I still get junk bodies, but not
> quite so many.
> 
> It doesn't always happen when a commit message body is empty, but I've
> never seen it happen where the message body has something in.
> 
> This happens with both git 1.5.3.7 and 1.5.4.rc0.1162.g3bfea, though
> the output is slightly different - the former has a number of
> <unknown> markers in it.  This is on OS X 10.5.1

That's strange.  Could you check if this happens with e52a5de, too?
That's the commit that introduced --pretty=format.  If it doesn't, could
you then please try to bisect the bug?

How many '<unknown>' markers are there in the output of version 1.5.3.7
(and e52a5de)?  One per %b?  Are they the only output or are they
combined with parts of unrelated commits, too?

Thanks,
René

Re: Serious bug with pretty format strings & empty bodies?

[Jonathan del Strother]

On Dec 19, 2007 6:28 PM, René Scharfe <rene.scharfe@lsrfire.ath.cx> wrote:
> That's strange.  Could you check if this happens with e52a5de, too?
> That's the commit that introduced --pretty=format.  If it doesn't, could
> you then please try to bisect the bug?

As far as I can tell, e52a... never compiled - it's missing prefixcmp
from git-compat-util.h, which doesn't get merged in till 8ab3e18.
And yes, I still get junk output from that build.  (Shame, I was
looking forward to trying out the bisect tool...)


> How many '<unknown>' markers are there in the output of version 1.5.3.7
> (and e52a5de)?  One per %b?  Are they the only output or are they
> combined with parts of unrelated commits, too?

Only one per %b, and where they appear, they're the only output.
However, even with those builds, I get junk output in addition to the
<unknown> markers.
That is, with a format string of "**%b**", there are three possible outputs :
The correct one -  eg **Added test for NSDictionary generator**
The unknown one - eg  **<unknown>**
The complete junk one -  eg **n del
Strothereb0d794bacc1761e8a0681e9b144c514702d9b7a**

The latter's 'body' is almost always terminated by an unprintable
character, which always seems to be 03.


Hmmm.   I wonder if it's coincidence that I can only seem to spot this
problem in my repos that have been set up via submodule init...

Re: Serious bug with pretty format strings & empty bodies?

[Jonathan del Strother]

> Hmmm.   I wonder if it's coincidence that I can only seem to spot this
> problem in my repos that have been set up via submodule init...

Ignore that last bit, red herring.

Re: Serious bug with pretty format strings & empty bodies?

[Alex Riesen]

Jonathan del Strother, Wed, Dec 19, 2007 15:32:20 +0100:
> I'm struggling to come up with a minimal test case that confirms this,
> but I seem to be hitting a bug in the pretty string formatter when
> trying to print bodies of commits that don't have bodies.

I tried

    $ git init
    $ git rev-list --pretty=format:"%s%n%b" \
	$(echo -n "Test" |git commit-tree $(git write-tree))

but it works. It is possible that the commit in question is created
with an older commit-tree code. Could you try

    git cat-file 18d2480ab689b483ef1ebbdb3f7420904049ba0b

(or any other problematic commit) and post its output here?
Or is the repo publised somewhere?

Re: Serious bug with pretty format strings & empty bodies?

[Jonathan del Strother]

On Dec 19, 2007 6:44 PM, Alex Riesen <raa.lkml@gmail.com> wrote:
> Could you try
>
>     git cat-file 18d2480ab689b483ef1ebbdb3f7420904049ba0b
>
> (or any other problematic commit) and post its output here?

You mean git cat-file commit ... ?
I get the normal output, but the problematic commits don't show a
newline character at the end of the cat-file output.


> Or is the repo publised somewhere?

Afraid not - it's private code.


I'll try bisecting e52a5de tomorrow, see if that gets me anywhere.

Re: Serious bug with pretty format strings & empty bodies?

[René Scharfe]

Jonathan del Strother schrieb:
> On Dec 19, 2007 6:44 PM, Alex Riesen <raa.lkml@gmail.com> wrote:
>> Could you try
>>
>>     git cat-file 18d2480ab689b483ef1ebbdb3f7420904049ba0b
>>
>> (or any other problematic commit) and post its output here?
> 
> You mean git cat-file commit ... ?
> I get the normal output, but the problematic commits don't show a
> newline character at the end of the cat-file output.

Just a shot in the dark: does this patch on top of master make a difference?

diff --git a/pretty.c b/pretty.c
index 9db75b4..5f95a59 100644
--- a/pretty.c
+++ b/pretty.c
@@ -412,7 +412,7 @@ static void parse_commit_header(struct format_commit_context *context)
 		if (i == eol) {
 			state++;
 			/* strip empty lines */
-			while (msg[eol + 1] == '\n')
+			while (msg[eol] == '\n' && msg[eol + 1] == '\n')
 				eol++;
 		} else if (!prefixcmp(msg + i, "author ")) {
 			context->author.off = i + 7;

Re: Serious bug with pretty format strings & empty bodies?

[Jonathan del Strother]

On Dec 19, 2007 11:23 PM, René Scharfe <rene.scharfe@lsrfire.ath.cx> wrote:
> Jonathan del Strother schrieb:
> > On Dec 19, 2007 6:44 PM, Alex Riesen <raa.lkml@gmail.com> wrote:
> >> Could you try
> >>
> >>     git cat-file 18d2480ab689b483ef1ebbdb3f7420904049ba0b
> >>
> >> (or any other problematic commit) and post its output here?
> >
> > You mean git cat-file commit ... ?
> > I get the normal output, but the problematic commits don't show a
> > newline character at the end of the cat-file output.
>
> Just a shot in the dark: does this patch on top of master make a difference?
>
> diff --git a/pretty.c b/pretty.c
> index 9db75b4..5f95a59 100644
> --- a/pretty.c
> +++ b/pretty.c
> @@ -412,7 +412,7 @@ static void parse_commit_header(struct format_commit_context *context)
>                 if (i == eol) {
>                         state++;
>                         /* strip empty lines */
> -                       while (msg[eol + 1] == '\n')
> +                       while (msg[eol] == '\n' && msg[eol + 1] == '\n')
>                                 eol++;
>                 } else if (!prefixcmp(msg + i, "author ")) {
>                         context->author.off = i + 7;
>

No luck with that, I'm afraid.

Re: Serious bug with pretty format strings & empty bodies?

[René Scharfe]

Jonathan del Strother schrieb:
> On Dec 19, 2007 11:23 PM, René Scharfe <rene.scharfe@lsrfire.ath.cx> wrote:
>> Just a shot in the dark: does this patch on top of master make a difference?
> No luck with that, I'm afraid.

And how is this one?

The first chunk is the same as in the last try -- it guards against commit
messages ending with a NUL without a prior \n _and_ being followed by a \n
(in memory which shouldn't be accessed by us at all as it doesn't belong to
the commit message).  I guess that's quite rare.

The second chunk keeps the body offset from being incremented by the for
loop if we've already found a terminating NUL.

diff --git a/pretty.c b/pretty.c
index 9db75b4..5b1078b 100644
--- a/pretty.c
+++ b/pretty.c
@@ -412,7 +412,7 @@ static void parse_commit_header(struct format_commit_context *context)
 		if (i == eol) {
 			state++;
 			/* strip empty lines */
-			while (msg[eol + 1] == '\n')
+			while (msg[eol] == '\n' && msg[eol + 1] == '\n')
 				eol++;
 		} else if (!prefixcmp(msg + i, "author ")) {
 			context->author.off = i + 7;
@@ -425,6 +425,8 @@ static void parse_commit_header(struct format_commit_context *context)
 			context->encoding.len = eol - i - 9;
 		}
 		i = eol;
+		if (!msg[i])
+			break;
 	}
 	context->body_off = i;
 	context->commit_header_parsed = 1;

Re: Serious bug with pretty format strings & empty bodies?

[Johannes Schindelin]

Hi,

On Thu, 20 Dec 2007, Ren? Scharfe wrote:

> The second chunk keeps the body offset from being incremented by the for 
> loop if we've already found a terminating NUL.

Ah!  I missed that one *writethisdownintothebook*

Thanks,
Dscho

Re: Serious bug with pretty format strings & empty bodies?

[Jonathan del Strother]

On Dec 20, 2007 12:20 PM, René Scharfe <rene.scharfe@lsrfire.ath.cx> wrote:
> Jonathan del Strother schrieb:
> > On Dec 19, 2007 11:23 PM, René Scharfe <rene.scharfe@lsrfire.ath.cx> wrote:
> >> Just a shot in the dark: does this patch on top of master make a difference?
> > No luck with that, I'm afraid.
>
> And how is this one?
>
> The first chunk is the same as in the last try -- it guards against commit
> messages ending with a NUL without a prior \n _and_ being followed by a \n
> (in memory which shouldn't be accessed by us at all as it doesn't belong to
> the commit message).  I guess that's quite rare.
>
> The second chunk keeps the body offset from being incremented by the for
> loop if we've already found a terminating NUL.
>
> diff --git a/pretty.c b/pretty.c
> index 9db75b4..5b1078b 100644
> --- a/pretty.c
> +++ b/pretty.c
> @@ -412,7 +412,7 @@ static void parse_commit_header(struct format_commit_context *context)
>                 if (i == eol) {
>                         state++;
>                         /* strip empty lines */
> -                       while (msg[eol + 1] == '\n')
> +                       while (msg[eol] == '\n' && msg[eol + 1] == '\n')
>                                 eol++;
>                 } else if (!prefixcmp(msg + i, "author ")) {
>                         context->author.off = i + 7;
> @@ -425,6 +425,8 @@ static void parse_commit_header(struct format_commit_context *context)
>                         context->encoding.len = eol - i - 9;
>                 }
>                 i = eol;
> +               if (!msg[i])
> +                       break;
>         }
>         context->body_off = i;
>         context->commit_header_parsed = 1;
>

Winnar!

Yep, that seems to have fixed things.  Wonder how I managed to get the
null into the commit message...

Re: Serious bug with pretty format strings & empty bodies?

[Jonathan del Strother]

Has anyone actually managed to reproduce my problem?  I've got
multiple repos here that show the problem in several commits, made by
different people.  However, I can't actually come up with a way to
reproduce it at will...

I'd really like to see René's patch accepted as it seems to magically
fix all my problems, but it's a bit hard to justify if I'm the only
one that's seeing these broken commit messages.

Re: Serious bug with pretty format strings & empty bodies?

[Junio C Hamano]

"Jonathan del Strother" <maillist@steelskies.com> writes:

> Has anyone actually managed to reproduce my problem?  I've got
> multiple repos here that show the problem in several commits, made by
> different people.  However, I can't actually come up with a way to
> reproduce it at will...

Marking an issue as a "serious bug" without giving enough
material for reproduction nor diagnosis tends to discourage
people from looking into it seriously, as the issue cannot be
even judged if it is really serious.  For example:

  On Dec 19, 2007 6:44 PM, Alex Riesen <raa.lkml@gmail.com> wrote:
  > Could you try
  >
  >     git cat-file 18d2480ab689b483ef1ebbdb3f7420904049ba0b
  >
  > (or any other problematic commit) and post its output here?

  You mean git cat-file commit ... ?
  I get the normal output, but the problematic commits don't show a
  newline character at the end of the cat-file output.

"I get the normal output" is not what Alex asked you to supply,
nor would be sufficient information.  There may be some
abnormality in the commit object that you probably did not spot,
but Alex or other people may have been able to if you were
actually posted its output here.

I have been suspecting that there is a NUL in the middle of the
message somehow (I know the lowest-level plumbing commit-tree
allows any byte sequence in the log message if you worked hard
enough), which the parser is not prepared to cope with, but we
haven't seen enough evidence to support nor refute that theory.
We do not have much to work from.

One thing I noticed funny in your original message was "-1-".
Is it essential that the number is spelled incorrectly to
reproduce this problem?

  $ git rev-list -1- --pretty=format:"%s%n%b"
  18d2480ab689b483ef1ebbdb3f7420904049ba0b
  commit 18d2480ab689b483ef1ebbdb3f7420904049ba0b
  Try to flush log files before terminating the app
  tree 57bc7cf30a10aee96251852125cf30fd2c81d7aa
  parent 04c833865828538315fcdf6e187da077869ce444
  author Jonathan del Strother <jon.delStrother@bestbefore.tv> 1197901755 +0000
  committer Jonathan del Strother <jon.delStrother@bestbefore.tv> 1197901755 +0000

  Check that ThreadWorker's work method actually returns a value with
  method signatures

Re: Serious bug with pretty format strings & empty bodies?

[Jonathan del Strother]

On Dec 22, 2007 8:39 AM, Junio C Hamano <gitster@pobox.com> wrote:
>   On Dec 19, 2007 6:44 PM, Alex Riesen <raa.lkml@gmail.com> wrote:
>   > Could you try
>   >
>   >     git cat-file 18d2480ab689b483ef1ebbdb3f7420904049ba0b
>   >
>   > (or any other problematic commit) and post its output here?
>
>   You mean git cat-file commit ... ?
>   I get the normal output, but the problematic commits don't show a
>   newline character at the end of the cat-file output.
>
> "I get the normal output" is not what Alex asked you to supply,
> nor would be sufficient information.  There may be some
> abnormality in the commit object that you probably did not spot,
> but Alex or other people may have been able to if you were
> actually posted its output here.
========
$ git cat-file commit 18d2480ab689b483ef1ebbdb3f7420904049ba0b
tree 2416c620c6f7c1864065a6e778588b71b3e0bd5d
parent 9fc80d0a05835c68885f253844ab586b38d09202
author Jonathan del Strother <jon.delStrother@bestbefore.tv> 1197907511 +0000
committer Jonathan del Strother <jon.delStrother@bestbefore.tv> 1197907511 +0000

Try to flush log files before terminating the app%
========
The '%' suffix is zsh showing that there's no newline there.  Under
bash I get no '%' and the next prompt starts immediately following
'app'.

> One thing I noticed funny in your original message was "-1-".
> Is it essential that the number is spelled incorrectly to
> reproduce this problem?

No, sorry - I don't know how that got there.  I get identical output
whether it's "rev-list -1"  or "rev-list -1-"



I've been trying to reproduce this on my home repo, and have only been
partially successful.  I haven't been able to make git rev-list -1
show a malformed message body.  I can only get the problem to occur
when printing multiple commits (eg git rev-list -20 ...) Bizarrely,
the first 2 commits shown in rev-list never show a problem.  After the
first 2, commits with the broken message bodies will always appear
broken, regardless of their position in the list.  However, the
content that appears instead of the body will change depending on the
commits that appear prior to that one.

I realise this doesn't appear to mesh with my observations the other
day, where I was seeing the broken bodies even when just using
rev-list -1 - I'm at a loss to describe it.  Both machines are intel
macs (one a MacPro, one a MacBook Pro), running OS X 10.5.1, with the
same git version (1.5.4.rc0.1162.g3bfea).  Unfortunately I've switched
my work machine off for christmas, or I'd ssh in and try to re-verify
things.


I do appreciate people's help in trying to reproduce & fix this - I
realise I've not given people much to go on.

Re: Serious bug with pretty format strings & empty bodies?

[Jonathan del Strother]

I ended up trying to filter-branch my repository, see if I could come
up with a version stripped of all our private code, suitable for
making public.  Disappointingly, filter-branch magically fixes all the
commits, even when simply run as "git filter-branch HEAD".

However, looking at the rewritten repository vs the original, they
share a lot of commits, then diverge halfway through their history.  I
can't see anything notable about the commit where they diverge, with
the exception that the rewritten commit has a newline after the
subject and the original doesn't.  Neither has a commit message body.

Re: Serious bug with pretty format strings & empty bodies?

[Johannes Schindelin]

Hi,

On Thu, 20 Dec 2007, Jonathan del Strother wrote:

> I ended up trying to filter-branch my repository, see if I could come
> up with a version stripped of all our private code, suitable for
> making public.  Disappointingly, filter-branch magically fixes all the
> commits, even when simply run as "git filter-branch HEAD".
> 
> However, looking at the rewritten repository vs the original, they
> share a lot of commits, then diverge halfway through their history.  I
> can't see anything notable about the commit where they diverge, with
> the exception that the rewritten commit has a newline after the
> subject and the original doesn't.  Neither has a commit message body.

I do not see how Rene's patch could _not_ have fixed that problem.

As it is, I cannot even reproduce without that patch.  This is what I was 
trying:

-- snip --
diff --git a/t/t4025-pretty-format.sh b/t/t4025-pretty-format.sh
new file mode 100644
index 0000000..2826bcb
--- /dev/null
+++ b/t/t4025-pretty-format.sh
@@ -0,0 +1,23 @@
+#!/bin/sh
+
+test_description='git pretty formats'
+
+. ./test-lib.sh
+
+test_expect_success 'setup' '
+
+	: > file &&
+	git add file &&
+	tree=$(git write-tree) &&
+	commit=$(echo No newlin | tr "\\012" e | git commit-tree $tree) &&
+	git update-ref HEAD $commit
+
+'
+
+test_expect_success 'format:%b' '
+
+	test -z "$(git log -1 --pretty=format:%b)"
+
+'
+
+test_done
-- snap --

Seems I cannot help any more, but at least this is a starting point to 
try to reproduce.

Ciao,
Dscho