t5505-remote fails on Windows

t5505-remote fails on Windows

[Johannes Sixt]

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

Re: t5505-remote fails on Windows

[Jeff King]

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

Re: t5505-remote fails on Windows

[Jeff King]

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 */

Re: t5505-remote fails on Windows

[Jay Soffian]

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.

Re: t5505-remote fails on Windows

[Jeff King]

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

Re: t5505-remote fails on Windows

[Johannes Sixt]

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

Re: t5505-remote fails on Windows

[Johannes Schindelin]

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

Re: t5505-remote fails on Windows

[Junio C Hamano]

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.

Re: t5505-remote fails on Windows

[Jay Soffian]

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.

Re: t5505-remote fails on Windows

[Jeff King]

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

Re: t5505-remote fails on Windows

[Jeff King]

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

Re: t5505-remote fails on Windows

[Jeff King]

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

Re: t5505-remote fails on Windows

[Johannes Schindelin]

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

[PATCH] remote: improve sorting of "configure for git push" list

[Jeff King]

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

Re: [PATCH] remote: improve sorting of "configure for git push" list

[Jay Soffian]

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.

Re: [PATCH] remote: improve sorting of "configure for git push" list

[Junio C Hamano]

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.

Re: [PATCH] remote: improve sorting of "configure for git push" list

[Johannes Sixt]

Tested-by: Johannes Sixt <j6t@kdbg.org>

Thanks,
-- Hannes

Re: [PATCH] remote: improve sorting of "configure for git push" list

[Junio C Hamano]

Thanks.