Re: [PATCH] Remove empty ref directories while reading loose refs

From: Jeff King <peff@peff.net>
Date: Feb 10, 2012, 20:53 UTC
Message-ID: <20120210205330.GE5504@sigill.intra.peff.net>
In-Reply-To: <1328891127-17150-1-git-send-email-pclouds@gmail.com>

On Fri, Feb 10, 2012 at 11:25:27PM +0700, Nguyen Thai Ngoc Duy wrote:

Show 6 quoted lines

> Empty directories in $GIT_DIR/refs increases overhead at startup.
> Removing a ref does not remove its parent directories even if it's the
> only file left so empty directories will be hanging around.
> [...]
> This patch removes empty directories as we see while traversing
> $GIT_DIR/refs and reverts be7c6d4 because it's no longer needed.

It feels wrong to me to be writing to the repository during what would otherwise be a read-only operation. Especially without locking. Doesn't this create a race condition with:

  git update-ref refs/foo/bar $sha1 &      (a)
  git for-each-ref                         (b)

if you have this sequence of events:

  1. (a) wants to create the ref, so it must first mkdir
     ".git/refs/foo".

  2. (b) is reading refs and notices the empty "foo" directory. It
     rmdirs it.

  3. (a) now attempts to create "bar" inside the newly created "foo"
     directory. This fails, because the directory does not exist.

A similar race already can happen with:

  git update-ref refs/foo/bar $sha1 &
  git update-ref refs/foo $sha1

since the latter will remove a stale "foo" directory before it can create the new ref file. But that race is OK, I think. Those are both write operations, and one of them _must_ fail, because they are in conflict (and I think even with the race they fail gracefully, with the latter one "winning").

> pack-refs was taught of cleaning up empty directories in be7c6d4
> (pack-refs: remove newly empty directories - 2010-07-06), but it only
> checks parent directories of packed refs only. Already empty dirs are
> left untouched.

I'd much rather have pack-refs simply learn to remove all stale directories. We at least know that "gc" is a slightly riskier operation.

-Peff