{"thread":{"id":"24298","subject":"[PATCH] pack-refs: remove newly empty directories","startedAt":"2010-07-05T22:27:28Z","lastAt":"2010-07-06T23:29:19Z","messageCount":8,"participants":["Greg Price","Junio C Hamano","Johannes Sixt","Andreas Schwab"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"144854","messageId":"1278368848-7037-1-git-send-email-price@ksplice.com","threadId":"24298","inReplyTo":null,"subject":"[PATCH] pack-refs: remove newly empty directories","fromName":"Greg Price","fromEmail":"price@ksplice.com","sentAt":"2010-07-05T22:27:28Z","receivedAt":"2010-07-05T22:27:28Z","isPatch":true,"sender":{"key":"price@mit.edu","avatar":"https://avatars.githubusercontent.com/u/28173?v=4"},"body":"In a large repository which uses directories to organize many refs,\n\"git pack-refs --all --prune\" does not improve performance so much\nas it should, unless we remove all the now-empty directories as well.\n\nSigned-off-by: Greg Price \n---\n pack-refs.c | 8 ++++++++\n t/t3210-pack-refs.sh | 6 ++++++\n 2 files changed, 14 insertions(+), 0 deletions(-)\n\ndiff --git a/pack-refs.c b/pack-refs.c\nindex 7f43f8a..33d7358 100644\n--- a/pack-refs.c\n+++ b/pack-refs.c\n@@ -63,11 +63,19 @@ static int handle_one_ref(const char *path, const unsigned char *sha1,\n /* make sure nobody touched the ref, and unlink */\n static void prune_ref(struct ref_to_prune *r)\n {\n+\tchar *p, *q;\n \tstruct ref_lock *lock = lock_ref_sha1(r->name + 5, r->sha1);\n \n \tif (lock) {\n \t\tunlink_or_warn(git_path(\"%s\", r->name));\n \t\tunlock_ref(lock);\n+\n+\t\t/* remove the directory if empty */\n+\t\tfor (q = p = r->name; *p; p++)\n+\t\t\tif (*p == '/')\n+\t\t\t\tq = p;\n+\t\t*q = '\\0';\n+\t\trmdir(r->name);\n \t}\n }\n \ndiff --git a/t/t3210-pack-refs.sh b/t/t3210-pack-refs.sh\nindex 413019a..c60ede1 100755\n--- a/t/t3210-pack-refs.sh\n+++ b/t/t3210-pack-refs.sh\n@@ -60,6 +60,12 @@ test_expect_success 'see if git pack-refs --prune remove ref files' '\n ! test -f .git/refs/heads/f\n '\n \n+test_expect_success 'see if git pack-refs --prune removes empty dirs' '\n+ git branch r/s &&\n+ git pack-refs --all --prune &&\n+ ! test -e .git/refs/heads/r\n+'\n+\n test_expect_success \\\n 'git branch g should work when git branch g/h has been deleted' \\\n 'git branch g/h &&\n-- \n1.6.6.32.g6380e\n"},{"id":"144869","messageId":"7vsk3x5n35.fsf@alter.siamese.dyndns.org","threadId":"24298","inReplyTo":"1278368848-7037-1-git-send-email-price@ksplice.com","subject":"Re: [PATCH] pack-refs: remove newly empty directories","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-07-06T03:02:54Z","receivedAt":"2010-07-06T03:02:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Greg Price writes:\n\n> diff --git a/pack-refs.c b/pack-refs.c\n> index 7f43f8a..33d7358 100644\n> --- a/pack-refs.c\n> +++ b/pack-refs.c\n> @@ -63,11 +63,19 @@ static int handle_one_ref(const char *path, const unsigned char *sha1,\n> /* make sure nobody touched the ref, and unlink */\n> static void prune_ref(struct ref_to_prune *r)\n> {\n> +\tchar *p, *q;\n> \tstruct ref_lock *lock = lock_ref_sha1(r->name + 5, r->sha1);\n> \n> \tif (lock) {\n> \t\tunlink_or_warn(git_path(\"%s\", r->name));\n> \t\tunlock_ref(lock);\n> +\n> +\t\t/* remove the directory if empty */\n> +\t\tfor (q = p = r->name; *p; p++)\n> +\t\t\tif (*p == '/')\n> +\t\t\t\tq = p;\n> +\t\t*q = '\\0';\n> +\t\trmdir(r->name);\n> \t}\n> }\n\nWill this keep refs/heads/p/q that is empty after packing p/q/r/s branch\nthat happens to be the only branch whose name begins with p/?\n\nI do not want a careless loop that will remove refs/heads after packing\n\"master\" that happens to be the only local branch, but still...\n"},{"id":"144872","messageId":"AANLkTilDcpdekvsw9b4TN8QNpubs6wkpibXdzz2AkTf-@mail.gmail.com","threadId":"24298","inReplyTo":"7vsk3x5n35.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] pack-refs: remove newly empty directories","fromName":"Greg Price","fromEmail":"price@ksplice.com","sentAt":"2010-07-06T03:25:36Z","receivedAt":"2010-07-06T03:25:36Z","isPatch":true,"sender":{"key":"price@mit.edu","avatar":"https://avatars.githubusercontent.com/u/28173?v=4"},"body":"On Mon, Jul 5, 2010 at 11:02 PM, Junio C Hamano wrote:\n> Will this keep refs/heads/p/q that is empty after packing p/q/r/s branch\n> that happens to be the only branch whose name begins with p/?\n>\n> I do not want a careless loop that will remove refs/heads after packing\n> \"master\" that happens to be the only local branch, but still...\n\nIt will. I could fix that with something like this (untested):\n\n/* Remove empty parents, but spare refs/ and immediate subdirs.\n Note, munges *name. */\nstatic void try_remove_empty_parents(char *name)\n{\n char *p, *q;\n int i;\n p = name;\n for (i = 0; i < 2; i++) { /* refs/{heads,tags,...}/ */\n while (*p && *p != '/')\n p++;\n if (*p)\n p++;\n }\n for (q = p; *q; q++)\n ;\n while (1) {\n for ( ; q > p && *q != '/'; q--)\n ;\n if (q == p)\n break;\n *q = '\\0';\n if (rmdir(git_path(\"%s\", name)))\n break;\n }\n}\n\nand then\n\n if (lock) {\n unlink_or_warn(git_path(\"%s\", r->name));\n unlock_ref(lock);\n+ try_remove_empty_parents(r->name);\n }\n\nSound reasonable?\n\nGreg\n"},{"id":"144876","messageId":"4C32C8EB.1090104@viscovery.net","threadId":"24298","inReplyTo":"1278368848-7037-1-git-send-email-price@ksplice.com","subject":"Re: [PATCH] pack-refs: remove newly empty directories","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2010-07-06T06:10:51Z","receivedAt":"2010-07-06T06:10:51Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 7/6/2010 0:27, schrieb Greg Price:\n> In a large repository which uses directories to organize many refs,\n> \"git pack-refs --all --prune\" does not improve performance so much\n> as it should, unless we remove all the now-empty directories as well.\n\nBefore your patch, when you create a ref refs/heads/foo/bar, then pack\nrefs, then it is impossible to create a ref refs/heads/foo because\nrefs/heads/foo still exists as directory. And this is a good thing.\n\nWith your patch, is there any mechanism that inhibits that refs/heads/foo\nis created when directory refs/heads/foo does not exist, but a packed ref\nrefs/heads/foo/bar is present?\n\n-- Hannes\n"},{"id":"144877","messageId":"7vbpal5e6q.fsf@alter.siamese.dyndns.org","threadId":"24298","inReplyTo":"4C32C8EB.1090104@viscovery.net","subject":"Re: [PATCH] pack-refs: remove newly empty directories","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-07-06T06:15:09Z","receivedAt":"2010-07-06T06:15:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt writes:\n\n> With your patch, is there any mechanism that inhibits that refs/heads/foo\n> is created when directory refs/heads/foo does not exist, but a packed ref\n> refs/heads/foo/bar is present?\n\n$ git grep -e is_refname_available refs.c\n"},{"id":"144945","messageId":"m2y6doqwch.fsf@igel.home","threadId":"24298","inReplyTo":"1278368848-7037-1-git-send-email-price@ksplice.com","subject":"Re: [PATCH] pack-refs: remove newly empty directories","fromName":"Andreas Schwab","fromEmail":"schwab@linux-m68k.org","sentAt":"2010-07-06T18:49:34Z","receivedAt":"2010-07-06T18:49:34Z","isPatch":true,"sender":{"key":"schwab@linux-m68k.org","avatar":"https://avatars.githubusercontent.com/u/2175493?v=4"},"body":"Greg Price writes:\n\n> In a large repository which uses directories to organize many refs,\n> \"git pack-refs --all --prune\" does not improve performance so much\n> as it should, unless we remove all the now-empty directories as well.\n\nWhat happens if a parallel running git wants to update a ref in one of\nthe now-empty directories? Can it get a spurious error after it has\ncalled safe_create_leading_directories?\n\nAndreas.\n\n-- \nAndreas Schwab, schwab@linux-m68k.org\nGPG Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5\n\"And now for something completely different.\"\n"},{"id":"144949","messageId":"AANLkTilsfURYKOQC-kAtNrr1cTmULOXDTn0zxicgb9-S@mail.gmail.com","threadId":"24298","inReplyTo":"m2y6doqwch.fsf@igel.home","subject":"Re: [PATCH] pack-refs: remove newly empty directories","fromName":"Greg Price","fromEmail":"price@ksplice.com","sentAt":"2010-07-06T19:13:55Z","receivedAt":"2010-07-06T19:13:55Z","isPatch":true,"sender":{"key":"price@mit.edu","avatar":"https://avatars.githubusercontent.com/u/28173?v=4"},"body":"On Tue, Jul 6, 2010 at 2:49 PM, Andreas Schwab wrote:\n> What happens if a parallel running git wants to update a ref in one of\n> the now-empty directories?  Can it get a spurious error after it has\n> called safe_create_leading_directories?\n\nGood question! I asked the same question. =) It can get the same\nkind of error that happens if two parallel running git processes try\nto update the same ref.\n\nAnything that tries to update a ref will call lock_ref_sha1_basic(),\nwhich calls safe_create_leading_directories() to make the directory\nand then hold_lock_file_for_update() to open(O_CREAT|O_EXCL) a lock\nfile inside the directory. So if the prune happens after the open(),\nit will simply not remove the directory and all is well. If it\nhappens between the safe_create_leading_directories() and the open(),\nthen the open() will fail. This is also what happens if the lock has\nbeen taken by another git process updating the same ref. Currently\nlock_ref_sha1_basic() chooses to have the process die in this case and\nprint an error message.\n\nSimilarly the prune could remove a directory just before\nsafe_create_leading_directories() creates one inside it. Then\nsafe_create_leading_directories() will fail and loc_ref_sha1_basic()\nwill print an error message and return failure. In both cases, since\nwe are exposed to a similar failure if a parallel process is updating\nthe same ref (a common operation) rather than packing refs (an\nuncommon operation), I don't think this is a problem.\n\nGreg\n"},{"id":"144969","messageId":"1278458959-22252-1-git-send-email-price@ksplice.com","threadId":"24298","inReplyTo":"AANLkTilDcpdekvsw9b4TN8QNpubs6wkpibXdzz2AkTf-@mail.gmail.com","subject":"[PATCH v2] pack-refs: remove newly empty directories","fromName":"Greg Price","fromEmail":"price@ksplice.com","sentAt":"2010-07-06T23:29:19Z","receivedAt":"2010-07-06T23:29:19Z","isPatch":true,"sender":{"key":"price@mit.edu","avatar":"https://avatars.githubusercontent.com/u/28173?v=4"},"body":"In a large repository which uses directories to organize many refs,\n\"git pack-refs --all --prune\" does not improve performance so much\nas it should, unless we remove all the now-empty directories as well.\n\nSigned-off-by: Greg Price \n---\nThis version removes empty grandparent directories, etc, but always\nleaves in place refs/heads/ and its siblings.\n\nWe also tolerate duplicate slashes in refnames, because\ncheck_ref_format() in refs.c does the same.\n\n pack-refs.c | 30 ++++++++++++++++++++++++++++++\n t/t3210-pack-refs.sh | 6 ++++++\n 2 files changed, 36 insertions(+), 0 deletions(-)\n\ndiff --git a/pack-refs.c b/pack-refs.c\nindex 7f43f8a..a935856 100644\n--- a/pack-refs.c\n+++ b/pack-refs.c\n@@ -60,6 +60,35 @@ static int handle_one_ref(const char *path, const unsigned char *sha1,\n \treturn 0;\n }\n \n+/* Remove empty parents, but spare refs/ and immediate subdirs.\n+ Note, munges *name. */\n+static void try_remove_empty_parents(char *name)\n+{\n+\tchar *p, *q;\n+\tint i;\n+\tp = name;\n+\tfor (i = 0; i < 2; i++) { /* refs/{heads,tags,...}/ */\n+\t\twhile (*p && *p != '/')\n+\t\t\tp++;\n+\t\t/* tolerate duplicate slashes; see check_ref_format() */\n+\t\twhile (*p == '/')\n+\t\t\tp++;\n+\t}\n+\tfor (q = p; *q; q++)\n+\t\t;\n+\twhile (1) {\n+\t\twhile (q > p && *q != '/')\n+\t\t\tq--;\n+\t\twhile (q > p && *(q-1) == '/')\n+\t\t\tq--;\n+\t\tif (q == p)\n+\t\t\tbreak;\n+\t\t*q = '\\0';\n+\t\tif (rmdir(git_path(\"%s\", name)))\n+\t\t\tbreak;\n+\t}\n+}\n+\n /* make sure nobody touched the ref, and unlink */\n static void prune_ref(struct ref_to_prune *r)\n {\n@@ -68,6 +97,7 @@ static void prune_ref(struct ref_to_prune *r)\n \tif (lock) {\n \t\tunlink_or_warn(git_path(\"%s\", r->name));\n \t\tunlock_ref(lock);\n+\t\ttry_remove_empty_parents(r->name);\n \t}\n }\n \ndiff --git a/t/t3210-pack-refs.sh b/t/t3210-pack-refs.sh\nindex 413019a..ffd4e9f 100755\n--- a/t/t3210-pack-refs.sh\n+++ b/t/t3210-pack-refs.sh\n@@ -60,6 +60,12 @@ test_expect_success 'see if git pack-refs --prune remove ref files' '\n ! test -f .git/refs/heads/f\n '\n \n+test_expect_success 'see if git pack-refs --prune removes empty dirs' '\n+ git branch r/s/t &&\n+ git pack-refs --all --prune &&\n+ ! test -e .git/refs/heads/r\n+'\n+\n test_expect_success \\\n 'git branch g should work when git branch g/h has been deleted' \\\n 'git branch g/h &&\n-- \n1.6.6.32.g6380e\n"}]}