* t5505-remote fails on Windows
@ 2009-03-18 11:42 Johannes Sixt
2009-03-19 4:18 ` Jeff King
0 siblings, 1 reply; 18+ messages in thread
From: Johannes Sixt @ 2009-03-18 11:42 UTC (permalink / raw)
To: Jay Soffian; +Cc: Git Mailing List
I see this failure related to 'git remote show':
* expecting success:
(cd test &&
git config --add remote.origin.fetch
refs/heads/master:refs/heads/upstream &&
git fetch &&
git checkout -b ahead origin/master &&
echo 1 >> file &&
test_tick &&
git commit -m update file &&
git checkout master &&
git branch --track octopus origin/master &&
git branch --track rebase origin/master &&
git branch -d -r origin/master &&
git config --add remote.two.url ../two &&
git config branch.rebase.rebase true &&
git config branch.octopus.merge "topic-a topic-b topic-c" &&
(cd ../one &&
echo 1 > file &&
test_tick &&
git commit -m update file) &&
git config --add remote.origin.push : &&
git config --add remote.origin.push
refs/heads/master:refs/heads/upstream &&
git config --add remote.origin.push +refs/tags/lastbackup &&
git config --add remote.two.push
+refs/heads/ahead:refs/heads/master &&
git config --add remote.two.push
refs/heads/master:refs/heads/another &&
git remote show origin two > output &&
git branch -d rebase octopus &&
test_cmp expect output)
>From d:/Src/mingw-git/t/trash directory.t5505-remote/one
* [new branch] master -> upstream
Branch ahead set up to track remote branch refs/remotes/origin/master.
Switched to a new branch "ahead"
[ahead 847549e] update
1 files changed, 1 insertions(+), 0 deletions(-)
Switched to branch "master"
Branch octopus set up to track remote branch refs/remotes/origin/master.
Branch rebase set up to track remote branch refs/remotes/origin/master.
Deleted remote branch origin/master (9d34b14).
[master 6329a3c] update
1 files changed, 1 insertions(+), 0 deletions(-)
error: src refspec refs/tags/lastbackup does not match any.
Deleted branch rebase (9d34b14).
Deleted branch octopus (9d34b14).
--- expect Wed Mar 18 11:22:53 2009
+++ output Wed Mar 18 11:22:54 2009
@@ -12,8 +12,8 @@
and with remote topic-c
rebase rebases onto remote master
Local refs configured for 'git push':
- master pushes to master (local out of date)
master pushes to upstream (create)
+ master pushes to master (local out of date)
* remote two
URL: ../two
HEAD branch (remote HEAD is ambiguous, may be one of the following):
* FAIL 8: show
As you can see, the entries for "master pushes to..." are reversed. It
seems that this output is not stable. Before I delve into this, do you
know whether there is some data structure involved that does not guarantee
the order? Such as a hash table, a opendir/readdir sequence, or perhaps
while reading the config file?
It looks like we have to explicitly sort the list somewhere.
-- Hannes
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: t5505-remote fails on Windows 2009-03-18 11:42 t5505-remote fails on Windows Johannes Sixt @ 2009-03-19 4:18 ` Jeff King 2009-03-19 4:43 ` Jeff King 0 siblings, 1 reply; 18+ messages in thread From: Jeff King @ 2009-03-19 4:18 UTC (permalink / raw) To: Johannes Sixt; +Cc: Jay Soffian, Git Mailing List On Wed, Mar 18, 2009 at 12:42:27PM +0100, Johannes Sixt wrote: > --- expect Wed Mar 18 11:22:53 2009 > +++ output Wed Mar 18 11:22:54 2009 > @@ -12,8 +12,8 @@ > and with remote topic-c > rebase rebases onto remote master > Local refs configured for 'git push': > - master pushes to master (local out of date) > master pushes to upstream (create) > + master pushes to master (local out of date) > * remote two > URL: ../two > HEAD branch (remote HEAD is ambiguous, may be one of the following): > * FAIL 8: show > > > As you can see, the entries for "master pushes to..." are reversed. It > seems that this output is not stable. Before I delve into this, do you > know whether there is some data structure involved that does not guarantee > the order? Such as a hash table, a opendir/readdir sequence, or perhaps > while reading the config file? That is quite curious, because it is sorted immediately before printing: $ sed -n 1034,1040p builtin-remote.c for_each_string_list(add_push_to_show_info, &states.push, &info); sort_string_list(info.list); if (info.list->nr) printf(" Local ref%s configured for 'git push'%s:\n", info.list->nr > 1 ? "s" : "", no_query ? " (status not queried)" : ""); for_each_string_list(show_push_info_item, info.list, &info); can you step through in a debugger and make sure the sort_string_list is actually sorting? -Peff ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: t5505-remote fails on Windows 2009-03-19 4:18 ` Jeff King @ 2009-03-19 4:43 ` Jeff King 2009-03-19 4:56 ` Jay Soffian ` (2 more replies) 0 siblings, 3 replies; 18+ messages in thread From: Jeff King @ 2009-03-19 4:43 UTC (permalink / raw) To: Johannes Sixt; +Cc: Jay Soffian, Git Mailing List On Thu, Mar 19, 2009 at 12:18:37AM -0400, Jeff King wrote: > > As you can see, the entries for "master pushes to..." are reversed. It > > seems that this output is not stable. Before I delve into this, do you > > know whether there is some data structure involved that does not guarantee > > the order? Such as a hash table, a opendir/readdir sequence, or perhaps > > while reading the config file? > > That is quite curious, because it is sorted immediately before printing: > > $ sed -n 1034,1040p builtin-remote.c > for_each_string_list(add_push_to_show_info, &states.push, &info); > sort_string_list(info.list); Oh, nevermind. I didn't look closely enough at your issue, which is one ref pushing to two different places. For that, the sort needs to take into account the util field, which holds the destination. The patch below probably fixes it, but as I can't actually reproduce here, it is largely untested. As a side note, I find this solution a little bit ugly. String lists should sort on their strings, not on some other random magic in the util field. This usage really abuses string_list a bit as a data type because we have no generic "list" type. --- diff --git a/builtin-remote.c b/builtin-remote.c index 993acd6..f94ecd6 100644 --- a/builtin-remote.c +++ b/builtin-remote.c @@ -922,6 +922,16 @@ int add_push_to_show_info(struct string_list_item *push_item, void *cb_data) return 0; } +static int push_cmp(const void *va, const void *vb) +{ + const struct string_list_item *a = va; + const struct string_list_item *b = vb; + const struct push_info *a_push = a->util; + const struct push_info *b_push = b->util; + int cmp = strcmp(a->string, b->string); + return cmp ? cmp : strcmp(a_push->dest, b_push->dest); +} + int show_push_info_item(struct string_list_item *item, void *cb_data) { struct show_info *show_info = cb_data; @@ -1032,7 +1042,7 @@ static int show(int argc, const char **argv) info.width = info.width2 = 0; for_each_string_list(add_push_to_show_info, &states.push, &info); - sort_string_list(info.list); + sort_string_list_with_fn(info.list, push_cmp); if (info.list->nr) printf(" Local ref%s configured for 'git push'%s:\n", info.list->nr > 1 ? "s" : "", diff --git a/string-list.c b/string-list.c index 1ac536e..f404a26 100644 --- a/string-list.c +++ b/string-list.c @@ -163,9 +163,15 @@ static int cmp_items(const void *a, const void *b) return strcmp(one->string, two->string); } +void sort_string_list_with_fn(struct string_list *list, + int (*fn)(const void *, const void *)) +{ + qsort(list->items, list->nr, sizeof(*list->items), fn); +} + void sort_string_list(struct string_list *list) { - qsort(list->items, list->nr, sizeof(*list->items), cmp_items); + sort_string_list_with_fn(list, cmp_items); } int unsorted_string_list_has_string(struct string_list *list, const char *string) diff --git a/string-list.h b/string-list.h index 14bbc47..4bbc7e8 100644 --- a/string-list.h +++ b/string-list.h @@ -37,6 +37,8 @@ struct string_list_item *string_list_lookup(const char *string, struct string_li /* Use these functions only on unsorted lists: */ struct string_list_item *string_list_append(const char *string, struct string_list *list); void sort_string_list(struct string_list *list); +void sort_string_list_with_fn(struct string_list *list, + int (*fn)(const void *, const void *)); int unsorted_string_list_has_string(struct string_list *list, const char *string); #endif /* PATH_LIST_H */ ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: t5505-remote fails on Windows 2009-03-19 4:43 ` Jeff King @ 2009-03-19 4:56 ` Jay Soffian 2009-03-19 5:03 ` Jeff King 2009-03-19 7:20 ` Johannes Sixt 2009-03-19 10:36 ` Johannes Schindelin 2 siblings, 1 reply; 18+ messages in thread From: Jay Soffian @ 2009-03-19 4:56 UTC (permalink / raw) To: Jeff King; +Cc: Johannes Sixt, Git Mailing List On Thu, Mar 19, 2009 at 12:43 AM, Jeff King <peff@peff.net> wrote: > The patch below probably fixes it, but as I can't actually reproduce > here, it is largely untested. Thanks. > As a side note, I find this solution a little bit ugly. String lists > should sort on their strings, not on some other random magic in the util > field. This usage really abuses string_list a bit as a data type because > we have no generic "list" type. I really don't think so. The string_list API accommodates this case quite nicely. So why not? (And anyway, what sane configuration pushes a single local branch to multiple remote branches? But the refspecs allow it, so I accommodated it in builtin-remote and tested for it. I never actually tested whether git push works with it tho.) :-) j. ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: t5505-remote fails on Windows 2009-03-19 4:56 ` Jay Soffian @ 2009-03-19 5:03 ` Jeff King 0 siblings, 0 replies; 18+ messages in thread From: Jeff King @ 2009-03-19 5:03 UTC (permalink / raw) To: Jay Soffian; +Cc: Johannes Sixt, Git Mailing List On Thu, Mar 19, 2009 at 12:56:37AM -0400, Jay Soffian wrote: > > As a side note, I find this solution a little bit ugly. String lists > > should sort on their strings, not on some other random magic in the util > > field. This usage really abuses string_list a bit as a data type because > > we have no generic "list" type. > > I really don't think so. The string_list API accommodates this case > quite nicely. So why not? I think the code would be more natural as a list of structs, each with a source and dest. But C does not make it pleasant to write generic data types, so things end up stuffed into string_lists with a magic util field. So I think leaving it as a string_list is probably the most sane thing to do. -Peff ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: t5505-remote fails on Windows 2009-03-19 4:43 ` Jeff King 2009-03-19 4:56 ` Jay Soffian @ 2009-03-19 7:20 ` Johannes Sixt 2009-03-19 20:04 ` Jeff King 2009-03-19 10:36 ` Johannes Schindelin 2 siblings, 1 reply; 18+ messages in thread From: Johannes Sixt @ 2009-03-19 7:20 UTC (permalink / raw) To: Jeff King; +Cc: Jay Soffian, Git Mailing List Jeff King schrieb: > I didn't look closely enough at your issue, which is one > ref pushing to two different places. For that, the sort needs to take > into account the util field, which holds the destination. > > The patch below probably fixes it, but as I can't actually reproduce > here, it is largely untested. Your patch fixes it. The following change to the test case would be a good addition; it protects later tests from failures in earlier tests. -- Hannes diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh index 5ec668d..13f32b8 100755 --- a/t/t5505-remote.sh +++ b/t/t5505-remote.sh @@ -160,7 +160,7 @@ cat > test/expect << EOF master pushes to another (up to date) EOF -test_expect_success 'show' ' +test_expect_success 'more setup for show' ' (cd test && git config --add remote.origin.fetch refs/heads/master:refs/heads/upstream && git fetch && @@ -183,12 +183,20 @@ test_expect_success 'show' ' git config --add remote.origin.push refs/heads/master:refs/heads/upstream && git config --add remote.origin.push +refs/tags/lastbackup && git config --add remote.two.push +refs/heads/ahead:refs/heads/master && - git config --add remote.two.push refs/heads/master:refs/heads/another && + git config --add remote.two.push refs/heads/master:refs/heads/another) +' + +test_expect_success 'show' ' + (cd test && git remote show origin two > output && - git branch -d rebase octopus && test_cmp expect output) ' +test_expect_success 'more setup for show -n' ' + (cd test && + git branch -d rebase octopus) +' + cat > test/expect << EOF * remote origin URL: $(pwd)/one ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: t5505-remote fails on Windows 2009-03-19 7:20 ` Johannes Sixt @ 2009-03-19 20:04 ` Jeff King 0 siblings, 0 replies; 18+ messages in thread From: Jeff King @ 2009-03-19 20:04 UTC (permalink / raw) To: Johannes Sixt; +Cc: Jay Soffian, Git Mailing List On Thu, Mar 19, 2009 at 08:20:18AM +0100, Johannes Sixt wrote: > Your patch fixes it. The following change to the test case would be a good > addition; it protects later tests from failures in earlier tests. I'll clean up and re-submit my patch, then (once we figure out the right API ;) ). Your patch seems reasonable, but it should be a separate patch, I think. -Peff ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: t5505-remote fails on Windows 2009-03-19 4:43 ` Jeff King 2009-03-19 4:56 ` Jay Soffian 2009-03-19 7:20 ` Johannes Sixt @ 2009-03-19 10:36 ` Johannes Schindelin 2009-03-19 11:02 ` Junio C Hamano 2009-03-19 19:52 ` t5505-remote fails on Windows Jeff King 2 siblings, 2 replies; 18+ messages in thread From: Johannes Schindelin @ 2009-03-19 10:36 UTC (permalink / raw) To: Jeff King; +Cc: Johannes Sixt, Jay Soffian, Git Mailing List Hi, On Thu, 19 Mar 2009, Jeff King wrote: > @@ -163,9 +163,15 @@ static int cmp_items(const void *a, const void *b) > return strcmp(one->string, two->string); > } > > +void sort_string_list_with_fn(struct string_list *list, > + int (*fn)(const void *, const void *)) > +{ > + qsort(list->items, list->nr, sizeof(*list->items), fn); > +} > + Do we really want an API for that? Calling qsort() directly should be obvious enough, no? Ciao, Dscho ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: t5505-remote fails on Windows 2009-03-19 10:36 ` Johannes Schindelin @ 2009-03-19 11:02 ` Junio C Hamano 2009-03-19 15:00 ` Jay Soffian 2009-03-19 20:03 ` Jeff King 2009-03-19 19:52 ` t5505-remote fails on Windows Jeff King 1 sibling, 2 replies; 18+ messages in thread From: Junio C Hamano @ 2009-03-19 11:02 UTC (permalink / raw) To: Johannes Schindelin Cc: Jeff King, Johannes Sixt, Jay Soffian, Git Mailing List Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: > On Thu, 19 Mar 2009, Jeff King wrote: > >> @@ -163,9 +163,15 @@ static int cmp_items(const void *a, const void *b) >> return strcmp(one->string, two->string); >> } >> >> +void sort_string_list_with_fn(struct string_list *list, >> + int (*fn)(const void *, const void *)) >> +{ >> + qsort(list->items, list->nr, sizeof(*list->items), fn); >> +} >> + > > Do we really want an API for that? Calling qsort() directly should be > obvious enough, no? I think so. If it were done like this (notice the lack of double indirection in the cmp_fn signature): typedef int string_list_item_cmp_fn(const struct string_list_item *, const struct string_list_item *); void sort_string_list_with_fn(struct string_list *list, string_list_item_cmp_fn *); it would have made more sense, though. ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: t5505-remote fails on Windows 2009-03-19 11:02 ` Junio C Hamano @ 2009-03-19 15:00 ` Jay Soffian 2009-03-19 20:03 ` Jeff King 1 sibling, 0 replies; 18+ messages in thread From: Jay Soffian @ 2009-03-19 15:00 UTC (permalink / raw) To: Junio C Hamano Cc: Johannes Schindelin, Jeff King, Johannes Sixt, Git Mailing List On Thu, Mar 19, 2009 at 7:02 AM, Junio C Hamano <gitster@pobox.com> wrote: >> Do we really want an API for that? Calling qsort() directly should be >> obvious enough, no? > > I think so. If it were done like this (notice the lack of double > indirection in the cmp_fn signature): > > typedef int string_list_item_cmp_fn(const struct string_list_item *, const struct string_list_item *); > > void sort_string_list_with_fn(struct string_list *list, string_list_item_cmp_fn *); > > it would have made more sense, though. Oh, wow, sorry, I didn't even realize Jeff had just added that function. Somehow I missed that part of his patch. j. ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: t5505-remote fails on Windows 2009-03-19 11:02 ` Junio C Hamano 2009-03-19 15:00 ` Jay Soffian @ 2009-03-19 20:03 ` Jeff King 2009-03-19 23:15 ` Johannes Schindelin 2009-03-22 8:59 ` [PATCH] remote: improve sorting of "configure for git push" list Jeff King 1 sibling, 2 replies; 18+ messages in thread From: Jeff King @ 2009-03-19 20:03 UTC (permalink / raw) To: Junio C Hamano Cc: Johannes Schindelin, Johannes Sixt, Jay Soffian, Git Mailing List On Thu, Mar 19, 2009 at 04:02:19AM -0700, Junio C Hamano wrote: > > Do we really want an API for that? Calling qsort() directly should be > > obvious enough, no? > > I think so. If it were done like this (notice the lack of double > indirection in the cmp_fn signature): > > typedef int string_list_item_cmp_fn(const struct string_list_item *, const struct string_list_item *); > > void sort_string_list_with_fn(struct string_list *list, string_list_item_cmp_fn *); > > it would have made more sense, though. IIRC, that is actually not valid C according to the standard (that is, even though a void* can be implicitly assigned to any other pointer, a function taking a void* and a function taking another pointer do not necessarily have the same function signature or calling conventions). Which is why cmp_items in string-list.c already does the indirection. That being said, I think this is probably one of those cases where it works fine on any sane platform that we care about and we can choose to ignore the standard. Actually, I think an even nicer API would be to give a function that _just_ compares the util fields; string_list_sort() would sort on the "string" fields, and only call the util cmp to sort entries with equal strings. Sadly, because qsort lacks a slot for extra data to pass to the callback, implementing this ends up with a global variable: static int (*cmp_util)(const void *, const void *); int cmp_items(const void *a, const void *b) { const struct string_list_item *one = a; const struct string_list_item *two = b; int cmp = strcmp(one->string, two->string); return cmp ? cmp : cmp_util(one->util, b->util); } int sort_string_list_with_util(struct string_list *list, int (*fn)(const void *, const void *)) { cmp_util = fn; qsort(list->items, list->nr, sizeof(*list->items), cmp_items); } If only C had closures. ;) But this is really getting beyond the issue at hand. I can submit a finalized patch; just let me know how you prefer it. -Peff ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: t5505-remote fails on Windows 2009-03-19 20:03 ` Jeff King @ 2009-03-19 23:15 ` Johannes Schindelin 2009-03-22 8:59 ` [PATCH] remote: improve sorting of "configure for git push" list Jeff King 1 sibling, 0 replies; 18+ messages in thread From: Johannes Schindelin @ 2009-03-19 23:15 UTC (permalink / raw) To: Jeff King; +Cc: Junio C Hamano, Johannes Sixt, Jay Soffian, Git Mailing List Hi, On Thu, 19 Mar 2009, Jeff King wrote: > On Thu, Mar 19, 2009 at 04:02:19AM -0700, Junio C Hamano wrote: > > > > Do we really want an API for that? Calling qsort() directly should be > > > obvious enough, no? > > > > I think so. If it were done like this (notice the lack of double > > indirection in the cmp_fn signature): > > > > typedef int string_list_item_cmp_fn(const struct string_list_item *, const struct string_list_item *); > > > > void sort_string_list_with_fn(struct string_list *list, string_list_item_cmp_fn *); > > > > it would have made more sense, though. > > IIRC, that is actually not valid C according to the standard (that is, > even though a void* can be implicitly assigned to any other pointer, a > function taking a void* and a function taking another pointer do not > necessarily have the same function signature or calling conventions). > Which is why cmp_items in string-list.c already does the indirection. AFAICT the idea was not to pass the function to qsort() directly, but I have to agree that I do not see how that should be possible with the current interface of qsort(); Ciao, Dscho ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH] remote: improve sorting of "configure for git push" list 2009-03-19 20:03 ` Jeff King 2009-03-19 23:15 ` Johannes Schindelin @ 2009-03-22 8:59 ` Jeff King 2009-03-22 14:47 ` Jay Soffian 2009-03-23 7:56 ` Johannes Sixt 1 sibling, 2 replies; 18+ messages in thread From: Jeff King @ 2009-03-22 8:59 UTC (permalink / raw) To: Junio C Hamano Cc: Johannes Schindelin, Johannes Sixt, Jay Soffian, Git Mailing List The data structure used to store this list is a string_list of sources with the destination in the util member. The current code just sorts on the source; if a single source is pushed to two different destination refs at a remote, then the order in which they are printed is non-deterministic. This patch implements a comparison using both fields. Besides being a little nicer on the eyes, giving a stable sort prevents false negatives in the test suite when comparing output. Signed-off-by: Jeff King <peff@peff.net> --- The discussion of the proper interface to the one-line wrapper to qsort didn't really resolve anything. It seems that the constraints of C make a nice wrapper a little painful, so let's just use a bare qsort. Once again, I really don't care what it looks like, so feel free to mark it up if you prefer differently; I just want it off my todo list, and to let JSixt run his tests in peace. I did have one somewhat sick thought for an API: have string_list's cmp_items (or an alternate cmp_items_with_util) treat a non-NULL util member as a pointer to a string, and use it as a secondary key in the sort. It works here because the first member of the push_info struct is the secondary key. But I think it is a bit too subtle for my taste. builtin-remote.c | 17 ++++++++++++++++- 1 files changed, 16 insertions(+), 1 deletions(-) diff --git a/builtin-remote.c b/builtin-remote.c index 993acd6..9ef846f 100644 --- a/builtin-remote.c +++ b/builtin-remote.c @@ -922,6 +922,20 @@ int add_push_to_show_info(struct string_list_item *push_item, void *cb_data) return 0; } +/* + * Sorting comparison for a string list that has push_info + * structs in its util field + */ +static int cmp_string_with_push(const void *va, const void *vb) +{ + const struct string_list_item *a = va; + const struct string_list_item *b = vb; + const struct push_info *a_push = a->util; + const struct push_info *b_push = b->util; + int cmp = strcmp(a->string, b->string); + return cmp ? cmp : strcmp(a_push->dest, b_push->dest); +} + int show_push_info_item(struct string_list_item *item, void *cb_data) { struct show_info *show_info = cb_data; @@ -1032,7 +1046,8 @@ static int show(int argc, const char **argv) info.width = info.width2 = 0; for_each_string_list(add_push_to_show_info, &states.push, &info); - sort_string_list(info.list); + qsort(info.list->items, info.list->nr, + sizeof(*info.list->items), cmp_string_with_push); if (info.list->nr) printf(" Local ref%s configured for 'git push'%s:\n", info.list->nr > 1 ? "s" : "", -- 1.6.2.1.276.gd47fa ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH] remote: improve sorting of "configure for git push" list 2009-03-22 8:59 ` [PATCH] remote: improve sorting of "configure for git push" list Jeff King @ 2009-03-22 14:47 ` Jay Soffian 2009-03-22 21:58 ` Junio C Hamano 2009-03-23 7:56 ` Johannes Sixt 1 sibling, 1 reply; 18+ messages in thread From: Jay Soffian @ 2009-03-22 14:47 UTC (permalink / raw) To: Jeff King Cc: Junio C Hamano, Johannes Schindelin, Johannes Sixt, Git Mailing List On Sun, Mar 22, 2009 at 4:59 AM, Jeff King <peff@peff.net> wrote: > The data structure used to store this list is a string_list > of sources with the destination in the util member. The > current code just sorts on the source; if a single source is > pushed to two different destination refs at a remote, then > the order in which they are printed is non-deterministic. > > This patch implements a comparison using both fields. > Besides being a little nicer on the eyes, giving a stable > sort prevents false negatives in the test suite when > comparing output. > > Signed-off-by: Jeff King <peff@peff.net> > --- > The discussion of the proper interface to the one-line wrapper to qsort > didn't really resolve anything. It seems that the constraints of C make > a nice wrapper a little painful, so let's just use a bare qsort. Once > again, I really don't care what it looks like, so feel free to mark it > up if you prefer differently; I just want it off my todo list, and to > let JSixt run his tests in peace. > > I did have one somewhat sick thought for an API: have string_list's > cmp_items (or an alternate cmp_items_with_util) treat a non-NULL util > member as a pointer to a string, and use it as a secondary key in the > sort. It works here because the first member of the push_info struct is > the secondary key. But I think it is a bit too subtle for my taste. Ack. There is one thing I wonder though. Is this even a sane config? It's accepted, but is that intentional or just an accident? Would anyone notice if the push parsing were changed to only accept a single dest for a given source branch? Thanks for the patch Jeff. j. ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] remote: improve sorting of "configure for git push" list 2009-03-22 14:47 ` Jay Soffian @ 2009-03-22 21:58 ` Junio C Hamano 0 siblings, 0 replies; 18+ messages in thread From: Junio C Hamano @ 2009-03-22 21:58 UTC (permalink / raw) To: Jay Soffian Cc: Jeff King, Johannes Schindelin, Johannes Sixt, Git Mailing List Jay Soffian <jaysoffian@gmail.com> writes: > On Sun, Mar 22, 2009 at 4:59 AM, Jeff King <peff@peff.net> wrote: >> The data structure used to store this list is a string_list >> of sources with the destination in the util member. The >> current code just sorts on the source; if a single source is >> pushed to two different destination refs at a remote, then >> the order in which they are printed is non-deterministic. >> ... > Ack. There is one thing I wonder though. Is this even a sane config? I do not think of a workflow that this feature is absolutely necessary in the sense that it cannot be worked around if you removed it, but that is not what you asked. I wouldn't be surprised if there are some kernel subsystem maintainers who usually push to their master but when they push their finished goods to their for-linus branch make sure they update their master at the same time. Configuring their local for-linus to push to master and for-linus may be one way to do so. In a one-branch project with two people, it is not entirely inconceivable to arrange for me to push to master and junio while you to push to master and jay, in order to keep track. ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] remote: improve sorting of "configure for git push" list 2009-03-22 8:59 ` [PATCH] remote: improve sorting of "configure for git push" list Jeff King 2009-03-22 14:47 ` Jay Soffian @ 2009-03-23 7:56 ` Johannes Sixt 2009-03-23 8:09 ` Junio C Hamano 1 sibling, 1 reply; 18+ messages in thread From: Johannes Sixt @ 2009-03-23 7:56 UTC (permalink / raw) To: Jeff King Cc: Junio C Hamano, Johannes Schindelin, Jay Soffian, Git Mailing List Tested-by: Johannes Sixt <j6t@kdbg.org> Thanks, -- Hannes ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] remote: improve sorting of "configure for git push" list 2009-03-23 7:56 ` Johannes Sixt @ 2009-03-23 8:09 ` Junio C Hamano 0 siblings, 0 replies; 18+ messages in thread From: Junio C Hamano @ 2009-03-23 8:09 UTC (permalink / raw) To: Johannes Sixt Cc: Jeff King, Johannes Schindelin, Jay Soffian, Git Mailing List Thanks. ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: t5505-remote fails on Windows 2009-03-19 10:36 ` Johannes Schindelin 2009-03-19 11:02 ` Junio C Hamano @ 2009-03-19 19:52 ` Jeff King 1 sibling, 0 replies; 18+ messages in thread From: Jeff King @ 2009-03-19 19:52 UTC (permalink / raw) To: Johannes Schindelin; +Cc: Johannes Sixt, Jay Soffian, Git Mailing List On Thu, Mar 19, 2009 at 11:36:50AM +0100, Johannes Schindelin wrote: > > +void sort_string_list_with_fn(struct string_list *list, > > + int (*fn)(const void *, const void *)) > > +{ > > + qsort(list->items, list->nr, sizeof(*list->items), fn); > > +} > > + > > Do we really want an API for that? Calling qsort() directly should be > obvious enough, no? I'm fine with that, too. I was just encapsulating out of habit. But if it is normal to peek directly at list->nr and list->items, then calling qsort directly is fine. -Peff ^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2009-03-23 8:11 UTC | newest] Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2009-03-18 11:42 t5505-remote fails on Windows Johannes Sixt 2009-03-19 4:18 ` Jeff King 2009-03-19 4:43 ` Jeff King 2009-03-19 4:56 ` Jay Soffian 2009-03-19 5:03 ` Jeff King 2009-03-19 7:20 ` Johannes Sixt 2009-03-19 20:04 ` Jeff King 2009-03-19 10:36 ` Johannes Schindelin 2009-03-19 11:02 ` Junio C Hamano 2009-03-19 15:00 ` Jay Soffian 2009-03-19 20:03 ` Jeff King 2009-03-19 23:15 ` Johannes Schindelin 2009-03-22 8:59 ` [PATCH] remote: improve sorting of "configure for git push" list Jeff King 2009-03-22 14:47 ` Jay Soffian 2009-03-22 21:58 ` Junio C Hamano 2009-03-23 7:56 ` Johannes Sixt 2009-03-23 8:09 ` Junio C Hamano 2009-03-19 19:52 ` t5505-remote fails on Windows Jeff King
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.