Re: Behavior of git rm

From: Jeff King <peff@peff.net>
Date: Apr 3, 2013, 20:36 UTC
Message-ID: <20130403203612.GB3982@sigill.intra.peff.net>
In-Reply-To: <7vli8z5xfr.fsf@alter.siamese.dyndns.org>

On Wed, Apr 03, 2013 at 10:35:52AM -0700, Junio C Hamano wrote:

Show 21 quoted lines

> > 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)
> 
> OK.  We may be running lstat() on D/F but there may be D that is not
> a directory.  If it is a file, we get ENOTDIR.
> 
> By the way, if D is a dangling symlink, we get ENOENT; in such a
> case, we report "rm 'D/F'" on the output and remove the index entry.
>
> 	$ rm -f .git/index && rm -fr D E
> 	$ mkdir D && >D/F && git add D && rm -fr D
>         $ ln -s erewhon D && git rm D/F && git ls-files
>         rm 'D/F'

That seems sane to me, and makes me feel like handling ENOTDIR here is the right direction. What that conditional is trying to say is "if it is because the file is not there...", and so far we know of three conditions where it is not there:

  1. There is no entry at that path.

  2. There is a non-directory in the prefix of that path.

  3. There is a dangling symlink in the prefix of that path.

(1) and (3) we already handle via ENOENT. I think it is sane to handle (2) the same as (3), but we do not do so currently.

Show 8 quoted lines

> Also if D is a symlink that point at a directory E, "git rm" does
> something interesting.
> 
> (1) Perhaps we want a complaint in this case.
> 
> 	$ rm -f .git/index && rm -fr D E
> 	$ mkdir D && >D/F && git add D && rm -fr D
> 	$ mkdir E && ln -s E D && git rm D/F

I think that is OK without complaint; the user asked to get rid of D/F, and it is indeed gone (as well as its index entry) after the call finishes. And we did not even need to delete anything, so we cannot be losing data. I am much more concerned about this case:

Show 6 quoted lines

> (2) Perhaps we want to make sure D/F is not beyond a symlink in this
>     case.
> 
> 	$ rm -f .git/index && rm -fr D E
> 	$ mkdir D && >D/F && git add D && rm -fr D
> 	$ mkdir E && ln -s E D && date >E/F && git rm D/F

where the user is deleting something that may or may not be related to the original D/F. On the other hand, I don't have that much sympathy; "rm" would make the same deletion. But hmm...shouldn't we be doing an up-to-date check? Indeed:

  $ git rm D/F
  error: 'D/F' has staged content different from both the file and the HEAD
  (use -f to force removal)
  $ git commit -m foo && git rm D/F
  $ git rm D/F
  error: 'D/F' has local modifications
  (use --cached to keep the file, or -f to force removal)

So I do not think we need any extra safety; the content-level checks should be enough to make sure we are not losing anything.

-Peff