git 1.5.1-rc1 doesn't like empty files

git 1.5.1-rc1 doesn't like empty files

[Pavel Roskin]

Hello!

I don't know where this problem appeared, but it present in the current
git (1.5.1-rc1).  Empty files become invalid objects in the repository:

$ touch file 
$ git-init
Initialized empty Git repository in .git/
$ git-add file
$ git-commit -m "first commit"
Created initial commit 16a476808d3cb0a4758997ba58193a9dcfad0fd8
error: garbage at end of loose object
'e69de29bb2d1d6434b8b29ae775ad8c2e48c5391'
 0 files changed, 0 insertions(+), 0 deletions(-)
 create mode 100644 file
$

A file with a one newline is OK (replace "touch file with "echo >file"
and the error will go away).

This is Linux, Fedora Development, i386.

The testsuite fails at test 7 in t9200-git-cvsexportcommit.sh, but it
seems to be unrelated.

-- 
Regards,
Pavel Roskin

Re: git 1.5.1-rc1 doesn't like empty files

[Nicolas Vilz]

On Mon, Mar 19, 2007 at 11:30:24PM -0400, Pavel Roskin wrote:
> Hello!
> 
> I don't know where this problem appeared, but it present in the current
> git (1.5.1-rc1).  Empty files become invalid objects in the repository:
> 
> $ touch file 
> $ git-init
> Initialized empty Git repository in .git/
> $ git-add file
> $ git-commit -m "first commit"
> Created initial commit 16a476808d3cb0a4758997ba58193a9dcfad0fd8
> error: garbage at end of loose object
> 'e69de29bb2d1d6434b8b29ae775ad8c2e48c5391'
>  0 files changed, 0 insertions(+), 0 deletions(-)
>  create mode 100644 file
> $

I get the following after the last command:

Created initial commit a0e02efbe85ee1c1f4b8ba703ccae2f4e64e6ed0
 0 files changed, 0 insertions(+), 0 deletions(-)
 create mode 100644 file

I use git version 1.5.1.rc1.595.gd1206 on a current gentoo system
(Portage 2.1.2.2 (default-linux/amd64/2006.1/desktop, gcc-4.1.2,
glibc-2.5-r1, 2.6.20-gentoo-r3 x86_64)) with a not so current portage 
tree (Timestamp of tree: Sat, 17 Mar 2007 12:00:01 +0000).

in a few hours, i can test that on ppc32 arch, too, if you like.

Regards,
Nicolas Vilz

Re: git 1.5.1-rc1 doesn't like empty files

[Linus Torvalds]

On Mon, 19 Mar 2007, Pavel Roskin wrote:
> 
> I don't know where this problem appeared, but it present in the current
> git (1.5.1-rc1).  Empty files become invalid objects in the repository:

Hmm.. Not for me. Can you bisect when it started happening?

> This is Linux, Fedora Development, i386.

FC6, tested both i386 and x86-64.

I wonder if this is a zlib issue. Although I seem to have libz-1.2.3 on 
both machines, and that should be the most recent version. I don't see 
Fedora development having anything else..

		Linus

Re: git 1.5.1-rc1 doesn't like empty files

[Pavel Roskin]

On Mon, 2007-03-19 at 21:54 -0700, Linus Torvalds wrote:
> 
> On Mon, 19 Mar 2007, Pavel Roskin wrote:
> > 
> > I don't know where this problem appeared, but it present in the current
> > git (1.5.1-rc1).  Empty files become invalid objects in the repository:
> 
> Hmm.. Not for me. Can you bisect when it started happening?

I was doing exactly that, and here's the result:

7efbff7531af4281487d54c1dc1401308d988e33 is first bad commit
commit 7efbff7531af4281487d54c1dc1401308d988e33
Author: Junio C Hamano <junkio@cox.net>
Date:   Mon Mar 5 00:21:37 2007 -0800

    unpack_sha1_file(): detect corrupt loose object files.
    
    We did not detect broken loose object files, either when
    underlying inflate() signalled the breakage, nor inflate()
    finished and we had garbage trailing at the end.  We do better
    now.
    
    We also make unpack_sha1_file() a static function to
    sha1_file.c, since it is not used by anybody outside.
    
    Signed-off-by: Junio C Hamano <junkio@cox.net>

:100644 100644 c291163e6d2096181ddf89954d2d65953d1ba687 4b5a7541a86c488f623793fdc498d0e149c40439
 M      cache.h
:100644 100644 6d0a72ed093d353a672129f7e460d0c1015212d7 ac6b5e00b6dec913a39cc54e84829dc3fc6c782f
 M      sha1_file.c


