Re: [PATCH 1/2] pack-refs: remove all empty directories under $GIT_DIR/refs
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Feb 11, 2012, 08:26 UTC
- Message-ID
- <7vhayxn5cg.fsf@alter.siamese.dyndns.org>
- In-Reply-To
- <1328946907-31650-1-git-send-email-pclouds@gmail.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.or something?
Show 11 quoted lines
> 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?
Show 25 quoted lines
>
> 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?
Show 19 quoted lines
> + 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.
Show 31 quoted lines
> + 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;