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);
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
