# t9401 fails with OS X sed 10 messages from 2012-10-25 to 2012-10-26. Participants: Brian Gernhardt, Geert Bosch, Jeff King, Torsten Bögershausen, Ben Walton. Thread: https://gitlist.dev/t/31936 ## Brian Gernhardt, 2012-10-25 03:54 Subject: t9401 fails with OS X sed Message-ID: <609AC6E7-45CD-4472-B1DC-FBB785D6B815@gernhardtsoftware.com> URL: https://gitlist.dev/e/609AC6E7-45CD-4472-B1DC-FBB785D6B815%40gernhardtsoftware.com ``` 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 ``` ## Geert Bosch, 2012-10-25 05:04 Subject: Re: t9401 fails with OS X sed Message-ID: URL: https://gitlist.dev/e/F721B376-F4E6-4274-9A6E-BD1CFCBDA39F%40adacore.com In-Reply-To: <609AC6E7-45CD-4472-B1DC-FBB785D6B815@gernhardtsoftware.com> ``` On Oct 24, 2012, at 23:54, Brian Gernhardt 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 ``` ## Jeff King, 2012-10-25 08:41 Subject: Re: t9401 fails with OS X sed Message-ID: <20121025084132.GB8390@sigill.intra.peff.net> URL: https://gitlist.dev/e/20121025084132.GB8390%40sigill.intra.peff.net In-Reply-To: ``` On Thu, Oct 25, 2012 at 01:04:11AM -0400, Geert Bosch wrote: > On Oct 24, 2012, at 23:54, Brian Gernhardt 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 ``` ## Torsten Bögershausen, 2012-10-25 12:51 Subject: Re: t9401 fails with OS X sed Message-ID: <508935CB.9020408@web.de> URL: https://gitlist.dev/e/508935CB.9020408%40web.de In-Reply-To: <20121025084132.GB8390@sigill.intra.peff.net> ``` 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 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)" ``` ## Ben Walton, 2012-10-25 15:58 Subject: [PATCH] Use character class for sed expression instead of \s Message-ID: <1351180699-24695-1-git-send-email-bdwalton@gmail.com> URL: https://gitlist.dev/e/1351180699-24695-1-git-send-email-bdwalton%40gmail.com In-Reply-To: <508935CB.9020408@web.de> ``` 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 --- 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 ``` ## Brian Gernhardt, 2012-10-25 16:00 Subject: Re: [PATCH] Use character class for sed expression instead of \s Message-ID: URL: https://gitlist.dev/e/C2AB6973-7BC2-45A4-836E-BB1FAAE7501C%40gernhardtsoftware.com In-Reply-To: <1351180699-24695-1-git-send-email-bdwalton@gmail.com> ``` On Oct 25, 2012, at 11:58 AM, 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 Acked-by: Brian Gernhardt 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 ``` ## Torsten Bögershausen, 2012-10-25 16:28 Subject: Re: [PATCH] Use character class for sed expression instead of \s Message-ID: <5089689A.9070301@web.de> URL: https://gitlist.dev/e/5089689A.9070301%40web.de In-Reply-To: ``` On 25.10.12 18:00, Brian Gernhardt wrote: > > On Oct 25, 2012, at 11:58 AM, 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 > > Acked-by: Brian Gernhardt > > 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 ) && ``` ## Ben Walton, 2012-10-25 18:08 Subject: Re: [PATCH] Use character class for sed expression instead of \s Message-ID: URL: https://gitlist.dev/e/CAP30j15n1hVn6zptDpAfM%2BAqc3LnRR4PN6jHTHpTkcjYLgPnjw%40mail.gmail.com In-Reply-To: <5089689A.9070301@web.de> ``` Hi Torsten, On Thu, Oct 25, 2012 at 5:28 PM, Torsten Bögershausen 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 --------------------------------------------------------------------------------------------------------------------------- ``` ## Torsten Bögershausen, 2012-10-25 20:09 Subject: Re: [PATCH] Use character class for sed expression instead of \s Message-ID: <50899C83.6090008@web.de> URL: https://gitlist.dev/e/50899C83.6090008%40web.de In-Reply-To: ``` On 10/25/2012 08:08 PM, Ben Walton wrote: > Hi Torsten, > > On Thu, Oct 25, 2012 at 5:28 PM, Torsten Bögershausen 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 ``` ## Jeff King, 2012-10-26 12:38 Subject: Re: [PATCH] Use character class for sed expression instead of \s Message-ID: <20121026123854.GB1455@sigill.intra.peff.net> URL: https://gitlist.dev/e/20121026123854.GB1455%40sigill.intra.peff.net In-Reply-To: <1351180699-24695-1-git-send-email-bdwalton@gmail.com> ``` 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 Thanks, I think this simple solution is the best. -Peff ```