[PATCH] Remove empty ref directories while reading loose refs

[PATCH] Remove empty ref directories while reading loose refs

[Nguyễn Thái Ngọc Duy]

Empty directories in $GIT_DIR/refs increases overhead at startup.
Removing a ref does not remove its parent directories even if it's the
only file left so empty directories will be hanging around.

pack-refs was taught of cleaning up empty directories in be7c6d4
(pack-refs: remove newly empty directories - 2010-07-06), but it only
checks parent directories of packed refs only. Already empty dirs are
left untouched.

This patch removes empty directories as we see while traversing
$GIT_DIR/refs and reverts be7c6d4 because it's no longer needed.

Some directories, even if empty, are not removed:

 - refs: this one is needed to recognize a git repository
 - refs/heads and refs/tags: these are created by init-db, people may
   expect them to always be there

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 I don't think the a few extra rmdir()s from time to time at startup
 are going to cause any problems. Making delete_ref() delete empty
 directories takes more effort and probably not worth it.

 Of course this only works if people do not expect empty directories
 to stay in $GIT_DIR/refs permanently. They now may need to put .keep
 file in to keep parent directories from being removed. Would anyone
 do that?

 pack-refs.c          |   32 --------------------------------
 refs.c               |   14 +++++++++++++-
 t/t3210-pack-refs.sh |    6 ------
 3 files changed, 13 insertions(+), 39 deletions(-)

diff --git a/pack-refs.c b/pack-refs.c
index f09a054..ec9e476 100644
--- a/pack-refs.c
+++ b/pack-refs.c
@@ -60,37 +60,6 @@ static int handle_one_ref(const char *path, const unsigned char *sha1,
 	return 0;
 }
 
-/*
- * Remove empty parents, but spare refs/ and immediate subdirs.
- * Note: munges *name.
- */
-static void try_remove_empty_parents(char *name)
-{
-	char *p, *q;
-	int i;
-	p = name;
-	for (i = 0; i < 2; i++) { /* refs/{heads,tags,...}/ */
-		while (*p && *p != '/')
-			p++;
-		/* tolerate duplicate slashes; see check_refname_format() */
-		while (*p == '/')
-			p++;
-	}
-	for (q = p; *q; q++)
-		;
-	while (1) {
-		while (q > p && *q != '/')
-			q--;
-		while (q > p && *(q-1) == '/')
-			q--;
-		if (q == p)
-			break;
-		*q = '\0';
-		if (rmdir(git_path("%s", name)))
-			break;
-	}
-}
-
 /* make sure nobody touched the ref, and unlink */
 static void prune_ref(struct ref_to_prune *r)
 {
@@ -99,7 +68,6 @@ static void prune_ref(struct ref_to_prune *r)
 	if (lock) {
 		unlink_or_warn(git_path("%s", r->name));
 		unlock_ref(lock);
-		try_remove_empty_parents(r->name);
 	}
 }
 
diff --git a/refs.c b/refs.c
index b8843bb..80ebba3 100644
--- a/refs.c
+++ b/refs.c
@@ -343,6 +343,12 @@ static void get_ref_dir(struct ref_cache *refs, const char *base,
 		struct dirent *de;
 		int baselen = strlen(base);
 		char *refname = xmalloc(baselen + 257);
+		int empty_dir = 1;
+
+		if (!strcmp(base, "refs") ||
+		    !strcmp(base, "refs/heads") ||
+		    !strcmp(base, "refs/tags"))
+			empty_dir = 0;
 
 		memcpy(refname, base, baselen);
 		if (baselen && base[baselen-1] != '/')
@@ -355,8 +361,12 @@ static void get_ref_dir(struct ref_cache *refs, const char *base,
 			int namelen;
 			const char *refdir;
 
-			if (de->d_name[0] == '.')
+			if (de->d_name[0] == '.') {
+				if (de->d_name[1] != '.' || de->d_name[2])
+					empty_dir = 0;
 				continue;
+			}
+			empty_dir = 0;
 			namelen = strlen(de->d_name);
 			if (namelen > 255)
 				continue;
@@ -387,6 +397,8 @@ static void get_ref_dir(struct ref_cache *refs, const char *base,
 		}
 		free(refname);
 		closedir(dir);
+		if (empty_dir)
+			rmdir(path);
 	}
 }
 
