[PATCH] refs: cleanup directories when deleting packed ref

When deleting a packed ref, a lockfile is made in the directory that
would contain the loose copy of that ref, creating any directories in
the ref's path that do not exist. When the transaction completes, the
lockfile is deleted, but any empty parent directories made when creating
the lockfile are left in place.  These empty directories are not removed
by 'pack-refs' or other housekeeping tasks and will accumulate over
time.

When deleting a loose ref, we remove all empty parent directories at the
end of the transaction.

This commit applies the parent directory cleanup logic used when
deleting loose refs to packed refs as well.

Signed-off-by: Will Chandler <redacted>
---
 refs/files-backend.c  | 12 ++++++------
 t/t1400-update-ref.sh |  9 +++++++++
 2 files changed, 15 insertions(+), 6 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 119972ee16..49e6ee069a 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c

@@ -45,10 +45,10 @@
 #define REF_UPDATE_VIA_HEAD (1 << 8)
 
 /*
- * Used as a flag in ref_update::flags when the loose reference has
- * been deleted.
+ * Used as a flag in ref_update::flags when a reference has been
+ * deleted and the ref's parent directories may need cleanup.
  */
-#define REF_DELETED_LOOSE (1 << 9)
+#define REF_DELETED_RMDIR (1 << 9)
 
 struct ref_lock {
 	char *ref_name;

@@ -2852,6 +2852,7 @@ static int files_transaction_finish(struct ref_store *ref_store,
 
 		if (update->flags & REF_DELETING &&
 		    !(update->flags & REF_LOG_ONLY)) {
+			update->flags |= REF_DELETED_RMDIR;
 			if (!(update->type & REF_ISPACKED) ||
 			    update->type & REF_ISSYMREF) {
 				/* It is a loose reference. */

@@ -2861,7 +2862,6 @@ static int files_transaction_finish(struct ref_store *ref_store,
 					ret = TRANSACTION_GENERIC_ERROR;
 					goto cleanup;
 				}
-				update->flags |= REF_DELETED_LOOSE;
 			}
 		}
 	}

@@ -2874,9 +2874,9 @@ static int files_transaction_finish(struct ref_store *ref_store,
 	for (i = 0; i < transaction->nr; i++) {
 		struct ref_update *update = transaction->updates[i];
 
-		if (update->flags & REF_DELETED_LOOSE) {
+		if (update->flags & REF_DELETED_RMDIR) {
 			/*
-			 * The loose reference was deleted. Delete any
+			 * The reference was deleted. Delete any
 			 * empty parent directories. (Note that this
 			 * can only work because we have already
 			 * removed the lockfile.)

diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index e31f65f381..adae9ef91f 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh

@@ -1598,4 +1598,13 @@ test_expect_success 'transaction cannot restart ongoing transaction' '
 	test_must_fail git show-ref --verify refs/heads/restart
 '
 
+test_expect_success 'directory not created deleting packed ref' '
+	git branch d1/d2/r1 HEAD &&
+	git pack-refs --all &&
+	test_path_is_missing .git/refs/heads/d1/d2 &&
+	git branch -d d1/d2/r1 &&
+	test_path_is_missing .git/refs/heads/d1/d2 &&
+	test_path_is_missing .git/refs/heads/d1
+'
+
 test_done

-- 
2.30.2

On Fri, May 07, 2021 at 10:37:25AM -0400, Will Chandler wrote:

When deleting a packed ref, a lockfile is made in the directory that
would contain the loose copy of that ref, creating any directories in
the ref's path that do not exist. When the transaction completes, the
lockfile is deleted, but any empty parent directories made when creating
the lockfile are left in place.  These empty directories are not removed
by 'pack-refs' or other housekeeping tasks and will accumulate over
time.

When deleting a loose ref, we remove all empty parent directories at the
end of the transaction.

This commit applies the parent directory cleanup logic used when
deleting loose refs to packed refs as well.

Hmm. I can certainly believe that such a bug exists, but...

+test_expect_success 'directory not created deleting packed ref' '
+	git branch d1/d2/r1 HEAD &&
+	git pack-refs --all &&
+	test_path_is_missing .git/refs/heads/d1/d2 &&
+	git branch -d d1/d2/r1 &&
+	test_path_is_missing .git/refs/heads/d1/d2 &&
+	test_path_is_missing .git/refs/heads/d1
+'

...this test passes even without your patch applied. I wonder if there's
something else required to trigger the problem.

-Peff

On Fri, May 07, 2021 at 05:56:47PM -0400, Jeff King wrote:

quoted

+test_expect_success 'directory not created deleting packed ref' '
+	git branch d1/d2/r1 HEAD &&
+	git pack-refs --all &&
+	test_path_is_missing .git/refs/heads/d1/d2 &&
+	git branch -d d1/d2/r1 &&
+	test_path_is_missing .git/refs/heads/d1/d2 &&
+	test_path_is_missing .git/refs/heads/d1
+'

...this test passes even without your patch applied. I wonder if there's
something else required to trigger the problem.

If I replace "git branch -d" with "git update-ref -d", then the problem
does trigger (and your patch does indeed clear it up). I wonder what the
difference is.

-Peff

On Fri, May 07, 2021 at 06:02:17PM -0400, Jeff King wrote:

On Fri, May 07, 2021 at 05:56:47PM -0400, Jeff King wrote:

quoted

quoted

+test_expect_success 'directory not created deleting packed ref' '
+	git branch d1/d2/r1 HEAD &&
+	git pack-refs --all &&
+	test_path_is_missing .git/refs/heads/d1/d2 &&
+	git branch -d d1/d2/r1 &&
+	test_path_is_missing .git/refs/heads/d1/d2 &&
+	test_path_is_missing .git/refs/heads/d1
+'

...this test passes even without your patch applied. I wonder if there's
something else required to trigger the problem.

If I replace "git branch -d" with "git update-ref -d", then the problem
does trigger (and your patch does indeed clear it up). I wonder what the
difference is.

I think this comes down to the interfaces. In update-ref, we call
delete_ref(), which creates a transaction to delete the single ref. It
realizes the ref is packed and there is no loose file to delete.

Whereas in git-branch, call the plural delete_refs(), which handles the
packed and loose stores separately. It first deletes everything from the
packed ref store in one go, and then the loose store. And it's actually
the deletion from the loose store which gets weird. The ref isn't
_anywhere_ at this point. So when we try to read it, we don't set the
REF_ISPACKED flag. And thus when it comes time to clean up the loose
ref, we say "not packed, so I guess it's worth calling unlink()". Of
course that syscall fails, but our unlink_or_msg() wrapper turns ENOENT
into success (which is sensible; we want it to be gone, and it is).

And so we think we've deleted a loose ref, and thus call
try_remove_empty_parents(), which cleans up the extra directory.

So I'd argue that it's actually delete_refs() which is called from
git-branch that is a little confused, or possibly even buggy. But I
don't think it's hurting anything, and working around it would probably
be awkward and/or inefficient.

Getting back to your patch, though, you are definitely fixing a problem
with update-ref (which correctly realizes there is no loose ref to clean
up, but forgets that we had to make a lockfile). And the solution you
have looks correct. I think you just need to update the test to exercise
it with "update-ref -d".

-Peff

On Fri, May 07, 2021 at 06:57:39PM -0400, Jeff King wrote:

Getting back to your patch, though, you are definitely fixing a problem
with update-ref (which correctly realizes there is no loose ref to clean
up, but forgets that we had to make a lockfile). And the solution you
have looks correct. I think you just need to update the test to exercise
it with "update-ref -d".

Thanks for the thorough analysis! Apologies for the confusion, I really
appreciate your patience.

I'll get the test fixed and note that this is specific to 'update-ref'.

Subject: [PATCH v2] refs: cleanup directories when deleting packed ref

When deleting a packed ref via 'update-ref -d', a lockfile is made in
the directory that would contain the loose copy of that ref, creating
any directories in the ref's path that do not exist. When the
transaction completes, the lockfile is deleted, but any empty parent
directories made when creating the lockfile are left in place.  These
empty directories are not removed by 'pack-refs' or other housekeeping
tasks and will accumulate over time.

When deleting a loose ref, we remove all empty parent directories at the
end of the transaction.

This commit applies the parent directory cleanup logic used when
deleting loose refs to packed refs as well.

Signed-off-by: Will Chandler <redacted>
---
 refs/files-backend.c  | 12 ++++++------
 t/t1400-update-ref.sh |  9 +++++++++
 2 files changed, 15 insertions(+), 6 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 119972ee16..49e6ee069a 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c

@@ -45,10 +45,10 @@
 #define REF_UPDATE_VIA_HEAD (1 << 8)
 
 /*
- * Used as a flag in ref_update::flags when the loose reference has
- * been deleted.
+ * Used as a flag in ref_update::flags when a reference has been
+ * deleted and the ref's parent directories may need cleanup.
  */
-#define REF_DELETED_LOOSE (1 << 9)
+#define REF_DELETED_RMDIR (1 << 9)
 
 struct ref_lock {
 	char *ref_name;

@@ -2852,6 +2852,7 @@ static int files_transaction_finish(struct ref_store *ref_store,
 
 		if (update->flags & REF_DELETING &&
 		    !(update->flags & REF_LOG_ONLY)) {
+			update->flags |= REF_DELETED_RMDIR;
 			if (!(update->type & REF_ISPACKED) ||
 			    update->type & REF_ISSYMREF) {
 				/* It is a loose reference. */

@@ -2861,7 +2862,6 @@ static int files_transaction_finish(struct ref_store *ref_store,
 					ret = TRANSACTION_GENERIC_ERROR;
 					goto cleanup;
 				}
-				update->flags |= REF_DELETED_LOOSE;
 			}
 		}
 	}

@@ -2874,9 +2874,9 @@ static int files_transaction_finish(struct ref_store *ref_store,
 	for (i = 0; i < transaction->nr; i++) {
 		struct ref_update *update = transaction->updates[i];
 
-		if (update->flags & REF_DELETED_LOOSE) {
+		if (update->flags & REF_DELETED_RMDIR) {
 			/*
-			 * The loose reference was deleted. Delete any
+			 * The reference was deleted. Delete any
 			 * empty parent directories. (Note that this
 			 * can only work because we have already
 			 * removed the lockfile.)

diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index e31f65f381..4506cd435b 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh

@@ -1598,4 +1598,13 @@ test_expect_success 'transaction cannot restart ongoing transaction' '
 	test_must_fail git show-ref --verify refs/heads/restart
 '
 
+test_expect_success 'directory not created deleting packed ref' '
+	git branch d1/d2/r1 HEAD &&
+	git pack-refs --all &&
+	test_path_is_missing .git/refs/heads/d1/d2 &&
+	git update-ref -d refs/heads/d1/d2/r1 &&
+	test_path_is_missing .git/refs/heads/d1/d2 &&
+	test_path_is_missing .git/refs/heads/d1
+'
+
 test_done

-- 
2.30.2

On 08/05/21 12.00, Will Chandler wrote:

I ask to you: why did you send v2 patch as reply to v1?

Supposed that I interested to apply only this v2, instead of v1.
With this situation, I downloaded mbox for v1, which contains v2
patch as reply to v1. And git-am would instead apply v1 instead.

So why not send this v2 as separate message-id?

-- 
An old man doll... just what I always wanted! - Clara

On Sat, May 08, 2021 at 01:00:43AM -0400, Will Chandler wrote:

Subject: [PATCH v2] refs: cleanup directories when deleting packed ref

When deleting a packed ref via 'update-ref -d', a lockfile is made in
the directory that would contain the loose copy of that ref, creating
any directories in the ref's path that do not exist. When the
transaction completes, the lockfile is deleted, but any empty parent
directories made when creating the lockfile are left in place.  These
empty directories are not removed by 'pack-refs' or other housekeeping
tasks and will accumulate over time.

When deleting a loose ref, we remove all empty parent directories at the
end of the transaction.

This commit applies the parent directory cleanup logic used when
deleting loose refs to packed refs as well.

This thread got off on a tangent about threads, but to get back to the
main idea: this v2 patch looks good to me. Thank you very much for
finding and fixing it.

Regarding threads, I agree with everything Junio already said. My only
suggestion would be to make sure the "Subject" above becomes the actual
email subject, rather than an in-body header (that may be what confused
the b4 tool, but more importantly, threaded readers like mutt will show
the changed subject in the thread index, making it very clear at a
glance that there is a v2 and not just more discussion).

-Peff