Subject: [PATCH 00/20] Delete directories left empty after ref deletion

From: Michael Haggerty <[email-masked]>
Date: Tue, 16 Feb 2016 14:22:13 +0100

Previously, we were pretty sloppy about leaving empty directories
behind (under both $GIT_DIR/refs and $GIT_DIR/logs) when deleting
references. Such directories could accumulate essentially forever.
It's true that `pack-refs` deletes directories that it empties, but if
a directory is *already* empty, then `pack-refs` doesn't remove it. It
is also true that if an empty directory gets in the way of the
creation of a *new* reference, then it is deleted. But otherwise there
is no systematic cleanup of empty directories.

Aside from being messy, wasting disk space, and costing extra time
when enumerating references, the proliferation of empty directories
was triggering a libgit2 bug [1] on our servers. (We use temporary,
uniquely-named references for internal purposes, causing a lot of
empty directories to accumulate in some cases.)

This problem was also recently reported by Karl Moskowski [2] to be
the cause of problem on non-case-sensitive filesystems.

This patch series makes the reference update machinery more aggressive
about deleting empty directories under $GIT_DIR/refs and under
$GIT_DIR/logs when deleting references and/or reflogs. This doesn't
eliminate all situations where empty directories can be left behind
[3], but it covers the worst offenders.

As prelude to the main change, there are a number of patches that make
the *creation* of reflog directories more robust against races. Since
we want to delete such directories more aggressively, we have to worry
more about a race between a process that is creating a new reflog, and
another process that might be deleting the containing directory at the
same time. (We already had protection against this sort of race for
reference creation, but not for reflog creation.)

And since I got tired of writing the same code over and over, I
abstracted the code for retrying directory creation into a new
function, raceproof_create_file(). This function replaces similar code
that appeared in two other places and is now also used for creating
reflog files. (Can anybody think of any other code that has to deal
with the same kind of race and could maybe benefit?)

This patch series is also available from my GitHub account [4] as
branch delete-empty-refs-dirs.

As you might imagine, this patch series conflicts with David Turner's
dt/refs-backend-lmdb series [5]. But the conflicts are not too bad [6].
When one or the other of these series is ready to progress, I'd be
happy to help resolve the conflicts with the other.

Michael

[1] https://github.com/libgit2/libgit2/issues/3576

[2] http://thread.gmane.org/gmane.comp.version-control.git/283504

[3] Off the top of my head, one case that is not covered is when a
    reflog is expired: a lock is acquired under $GIT_DIR/refs, but the
    directory that held it is not deleted when the lock is released.

[4] http://github.com/mhagger/git

[5] http://thread.gmane.org/gmane.comp.version-control.git/285604

[6] For a merge of this patch series with dt/refs-backend-lmdb v4, see
    branch merge-delete-empty-refs-dirs-refs-backend-lmdb at my GitHub
    repository [4].

Michael Haggerty (20):
  safe_create_leading_directories_const(): preserve errno
  safe_create_leading_directories(): set errno on SCLD_EXISTS
  raceproof_create_file(): new function
  lock_ref_sha1_basic(): use raceproof_create_file()
  rename_tmp_log(): use raceproof_create_file()
  rename_tmp_log(): improve error reporting
  log_ref_setup(): separate code for create vs non-create
  log_ref_setup(): improve robustness against races
  log_ref_setup(): pass the open file descriptor back to the caller
  log_ref_write_1(): don't depend on logfile
  log_ref_setup(): manage the name of the reflog file internally
  log_ref_write_1(): inline function
  try_remove_empty_parents(): rename parameter "name" -> "refname"
  try_remove_empty_parents(): don't trash argument contents
  try_remove_empty_parents(): don't accommodate consecutive slashes
  t5505: use "for-each-ref" to test for the non-existence of references
  delete_ref_loose(): derive loose reference path from lock
  delete_ref_loose(): inline function
  try_remove_empty_parents(): teach to remove parents of reflogs, too
  ref_transaction_commit(): clean up empty directories

 cache.h               |  26 +++-
 refs/files-backend.c  | 370 ++++++++++++++++++++++++++------------------------
 refs/refs-internal.h  |   9 +-
 sha1_file.c           |  77 ++++++++++-
 t/t1400-update-ref.sh |  27 ++++
 t/t5505-remote.sh     |   2 +-
 6 files changed, 325 insertions(+), 186 deletions(-)

-- 
2.7.0