diff --git a/t/t3210-pack-refs.sh b/t/t3210-pack-refs.sh
index cd04361..5251740 100755
--- a/t/t3210-pack-refs.sh
+++ b/t/t3210-pack-refs.sh
@@ -60,12 +60,6 @@ test_expect_success 'see if git pack-refs --prune remove ref files' '
      ! test -f .git/refs/heads/f
 '
 
-test_expect_success 'see if git pack-refs --prune removes empty dirs' '
-     git branch r/s/t &&
-     git pack-refs --all --prune &&
-     ! test -e .git/refs/heads/r
-'
-
 test_expect_success \
     'git branch g should work when git branch g/h has been deleted' \
     'git branch g/h &&
-- 
1.7.8.36.g69ee2

Re: [PATCH] Remove empty ref directories while reading loose refs

[Junio C Hamano]

Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:

>  I don't think the a few extra rmdir()s from time to time at startup
>  are going to cause any problems. Making delete_ref() delete empty
>  directories takes more effort and probably not worth it.

That reads as a very poorly phrased excuse for not solving the problem at
the right location.  Compared to all the codepaths that want to resolve
ref, delete_ref() is run much less often, and it is where the problem you
are solving (i.e. directories that have just become unnecessary are not
removed) originates, no?

Wouldn't it be just the matter of replacing two unlink_or_warn() calls in
delete_ref(), one for cleaning refs/ hierarchy and the other for cleaning
logs/ hierarcy, with a new helper that calls unlink_or_warn() and then
tries rmdir going upwards until it hits the limit, perhaps using a helper
function that refactors dir.c::remove_path() that takes an extra parameter
telling it where to stop?

Re: [PATCH] Remove empty ref directories while reading loose refs

[Jeff King]

On Fri, Feb 10, 2012 at 11:25:27PM +0700, Nguyen Thai Ngoc Duy wrote:

> Empty directories in $GIT_DIR/refs increases overhead at startup.
> Removing a ref does not remove its parent directories even if it's the
> only file left so empty directories will be hanging around.
> [...]
> This patch removes empty directories as we see while traversing
> $GIT_DIR/refs and reverts be7c6d4 because it's no longer needed.

It feels wrong to me to be writing to the repository during what would
otherwise be a read-only operation. Especially without locking. Doesn't
this create a race condition with:

  git update-ref refs/foo/bar $sha1 &      (a)
  git for-each-ref                         (b)

if you have this sequence of events:

  1. (a) wants to create the ref, so it must first mkdir
     ".git/refs/foo".

  2. (b) is reading refs and notices the empty "foo" directory. It
     rmdirs it.

  3. (a) now attempts to create "bar" inside the newly created "foo"
     directory. This fails, because the directory does not exist.

A similar race already can happen with:

  git update-ref refs/foo/bar $sha1 &
  git update-ref refs/foo $sha1

since the latter will remove a stale "foo" directory before it can
create the new ref file.  But that race is OK, I think. Those are both
write operations, and one of them _must_ fail, because they are in
conflict (and I think even with the race they fail gracefully, with the
latter one "winning").

> pack-refs was taught of cleaning up empty directories in be7c6d4
> (pack-refs: remove newly empty directories - 2010-07-06), but it only
> checks parent directories of packed refs only. Already empty dirs are
> left untouched.

I'd much rather have pack-refs simply learn to remove all stale
directories. We at least know that "gc" is a slightly riskier operation.

-Peff

[PATCH 1/2] pack-refs: remove all empty directories under $GIT_DIR/refs

[Nguyễn Thái Ngọc Duy]

Deleting refs does not remove parent directories if they are empty.
Empty directories add extra overhead to startup time of most of git
commands because they have to traverse $GIT_DIR/refs.

