Re: [PATCH v7 1/3] tests: Adjust the configuration for Apache 2.2

Johannes Schindelin [off-list ref] 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
<RequireAll/> 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 <redacted>
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 <RequireAll> 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 <redacted>
Signed-off-by: Junio C Hamano <redacted>
---
 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
 </LocationMatch>
 <LocationMatch /smart_headers/>
-	<RequireAll>
-		Require expr %{HTTP:x-magic-one} == 'abra'
-		Require expr %{HTTP:x-magic-two} == 'cadabra'
-	</RequireAll>
 	SetEnv GIT_EXEC_PATH ${GIT_EXEC_PATH}
 	SetEnv GIT_HTTP_EXPORT_ALL
 </LocationMatch>

@@ -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 <RequireAll>, 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]
+
 <IfDefine SSL>
 LoadModule ssl_module modules/mod_ssl.so
 

-- 
2.8.2-557-gee41d5e

On Mon, May 09, 2016 at 09:42:32AM -0700, Junio C Hamano wrote:

quoted

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

Hi Junio,

On Mon, 9 May 2016, Junio C Hamano wrote:

Johannes Schindelin [off-list ref] writes:

quoted

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.

quoted

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