{"thread":{"id":"56380","subject":"[PATCH] test-lib-functions: avoid non POSIX ERE in test_dir_is_empty()","startedAt":"2021-08-26T03:18:29Z","lastAt":"2021-08-26T19:21:25Z","messageCount":6,"participants":["Carlo Marcelo Arenas Belón","Taylor Blau","Junio C Hamano","Carlo Arenas"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"433799","messageId":"20210826031710.32980-1-carenas@gmail.com","threadId":"56380","inReplyTo":null,"subject":"[PATCH] test-lib-functions: avoid non POSIX ERE in test_dir_is_empty()","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2021-08-26T03:17:10Z","receivedAt":"2021-08-26T03:18:29Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"0be7d9b73d (test-lib: add test_dir_is_empty(), 2014-06-19) uses an\nERE through the egrep tool (which is not POSIX) using an ? operator\nthat isn't either.\n\nreplace invocation with two equivalent simpler BRE instead.\n\nSigned-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>\n---\n t/test-lib-functions.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex e28411bb75..2803c97df3 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -790,7 +790,7 @@ test_path_exists () {\n test_dir_is_empty () {\n \ttest \"$#\" -ne 1 && BUG \"1 param\"\n \ttest_path_is_dir \"$1\" &&\n-\tif test -n \"$(ls -a1 \"$1\" | egrep -v '^\\.\\.?$')\"\n+\tif test -n \"$(ls -a1 \"$1\" | grep -v '^\\.$' | grep -v '^\\.\\.$')\"\n \tthen\n \t\techo \"Directory '$1' is not empty, it contains:\"\n \t\tls -la \"$1\"\n-- \n2.33.0.481.g26d3bed244\n\n"},{"id":"433803","messageId":"YScXboC0M1IPNFon@nand.local","threadId":"56380","inReplyTo":"20210826031710.32980-1-carenas@gmail.com","subject":"Re: [PATCH] test-lib-functions: avoid non POSIX ERE in test_dir_is_empty()","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-08-26T04:24:14Z","receivedAt":"2021-08-26T04:24:17Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Aug 25, 2021 at 08:17:10PM -0700, Carlo Marcelo Arenas Belón wrote:\n> 0be7d9b73d (test-lib: add test_dir_is_empty(), 2014-06-19) uses an\n> ERE through the egrep tool (which is not POSIX) using an ? operator\n> that isn't either.\n>\n> replace invocation with two equivalent simpler BRE instead.\n>\n> Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>\n> ---\n>  t/test-lib-functions.sh | 2 +-\n>  1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\n> index e28411bb75..2803c97df3 100644\n> --- a/t/test-lib-functions.sh\n> +++ b/t/test-lib-functions.sh\n> @@ -790,7 +790,7 @@ test_path_exists () {\n>  test_dir_is_empty () {\n>  \ttest \"$#\" -ne 1 && BUG \"1 param\"\n>  \ttest_path_is_dir \"$1\" &&\n> -\tif test -n \"$(ls -a1 \"$1\" | egrep -v '^\\.\\.?$')\"\n> +\tif test -n \"$(ls -a1 \"$1\" | grep -v '^\\.$' | grep -v '^\\.\\.$')\"\n\nThis replacement is correct, but I'm not sure that I necessarily find it\nsimpler. If we really are concerned about egrep usage, then\n\n    if test -n \"$(find \"$1\" | grep -v '^\\.$')\"\n\nwould suffice. But it looks like we are fairly OK with egrep in t (`git\ngrep 'egrep' -- t | wc -l` turns up 19 matches), so I'm not sure the\nchange is necessary in the first place.\n\nThanks,\nTaylor\n"},{"id":"433808","messageId":"xmqqlf4ohgdv.fsf@gitster.g","threadId":"56380","inReplyTo":"YScXboC0M1IPNFon@nand.local","subject":"Re: [PATCH] test-lib-functions: avoid non POSIX ERE in test_dir_is_empty()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-08-26T06:25:16Z","receivedAt":"2021-08-26T06:25:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau <me@ttaylorr.com> writes:\n\n>> -\tif test -n \"$(ls -a1 \"$1\" | egrep -v '^\\.\\.?$')\"\n>> +\tif test -n \"$(ls -a1 \"$1\" | grep -v '^\\.$' | grep -v '^\\.\\.$')\"\n>\n> This replacement is correct, but I'm not sure that I necessarily find it\n> simpler. If we really are concerned about egrep usage, then\n>\n>     if test -n \"$(find \"$1\" | grep -v '^\\.$')\"\n>\n> would suffice. But it looks like we are fairly OK with egrep in t (`git\n> grep 'egrep' -- t | wc -l` turns up 19 matches), so I'm not sure the\n> change is necessary in the first place.\n\nIt is true that we have been OK with egrep.\n\n\"grep -E\" is in POSIX (so is \"grep -F\") and that you might be able\nto make an argument to use them instead of \"egrep\" and \"fgrep\", but\nat least at one point in the past, 87539416 (tests: grep portability\nfixes, 2008-09-30) favoured \"fgrep\" over \"grep -F\", claiming that\nthe former was more widely available than the latter.\n\nThe world may have changed since then, though.  IIRC, the 2008's\ntopic was mostly about portability to Solaris and AIX, and their\npeculiarity (or their relevance) may have waned.\n\n"},{"id":"433810","messageId":"CAPUEspjGkHhFNgTe3HnnUvkzwHfKqb9dYO3aCXDh_fRyFMRN6A@mail.gmail.com","threadId":"56380","inReplyTo":"YScXboC0M1IPNFon@nand.local","subject":"Re: [PATCH] test-lib-functions: avoid non POSIX ERE in test_dir_is_empty()","fromName":"Carlo Arenas","fromEmail":"carenas@gmail.com","sentAt":"2021-08-26T06:28:40Z","receivedAt":"2021-08-26T06:28:53Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Wed, Aug 25, 2021 at 9:24 PM Taylor Blau <me@ttaylorr.com> wrote:\n> On Wed, Aug 25, 2021 at 08:17:10PM -0700, Carlo Marcelo Arenas Belón wrote:\n> > diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\n> > index e28411bb75..2803c97df3 100644\n> > --- a/t/test-lib-functions.sh\n> > +++ b/t/test-lib-functions.sh\n> > @@ -790,7 +790,7 @@ test_path_exists () {\n> >  test_dir_is_empty () {\n> >       test \"$#\" -ne 1 && BUG \"1 param\"\n> >       test_path_is_dir \"$1\" &&\n> > -     if test -n \"$(ls -a1 \"$1\" | egrep -v '^\\.\\.?$')\"\n> > +     if test -n \"$(ls -a1 \"$1\" | grep -v '^\\.$' | grep -v '^\\.\\.$')\"\n>\n> This replacement is correct, but I'm not sure that I necessarily find it\n> simpler. If we really are concerned about egrep usage, then\n>\n>     if test -n \"$(find \"$1\" | grep -v '^\\.$')\"\n\nInteresting idea; if having a much simpler expression is so important\nthen we could do instead [^.], since all use cases will be covered\n(nobody is going to create a three dotted file to workaround a test\nIMHO)\n\n> But it looks like we are fairly OK with egrep in t (`git\n> grep 'egrep' -- t | wc -l` turns up 19 matches), so I'm not sure the\n> change is necessary in the first place.\n\negrep (and also fgrep, which we intentionally support because it is\nmissing from some ancient AIX system[1]) will be removed in the\nnext[2] release of GNU grep.\n\nCarlo\n\n[1] 87539416fd (tests: grep portability fixes, 2008-09-30)\n[2] https://git.savannah.gnu.org/cgit/grep.git/commit/?id=a9515624709865d480e3142fd959bccd1c9372d1\n"},{"id":"433813","messageId":"CAPUEspj4F6=c_WPnb==enx6n1wY6LgOXbRm_kZCSnoOf5qcNsg@mail.gmail.com","threadId":"56380","inReplyTo":"xmqqlf4ohgdv.fsf@gitster.g","subject":"Re: [PATCH] test-lib-functions: avoid non POSIX ERE in test_dir_is_empty()","fromName":"Carlo Arenas","fromEmail":"carenas@gmail.com","sentAt":"2021-08-26T07:48:41Z","receivedAt":"2021-08-26T07:48:55Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Wed, Aug 25, 2021 at 11:25 PM Junio C Hamano <gitster@pobox.com> wrote:\n> Taylor Blau <me@ttaylorr.com> writes:\n> >> -    if test -n \"$(ls -a1 \"$1\" | egrep -v '^\\.\\.?$')\"\n> >> +    if test -n \"$(ls -a1 \"$1\" | grep -v '^\\.$' | grep -v '^\\.\\.$')\"\n> >\n> > This replacement is correct, but I'm not sure that I necessarily find it\n> > simpler. If we really are concerned about egrep usage, then\n> >\n> >     if test -n \"$(find \"$1\" | grep -v '^\\.$')\"\n> >\n> > would suffice. But it looks like we are fairly OK with egrep in t (`git\n> > grep 'egrep' -- t | wc -l` turns up 19 matches), so I'm not sure the\n> > change is necessary in the first place.\n>\n> It is true that we have been OK with egrep.\n\nnote that will be an issue at least in a future release of GNU grep\nwhere the deprecated {e,f}grep commands won't be available.\n\n> \"grep -E\" is in POSIX (so is \"grep -F\") and that you might be able\n> to make an argument to use them instead of \"egrep\" and \"fgrep\".\n\nI misread the POSIX specification, and had confirmed that the ?\noperator works fine in BSD and busybox grep so apologies and\nwill see to resubmit this change together with the others that will\nbe required to deal with the deprecated binaries.\n\n> at least at one point in the past, 87539416 (tests: grep portability\n> fixes, 2008-09-30) favoured \"fgrep\" over \"grep -F\", claiming that\n> the former was more widely available than the latter.\n>\n> The world may have changed since then, though.  IIRC, the 2008's\n> topic was mostly about portability to Solaris and AIX, and their\n> peculiarity (or their relevance) may have waned.\n\nwas hoping to deal with fgrep using a function (just like is done with\nperl), but was also hoping that a compatibility layer wouldn't be\nneeded.\ntesting on AIX (that seems to stick around) will definitely be needed.\n\nCarlo\n"},{"id":"433863","messageId":"xmqqfsuwdnbk.fsf@gitster.g","threadId":"56380","inReplyTo":"CAPUEspjGkHhFNgTe3HnnUvkzwHfKqb9dYO3aCXDh_fRyFMRN6A@mail.gmail.com","subject":"Re: [PATCH] test-lib-functions: avoid non POSIX ERE in test_dir_is_empty()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-08-26T19:21:19Z","receivedAt":"2021-08-26T19:21:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Carlo Arenas <carenas@gmail.com> writes:\n\n> egrep (and also fgrep, which we intentionally support because it is\n> missing from some ancient AIX system[1]) will be removed in the\n> next[2] release of GNU grep.\n\nIt is not a reason not to nudge us to prepare for the eventual\nremoval of these two commands, but we need to keep the facts\nstraight in our log messages.  \n\nThe way I read [2], they will only start giving a warning message\nnudging the users to use \"grep -[EF]\", and for them to be able to\nwarn, I would imagine they have to stay in the release without\ngetting removed.\n\n> [1] 87539416fd (tests: grep portability fixes, 2008-09-30)\n> [2] https://git.savannah.gnu.org/cgit/grep.git/commit/?id=a9515624709865d480e3142fd959bccd1c9372d1\n\nThere are about 35 places in t/ we call egrep or fgrep; if we can\nadd a pair of replacement shell functions in t/test-lib.sh for them\nto use whatever command GIT_TEST_EGREP and GIT_TEST_FGREP\nenvironment variables specify and fall back on \"grep -E/-F\"\notherwise, we can prepare for the change without too much code\nchurning.\n\nSomething along the lines of\n\n\t# in test-lib-functions.sh\n        : ${GIT_TEST_EGREP:=grep -E} ${GIT_TEST_FGREP:=grep -F}\n        egrep () { $GIT_TEST_EGREP \"$@\" }\n        fgrep () { $GIT_TEST_FGREP \"$@\" }\n\nwhere people could do something silly like\n\n\tGIT_TEST_FGREP='command fgrep' \\\n\tGIT_TEST_EGREP='command egrep' \\\n\tmake test\n\nperhaps?\n"}]}