Some directories are kept by this patch even if they are empty (refs,
refs/heads and refs/tags). The first one is one of git repository
signature. The rest is created by init-db, one may expect them to always
be there.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 v2, no more refs code change.

 Part of the reason I do not want to update delete_ref() is because it
 won't remove empty directories in existing repositories.

 pack-refs.c |   53 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 53 insertions(+), 0 deletions(-)

diff --git a/pack-refs.c b/pack-refs.c
index f09a054..bb3a9c4 100644
--- a/pack-refs.c
+++ b/pack-refs.c
@@ -91,6 +91,58 @@ static void try_remove_empty_parents(char *name)
 	}
 }
 
+static int prune_empty_dirs(const char *path)
+{
+	int nr_entries = 0, pathlen = strlen(path);
+	DIR *dir;
+	struct dirent *de;
+	char *subpath;
+
+	dir = opendir(git_path("%s", path));
+
+	if (!dir)
+		return 0;
+
+	subpath = xmalloc(pathlen + 257);
+	memcpy(subpath, path, pathlen);
+	if (pathlen && path[pathlen-1] != '/')
+		subpath[pathlen++] = '/';
+
+	while ((de = readdir(dir)) != NULL) {
+		struct stat st;
+		int namelen;
+
+		if (de->d_name[0] == '.') {
+			if (strcmp(de->d_name, "..") && strcmp(de->d_name, "."))
+				nr_entries++;
+			continue;
+		}
+		nr_entries++;
+		namelen = strlen(de->d_name);
+		if (namelen > 255)
+			continue;
+		if (has_extension(de->d_name, ".lock"))
+			continue;
+		memcpy(subpath + pathlen, de->d_name, namelen+1);
+		if (stat(git_path("%s", subpath), &st) < 0)
+			continue;
+		if (S_ISDIR(st.st_mode)) {
+			int removed = prune_empty_dirs(subpath);
+			if (removed)
+				nr_entries--;
+			continue;
+		}
+	}
+	free(subpath);
+	closedir(dir);
+	if (nr_entries == 0 &&
+	    strcmp(path, "refs") &&
+	    strcmp(path, "refs/heads") &&
+	    strcmp(path, "refs/tags"))
+		return rmdir(git_path("%s", path)) == 0;
+	return 0;
+}
+
 /* make sure nobody touched the ref, and unlink */
 static void prune_ref(struct ref_to_prune *r)
 {
@@ -109,6 +161,7 @@ static void prune_refs(struct ref_to_prune *r)
 		prune_ref(r);
 		r = r->next;
 	}
+	prune_empty_dirs("refs");
 }
 
 static struct lock_file packed;
-- 
1.7.8.36.g69ee2

[PATCH 2/2] Revert be7c6d4 (pack-refs: remove newly empty directories)

[Nguyễn Thái Ngọc Duy]

The functionality is taken over by prune_empty_dirs. Only code is
reverted. The added test remains to verify.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 pack-refs.c |   32 --------------------------------
 1 files changed, 0 insertions(+), 32 deletions(-)

diff --git a/pack-refs.c b/pack-refs.c
index bb3a9c4..746211e 100644
--- a/pack-refs.c
+++ b/pack-refs.c
@@ -60,37 +60,6 @@ static int handle_one_ref(const char *path, const unsigned char *sha1,
 	return 0;
 }
 
-/*
- * Remove empty parents, but spare refs/ and immediate subdirs.
- * Note: munges *name.
- */
-static void try_remove_empty_parents(char *name)
-{
-	char *p, *q;
-	int i;
-	p = name;
-	for (i = 0; i < 2; i++) { /* refs/{heads,tags,...}/ */
-		while (*p && *p != '/')
-			p++;
-		/* tolerate duplicate slashes; see check_refname_format() */
-		while (*p == '/')
-			p++;
-	}
-	for (q = p; *q; q++)
-		;
-	while (1) {
-		while (q > p && *q != '/')
-			q--;
-		while (q > p && *(q-1) == '/')
-			q--;
-		if (q == p)
-			break;
-		*q = '\0';
-		if (rmdir(git_path("%s", name)))
-			break;
-	}
-}
-
 static int prune_empty_dirs(const char *path)
 {
 	int nr_entries = 0, pathlen = strlen(path);
@@ -151,7 +120,6 @@ static void prune_ref(struct ref_to_prune *r)
 	if (lock) {
 		unlink_or_warn(git_path("%s", r->name));
 		unlock_ref(lock);
-		try_remove_empty_parents(r->name);
 	}
 }
 
