{"thread":{"id":"29596","subject":"[PATCH] Remove empty ref directories while reading loose refs","startedAt":"2012-02-10T16:25:27Z","lastAt":"2012-02-11T17:59:12Z","messageCount":10,"participants":["Nguyễn Thái Ngọc Duy","Junio C Hamano","Jeff King","Nguyen Thai Ngoc Duy","Thomas Adam"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"184359","messageId":"1328891127-17150-1-git-send-email-pclouds@gmail.com","threadId":"29596","inReplyTo":null,"subject":"[PATCH] Remove empty ref directories while reading loose refs","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2012-02-10T16:25:27Z","receivedAt":"2012-02-10T16:25:27Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"Empty directories in $GIT_DIR/refs increases overhead at startup.\nRemoving a ref does not remove its parent directories even if it's the\nonly file left so empty directories will be hanging around.\n\npack-refs was taught of cleaning up empty directories in be7c6d4\n(pack-refs: remove newly empty directories - 2010-07-06), but it only\nchecks parent directories of packed refs only. Already empty dirs are\nleft untouched.\n\nThis patch removes empty directories as we see while traversing\n$GIT_DIR/refs and reverts be7c6d4 because it's no longer needed.\n\nSome directories, even if empty, are not removed:\n\n - refs: this one is needed to recognize a git repository\n - refs/heads and refs/tags: these are created by init-db, people may\n   expect them to always be there\n\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n I don't think the a few extra rmdir()s from time to time at startup\n are going to cause any problems. Making delete_ref() delete empty\n directories takes more effort and probably not worth it.\n\n Of course this only works if people do not expect empty directories\n to stay in $GIT_DIR/refs permanently. They now may need to put .keep\n file in to keep parent directories from being removed. Would anyone\n do that?\n\n pack-refs.c          |   32 --------------------------------\n refs.c               |   14 +++++++++++++-\n t/t3210-pack-refs.sh |    6 ------\n 3 files changed, 13 insertions(+), 39 deletions(-)\n\ndiff --git a/pack-refs.c b/pack-refs.c\nindex f09a054..ec9e476 100644\n--- a/pack-refs.c\n+++ b/pack-refs.c\n@@ -60,37 +60,6 @@ static int handle_one_ref(const char *path, const unsigned char *sha1,\n \treturn 0;\n }\n \n-/*\n- * Remove empty parents, but spare refs/ and immediate subdirs.\n- * Note: munges *name.\n- */\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_refname_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@@ -99,7 +68,6 @@ 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/refs.c b/refs.c\nindex b8843bb..80ebba3 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -343,6 +343,12 @@ static void get_ref_dir(struct ref_cache *refs, const char *base,\n \t\tstruct dirent *de;\n \t\tint baselen = strlen(base);\n \t\tchar *refname = xmalloc(baselen + 257);\n+\t\tint empty_dir = 1;\n+\n+\t\tif (!strcmp(base, \"refs\") ||\n+\t\t    !strcmp(base, \"refs/heads\") ||\n+\t\t    !strcmp(base, \"refs/tags\"))\n+\t\t\tempty_dir = 0;\n \n \t\tmemcpy(refname, base, baselen);\n \t\tif (baselen && base[baselen-1] != '/')\n@@ -355,8 +361,12 @@ static void get_ref_dir(struct ref_cache *refs, const char *base,\n \t\t\tint namelen;\n \t\t\tconst char *refdir;\n \n-\t\t\tif (de->d_name[0] == '.')\n+\t\t\tif (de->d_name[0] == '.') {\n+\t\t\t\tif (de->d_name[1] != '.' || de->d_name[2])\n+\t\t\t\t\tempty_dir = 0;\n \t\t\t\tcontinue;\n+\t\t\t}\n+\t\t\tempty_dir = 0;\n \t\t\tnamelen = strlen(de->d_name);\n \t\t\tif (namelen > 255)\n \t\t\t\tcontinue;\n@@ -387,6 +397,8 @@ static void get_ref_dir(struct ref_cache *refs, const char *base,\n \t\t}\n \t\tfree(refname);\n \t\tclosedir(dir);\n+\t\tif (empty_dir)\n+\t\t\trmdir(path);\n \t}\n }\n \ndiff --git a/t/t3210-pack-refs.sh b/t/t3210-pack-refs.sh\nindex cd04361..5251740 100755\n--- a/t/t3210-pack-refs.sh\n+++ b/t/t3210-pack-refs.sh\n@@ -60,12 +60,6 @@ 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.7.8.36.g69ee2\n"},{"id":"184379","messageId":"7v39aiqzda.fsf@alter.siamese.dyndns.org","threadId":"29596","inReplyTo":"1328891127-17150-1-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH] Remove empty ref directories while reading loose refs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-02-10T19:09:37Z","receivedAt":"2012-02-10T19:09:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:\n\n>  I don't think the a few extra rmdir()s from time to time at startup\n>  are going to cause any problems. Making delete_ref() delete empty\n>  directories takes more effort and probably not worth it.\n\nThat reads as a very poorly phrased excuse for not solving the problem at\nthe right location.  Compared to all the codepaths that want to resolve\nref, delete_ref() is run much less often, and it is where the problem you\nare solving (i.e. directories that have just become unnecessary are not\nremoved) originates, no?\n\nWouldn't it be just the matter of replacing two unlink_or_warn() calls in\ndelete_ref(), one for cleaning refs/ hierarchy and the other for cleaning\nlogs/ hierarcy, with a new helper that calls unlink_or_warn() and then\ntries rmdir going upwards until it hits the limit, perhaps using a helper\nfunction that refactors dir.c::remove_path() that takes an extra parameter\ntelling it where to stop?\n"},{"id":"184395","messageId":"20120210205330.GE5504@sigill.intra.peff.net","threadId":"29596","inReplyTo":"1328891127-17150-1-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH] Remove empty ref directories while reading loose refs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-02-10T20:53:30Z","receivedAt":"2012-02-10T20:53:30Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Feb 10, 2012 at 11:25:27PM +0700, Nguyen Thai Ngoc Duy wrote:\n\n> Empty directories in $GIT_DIR/refs increases overhead at startup.\n> Removing a ref does not remove its parent directories even if it's the\n> only file left so empty directories will be hanging around.\n> [...]\n> This patch removes empty directories as we see while traversing\n> $GIT_DIR/refs and reverts be7c6d4 because it's no longer needed.\n\nIt feels wrong to me to be writing to the repository during what would\notherwise be a read-only operation. Especially without locking. Doesn't\nthis create a race condition with:\n\n  git update-ref refs/foo/bar $sha1 &      (a)\n  git for-each-ref                         (b)\n\nif you have this sequence of events:\n\n  1. (a) wants to create the ref, so it must first mkdir\n     \".git/refs/foo\".\n\n  2. (b) is reading refs and notices the empty \"foo\" directory. It\n     rmdirs it.\n\n  3. (a) now attempts to create \"bar\" inside the newly created \"foo\"\n     directory. This fails, because the directory does not exist.\n\nA similar race already can happen with:\n\n  git update-ref refs/foo/bar $sha1 &\n  git update-ref refs/foo $sha1\n\nsince the latter will remove a stale \"foo\" directory before it can\ncreate the new ref file.  But that race is OK, I think. Those are both\nwrite operations, and one of them _must_ fail, because they are in\nconflict (and I think even with the race they fail gracefully, with the\nlatter one \"winning\").\n\n> pack-refs was taught of cleaning up empty directories in be7c6d4\n> (pack-refs: remove newly empty directories - 2010-07-06), but it only\n> checks parent directories of packed refs only. Already empty dirs are\n> left untouched.\n\nI'd much rather have pack-refs simply learn to remove all stale\ndirectories. We at least know that \"gc\" is a slightly riskier operation.\n\n-Peff\n"},{"id":"184437","messageId":"1328946907-31650-1-git-send-email-pclouds@gmail.com","threadId":"29596","inReplyTo":"1328891127-17150-1-git-send-email-pclouds@gmail.com","subject":"[PATCH 1/2] pack-refs: remove all empty directories under $GIT_DIR/refs","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2012-02-11T07:55:06Z","receivedAt":"2012-02-11T07:55:06Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"Deleting refs does not remove parent directories if they are empty.\nEmpty directories add extra overhead to startup time of most of git\ncommands because they have to traverse $GIT_DIR/refs.\n\nSome directories are kept by this patch even if they are empty (refs,\nrefs/heads and refs/tags). The first one is one of git repository\nsignature. The rest is created by init-db, one may expect them to always\nbe there.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n v2, no more refs code change.\n\n Part of the reason I do not want to update delete_ref() is because it\n won't remove empty directories in existing repositories.\n\n pack-refs.c |   53 +++++++++++++++++++++++++++++++++++++++++++++++++++++\n 1 files changed, 53 insertions(+), 0 deletions(-)\n\ndiff --git a/pack-refs.c b/pack-refs.c\nindex f09a054..bb3a9c4 100644\n--- a/pack-refs.c\n+++ b/pack-refs.c\n@@ -91,6 +91,58 @@ static void try_remove_empty_parents(char *name)\n \t}\n }\n \n+static int prune_empty_dirs(const char *path)\n+{\n+\tint nr_entries = 0, pathlen = strlen(path);\n+\tDIR *dir;\n+\tstruct dirent *de;\n+\tchar *subpath;\n+\n+\tdir = opendir(git_path(\"%s\", path));\n+\n+\tif (!dir)\n+\t\treturn 0;\n+\n+\tsubpath = xmalloc(pathlen + 257);\n+\tmemcpy(subpath, path, pathlen);\n+\tif (pathlen && path[pathlen-1] != '/')\n+\t\tsubpath[pathlen++] = '/';\n+\n+\twhile ((de = readdir(dir)) != NULL) {\n+\t\tstruct stat st;\n+\t\tint namelen;\n+\n+\t\tif (de->d_name[0] == '.') {\n+\t\t\tif (strcmp(de->d_name, \"..\") && strcmp(de->d_name, \".\"))\n+\t\t\t\tnr_entries++;\n+\t\t\tcontinue;\n+\t\t}\n+\t\tnr_entries++;\n+\t\tnamelen = strlen(de->d_name);\n+\t\tif (namelen > 255)\n+\t\t\tcontinue;\n+\t\tif (has_extension(de->d_name, \".lock\"))\n+\t\t\tcontinue;\n+\t\tmemcpy(subpath + pathlen, de->d_name, namelen+1);\n+\t\tif (stat(git_path(\"%s\", subpath), &st) < 0)\n+\t\t\tcontinue;\n+\t\tif (S_ISDIR(st.st_mode)) {\n+\t\t\tint removed = prune_empty_dirs(subpath);\n+\t\t\tif (removed)\n+\t\t\t\tnr_entries--;\n+\t\t\tcontinue;\n+\t\t}\n+\t}\n+\tfree(subpath);\n+\tclosedir(dir);\n+\tif (nr_entries == 0 &&\n+\t    strcmp(path, \"refs\") &&\n+\t    strcmp(path, \"refs/heads\") &&\n+\t    strcmp(path, \"refs/tags\"))\n+\t\treturn rmdir(git_path(\"%s\", path)) == 0;\n+\treturn 0;\n+}\n+\n /* make sure nobody touched the ref, and unlink */\n static void prune_ref(struct ref_to_prune *r)\n {\n@@ -109,6 +161,7 @@ static void prune_refs(struct ref_to_prune *r)\n \t\tprune_ref(r);\n \t\tr = r->next;\n \t}\n+\tprune_empty_dirs(\"refs\");\n }\n \n static struct lock_file packed;\n-- \n1.7.8.36.g69ee2\n"},{"id":"184438","messageId":"1328946907-31650-2-git-send-email-pclouds@gmail.com","threadId":"29596","inReplyTo":"1328946907-31650-1-git-send-email-pclouds@gmail.com","subject":"[PATCH 2/2] Revert be7c6d4 (pack-refs: remove newly empty directories)","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2012-02-11T07:55:07Z","receivedAt":"2012-02-11T07:55:07Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"The functionality is taken over by prune_empty_dirs. Only code is\nreverted. The added test remains to verify.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n pack-refs.c |   32 --------------------------------\n 1 files changed, 0 insertions(+), 32 deletions(-)\n\ndiff --git a/pack-refs.c b/pack-refs.c\nindex bb3a9c4..746211e 100644\n--- a/pack-refs.c\n+++ b/pack-refs.c\n@@ -60,37 +60,6 @@ static int handle_one_ref(const char *path, const unsigned char *sha1,\n \treturn 0;\n }\n \n-/*\n- * Remove empty parents, but spare refs/ and immediate subdirs.\n- * Note: munges *name.\n- */\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_refname_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 static int prune_empty_dirs(const char *path)\n {\n \tint nr_entries = 0, pathlen = strlen(path);\n@@ -151,7 +120,6 @@ 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 \n-- \n1.7.8.36.g69ee2\n"},{"id":"184444","messageId":"7vhayxn5cg.fsf@alter.siamese.dyndns.org","threadId":"29596","inReplyTo":"1328946907-31650-1-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH 1/2] pack-refs: remove all empty directories under $GIT_DIR/refs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-02-11T08:26:23Z","receivedAt":"2012-02-11T08:26:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:\n\n> Deleting refs does not remove parent directories if they are empty.\n> Empty directories add extra overhead to startup time of most of git\n> commands because they have to traverse $GIT_DIR/refs.\n\nPerhaps drop the first line and replace with the description of what you\ndo differently from the first round?\n\n    \"git pack-refs\" tries to remove directory that becomes empty but it\n    does not try to do so hard enough, leaving a parent directory full of\n    empty children directories without removing.\n\nor something?\n\n> Some directories are kept by this patch even if they are empty (refs,\n> refs/heads and refs/tags). The first one is one of git repository\n> signature. The rest is created by init-db, one may expect them to always\n> be there.\n>\n> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n> ---\n>  v2, no more refs code change.\n>\n>  Part of the reason I do not want to update delete_ref() is because it\n>  won't remove empty directories in existing repositories.\n\nWhile I agree with Peff that people would expect doing other things while\npack-refs is running would be much \"riskier\" and doing this inside\npack-refs is far more preferable than doing so during normal read-only\noperation, I wonder why we would want a completely separate pass that\nscans the entire hierarchy.  Would it make more sense to note the\ndirectory for which rmdir() fails in try_remove_empty_parents(), and\nrevisit only these directories, at least?\n\nWouldn't we want to rmdir() the corresponding logs/ hierarchy while at it\nto be consistent?\n\n>\n>  pack-refs.c |   53 +++++++++++++++++++++++++++++++++++++++++++++++++++++\n>  1 files changed, 53 insertions(+), 0 deletions(-)\n>\n> diff --git a/pack-refs.c b/pack-refs.c\n> index f09a054..bb3a9c4 100644\n> --- a/pack-refs.c\n> +++ b/pack-refs.c\n> @@ -91,6 +91,58 @@ static void try_remove_empty_parents(char *name)\n>  \t}\n>  }\n>  \n> +static int prune_empty_dirs(const char *path)\n> +{\n> +\tint nr_entries = 0, pathlen = strlen(path);\n> +\tDIR *dir;\n> +\tstruct dirent *de;\n> +\tchar *subpath;\n> +\n> +\tdir = opendir(git_path(\"%s\", path));\n> +\n> +\tif (!dir)\n> +\t\treturn 0;\n> +\n> +\tsubpath = xmalloc(pathlen + 257);\n\nWhat is this 257 about?\n\n> +\tmemcpy(subpath, path, pathlen);\n> +\tif (pathlen && path[pathlen-1] != '/')\n> +\t\tsubpath[pathlen++] = '/';\n> +\n> +\twhile ((de = readdir(dir)) != NULL) {\n> +\t\tstruct stat st;\n> +\t\tint namelen;\n> +\n> +\t\tif (de->d_name[0] == '.') {\n> +\t\t\tif (strcmp(de->d_name, \"..\") && strcmp(de->d_name, \".\"))\n> +\t\t\t\tnr_entries++;\n> +\t\t\tcontinue;\n> +\t\t}\n> +\t\tnr_entries++;\n> +\t\tnamelen = strlen(de->d_name);\n> +\t\tif (namelen > 255)\n> +\t\t\tcontinue;\n> +\t\tif (has_extension(de->d_name, \".lock\"))\n> +\t\t\tcontinue;\n\nThis is a sign that somebody else might be actively accessing this\nrepository.\n\n> +\t\tmemcpy(subpath + pathlen, de->d_name, namelen+1);\n> +\t\tif (stat(git_path(\"%s\", subpath), &st) < 0)\n> +\t\t\tcontinue;\n> +\t\tif (S_ISDIR(st.st_mode)) {\n> +\t\t\tint removed = prune_empty_dirs(subpath);\n> +\t\t\tif (removed)\n> +\t\t\t\tnr_entries--;\n> +\t\t\tcontinue;\n> +\t\t}\n> +\t}\n> +\tfree(subpath);\n> +\tclosedir(dir);\n> +\tif (nr_entries == 0 &&\n> +\t    strcmp(path, \"refs\") &&\n> +\t    strcmp(path, \"refs/heads\") &&\n> +\t    strcmp(path, \"refs/tags\"))\n> +\t\treturn rmdir(git_path(\"%s\", path)) == 0;\n> +\treturn 0;\n> +}\n> +\n>  /* make sure nobody touched the ref, and unlink */\n>  static void prune_ref(struct ref_to_prune *r)\n>  {\n> @@ -109,6 +161,7 @@ static void prune_refs(struct ref_to_prune *r)\n>  \t\tprune_ref(r);\n>  \t\tr = r->next;\n>  \t}\n> +\tprune_empty_dirs(\"refs\");\n>  }\n>  \n>  static struct lock_file packed;\n"},{"id":"184445","messageId":"CACsJy8Bh=FZ6kNN5hERK5_H7XnZ83BZ_EfsZ5XmJbrnn+CfgcQ@mail.gmail.com","threadId":"29596","inReplyTo":"7vhayxn5cg.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/2] pack-refs: remove all empty directories under $GIT_DIR/refs","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2012-02-11T08:55:31Z","receivedAt":"2012-02-11T08:55:31Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"2012/2/11 Junio C Hamano <gitster@pobox.com>:\n> Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:\n>\n>> Deleting refs does not remove parent directories if they are empty.\n>> Empty directories add extra overhead to startup time of most of git\n>> commands because they have to traverse $GIT_DIR/refs.\n>\n> Perhaps drop the first line and replace with the description of what you\n> do differently from the first round?\n>\n>    \"git pack-refs\" tries to remove directory that becomes empty but it\n>    does not try to do so hard enough, leaving a parent directory full of\n>    empty children directories without removing.\n\nSure.\n\n> While I agree with Peff that people would expect doing other things while\n> pack-refs is running would be much \"riskier\" and doing this inside\n> pack-refs is far more preferable than doing so during normal read-only\n> operation, I wonder why we would want a completely separate pass that\n> scans the entire hierarchy\n\nLess complex code. Doing it in one pass, I think get_ref_dir() needs\nto learn read-only vs read-write mode and I haven't figured out a\nnon-ugly way to do it.\n\n> Would it make more sense to note the\n> directory for which rmdir() fails in try_remove_empty_parents(), and\n> revisit only these directories, at least?\n\nThat would leave empty directories not sharing the ref's path until\nthe failed rmdir() unexamined, I think.\n\n> Wouldn't we want to rmdir() the corresponding logs/ hierarchy while at it\n> to be consistent?\n\nGood idea.\n\n>> +     subpath = xmalloc(pathlen + 257);\n>\n> What is this 257 about?\n\nThis function is a ripoff from get_ref_dir(). I think 257 is 255 below\nplus '/' and NIL.\n\n>> +             if (namelen > 255)\n>> +                     continue;\n-- \nDuy\n"},{"id":"184451","messageId":"1328958484-4202-1-git-send-email-pclouds@gmail.com","threadId":"29596","inReplyTo":"1328946907-31650-1-git-send-email-pclouds@gmail.com","subject":"[PATCH] pack-refs: remove all empty dirs under .git/{refs,logs/refs}","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2012-02-11T11:08:04Z","receivedAt":"2012-02-11T11:08:04Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"\"git pack-refs\" tries to remove directory that becomes empty but it\ndoes not try to do so hard enough. Only empty directories created\nbecause a ref is packed are considered.\n\nThis patch introduces a global switch, which instructs ref machinery\nto collect all empty directories (or ones containing only empty\ndirectories) in removable order. \"git pack-refs\" uses this information\nto clean $GIT_DIR/refs and $GIT_DIR/logs/refs.\n\nSome directories are kept by this patch even if they are empty: refs,\nrefs/heads and refs/tags. The first one is one of git repository\nsignature. The rest is created by init-db, one may expect them to always\nbe there.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n v3, no second look at $GIT_DIR/refs and also clean\n $GIT_DIR/logs/refs. Not really fond of the global switch, but it does\n not look very intrusive to refs.c\n\n builtin/pack-refs.c  |    2 ++\n pack-refs.c          |   10 ++++++++++\n refs.c               |   35 +++++++++++++++++++++++++++++++----\n refs.h               |    4 ++++\n t/t3210-pack-refs.sh |   10 ++++++++++\n 5 files changed, 57 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/pack-refs.c b/builtin/pack-refs.c\nindex 39a9d89..044ae8f 100644\n--- a/builtin/pack-refs.c\n+++ b/builtin/pack-refs.c\n@@ -1,6 +1,7 @@\n #include \"builtin.h\"\n #include \"parse-options.h\"\n #include \"pack-refs.h\"\n+#include \"refs.h\"\n \n static char const * const pack_refs_usage[] = {\n \t\"git pack-refs [options]\",\n@@ -15,6 +16,7 @@ int cmd_pack_refs(int argc, const char **argv, const char *prefix)\n \t\tOPT_BIT(0, \"prune\", &flags, \"prune loose refs (default)\", PACK_REFS_PRUNE),\n \t\tOPT_END(),\n \t};\n+\tsave_empty_ref_directories = 1;\n \tif (parse_options(argc, argv, prefix, opts, pack_refs_usage, 0))\n \t\tusage_with_options(pack_refs_usage, opts);\n \treturn pack_refs(flags);\ndiff --git a/pack-refs.c b/pack-refs.c\nindex f09a054..76d3408 100644\n--- a/pack-refs.c\n+++ b/pack-refs.c\n@@ -2,6 +2,7 @@\n #include \"refs.h\"\n #include \"tag.h\"\n #include \"pack-refs.h\"\n+#include \"string-list.h\"\n \n struct ref_to_prune {\n \tstruct ref_to_prune *next;\n@@ -105,10 +106,19 @@ static void prune_ref(struct ref_to_prune *r)\n \n static void prune_refs(struct ref_to_prune *r)\n {\n+\tstruct string_list *list = get_empty_ref_directories();;\n+\tint i;\n+\n \twhile (r) {\n \t\tprune_ref(r);\n \t\tr = r->next;\n \t}\n+\n+\tfor (i = 0; i < list->nr; i++) {\n+\t\tconst char *s = list->items[i].string;\n+\t\trmdir(git_path(\"%s\", s));\n+\t\trmdir(git_path(\"logs/%s\", s));\n+\t}\n }\n \n static struct lock_file packed;\ndiff --git a/refs.c b/refs.c\nindex b8843bb..7e9a250 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -3,6 +3,7 @@\n #include \"object.h\"\n #include \"tag.h\"\n #include \"dir.h\"\n+#include \"string-list.h\"\n \n /* ISSYMREF=0x01, ISPACKED=0x02 and ISBROKEN=0x04 are public interfaces */\n #define REF_KNOWS_PEELED 0x10\n@@ -29,6 +30,8 @@ struct ref_array {\n \tstruct ref_entry **refs;\n };\n \n+int save_empty_ref_directories;\n+\n /*\n  * Parse one line from a packed-refs file.  Write the SHA1 to sha1.\n  * Return a pointer to the refname within the line (null-terminated),\n@@ -177,6 +180,7 @@ static struct ref_cache {\n \tchar did_packed;\n \tstruct ref_array loose;\n \tstruct ref_array packed;\n+\tstruct string_list empty_dirs;\n \t/* The submodule name, or \"\" for the main repo. */\n \tchar name[FLEX_ARRAY];\n } *ref_cache;\n@@ -326,7 +330,7 @@ void add_packed_ref(const char *refname, const unsigned char *sha1)\n }\n \n static void get_ref_dir(struct ref_cache *refs, const char *base,\n-\t\t\tstruct ref_array *array)\n+\t\t\tstruct ref_array *array, int *would_be_empty)\n {\n \tDIR *dir;\n \tconst char *path;\n@@ -343,6 +347,7 @@ static void get_ref_dir(struct ref_cache *refs, const char *base,\n \t\tstruct dirent *de;\n \t\tint baselen = strlen(base);\n \t\tchar *refname = xmalloc(baselen + 257);\n+\t\tint nr = 0;\n \n \t\tmemcpy(refname, base, baselen);\n \t\tif (baselen && base[baselen-1] != '/')\n@@ -355,8 +360,13 @@ static void get_ref_dir(struct ref_cache *refs, const char *base,\n \t\t\tint namelen;\n \t\t\tconst char *refdir;\n \n-\t\t\tif (de->d_name[0] == '.')\n+\t\t\tif (de->d_name[0] == '.') {\n+\t\t\t\tif (strcmp(de->d_name, \"..\") &&\n+\t\t\t\t    strcmp(de->d_name, \".\"))\n+\t\t\t\t\tnr++;\n \t\t\t\tcontinue;\n+\t\t\t}\n+\t\t\tnr++;\n \t\t\tnamelen = strlen(de->d_name);\n \t\t\tif (namelen > 255)\n \t\t\t\tcontinue;\n@@ -369,7 +379,10 @@ static void get_ref_dir(struct ref_cache *refs, const char *base,\n \t\t\tif (stat(refdir, &st) < 0)\n \t\t\t\tcontinue;\n \t\t\tif (S_ISDIR(st.st_mode)) {\n-\t\t\t\tget_ref_dir(refs, refname, array);\n+\t\t\t\tint empty = 0;\n+\t\t\t\tget_ref_dir(refs, refname, array, &empty);\n+\t\t\t\tif (empty)\n+\t\t\t\t\tnr--;\n \t\t\t\tcontinue;\n \t\t\t}\n \t\t\tif (*refs->name) {\n@@ -387,6 +400,15 @@ static void get_ref_dir(struct ref_cache *refs, const char *base,\n \t\t}\n \t\tfree(refname);\n \t\tclosedir(dir);\n+\t\tif (save_empty_ref_directories &&\n+\t\t    nr == 0 &&\n+\t\t    strcmp(base, \"refs\") &&\n+\t\t    strcmp(base, \"refs/heads\") &&\n+\t\t    strcmp(base, \"refs/tags\")) {\n+\t\t\tstring_list_append(&refs->empty_dirs, xstrdup(base));\n+\t\t\tif (would_be_empty)\n+\t\t\t\t*would_be_empty = 1;\n+\t\t}\n \t}\n }\n \n@@ -427,12 +449,17 @@ void warn_dangling_symref(FILE *fp, const char *msg_fmt, const char *refname)\n static struct ref_array *get_loose_refs(struct ref_cache *refs)\n {\n \tif (!refs->did_loose) {\n-\t\tget_ref_dir(refs, \"refs\", &refs->loose);\n+\t\tget_ref_dir(refs, \"refs\", &refs->loose, NULL);\n \t\trefs->did_loose = 1;\n \t}\n \treturn &refs->loose;\n }\n \n+struct string_list *get_empty_ref_directories()\n+{\n+\treturn &get_ref_cache(NULL)->empty_dirs;\n+}\n+\n /* We allow \"recursive\" symbolic refs. Only within reason, though */\n #define MAXDEPTH 5\n #define MAXREFLEN (1024)\ndiff --git a/refs.h b/refs.h\nindex 00ba1e2..21a2a00 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -14,6 +14,10 @@ struct ref_lock {\n #define REF_ISPACKED 0x02\n #define REF_ISBROKEN 0x04\n \n+struct string_list;\n+extern int save_empty_ref_directories;\n+extern struct string_list *get_empty_ref_directories();\n+\n /*\n  * Calls the specified function for each ref file until it returns nonzero,\n  * and returns the value\ndiff --git a/t/t3210-pack-refs.sh b/t/t3210-pack-refs.sh\nindex cd04361..40fcd54 100755\n--- a/t/t3210-pack-refs.sh\n+++ b/t/t3210-pack-refs.sh\n@@ -66,6 +66,16 @@ test_expect_success 'see if git pack-refs --prune removes empty dirs' '\n      ! test -e .git/refs/heads/r\n '\n \n+test_expect_success 'pack-refs --prune removes all empty dirs in refs and logs' '\n+     mkdir -p .git/refs/empty/outside/heads &&\n+     mkdir -p .git/refs/heads/empty/dir/ectory &&\n+     mkdir -p .git/logs/refs/heads/empty/dir/ectory &&\n+     git pack-refs --all --prune &&\n+     ! test -e .git/refs/empty &&\n+     ! test -e .git/refs/heads/empty &&\n+     ! test -e .git/logs/refs/heads/empty\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.7.8.36.g69ee2\n"},{"id":"184452","messageId":"CA+39Oz5CCxgM3s804VgnvvTYC2Yynt5YOdswD0cuZ8WWXTcqwA@mail.gmail.com","threadId":"29596","inReplyTo":"1328958484-4202-1-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH] pack-refs: remove all empty dirs under .git/{refs,logs/refs}","fromName":"Thomas Adam","fromEmail":"thomas@xteddy.org","sentAt":"2012-02-11T11:27:56Z","receivedAt":"2012-02-11T11:27:56Z","isPatch":true,"sender":{"key":"thomas@xteddy.org","avatar":"https://gravatar.com/avatar/e7256db4738e501e5d2e84f00bb0bd99503165729573848a03330301fc2adc4a?d=mp&s=160"},"body":"2012/2/11 Nguyễn Thái Ngọc Duy <pclouds@gmail.com>:\n>  static void prune_refs(struct ref_to_prune *r)\n>  {\n> +       struct string_list *list = get_empty_ref_directories();;\n\nDouble \";;\" at end of line.\n\n-- Thomas Adam\n"},{"id":"184463","messageId":"7vd39lmetr.fsf@alter.siamese.dyndns.org","threadId":"29596","inReplyTo":"CACsJy8Bh=FZ6kNN5hERK5_H7XnZ83BZ_EfsZ5XmJbrnn+CfgcQ@mail.gmail.com","subject":"Re: [PATCH 1/2] pack-refs: remove all empty directories under $GIT_DIR/refs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-02-11T17:59:12Z","receivedAt":"2012-02-11T17:59:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:\n\n> 2012/2/11 Junio C Hamano <gitster@pobox.com>:\n>> Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:\n> ...\n>> Would it make more sense to note the\n>> directory for which rmdir() fails in try_remove_empty_parents(), and\n>> revisit only these directories, at least?\n>\n> That would leave empty directories not sharing the ref's path until\n> the failed rmdir() unexamined, I think.\n\nTrue. Thanks.\n\n>>> +     subpath = xmalloc(pathlen + 257);\n>>\n>> What is this 257 about?\n>\n> This function is a ripoff from get_ref_dir(). I think 257 is 255 below\n> plus '/' and NIL.\n\nI do not think there is any justification to copy-and-paste from code that\npredates the strbuf infrastructure these days.\n"}]}