Re: Possible Solaris problem in 'checkout_entry()'

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 19, 2008, 01:05 UTC
Message-ID: <7vwsnz5xrf.fsf@gitster.siamese.dyndns.org>
In-Reply-To: <alpine.LFD.1.00.0803170850090.3020@woody.linux-foundation.org>

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

Show 18 quoted lines

> On Mon, 17 Mar 2008, Linus Torvalds wrote:
>>
>> IOW, the real "checking" is taking place in "create_file()", so if the 
>> unlinking failed (due to a read-only directory or something), that's where 
>> we'll do the proper error reporting.
>
> Thinking about this, I'm probably full of sh*t.
>
> My argument is admittedly true in general, but there is one case it is 
> *not* true for: if the old entry was a symlink.
>
> IOW, let's imagine that the directory is read-only (or other permission 
> issue), and we want to unlink the old symlink, which points somewhere we 
> can write to. In that case, the symlink removal is important, because we 
> won't necessarily catch the error when we create the file in place later 
> (because that will just follow the symlink).
>
> So I retract my statement. We *should* check the result of the unlink.

While I agree we should check the result, I think we are safe against the un-unlinkable symlink case. If you have a stale symlink at "dir/file" where you are checking out a new blob, and the directory "dir" the symlink is in is unwritable, then our callpath would look like this:

	checkout_entry()
         unlink("dir/file") -- failure silently ignored which is bad
	 write_entry()
          create_file("dir/file")
           open("dir/file", O_WRONLY | O_CREAT | O_EXCL)

which would fail, and we get:

    error: git-checkout-index: unable to create file a/b (File exists)

from around ll.135 in entry.c::write_entry()

So I'll apply the patch purely as "Root on Solaris safety fix".