Re: [RFC][PATCH] for_each_ref() returning heads in wrong order

From: Linus Torvalds <torvalds@osdl.org>
Date: Sep 23, 2006, 17:12 UTC
Message-ID: <Pine.LNX.4.64.0609231004210.4388@g5.osdl.org>
In-Reply-To: <20060923160712.5890.73139.stgit@machine.or.cz>

On Sat, 23 Sep 2006, Petr Baudis wrote:

Show 7 quoted lines

>
> Using the #next branch I've now hit a problem with git-fetch-pack
> master choosing refs/bases/master (I geuss created by StGIT) instead
> of refs/heads/master. The old upload-pack returned the refs in the order
> heads-tags-everything_else but the new one just goes for whatever order
> readdir() returns them in (modulo merging with packed refs). I actually
> can't see the difference that caused this right now, though.

Actually, I think it's exactly the other way around.

The _old_ "for_each_refs()" returned things in a totally random order, depending on the readdir(). On many (but not all) filesystems, that ends up being somewhat decided by the order the entries were created in, so for example, you'd generally get "refs/heads/" and "refs/tags/" early (discounting HEAD, which is always a special case and comes first).

The _new_ thing is totally reliable. It returns things sorted alphabetically according to "strcmp". It doesn't matter what ordering readdir() gives.

Now, we could change the sorting order artificially, but I think your patch is actually incorrect. Because you no longer sort the list appropriately, the merge-sort done by "do_show_each_ref()" is no longer guaranteed to work, I think.

Ugly.

The proper way to fix it (if you want to do this at all) is to just define the sort-order to be something else than a plain "strcmp()", and change the things that compare ordering to just use the new ordering instead.

In other words, start from something like THIS, and just change "ref_name_compare()" to taste. Make sure it's a complete ordering, though.

		Linus

---

diff --git a/refs.c b/refs.c
index 2cef2b4..05b7006 100644
--- a/refs.c
+++ b/refs.c
@@ -37,6 +37,17 @@ static const char *parse_ref_line(char *
 	return line;
 }
 
+/*
+ * This is the ordering function for refnames. It has the
+ * same semantics as "strcmp()", but you can define it to
+ * order "refs/heads/" and "refs/tags/" before other names,
+ * for example.
+ */
+static int ref_name_compare(const char *a, const char *b)
+{
+	return strcmp(a,b);
+}
+
 static struct ref_list *add_ref(const char *name, const unsigned char *sha1,
 				int flag, struct ref_list *list)
 {
@@ -45,7 +56,7 @@ static struct ref_list *add_ref(const ch
 
 	/* Find the place to insert the ref into.. */
 	while ((entry = *p) != NULL) {
-		int cmp = strcmp(entry->name, name);
+		int cmp = ref_name_compare(entry->name, name);
 		if (cmp > 0)
 			break;
 
@@ -179,7 +190,7 @@ const char *resolve_ref(const char *ref,
 		if (lstat(path, &st) < 0) {
 			struct ref_list *list = get_packed_refs();
 			while (list) {
-				if (!strcmp(ref, list->name)) {
+				if (!ref_name_compare(ref, list->name)) {
 					hashcpy(sha1, list->sha1);
 					if (flag)
 						*flag |= REF_ISPACKED;
@@ -297,7 +308,7 @@ static int do_for_each_ref(const char *b
 
 	while (packed && loose) {
 		struct ref_list *entry;
-		int cmp = strcmp(packed->name, loose->name);
+		int cmp = ref_name_compare(packed->name, loose->name);
 		if (!cmp) {
 			packed = packed->next;
 			continue;