-- 
1.7.8.36.g69ee2

Re: [PATCH 1/2] pack-refs: remove all empty directories under $GIT_DIR/refs

[Junio C Hamano]

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

> Deleting refs does not remove parent directories if they are empty.
> Empty directories add extra overhead to startup time of most of git
> commands because they have to traverse $GIT_DIR/refs.

Perhaps drop the first line and replace with the description of what you
do differently from the first round?

    "git pack-refs" tries to remove directory that becomes empty but it
    does not try to do so hard enough, leaving a parent directory full of
    empty children directories without removing.

or something?

> Some directories are kept by this patch even if they are empty (refs,
> refs/heads and refs/tags). The first one is one of git repository
> signature. The rest is created by init-db, one may expect them to always
> be there.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  v2, no more refs code change.
>
>  Part of the reason I do not want to update delete_ref() is because it
>  won't remove empty directories in existing repositories.

While I agree with Peff that people would expect doing other things while
pack-refs is running would be much "riskier" and doing this inside
pack-refs is far more preferable than doing so during normal read-only
operation, I wonder why we would want a completely separate pass that
scans the entire hierarchy.  Would it make more sense to note the
directory for which rmdir() fails in try_remove_empty_parents(), and
revisit only these directories, at least?

Wouldn't we want to rmdir() the corresponding logs/ hierarchy while at it
to be consistent?

>
>  pack-refs.c |   53 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 53 insertions(+), 0 deletions(-)
>
> diff --git a/pack-refs.c b/pack-refs.c
> index f09a054..bb3a9c4 100644
> --- a/pack-refs.c
> +++ b/pack-refs.c
> @@ -91,6 +91,58 @@ static void try_remove_empty_parents(char *name)
>  	}
>  }
>  
> +static int prune_empty_dirs(const char *path)
> +{
> +	int nr_entries = 0, pathlen = strlen(path);
> +	DIR *dir;
> +	struct dirent *de;
> +	char *subpath;
> +
> +	dir = opendir(git_path("%s", path));
> +
> +	if (!dir)
> +		return 0;
> +
> +	subpath = xmalloc(pathlen + 257);

What is this 257 about?

> +	memcpy(subpath, path, pathlen);
> +	if (pathlen && path[pathlen-1] != '/')
> +		subpath[pathlen++] = '/';
> +
> +	while ((de = readdir(dir)) != NULL) {
> +		struct stat st;
> +		int namelen;
> +
> +		if (de->d_name[0] == '.') {
> +			if (strcmp(de->d_name, "..") && strcmp(de->d_name, "."))
> +				nr_entries++;
> +			continue;
> +		}
> +		nr_entries++;
> +		namelen = strlen(de->d_name);
> +		if (namelen > 255)
> +			continue;
> +		if (has_extension(de->d_name, ".lock"))
> +			continue;

This is a sign that somebody else might be actively accessing this
repository.

> +		memcpy(subpath + pathlen, de->d_name, namelen+1);
> +		if (stat(git_path("%s", subpath), &st) < 0)
> +			continue;
> +		if (S_ISDIR(st.st_mode)) {
> +			int removed = prune_empty_dirs(subpath);
> +			if (removed)
> +				nr_entries--;
> +			continue;
> +		}
> +	}
> +	free(subpath);
> +	closedir(dir);
> +	if (nr_entries == 0 &&
> +	    strcmp(path, "refs") &&
> +	    strcmp(path, "refs/heads") &&
> +	    strcmp(path, "refs/tags"))
> +		return rmdir(git_path("%s", path)) == 0;
> +	return 0;
> +}
> +
>  /* make sure nobody touched the ref, and unlink */
>  static void prune_ref(struct ref_to_prune *r)
>  {
> @@ -109,6 +161,7 @@ static void prune_refs(struct ref_to_prune *r)
>  		prune_ref(r);
>  		r = r->next;
>  	}
> +	prune_empty_dirs("refs");
>  }
>  
>  static struct lock_file packed;

