[PATCH 1/2] Move code resolving packed refs into its own function.

[PATCH 1/2] Move code resolving packed refs into its own function.

[Christian Couder]

This patch move Linus' packed refs resolving code from
"resolve_ref" into a new "resolve_packed_ref" extern
function so that it can be reused when needed.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 cache.h |    2 ++
 refs.c  |   34 ++++++++++++++++++++--------------
 2 files changed, 22 insertions(+), 14 deletions(-)

diff --git a/cache.h b/cache.h
index 4e01a74..f37bc18 100644
--- a/cache.h
+++ b/cache.h
@@ -296,6 +296,8 @@ extern int get_sha1_hex(const char *hex,
 extern char *sha1_to_hex(const unsigned char *sha1);	/* static buffer result! */
 extern int read_ref(const char *filename, unsigned char *sha1);
 extern const char *resolve_ref(const char *path, unsigned char *sha1, int, int *);
+extern const char *resolve_packed_ref(const char *ref, unsigned char *sha1, int, int *);
+
 extern int create_symref(const char *ref, const char *refs_heads_master);
 extern int validate_symref(const char *ref);
 
diff --git a/refs.c b/refs.c
index 3d4cdd1..5e3988b 100644
--- a/refs.c
+++ b/refs.c
@@ -148,6 +148,25 @@ static struct ref_list *get_loose_refs(v
 	return refs;
 }
 
+const char *resolve_packed_ref(const char *ref, unsigned char *sha1,
+			       int reading, int *flag)
+{
+	struct ref_list *list = get_packed_refs();
+	while (list) {
+		if (!strcmp(ref, list->name)) {
+			hashcpy(sha1, list->sha1);
+			if (flag)
+				*flag |= REF_ISPACKED;
+			return ref;
+		}
+		list = list->next;
+	}
+	if (reading || errno != ENOENT)
+		return NULL;
+	hashclr(sha1);
+	return ref;
+}
+
 /* We allow "recursive" symbolic refs. Only within reason, though */
 #define MAXDEPTH 5
 
@@ -177,20 +196,7 @@ const char *resolve_ref(const char *ref,
 		 * reading.
 		 */
 		if (lstat(path, &st) < 0) {
-			struct ref_list *list = get_packed_refs();
-			while (list) {
-				if (!strcmp(ref, list->name)) {
-					hashcpy(sha1, list->sha1);
-					if (flag)
-						*flag |= REF_ISPACKED;
-					return ref;
-				}
-				list = list->next;
-			}
-			if (reading || errno != ENOENT)
-				return NULL;
-			hashclr(sha1);
-			return ref;
+			return resolve_packed_ref(ref, sha1, reading, flag);
 		}
 
 		/* Follow "normalized" - ie "refs/.." symlinks by hand */
-- 
1.4.2.1.g7cf04

Re: [PATCH 1/2] Move code resolving packed refs into its own function.

[Junio C Hamano]

Christian Couder <chriscool@tuxfamily.org> writes:

> This patch move Linus' packed refs resolving code from
> "resolve_ref" into a new "resolve_packed_ref" extern
> function so that it can be reused when needed.

I think we are stepping on each other's toes.  How far into the
process of making correct branch deletion are you?

[PATCH 0/6] ref deletion and D/F conflict avoidance with packed-refs.

[Junio C Hamano]

This series cleans up the area that was affected by the recent
addition of "packed-refs".  Christian Couder and Jeff King CC'ed
since they seem to be touching in the general vicinity of the
code these patches touch.

[1/6] ref locking: allow 'foo' when 'foo/bar' used to exist but not anymore.
[2/6] refs: minor restructuring of cached refs data.
[3/6] lock_ref_sha1(): do not sometimes error() and sometimes die().
[4/6] lock_ref_sha1(): check D/F conflict with packed ref when creating.
[5/6] delete_ref(): delete packed ref
[6/6] git-branch: remove D/F check done by hand.

I opted for removing from the packed-ref file when a ref that is
packed is deleted.  

[PATCH 1/6] ref locking: allow 'foo' when 'foo/bar' used to exist but not anymore.

[Junio C Hamano]

It is normal to have .git/refs/heads/foo directory which is
empty after the last branch whose name starts with foo/ is
removed.  Make sure we notice this case and allow creation of
branch foo by removing the empty directory.

Signed-off-by: Junio C Hamano <junkio@cox.net>
---
 * This and the next one are essentially the same as my previous
   two patch series.

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

diff --git a/refs.c b/refs.c
index 3d4cdd1..b433c0c 100644
--- a/refs.c
+++ b/refs.c
@@ -473,6 +473,59 @@ static struct ref_lock *verify_lock(stru
 	return lock;
 }
 
+static int remove_empty_dir_recursive(char *path, int len)
+{
+	DIR *dir = opendir(path);
+	struct dirent *e;
+	int ret = 0;
+
+	if (!dir)
+		return -1;
+	if (path[len-1] != '/')
+		path[len++] = '/';
+	while ((e = readdir(dir)) != NULL) {
+		struct stat st;
+		int namlen;
+		if ((e->d_name[0] == '.') &&
+		    ((e->d_name[1] == 0) ||
+		     ((e->d_name[1] == '.') && e->d_name[2] == 0)))
+			continue; /* "." and ".." */
+
+		namlen = strlen(e->d_name);
+		if ((len + namlen < PATH_MAX) &&
+		    strcpy(path + len, e->d_name) &&
+		    !lstat(path, &st) &&
+		    S_ISDIR(st.st_mode) &&
+		    remove_empty_dir_recursive(path, len + namlen))
+			continue; /* happy */
+
+		/* path too long, stat fails, or non-directory still exists */
+		ret = -1;
+		break;
+	}
+	closedir(dir);
+	if (!ret) {
+		path[len] = 0;
+		ret = rmdir(path);
+	}
+	return ret;
+}
+
+static int remove_empty_directories(char *file)
+{
+	/* we want to create a file but there is a directory there;
+	 * if that is an empty directory (or a directory that contains
+	 * only empty directories), remove them.
+	 */
+	char path[PATH_MAX];
+	int len = strlen(file);
+
+	if (len >= PATH_MAX) /* path too long ;-) */
+		return -1;
+	strcpy(path, file);
+	return remove_empty_dir_recursive(path, len);
+}
+
 static struct ref_lock *lock_ref_sha1_basic(const char *ref, const unsigned char *old_sha1)
 {
 	char *ref_file;
@@ -485,6 +538,17 @@ static struct ref_lock *lock_ref_sha1_ba
 	lock->lock_fd = -1;
 
 	ref = resolve_ref(ref, lock->old_sha1, mustexist, NULL);
+	if (!ref && errno == EISDIR) {
+		/* we are trying to lock foo but we used to
+		 * have foo/bar which now does not exist;
+		 * it is normal for the empty directory 'foo'
+		 * to remain.
+		 */
+		ref_file = git_path("%s", orig_ref);
+		if (remove_empty_directories(ref_file))
+			die("there are still refs under '%s'", orig_ref);
+		ref = resolve_ref(orig_ref, lock->old_sha1, mustexist, NULL);
+	}
 	if (!ref) {
 		int last_errno = errno;
 		error("unable to resolve reference %s: %s",
-- 
1.4.2.1.g5a98f

[PATCH 2/6] refs: minor restructuring of cached refs data.

[Junio C Hamano]

Once we read packed and loose refs, for_each_ref() and friends
kept using them even after write_ref_sha1() and delete_ref()
changed the refs.  This adds invalidate_cached_refs() as a way
to flush the cached information.

Signed-off-by: Junio C Hamano <junkio@cox.net>
---
 * This and the previous one are essentially the same as my previous
   two patch series.

   getting rid of "we run only once" mentality.  otherwise we
   can never libify the thing...

 refs.c |   56 +++++++++++++++++++++++++++++++++++++++++++-------------
 1 files changed, 43 insertions(+), 13 deletions(-)

diff --git a/refs.c b/refs.c
index b433c0c..6ee5f96 100644
--- a/refs.c
+++ b/refs.c
@@ -66,12 +66,42 @@ static struct ref_list *add_ref(const ch
 	return list;
 }
 
-static struct ref_list *get_packed_refs(void)
+/*
+ * Future: need to be in "struct repository"
+ * when doing a full libification.
+ */
+struct cached_refs {
+	char did_loose;
+	char did_packed;
+	struct ref_list *loose;
+	struct ref_list *packed;
+} cached_refs;
+
+static void free_ref_list(struct ref_list *list)
+{
+	struct ref_list *next;
+	for ( ; list; list = next) {
+		next = list->next;
+		free(list);
+	}
+}
+
+static void invalidate_cached_refs(void)
 {
-	static int did_refs = 0;
-	static struct ref_list *refs = NULL;
+	struct cached_refs *ca = &cached_refs;
+
+	if (ca->did_loose && ca->loose)
+		free_ref_list(ca->loose);
+	if (ca->did_packed && ca->packed)
+		free_ref_list(ca->packed);
+	ca->loose = ca->packed = NULL;
+	ca->did_loose = ca->did_packed = 0;
+}
 
-	if (!did_refs) {
+static struct ref_list *get_packed_refs(void)
+{
+	if (!cached_refs.did_packed) {
+		struct ref_list *refs = NULL;
 		FILE *f = fopen(git_path("packed-refs"), "r");
 		if (f) {
 			struct ref_list *list = NULL;
@@ -86,9 +116,10 @@ static struct ref_list *get_packed_refs(
 			fclose(f);
 			refs = list;
 		}
-		did_refs = 1;
+		cached_refs.packed = refs;
+		cached_refs.did_packed = 1;
 	}
-	return refs;
+	return cached_refs.packed;
 }
 
 static struct ref_list *get_ref_dir(const char *base, struct ref_list *list)
@@ -138,14 +169,11 @@ static struct ref_list *get_ref_dir(cons
 
 static struct ref_list *get_loose_refs(void)
 {
-	static int did_refs = 0;
-	static struct ref_list *refs = NULL;
-
-	if (!did_refs) {
-		refs = get_ref_dir("refs", NULL);
-		did_refs = 1;
+	if (!cached_refs.did_loose) {
+		cached_refs.loose = get_ref_dir("refs", NULL);
+		cached_refs.did_loose = 1;
 	}
-	return refs;
+	return cached_refs.loose;
 }
 
 /* We allow "recursive" symbolic refs. Only within reason, though */
@@ -401,6 +429,7 @@ int delete_ref(const char *refname, unsi
 		fprintf(stderr, "warning: unlink(%s) failed: %s",
 			lock->log_file, strerror(errno));
 
+	invalidate_cached_refs();
 	return ret;
 }
 
@@ -665,6 +694,7 @@ int write_ref_sha1(struct ref_lock *lock
 		unlock_ref(lock);
 		return -1;
 	}
+	invalidate_cached_refs();
 	if (log_ref_write(lock, sha1, logmsg) < 0) {
 		unlock_ref(lock);
 		return -1;
-- 
1.4.2.1.g5a98f

[PATCH 3/6] lock_ref_sha1(): do not sometimes error() and sometimes die().

[Junio C Hamano]

This cleans up the error path in the function so it does not
die() itself sometimes while signalling an error with NULL some
other times which was inconsistent and confusing.

Signed-off-by: Junio C Hamano <junkio@cox.net>
---
 refs.c |   26 ++++++++++++++++++--------
 1 files changed, 18 insertions(+), 8 deletions(-)

diff --git a/refs.c b/refs.c
index 6ee5f96..157de43 100644
--- a/refs.c
+++ b/refs.c
@@ -561,6 +561,7 @@ static struct ref_lock *lock_ref_sha1_ba
 	const char *orig_ref = ref;
 	struct ref_lock *lock;
 	struct stat st;
+	int last_errno = 0;
 	int mustexist = (old_sha1 && !is_null_sha1(old_sha1));
 
 	lock = xcalloc(1, sizeof(struct ref_lock));
@@ -574,17 +575,18 @@ static struct ref_lock *lock_ref_sha1_ba
 		 * to remain.
 		 */
 		ref_file = git_path("%s", orig_ref);
-		if (remove_empty_directories(ref_file))
-			die("there are still refs under '%s'", orig_ref);
+		if (remove_empty_directories(ref_file)) {
+			last_errno = errno;
+			error("there are still refs under '%s'", orig_ref);
+			goto error_return;
+		}
 		ref = resolve_ref(orig_ref, lock->old_sha1, mustexist, NULL);
 	}
 	if (!ref) {
-		int last_errno = errno;
+		last_errno = errno;
 		error("unable to resolve reference %s: %s",
 			orig_ref, strerror(errno));
-		unlock_ref(lock);
-		errno = last_errno;
-		return NULL;
+		goto error_return;
 	}
 	lock->lk = xcalloc(1, sizeof(struct lock_file));
 
@@ -593,11 +595,19 @@ static struct ref_lock *lock_ref_sha1_ba
 	ref_file = git_path("%s", ref);
 	lock->force_write = lstat(ref_file, &st) && errno == ENOENT;
 
-	if (safe_create_leading_directories(ref_file))
-		die("unable to create directory for %s", ref_file);
+	if (safe_create_leading_directories(ref_file)) {
+		last_errno = errno;
+		error("unable to create directory for %s", ref_file);
+		goto error_return;
+	}
 	lock->lock_fd = hold_lock_file_for_update(lock->lk, ref_file, 1);
 
 	return old_sha1 ? verify_lock(lock, old_sha1, mustexist) : lock;
+
+ error_return:
+	unlock_ref(lock);
+	errno = last_errno;
+	return NULL;
 }
 
 struct ref_lock *lock_ref_sha1(const char *ref, const unsigned char *old_sha1)
-- 
1.4.2.1.g5a98f

[PATCH 4/6] lock_ref_sha1(): check D/F conflict with packed ref when creating.

[Junio C Hamano]

This makes the ref locking codepath to notice if an existing ref
overlaps with the ref we are creating.

Signed-off-by: Junio C Hamano <junkio@cox.net>
---
 refs.c |   24 ++++++++++++++++++++++++
 1 files changed, 24 insertions(+), 0 deletions(-)

diff --git a/refs.c b/refs.c
index 157de43..2bfa92a 100644
--- a/refs.c
+++ b/refs.c
@@ -588,6 +588,30 @@ static struct ref_lock *lock_ref_sha1_ba
 			orig_ref, strerror(errno));
 		goto error_return;
 	}
+	if (is_null_sha1(lock->old_sha1)) {
+		/* The ref did not exist and we are creating it.
+		 * Make sure there is no existing ref that is packed
+		 * whose name begins with our refname, nor a ref whose
+		 * name is a proper prefix of our refname.
+		 */
+		int namlen = strlen(ref); /* e.g. 'foo/bar' */
+		struct ref_list *list = get_packed_refs();
+		while (list) {
+			/* list->name could be 'foo' or 'foo/bar/baz' */
+			int len = strlen(list->name);
+			int cmplen = (namlen < len) ? namlen : len;
+			const char *lead = (namlen < len) ? list->name : ref;
+
+			if (!strncmp(ref, list->name, cmplen) &&
+			    lead[cmplen] == '/') {
+				error("'%s' exists; cannot create '%s'",
+				      list->name, ref);
+				goto error_return;
+			}
+			list = list->next;
+		}
+	}
+
 	lock->lk = xcalloc(1, sizeof(struct lock_file));
 
 	lock->ref_name = xstrdup(ref);
-- 
1.4.2.1.g5a98f

[PATCH 5/6] delete_ref(): delete packed ref

[Junio C Hamano]

This implements deletion of a packed ref.  Since it is a very
rare event to delete a ref compared to looking up, creating and
updating, this opts to remove the ref from the packed-ref file
instead of doing any of the filesystem based "negative ref" trick
to optimize the deletion path.

Signed-off-by: Junio C Hamano <junkio@cox.net>
---
 * This looks larger than it actually is, because it moves delete_ref()
   down so that it can use lock_ref_sha1_basic().

 refs.c |  109 +++++++++++++++++++++++++++++++++++++++++++++-------------------
 1 files changed, 77 insertions(+), 32 deletions(-)

diff --git a/refs.c b/refs.c
index 2bfa92a..858c534 100644
--- a/refs.c
+++ b/refs.c
@@ -406,33 +406,6 @@ int get_ref_sha1(const char *ref, unsign
 	return read_ref(mkpath("refs/%s", ref), sha1);
 }
 
-int delete_ref(const char *refname, unsigned char *sha1)
-{
-	struct ref_lock *lock;
-	int err, i, ret = 0;
-
-	lock = lock_any_ref_for_update(refname, sha1);
-	if (!lock)
-		return 1;
-	i = strlen(lock->lk->filename) - 5; /* .lock */
-	lock->lk->filename[i] = 0;
-	err = unlink(lock->lk->filename);
-	if (err) {
-		ret = 1;
-		error("unlink(%s) failed: %s",
-		      lock->lk->filename, strerror(errno));
-	}
-	lock->lk->filename[i] = '.';
-
-	err = unlink(lock->log_file);
-	if (err && errno != ENOENT)
-		fprintf(stderr, "warning: unlink(%s) failed: %s",
-			lock->log_file, strerror(errno));
-
-	invalidate_cached_refs();
-	return ret;
-}
-
 /*
  * Make sure "ref" is something reasonable to have under ".git/refs/";
  * We do not like it if:
@@ -555,7 +528,7 @@ static int remove_empty_directories(char
 	return remove_empty_dir_recursive(path, len);
 }
 
-static struct ref_lock *lock_ref_sha1_basic(const char *ref, const unsigned char *old_sha1)
+static struct ref_lock *lock_ref_sha1_basic(const char *ref, const unsigned char *old_sha1, int *flag)
 {
 	char *ref_file;
 	const char *orig_ref = ref;
@@ -567,7 +540,7 @@ static struct ref_lock *lock_ref_sha1_ba
 	lock = xcalloc(1, sizeof(struct ref_lock));
 	lock->lock_fd = -1;
 
-	ref = resolve_ref(ref, lock->old_sha1, mustexist, NULL);
+	ref = resolve_ref(ref, lock->old_sha1, mustexist, flag);
 	if (!ref && errno == EISDIR) {
 		/* we are trying to lock foo but we used to
 		 * have foo/bar which now does not exist;
@@ -580,7 +553,7 @@ static struct ref_lock *lock_ref_sha1_ba
 			error("there are still refs under '%s'", orig_ref);
 			goto error_return;
 		}
-		ref = resolve_ref(orig_ref, lock->old_sha1, mustexist, NULL);
+		ref = resolve_ref(orig_ref, lock->old_sha1, mustexist, flag);
 	}
 	if (!ref) {
 		last_errno = errno;
@@ -640,12 +613,84 @@ struct ref_lock *lock_ref_sha1(const cha
 	if (check_ref_format(ref))
 		return NULL;
 	strcpy(refpath, mkpath("refs/%s", ref));
-	return lock_ref_sha1_basic(refpath, old_sha1);
+	return lock_ref_sha1_basic(refpath, old_sha1, NULL);
 }
 
 struct ref_lock *lock_any_ref_for_update(const char *ref, const unsigned char *old_sha1)
 {
-	return lock_ref_sha1_basic(ref, old_sha1);
+	return lock_ref_sha1_basic(ref, old_sha1, NULL);
+}
+
+static int repack_without_ref(const char *refname)
+{
+	struct ref_list *list, *packed_ref_list;
+	int fd;
+	int found = 0;
+	struct lock_file packlock;
+
+	packed_ref_list = get_packed_refs();
+	for (list = packed_ref_list; list; list = list->next) {
+		if (!strcmp(refname, list->name)) {
+			found = 1;
+			break;
+		}
+	}
+	if (!found)
+		return 0;
+	memset(&packlock, 0, sizeof(packlock));
+	fd = hold_lock_file_for_update(&packlock, git_path("packed-refs"), 0);
+	if (fd < 0)
+		return error("cannot delete '%s' from packed refs", refname);
+
+	for (list = packed_ref_list; list; list = list->next) {
+		char line[PATH_MAX + 100];
+		int len;
+
+		if (!strcmp(refname, list->name))
+			continue;
+		len = snprintf(line, sizeof(line), "%s %s\n",
+			       sha1_to_hex(list->sha1), list->name);
+		/* this should not happen but just being defensive */
+		if (len > sizeof(line))
+			die("too long a refname '%s'", list->name);
+		write_or_die(fd, line, len);
+	}
+	return commit_lock_file(&packlock);
+}
+
+int delete_ref(const char *refname, unsigned char *sha1)
+{
+	struct ref_lock *lock;
+	int err, i, ret = 0, flag = 0;
+
+	lock = lock_ref_sha1_basic(refname, sha1, &flag);
+	if (!lock)
+		return 1;
+	if (!(flag & REF_ISPACKED)) {
+		/* loose */
+		i = strlen(lock->lk->filename) - 5; /* .lock */
+		lock->lk->filename[i] = 0;
+		err = unlink(lock->lk->filename);
+		if (err) {
+			ret = 1;
+			error("unlink(%s) failed: %s",
+			      lock->lk->filename, strerror(errno));
+		}
+		lock->lk->filename[i] = '.';
+	}
+	/* removing the loose one could have resurrected an earlier
+	 * packed one.  Also, if it was not loose we need to repack
+	 * without it.
+	 */
+	ret |= repack_without_ref(refname);
+
+	err = unlink(lock->log_file);
+	if (err && errno != ENOENT)
+		fprintf(stderr, "warning: unlink(%s) failed: %s",
+			lock->log_file, strerror(errno));
+	invalidate_cached_refs();
+	unlock_ref(lock);
+	return ret;
 }
 
 void unlock_ref(struct ref_lock *lock)
-- 
1.4.2.1.g5a98f

[PATCH 6/6] git-branch: remove D/F check done by hand.

[Junio C Hamano]

Now ref creation codepath in lock_ref_sha1() and friends notices
the directory/file conflict situation, we do not do this by hand
in git-branch anymore.

Signed-off-by: Junio C Hamano <junkio@cox.net>
---
 git-branch.sh |   10 ----------
 1 files changed, 0 insertions(+), 10 deletions(-)

diff --git a/git-branch.sh b/git-branch.sh
index c616830..bf84b30 100755
--- a/git-branch.sh
+++ b/git-branch.sh
@@ -121,16 +121,6 @@ then
 	done
 fi
 
-branchdir=$(dirname $branchname)
-while test "$branchdir" != "."
-do
-	if git-show-ref --verify --quiet -- "refs/heads/$branchdir"
-	then
-		die "$branchdir already exists."
-	fi
-	branchdir=$(dirname $branchdir)
-done
-
 prev=''
 if git-show-ref --verify --quiet -- "refs/heads/$branchname"
 then
-- 
1.4.2.1.g5a98f

Re: [PATCH 0/6] ref deletion and D/F conflict avoidance with packed-refs.

[Jeff King]

On Sat, Sep 30, 2006 at 03:26:25PM -0700, Junio C Hamano wrote:

> This series cleans up the area that was affected by the recent
> addition of "packed-refs".  Christian Couder and Jeff King CC'ed
> since they seem to be touching in the general vicinity of the
> code these patches touch.

I don't believe there is any conflict between my patches and yours.

-Peff

Re: [PATCH 1/2] Move code resolving packed refs into its own function.

[Christian Couder]

Junio C Hamano wrote:
> Christian Couder <chriscool@tuxfamily.org> writes:
> > This patch move Linus' packed refs resolving code from
> > "resolve_ref" into a new "resolve_packed_ref" extern
> > function so that it can be reused when needed.
>
> I think we are stepping on each other's toes.  How far into the
> process of making correct branch deletion are you?

I am not farther than the 2 patches I sent yesterday (before going to bed).

Now I will probably add a few more test cases and see what needs to be done 
for tags. Tell me when you have finished with branch deletion so I can see 
what I can do in this area.

Thanks,
Christian.

Re: [PATCH 1/2] Move code resolving packed refs into its own function.

[Junio C Hamano]

Christian Couder <chriscool@tuxfamily.org> writes:

> Junio C Hamano wrote:
>> Christian Couder <chriscool@tuxfamily.org> writes:
>> > This patch move Linus' packed refs resolving code from
>> > "resolve_ref" into a new "resolve_packed_ref" extern
>> > function so that it can be reused when needed.
>>
>> I think we are stepping on each other's toes.  How far into the
>> process of making correct branch deletion are you?
>
> I am not farther than the 2 patches I sent yesterday (before going to bed).

Thanks.  I just did not want to waste your work with overlapping
duplicated efforts.

I think what we have in "next" tonight is in a more-or-less
testable shape, although it has still a long way to reach
"master".  Things I know we need to address:

 - I've updated lock_ref_sha1_basic() to remove empty left-over
   directories and to notice conflicts between 'foo/bar' vs
   'foo' when creating a new ref, hopefully in the same spirit
   as your patch to safe_create_leading_directories(), but done
   differently (safe_... function is meant to be used anywhere
   not just $GIT_DIR/refs/, and it felt wrong for it to take
   exception for packed refs).  We should do the same for the
   reflog hierarchy but we currently don't.

 - We need to audit our shell scripts to make sure they do not
   depend on being able to look directly into $GIT_DIR/refs to
   see if the ref they are interested in exists.  I've fixed a
   few in git-fetch while handling the patch to clean up its
   output from Santi, but I would not be surprised if there are
   more.  The code in git-branch and git-tag to list what's
   there are Ok; they use "rev-parse --symbolic --all/--tags".

 - I think gitweb should be Ok; it does peek-remote on the
   repository.  Although we would probably want to update
   git_get_references and git_get_refs_list sub to use
   for-each-ref there, that can be done as a later optimization.

 - Dumb transports are not aware of packed refs on the remote
   side.  The underlying commit walkers (anything that links
   with fetch.c) needs their fetch_ref() implementation updated
   to look at the packed-refs file from the remote side and we
   should be fine after that.  I haven't looked at rsync
   transport but the change necessary there shouldn't be too
   involved.

Re: [PATCH 1/2] Move code resolving packed refs into its own function.

[Jakub Narebski]

Junio C Hamano wrote:

>  - I think gitweb should be Ok; it does peek-remote on the
>    repository.  Although we would probably want to update
>    git_get_references and git_get_refs_list sub to use
>    for-each-ref there, that can be done as a later optimization.

I'd rather have gitweb use git-for-each-ref after the command
is in the official release... if not, then keep it in the 'next'
branch.

By the way, I have an idea to join the git-show-ref and git-for-each-ref:
simply add --deref option to git-for-each-ref, which would output
dereferenced tag just after the reference itself (well, perhaps with the
exception when sorting, perhaps not), with the deref name like in
git-show-ref and git-peek-remote, i.e. <ref>^{}
-- 
Jakub Narebski
Warsaw, Poland
ShadeHawk on #git

Re: [PATCH 1/2] Move code resolving packed refs into its own function.

[Junio C Hamano]

Jakub Narebski <jnareb@gmail.com> writes:

> Junio C Hamano wrote:
>
>>  - I think gitweb should be Ok; it does peek-remote on the
>>    repository.  Although we would probably want to update
>>    git_get_references and git_get_refs_list sub to use
>>    for-each-ref there, that can be done as a later optimization.
>
> I'd rather have gitweb use git-for-each-ref after the command
> is in the official release... if not, then keep it in the 'next'
> branch.

Of course.  What else did you expect?

> By the way, I have an idea to join the git-show-ref and git-for-each-ref:
> simply add --deref option to git-for-each-ref, which would output
> dereferenced tag just after the reference itself (well, perhaps with the
> exception when sorting, perhaps not), with the deref name like in
> git-show-ref and git-peek-remote, i.e. <ref>^{}

Patches welcome.  --verify needs to use totally separate
codepath though.  for-each-ref is primarily for easy displaying
and expects the caller to script in order to skip ones that are
irrelevant while listing (e.g. asking for *objectname does not
error for a non-tag but the caller can deduce that ref is not a
tag by it being empty and skip it); show-ref --verify is for
easy checking for an exact match and should not even walk.