Re: [PATCH v3 1/3] packed-backend: fsck should allow an empty "packed-refs" file

From: shejialuo <shejialuo@gmail.com>
Date: May 12, 2025, 12:25 UTC
Message-ID: <aCHoovrKiSUemBCL@ArchLinux>
In-Reply-To: <aCGzIlLH_ESNg6-v@pks.im>

On Mon, May 12, 2025 at 10:36:50AM +0200, Patrick Steinhardt wrote:

Show 12 quoted lines

> On Sun, May 11, 2025 at 10:01:43PM +0800, shejialuo wrote:
> > During fsck, an empty "packed-refs" gives an error; this is unwarranted.
> > We should just skip checking the content of "packed-refs" just like the
> > runtime code paths such as "create_snapshot" which simply returns the
> > "snapshot" without checking the content of "packed-refs".
> 
> I think this doesn't quite answer the question whether this is a _good_
> idea though. The question that we need to answer is whether there are
> any writing code paths that may end up writing a "packed-refs" file that
> is completely empty. Modern Git would at least write the packed-refs
> header, wouldn't it?
>

That's right. In the current codebase, we would always write the header which could be easily reproduced by using the following command:

    git init repo
    cd repo && git pack-refs
    cat .git/packed-refs

And in "packed-backend.c::write_with_updates", we would always write the header.

Show 15 quoted lines

> The reason why I'm a little sceptical is that there is a common problem
> with ext4 caused by its delayed allocation [1]. If you:
> 
>   1. Write data to a temporary file.
>   2. Rename the file into place.
>   3. The host system crashes.
> 
> Then it may happen that the renamed file is now completely empty.
> 
> The root cause is a bug in the application: before renaming the file
> into place it _must_ fsync the file to disk. Git does that by default,
> but it is extremely easy to get wrong and we had bugs around this until
> ~2 years ago, if I remember correctly. We hit the problem several times
> in our production systems.
>

I see. I agree that in the most situation, an empty "packed-refs" file means that there is an issue.

> So I wonder whether ignoring empty files would cause us to miss such a
> common error. But I guess if there are valid cases where we may end up
> with an empty "packed-refs" file we cannot do anything about it.
>

I somehow think we would always write header in the Modern Git. But "create_snapshot" accept an empty existing "packed-refs" file at runtime.

And header is introduced in 694b7a1999 (repack_without_ref(): write peeled refs in the rewritten file, 2013-04-22). At this commit, we would always write the header into the "packed-refs" file.

But in runtime, we accept empty file or no header of the file content as we want to keep compatible. In my humble word, I think we should allow empty file at now. Then, In Git 3.0, we tighten all the rules (there must always be a header etc) and also update the runtime behavior.

> Patrick
> 
> [1]: https://thunk.org/tytso/blog/2009/03/12/delayed-allocation-and-the-zero-length-file-problem/

Thanks, Jialuo