Re: [PATCH 1/2] pack-refs: remove all empty directories under $GIT_DIR/refs

[Nguyen Thai Ngoc Duy]

2012/2/11 Junio C Hamano <gitster@pobox.com>:
> Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
>
>> Deleting refs does not remove parent directories if they are empty.
>> Empty directories add extra overhead to startup time of most of git
>> commands because they have to traverse $GIT_DIR/refs.
>
> Perhaps drop the first line and replace with the description of what you
> do differently from the first round?
>
>    "git pack-refs" tries to remove directory that becomes empty but it
>    does not try to do so hard enough, leaving a parent directory full of
>    empty children directories without removing.

Sure.

> While I agree with Peff that people would expect doing other things while
> pack-refs is running would be much "riskier" and doing this inside
> pack-refs is far more preferable than doing so during normal read-only
> operation, I wonder why we would want a completely separate pass that
> scans the entire hierarchy

Less complex code. Doing it in one pass, I think get_ref_dir() needs
to learn read-only vs read-write mode and I haven't figured out a
non-ugly way to do it.

> Would it make more sense to note the
> directory for which rmdir() fails in try_remove_empty_parents(), and
> revisit only these directories, at least?

That would leave empty directories not sharing the ref's path until
the failed rmdir() unexamined, I think.

> Wouldn't we want to rmdir() the corresponding logs/ hierarchy while at it
> to be consistent?

Good idea.

>> +     subpath = xmalloc(pathlen + 257);
>
> What is this 257 about?

This function is a ripoff from get_ref_dir(). I think 257 is 255 below
plus '/' and NIL.

>> +             if (namelen > 255)
>> +                     continue;
-- 
Duy

[PATCH] pack-refs: remove all empty dirs under .git/{refs,logs/refs}

[Nguyễn Thái Ngọc Duy]

"git pack-refs" tries to remove directory that becomes empty but it
does not try to do so hard enough. Only empty directories created
because a ref is packed are considered.

This patch introduces a global switch, which instructs ref machinery
to collect all empty directories (or ones containing only empty
directories) in removable order. "git pack-refs" uses this information
to clean $GIT_DIR/refs and $GIT_DIR/logs/refs.

Some directories are kept by this patch even if they are empty: refs,
refs/heads and refs/tags. The first one is one of git repository
signature. The rest is created by init-db, one may expect them to always
be there.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 v3, no second look at $GIT_DIR/refs and also clean
 $GIT_DIR/logs/refs. Not really fond of the global switch, but it does
 not look very intrusive to refs.c

 builtin/pack-refs.c  |    2 ++
 pack-refs.c          |   10 ++++++++++
 refs.c               |   35 +++++++++++++++++++++++++++++++----
 refs.h               |    4 ++++
 t/t3210-pack-refs.sh |   10 ++++++++++
 5 files changed, 57 insertions(+), 4 deletions(-)

diff --git a/builtin/pack-refs.c b/builtin/pack-refs.c
index 39a9d89..044ae8f 100644
--- a/builtin/pack-refs.c
+++ b/builtin/pack-refs.c
@@ -1,6 +1,7 @@
 #include "builtin.h"
 #include "parse-options.h"
 #include "pack-refs.h"
+#include "refs.h"
 
 static char const * const pack_refs_usage[] = {
 	"git pack-refs [options]",
@@ -15,6 +16,7 @@ int cmd_pack_refs(int argc, const char **argv, const char *prefix)
 		OPT_BIT(0, "prune", &flags, "prune loose refs (default)", PACK_REFS_PRUNE),
 		OPT_END(),
 	};
