{"thread":{"id":"5662","subject":"[RFC][PATCH] for_each_ref() returning heads in wrong order","startedAt":"2006-09-23T16:36:21Z","lastAt":"2006-09-23T19:31:59Z","messageCount":6,"participants":["Petr Baudis","Linus Torvalds","Johannes Schindelin","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"27481","messageId":"20060923160712.5890.73139.stgit@machine.or.cz","threadId":"5662","inReplyTo":null,"subject":"[RFC][PATCH] for_each_ref() returning heads in wrong order","fromName":"Petr Baudis","fromEmail":"pasky@suse.cz","sentAt":"2006-09-23T16:36:21Z","receivedAt":"2006-09-23T16:36:21Z","isPatch":true,"sender":{"key":"pasky@ucw.cz","avatar":"https://avatars.githubusercontent.com/u/18439?v=4"},"body":"Using the #next branch I've now hit a problem with git-fetch-pack\nmaster choosing refs/bases/master (I geuss created by StGIT) instead\nof refs/heads/master. The old upload-pack returned the refs in the order\nheads-tags-everything_else but the new one just goes for whatever order\nreaddir() returns them in (modulo merging with packed refs). I actually\ncan't see the difference that caused this right now, though.\n\nThis is a _really ugly_ patch to fix that. I wonder if there's a more\nelegant solution to this.\n\nIf you have your refs already packed, you should somehow unpack them\nand then pack them again.\n\nSigned-off-by: Petr Baudis \n---\n\n I send the patch with the hope that someone will be so irritated by it\nthat he'll create a nicer one. ;-)\n\n refs.c | 39 ++++++++++++++++++++++++++++++++++++---\n 1 files changed, 36 insertions(+), 3 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 2cef2b4..9b3986a 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -91,7 +91,8 @@ static struct ref_list *get_packed_refs(\n \treturn refs;\n }\n \n-static struct ref_list *get_ref_dir(const char *base, struct ref_list *list)\n+static struct ref_list *get_ref_dir(const char *base, struct ref_list *list,\n+ char **except)\n {\n \tDIR *dir = opendir(git_path(\"%s\", base));\n \n@@ -121,7 +122,13 @@ static struct ref_list *get_ref_dir(cons\n \t\t\tif (stat(git_path(\"%s\", ref), &st) < 0)\n \t\t\t\tcontinue;\n \t\t\tif (S_ISDIR(st.st_mode)) {\n-\t\t\t\tlist = get_ref_dir(ref, list);\n+\t\t\t\tif (except) {\n+\t\t\t\t\tchar **e;\n+\t\t\t\t\tfor (e = except; *e; e++)\n+\t\t\t\t\t\tif (!strcmp(*e, ref))\n+\t\t\t\t\t\t\tgoto nextitem;\n+\t\t\t\t}\n+\t\t\t\tlist = get_ref_dir(ref, list, except);\n \t\t\t\tcontinue;\n \t\t\t}\n \t\t\tif (!resolve_ref(ref, sha1, 1, &flag)) {\n@@ -129,6 +136,7 @@ static struct ref_list *get_ref_dir(cons\n \t\t\t\tcontinue;\n \t\t\t}\n \t\t\tlist = add_ref(ref, sha1, flag, list);\n+nextitem:;\n \t\t}\n \t\tfree(ref);\n \t\tclosedir(dir);\n@@ -142,7 +150,23 @@ static struct ref_list *get_loose_refs(v\n \tstatic struct ref_list *refs = NULL;\n \n \tif (!did_refs) {\n-\t\trefs = get_ref_dir(\"refs\", NULL);\n+\t\t/* We need to make sure refs/heads and refs/tags always\n+\t\t * go before everything else. */\n+\t\tchar *except[] = {\"refs/heads\", \"refs/tags\", NULL};\n+\t\tstruct ref_list *other_refs = NULL, *r;\n+\n+\t\trefs = get_ref_dir(\"refs/heads\", NULL, NULL);\n+\t\trefs = get_ref_dir(\"refs/tags\", refs, NULL);\n+\n+\t\tother_refs = get_ref_dir(\"refs\", NULL, except);\n+\t\tif (refs) {\n+\t\t\tfor (r = refs; r->next; r = r->next)\n+\t\t\t\t;\n+\t\t\tr->next = other_refs;\n+\t\t} else {\n+\t\t\trefs = other_refs;\n+\t\t}\n+\n \t\tdid_refs = 1;\n \t}\n \treturn refs;\n@@ -298,10 +322,19 @@ static int do_for_each_ref(const char *b\n \twhile (packed && loose) {\n \t\tstruct ref_list *entry;\n \t\tint cmp = strcmp(packed->name, loose->name);\n+\t\tint packed_is_ht, loose_is_ht;\n \t\tif (!cmp) {\n \t\t\tpacked = packed->next;\n \t\t\tcontinue;\n \t\t}\n+\t\t/* We need to make sure refs/heads and refs/tags always\n+\t\t * go before everything else. */\n+\t\tpacked_is_ht = !strncmp(packed->name, \"refs/heads\", 10)\n+\t\t || !strncmp(packed->name, \"refs/tags\", 9);\n+\t\tloose_is_ht = !strncmp(loose->name, \"refs/heads\", 10)\n+\t\t || !strncmp(loose->name, \"refs/tags\", 9);\n+\t\tif (packed_is_ht != loose_is_ht)\n+\t\t\tcmp = loose_is_ht - packed_is_ht;\n \t\tif (cmp > 0) {\n \t\t\tentry = loose;\n \t\t\tloose = loose->next;\n"},{"id":"27483","messageId":"20060923164759.GP8259@pasky.or.cz","threadId":"5662","inReplyTo":"20060923160712.5890.73139.stgit@machine.or.cz","subject":"Re: [RFC][PATCH] for_each_ref() returning heads in wrong order","fromName":"Petr Baudis","fromEmail":"pasky@suse.cz","sentAt":"2006-09-23T16:47:59Z","receivedAt":"2006-09-23T16:47:59Z","isPatch":true,"sender":{"key":"pasky@ucw.cz","avatar":"https://avatars.githubusercontent.com/u/18439?v=4"},"body":"Dear diary, on Sat, Sep 23, 2006 at 06:36:21PM CEST, I got a letter\nwhere Petr Baudis said that...\n> Using the #next branch I've now hit a problem with git-fetch-pack\n> master choosing refs/bases/master (I geuss created by StGIT) instead\n> of refs/heads/master. The old upload-pack returned the refs in the order\n> heads-tags-everything_else but the new one just goes for whatever order\n> readdir() returns them in (modulo merging with packed refs). I actually\n> can't see the difference that caused this right now, though.\n\nPortion of this is obsolete, I've since noticed what the difference\nactually is - the _old_ one processed the directory unsorted and the new\none actually keeps it sorted in add_ref().\n\nAlternate approach would be just to modify add_ref() sort order to take\nheads and tags into account (but you still need to keep the cmp hack for\nmerging packed/loose refs). Should be pretty easy to do, but I\npersonally need to proceed with my TODO list for now since I've already\na working workaround.\n\n-- \n\t\t\t\tPetr \"Pasky\" Baudis\nStuff: http://pasky.or.cz/\n#!/bin/perl -sp0777i\n> Using the #next branch I've now hit a problem with git-fetch-pack\n> master choosing refs/bases/master (I geuss created by StGIT) instead\n> of refs/heads/master. The old upload-pack returned the refs in the order\n> heads-tags-everything_else but the new one just goes for whatever order\n> readdir() returns them in (modulo merging with packed refs). I actually\n> can't see the difference that caused this right now, though.\n\nActually, I think it's exactly the other way around.\n\nThe _old_ \"for_each_refs()\" returned things in a totally random order, \ndepending on the readdir(). On many (but not all) filesystems, that ends \nup being somewhat decided by the order the entries were created in, so for \nexample, you'd generally get \"refs/heads/\" and \"refs/tags/\" early \n(discounting HEAD, which is always a special case and comes first).\n\nThe _new_ thing is totally reliable. It returns things sorted \nalphabetically according to \"strcmp\". It doesn't matter what ordering \nreaddir() gives.\n\nNow, we could change the sorting order artificially, but I think your \npatch is actually incorrect. Because you no longer sort the list \nappropriately, the merge-sort done by \"do_show_each_ref()\" is no longer \nguaranteed to work, I think.\n\nUgly.\n\nThe proper way to fix it (if you want to do this at all) is to just define \nthe sort-order to be something else than a plain \"strcmp()\", and change \nthe things that compare ordering to just use the new ordering instead.\n\nIn other words, start from something like THIS, and just change \n\"ref_name_compare()\" to taste. Make sure it's a complete ordering, though.\n\n\t\tLinus\n\n---\ndiff --git a/refs.c b/refs.c\nindex 2cef2b4..05b7006 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -37,6 +37,17 @@ static const char *parse_ref_line(char *\n \treturn line;\n }\n \n+/*\n+ * This is the ordering function for refnames. It has the\n+ * same semantics as \"strcmp()\", but you can define it to\n+ * order \"refs/heads/\" and \"refs/tags/\" before other names,\n+ * for example.\n+ */\n+static int ref_name_compare(const char *a, const char *b)\n+{\n+\treturn strcmp(a,b);\n+}\n+\n static struct ref_list *add_ref(const char *name, const unsigned char *sha1,\n \t\t\t\tint flag, struct ref_list *list)\n {\n@@ -45,7 +56,7 @@ static struct ref_list *add_ref(const ch\n \n \t/* Find the place to insert the ref into.. */\n \twhile ((entry = *p) != NULL) {\n-\t\tint cmp = strcmp(entry->name, name);\n+\t\tint cmp = ref_name_compare(entry->name, name);\n \t\tif (cmp > 0)\n \t\t\tbreak;\n \n@@ -179,7 +190,7 @@ const char *resolve_ref(const char *ref,\n \t\tif (lstat(path, &st) < 0) {\n \t\t\tstruct ref_list *list = get_packed_refs();\n \t\t\twhile (list) {\n-\t\t\t\tif (!strcmp(ref, list->name)) {\n+\t\t\t\tif (!ref_name_compare(ref, list->name)) {\n \t\t\t\t\thashcpy(sha1, list->sha1);\n \t\t\t\t\tif (flag)\n \t\t\t\t\t\t*flag |= REF_ISPACKED;\n@@ -297,7 +308,7 @@ static int do_for_each_ref(const char *b\n \n \twhile (packed && loose) {\n \t\tstruct ref_list *entry;\n-\t\tint cmp = strcmp(packed->name, loose->name);\n+\t\tint cmp = ref_name_compare(packed->name, loose->name);\n \t\tif (!cmp) {\n \t\t\tpacked = packed->next;\n \t\t\tcontinue;\n"},{"id":"27488","messageId":"Pine.LNX.4.63.0609231919510.25853@wbgn013.biozentrum.uni-wuerzburg.de","threadId":"5662","inReplyTo":"20060923164759.GP8259@pasky.or.cz","subject":"Re: [RFC][PATCH] for_each_ref() returning heads in wrong order","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2006-09-23T17:20:42Z","receivedAt":"2006-09-23T17:20:42Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Sat, 23 Sep 2006, Petr Baudis wrote:\n\n> Dear diary, on Sat, Sep 23, 2006 at 06:36:21PM CEST, I got a letter\n> where Petr Baudis said that...\n> > Using the #next branch I've now hit a problem with git-fetch-pack\n> > master choosing refs/bases/master (I geuss created by StGIT) instead\n> > of refs/heads/master. The old upload-pack returned the refs in the order\n> > heads-tags-everything_else but the new one just goes for whatever order\n> > readdir() returns them in (modulo merging with packed refs). I actually\n> > can't see the difference that caused this right now, though.\n\nWhy don't you just specify (or match) \"heads/refs/master\", so that \ngit-fetch-pack cannot choose anything wrong to begin with?\n\nCiao,\nDscho\n"},{"id":"27491","messageId":"20060923174235.GB20017@pasky.or.cz","threadId":"5662","inReplyTo":"Pine.LNX.4.63.0609231919510.25853@wbgn013.biozentrum.uni-wuerzburg.de","subject":"Re: [RFC][PATCH] for_each_ref() returning heads in wrong order","fromName":"Petr Baudis","fromEmail":"pasky@suse.cz","sentAt":"2006-09-23T17:42:35Z","receivedAt":"2006-09-23T17:42:35Z","isPatch":true,"sender":{"key":"pasky@ucw.cz","avatar":"https://avatars.githubusercontent.com/u/18439?v=4"},"body":" Hi,\n\nDear diary, on Sat, Sep 23, 2006 at 07:20:42PM CEST, I got a letter\nwhere Johannes Schindelin said that...\n> On Sat, 23 Sep 2006, Petr Baudis wrote:\n> \n> > Dear diary, on Sat, Sep 23, 2006 at 06:36:21PM CEST, I got a letter\n> > where Petr Baudis said that...\n> > > Using the #next branch I've now hit a problem with git-fetch-pack\n> > > master choosing refs/bases/master (I geuss created by StGIT) instead\n> > > of refs/heads/master. The old upload-pack returned the refs in the order\n> > > heads-tags-everything_else but the new one just goes for whatever order\n> > > readdir() returns them in (modulo merging with packed refs). I actually\n> > > can't see the difference that caused this right now, though.\n> \n> Why don't you just specify (or match) \"heads/refs/master\", so that \n> git-fetch-pack cannot choose anything wrong to begin with?\n\n hmm, does git-fetch prepend refs/heads/ to the refspecs or does it\npass them as is to git-fetch-pack?\n\n If the former, then we could as well do that, but in that case I'd\nrather just require that for any refs passed to git-fetch-pack since\npassing the \"short form\" just serves for buggy scripts to get confused\nwhen someone adds some refs directory that sorts before heads/ to the\nrepository.\n\n-- \n\t\t\t\tPetr \"Pasky\" Baudis\nStuff: http://pasky.or.cz/\n#!/bin/perl -sp0777i writes:\n\n> Using the #next branch I've now hit a problem with git-fetch-pack\n> master choosing refs/bases/master (I geuss created by StGIT) instead\n> of refs/heads/master. The old upload-pack returned the refs in the order\n> heads-tags-everything_else but the new one just goes for whatever order\n> readdir() returns them in (modulo merging with packed refs). I actually\n> can't see the difference that caused this right now, though.\n\nI think it is the other way around (the new one sorts, the old\none doesn't). fetch-pack lets for_each_ref() to pick, but the\nwrapper git-fetch I think prepends refs/ and refs/heads as\nneeded, so if you explicitly say heads/master I do not think you\nhave a problem.\n\nHowever, I see a bit bigger problem here. I think rev-parse\nwould not complain \"master\" is ambiguous in your repository,\nbecause it has a fixed list of prefixes (\"\", refs, refs/tags,\nrefs/heads, refs/remotes, refs/remotes/%/HEAD) it uses to DWIM\nin sha1_file.c::get_sha1_basic(), and refs/bases is not part of\nthe prefixes.\n\nI suspect we should fix connect.c::count_refspec_match(), which\ncurrently is just a strict tail match, to use the same list of\nprefix.\n"}]}