Re: [PATCH v7 1/3] tests: Adjust the configuration for Apache 2.2
Lars Schneider writes: >> On 09 May 2016, at 08:18, Johannes Schindelin >> wrote: >> >> Lars Schneider noticed that the configuration introduced to test the extra >> HTTP headers cannot be used with Apache 2.2 (which is still actively >> maintained, as pointed out by Junio Hamano). >> >> To let the tests pass with Apache 2.2 again, let's substitute the >> offending and `expr` by using old school RewriteCond >> statements. > > All Apache 2.2 tests run nicely on Travis CI with Ubuntu and OSX using > this patch series: > https://travis-ci.org/larsxschneider/git/builds/128955548 > > Thanks, > Lars Thanks for a report. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7 1/3] tests: Adjust the configuration for Apache 2.2
Johannes Schindelin writes: >> To be honest, I do not quite understand why you call it "ugly hack" >> at all. > > Well, it is convoluted. I would have preferred to say "if this condition > is not met or that condition is not met, fail". Instead I had to say "If` > these two conditions are met, proceed as before. Otherwise, fail." > > And of course its ugliness increased in my mind because I had to go > through so many iterations until it finally worked. Not really > straight-forward a solution. To my eyes without your battle scar, it felt more natural to say "If somebody says he is user $U and gives a password $P, let him in; keep everybody else out" when configuring a server than saying "Fail anybody whose user name is not $U or who says his password is not $P". I can certainly understand if somebody who has spent a lot of effort to make things _fail_ finds the latter more natural. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7 1/3] tests: Adjust the configuration for Apache 2.2
Hi Junio, On Mon, 9 May 2016, Junio C Hamano wrote: > Johannes Schindelin writes: > > > Okay, I already force-pushed my extra-http-header branch and the next > > iteration will sport this paragraph. > > The new explanation is well written and can and should also replace the > comment before the implementation in the configuration file to help > readers. I picked your commit and force-pushed my branch; it will be part of the next iteration. > To be honest, I do not quite understand why you call it "ugly hack" > at all. Well, it is convoluted. I would have preferred to say "if this condition is not met or that condition is not met, fail". Instead I had to say "If` these two conditions are met, proceed as before. Otherwise, fail." And of course its ugliness increased in my mind because I had to go through so many iterations until it finally worked. Not really straight-forward a solution. > > Hopefully your patch to remove the -c ... sanitizing makes it to > > `master` soon, then I can submit my next iteration. > > Or we can just merge that "do not sanitize" branch in, and then queue > the "next iteration" which I'd assume would only be the test addition? I'll prepare something. Ciao, Dscho -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7 1/3] tests: Adjust the configuration for Apache 2.2
> On 09 May 2016, at 08:18, Johannes Schindelin
> wrote:
>
> Lars Schneider noticed that the configuration introduced to test the extra
> HTTP headers cannot be used with Apache 2.2 (which is still actively
> maintained, as pointed out by Junio Hamano).
>
> To let the tests pass with Apache 2.2 again, let's substitute the
> offending and `expr` by using old school RewriteCond
> statements.
All Apache 2.2 tests run nicely on Travis CI with Ubuntu and OSX using
this patch series:
https://travis-ci.org/larsxschneider/git/builds/128955548
Thanks,
Lars
>
> Signed-off-by: Johannes Schindelin
> ---
> t/lib-httpd/apache.conf | 12
> 1 file changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf
> index b8ed96f..29b34bb 100644
> --- a/t/lib-httpd/apache.conf
> +++ b/t/lib-httpd/apache.conf
> @@ -103,10 +103,6 @@ Alias /auth/dumb/ www/auth/dumb/
> Header set Set-Cookie name=value
>
>
> -
> - Require expr %{HTTP:x-magic-one} == 'abra'
> - Require expr %{HTTP:x-magic-two} == 'cadabra'
> -
> SetEnv GIT_EXEC_PATH ${GIT_EXEC_PATH}
> SetEnv GIT_HTTP_EXPORT_ALL
>
> @@ -136,6 +132,14 @@ RewriteRule ^/ftp-redir/(.*)$ ftp://localhost:1000/$1
> [R=302]
> RewriteRule ^/loop-redir/x-x-x-x-x-x-x-x-x-x-x-x-x-x-x-x-x-x-x-x-(.*) /$1
> [R=302]
> RewriteRule ^/loop-redir/(.*)$ /loop-redir/x-$1 [R=302]
>
> +# Apache 2.2 does not understand , so we use RewriteCond.
> +# And as RewriteCond unfortunately lacks "not equal" matching, we use this
> +# ugly trick to fail *unless* the two headers are present.
> +RewriteCond %{HTTP:x-magic-one} =abra
> +RewriteCond %{HTTP:x-magic-two} =cadabra
> +RewriteRule ^/smart_headers/.* - [L]
> +RewriteRule ^/smart_headers/.* - [F]
> +
>
> LoadModule ssl_module modules/mod_ssl.so
>
> --
> 2.8.2.463.g99156ee
>
>
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7 1/3] tests: Adjust the configuration for Apache 2.2
Jeff King writes: > On Mon, May 09, 2016 at 09:42:32AM -0700, Junio C Hamano wrote: > >> > Hopefully your patch to remove the -c ... sanitizing makes it to `master` >> > soon, then I can submit my next iteration. >> >> Or we can just merge that "do not sanitize" branch in, and then >> queue the "next iteration" which I'd assume would only be the test >> addition? > > I think we'd also want the change to the test script to make sure that > it fails with only a single header (Dscho's patch 2). I think so, too. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7 1/3] tests: Adjust the configuration for Apache 2.2
On Mon, May 09, 2016 at 09:42:32AM -0700, Junio C Hamano wrote: > > Hopefully your patch to remove the -c ... sanitizing makes it to `master` > > soon, then I can submit my next iteration. > > Or we can just merge that "do not sanitize" branch in, and then > queue the "next iteration" which I'd assume would only be the test > addition? I think we'd also want the change to the test script to make sure that it fails with only a single header (Dscho's patch 2). -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7 1/3] tests: Adjust the configuration for Apache 2.2
Johannes Schindelin writes:
> Okay, I already force-pushed my extra-http-header branch and the next
> iteration will sport this paragraph.
The new explanation is well written and can and should also replace
the comment before the implementation in the configuration file to
help readers.
To be honest, I do not quite understand why you call it "ugly hack"
at all. It is saying "The client is sending a satisfactory request
when these two headers are there; otherwise the request is not
good", which sounds quite natural way to express what is being
tested. The rejection being part of "Rewrite" may be clever, but I
do not see it as ugly.
And it matches the spirit of the implementation for 2.3+ that uses
quite well--you just do not need to say "Fail"
yourself over there, as that is implied.
> Hopefully your patch to remove the -c ... sanitizing makes it to `master`
> soon, then I can submit my next iteration.
Or we can just merge that "do not sanitize" branch in, and then
queue the "next iteration" which I'd assume would only be the test
addition?
Thanks.
-- >8 --
From: Johannes Schindelin
Date: Mon, 9 May 2016 07:59:16 +0200
Subject: [PATCH] tests: adjust the configuration for Apache 2.2
Lars Schneider noticed that the configuration introduced to test the
extra HTTP headers cannot be used with Apache 2.2 (which is still
actively maintained, as pointed out by Junio Hamano).
To let the tests pass with Apache 2.2 again, let's substitute the
offending and `expr` by using old school RewriteCond
statements.
As RewriteCond does not allow testing for *non*-matches, we simply match
the desired case first and let it pass by marking the RewriteRule as
'[L]' ("last rule, do not process any other matching RewriteRules after
this"), and then have another RewriteRule that matches all other cases
and lets them fail via '[F]' ("fail").
Signed-off-by: Johannes Schindelin
Signed-off-by: Junio C Hamano
---
t/lib-httpd/apache.conf | 16
1 file changed, 12 insertions(+), 4 deletions(-)
diff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf
index 838770c..2dcbb00 100644
--- a/t/lib-httpd/apache.conf
+++ b/t/lib-httpd/apache.conf
@@ -102,10 +102,6 @@ Alias /auth/dumb/ www/auth/dumb/
Header set Set-Cookie name=value
-
- Require expr %{HTTP:x-magic-one} == 'abra'
- Require expr %{HTTP:x-magic-two} == 'cadabra'
-
SetEnv GIT_EXEC_PATH ${GIT_EXEC_PATH}
SetEnv GIT_HTTP_EXPORT_ALL
@@ -135,6 +131,18 @@ RewriteRule ^/ftp-redir/(.*)$ ftp://localhost:1000/$1
[R=302]
RewriteRule ^/loop-redir/x-x-x-x-x-x-x-x-x-x-x-x-x-x-x-x-x-x-x-x-(.*) /$1
[R=302]
RewriteRule ^/loop-redir/(.*)$ /loop-redir/x-$1 [R=302]
+# Apache 2.2 does not understand , so we use RewriteCond.
+# And as RewriteCond does not allow testing for non-matches, we match
+# the desired case first (one has abra, two has cadabra), and let it
+# pass by marking the RewriteRule as [L], "last rule, do not process
+# any other matching RewriteRules after this"), and then have another
+# RewriteRule that matches all other cases and lets them fail via '[F]',
+# "fail the request".
+RewriteCond %{HTTP:x-magic-one} =abra
+RewriteCond %{HTTP:x-magic-two} =cadabra
+RewriteRule ^/smart_headers/.* - [L]
+RewriteRule ^/smart_headers/.* - [F]
+
LoadModule ssl_module modules/mod_ssl.so
--
2.8.2-557-gee41d5e
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7 1/3] tests: Adjust the configuration for Apache 2.2
Johannes Schindelin writes:
> +# Apache 2.2 does not understand , so we use RewriteCond.
> +# And as RewriteCond unfortunately lacks "not equal" matching, we use this
> +# ugly trick to fail *unless* the two headers are present.
> +RewriteCond %{HTTP:x-magic-one} =abra
> +RewriteCond %{HTTP:x-magic-two} =cadabra
> +RewriteRule ^/smart_headers/.* - [L]
> +RewriteRule ^/smart_headers/.* - [F]
Clever. Thanks. Will queue.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7 1/3] tests: Adjust the configuration for Apache 2.2
Hi Peff,
On Mon, 9 May 2016, Jeff King wrote:
> On Mon, May 09, 2016 at 04:03:48PM +0200, Johannes Schindelin wrote:
>
> > How about this:
> >
> > As RewriteCond does not allow testing for *non*-matches, we simply
> > match the desired case first and let it pass by marking the
> > RewriteRule as '[L]' ("last rule, do not process any other
> > matching RewriteRules after this"), and then have another
> > RewriteRule that matches all other cases and lets them fail via
> > '[F]' ("fail").
> >
> > Good enough?
>
> Yep, I think that explains it. Thanks.
Okay, I already force-pushed my extra-http-header branch and the next
iteration will sport this paragraph.
Hopefully your patch to remove the -c ... sanitizing makes it to `master`
soon, then I can submit my next iteration.
Ciao,
Dscho
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7 1/3] tests: Adjust the configuration for Apache 2.2
On Mon, May 09, 2016 at 04:03:48PM +0200, Johannes Schindelin wrote:
> How about this:
>
> As RewriteCond does not allow testing for *non*-matches, we simply
> match the desired case first and let it pass by marking the
> RewriteRule as '[L]' ("last rule, do not process any other
> matching RewriteRules after this"), and then have another
> RewriteRule that matches all other cases and lets them fail via
> '[F]' ("fail").
>
> Good enough?
Yep, I think that explains it. Thanks.
-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7 1/3] tests: Adjust the configuration for Apache 2.2
Hi Peff,
On Mon, 9 May 2016, Jeff King wrote:
> On Mon, May 09, 2016 at 08:18:52AM +0200, Johannes Schindelin wrote:
>
> > +# Apache 2.2 does not understand , so we use RewriteCond.
> > +# And as RewriteCond unfortunately lacks "not equal" matching, we use this
> > +# ugly trick to fail *unless* the two headers are present.
> > +RewriteCond %{HTTP:x-magic-one} =abra
> > +RewriteCond %{HTTP:x-magic-two} =cadabra
> > +RewriteRule ^/smart_headers/.* - [L]
> > +RewriteRule ^/smart_headers/.* - [F]
> > +
>
> Thanks, this is the magic that eluded me earlier. I had to look up the
> flags, so for any observers in the same boat, this works because:
>
> - the '[L]' flag says "stop doing any more rewrite rules"; it triggers
> only when the RewriteConds above match
>
> - the '[F]' flag says "return 403 Forbidden"; it triggers always,
> because after a RewriteRule, all RewriteConds are reset
>
> I'm sure that is all apparent to somebody who is familiar with Apache
> config, but I think that does not include most people on this project. I
> dunno if it is worth a comment here or in the commit message.
Oh, you're absolutely correct, I should have described this better. It
took me quite a couple of iterations to get it right, after all.
How about this:
As RewriteCond does not allow testing for *non*-matches, we simply
match the desired case first and let it pass by marking the
RewriteRule as '[L]' ("last rule, do not process any other
matching RewriteRules after this"), and then have another
RewriteRule that matches all other cases and lets them fail via
'[F]' ("fail").
Good enough?
Ciao,
Dscho
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7 1/3] tests: Adjust the configuration for Apache 2.2
On Mon, May 09, 2016 at 08:18:52AM +0200, Johannes Schindelin wrote:
> +# Apache 2.2 does not understand , so we use RewriteCond.
> +# And as RewriteCond unfortunately lacks "not equal" matching, we use this
> +# ugly trick to fail *unless* the two headers are present.
> +RewriteCond %{HTTP:x-magic-one} =abra
> +RewriteCond %{HTTP:x-magic-two} =cadabra
> +RewriteRule ^/smart_headers/.* - [L]
> +RewriteRule ^/smart_headers/.* - [F]
> +
Thanks, this is the magic that eluded me earlier. I had to look up the
flags, so for any observers in the same boat, this works because:
- the '[L]' flag says "stop doing any more rewrite rules"; it triggers
only when the RewriteConds above match
- the '[F]' flag says "return 403 Forbidden"; it triggers always,
because after a RewriteRule, all RewriteConds are reset
I'm sure that is all apparent to somebody who is familiar with Apache
config, but I think that does not include most people on this project. I
dunno if it is worth a comment here or in the commit message.
-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