+	save_empty_ref_directories = 1;
 	if (parse_options(argc, argv, prefix, opts, pack_refs_usage, 0))
 		usage_with_options(pack_refs_usage, opts);
 	return pack_refs(flags);
diff --git a/pack-refs.c b/pack-refs.c
index f09a054..76d3408 100644
--- a/pack-refs.c
+++ b/pack-refs.c
@@ -2,6 +2,7 @@
 #include "refs.h"
 #include "tag.h"
 #include "pack-refs.h"
+#include "string-list.h"
 
 struct ref_to_prune {
 	struct ref_to_prune *next;
@@ -105,10 +106,19 @@ static void prune_ref(struct ref_to_prune *r)
 
 static void prune_refs(struct ref_to_prune *r)
 {
+	struct string_list *list = get_empty_ref_directories();;
+	int i;
+
 	while (r) {
 		prune_ref(r);
 		r = r->next;
 	}
+
+	for (i = 0; i < list->nr; i++) {
+		const char *s = list->items[i].string;
+		rmdir(git_path("%s", s));
+		rmdir(git_path("logs/%s", s));
+	}
 }
 
 static struct lock_file packed;
diff --git a/refs.c b/refs.c
index b8843bb..7e9a250 100644
--- a/refs.c
+++ b/refs.c
@@ -3,6 +3,7 @@
 #include "object.h"
 #include "tag.h"
 #include "dir.h"
+#include "string-list.h"
 
 /* ISSYMREF=0x01, ISPACKED=0x02 and ISBROKEN=0x04 are public interfaces */
 #define REF_KNOWS_PEELED 0x10
@@ -29,6 +30,8 @@ struct ref_array {
 	struct ref_entry **refs;
 };
 
+int save_empty_ref_directories;
+
 /*
  * Parse one line from a packed-refs file.  Write the SHA1 to sha1.
  * Return a pointer to the refname within the line (null-terminated),
@@ -177,6 +180,7 @@ static struct ref_cache {
 	char did_packed;
 	struct ref_array loose;
 	struct ref_array packed;
+	struct string_list empty_dirs;
 	/* The submodule name, or "" for the main repo. */
 	char name[FLEX_ARRAY];
 } *ref_cache;
