Why O_EXCL would make this difference? I am puzzled..

Why O_EXCL would make this difference? I am puzzled..

[Junio C Hamano]

The bisect search found that the commit

	Make "git-checkout" create files with O_EXCL

makes the test t1005 fail.  But it is getting late so I give up
to figuire this out tonight.

Re: Why O_EXCL would make this difference? I am puzzled..

[Linus Torvalds]

On Thu, 14 Jul 2005, Junio C Hamano wrote:
>
> The bisect search found that the commit
> 
> 	Make "git-checkout" create files with O_EXCL
> 
> makes the test t1005 fail.  But it is getting late so I give up
> to figuire this out tonight.

Ahh, thanks for noticing.

It says

	* expecting success: rm -f .git/index &&
	     rm -fr DF &&
	     mkdir DF &&
	     echo DF/DF >DF/DF &&
	     git-update-cache --add DF/DF &&
	     read_tree_twoway $treeDFDF $treeDF &&
	     git-ls-files --stage >DFDFcheck.out &&
	     diff -u DF.out DFDFcheck.out &&
	     check_cache_at DF clean && # different from pure 2-way
	     :
	100644 052efc3abbc31348f7abd34535b1953d38273257 3       DF
	100644 b90ea14b2dd74b6f377c10870b3757344bbe077c 1       DF/DF
	100644 b90ea14b2dd74b6f377c10870b3757344bbe077c 2       DF/DF
	Adding DF
	error: git-checkout-cache: unable to create file DF (File exists)
	Removing DF/DF
	100644 052efc3abbc31348f7abd34535b1953d38273257 0       DF
	DF: dirty
	* FAIL 24: DF vs DF/DF case test (#2) rm -f .git/index &&

Which is strange. If the path already exists, the "lstat(path, &st)" 
should have returned 0 in checkout_entry (which is the only caller of 
"write_entry()"), and that code does an "unlink(path)" before it calls 
write-entry. So I was sure O_EXCL wouldn't make any difference, but I'm 
obviously wrong, wrong, wrong.

I'll strace the dang thing.

		Linus

Re: Why O_EXCL would make this difference? I am puzzled..

[Linus Torvalds]

On Thu, 14 Jul 2005, Linus Torvalds wrote:
> 
> I'll strace the dang thing.

It's the "Adding" case in git-merge-one-file-script, which does

	git-checkout-cache -u -f -- "$4"

and it's because of this:

	lstat64("DF", {st_mode=S_IFDIR|0775, st_size=4096, ...}) = 0
	unlink("DF")                            = -1 EISDIR (Is a directory)
	.. unpack the object ..
	open("DF", O_WRONLY|O_CREAT|O_TRUNC|O_EXCL, 0666) = -1 EEXIST (File exists)

ie the problem is that we actually _have_ a test for this, but it's:

	if (errno == EISDIR && force) {

but if the directory already exists, we do that wrong.

Btw, this also shows a different problem: the symlink handling doesn't do 
any of this, so you cannot even force a directory to become a symlink.

I think both problems can be fixed by just moving the (EISDIR && force) 
test down to the "unlink()".

Will try.

		Linus