t9401 fails with OS X sed

t9401 fails with OS X sed

[Brian Gernhardt]

I seem to write these kinds of e-mails fairly regularly...

When running t9401-git-cvsserver-crlf:

expecting success: 
    check_status_options cvswork2 textfile.c "" &&
    check_status_options cvswork2 binfile.bin -kb &&
    check_status_options cvswork2 .gitattributes "" &&
    check_status_options cvswork2 mixedUp.c -kb &&
    check_status_options cvswork2 multiline.c -kb &&
    check_status_options cvswork2 multilineTxt.c "" &&
    check_status_options cvswork2/subdir withCr.bin -kb &&
    check_status_options cvswork2 subdir/withCr.bin -kb &&
    check_status_options cvswork2/subdir file.h "" &&
    check_status_options cvswork2 subdir/file.h "" &&
    check_status_options cvswork2/subdir unspecified.other "" &&
    check_status_options cvswork2/subdir newfile.bin "" &&
    check_status_options cvswork2/subdir newfile.c ""

not ok - 12 cvs status - sticky options

I have tracked it down to a sed expression that is parsing the output of cvs status:

49:    got="$(sed -n -e 's/^\s*Sticky Options:\s*//p' "${WORKDIR}/status.out")"

The problem is that cvs outputs "Sticky Options:\t\t(none)\n", but OS X's sed doesn't recognize the \s shortcut.  (According to re_format(5), \s is part of the "enhanced" regex format, which sed doesn't use.)  

It works if I change \s to [[:space:]], but I don't know how portable that is.

~~ Brian Gernhardt

Re: t9401 fails with OS X sed

[Geert Bosch]

On Oct 24, 2012, at 23:54, Brian Gernhardt <brian@gernhardtsoftware.com> wrote:

> It works if I change \s to [[:space:]], but I don't know how portable that is.

As \s is shorthand for the POSIX character class [:space:], I'd say the latter
should be more portable: anything accepting the shorthand should also accept
the full character class. If not, you probably only care about horizontal tab
and space, for which you could just use a simple regular expression. Just a
literal space and tab character between square brackets is probably going to be
most portable, though not most readable.

  -Geert

Re: t9401 fails with OS X sed

[Jeff King]

On Thu, Oct 25, 2012 at 01:04:11AM -0400, Geert Bosch wrote:

> On Oct 24, 2012, at 23:54, Brian Gernhardt <brian@gernhardtsoftware.com> wrote:
> 
> > It works if I change \s to [[:space:]], but I don't know how portable that is.
> 
> As \s is shorthand for the POSIX character class [:space:], I'd say the latter
> should be more portable: anything accepting the shorthand should also accept
> the full character class. If not, you probably only care about horizontal tab
> and space, for which you could just use a simple regular expression. Just a
> literal space and tab character between square brackets is probably going to be
> most portable, though not most readable.

I agree that the POSIX character class would be more portable than "\s",
but we do not have any existing uses of them, and I would worry a little
about older systems like Solaris. If we can simply use a literal space
and tab, that seems like the safest.

Brian, can you work up a patch?

-Peff

Re: t9401 fails with OS X sed

[Torsten Bögershausen]

On 25.10.12 10:41, Jeff King wrote:
> On Thu, Oct 25, 2012 at 01:04:11AM -0400, Geert Bosch wrote:
> 
>> On Oct 24, 2012, at 23:54, Brian Gernhardt <brian@gernhardtsoftware.com> wrote:
>>
>>> It works if I change \s to [[:space:]], but I don't know how portable that is.
>>
>> As \s is shorthand for the POSIX character class [:space:], I'd say the latter
>> should be more portable: anything accepting the shorthand should also accept
>> the full character class. If not, you probably only care about horizontal tab
>> and space, for which you could just use a simple regular expression. Just a
>> literal space and tab character between square brackets is probably going to be
>> most portable, though not most readable.
> 
> I agree that the POSIX character class would be more portable than "\s",
> but we do not have any existing uses of them, and I would worry a little
> about older systems like Solaris. If we can simply use a literal space
> and tab, that seems like the safest.
> 
> Brian, can you work up a patch?
> 
> -Peff

Would this be portable:
(It works on my Mac OS X box after installing cvs)
But I don't have solaris


diff --git a/t/t9401-git-cvsserver-crlf.sh b/t/t9401-git-cvsserver-crlf.sh
index cdb8360..f2ec9d2 100755
--- a/t/t9401-git-cvsserver-crlf.sh
+++ b/t/t9401-git-cvsserver-crlf.sh
@@ -46,7 +46,7 @@ check_status_options() {
        echo "Error from cvs status: $1 $2" >> "${WORKDIR}/marked.log"
        return 1;
     fi
-    got="$(sed -n -e 's/^\s*Sticky Options:\s*//p' "${WORKDIR}/status.out")"
+    got="$(tr '\t' ' ' < "${WORKDIR}/status.out" | sed -n -e 's/^ *Sticky Options: *//p')"
     expect="$3"
     if [ x"$expect" = x"" ] ; then
        expect="(none)"

[PATCH] Use character class for sed expression instead of \s

[Ben Walton]

Sed on Mac OS X doesn't handle \s in a sed expressions so use a more
portable character set expression instead.

Signed-off-by: Ben Walton <bdwalton@gmail.com>
---

Hi Torsten,

I think this would be a nicer fix for the issue although your solution
should work as well.

Thanks
-Ben

 t/t9401-git-cvsserver-crlf.sh |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t9401-git-cvsserver-crlf.sh b/t/t9401-git-cvsserver-crlf.sh
index cdb8360..1c5bc84 100755
--- a/t/t9401-git-cvsserver-crlf.sh
+++ b/t/t9401-git-cvsserver-crlf.sh
@@ -46,7 +46,7 @@ check_status_options() {
 	echo "Error from cvs status: $1 $2" >> "${WORKDIR}/marked.log"
 	return 1;
     fi
-    got="$(sed -n -e 's/^\s*Sticky Options:\s*//p' "${WORKDIR}/status.out")"
+    got="$(sed -n -e 's/^[ 	]*Sticky Options:[ 	]*//p' "${WORKDIR}/status.out")"
     expect="$3"
     if [ x"$expect" = x"" ] ; then
 	expect="(none)"
-- 
1.7.9.5

Re: [PATCH] Use character class for sed expression instead of \s

[Brian Gernhardt]

On Oct 25, 2012, at 11:58 AM, Ben Walton <bdwalton@gmail.com> wrote:

> Sed on Mac OS X doesn't handle \s in a sed expressions so use a more
> portable character set expression instead.
> 
> Signed-off-by: Ben Walton <bdwalton@gmail.com>

Acked-by: Brian Gernhardt <brian@gernhardtsoftware.com>

I have an identical change sitting in my git.git, I've just been too distracted by other things to commit and send it.

~~ Brian Gernhardt

Re: [PATCH] Use character class for sed expression instead of \s

[Torsten Bögershausen]

On 25.10.12 18:00, Brian Gernhardt wrote:
> 
> On Oct 25, 2012, at 11:58 AM, Ben Walton <bdwalton@gmail.com> wrote:
> 
>> Sed on Mac OS X doesn't handle \s in a sed expressions so use a more
>> portable character set expression instead.
>>
>> Signed-off-by: Ben Walton <bdwalton@gmail.com>
> 
> Acked-by: Brian Gernhardt <brian@gernhardtsoftware.com>
> 
> I have an identical change sitting in my git.git, I've just been too distracted by other things to commit and send it.
> 
> ~~ Brian Gernhardt

Much nicer, indeed.

BTW: While we are talking CVS: (I installed a fresh version)
cvs --version
Concurrent Versions System (CVS) 1.11.23 (client/server)


And t9200 fails:
git checkout t9200-git-cvsexportcommit.sh
tb@birne:~/projects/git/git.pu/t> ./t9200-git-cvsexportcommit.sh
cvs [init aborted]: Cannot initialize repository under existing CVSROOT: `/Users/tb/projects/git/git.pu/t/trash directory.t9200-git-cvsexportcommit'
FATAL: Unexpected exit with code 1

The following fixes it, but there are possibly better solutions.
Any comments/suggestions ?

diff t9200-git-cvsexportcommit.sh t9200-git-cvsexportcommit.tb.sh
28,29c28
< mkdir "$CVSROOT" &&
< cvs init &&
---
> (cvs init || mkdir "$CVSROOT" && cvs init ) &&

Re: [PATCH] Use character class for sed expression instead of \s

[Ben Walton]

Hi Torsten,

On Thu, Oct 25, 2012 at 5:28 PM, Torsten Bögershausen <tboegi@web.de> wrote:

> BTW: While we are talking CVS: (I installed a fresh version)
> cvs --version
> Concurrent Versions System (CVS) 1.11.23 (client/server)

I have 1.12.13-MirDebian-8 here.

> And t9200 fails:
> git checkout t9200-git-cvsexportcommit.sh
> tb@birne:~/projects/git/git.pu/t> ./t9200-git-cvsexportcommit.sh
> cvs [init aborted]: Cannot initialize repository under existing CVSROOT: `/Users/tb/projects/git/git.pu/t/trash directory.t9200-git-cvsexportcommit'
> FATAL: Unexpected exit with code 1

I'm not able to reproduce this manually...are you able to make it fail
this way outside of the test harness?

$ CVSROOT=$PWD/bw
$ export CVSROOT
$ mkdir $CVSROOT && cvs init && echo ok
ok
$ rm -rf $CVSROOT
$ cvs init && echo ok
ok

>> (cvs init || mkdir "$CVSROOT" && cvs init ) &&

If your version of cvs fails the checks above in manual testing, we
could see if there is a flag that works in all (old and new) versions
to override the failure if CVSROOT exists.  Otherwise, this isn't a
bad fix, I don't think.

If your version does fail the manual checks, I think it's likely a
regression that was introduced and later reverted.  I don't see those
strings inside my cvs binary at all...?

HTH.

Thanks
-Ben
-- 
---------------------------------------------------------------------------------------------------------------------------
Take the risk of thinking for yourself.  Much more happiness,
truth, beauty and wisdom will come to you that way.

-Christopher Hitchens
---------------------------------------------------------------------------------------------------------------------------

Re: [PATCH] Use character class for sed expression instead of \s

[Torsten Bögershausen]

On 10/25/2012 08:08 PM, Ben Walton wrote:
> Hi Torsten,
>
> On Thu, Oct 25, 2012 at 5:28 PM, Torsten Bögershausen <tboegi@web.de> wrote:
>
>> BTW: While we are talking CVS: (I installed a fresh version)
>> cvs --version
>> Concurrent Versions System (CVS) 1.11.23 (client/server)
>
> I have 1.12.13-MirDebian-8 here.
>
>> And t9200 fails:
>> git checkout t9200-git-cvsexportcommit.sh
>> tb@birne:~/projects/git/git.pu/t> ./t9200-git-cvsexportcommit.sh
>> cvs [init aborted]: Cannot initialize repository under existing CVSROOT: `/Users/tb/projects/git/git.pu/t/trash directory.t9200-git-cvsexportcommit'
>> FATAL: Unexpected exit with code 1
>
> I'm not able to reproduce this manually...are you able to make it fail
> this way outside of the test harness?
>
> $ CVSROOT=$PWD/bw
> $ export CVSROOT
> $ mkdir $CVSROOT && cvs init && echo ok
> ok
> $ rm -rf $CVSROOT
> $ cvs init && echo ok
> ok
>
>>> (cvs init || mkdir "$CVSROOT" && cvs init ) &&
>
> If your version of cvs fails the checks above in manual testing, we
> could see if there is a flag that works in all (old and new) versions
> to override the failure if CVSROOT exists.  Otherwise, this isn't a
> bad fix, I don't think.
>
> If your version does fail the manual checks, I think it's likely a
> regression that was introduced and later reverted.  I don't see those
> strings inside my cvs binary at all...?
>
> HTH.
>
> Thanks
> -Ben
>
Hej Ben,
thanks for looking into that - here some short answers:

a) The manual test (as you describe it) succeeds
b) The test case 9200 failes, and now I know why:

diff --git a/t/t9200-git-cvsexportcommit.sh b/t/t9200-git-cvsexportcommit.sh
index b59be9a..d2c3c37 100755
--- a/t/t9200-git-cvsexportcommit.sh
+++ b/t/t9200-git-cvsexportcommit.sh
@@ -19,7 +19,7 @@ then
      test_done
  fi

-CVSROOT=$PWD/cvsroot
+CVSROOT=$PWD/xx
  CVSWORK=$PWD/cvswork
  GIT_DIR=$PWD/.git
  export CVSROOT CVSWORK GIT_DIR



c) I need to send a patch tomorrow

d) FYI: I compiled cvs from scratch, from a file called cvs-1.11.23.tar.gz
    and the code is in cvs-1.11.23/src/mkmodules.c:942

if (root_dir && strcmp (root_dir, current_parsed_root->directory))
   error (1, 0,
          "Cannot initialize repository under existing CVSROOT: `%s'",
          root_dir);
     free (root_dir);

/Torsten

Re: [PATCH] Use character class for sed expression instead of \s

[Jeff King]

On Thu, Oct 25, 2012 at 04:58:19PM +0100, Ben Walton wrote:

> Sed on Mac OS X doesn't handle \s in a sed expressions so use a more
> portable character set expression instead.
> 
> Signed-off-by: Ben Walton <bdwalton@gmail.com>

Thanks, I think this simple solution is the best.

-Peff