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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




On Sat, 23 Sep 2006, Petr Baudis wrote:
>
> 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;
-
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]