@@ -326,7 +330,7 @@ void add_packed_ref(const char *refname, const unsigned char *sha1)
 }
 
 static void get_ref_dir(struct ref_cache *refs, const char *base,
-			struct ref_array *array)
+			struct ref_array *array, int *would_be_empty)
 {
 	DIR *dir;
 	const char *path;
@@ -343,6 +347,7 @@ static void get_ref_dir(struct ref_cache *refs, const char *base,
 		struct dirent *de;
 		int baselen = strlen(base);
 		char *refname = xmalloc(baselen + 257);
+		int nr = 0;
 
 		memcpy(refname, base, baselen);
 		if (baselen && base[baselen-1] != '/')
@@ -355,8 +360,13 @@ static void get_ref_dir(struct ref_cache *refs, const char *base,
 			int namelen;
 			const char *refdir;
 
-			if (de->d_name[0] == '.')
+			if (de->d_name[0] == '.') {
+				if (strcmp(de->d_name, "..") &&
+				    strcmp(de->d_name, "."))
+					nr++;
 				continue;
+			}
+			nr++;
 			namelen = strlen(de->d_name);
 			if (namelen > 255)
 				continue;
@@ -369,7 +379,10 @@ static void get_ref_dir(struct ref_cache *refs, const char *base,
 			if (stat(refdir, &st) < 0)
 				continue;
 			if (S_ISDIR(st.st_mode)) {
-				get_ref_dir(refs, refname, array);
+				int empty = 0;
+				get_ref_dir(refs, refname, array, &empty);
+				if (empty)
+					nr--;
 				continue;
 			}
 			if (*refs->name) {
@@ -387,6 +400,15 @@ static void get_ref_dir(struct ref_cache *refs, const char *base,
 		}
 		free(refname);
 		closedir(dir);
+		if (save_empty_ref_directories &&
+		    nr == 0 &&
+		    strcmp(base, "refs") &&
+		    strcmp(base, "refs/heads") &&
+		    strcmp(base, "refs/tags")) {
+			string_list_append(&refs->empty_dirs, xstrdup(base));
+			if (would_be_empty)
+				*would_be_empty = 1;
+		}
 	}
 }
 
@@ -427,12 +449,17 @@ void warn_dangling_symref(FILE *fp, const char *msg_fmt, const char *refname)
 static struct ref_array *get_loose_refs(struct ref_cache *refs)
 {
 	if (!refs->did_loose) {
-		get_ref_dir(refs, "refs", &refs->loose);
+		get_ref_dir(refs, "refs", &refs->loose, NULL);
 		refs->did_loose = 1;
 	}
 	return &refs->loose;
 }
 
+struct string_list *get_empty_ref_directories()
+{
+	return &get_ref_cache(NULL)->empty_dirs;
+}
+
 /* We allow "recursive" symbolic refs. Only within reason, though */
 #define MAXDEPTH 5
 #define MAXREFLEN (1024)
diff --git a/refs.h b/refs.h
index 00ba1e2..21a2a00 100644
--- a/refs.h
+++ b/refs.h
@@ -14,6 +14,10 @@ struct ref_lock {
 #define REF_ISPACKED 0x02
 #define REF_ISBROKEN 0x04
 
+struct string_list;
+extern int save_empty_ref_directories;
+extern struct string_list *get_empty_ref_directories();
+
 /*
  * Calls the specified function for each ref file until it returns nonzero,
  * and returns the value
diff --git a/t/t3210-pack-refs.sh b/t/t3210-pack-refs.sh
index cd04361..40fcd54 100755
--- a/t/t3210-pack-refs.sh
+++ b/t/t3210-pack-refs.sh
@@ -66,6 +66,16 @@ test_expect_success 'see if git pack-refs --prune removes empty dirs' '
      ! test -e .git/refs/heads/r
 '
 
+test_expect_success 'pack-refs --prune removes all empty dirs in refs and logs' '
+     mkdir -p .git/refs/empty/outside/heads &&
+     mkdir -p .git/refs/heads/empty/dir/ectory &&
+     mkdir -p .git/logs/refs/heads/empty/dir/ectory &&
+     git pack-refs --all --prune &&
+     ! test -e .git/refs/empty &&
+     ! test -e .git/refs/heads/empty &&
+     ! test -e .git/logs/refs/heads/empty
+'
+
 test_expect_success \
     'git branch g should work when git branch g/h has been deleted' \
     'git branch g/h &&
-- 
1.7.8.36.g69ee2

Re: [PATCH] pack-refs: remove all empty dirs under .git/{refs,logs/refs}

[Thomas Adam]

2012/2/11 Nguyễn Thái Ngọc Duy <pclouds@gmail.com>:
>  static void prune_refs(struct ref_to_prune *r)
>  {
> +       struct string_list *list = get_empty_ref_directories();;

Double ";;" at end of line.

-- Thomas Adam

Re: [PATCH 1/2] pack-refs: remove all empty directories under $GIT_DIR/refs

[Junio C Hamano]

Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:

> 2012/2/11 Junio C Hamano <gitster@pobox.com>:
>> Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
> ...
>> Would it make more sense to note the
>> directory for which rmdir() fails in try_remove_empty_parents(), and
>> revisit only these directories, at least?
>
> That would leave empty directories not sharing the ref's path until
> the failed rmdir() unexamined, I think.

True. Thanks.

>>> +     subpath = xmalloc(pathlen + 257);
>>
>> What is this 257 about?
>
> This function is a ripoff from get_ref_dir(). I think 257 is 255 below
> plus '/' and NIL.

I do not think there is any justification to copy-and-paste from code that
predates the strbuf infrastructure these days.