git/list[1] front-page[2] threads[3] people[4] search[5] about
 

[PATCH v3 23/23] files_transaction_commit(): clean up empty directories

From
Michael Haggerty <mhagger@alum.mit.edu>
Date
Dec 31, 2016, 03:13 UTC
Message-ID
<732e8a06acd424f718add206214515919b107ebc.1483153436.git.mhagger@alum.mit.edu>
In-Reply-To
<cover.1483153436.git.mhagger@alum.mit.edu>

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>
---
 refs/files-backend.c  | 34 ++++++++++++++++++++++++++++------
 refs/refs-internal.h  | 11 +++++++++--
 t/t1400-update-ref.sh | 27 +++++++++++++++++++++++++++
 3 files changed, 64 insertions(+), 8 deletions(-)
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 2155acf..26cfea3 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2345,7 +2345,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)
@@ -3792,6 +3791,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))
@@ -3804,16 +3804,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 708b260..de49362 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -56,6 +56,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
  * example by causing a file that is not a reference to be deleted.
@@ -157,8 +163,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 d4fb977..97d8793 100755
--- 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

-- 
2.9.3
Previous: Jeff KingNext: Michael Haggerty
Message 35 of 55 in “Delete directories left empty after ref deletion”
  1. 00/23 Delete directories left empty after ref deletionMichael Haggerty, Dec 31, 2016
  2. 01/23 files_rename_ref(): tidy up whitespaceMichael Haggerty, Dec 31, 2016
  3. 02/23 t5505: use "for-each-ref" to test for the non-existence of referencesMichael Haggerty, Dec 31, 2016
  4. 03/23 safe_create_leading_directories_const(): preserve errnoMichael Haggerty, Dec 31, 2016
  5. 05/23 raceproof_create_file(): new functionMichael Haggerty, Dec 31, 2016
  6. Jeff KingDec 31, 2016
  7. Michael HaggertyDec 31, 2016
  8. Junio C HamanoJan 1, 2017
  9. 06/23 lock_ref_sha1_basic(): inline constantMichael Haggerty, Dec 31, 2016
  10. 07/23 lock_ref_sha1_basic(): use raceproof_create_file()Michael Haggerty, Dec 31, 2016
  11. 13/23 log_ref_setup(): pass the open file descriptor back to the callerMichael Haggerty, Dec 31, 2016
  12. Jeff KingDec 31, 2016
  13. Michael HaggertyDec 31, 2016
  14. Jeff KingDec 31, 2016
  15. Junio C HamanoJan 1, 2017
  16. 12/23 log_ref_setup(): improve robustness against racesMichael Haggerty, Dec 31, 2016
  17. 11/23 log_ref_setup(): separate code for create vs non-createMichael Haggerty, Dec 31, 2016
  18. Jeff KingDec 31, 2016
  19. Michael HaggertyDec 31, 2016
  20. Junio C HamanoJan 1, 2017
  21. Michael HaggertyJan 1, 2017
  22. 15/23 log_ref_setup(): manage the name of the reflog file internallyMichael Haggerty, Dec 31, 2016
  23. 16/23 log_ref_write_1(): inline functionMichael Haggerty, Dec 31, 2016
  24. 19/23 try_remove_empty_parents(): rename parameter "name" -> "refname"Michael Haggerty, Dec 31, 2016
  25. 20/23 try_remove_empty_parents(): don't trash argument contentsMichael Haggerty, Dec 31, 2016
  26. Jeff KingDec 31, 2016
  27. Michael HaggertyJan 2, 2017
  28. Jeff KingJan 2, 2017
  29. 22/23 try_remove_empty_parents(): teach to remove parents of reflogs, tooMichael Haggerty, Dec 31, 2016
  30. 21/23 try_remove_empty_parents(): don't accommodate consecutive slashesMichael Haggerty, Dec 31, 2016
  31. Junio C HamanoJan 1, 2017
  32. Jeff KingJan 1, 2017
  33. Michael HaggertyJan 2, 2017
  34. Jeff KingJan 2, 2017
  35. 23/23 files_transaction_commit(): clean up empty directoriesMichael Haggerty, Dec 31, 2016
  36. 04/23 safe_create_leading_directories(): set errno on SCLD_EXISTSMichael Haggerty, Dec 31, 2016
  37. 08/23 rename_tmp_log(): use raceproof_create_file()Michael Haggerty, Dec 31, 2016
  38. 14/23 log_ref_write_1(): don't depend on logfile argumentMichael Haggerty, Dec 31, 2016
  39. Jeff KingDec 31, 2016
  40. Michael HaggertyDec 31, 2016
  41. 17/23 delete_ref_loose(): derive loose reference path from lockMichael Haggerty, Dec 31, 2016
  42. 18/23 delete_ref_loose(): inline functionMichael Haggerty, Dec 31, 2016
  43. 09/23 rename_tmp_log(): improve error reportingMichael Haggerty, Dec 31, 2016
  44. 10/23 log_ref_write(): inline functionMichael Haggerty, Dec 31, 2016
  45. Junio C HamanoJan 1, 2017
  46. Michael HaggertyJan 1, 2017
  47. Jeff KingDec 31, 2016
  48. Junio C HamanoJan 1, 2017
  49. Jacob KellerJan 1, 2017
  50. Jacob KellerJan 1, 2017
  51. Philip OakleyJan 1, 2017
  52. Jacob KellerJan 1, 2017
  53. Jeff KingJan 2, 2017
  54. Michael HaggertyJan 2, 2017
  55. Jacob KellerJan 2, 2017

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.