git 1.5.1-rc1 doesn't like empty files

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.

On Mon, Mar 19, 2007 at 11:30:24PM -0400, Pavel Roskin wrote:

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

On Mon, 19 Mar 2007, Pavel Roskin wrote:

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

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

On Mon, 2007-03-19 at 21:54 -0700, Linus Torvalds wrote:

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.

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.

On Tue, 20 Mar 2007, Pavel Roskin wrote:

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

On Mon, 2007-03-19 at 22:41 -0700, Linus Torvalds wrote:

Yes.

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

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.

В сообщении от Tuesday 20 March 2007 11:41 Linus Torvalds написал:

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

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

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

Andy Parkins <andyparkins@gmail.com> writes:

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

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

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

Andy

On Tue, 20 Mar 2007, Andy Parkins wrote:

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

Pavel Roskin <proski@gnu.org> writes:

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?

On Mon, 2007-03-19 at 22:29 -0700, Junio C Hamano wrote:

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".

On Mon, 19 Mar 2007, Junio C Hamano wrote:

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(-)

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

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

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

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.

On Mon, 19 Mar 2007, Junio C Hamano wrote:

Ack.

		Linus