"git pack-refs --prune", after successfully packing the existing
refs, removes the loose ref files. It tries to protect against
race by doing the usual lock_ref_sha1() which makes sure the
contents of the ref has not changed since we last looked at.
I am not sure I got the locking right, hence this RFC.
We would probably need to perform some sort of 'sync' after
closing and renaming the lockfile to its final location before
pruning. Is there a way cheaper than sync(2) to make sure the
effect of rename(2) hits the disk platter?
---
diff --git a/builtin-pack-refs.c b/builtin-pack-refs.c
index 0f5d827..2b3a483 100644
--- a/builtin-pack-refs.c
+++ b/builtin-pack-refs.c
@@ -3,6 +3,15 @@ #include "refs.h"
static FILE *refs_file;
static const char *result_path, *lock_path;
+static const char builtin_pack_refs_usage[] =
+"git-pack-refs [--prune]";
+
+static int prune;
+struct keepref {
+ struct keepref *next;
+ unsigned char sha1[20];
+ char name[FLEX_ARRAY];
+} *keepref;
static void remove_lock_file(void)
{@@ -13,12 +22,50 @@ static void remove_lock_file(void)
static int handle_one_ref(const char *path, const unsigned char *sha1)
{
fprintf(refs_file, "%s %s\n", sha1_to_hex(sha1), path);
+ if (prune) {
+ int namelen = strlen(path) + 1;
+ struct keepref *n = xcalloc(1, sizeof(*n) + namelen);
+ hashcpy(n->sha1, sha1);
+ strcpy(n->name, path);
+ n->next = keepref;
+ keepref = n;
+ }
return 0;
}
+/* make sure nobody touched the ref, and unlink */
+static void prune_ref(struct keepref *r)
+{
+ struct ref_lock *lock = lock_ref_sha1(r->name + 5, r->sha1, 1);
+
+ if (lock) {
+ unlink(git_path(r->name));
+ unlock_ref(lock);
+ }
+}
+
+static void prune_refs(void)
+{
+ struct keepref *r;
+ for (r = keepref; r; r = r->next)
+ prune_ref(r);
+}
+
int cmd_pack_refs(int argc, const char **argv, const char *prefix)
{
- int fd;
+ int fd, i;
+
+ for (i = 1; i < argc; i++) {
+ const char *arg = argv[i];
+ if (!strcmp(arg, "--prune")) {
+ prune = 1;
+ continue;
+ }
+ /* perhaps other parameters later... */
+ break;
+ }
+ if (i != argc)
+ usage(builtin_pack_refs_usage);
result_path = xstrdup(git_path("packed-refs"));
lock_path = xstrdup(mkpath("%s.lock", result_path));@@ -37,5 +84,7 @@ int cmd_pack_refs(int argc, const char *
if (rename(lock_path, result_path) < 0)
die("unable to overwrite old ref-pack file (%s)", strerror(errno));
lock_path = NULL;
+ if (prune)
+ prune_refs();
return 0;
}
On Mon, 18 Sep 2006, Junio C Hamano wrote:
I am not sure I got the locking right, hence this RFC.
It looks correct (the important part to check is that the SHA1 of the ref
you remove still matches the SHA1 of the object you packed).
That said, we should fix it up a bit, notably
- we should _not_ prune refs that are indirect.
Right now, if we have a symbolic link, we _incorrectly_ pack it as
unlinked. The packed format doesn't have any "link" format.
This isn't a problem in practice, because the only link we ever use is
the HEAD link, but it's incorrect. As long as we don't prune, it wasn't
an issue - a unpacked head will always override a packed one, so
packing the thing didn't really matter.
- we should probably avoid even trying to prune stuff that was already
packed.
The way to fix both these problems at once would be to add a flag to the
"for_each_ref()", which says whether it followed a link, or whether it was
already packed, so that we wouldn't pack symlinks at all, and we wouldn't
add already-packed refs to the "keeprefs" list.
But that requires a sligh semantic extension to "do_for_each_ref()" (and
"struct ref_list" needs a flag to say whether it was looked up through a
symlink).
I was thinking that the easy way to solve it is to just _pack_ everything
(the way we do now - incorrectly for symrefs), but never prune a symref.
Linus
Linus Torvalds [off-list ref] writes:
The way to fix both these problems at once would be to add a flag to the
"for_each_ref()", which says whether it followed a link, or whether it was
already packed, so that we wouldn't pack symlinks at all, and we wouldn't
add already-packed refs to the "keeprefs" list.
But that requires a sligh semantic extension to "do_for_each_ref()" (and
"struct ref_list" needs a flag to say whether it was looked up through a
symlink).
I was thinking that the easy way to solve it is to just _pack_ everything
(the way we do now - incorrectly for symrefs), but never prune a symref.
I see. Thanks for pointing out the issue with symrefs. I think
clone with --use-separate-remote creates remote/$that_repo/HEAD
that points at the branch the remote side's HEAD points at (to
be precise, the one it guessed the remote side's HEAD points
at), so this is a real issue already.
I wanted to update for_each_ref() anyway for other reasons (it
really should take callback data -- the way the current users
use global variables to work this around is eyesore), so
hopefully I'll find time to take a look at it.
Rough outline:
- for_each_ref() and friends become:
typedef int each_ref_fn(const char *refname,
const unsigned char *sha1,
#define REF_IS_SYMREF 01
#define REF_IS_PACKED 02
int flags, /* above bits or'ed */
void *cb_data);
int for_each_ref(each_ref_fn fn, void *cb_data);
- handle_one_ref notices a symref and ignores it; it remembers
refs that are not symref and are still loose for later
pruning under --prune.
We might want to update the initial handshake of upload-pack
protocol so that peek-remote and fetch-pack can tell which one
is a symref pointing at what. Do the usual server_capabilities
discovery in connect.c::get_remote_heads(), and if an extension
"symref" is supported than ask for symref information (typically
we would only get "HEAD points at refs/heads/foo" and nothing
else). Then git-clone.sh does not have to make a guess. But
that is a separate topic.