{"thread":{"id":"18371","subject":"t5505-remote fails on Windows","startedAt":"2009-03-18T11:42:27Z","lastAt":"2009-03-23T08:09:25Z","messageCount":18,"participants":["Johannes Sixt","Jeff King","Jay Soffian","Johannes Schindelin","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"108368","messageId":"49C0DE23.8020809@viscovery.net","threadId":"18371","inReplyTo":null,"subject":"t5505-remote fails on Windows","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2009-03-18T11:42:27Z","receivedAt":"2009-03-18T11:42:27Z","isPatch":false,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"I see this failure related to 'git remote show':\n\n* expecting success:\n (cd test &&\n git config --add remote.origin.fetch\nrefs/heads/master:refs/heads/upstream &&\n git fetch &&\n git checkout -b ahead origin/master &&\n echo 1 >> file &&\n test_tick &&\n git commit -m update file &&\n git checkout master &&\n git branch --track octopus origin/master &&\n git branch --track rebase origin/master &&\n git branch -d -r origin/master &&\n git config --add remote.two.url ../two &&\n git config branch.rebase.rebase true &&\n git config branch.octopus.merge \"topic-a topic-b topic-c\" &&\n (cd ../one &&\n echo 1 > file &&\n test_tick &&\n git commit -m update file) &&\n git config --add remote.origin.push : &&\n git config --add remote.origin.push\nrefs/heads/master:refs/heads/upstream &&\n git config --add remote.origin.push +refs/tags/lastbackup &&\n git config --add remote.two.push\n+refs/heads/ahead:refs/heads/master &&\n\n git config --add remote.two.push\nrefs/heads/master:refs/heads/another &&\n git remote show origin two > output &&\n git branch -d rebase octopus &&\n test_cmp expect output)\n\n>From d:/Src/mingw-git/t/trash directory.t5505-remote/one\n * [new branch] master -> upstream\nBranch ahead set up to track remote branch refs/remotes/origin/master.\nSwitched to a new branch \"ahead\"\n[ahead 847549e] update\n 1 files changed, 1 insertions(+), 0 deletions(-)\nSwitched to branch \"master\"\nBranch octopus set up to track remote branch refs/remotes/origin/master.\nBranch rebase set up to track remote branch refs/remotes/origin/master.\nDeleted remote branch origin/master (9d34b14).\n[master 6329a3c] update\n 1 files changed, 1 insertions(+), 0 deletions(-)\nerror: src refspec refs/tags/lastbackup does not match any.\nDeleted branch rebase (9d34b14).\nDeleted branch octopus (9d34b14).\n--- expect Wed Mar 18 11:22:53 2009\n+++ output Wed Mar 18 11:22:54 2009\n@@ -12,8 +12,8 @@\n and with remote topic-c\n rebase rebases onto remote master\n Local refs configured for 'git push':\n- master pushes to master (local out of date)\n master pushes to upstream (create)\n+ master pushes to master (local out of date)\n * remote two\n URL: ../two\n HEAD branch (remote HEAD is ambiguous, may be one of the following):\n* FAIL 8: show\n\n\nAs you can see, the entries for \"master pushes to...\" are reversed. It\nseems that this output is not stable. Before I delve into this, do you\nknow whether there is some data structure involved that does not guarantee\nthe order? Such as a hash table, a opendir/readdir sequence, or perhaps\nwhile reading the config file?\n\nIt looks like we have to explicitly sort the list somewhere.\n\n-- Hannes\n"},{"id":"108469","messageId":"20090319041837.GA32642@coredump.intra.peff.net","threadId":"18371","inReplyTo":"49C0DE23.8020809@viscovery.net","subject":"Re: t5505-remote fails on Windows","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-03-19T04:18:37Z","receivedAt":"2009-03-19T04:18:37Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Mar 18, 2009 at 12:42:27PM +0100, Johannes Sixt wrote:\n\n> --- expect Wed Mar 18 11:22:53 2009\n> +++ output Wed Mar 18 11:22:54 2009\n> @@ -12,8 +12,8 @@\n> and with remote topic-c\n> rebase rebases onto remote master\n> Local refs configured for 'git push':\n> - master pushes to master (local out of date)\n> master pushes to upstream (create)\n> + master pushes to master (local out of date)\n> * remote two\n> URL: ../two\n> HEAD branch (remote HEAD is ambiguous, may be one of the following):\n> * FAIL 8: show\n> \n> \n> As you can see, the entries for \"master pushes to...\" are reversed. It\n> seems that this output is not stable. Before I delve into this, do you\n> know whether there is some data structure involved that does not guarantee\n> the order? Such as a hash table, a opendir/readdir sequence, or perhaps\n> while reading the config file?\n\nThat is quite curious, because it is sorted immediately before printing:\n\n $ sed -n 1034,1040p builtin-remote.c\n for_each_string_list(add_push_to_show_info, &states.push, &info);\n sort_string_list(info.list);\n if (info.list->nr)\n printf(\" Local ref%s configured for 'git push'%s:\\n\",\n info.list->nr > 1 ? \"s\" : \"\",\n no_query ? \" (status not queried)\" : \"\");\n for_each_string_list(show_push_info_item, info.list, &info);\n\ncan you step through in a debugger and make sure the sort_string_list is\nactually sorting?\n\n-Peff\n"},{"id":"108470","messageId":"20090319044313.GA341@coredump.intra.peff.net","threadId":"18371","inReplyTo":"20090319041837.GA32642@coredump.intra.peff.net","subject":"Re: t5505-remote fails on Windows","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-03-19T04:43:13Z","receivedAt":"2009-03-19T04:43:13Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Mar 19, 2009 at 12:18:37AM -0400, Jeff King wrote:\n\n> > As you can see, the entries for \"master pushes to...\" are reversed. It\n> > seems that this output is not stable. Before I delve into this, do you\n> > know whether there is some data structure involved that does not guarantee\n> > the order? Such as a hash table, a opendir/readdir sequence, or perhaps\n> > while reading the config file?\n> \n> That is quite curious, because it is sorted immediately before printing:\n> \n> $ sed -n 1034,1040p builtin-remote.c\n> for_each_string_list(add_push_to_show_info, &states.push, &info);\n> sort_string_list(info.list);\n\nOh, nevermind. I didn't look closely enough at your issue, which is one\nref pushing to two different places. For that, the sort needs to take\ninto account the util field, which holds the destination.\n\nThe patch below probably fixes it, but as I can't actually reproduce\nhere, it is largely untested.\n\nAs a side note, I find this solution a little bit ugly. String lists\nshould sort on their strings, not on some other random magic in the util\nfield. This usage really abuses string_list a bit as a data type because\nwe have no generic \"list\" type.\n\n---\ndiff --git a/builtin-remote.c b/builtin-remote.c\nindex 993acd6..f94ecd6 100644\n--- a/builtin-remote.c\n+++ b/builtin-remote.c\n@@ -922,6 +922,16 @@ int add_push_to_show_info(struct string_list_item *push_item, void *cb_data)\n \treturn 0;\n }\n \n+static int push_cmp(const void *va, const void *vb)\n+{\n+\tconst struct string_list_item *a = va;\n+\tconst struct string_list_item *b = vb;\n+\tconst struct push_info *a_push = a->util;\n+\tconst struct push_info *b_push = b->util;\n+\tint cmp = strcmp(a->string, b->string);\n+\treturn cmp ? cmp : strcmp(a_push->dest, b_push->dest);\n+}\n+\n int show_push_info_item(struct string_list_item *item, void *cb_data)\n {\n \tstruct show_info *show_info = cb_data;\n@@ -1032,7 +1042,7 @@ static int show(int argc, const char **argv)\n \n \t\tinfo.width = info.width2 = 0;\n \t\tfor_each_string_list(add_push_to_show_info, &states.push, &info);\n-\t\tsort_string_list(info.list);\n+\t\tsort_string_list_with_fn(info.list, push_cmp);\n \t\tif (info.list->nr)\n \t\t\tprintf(\" Local ref%s configured for 'git push'%s:\\n\",\n \t\t\t\tinfo.list->nr > 1 ? \"s\" : \"\",\ndiff --git a/string-list.c b/string-list.c\nindex 1ac536e..f404a26 100644\n--- a/string-list.c\n+++ b/string-list.c\n@@ -163,9 +163,15 @@ static int cmp_items(const void *a, const void *b)\n \treturn strcmp(one->string, two->string);\n }\n \n+void sort_string_list_with_fn(struct string_list *list,\n+\t\t\t int (*fn)(const void *, const void *))\n+{\n+\tqsort(list->items, list->nr, sizeof(*list->items), fn);\n+}\n+\n void sort_string_list(struct string_list *list)\n {\n-\tqsort(list->items, list->nr, sizeof(*list->items), cmp_items);\n+\tsort_string_list_with_fn(list, cmp_items);\n }\n \n int unsorted_string_list_has_string(struct string_list *list, const char *string)\ndiff --git a/string-list.h b/string-list.h\nindex 14bbc47..4bbc7e8 100644\n--- a/string-list.h\n+++ b/string-list.h\n@@ -37,6 +37,8 @@ struct string_list_item *string_list_lookup(const char *string, struct string_li\n /* Use these functions only on unsorted lists: */\n struct string_list_item *string_list_append(const char *string, struct string_list *list);\n void sort_string_list(struct string_list *list);\n+void sort_string_list_with_fn(struct string_list *list,\n+\t\t\t int (*fn)(const void *, const void *));\n int unsorted_string_list_has_string(struct string_list *list, const char *string);\n \n #endif /* PATH_LIST_H */\n"},{"id":"108471","messageId":"76718490903182156y3b2b9d8aw708829a6ed151aa@mail.gmail.com","threadId":"18371","inReplyTo":"20090319044313.GA341@coredump.intra.peff.net","subject":"Re: t5505-remote fails on Windows","fromName":"Jay Soffian","fromEmail":"jaysoffian@gmail.com","sentAt":"2009-03-19T04:56:37Z","receivedAt":"2009-03-19T04:56:37Z","isPatch":false,"sender":{"key":"jaysoffian@gmail.com","avatar":"https://avatars.githubusercontent.com/u/155970?v=4"},"body":"On Thu, Mar 19, 2009 at 12:43 AM, Jeff King wrote:\n> The patch below probably fixes it, but as I can't actually reproduce\n> here, it is largely untested.\n\nThanks.\n\n> As a side note, I find this solution a little bit ugly. String lists\n> should sort on their strings, not on some other random magic in the util\n> field. This usage really abuses string_list a bit as a data type because\n> we have no generic \"list\" type.\n\nI really don't think so. The string_list API accommodates this case\nquite nicely. So why not?\n\n(And anyway, what sane configuration pushes a single local branch to\nmultiple remote branches? But the refspecs allow it, so I accommodated\nit in builtin-remote and tested for it. I never actually tested\nwhether git push works with it tho.) :-)\n\nj.\n"},{"id":"108472","messageId":"20090319050328.GA9746@coredump.intra.peff.net","threadId":"18371","inReplyTo":"76718490903182156y3b2b9d8aw708829a6ed151aa@mail.gmail.com","subject":"Re: t5505-remote fails on Windows","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-03-19T05:03:28Z","receivedAt":"2009-03-19T05:03:28Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Mar 19, 2009 at 12:56:37AM -0400, Jay Soffian wrote:\n\n> > As a side note, I find this solution a little bit ugly. String lists\n> > should sort on their strings, not on some other random magic in the util\n> > field. This usage really abuses string_list a bit as a data type because\n> > we have no generic \"list\" type.\n> \n> I really don't think so. The string_list API accommodates this case\n> quite nicely. So why not?\n\nI think the code would be more natural as a list of structs, each with a\nsource and dest. But C does not make it pleasant to write generic data\ntypes, so things end up stuffed into string_lists with a magic util\nfield. So I think leaving it as a string_list is probably the most sane\nthing to do.\n\n-Peff\n"},{"id":"108481","messageId":"49C1F232.2090009@viscovery.net","threadId":"18371","inReplyTo":"20090319044313.GA341@coredump.intra.peff.net","subject":"Re: t5505-remote fails on Windows","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2009-03-19T07:20:18Z","receivedAt":"2009-03-19T07:20:18Z","isPatch":false,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Jeff King schrieb:\n> I didn't look closely enough at your issue, which is one\n> ref pushing to two different places. For that, the sort needs to take\n> into account the util field, which holds the destination.\n> \n> The patch below probably fixes it, but as I can't actually reproduce\n> here, it is largely untested.\n\nYour patch fixes it. The following change to the test case would be a good\naddition; it protects later tests from failures in earlier tests.\n\n-- Hannes\n\ndiff --git a/t/t5505-remote.sh b/t/t5505-remote.sh\nindex 5ec668d..13f32b8 100755\n--- a/t/t5505-remote.sh\n+++ b/t/t5505-remote.sh\n@@ -160,7 +160,7 @@ cat > test/expect << EOF\n master pushes to another (up to date)\n EOF\n\n-test_expect_success 'show' '\n+test_expect_success 'more setup for show' '\n \t(cd test &&\n \t git config --add remote.origin.fetch refs/heads/master:refs/heads/upstream &&\n \t git fetch &&\n@@ -183,12 +183,20 @@ test_expect_success 'show' '\n \t git config --add remote.origin.push refs/heads/master:refs/heads/upstream &&\n \t git config --add remote.origin.push +refs/tags/lastbackup &&\n \t git config --add remote.two.push +refs/heads/ahead:refs/heads/master &&\n-\t git config --add remote.two.push refs/heads/master:refs/heads/another &&\n+\t git config --add remote.two.push refs/heads/master:refs/heads/another)\n+'\n+\n+test_expect_success 'show' '\n+\t(cd test &&\n \t git remote show origin two > output &&\n-\t git branch -d rebase octopus &&\n \t test_cmp expect output)\n '\n\n+test_expect_success 'more setup for show -n' '\n+\t(cd test &&\n+\t git branch -d rebase octopus)\n+'\n+\n cat > test/expect << EOF\n * remote origin\n URL: $(pwd)/one\n"},{"id":"108507","messageId":"alpine.DEB.1.00.0903191135530.10279@pacific.mpi-cbg.de","threadId":"18371","inReplyTo":"20090319044313.GA341@coredump.intra.peff.net","subject":"Re: t5505-remote fails on Windows","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2009-03-19T10:36:50Z","receivedAt":"2009-03-19T10:36:50Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Thu, 19 Mar 2009, Jeff King wrote:\n\n> @@ -163,9 +163,15 @@ static int cmp_items(const void *a, const void *b)\n> \treturn strcmp(one->string, two->string);\n> }\n> \n> +void sort_string_list_with_fn(struct string_list *list,\n> +\t\t\t int (*fn)(const void *, const void *))\n> +{\n> +\tqsort(list->items, list->nr, sizeof(*list->items), fn);\n> +}\n> +\n\nDo we really want an API for that? Calling qsort() directly should be \nobvious enough, no?\n\nCiao,\nDscho\n"},{"id":"108514","messageId":"7vhc1pai84.fsf@gitster.siamese.dyndns.org","threadId":"18371","inReplyTo":"alpine.DEB.1.00.0903191135530.10279@pacific.mpi-cbg.de","subject":"Re: t5505-remote fails on Windows","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-03-19T11:02:19Z","receivedAt":"2009-03-19T11:02:19Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> On Thu, 19 Mar 2009, Jeff King wrote:\n>\n>> @@ -163,9 +163,15 @@ static int cmp_items(const void *a, const void *b)\n>> \treturn strcmp(one->string, two->string);\n>> }\n>> \n>> +void sort_string_list_with_fn(struct string_list *list,\n>> +\t\t\t int (*fn)(const void *, const void *))\n>> +{\n>> +\tqsort(list->items, list->nr, sizeof(*list->items), fn);\n>> +}\n>> +\n>\n> Do we really want an API for that? Calling qsort() directly should be \n> obvious enough, no?\n\nI think so. If it were done like this (notice the lack of double\nindirection in the cmp_fn signature):\n\n typedef int string_list_item_cmp_fn(const struct string_list_item *, const struct string_list_item *);\n\n void sort_string_list_with_fn(struct string_list *list, string_list_item_cmp_fn *);\n\nit would have made more sense, though.\n"},{"id":"108531","messageId":"76718490903190800o4d82a037mb6fc5bf901c418d8@mail.gmail.com","threadId":"18371","inReplyTo":"7vhc1pai84.fsf@gitster.siamese.dyndns.org","subject":"Re: t5505-remote fails on Windows","fromName":"Jay Soffian","fromEmail":"jaysoffian@gmail.com","sentAt":"2009-03-19T15:00:50Z","receivedAt":"2009-03-19T15:00:50Z","isPatch":false,"sender":{"key":"jaysoffian@gmail.com","avatar":"https://avatars.githubusercontent.com/u/155970?v=4"},"body":"On Thu, Mar 19, 2009 at 7:02 AM, Junio C Hamano wrote:\n>> Do we really want an API for that?  Calling qsort() directly should be\n>> obvious enough, no?\n>\n> I think so.  If it were done like this (notice the lack of double\n> indirection in the cmp_fn signature):\n>\n>    typedef int string_list_item_cmp_fn(const struct string_list_item *, const struct string_list_item *);\n>\n>    void sort_string_list_with_fn(struct string_list *list, string_list_item_cmp_fn *);\n>\n> it would have made more sense, though.\n\nOh, wow, sorry, I didn't even realize Jeff had just added that\nfunction. Somehow I missed that part of his patch.\n\nj.\n"},{"id":"108570","messageId":"20090319195254.GA17028@coredump.intra.peff.net","threadId":"18371","inReplyTo":"alpine.DEB.1.00.0903191135530.10279@pacific.mpi-cbg.de","subject":"Re: t5505-remote fails on Windows","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-03-19T19:52:54Z","receivedAt":"2009-03-19T19:52:54Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Mar 19, 2009 at 11:36:50AM +0100, Johannes Schindelin wrote:\n\n> > +void sort_string_list_with_fn(struct string_list *list,\n> > +\t\t\t int (*fn)(const void *, const void *))\n> > +{\n> > +\tqsort(list->items, list->nr, sizeof(*list->items), fn);\n> > +}\n> > +\n> \n> Do we really want an API for that? Calling qsort() directly should be \n> obvious enough, no?\n\nI'm fine with that, too. I was just encapsulating out of habit. But if\nit is normal to peek directly at list->nr and list->items, then calling\nqsort directly is fine.\n\n-Peff\n"},{"id":"108574","messageId":"20090319200308.GB17028@coredump.intra.peff.net","threadId":"18371","inReplyTo":"7vhc1pai84.fsf@gitster.siamese.dyndns.org","subject":"Re: t5505-remote fails on Windows","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-03-19T20:03:08Z","receivedAt":"2009-03-19T20:03:08Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Mar 19, 2009 at 04:02:19AM -0700, Junio C Hamano wrote:\n\n> > Do we really want an API for that? Calling qsort() directly should be \n> > obvious enough, no?\n> \n> I think so. If it were done like this (notice the lack of double\n> indirection in the cmp_fn signature):\n> \n> typedef int string_list_item_cmp_fn(const struct string_list_item *, const struct string_list_item *);\n> \n> void sort_string_list_with_fn(struct string_list *list, string_list_item_cmp_fn *);\n> \n> it would have made more sense, though.\n\nIIRC, that is actually not valid C according to the standard (that is,\neven though a void* can be implicitly assigned to any other pointer, a\nfunction taking a void* and a function taking another pointer do not\nnecessarily have the same function signature or calling conventions).\nWhich is why cmp_items in string-list.c already does the indirection.\n\nThat being said, I think this is probably one of those cases where it\nworks fine on any sane platform that we care about and we can choose to\nignore the standard.\n\nActually, I think an even nicer API would be to give a function that\n_just_ compares the util fields; string_list_sort() would sort on the\n\"string\" fields, and only call the util cmp to sort entries with equal\nstrings. Sadly, because qsort lacks a slot for extra data to pass to the\ncallback, implementing this ends up with a global variable:\n\n static int (*cmp_util)(const void *, const void *);\n\n int cmp_items(const void *a, const void *b)\n {\n const struct string_list_item *one = a;\n const struct string_list_item *two = b;\n int cmp = strcmp(one->string, two->string);\n return cmp ? cmp : cmp_util(one->util, b->util);\n }\n\n int sort_string_list_with_util(struct string_list *list,\n int (*fn)(const void *, const void *))\n {\n cmp_util = fn;\n qsort(list->items, list->nr, sizeof(*list->items), cmp_items);\n }\n\nIf only C had closures. ;)\n\nBut this is really getting beyond the issue at hand. I can submit a\nfinalized patch; just let me know how you prefer it.\n\n-Peff\n"},{"id":"108575","messageId":"20090319200428.GC17028@coredump.intra.peff.net","threadId":"18371","inReplyTo":"49C1F232.2090009@viscovery.net","subject":"Re: t5505-remote fails on Windows","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-03-19T20:04:28Z","receivedAt":"2009-03-19T20:04:28Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Mar 19, 2009 at 08:20:18AM +0100, Johannes Sixt wrote:\n\n> Your patch fixes it. The following change to the test case would be a good\n> addition; it protects later tests from failures in earlier tests.\n\nI'll clean up and re-submit my patch, then (once we figure out the right\nAPI ;) ). Your patch seems reasonable, but it should be a separate\npatch, I think.\n\n-Peff\n"},{"id":"108613","messageId":"alpine.DEB.1.00.0903200014370.10279@pacific.mpi-cbg.de","threadId":"18371","inReplyTo":"20090319200308.GB17028@coredump.intra.peff.net","subject":"Re: t5505-remote fails on Windows","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2009-03-19T23:15:50Z","receivedAt":"2009-03-19T23:15:50Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Thu, 19 Mar 2009, Jeff King wrote:\n\n> On Thu, Mar 19, 2009 at 04:02:19AM -0700, Junio C Hamano wrote:\n> \n> > > Do we really want an API for that? Calling qsort() directly should be \n> > > obvious enough, no?\n> > \n> > I think so. If it were done like this (notice the lack of double\n> > indirection in the cmp_fn signature):\n> > \n> > typedef int string_list_item_cmp_fn(const struct string_list_item *, const struct string_list_item *);\n> > \n> > void sort_string_list_with_fn(struct string_list *list, string_list_item_cmp_fn *);\n> > \n> > it would have made more sense, though.\n> \n> IIRC, that is actually not valid C according to the standard (that is, \n> even though a void* can be implicitly assigned to any other pointer, a \n> function taking a void* and a function taking another pointer do not \n> necessarily have the same function signature or calling conventions). \n> Which is why cmp_items in string-list.c already does the indirection.\n\nAFAICT the idea was not to pass the function to qsort() directly, but I \nhave to agree that I do not see how that should be possible with the \ncurrent interface of qsort();\n\nCiao,\nDscho\n"},{"id":"108887","messageId":"20090322085920.GA5201@coredump.intra.peff.net","threadId":"18371","inReplyTo":"20090319200308.GB17028@coredump.intra.peff.net","subject":"[PATCH] remote: improve sorting of \"configure for git push\" list","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-03-22T08:59:20Z","receivedAt":"2009-03-22T08:59:20Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The data structure used to store this list is a string_list\nof sources with the destination in the util member. The\ncurrent code just sorts on the source; if a single source is\npushed to two different destination refs at a remote, then\nthe order in which they are printed is non-deterministic.\n\nThis patch implements a comparison using both fields.\nBesides being a little nicer on the eyes, giving a stable\nsort prevents false negatives in the test suite when\ncomparing output.\n\nSigned-off-by: Jeff King \n---\nThe discussion of the proper interface to the one-line wrapper to qsort\ndidn't really resolve anything. It seems that the constraints of C make\na nice wrapper a little painful, so let's just use a bare qsort. Once\nagain, I really don't care what it looks like, so feel free to mark it\nup if you prefer differently; I just want it off my todo list, and to\nlet JSixt run his tests in peace.\n\nI did have one somewhat sick thought for an API: have string_list's\ncmp_items (or an alternate cmp_items_with_util) treat a non-NULL util\nmember as a pointer to a string, and use it as a secondary key in the\nsort. It works here because the first member of the push_info struct is\nthe secondary key. But I think it is a bit too subtle for my taste.\n\n builtin-remote.c | 17 ++++++++++++++++-\n 1 files changed, 16 insertions(+), 1 deletions(-)\n\ndiff --git a/builtin-remote.c b/builtin-remote.c\nindex 993acd6..9ef846f 100644\n--- a/builtin-remote.c\n+++ b/builtin-remote.c\n@@ -922,6 +922,20 @@ int add_push_to_show_info(struct string_list_item *push_item, void *cb_data)\n \treturn 0;\n }\n \n+/*\n+ * Sorting comparison for a string list that has push_info\n+ * structs in its util field\n+ */\n+static int cmp_string_with_push(const void *va, const void *vb)\n+{\n+\tconst struct string_list_item *a = va;\n+\tconst struct string_list_item *b = vb;\n+\tconst struct push_info *a_push = a->util;\n+\tconst struct push_info *b_push = b->util;\n+\tint cmp = strcmp(a->string, b->string);\n+\treturn cmp ? cmp : strcmp(a_push->dest, b_push->dest);\n+}\n+\n int show_push_info_item(struct string_list_item *item, void *cb_data)\n {\n \tstruct show_info *show_info = cb_data;\n@@ -1032,7 +1046,8 @@ static int show(int argc, const char **argv)\n \n \t\tinfo.width = info.width2 = 0;\n \t\tfor_each_string_list(add_push_to_show_info, &states.push, &info);\n-\t\tsort_string_list(info.list);\n+\t\tqsort(info.list->items, info.list->nr,\n+\t\t\tsizeof(*info.list->items), cmp_string_with_push);\n \t\tif (info.list->nr)\n \t\t\tprintf(\" Local ref%s configured for 'git push'%s:\\n\",\n \t\t\t\tinfo.list->nr > 1 ? \"s\" : \"\",\n-- \n1.6.2.1.276.gd47fa\n"},{"id":"108899","messageId":"76718490903220747p74cf3704vbbbc085c28a105b2@mail.gmail.com","threadId":"18371","inReplyTo":"20090322085920.GA5201@coredump.intra.peff.net","subject":"Re: [PATCH] remote: improve sorting of \"configure for git push\" list","fromName":"Jay Soffian","fromEmail":"jaysoffian@gmail.com","sentAt":"2009-03-22T14:47:07Z","receivedAt":"2009-03-22T14:47:07Z","isPatch":true,"sender":{"key":"jaysoffian@gmail.com","avatar":"https://avatars.githubusercontent.com/u/155970?v=4"},"body":"On Sun, Mar 22, 2009 at 4:59 AM, Jeff King wrote:\n> The data structure used to store this list is a string_list\n> of sources with the destination in the util member. The\n> current code just sorts on the source; if a single source is\n> pushed to two different destination refs at a remote, then\n> the order in which they are printed is non-deterministic.\n>\n> This patch implements a comparison using both fields.\n> Besides being a little nicer on the eyes, giving a stable\n> sort prevents false negatives in the test suite when\n> comparing output.\n>\n> Signed-off-by: Jeff King \n> ---\n> The discussion of the proper interface to the one-line wrapper to qsort\n> didn't really resolve anything. It seems that the constraints of C make\n> a nice wrapper a little painful, so let's just use a bare qsort. Once\n> again, I really don't care what it looks like, so feel free to mark it\n> up if you prefer differently; I just want it off my todo list, and to\n> let JSixt run his tests in peace.\n>\n> I did have one somewhat sick thought for an API: have string_list's\n> cmp_items (or an alternate cmp_items_with_util) treat a non-NULL util\n> member as a pointer to a string, and use it as a secondary key in the\n> sort. It works here because the first member of the push_info struct is\n> the secondary key.  But I think it is a bit too subtle for my taste.\n\nAck. There is one thing I wonder though. Is this even a sane config?\nIt's accepted, but is that intentional or just an accident? Would\nanyone notice if the push parsing were changed to only accept a single\ndest for a given source branch?\n\nThanks for the patch Jeff.\n\nj.\n"},{"id":"108933","messageId":"7vk56hdxtp.fsf@gitster.siamese.dyndns.org","threadId":"18371","inReplyTo":"76718490903220747p74cf3704vbbbc085c28a105b2@mail.gmail.com","subject":"Re: [PATCH] remote: improve sorting of \"configure for git push\" list","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-03-22T21:58:26Z","receivedAt":"2009-03-22T21:58:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jay Soffian writes:\n\n> On Sun, Mar 22, 2009 at 4:59 AM, Jeff King wrote:\n>> The data structure used to store this list is a string_list\n>> of sources with the destination in the util member. The\n>> current code just sorts on the source; if a single source is\n>> pushed to two different destination refs at a remote, then\n>> the order in which they are printed is non-deterministic.\n>> ...\n> Ack. There is one thing I wonder though. Is this even a sane config?\n\nI do not think of a workflow that this feature is absolutely necessary in\nthe sense that it cannot be worked around if you removed it, but that is\nnot what you asked.\n\nI wouldn't be surprised if there are some kernel subsystem maintainers who\nusually push to their master but when they push their finished goods to\ntheir for-linus branch make sure they update their master at the same time.\nConfiguring their local for-linus to push to master and for-linus may be\none way to do so.\n\nIn a one-branch project with two people, it is not entirely inconceivable\nto arrange for me to push to master and junio while you to push to master\nand jay, in order to keep track.\n"},{"id":"109007","messageId":"49C74099.3090806@viscovery.net","threadId":"18371","inReplyTo":"20090322085920.GA5201@coredump.intra.peff.net","subject":"Re: [PATCH] remote: improve sorting of \"configure for git push\" list","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2009-03-23T07:56:09Z","receivedAt":"2009-03-23T07:56:09Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Tested-by: Johannes Sixt \n\nThanks,\n-- Hannes\n"},{"id":"109017","messageId":"7vab7c8xu2.fsf@gitster.siamese.dyndns.org","threadId":"18371","inReplyTo":"49C74099.3090806@viscovery.net","subject":"Re: [PATCH] remote: improve sorting of \"configure for git push\" list","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-03-23T08:09:25Z","receivedAt":"2009-03-23T08:09:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thanks.\n"}]}