Re: Behavior of git rm

From: Jeff King <peff@peff.net>
Date: Apr 3, 2013, 15:58 UTC
Message-ID: <20130403155841.GA16885@sigill.intra.peff.net>
In-Reply-To: <1365000624535-7581485.post@n2.nabble.com>

On Wed, Apr 03, 2013 at 07:50:24AM -0700, jpinheiro wrote:

Show 13 quoted lines

> While experimenting with git we found an unexpected behavior with git rm.
> Here is a trace of the unexpected behavior:
> 
> $ git init
> $ mkdir D
> $ echo "Hi" > D/F
> $ git add D/F
> $ rm -r D
> $ echo "Hey" > D
> $ git rm D/F
> warning: 'D/F': Not a directory
> rm 'D/F'
> fatal: git rm: 'D/F': Not a directory

We drop the D/F entry from the index, but then fail to actually remove it from the filesystem, because it has already been replaced. It is impossible to tell from this toy example what the true intent was, but in such a situation, there is a reasonable chance that the user should have invoked "rm --cached" in the first place.

That being said, we do try to handle files which have already gone missing; when unlink() fails, we do not consider it an error if we got ENOENT. We could perhaps add ENOTDIR to that list, as it also indicates that the file is gone (it just happens that one of its prefix directories was replaced with something else).

The opposite case is also interesting:

  $ git init
  $ echo 1 >D
  $ git add D
  $ rm D
  $ mkdir D
  $ echo 2 >D/F
  $ git rm D
  rm 'D'
  fatal: git rm: 'D': Is a directory

We expect to see 'D' as a file, but it is now a directory. We _could_ recursively remove the directory, but that has the potential to delete files that the user does not expect.

So in both cases, "git rm" could certainly detect the situation and proceed with the destructive operation. But when there is such a conflict between what's in the working tree and what's in the index, I think we may be better off erring on the conservative side and bailing, and letting the user reconcile the differences themselves (using either "git add" or "git rm --cached" to update the index, or deciding how to handle the working tree contents themselves with regular "rm").

Of the two situations, I think the first one is less likely to be destructive (noticing that a file is already gone via ENOTDIR), as we are only proceeding with the index deletion, and we end up not touching the filesystem at all. That patch would look something like:

diff --git a/builtin/rm.c b/builtin/rm.c
index dabfcf6..7b91d52 100644
--- a/builtin/rm.c
+++ b/builtin/rm.c
@@ -110,7 +110,7 @@ static int check_local_mod(unsigned char *head, int index_only)
 		ce = active_cache[pos];
 
 		if (lstat(ce->name, &st) < 0) {
-			if (errno != ENOENT)
+			if (errno != ENOENT && errno != ENOTDIR)
 				warning("'%s': %s", ce->name, strerror(errno));
 			/* It already vanished from the working tree */
 			continue;
diff --git a/dir.c b/dir.c
index 57394e4..f9e7355 100644
--- a/dir.c
+++ b/dir.c
@@ -1603,7 +1603,7 @@ int remove_path(const char *name)
 {
 	char *slash;
 
-	if (unlink(name) && errno != ENOENT)
+	if (unlink(name) && errno != ENOENT && errno != ENOTDIR)
 		return -1;
 
 	slash = strrchr(name, '/');