Re: Possible Solaris problem in 'checkout_entry()'

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.

So maybe something like this (which does the "avoid Solaris-is-crap"
issue too by moving the unlink to being after the directory test).

Untested.

		Linus

---
 entry.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/entry.c b/entry.c
index 44f4b89..222aaa3 100644
--- a/entry.c
+++ b/entry.c

@@ -218,7 +218,6 @@ int checkout_entry(struct cache_entry *ce, const struct checkout *state, char *t
 		 * to emulate by hand - much easier to let the system
 		 * just do the right thing)
 		 */
-		unlink(path);
 		if (S_ISDIR(st.st_mode)) {
 			/* If it is a gitlink, leave it alone! */
 			if (S_ISGITLINK(ce->ce_mode))

@@ -226,7 +225,8 @@ int checkout_entry(struct cache_entry *ce, const struct checkout *state, char *t
 			if (!state->force)
 				return error("%s is a directory", path);
 			remove_subtree(path);
-		}
+		} else if (unlink(path))
+			return error("unable to unlink old '%s' (%s)", path, strerror(errno));
 	} else if (state->not_new)
 		return 0;
 	create_directories(path, state);