* t9401 fails with OS X sed
@ 2012-10-25 3:54 Brian Gernhardt
2012-10-25 5:04 ` Geert Bosch
0 siblings, 1 reply; 10+ messages in thread
From: Brian Gernhardt @ 2012-10-25 3:54 UTC (permalink / raw)
To: Git List
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
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: t9401 fails with OS X sed 2012-10-25 3:54 t9401 fails with OS X sed Brian Gernhardt @ 2012-10-25 5:04 ` Geert Bosch 2012-10-25 8:41 ` Jeff King 0 siblings, 1 reply; 10+ messages in thread From: Geert Bosch @ 2012-10-25 5:04 UTC (permalink / raw) To: Brian Gernhardt; +Cc: Git List 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: t9401 fails with OS X sed 2012-10-25 5:04 ` Geert Bosch @ 2012-10-25 8:41 ` Jeff King 2012-10-25 12:51 ` Torsten Bögershausen 0 siblings, 1 reply; 10+ messages in thread From: Jeff King @ 2012-10-25 8:41 UTC (permalink / raw) To: Geert Bosch; +Cc: Brian Gernhardt, Git List 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: t9401 fails with OS X sed 2012-10-25 8:41 ` Jeff King @ 2012-10-25 12:51 ` Torsten Bögershausen 2012-10-25 15:58 ` [PATCH] Use character class for sed expression instead of \s Ben Walton 0 siblings, 1 reply; 10+ messages in thread From: Torsten Bögershausen @ 2012-10-25 12:51 UTC (permalink / raw) To: Jeff King Cc: Geert Bosch, Brian Gernhardt, Git List, 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)" ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH] Use character class for sed expression instead of \s 2012-10-25 12:51 ` Torsten Bögershausen @ 2012-10-25 15:58 ` Ben Walton 2012-10-25 16:00 ` Brian Gernhardt 2012-10-26 12:38 ` Jeff King 0 siblings, 2 replies; 10+ messages in thread From: Ben Walton @ 2012-10-25 15:58 UTC (permalink / raw) To: tboegi, peff, bosch, brian, git; +Cc: 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 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] Use character class for sed expression instead of \s 2012-10-25 15:58 ` [PATCH] Use character class for sed expression instead of \s Ben Walton @ 2012-10-25 16:00 ` Brian Gernhardt 2012-10-25 16:28 ` Torsten Bögershausen 2012-10-26 12:38 ` Jeff King 1 sibling, 1 reply; 10+ messages in thread From: Brian Gernhardt @ 2012-10-25 16:00 UTC (permalink / raw) To: Ben Walton; +Cc: tboegi, peff, bosch, git 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] Use character class for sed expression instead of \s 2012-10-25 16:00 ` Brian Gernhardt @ 2012-10-25 16:28 ` Torsten Bögershausen 2012-10-25 18:08 ` Ben Walton 0 siblings, 1 reply; 10+ messages in thread From: Torsten Bögershausen @ 2012-10-25 16:28 UTC (permalink / raw) To: Brian Gernhardt; +Cc: Ben Walton, tboegi, peff, bosch, git 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 ) && ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] Use character class for sed expression instead of \s 2012-10-25 16:28 ` Torsten Bögershausen @ 2012-10-25 18:08 ` Ben Walton 2012-10-25 20:09 ` Torsten Bögershausen 0 siblings, 1 reply; 10+ messages in thread From: Ben Walton @ 2012-10-25 18:08 UTC (permalink / raw) To: Torsten Bögershausen; +Cc: Brian Gernhardt, peff, bosch, git 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 --------------------------------------------------------------------------------------------------------------------------- ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] Use character class for sed expression instead of \s 2012-10-25 18:08 ` Ben Walton @ 2012-10-25 20:09 ` Torsten Bögershausen 0 siblings, 0 replies; 10+ messages in thread From: Torsten Bögershausen @ 2012-10-25 20:09 UTC (permalink / raw) To: Ben Walton; +Cc: Torsten Bögershausen, Brian Gernhardt, peff, bosch, git 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 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] Use character class for sed expression instead of \s 2012-10-25 15:58 ` [PATCH] Use character class for sed expression instead of \s Ben Walton 2012-10-25 16:00 ` Brian Gernhardt @ 2012-10-26 12:38 ` Jeff King 1 sibling, 0 replies; 10+ messages in thread From: Jeff King @ 2012-10-26 12:38 UTC (permalink / raw) To: Ben Walton; +Cc: tboegi, bosch, brian, git 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2012-10-26 12:39 UTC | newest] Thread overview: 10+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2012-10-25 3:54 t9401 fails with OS X sed Brian Gernhardt 2012-10-25 5:04 ` Geert Bosch 2012-10-25 8:41 ` Jeff King 2012-10-25 12:51 ` Torsten Bögershausen 2012-10-25 15:58 ` [PATCH] Use character class for sed expression instead of \s Ben Walton 2012-10-25 16:00 ` Brian Gernhardt 2012-10-25 16:28 ` Torsten Bögershausen 2012-10-25 18:08 ` Ben Walton 2012-10-25 20:09 ` Torsten Bögershausen 2012-10-26 12:38 ` Jeff King
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).