files_transaction_commit(): clean up empty directories

When deleting/pruning references, remove any directories that are made
empty by the deletion of loose references or of reflogs. Otherwise such
empty directories can survive forever and accumulate over time. (Even
'pack-refs', which is smart enough to remove the parent directories of
loose references that it prunes, leaves directories that were already
empty.)

And now that files_transaction_commit() takes care of deleting the
parent directories of loose references that it prunes, we don't have to
do that in prune_ref() anymore.

This change would be unwise if the *creation* of these directories could
race with our deletion of them. But the earlier changes in this patch
series made the creation paths robust against races, so now it is safe
to tidy them up more aggressively.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
Reviewed-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/refs/files-backend.c b/refs/files-backend.c
index bce00223459b16bbdb319b9efd68fdf4665e33d2..c426575fe9d4832dc101c08ff86964f129f3a94b 100644 (file)
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2346,7 +2346,6 @@ static void prune_ref(struct ref_to_prune *r)
        }
        ref_transaction_free(transaction);
        strbuf_release(&err);
-       try_remove_empty_parents(r->name, REMOVE_EMPTY_PARENTS_REF);
 }
 
 static void prune_refs(struct ref_to_prune *r)
@@ -3794,6 +3793,7 @@ static int files_transaction_commit(struct ref_store *ref_store,
                                        ret = TRANSACTION_GENERIC_ERROR;
                                        goto cleanup;
                                }
+                               update->flags |= REF_DELETED_LOOSE;
                        }
 
                        if (!(update->flags & REF_ISPRUNING))
@@ -3806,16 +3806,38 @@ static int files_transaction_commit(struct ref_store *ref_store,
                ret = TRANSACTION_GENERIC_ERROR;
                goto cleanup;
        }
-       for_each_string_list_item(ref_to_delete, &refs_to_delete)
-               unlink_or_warn(git_path("logs/%s", ref_to_delete->string));
+
+       /* Delete the reflogs of any references that were deleted: */
+       for_each_string_list_item(ref_to_delete, &refs_to_delete) {
+               if (!unlink_or_warn(git_path("logs/%s", ref_to_delete->string)))
+                       try_remove_empty_parents(ref_to_delete->string,
+                                                REMOVE_EMPTY_PARENTS_REFLOG);
+       }
+
        clear_loose_ref_cache(refs);
 
 cleanup:
        transaction->state = REF_TRANSACTION_CLOSED;
 
-       for (i = 0; i < transaction->nr; i++)
-               if (transaction->updates[i]->backend_data)
-                       unlock_ref(transaction->updates[i]->backend_data);
+       for (i = 0; i < transaction->nr; i++) {
+               struct ref_update *update = transaction->updates[i];
+               struct ref_lock *lock = update->backend_data;
+
+               if (lock)
+                       unlock_ref(lock);
+
+               if (update->flags & REF_DELETED_LOOSE) {
+                       /*
+                        * The loose reference was deleted. Delete any
+                        * empty parent directories. (Note that this
+                        * can only work because we have already
+                        * removed the lockfile.)
+                        */
+                       try_remove_empty_parents(update->refname,
+                                                REMOVE_EMPTY_PARENTS_REF);
+               }
+       }
+
        string_list_clear(&refs_to_delete, 0);
        free(head_ref);
        string_list_clear(&affected_refnames, 0);

diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index dc81acc902346cde6637033720d1ceea939018c4..15d5a1e08842f8ec0b35d67cfb573d6ad8784521 100644 (file)
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -55,6 +55,12 @@
  */
 #define REF_UPDATE_VIA_HEAD 0x100
 
+/*
+ * Used as a flag in ref_update::flags when the loose reference has
+ * been deleted.
+ */
+#define REF_DELETED_LOOSE 0x200
+
 /*
  * Return true iff refname is minimally safe. "Safe" here means that
  * deleting a loose reference by this name will not do any damage, for
@@ -158,8 +164,9 @@ struct ref_update {
 
        /*
         * One or more of REF_HAVE_NEW, REF_HAVE_OLD, REF_NODEREF,
-        * REF_DELETING, REF_ISPRUNING, REF_LOG_ONLY, and
-        * REF_UPDATE_VIA_HEAD:
+        * REF_DELETING, REF_ISPRUNING, REF_LOG_ONLY,
+        * REF_UPDATE_VIA_HEAD, REF_NEEDS_COMMIT, and
+        * REF_DELETED_LOOSE:
         */
        unsigned int flags;
 

diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index d4fb9770600df75d3162ffbc6c567b1ae152fccd..97d87936ff77d08cf4b3d4ef01d21b7fc73c090f 100755 (executable)
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -191,6 +191,33 @@ test_expect_success \
         git update-ref HEAD'" $A &&
         test $A"' = $(cat .git/'"$m"')'
 
+test_expect_success "empty directory removal" '
+       git branch d1/d2/r1 HEAD &&
+       git branch d1/r2 HEAD &&
+       test -f .git/refs/heads/d1/d2/r1 &&
+       test -f .git/logs/refs/heads/d1/d2/r1 &&
+       git branch -d d1/d2/r1 &&
+       ! test -e .git/refs/heads/d1/d2 &&
+       ! test -e .git/logs/refs/heads/d1/d2 &&
+       test -f .git/refs/heads/d1/r2 &&
+       test -f .git/logs/refs/heads/d1/r2
+'
+
+test_expect_success "symref empty directory removal" '
+       git branch e1/e2/r1 HEAD &&
+       git branch e1/r2 HEAD &&
+       git checkout e1/e2/r1 &&
+       test_when_finished "git checkout master" &&
+       test -f .git/refs/heads/e1/e2/r1 &&
+       test -f .git/logs/refs/heads/e1/e2/r1 &&
+       git update-ref -d HEAD &&
+       ! test -e .git/refs/heads/e1/e2 &&
+       ! test -e .git/logs/refs/heads/e1/e2 &&
+       test -f .git/refs/heads/e1/r2 &&
+       test -f .git/logs/refs/heads/e1/r2 &&
+       test -f .git/logs/HEAD
+'
+
 cat >expect <<EOF
 $Z $A $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150200 +0000      Initial Creation
 $A $B $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150260 +0000      Switch