.git/objects/e6/9de29bb2d1d6434b8b29ae775ad8c2e48c5391 is the same 9
bytes:  30 78 9c 03 00 00 00 00 01

But it's considered corrupt by the current git.

> I wonder if this is a zlib issue. Although I seem to have libz-1.2.3 on 
> both machines, and that should be the most recent version. I don't see 
> Fedora development having anything else..

That's also 1.2.3.  I can reproduce the issue on another machine running
Fedora Core 6 i386 with all updates, also with zlib 1.2.3.

-- 
Regards,
Pavel Roskin

Re: git 1.5.1-rc1 doesn't like empty files

[Junio C Hamano]

Pavel Roskin <proski@gnu.org> writes:

> Hello!
>
> I don't know where this problem appeared, but it present in the current
> git (1.5.1-rc1).  Empty files become invalid objects in the repository:
>
> $ touch file 
> $ git-init
> Initialized empty Git repository in .git/
> $ git-add file
> $ git-commit -m "first commit"
> Created initial commit 16a476808d3cb0a4758997ba58193a9dcfad0fd8
> error: garbage at end of loose object
> 'e69de29bb2d1d6434b8b29ae775ad8c2e48c5391'

If the error message above is linewrapped by e-mail, I think it
is coming from here:

    static void *unpack_sha1_rest(z_stream *stream, void *buffer,...
    {
    ...
            if (bytes < size) {
                    stream->next_out = buf + bytes;
                    stream->avail_out = size - bytes;
                    while (status == Z_OK)
                            status = inflate(stream, Z_FINISH);
            }
            buf[size] = 0;
            if ((status == Z_OK || status == Z_STREAM_END) && !stream->avail_in) {
                    inflateEnd(stream);
                    return buf;
            }

            if (status < 0)
                    error("corrupt loose object '%s'", sha1_to_hex(sha1));
            else if (stream->avail_in)
                    error("garbage at end of loose object '%s'",
                          sha1_to_hex(sha1));
            free(buf);
            return NULL;
    }

Can you check what the value of the status is at that point?

Re: git 1.5.1-rc1 doesn't like empty files

[Linus Torvalds]

On Tue, 20 Mar 2007, Pavel Roskin wrote:
> 
> .git/objects/e6/9de29bb2d1d6434b8b29ae775ad8c2e48c5391 is the same 9
> bytes:  30 78 9c 03 00 00 00 00 01

Ahh.. You have

	[core]
		legacyheaders = false

don't you? If you didn't, you should see a 15-byte object, not a 9-byte 
one.

And yes, I can reproduce this with that "core.legacyheaders=false" 
setting. It seems that config option is simply broken, and we never 
noticed, because almost nobody uses it.

Alexander - do you happen to have that "legacyheaders" setting too? Maybe 
that explains your pack corruption?

		Linus

Re: git 1.5.1-rc1 doesn't like empty files

[Pavel Roskin]

On Mon, 2007-03-19 at 22:29 -0700, Junio C Hamano wrote:
>             if (status < 0)
>                     error("corrupt loose object '%s'",
> sha1_to_hex(sha1));
>             else if (stream->avail_in)
>                     error("garbage at end of loose object '%s'",
>                           sha1_to_hex(sha1));
>             free(buf);
>             return NULL;
>     }
> 
> Can you check what the value of the status is at that point?

status is 0 (Z_OK), stream->avail_in is 8.  I changed the code to print
them from the error() that prints "garbage at end of loose object".

-- 
Regards,
Pavel Roskin

Re: git 1.5.1-rc1 doesn't like empty files

[Linus Torvalds]

On Mon, 19 Mar 2007, Junio C Hamano wrote:
> 
> If the error message above is linewrapped by e-mail, I think it
> is coming from here:

I think I found it.

The thing is, if the output buffer is empty, we should *still* actually 
use the zlib routines to *unpack* that empty output buffer.

But we had a test that said "only unpack if we still expect more output".

So we wouldn't use up all the zlib stream, because we felt that we didn't 
need it, because we already had all the bytes we wanted. And it was 
"true": we did have all the output data. We just needed to also eat all 
the input data!

We've had this bug before - thinking that we don't need to inflate() 
anything because we already had it all..

		Linus

---
 sha1_file.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/sha1_file.c b/sha1_file.c
