Re: [Btrfs-devel] btrfs and git-reflog

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 25, 2008, 20:05 UTC
Message-ID: <7vmyqt657t.fsf@gitster.siamese.dyndns.org>
In-Reply-To: <alpine.LFD.1.00.0801250905570.14161@hp.linux-foundation.org>

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

Show 5 quoted lines

> Yes indeed. This is clearly git mis-using "readdir()", and should be 
> fixed. It could happen on other filesystems, and very much including ones 
> where there is no option of just "fixing" the filesystem (including 
> Linux: not everybody can upgrade their kernels just because git makes 
> some broken assumptions).

I agree that this is a broken assumption on git's part. When we have this loop:

	while (ent = readdir()) {
        	... do something in that directory ...
	}

we should expect readdir() may return new entries and cope with it, as "If a file is removed from or added to the directory after the most recent call to opendir() or rewinddir(), whether a subsequent call to readdir() returns an entry for that file is unspecified." [*1*]

The modification may not be done by us, but by "git push" from elsewhere to update an existing ref, which can run while we are inside this loop.

For this particular case, I think keeping track of what we have already dealt with and skipping when we see duplicates is a good enough solution. Removal of a loose ref is protected by taking a lock on the packed-refs file, so we shouldn't have to worry about races between pack-refs and a push to delete a ref (IOW, it won't be an issue that readdir(3) may report a file that is removd by simultaneous git-push).

However, I am not sure if the way delete_ref() takes locks (one for the ref itself and another for packed-refs file) is deadlock-free. That may need an independent fix if there is; I haven't closely looked at this.

I suspect we might want to change the current ".lock" suffix to something that would make the lock filenames an invalid ref (e.g., "..lock", or "~"), so that people can use "foo.lock" as a branch name. That is also outside of the scope of readdir(3) abuse fix, though.

[Reference]

*1* http://www.opengroup.org/onlinepubs/000095399/functions/readdir.html