t5551: compare sorted cookies files
diff mbox series

Message ID 20180917191806.19958-1-t.gummerer@gmail.com
State New
Headers show
Series
  • t5551: compare sorted cookies files
Related show

Commit Message

Thomas Gummerer Sept. 17, 2018, 7:18 p.m. UTC
In t5551 we check that we save cookies correctly to a file when
http.cookiefile and http.savecookies are set.  To do so we create an
expect file that expects the cookies in a certain order.

However after e2ef8d6fa ("cookies: support creation-time attribute for
cookies", 2018-08-28) in curl.git (released in curl 7.61.1) that order
changed.

We document the file format as "Netscape/Mozilla cookie file
format (see curl(1))", so any format produced by libcurl should be
fine here.  Sort the files, to be agnostic to the order of the
cookies, and make the test pass with both curl versions > 7.61.1 and
earlier curl versions.

Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
---
 t/t5551-http-fetch-smart.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Junio C Hamano Sept. 17, 2018, 7:55 p.m. UTC | #1
Thomas Gummerer <t.gummerer@gmail.com> writes:

> In t5551 we check that we save cookies correctly to a file when
> http.cookiefile and http.savecookies are set.  To do so we create an
> expect file that expects the cookies in a certain order.
>
> However after e2ef8d6fa ("cookies: support creation-time attribute for
> cookies", 2018-08-28) in curl.git (released in curl 7.61.1) that order
> changed.
>
> We document the file format as "Netscape/Mozilla cookie file
> format (see curl(1))", so any format produced by libcurl should be
> fine here.  Sort the files, to be agnostic to the order of the
> cookies, and make the test pass with both curl versions > 7.61.1 and
> earlier curl versions.
>
> Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
> ---

Thanks.  f5b2c9c9 ("t5551-http-fetch-smart.sh: sort cookies before
comparing", 2018-09-07) that came from

    https://public-inbox.org/git/20180907232205.31328-1-tmz@pobox.com

has almost the identical patch text, and this (presumably an
independent effort) confirms that the patch is needed.  The other
effort implicitly depends on the expected output is kept sorted, but
this one is more explicit---I tend to prefer this approach as tools
and automation is easier to maintain than having to remember that
the source must be sorted.

Thanks.

>  t/t5551-http-fetch-smart.sh | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh
> index 771f36f9ff..d13b993201 100755
> --- a/t/t5551-http-fetch-smart.sh
> +++ b/t/t5551-http-fetch-smart.sh
> @@ -206,7 +206,7 @@ test_expect_success 'dumb clone via http-backend respects namespace' '
>  cat >cookies.txt <<EOF
>  127.0.0.1	FALSE	/smart_cookies/	FALSE	0	othername	othervalue
>  EOF
> -cat >expect_cookies.txt <<EOF
> +cat <<EOF | sort >expect_cookies.txt
>  
>  127.0.0.1	FALSE	/smart_cookies/	FALSE	0	othername	othervalue
>  127.0.0.1	FALSE	/smart_cookies/repo.git/info/	FALSE	0	name	value
> @@ -215,7 +215,7 @@ test_expect_success 'cookies stored in http.cookiefile when http.savecookies set
>  	git config http.cookiefile cookies.txt &&
>  	git config http.savecookies true &&
>  	git ls-remote $HTTPD_URL/smart_cookies/repo.git master &&
> -	tail -3 cookies.txt >cookies_tail.txt &&
> +	tail -3 cookies.txt | sort >cookies_tail.txt &&
>  	test_cmp expect_cookies.txt cookies_tail.txt
>  '
Thomas Gummerer Sept. 17, 2018, 9:10 p.m. UTC | #2
On 09/17, Junio C Hamano wrote:
> Thomas Gummerer <t.gummerer@gmail.com> writes:
> 
> > In t5551 we check that we save cookies correctly to a file when
> > http.cookiefile and http.savecookies are set.  To do so we create an
> > expect file that expects the cookies in a certain order.
> >
> > However after e2ef8d6fa ("cookies: support creation-time attribute for
> > cookies", 2018-08-28) in curl.git (released in curl 7.61.1) that order
> > changed.
> >
> > We document the file format as "Netscape/Mozilla cookie file
> > format (see curl(1))", so any format produced by libcurl should be
> > fine here.  Sort the files, to be agnostic to the order of the
> > cookies, and make the test pass with both curl versions > 7.61.1 and
> > earlier curl versions.
> >
> > Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
> > ---
> 
> Thanks.  f5b2c9c9 ("t5551-http-fetch-smart.sh: sort cookies before
> comparing", 2018-09-07) that came from
> 
>     https://public-inbox.org/git/20180907232205.31328-1-tmz@pobox.com
> 
> has almost the identical patch text, and this (presumably an
> independent effort) confirms that the patch is needed.

Whoops awkward, I should have checked 'pu' before starting to work on
this.  This was an independent effort, but I really should
have checked 'pu' before starting on this.

>                                                         The other
> effort implicitly depends on the expected output is kept sorted, but
> this one is more explicit---I tend to prefer this approach as tools
> and automation is easier to maintain than having to remember that
> the source must be sorted.

I'm happy going with either patch, but if we want to go with mine, I'd
like to make sure Todd is credited appropriately, as he sent a very
similar patch first.  Not sure what the appropriate way here is
though?

> Thanks.
> 
> >  t/t5551-http-fetch-smart.sh | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh
> > index 771f36f9ff..d13b993201 100755
> > --- a/t/t5551-http-fetch-smart.sh
> > +++ b/t/t5551-http-fetch-smart.sh
> > @@ -206,7 +206,7 @@ test_expect_success 'dumb clone via http-backend respects namespace' '
> >  cat >cookies.txt <<EOF
> >  127.0.0.1	FALSE	/smart_cookies/	FALSE	0	othername	othervalue
> >  EOF
> > -cat >expect_cookies.txt <<EOF
> > +cat <<EOF | sort >expect_cookies.txt
> >  
> >  127.0.0.1	FALSE	/smart_cookies/	FALSE	0	othername	othervalue
> >  127.0.0.1	FALSE	/smart_cookies/repo.git/info/	FALSE	0	name	value
> > @@ -215,7 +215,7 @@ test_expect_success 'cookies stored in http.cookiefile when http.savecookies set
> >  	git config http.cookiefile cookies.txt &&
> >  	git config http.savecookies true &&
> >  	git ls-remote $HTTPD_URL/smart_cookies/repo.git master &&
> > -	tail -3 cookies.txt >cookies_tail.txt &&
> > +	tail -3 cookies.txt | sort >cookies_tail.txt &&
> >  	test_cmp expect_cookies.txt cookies_tail.txt
> >  '
Jonathan Nieder Sept. 17, 2018, 9:19 p.m. UTC | #3
Thomas Gummerer wrote:
> On 09/17, Junio C Hamano wrote:

>>                                                         The other
>> effort implicitly depends on the expected output is kept sorted, but
>> this one is more explicit---I tend to prefer this approach as tools
>> and automation is easier to maintain than having to remember that
>> the source must be sorted.
>
> I'm happy going with either patch, but if we want to go with mine, I'd
> like to make sure Todd is credited appropriately, as he sent a very
> similar patch first.  Not sure what the appropriate way here is
> though?

Thanks for asking.  Credit is a subject that is dear to my heart.

You can for example use
Reported-by: Todd Zullinger <tmz@pobox.com>

to credit him for the patch and analysis that appears to have helped
with reviews (and to signal that this fixes the bug he reported).

[...]
>>> --- a/t/t5551-http-fetch-smart.sh
>>> +++ b/t/t5551-http-fetch-smart.sh
>>> @@ -206,7 +206,7 @@ test_expect_success 'dumb clone via http-backend respects namespace' '
>>>  cat >cookies.txt <<EOF
>>>  127.0.0.1	FALSE	/smart_cookies/	FALSE	0	othername	othervalue
>>>  EOF
>>> -cat >expect_cookies.txt <<EOF
>>> +cat <<EOF | sort >expect_cookies.txt

Should this be

	sort >expect_cookies.txt <<\EOF

?  That is simpler since it avoids a pipe and means the reader doesn't
have to look out for shell metacharacters like $ inside the text.

Bonus points if this kind of setup moves to inside the test (using
<<-\EOF), which can make the test script easier to read.

Thanks and hope that helps,
Jonathan

Patch
diff mbox series

diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh
index 771f36f9ff..d13b993201 100755
--- a/t/t5551-http-fetch-smart.sh
+++ b/t/t5551-http-fetch-smart.sh
@@ -206,7 +206,7 @@  test_expect_success 'dumb clone via http-backend respects namespace' '
 cat >cookies.txt <<EOF
 127.0.0.1	FALSE	/smart_cookies/	FALSE	0	othername	othervalue
 EOF
-cat >expect_cookies.txt <<EOF
+cat <<EOF | sort >expect_cookies.txt
 
 127.0.0.1	FALSE	/smart_cookies/	FALSE	0	othername	othervalue
 127.0.0.1	FALSE	/smart_cookies/repo.git/info/	FALSE	0	name	value
@@ -215,7 +215,7 @@  test_expect_success 'cookies stored in http.cookiefile when http.savecookies set
 	git config http.cookiefile cookies.txt &&
 	git config http.savecookies true &&
 	git ls-remote $HTTPD_URL/smart_cookies/repo.git master &&
-	tail -3 cookies.txt >cookies_tail.txt &&
+	tail -3 cookies.txt | sort >cookies_tail.txt &&
 	test_cmp expect_cookies.txt cookies_tail.txt
 '