[PATCH] pack-refs: remove newly empty directories

[PATCH] pack-refs: remove newly empty directories

[Greg Price]

In a large repository which uses directories to organize many refs,
"git pack-refs --all --prune" does not improve performance so much
as it should, unless we remove all the now-empty directories as well.

Signed-off-by: Greg Price <price@ksplice.com>
---
 pack-refs.c          |    8 ++++++++
 t/t3210-pack-refs.sh |    6 ++++++
 2 files changed, 14 insertions(+), 0 deletions(-)

diff --git a/pack-refs.c b/pack-refs.c
index 7f43f8a..33d7358 100644
--- a/pack-refs.c
+++ b/pack-refs.c
@@ -63,11 +63,19 @@ static int handle_one_ref(const char *path, const unsigned char *sha1,
 /* make sure nobody touched the ref, and unlink */
 static void prune_ref(struct ref_to_prune *r)
 {
+	char *p, *q;
 	struct ref_lock *lock = lock_ref_sha1(r->name + 5, r->sha1);
 
 	if (lock) {
 		unlink_or_warn(git_path("%s", r->name));
 		unlock_ref(lock);
+
+		/* remove the directory if empty */
+		for (q = p = r->name; *p; p++)
+			if (*p == '/')
+				q = p;
+		*q = '\0';
+		rmdir(r->name);
 	}
 }
 
diff --git a/t/t3210-pack-refs.sh b/t/t3210-pack-refs.sh
index 413019a..c60ede1 100755
--- a/t/t3210-pack-refs.sh
+++ b/t/t3210-pack-refs.sh
@@ -60,6 +60,12 @@ 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 &&
+     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.6.6.32.g6380e

Re: [PATCH] pack-refs: remove newly empty directories

[Junio C Hamano]

Greg Price <price@ksplice.com> writes:

> diff --git a/pack-refs.c b/pack-refs.c
> index 7f43f8a..33d7358 100644
> --- a/pack-refs.c
> +++ b/pack-refs.c
> @@ -63,11 +63,19 @@ static int handle_one_ref(const char *path, const unsigned char *sha1,
>  /* make sure nobody touched the ref, and unlink */
>  static void prune_ref(struct ref_to_prune *r)
>  {
> +	char *p, *q;
>  	struct ref_lock *lock = lock_ref_sha1(r->name + 5, r->sha1);
>  
>  	if (lock) {
>  		unlink_or_warn(git_path("%s", r->name));
>  		unlock_ref(lock);
> +
> +		/* remove the directory if empty */
> +		for (q = p = r->name; *p; p++)
> +			if (*p == '/')
> +				q = p;
> +		*q = '\0';
> +		rmdir(r->name);
>  	}
>  }

Will this keep refs/heads/p/q that is empty after packing p/q/r/s branch
that happens to be the only branch whose name begins with p/?

I do not want a careless loop that will remove refs/heads after packing
"master" that happens to be the only local branch, but still...

Re: [PATCH] pack-refs: remove newly empty directories

[Greg Price]

On Mon, Jul 5, 2010 at 11:02 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Will this keep refs/heads/p/q that is empty after packing p/q/r/s branch
> that happens to be the only branch whose name begins with p/?
>
> I do not want a careless loop that will remove refs/heads after packing
> "master" that happens to be the only local branch, but still...

It will.  I could fix that with something like this (untested):

/* 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++;
    if (*p)
      p++;
  }
  for (q = p; *q; q++)
    ;
  while (1) {
    for ( ; q > p && *q != '/'; q--)
      ;
    if (q == p)
      break;
    *q = '\0';
    if (rmdir(git_path("%s", name)))
      break;
  }
}

and then

      if (lock) {
              unlink_or_warn(git_path("%s", r->name));
              unlock_ref(lock);
+              try_remove_empty_parents(r->name);
      }

Sound reasonable?

Greg

[PATCH v2] pack-refs: remove newly empty directories

[Greg Price]

In a large repository which uses directories to organize many refs,
"git pack-refs --all --prune" does not improve performance so much
as it should, unless we remove all the now-empty directories as well.

Signed-off-by: Greg Price <price@ksplice.com>
---
This version removes empty grandparent directories, etc, but always
leaves in place refs/heads/ and its siblings.

We also tolerate duplicate slashes in refnames, because
check_ref_format() in refs.c does the same.

 pack-refs.c          |   30 ++++++++++++++++++++++++++++++
 t/t3210-pack-refs.sh |    6 ++++++
 2 files changed, 36 insertions(+), 0 deletions(-)

diff --git a/pack-refs.c b/pack-refs.c
index 7f43f8a..a935856 100644
--- a/pack-refs.c
+++ b/pack-refs.c
@@ -60,6 +60,35 @@ 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_ref_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)
 {
@@ -68,6 +97,7 @@ 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/t/t3210-pack-refs.sh b/t/t3210-pack-refs.sh
index 413019a..ffd4e9f 100755
--- a/t/t3210-pack-refs.sh
+++ b/t/t3210-pack-refs.sh
@@ -60,6 +60,12 @@ 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.6.6.32.g6380e

Re: [PATCH] pack-refs: remove newly empty directories

[Johannes Sixt]

Am 7/6/2010 0:27, schrieb Greg Price:
> In a large repository which uses directories to organize many refs,
> "git pack-refs --all --prune" does not improve performance so much
> as it should, unless we remove all the now-empty directories as well.

Before your patch, when you create a ref refs/heads/foo/bar, then pack
refs, then it is impossible to create a ref refs/heads/foo because
refs/heads/foo still exists as directory. And this is a good thing.

With your patch, is there any mechanism that inhibits that refs/heads/foo
is created when directory refs/heads/foo does not exist, but a packed ref
refs/heads/foo/bar is present?

-- Hannes

Re: [PATCH] pack-refs: remove newly empty directories

[Junio C Hamano]

Johannes Sixt <j.sixt@viscovery.net> writes:

> With your patch, is there any mechanism that inhibits that refs/heads/foo
> is created when directory refs/heads/foo does not exist, but a packed ref
> refs/heads/foo/bar is present?

$ git grep -e is_refname_available refs.c

Re: [PATCH] pack-refs: remove newly empty directories

[Andreas Schwab]

Greg Price <price@ksplice.com> writes:

> In a large repository which uses directories to organize many refs,
> "git pack-refs --all --prune" does not improve performance so much
> as it should, unless we remove all the now-empty directories as well.

What happens if a parallel running git wants to update a ref in one of
the now-empty directories?  Can it get a spurious error after it has
called safe_create_leading_directories?

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [PATCH] pack-refs: remove newly empty directories

[Greg Price]

On Tue, Jul 6, 2010 at 2:49 PM, Andreas Schwab <schwab@linux-m68k.org> wrote:
> What happens if a parallel running git wants to update a ref in one of
> the now-empty directories?  Can it get a spurious error after it has
> called safe_create_leading_directories?

Good question!  I asked the same question. =)  It can get the same
kind of error that happens if two parallel running git processes try
to update the same ref.

Anything that tries to update a ref will call lock_ref_sha1_basic(),
which calls safe_create_leading_directories() to make the directory
and then hold_lock_file_for_update() to open(O_CREAT|O_EXCL) a lock
file inside the directory.  So if the prune happens after the open(),
it will simply not remove the directory and all is well.  If it
happens between the safe_create_leading_directories() and the open(),
then the open() will fail.  This is also what happens if the lock has
been taken by another git process updating the same ref.  Currently
lock_ref_sha1_basic() chooses to have the process die in this case and
print an error message.

Similarly the prune could remove a directory just before
safe_create_leading_directories() creates one inside it.  Then
safe_create_leading_directories() will fail and loc_ref_sha1_basic()
will print an error message and return failure.  In both cases, since
we are exposed to a similar failure if a parallel process is updating
the same ref (a common operation) rather than packing refs (an
uncommon operation), I don't think this is a problem.

Greg