patcht5540-http-push.sh: avoid non-portable grep -P

OS X's GNU grep does not support -P/--perl-regexp; use egrep instead, avoiding non-portable braces ({}) as well.

Signed-off-by: Jay Soffian <jaysoffian@gmail.com>
---
 t/t5540-http-push.sh |    8 +++++++-
 1 files changed, 7 insertions(+), 1 deletions(-)

Jay Soffian <jaysoffian@gmail.com> writes:

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

I'd rather see the basic BRE grep used if you are shooting for portability.

There are some oddballs in the source (git-submodule.sh is a notable offender) but none of the core-ish scripts uses egrep nor "grep -E".

No reason. It's just what popped out of my head.

Sigh, I just wanted the test to pass. I did a check to see if any other tests were already using egrep, and when I found that they were, I thought that would be good enough.

Originally I had switched to perl. Would you prefer:

On Thu, Feb 26, 2009 at 3:43 PM, Jay Soffian <jaysoffian@gmail.com> wrote:

gmail mangled the lines. Sorry about that.

BTW, I was not aiming for "portable" to the extent that git is portable, I was aiming for "portable enough" to the extent that the test suite is portable. :-)

Let me know if the perl above is okay, or if you'd prefer a basic RE and I'll send a new patch.

j.

Jay Soffian <jaysoffian@gmail.com> writes:

Not really.  

If we are fixing it we should do it right. The regexp you inherited seem somewhat suboptimal, too.

"(?:PUT|MOVE) .+objects/[\da-z]{2}/[\da-z]{38}_[\da-z\-]{40} HTTP/[0-9.]+" 20\d"

 - It has a double-quote. can it be anywhere or only at the beginning (if
   so "^" is missing at the beginning -- I didn't check)?

 - We then expect PUT or MOVE; Ok (that's "\(PUT\|MOVE\)").

 - We then expect one space and one or more garbage before we see
   "objects/"; do we really care if it is one-or-more garbage, or is it
   zero-or-more garbage?  Don't we want to see slash before "objects/" (I
   think we do)?

 - Then we expect objects/ and two hexdigits that should be spelled as
   [0-9a-f]{2} or [0-9a-f][0-9a-f] (or $x2).

 - Then we expect / and thiry-eight hexdigits, underscore and then 40
   hexdigits (I think your $x38 and $x40 are fine);

 - Then we expect exactly one SP followed by HTTP/ (Ok);

 - And version can be one-or-more of [0-9.]; do we really allow
   HTTP/..999?, or we don't care?  I think we shouldn't care, and in that
   case I think [.0-9]* is good enough here;

So perhaps define a variable:

good="\"\(PUT\|MOVE\) .*/objects/$x2/${x38}_$x40 HTTP/[.0-9]*" 20[0-9]\""

and grep for it?

On Thu, Feb 26, 2009 at 4:48 PM, Junio C Hamano <gitster@pobox.com> wrote:

Hmpfh. According to my re_format man page:

Obsolete (``basic'') regular expressions differ in several respects. `|' is an ordinary character and there is no equivalent for its functionality.

j.

Jay Soffian <jaysoffian@gmail.com> writes:

Ahh, you are right; there is no '|'-alternation in BRE.

We of course could do something like:

    good=" .*/objects/$x2/${x38}_$x40 HTTP/[.0-9]*" 20[0-9]\""
    grep -e "\"PUT $good" -e "\"MOVE $good"

But that's too ugly.

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

On Thu, Feb 26, 2009 at 5:37 PM, Junio C Hamano <gitster@pobox.com> wrote:

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.

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. 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 <jaysoffian@gmail.com>
---
Junio,

This is the best I can do. If it's not good enough, and you'd rather have the existing non-portable grep -P, so be it.

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

Jay Soffian <jaysoffian@gmail.com> writes:

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 <gitster@pobox.com> wrote:

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

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:

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 <jaysoffian@gmail.com>
---
On Thu, Feb 26, 2009 at 6:29 PM, Junio C Hamano <gitster@pobox.com> wrote:

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(-)

Junio C Hamano wrote:

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:

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:

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? Your first argument is valid, but this last sentence means nothing.

-brandon

Brandon Casey <casey@nrlssc.navy.mil> writes:

Ok, ok, I heard all of you. I think "sed -e 'script' file" is better because it is one letter shorter than with "<" rediretion, nothing else.

I merely was shooting for rejecting an obvious crap, not aiming for perfection.

Here is the final one. Let's stop wasting time and go on with our lives ;-)

Thanks.

-- >8 --
From: Jay Soffian <jaysoffian@gmail.com>
Date: Thu, 26 Feb 2009 18:44:40 -0500
Subject: [PATCH] t5540-http-push.sh: avoid non-portable grep -P

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 <jaysoffian@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/t5540-http-push.sh |   11 ++++++++---
 1 files changed, 8 insertions(+), 3 deletions(-)

OS X's GNU grep does not support -P/--perl-regexp; use basic REs instead.

Signed-off-by: Jay Soffian <jaysoffian@gmail.com>
---
 t/t5540-http-push.sh |   11 ++++++++---
 1 files changed, 8 insertions(+), 3 deletions(-)