index b0b2177..c0efed3 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -1030,7 +1030,7 @@ static void *unpack_sha1_rest(z_stream *stream, void *buffer, unsigned long size
 		n = size;
 	memcpy(buf, (char *) buffer + bytes, n);
 	bytes = n;
-	if (bytes < size) {
+	if (bytes <= size) {
 		stream->next_out = buf + bytes;
 		stream->avail_out = size - bytes;
 		while (status == Z_OK)

Re: git 1.5.1-rc1 doesn't like empty files

[Pavel Roskin]

On Mon, 2007-03-19 at 22:41 -0700, Linus Torvalds wrote:
> 
> On Tue, 20 Mar 2007, Pavel Roskin wrote:
> > 
> > .git/objects/e6/9de29bb2d1d6434b8b29ae775ad8c2e48c5391 is the same 9
> > bytes:  30 78 9c 03 00 00 00 00 01
> 
> Ahh.. You have
> 
> 	[core]
> 		legacyheaders = false

Yes.

-- 
Regards,
Pavel Roskin

Re: git 1.5.1-rc1 doesn't like empty files

[Shawn O. Pearce]

Linus Torvalds <torvalds@linux-foundation.org> wrote:
> On Tue, 20 Mar 2007, Pavel Roskin wrote:
> > 
> > .git/objects/e6/9de29bb2d1d6434b8b29ae775ad8c2e48c5391 is the same 9
> > bytes:  30 78 9c 03 00 00 00 00 01
> 
> Ahh.. You have
> 
> 	[core]
> 		legacyheaders = false
> 
> don't you? If you didn't, you should see a 15-byte object, not a 9-byte 
> one.
> 
> And yes, I can reproduce this with that "core.legacyheaders=false" 
> setting. It seems that config option is simply broken, and we never 
> noticed, because almost nobody uses it.

For what it is worth, I have been running core.legacyheaders=false
on both my PowerBook (my main dev system) and on my x86 Cygwin
POS.  I guess I've been lucky, as I've never noticed any sort
of corruption.

Oh, wait, yes I did.  Just the other day.  A loose object got the
same zlib error as Pavel asked about.  But git-prune whacked the
damn thing.  I figured it was just a short write by Cygwin during
some sort of operation that I may have aborted; e.g. aborting an
update-index and running it again later, thus never actually using
that particular blob.

I didn't think twice about the error (until now), especially since
`git-fsck --full` did not whine after the corrupt loose object
was gone.

-- 
Shawn.

Re: git 1.5.1-rc1 doesn't like empty files

[Junio C Hamano]

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

> We've had this bug before - thinking that we don't need to inflate() 
> anything because we already had it all..
>
> 		Linus

Thanks.  I think we _do_ need a big fat warning near the code to
avoid the same mistake in the future.  Something like this?


diff --git a/sha1_file.c b/sha1_file.c
index 9fe2bd6..d273aff 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -1030,7 +1030,17 @@ static void *unpack_sha1_rest(z_stream *stream, void *buffer, unsigned long size
 		n = size;
 	memcpy(buf, (char *) buffer + bytes, n);
 	bytes = n;
-	if (bytes < size) {
+	if (bytes <= size) {
+		/*
+		 * The above condition must be (bytes <= size), not
+		 * (bytes < size).  In other words, even if we expect
+		 * no more output, the input zlib stream may have bytes
+		 * that express "this concludes the stream", and we do
+		 * want to eat that input.  Otherwise we would not be
+		 * able to test that we consumed all the input to reach
+		 * the expected size *and* zlib gave status == Z_STREAM_END
+		 * to signal all went well.
+		 */
 		stream->next_out = buf + bytes;
 		stream->avail_out = size - bytes;
 		while (status == Z_OK)

Re: git 1.5.1-rc1 doesn't like empty files

[Junio C Hamano]

Junio C Hamano <junkio@cox.net> writes:

> Linus Torvalds <torvalds@linux-foundation.org> writes:
>
>> We've had this bug before - thinking that we don't need to inflate() 
>> anything because we already had it all..
>>
>> 		Linus
>
> Thanks.  I think we _do_ need a big fat warning near the code to
> avoid the same mistake in the future...

By the way, I think the test that comes after the part you fixed
is wrong (I know it is my bad without running git-blame).  Since
we are making sure that we eat everything, we should expect
Z_STREAM_END and no avail_in.

diff --git a/sha1_file.c b/sha1_file.c
index d7dc80d..7dc16ea 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -1037,7 +1037,7 @@ static void *unpack_sha1_rest(z_stream *stream, void *buffer, unsigned long size
 			status = inflate(stream, Z_FINISH);
 	}
 	buf[size] = 0;
-	if ((status == Z_OK || status == Z_STREAM_END) && !stream->avail_in) {
+	if (status == Z_STREAM_END && !stream->avail_in) {
 		inflateEnd(stream);
 		return buf;
 	}

Re: git 1.5.1-rc1 doesn't like empty files

[Alexander Litvinov]

В сообщении от Tuesday 20 March 2007 11:41 Linus Torvalds написал:
> Alexander - do you happen to have that "legacyheaders" setting too? Maybe
> that explains your pack corruption?

It seems no:

$  git config core.legacyheaders
$  git config -l
user.name=Alexander Litvinov
user.email=XXX
core.logallrefupdates=true
core.filemode=false
core.autocrlf=true
diff.color=auto
status.color=auto
apply.whitespace=strip
core.repositoryformatversion=0
core.filemode=false
core.bare=false
remote.origin.url=/home/lan/src/XXX
remote.origin.fetch=+refs/heads/*:refs/remotes/origin/*
branch.master.remote=origin
branch.master.merge=refs/heads/master
branch.XXX.remote=origin
branch.XXX.merge=refs/heads/XXX

Re: git 1.5.1-rc1 doesn't like empty files

[Andy Parkins]

On Tuesday 2007 March 20 05:41, Linus Torvalds wrote:

> 	[core]
> 		legacyheaders = false
> noticed, because almost nobody uses it.

I'm not sure that's going to be true for long - the 1.5.0 release notes 
recommended setting it (assuming you didn't need backward compatibility) - 
which is exactly what I (and I'm sure others) did.


Andy
-- 
Dr Andy Parkins, M Eng (hons), MIET
andyparkins@gmail.com

Re: git 1.5.1-rc1 doesn't like empty files

[Junio C Hamano]

Andy Parkins <andyparkins@gmail.com> writes:

> On Tuesday 2007 March 20 05:41, Linus Torvalds wrote:
>
>> 	[core]
>> 		legacyheaders = false
>> noticed, because almost nobody uses it.
>
> I'm not sure that's going to be true for long - the 1.5.0 release notes 
> recommended setting it (assuming you didn't need backward compatibility) - 
> which is exactly what I (and I'm sure others) did.

Well, it is fixed in 'master' to be in -rc2, and that validation
does not exist in 'maint', so no harm is done.

Re: git 1.5.1-rc1 doesn't like empty files

[Andy Parkins]

On Tuesday 2007 March 20 08:49, Junio C Hamano wrote:

> >> noticed, because almost nobody uses it.
> >
> > I'm not sure that's going to be true for long - the 1.5.0 release notes
> > recommended setting it (assuming you didn't need backward compatibility)
> > - which is exactly what I (and I'm sure others) did.
>
> Well, it is fixed in 'master' to be in -rc2, and that validation
> does not exist in 'maint', so no harm is done.

It wasn't the presence of the bug I was highlighting - it was the idea that 
nobody uses that option.


Andy
-- 
Dr Andy Parkins, M Eng (hons), MIET
andyparkins@gmail.com

Re: git 1.5.1-rc1 doesn't like empty files

[Linus Torvalds]

On Tue, 20 Mar 2007, Andy Parkins wrote:

> On Tuesday 2007 March 20 08:49, Junio C Hamano wrote:
> 
> > >> noticed, because almost nobody uses it.
> > >
> > > I'm not sure that's going to be true for long - the 1.5.0 release notes
> > > recommended setting it (assuming you didn't need backward compatibility)
> > > - which is exactly what I (and I'm sure others) did.
> >
> > Well, it is fixed in 'master' to be in -rc2, and that validation
> > does not exist in 'maint', so no harm is done.
> 
> It wasn't the presence of the bug I was highlighting - it was the idea that 
> nobody uses that option.

Yeah, I'm actually happy that people seem to be using it, and it looks 
like the bug really was totally harmless apart from triggering a bogus 
error (ie it would never have corrupted anything, it just triggered an 
error that it shouldn't have).

And I think the only thing that could ever trigger it was really that 
special case of a zero-sized blob (or maybe an empty tree, but git doesn't 
generate those natively unless you do magic stuff by hand).

		Linus

Re: git 1.5.1-rc1 doesn't like empty files

[Linus Torvalds]

On Mon, 19 Mar 2007, Junio C Hamano wrote:
> 
> By the way, I think the test that comes after the part you fixed
> is wrong (I know it is my bad without running git-blame).  Since
> we are making sure that we eat everything, we should expect
> Z_STREAM_END and no avail_in.

Ack.

		Linus