{"thread":{"id":"55644","subject":"[PATCH] refs: cleanup directories when deleting packed ref","startedAt":"2021-05-07T14:37:30Z","lastAt":"2021-05-11T05:01:11Z","messageCount":12,"participants":["Will Chandler","Jeff King","Bagas Sanjaya","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"423874","messageId":"YJVQpaDwkQH/aCee@mini.wfchandler.org","threadId":"55644","inReplyTo":null,"subject":"[PATCH] refs: cleanup directories when deleting packed ref","fromName":"Will Chandler","fromEmail":"wfc@wfchandler.org","sentAt":"2021-05-07T14:37:25Z","receivedAt":"2021-05-07T14:37:30Z","isPatch":true,"sender":{"key":"wfc@wfchandler.org","avatar":null},"body":"When deleting a packed ref, a lockfile is made in the directory that\nwould contain the loose copy of that ref, creating any directories in\nthe ref's path that do not exist. When the transaction completes, the\nlockfile is deleted, but any empty parent directories made when creating\nthe lockfile are left in place.  These empty directories are not removed\nby 'pack-refs' or other housekeeping tasks and will accumulate over\ntime.\n\nWhen deleting a loose ref, we remove all empty parent directories at the\nend of the transaction.\n\nThis commit applies the parent directory cleanup logic used when\ndeleting loose refs to packed refs as well.\n\nSigned-off-by: Will Chandler <wfc@wfchandler.org>\n---\n refs/files-backend.c  | 12 ++++++------\n t/t1400-update-ref.sh |  9 +++++++++\n 2 files changed, 15 insertions(+), 6 deletions(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 119972ee16..49e6ee069a 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -45,10 +45,10 @@\n #define REF_UPDATE_VIA_HEAD (1 << 8)\n \n /*\n- * Used as a flag in ref_update::flags when the loose reference has\n- * been deleted.\n+ * Used as a flag in ref_update::flags when a reference has been\n+ * deleted and the ref's parent directories may need cleanup.\n  */\n-#define REF_DELETED_LOOSE (1 << 9)\n+#define REF_DELETED_RMDIR (1 << 9)\n \n struct ref_lock {\n \tchar *ref_name;\n@@ -2852,6 +2852,7 @@ static int files_transaction_finish(struct ref_store *ref_store,\n \n \t\tif (update->flags & REF_DELETING &&\n \t\t    !(update->flags & REF_LOG_ONLY)) {\n+\t\t\tupdate->flags |= REF_DELETED_RMDIR;\n \t\t\tif (!(update->type & REF_ISPACKED) ||\n \t\t\t    update->type & REF_ISSYMREF) {\n \t\t\t\t/* It is a loose reference. */\n@@ -2861,7 +2862,6 @@ static int files_transaction_finish(struct ref_store *ref_store,\n \t\t\t\t\tret = TRANSACTION_GENERIC_ERROR;\n \t\t\t\t\tgoto cleanup;\n \t\t\t\t}\n-\t\t\t\tupdate->flags |= REF_DELETED_LOOSE;\n \t\t\t}\n \t\t}\n \t}\n@@ -2874,9 +2874,9 @@ static int files_transaction_finish(struct ref_store *ref_store,\n \tfor (i = 0; i < transaction->nr; i++) {\n \t\tstruct ref_update *update = transaction->updates[i];\n \n-\t\tif (update->flags & REF_DELETED_LOOSE) {\n+\t\tif (update->flags & REF_DELETED_RMDIR) {\n \t\t\t/*\n-\t\t\t * The loose reference was deleted. Delete any\n+\t\t\t * The reference was deleted. Delete any\n \t\t\t * empty parent directories. (Note that this\n \t\t\t * can only work because we have already\n \t\t\t * removed the lockfile.)\ndiff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh\nindex e31f65f381..adae9ef91f 100755\n--- a/t/t1400-update-ref.sh\n+++ b/t/t1400-update-ref.sh\n@@ -1598,4 +1598,13 @@ test_expect_success 'transaction cannot restart ongoing transaction' '\n \ttest_must_fail git show-ref --verify refs/heads/restart\n '\n \n+test_expect_success 'directory not created deleting packed ref' '\n+\tgit branch d1/d2/r1 HEAD &&\n+\tgit pack-refs --all &&\n+\ttest_path_is_missing .git/refs/heads/d1/d2 &&\n+\tgit branch -d d1/d2/r1 &&\n+\ttest_path_is_missing .git/refs/heads/d1/d2 &&\n+\ttest_path_is_missing .git/refs/heads/d1\n+'\n+\n test_done\n-- \n2.30.2\n\n"},{"id":"423881","messageId":"YJW3n6lVWVAuLQap@coredump.intra.peff.net","threadId":"55644","inReplyTo":"YJVQpaDwkQH/aCee@mini.wfchandler.org","subject":"Re: [PATCH] refs: cleanup directories when deleting packed ref","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-05-07T21:56:47Z","receivedAt":"2021-05-07T21:56:50Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, May 07, 2021 at 10:37:25AM -0400, Will Chandler wrote:\n\n> When deleting a packed ref, a lockfile is made in the directory that\n> would contain the loose copy of that ref, creating any directories in\n> the ref's path that do not exist. When the transaction completes, the\n> lockfile is deleted, but any empty parent directories made when creating\n> the lockfile are left in place.  These empty directories are not removed\n> by 'pack-refs' or other housekeeping tasks and will accumulate over\n> time.\n> \n> When deleting a loose ref, we remove all empty parent directories at the\n> end of the transaction.\n> \n> This commit applies the parent directory cleanup logic used when\n> deleting loose refs to packed refs as well.\n\nHmm. I can certainly believe that such a bug exists, but...\n\n> +test_expect_success 'directory not created deleting packed ref' '\n> +\tgit branch d1/d2/r1 HEAD &&\n> +\tgit pack-refs --all &&\n> +\ttest_path_is_missing .git/refs/heads/d1/d2 &&\n> +\tgit branch -d d1/d2/r1 &&\n> +\ttest_path_is_missing .git/refs/heads/d1/d2 &&\n> +\ttest_path_is_missing .git/refs/heads/d1\n> +'\n\n...this test passes even without your patch applied. I wonder if there's\nsomething else required to trigger the problem.\n\n-Peff\n"},{"id":"423882","messageId":"YJW46fsdKaUv2Fln@coredump.intra.peff.net","threadId":"55644","inReplyTo":"YJW3n6lVWVAuLQap@coredump.intra.peff.net","subject":"Re: [PATCH] refs: cleanup directories when deleting packed ref","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-05-07T22:02:17Z","receivedAt":"2021-05-07T22:02:19Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, May 07, 2021 at 05:56:47PM -0400, Jeff King wrote:\n\n> > +test_expect_success 'directory not created deleting packed ref' '\n> > +\tgit branch d1/d2/r1 HEAD &&\n> > +\tgit pack-refs --all &&\n> > +\ttest_path_is_missing .git/refs/heads/d1/d2 &&\n> > +\tgit branch -d d1/d2/r1 &&\n> > +\ttest_path_is_missing .git/refs/heads/d1/d2 &&\n> > +\ttest_path_is_missing .git/refs/heads/d1\n> > +'\n> \n> ...this test passes even without your patch applied. I wonder if there's\n> something else required to trigger the problem.\n\nIf I replace \"git branch -d\" with \"git update-ref -d\", then the problem\ndoes trigger (and your patch does indeed clear it up). I wonder what the\ndifference is.\n\n-Peff\n"},{"id":"423886","messageId":"YJXF4xA0GFx2/2v4@coredump.intra.peff.net","threadId":"55644","inReplyTo":"YJW46fsdKaUv2Fln@coredump.intra.peff.net","subject":"Re: [PATCH] refs: cleanup directories when deleting packed ref","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-05-07T22:57:39Z","receivedAt":"2021-05-07T22:57:42Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, May 07, 2021 at 06:02:17PM -0400, Jeff King wrote:\n\n> On Fri, May 07, 2021 at 05:56:47PM -0400, Jeff King wrote:\n> \n> > > +test_expect_success 'directory not created deleting packed ref' '\n> > > +\tgit branch d1/d2/r1 HEAD &&\n> > > +\tgit pack-refs --all &&\n> > > +\ttest_path_is_missing .git/refs/heads/d1/d2 &&\n> > > +\tgit branch -d d1/d2/r1 &&\n> > > +\ttest_path_is_missing .git/refs/heads/d1/d2 &&\n> > > +\ttest_path_is_missing .git/refs/heads/d1\n> > > +'\n> > \n> > ...this test passes even without your patch applied. I wonder if there's\n> > something else required to trigger the problem.\n> \n> If I replace \"git branch -d\" with \"git update-ref -d\", then the problem\n> does trigger (and your patch does indeed clear it up). I wonder what the\n> difference is.\n\nI think this comes down to the interfaces. In update-ref, we call\ndelete_ref(), which creates a transaction to delete the single ref. It\nrealizes the ref is packed and there is no loose file to delete.\n\nWhereas in git-branch, call the plural delete_refs(), which handles the\npacked and loose stores separately. It first deletes everything from the\npacked ref store in one go, and then the loose store. And it's actually\nthe deletion from the loose store which gets weird. The ref isn't\n_anywhere_ at this point. So when we try to read it, we don't set the\nREF_ISPACKED flag. And thus when it comes time to clean up the loose\nref, we say \"not packed, so I guess it's worth calling unlink()\". Of\ncourse that syscall fails, but our unlink_or_msg() wrapper turns ENOENT\ninto success (which is sensible; we want it to be gone, and it is).\n\nAnd so we think we've deleted a loose ref, and thus call\ntry_remove_empty_parents(), which cleans up the extra directory.\n\nSo I'd argue that it's actually delete_refs() which is called from\ngit-branch that is a little confused, or possibly even buggy. But I\ndon't think it's hurting anything, and working around it would probably\nbe awkward and/or inefficient.\n\nGetting back to your patch, though, you are definitely fixing a problem\nwith update-ref (which correctly realizes there is no loose ref to clean\nup, but forgets that we had to make a lockfile). And the solution you\nhave looks correct. I think you just need to update the test to exercise\nit with \"update-ref -d\".\n\n-Peff\n"},{"id":"423912","messageId":"YJYTQYk5q8g6HaIm@mini.wfchandler.org","threadId":"55644","inReplyTo":"YJXF4xA0GFx2/2v4@coredump.intra.peff.net","subject":"Re: [PATCH] refs: cleanup directories when deleting packed ref","fromName":"Will Chandler","fromEmail":"wfc@wfchandler.org","sentAt":"2021-05-08T04:27:45Z","receivedAt":"2021-05-08T04:28:37Z","isPatch":true,"sender":{"key":"wfc@wfchandler.org","avatar":null},"body":"On Fri, May 07, 2021 at 06:57:39PM -0400, Jeff King wrote:\n> Getting back to your patch, though, you are definitely fixing a problem\n> with update-ref (which correctly realizes there is no loose ref to clean\n> up, but forgets that we had to make a lockfile). And the solution you\n> have looks correct. I think you just need to update the test to exercise\n> it with \"update-ref -d\".\n\nThanks for the thorough analysis! Apologies for the confusion, I really\nappreciate your patience.\n\nI'll get the test fixed and note that this is specific to 'update-ref'.\n"},{"id":"423914","messageId":"YJYa+7yUyt2YD16r@mini.wfchandler.org","threadId":"55644","inReplyTo":"YJYTQYk5q8g6HaIm@mini.wfchandler.org","subject":"Re: [PATCH] refs: cleanup directories when deleting packed ref","fromName":"Will Chandler","fromEmail":"wfc@wfchandler.org","sentAt":"2021-05-08T05:00:43Z","receivedAt":"2021-05-08T05:03:40Z","isPatch":true,"sender":{"key":"wfc@wfchandler.org","avatar":null},"body":"Subject: [PATCH v2] refs: cleanup directories when deleting packed ref\n\nWhen deleting a packed ref via 'update-ref -d', a lockfile is made in\nthe directory that would contain the loose copy of that ref, creating\nany directories in the ref's path that do not exist. When the\ntransaction completes, the lockfile is deleted, but any empty parent\ndirectories made when creating the lockfile are left in place.  These\nempty directories are not removed by 'pack-refs' or other housekeeping\ntasks and will accumulate over time.\n\nWhen deleting a loose ref, we remove all empty parent directories at the\nend of the transaction.\n\nThis commit applies the parent directory cleanup logic used when\ndeleting loose refs to packed refs as well.\n\nSigned-off-by: Will Chandler <wfc@wfchandler.org>\n---\n refs/files-backend.c  | 12 ++++++------\n t/t1400-update-ref.sh |  9 +++++++++\n 2 files changed, 15 insertions(+), 6 deletions(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 119972ee16..49e6ee069a 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -45,10 +45,10 @@\n #define REF_UPDATE_VIA_HEAD (1 << 8)\n \n /*\n- * Used as a flag in ref_update::flags when the loose reference has\n- * been deleted.\n+ * Used as a flag in ref_update::flags when a reference has been\n+ * deleted and the ref's parent directories may need cleanup.\n  */\n-#define REF_DELETED_LOOSE (1 << 9)\n+#define REF_DELETED_RMDIR (1 << 9)\n \n struct ref_lock {\n \tchar *ref_name;\n@@ -2852,6 +2852,7 @@ static int files_transaction_finish(struct ref_store *ref_store,\n \n \t\tif (update->flags & REF_DELETING &&\n \t\t    !(update->flags & REF_LOG_ONLY)) {\n+\t\t\tupdate->flags |= REF_DELETED_RMDIR;\n \t\t\tif (!(update->type & REF_ISPACKED) ||\n \t\t\t    update->type & REF_ISSYMREF) {\n \t\t\t\t/* It is a loose reference. */\n@@ -2861,7 +2862,6 @@ static int files_transaction_finish(struct ref_store *ref_store,\n \t\t\t\t\tret = TRANSACTION_GENERIC_ERROR;\n \t\t\t\t\tgoto cleanup;\n \t\t\t\t}\n-\t\t\t\tupdate->flags |= REF_DELETED_LOOSE;\n \t\t\t}\n \t\t}\n \t}\n@@ -2874,9 +2874,9 @@ static int files_transaction_finish(struct ref_store *ref_store,\n \tfor (i = 0; i < transaction->nr; i++) {\n \t\tstruct ref_update *update = transaction->updates[i];\n \n-\t\tif (update->flags & REF_DELETED_LOOSE) {\n+\t\tif (update->flags & REF_DELETED_RMDIR) {\n \t\t\t/*\n-\t\t\t * The loose reference was deleted. Delete any\n+\t\t\t * The reference was deleted. Delete any\n \t\t\t * empty parent directories. (Note that this\n \t\t\t * can only work because we have already\n \t\t\t * removed the lockfile.)\ndiff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh\nindex e31f65f381..4506cd435b 100755\n--- a/t/t1400-update-ref.sh\n+++ b/t/t1400-update-ref.sh\n@@ -1598,4 +1598,13 @@ test_expect_success 'transaction cannot restart ongoing transaction' '\n \ttest_must_fail git show-ref --verify refs/heads/restart\n '\n \n+test_expect_success 'directory not created deleting packed ref' '\n+\tgit branch d1/d2/r1 HEAD &&\n+\tgit pack-refs --all &&\n+\ttest_path_is_missing .git/refs/heads/d1/d2 &&\n+\tgit update-ref -d refs/heads/d1/d2/r1 &&\n+\ttest_path_is_missing .git/refs/heads/d1/d2 &&\n+\ttest_path_is_missing .git/refs/heads/d1\n+'\n+\n test_done\n-- \n2.30.2\n\n"},{"id":"423915","messageId":"1bb65e54-a18d-852d-bb01-130cc57cce1b@gmail.com","threadId":"55644","inReplyTo":"YJYa+7yUyt2YD16r@mini.wfchandler.org","subject":"Re: [PATCH] refs: cleanup directories when deleting packed ref","fromName":"Bagas Sanjaya","fromEmail":"bagasdotme@gmail.com","sentAt":"2021-05-08T05:21:23Z","receivedAt":"2021-05-08T05:21:30Z","isPatch":true,"sender":{"key":"bagasdotme@gmail.com","avatar":"https://avatars.githubusercontent.com/u/40219486?v=4"},"body":"On 08/05/21 12.00, Will Chandler wrote:\n> Subject: [PATCH v2] refs: cleanup directories when deleting packed ref\n> \n> When deleting a packed ref via 'update-ref -d', a lockfile is made in\n> the directory that would contain the loose copy of that ref, creating\n> any directories in the ref's path that do not exist. When the\n> transaction completes, the lockfile is deleted, but any empty parent\n> directories made when creating the lockfile are left in place.  These\n> empty directories are not removed by 'pack-refs' or other housekeeping\n> tasks and will accumulate over time.\n> \n> When deleting a loose ref, we remove all empty parent directories at the\n> end of the transaction.\n> \n> This commit applies the parent directory cleanup logic used when\n> deleting loose refs to packed refs as well.\n> \n> Signed-off-by: Will Chandler <wfc@wfchandler.org>\n> ---\n>   refs/files-backend.c  | 12 ++++++------\n>   t/t1400-update-ref.sh |  9 +++++++++\n>   2 files changed, 15 insertions(+), 6 deletions(-)\n> \n> diff --git a/refs/files-backend.c b/refs/files-backend.c\n> index 119972ee16..49e6ee069a 100644\n> --- a/refs/files-backend.c\n> +++ b/refs/files-backend.c\n> @@ -45,10 +45,10 @@\n>   #define REF_UPDATE_VIA_HEAD (1 << 8)\n>   \n>   /*\n> - * Used as a flag in ref_update::flags when the loose reference has\n> - * been deleted.\n> + * Used as a flag in ref_update::flags when a reference has been\n> + * deleted and the ref's parent directories may need cleanup.\n>    */\n> -#define REF_DELETED_LOOSE (1 << 9)\n> +#define REF_DELETED_RMDIR (1 << 9)\n>   \n>   struct ref_lock {\n>   \tchar *ref_name;\n> @@ -2852,6 +2852,7 @@ static int files_transaction_finish(struct ref_store *ref_store,\n>   \n>   \t\tif (update->flags & REF_DELETING &&\n>   \t\t    !(update->flags & REF_LOG_ONLY)) {\n> +\t\t\tupdate->flags |= REF_DELETED_RMDIR;\n>   \t\t\tif (!(update->type & REF_ISPACKED) ||\n>   \t\t\t    update->type & REF_ISSYMREF) {\n>   \t\t\t\t/* It is a loose reference. */\n> @@ -2861,7 +2862,6 @@ static int files_transaction_finish(struct ref_store *ref_store,\n>   \t\t\t\t\tret = TRANSACTION_GENERIC_ERROR;\n>   \t\t\t\t\tgoto cleanup;\n>   \t\t\t\t}\n> -\t\t\t\tupdate->flags |= REF_DELETED_LOOSE;\n>   \t\t\t}\n>   \t\t}\n>   \t}\n> @@ -2874,9 +2874,9 @@ static int files_transaction_finish(struct ref_store *ref_store,\n>   \tfor (i = 0; i < transaction->nr; i++) {\n>   \t\tstruct ref_update *update = transaction->updates[i];\n>   \n> -\t\tif (update->flags & REF_DELETED_LOOSE) {\n> +\t\tif (update->flags & REF_DELETED_RMDIR) {\n>   \t\t\t/*\n> -\t\t\t * The loose reference was deleted. Delete any\n> +\t\t\t * The reference was deleted. Delete any\n>   \t\t\t * empty parent directories. (Note that this\n>   \t\t\t * can only work because we have already\n>   \t\t\t * removed the lockfile.)\n> diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh\n> index e31f65f381..4506cd435b 100755\n> --- a/t/t1400-update-ref.sh\n> +++ b/t/t1400-update-ref.sh\n> @@ -1598,4 +1598,13 @@ test_expect_success 'transaction cannot restart ongoing transaction' '\n>   \ttest_must_fail git show-ref --verify refs/heads/restart\n>   '\n>   \n> +test_expect_success 'directory not created deleting packed ref' '\n> +\tgit branch d1/d2/r1 HEAD &&\n> +\tgit pack-refs --all &&\n> +\ttest_path_is_missing .git/refs/heads/d1/d2 &&\n> +\tgit update-ref -d refs/heads/d1/d2/r1 &&\n> +\ttest_path_is_missing .git/refs/heads/d1/d2 &&\n> +\ttest_path_is_missing .git/refs/heads/d1\n> +'\n> +\n>   test_done\n>\n\nI ask to you: why did you send v2 patch as reply to v1?\n\nSupposed that I interested to apply only this v2, instead of v1.\nWith this situation, I downloaded mbox for v1, which contains v2\npatch as reply to v1. And git-am would instead apply v1 instead.\n\nSo why not send this v2 as separate message-id?\n\n-- \nAn old man doll... just what I always wanted! - Clara\n"},{"id":"423918","messageId":"xmqqmtt5vj2x.fsf@gitster.g","threadId":"55644","inReplyTo":"1bb65e54-a18d-852d-bb01-130cc57cce1b@gmail.com","subject":"Re: [PATCH] refs: cleanup directories when deleting packed ref","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-05-08T06:24:38Z","receivedAt":"2021-05-08T06:24:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Bagas Sanjaya <bagasdotme@gmail.com> writes:\n\n> I ask to you: why did you send v2 patch as reply to v1?\n\nBecause it is a natural progression of the ongoing discussion, I\nwould presume?\n\n> Supposed that I interested to apply only this v2, instead of v1.\n> With this situation, I downloaded mbox for v1, which contains v2\n> patch as reply to v1. And git-am would instead apply v1 instead.\n\nIf you are interested in v2, fetch individual pieces of e-mail for\nv2 and not v1 (in this case, a single message that contains v2\nonly).  If you feed v1 to \"am\", of course it would happily apply\nit.  Or if you grab the whole discussion in a mbox, of couse you'd\nneed to pick the message you would want to use (e.g. \"mailx -f mbox\"\nand choose individual messages and then tell it to \"s\"ave).\n\n> So why not send this v2 as separate message-id?\n\nI sense you are somehow upset or frustrated, but I do not see a need\nto behave so rudely to your fellow community member(s).\n\nBy the way, kostantin, \"b4 am\" seems to mishandle this thread.  As I\nunderstand it, it is supposed to grab the latest and greatest\niteration given any message in the discussion thread, right?  In\nthis thread, we discuss a single-patch \"series\" but with two\niterations.  The original submission is followed by some discussion\nmessages, that is followed by a v2 patch, and then a few more\nnon-patch messaages.\n\nBetween these two invocations\n\n$ b4 am YJVQpaDwkQH/aCee@mini.wfchandler.org\n$ b4 am YJYa+7yUyt2YD16r@mini.wfchandler.org\n\nwhere the first one is the the original, and the second one is the\nimproved v2 patch, both requests end up fetching the original one.\n\nOne thing curious about the behaviour (see the transcript at the\nend) is that it gives this \"NOTE\" about \"trailers\", which hints me\nthat this vintage of \"b4\" may not understand the in-body header like\n\"git am\" and \"git mailinfo\" do?  The v2 patch begins with a Subject:\nin-body header to retitle the patch.\n\nFWIW, I am using prepackaged /usr/bin/b4 at a work machine, which\nidentifies itself as\n\n$ b4 --version\n0.6.2\n\nThanks.\n\n\n---------------------- >8 ----------------------\n\n$ b4 am YJVQpaDwkQH/aCee@mini.wfchandler.org\nLooking up https://lore.kernel.org/r/YJVQpaDwkQH%2FaCee%40mini.wfchandler.org\nGrabbing thread from lore.kernel.org/git\nAnalyzing 7 messages in the thread\n---\nWriting ./20210507_wfc_refs_cleanup_directories_when_deleting_packed_ref.mbx\n  ✓ [PATCH] refs: cleanup directories when deleting packed ref\n  ---\n  ✓ Attestation-by: DKIM/wfchandler.org (From: wfc@wfchandler.org)\n---\nTotal patches: 1\n---\nNOTE: some trailers ignored due to from/email mismatches:\n    ! Trailer: Subject: [PATCH v2] refs: cleanup directories when deleting packed ref\n     Msg From: Will Chandler <wfc@wfchandler.org>\nNOTE: Rerun with -S to apply them anyway\n---\n Link: https://lore.kernel.org/r/YJVQpaDwkQH/aCee@mini.wfchandler.org\n Base: not found (applies clean to current tree)\n       git am ./20210507_wfc_refs_cleanup_directories_when_deleting_packed_ref.mbx\n\n\n\n\n\n\n"},{"id":"423975","messageId":"YJgt2UzAdhFfrDHm@mini.wfchandler.org","threadId":"55644","inReplyTo":"xmqqmtt5vj2x.fsf@gitster.g","subject":"Re: [PATCH] refs: cleanup directories when deleting packed ref","fromName":"Will Chandler","fromEmail":"wfc@wfchandler.org","sentAt":"2021-05-09T18:45:45Z","receivedAt":"2021-05-09T18:45:50Z","isPatch":true,"sender":{"key":"wfc@wfchandler.org","avatar":null},"body":"Thank you, Junio. Would it be helpful if I sent a separate '[PATCH v2]'\nemail?\n\nApologies for not following the correct procedure for the revised patch.\nI was relying on 'SubmittingPatches', but I've since found the detailed\ninstructions on submitting revisions in 'MyFirstContribution.txt'.\n"},{"id":"423986","messageId":"xmqqzgx3s81m.fsf@gitster.g","threadId":"55644","inReplyTo":"YJgt2UzAdhFfrDHm@mini.wfchandler.org","subject":"Re: [PATCH] refs: cleanup directories when deleting packed ref","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-05-10T01:15:49Z","receivedAt":"2021-05-10T01:15:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Will Chandler <wfc@wfchandler.org> writes:\n\n> Thank you, Junio. Would it be helpful if I sent a separate '[PATCH v2]'\n> email?\n>\n> Apologies for not following the correct procedure for the revised patch.\n> I was relying on 'SubmittingPatches', but I've since found the detailed\n> instructions on submitting revisions in 'MyFirstContribution.txt'.\n\nI am not sure if there is anything to apologize for on your part.\nThe procedures we use have grown over time, and there certainly\nwould be documentation gaps.\n\nI think the best current practice is\n\n1. In a thread that originates at a non-patch message, or in a\n   review discussion thread for a patch, it is welcome to use \"How\n   about doing it this way?\" patches as an illustration to explain\n   your idea in a more concrete way than just in prose.  But it is\n   unwelcome to leave the patch buried in the discussion, without\n   making it easier to find it (see 3.)\n\n2. In such a thread, \"By the way, I thought of this unrelated\n   tangent, and here is a patch to demonstrate the idea\" is not\n   entirely unwelcome, but keep it brief and make sure you get out\n   of the thread quickly to avoid distracting the main discussion.\n\n3. In either case, it makes it easier to find the final submission\n   of the patch if it is not buried deep in the discussion.\n\n   a. It is OK to start a new thread (without in-reply-to), with\n      \"here is a polished version of the patch and/or the idea I\n      floated in <message-id>\" under the three-dash line (for a\n      single patch) or in the cover letter (for a series).\n\n   b. An updated iteration of a multi-patch series sometimes is made\n      as a direct response to the cover letter of the previous\n      iteration (iow, the cover letter for vN+1 has the message id\n      of the cover letter for vN on its in-reply-to header).\n\nI think the \"b4\" tool prefers 3b. over 3a., and it may be also easy\non human readers who read the list in threaded mail/news reader.\n\nThanks.\n"},{"id":"424087","messageId":"YJnfV7op4yhyLqdg@coredump.intra.peff.net","threadId":"55644","inReplyTo":"YJYa+7yUyt2YD16r@mini.wfchandler.org","subject":"Re: [PATCH] refs: cleanup directories when deleting packed ref","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-05-11T01:35:19Z","receivedAt":"2021-05-11T01:35:23Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, May 08, 2021 at 01:00:43AM -0400, Will Chandler wrote:\n\n> Subject: [PATCH v2] refs: cleanup directories when deleting packed ref\n> \n> When deleting a packed ref via 'update-ref -d', a lockfile is made in\n> the directory that would contain the loose copy of that ref, creating\n> any directories in the ref's path that do not exist. When the\n> transaction completes, the lockfile is deleted, but any empty parent\n> directories made when creating the lockfile are left in place.  These\n> empty directories are not removed by 'pack-refs' or other housekeeping\n> tasks and will accumulate over time.\n> \n> When deleting a loose ref, we remove all empty parent directories at the\n> end of the transaction.\n> \n> This commit applies the parent directory cleanup logic used when\n> deleting loose refs to packed refs as well.\n\nThis thread got off on a tangent about threads, but to get back to the\nmain idea: this v2 patch looks good to me. Thank you very much for\nfinding and fixing it.\n\nRegarding threads, I agree with everything Junio already said. My only\nsuggestion would be to make sure the \"Subject\" above becomes the actual\nemail subject, rather than an in-body header (that may be what confused\nthe b4 tool, but more importantly, threaded readers like mutt will show\nthe changed subject in the thread index, making it very clear at a\nglance that there is a v2 and not just more discussion).\n\n-Peff\n"},{"id":"424100","messageId":"xmqqk0o5ooin.fsf@gitster.g","threadId":"55644","inReplyTo":"YJnfV7op4yhyLqdg@coredump.intra.peff.net","subject":"Re: [PATCH] refs: cleanup directories when deleting packed ref","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-05-11T04:58:08Z","receivedAt":"2021-05-11T05:01:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> This thread got off on a tangent about threads, but to get back to the\n> main idea: this v2 patch looks good to me. Thank you very much for\n> finding and fixing it.\n\nThanks.  But now we have a harder time to find that v2 patch X-<.\n\n"}]}