Re: [PATCH] t5540-http-push.sh: avoid non-portable grep -P

Jay Soffian [off-list ref] writes:

On Thu, Feb 26, 2009 at 5:37 PM, Junio C Hamano [off-list ref] wrote:

quoted

I don't mind Perl as we already depend on it; the looseness of the regexp
stil bothers me somewhat, though...

I think you're letting the perfect be the enemy of the good. The point
of the test is merely to check for the SHA-1 has suffix in PUT/MOVE
operations. Any of my suggestions so far are better than what is there
now. Why so much fuss?

I'll send one more iteration, and if that's not good enough, I give up.

j.

Heh, at least with /a-z/a-f/, I think it is usable.

Or is there a reason I am missing that we want to allow g-z there?

On Thu, Feb 26, 2009 at 6:29 PM, Junio C Hamano [off-list ref] wrote:

Heh, at least with /a-z/a-f/, I think it is usable.

Or is there a reason I am missing that we want to allow g-z there?

OMG, I didn't even notice that until this message. When you asked:

Why [a-z0-9] not [0-9a-f]?

I noticed only that you had flipped the relative positions of the
alphas and the digits, no that you had also a-z to a-f. Please squash
this on top of the last patch I sent:

diff --git a/t/t5540-http-push.sh b/t/t5540-http-push.sh
index 470bca3..bd45203 100755
--- a/t/t5540-http-push.sh
+++ b/t/t5540-http-push.sh

@@ -94,7 +94,7 @@ test_expect_success 'MKCOL sends directory names

with trailing slashes' '

 '

-x1="[0-9a-z]"
+x1="[0-9a-f]"
 x2="$x1$x1"
 x5="$x1$x1$x1$x1$x1"
 x38="$x5$x5$x5$x5$x5$x5$x5$x1$x1$x1"


Sheesh.

j.

OS X's GNU grep does not support -P/--perl-regexp.

We use a basic RE instead, and simplify the pattern slightly by
replacing '+' with '*' so it can be more easily expressed using a basic
RE. The important part of pattern, checking for a SHA-1 has suffix in
the successful PUT/MOVE operations, remains the same. Also, a-z instead
of a-f was an obvious mistake in the original RE. Here are samples of
what we want to match:

127.0.0.1 - - [26/Feb/2009:22:38:13 +0000] "PUT /test_repo.git/objects/3e/a4fbb9e18a401a6463c595d08118fcb9fb7426_fab55116904c665a95438bcc78521444a7db6096 HTTP/1.1" 201 277
127.0.0.1 - - [26/Feb/2009:22:38:13 +0000] "MOVE /test_repo.git/objects/3e/a4fbb9e18a401a6463c595d08118fcb9fb7426_fab55116904c665a95438bcc78521444a7db6096 HTTP/1.1" 201 277

Signed-off-by: Jay Soffian <redacted>
---
On Thu, Feb 26, 2009 at 6:29 PM, Junio C Hamano [off-list ref] wrote:

Heh, at least with /a-z/a-f/, I think it is usable.

Or is there a reason I am missing that we want to allow g-z there?

I can't believe I didn't notice that. This is really really my last
send. I noticed gmail line wrapped my squash, so I'm resending properly.
Updated the commit note as well to mention the change from a-z to a-f is
on purpose.

j.

 t/t5540-http-push.sh |   11 ++++++++---
 1 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/t/t5540-http-push.sh b/t/t5540-http-push.sh
index 11b3432..bd45203 100755
--- a/t/t5540-http-push.sh
+++ b/t/t5540-http-push.sh

@@ -94,10 +94,15 @@ test_expect_success 'MKCOL sends directory names with trailing slashes' '
 
 '
 
-test_expect_success 'PUT and MOVE sends object to URLs with SHA-1 hash suffix' '
+x1="[0-9a-f]"
+x2="$x1$x1"
+x5="$x1$x1$x1$x1$x1"
+x38="$x5$x5$x5$x5$x5$x5$x5$x1$x1$x1"
+x40="$x38$x2"
 
-	grep -P "\"(?:PUT|MOVE) .+objects/[\da-z]{2}/[\da-z]{38}_[\da-z\-]{40} HTTP/[0-9.]+\" 20\d" \
-		< "$HTTPD_ROOT_PATH"/access.log
+test_expect_success 'PUT and MOVE sends object to URLs with SHA-1 hash suffix' '
+	sed -e "s/PUT/OP/" -e "s/MOVE/OP/" < "$HTTPD_ROOT_PATH"/access.log \
+	| grep "\"OP .*/objects/$x2/${x38}_$x40 HTTP/[.0-9]*\" 20[0-9]"
 
 '
 

-- 
1.6.2.rc1.309.g5f417

Junio C Hamano wrote:

Jay Soffian [off-list ref] writes:

quoted

On Thu, Feb 26, 2009 at 5:37 PM, Junio C Hamano [off-list ref] wrote:

quoted

I don't mind Perl as we already depend on it; the looseness of the regexp
stil bothers me somewhat, though...

I think you're letting the perfect be the enemy of the good. The point
of the test is merely to check for the SHA-1 has suffix in PUT/MOVE
operations. Any of my suggestions so far are better than what is there
now. Why so much fuss?

I'll send one more iteration, and if that's not good enough, I give up.

j.

Heh, at least with /a-z/a-f/, I think it is usable.

Two minor style issues can also be fixed.

I think the file name can be specified as an argument to sed rather than using
the shell's redirection mechanism.

   sed -e 'script' input-file

rather than

   sed -e 'script' < input-file

I think /that/, and moving the pipe character to the end of the sed line so that
you don't need to escape the newline will conform to git scripting style so it
becomes:

   sed -e "s/PUT/OP/" -e "s/MOVE/OP/" "$HTTPD_ROOT_PATH"/access.log |
     grep "\"OP .*/objects/$x2/${x38}_$x40 HTTP/[.0-9]*\" 20[0-9]"

-brandon

Hi,

On Thu, 26 Feb 2009, Brandon Casey wrote:

   sed -e 'script' input-file

rather than

   sed -e 'script' < input-file

What should make the former more preferable to the latter?

Especially given that the latter way is preferable with other commands (at 
least as far as our test suite is concerned), such as grep, because you do 
not get the file name as part of the result?

And especially given that sed means _stream_ editor, not file editor?

Ciao,
Dscho

Johannes Schindelin wrote:

Hi,

On Thu, 26 Feb 2009, Brandon Casey wrote:

quoted

   sed -e 'script' input-file

rather than

   sed -e 'script' < input-file

What should make the former more preferable to the latter?

It's less complex, but as you describe in the next paragraph, if the
file name is not desired in the result then the latter is preferable.
I initially viewed the latter form as a useless use of cat, equivalent
to:

   cat input-file | sed -e 'script'

Especially given that the latter way is preferable with other commands (at 
least as far as our test suite is concerned), such as grep, because you do 
not get the file name as part of the result?

And especially given that sed means _stream_ editor, not file editor?

especially? Your first argument is valid, but this last sentence means nothing.

-brandon