{"thread":{"id":"63758","subject":"[PATCH] refs/files: remove empty parent dirs when ref creation fails","startedAt":"2025-07-08T10:20:00Z","lastAt":"2025-07-08T21:52:24Z","messageCount":2,"participants":["Patrick Steinhardt","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"521526","messageId":"20250708-b4-pks-reffiles-prune-empty-dirs-on-abort-v1-1-3bae02e4f034@pks.im","threadId":"63758","inReplyTo":null,"subject":"[PATCH] refs/files: remove empty parent dirs when ref creation fails","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-07-08T10:19:54Z","receivedAt":"2025-07-08T10:20:00Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When creating a new reference in the \"files\" backend we first create the\ndirectory hierarchy for that reference, then create the lockfile for\nthat reference, and finally rename the lockfile into place. When the\ntransaction gets aborted we prune the lockfile, but we don't clean up\nthe directory hierarchy that we may have created for the lockfile.\n\nIn some egde cases this can lead to lots of empty directories being\ncluttered in the \".git/refs\" directory that really serve no purpose at\nall. We know to prune such empty directories when packing refs, but that\nonly patches over the issue.\n\nImprove this by removing empty parents when cleaning up still-locked\nreferences in `files_transaction_cleanup()`. This function is also\ncalled when preparing or committing the transaction, so this change also\nhelps when not explicitly aborting the transaction.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\nHi,\n\nthis issue is something we recently discovered in Gitaly. It's nothing\nworld breaking, but I think it makes sense to try and keep the refdb in\na as-clean-as-possible state anyway.\n\nThanks!\n\nPatrick\n---\n refs/files-backend.c  |  2 ++\n t/t1400-update-ref.sh | 19 +++++++++++++++++++\n 2 files changed, 21 insertions(+)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex bf6f89b1d19..00128f21832 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -2760,6 +2760,8 @@ static void files_transaction_cleanup(struct files_ref_store *refs,\n \n \t\tif (lock) {\n \t\t\tunlock_ref(lock);\n+\t\t\ttry_remove_empty_parents(refs, update->refname,\n+\t\t\t\t\t\t REMOVE_EMPTY_PARENTS_REF);\n \t\t\tupdate->backend_data = NULL;\n \t\t}\n \t}\ndiff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh\nindex e373d9108b6..c9893f65464 100755\n--- a/t/t1400-update-ref.sh\n+++ b/t/t1400-update-ref.sh\n@@ -2304,4 +2304,23 @@ test_expect_success 'update-ref should also create reflog for HEAD' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success REFFILES 'empty directories are pruned when aborting a transaction' '\n+\ttest_path_is_missing .git/refs/heads/nested &&\n+\tgit update-ref --stdin <<-EOF &&\n+\tcreate refs/heads/nested/something HEAD\n+\tprepare\n+\tabort\n+\tEOF\n+\ttest_path_is_missing .git/refs/heads/nested\n+'\n+\n+test_expect_success REFFILES 'empty directories are pruned when not committing' '\n+\ttest_path_is_missing .git/refs/heads/nested &&\n+\tgit update-ref --stdin <<-EOF &&\n+\tcreate refs/heads/nested/something HEAD\n+\tprepare\n+\tEOF\n+\ttest_path_is_missing .git/refs/heads/nested\n+'\n+\n test_done\n\n---\nbase-commit: 41905d60226a0346b22f0d0d99428c746a5a3b14\nchange-id: 20250708-b4-pks-reffiles-prune-empty-dirs-on-abort-15c7a7f279bd\n\n"},{"id":"521579","messageId":"xmqqzfdes63u.fsf@gitster.g","threadId":"63758","inReplyTo":"20250708-b4-pks-reffiles-prune-empty-dirs-on-abort-v1-1-3bae02e4f034@pks.im","subject":"Re: [PATCH] refs/files: remove empty parent dirs when ref creation fails","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-07-08T21:52:21Z","receivedAt":"2025-07-08T21:52:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n> When creating a new reference in the \"files\" backend we first create the\n> directory hierarchy for that reference, then create the lockfile for\n> that reference, and finally rename the lockfile into place. When the\n> transaction gets aborted we prune the lockfile, but we don't clean up\n> the directory hierarchy that we may have created for the lockfile.\n>\n> In some egde cases this can lead to lots of empty directories being\n> cluttered in the \".git/refs\" directory that really serve no purpose at\n> all. We know to prune such empty directories when packing refs, but that\n> only patches over the issue.\n>\n> Improve this by removing empty parents when cleaning up still-locked\n> references in `files_transaction_cleanup()`. This function is also\n> called when preparing or committing the transaction, so this change also\n> helps when not explicitly aborting the transaction.\n>\n> Signed-off-by: Patrick Steinhardt <ps@pks.im>\n> ---\n> Hi,\n>\n> this issue is something we recently discovered in Gitaly. It's nothing\n> world breaking, but I think it makes sense to try and keep the refdb in\n> a as-clean-as-possible state anyway.\n\nWould we lose an empty \".git/refs/tags/\" directory if we fail to\ncreate the first tag?\n\n\t... goes and looks at the try_remove function ...\n\nOK, we protect the leading two levels when doing so, so we are safe.\n\nWill queue.  Thanks.\n"}]}