Re: [PATCH] tests: turn on test-lint-shell-syntax by default

2013-02-05 Thread Torsten Bögershausen 
On 27.01.13 18:34, Junio C Hamano wrote:
 Torsten Bögershausen tbo...@web.de writes:

 Back to the which:
 ...
 and running make test gives the following, at least in my system:
 ...
 I think everybody involved in this discussion already knows that;
 the point is that it can easily give false negative, without the
 scripts working very hard to do so.

 If we did not care about incurring runtime performance cost, we
 could arrange:

  - the test framework to define a variable $TEST_ABORT that has a
full path to a file that is in somewhere test authors cannot
touch unless they really try hard to (i.e. preferrably outside
$TRASH_DIRECTORY, as it is not uncommon for to tests to do rm *
there). This location should be per $(basename $0 .sh) to allow
running multiple tests in paralell;

  - the test framework to rm -f $TEST_ABORT at the beginning of
test_expect_success/failure;

  - test_expect_success/failure to check $TEST_ABORT and if it
exists, abort the execution, showing the contents of the file as
an error message.

 Then you can wrap commands whose use we want to limit, perhaps like
 this, in the test framework:

   which () {
   cat $TEST_ABORT -\EOF
   Do not use unportable 'which' in the test script.
 if type $cmd is a good way to see if $cmd exists.
   EOF
   }

   sed () {
   saw_wantarg= must_abort=
 for arg
 do
   if test -n $saw_wantarg
 then
   saw_wantarg=
 continue
   fi
   case $arg in
   --) break ;; # end of options
   -i) echo $TEST_ABORT Do not use 'sed -i'
   must_abort=yes
   break ;;
 -e)   saw_wantarg=yes ;; # skip next arg
   -*) continue ;; # options without arg
   *)  break ;; # filename
   esac
   done
   if test -z $must_abort
   sed $@
   fi
   }

 Then you can check that TEST_ABORT does not appear in test scripts
 (ensuring that they do not attempt to circumvent the mechanis) and
 catch use of unwanted commands or unwanted extended features of
 commands at runtime.

 But this will incur runtime performace hit, so I am not sure it
 would be worth it.
Thanks for the detailed suggestion.
Instead of using a file for putting out non portable syntax,
can we can use a similar logic as test_failure ?

I did some benchmarking, the test suite on a Laptop is 37..38 minutes,
including make clean  make both on next, pu, master or with the patch below.
I couldn't find a measurable impact on the execution time.
What do we think about a patch like this
(Not sure if this cut-and-paste data applies, it's for review)


[PATCH] test-lib: which, echo -n and sed -i are not portable

The posix version of sed command supports options -n -e -f
The gnu extension -i to edit a file in place is not
available on all systems.
To catch the usage of non-posix options with sed a
wrapper function is added in test-lib.sh.
The wrapper checks that only -n -e -f are used.
The short form -ne for -n -e is allowed as well.

echo -n is not portable and not available on all systems,
printf can be used instead.
Add a wrapper to catch echo -n

which is not portable, the output differs between different
implementations, and the return code may not be reliable.

Add a function test_bad_syntax_ in test-lib.sh, which increments
the variable test_bad_syntax and works similar to test_failure_

Signed-off-by: Torsten Bögershausen tbo...@web.de
---
 t/test-lib.sh | 46 ++
 1 file changed, 46 insertions(+)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index 1a6c4ab..248ed34 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -266,6 +266,7 @@ else
 exec 4/dev/null 3/dev/null
 fi
 
+test_bad_syntax=0
 test_failure=0
 test_count=0
 test_fixed=0
@@ -300,6 +301,12 @@ test_ok_ () {
 say_color  ok $test_count - $@
 }
 
+test_bad_syntax_ () {
+test_bad_syntax=$(($test_bad_syntax + 1))
+say_color error $@
+test $immediate =  || { GIT_EXIT_OK=t; exit 1; }
+}
+
 test_failure_ () {
 test_failure=$(($test_failure + 1))
 say_color error not ok $test_count - $1
@@ -402,10 +409,15 @@ test_done () {
 fixed $test_fixed
 broken $test_broken
 failed $test_failure
+bad_syntax $test_bad_syntax
 
 EOF
 fi
 
+if test $test_bad_syntax != 0
+then
+say_color error # $test_bad_syntax non portable shell syntax
+fi
 if test $test_fixed != 0
 then
 say_color error # $test_fixed known breakage(s) vanished; please 
update test(s)
@@ -645,6 +657,40 @@ yes () {
 done
 }
 
+
+# which is

Re: [PATCH] tests: turn on test-lint-shell-syntax by default

2013-02-05 Thread Junio C Hamano 
Torsten Bögershausen tbo...@web.de writes:

 Thanks for the detailed suggestion.
 Instead of using a file for putting out non portable syntax,
 can we can use a similar logic as test_failure ?

Your test_bad_syntax_ function can be called from a subshell, and
its exit 1 will not exit, no?

test_expect_success 'prepare a blob with incomplete line' '
(
echo first line
echo second line
echo -n final and incomplete line
) incomplete.txt
'

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] tests: turn on test-lint-shell-syntax by default

2013-02-05 Thread Junio C Hamano 
Junio C Hamano gits...@pobox.com writes:

 Torsten Bögershausen tbo...@web.de writes:

 Thanks for the detailed suggestion.
 Instead of using a file for putting out non portable syntax,
 can we can use a similar logic as test_failure ?

 Your test_bad_syntax_ function can be called from a subshell, and
 its exit 1 will not exit, no?

What is more important is that the increment to $test_bad_syntax
done in that function will not be propagated up to the main process
that runs the test framework.

Of course, that is why I mentioned communicating using the
filesystem.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] tests: turn on test-lint-shell-syntax by default

2013-01-27 Thread Jonathan Nieder 
Hi,

Torsten Bögershausen wrote:
 On 15.01.13 21:38, Junio C Hamano wrote:
 Torsten Bögershausen tbo...@web.de writes:

 What do we think about something like this for fishing for which:
[...]
 +which () {
 +   echo 2 which is not portable (please use type)
 +   exit 1
 +}
[...]
  if (
  which frotz 
 test $(frobonitz --version -le 2.0
 )

With the above definition of which, the only sign of a mistake would
be some extra output to stderr (which is quelled when running tests in
the normal way).  The exit is caught by the subshell and just makes
the if condition false.

That's not so terrible --- it could still dissuade new test authors
from using which.  The downside I'd worry about is that it provides
a false sense of security despite not catching problems like

write_script x -EOF 
# Use foo if possible.  Otherwise use bar.
if which foo  test $(foo --version) -le 2.0
then
...
...
EOF
./x

That's not a great tradeoff relative to the impact of the problem
being solved.

Don't get me wrong.  I really do want to see more static or dynamic
analysis of git's shell scripts in the future.  I fear that for the
tradeoffs to make sense, though, the analysis needs to be more
sophisticated:

 * A very common error in test scripts is leaving out the 
   connecting adjacent statements, which causes early errors
   in a test assertion to be missed and tests to pass by mistake.
   Unfortunately the grammar of the dialect of shell used in tests is
   not regular enough to make this easily detectable using regexps.

 * Another common mistake is using cd without entering a subshell.
   Detecting this requires counting nested parentheses and noticing
   when a parenthesis is quoted.

 * Another common mistake is relying on the semantics of variable
   assignments in front of function calls.  Detecting this requires
   recognizing which commands are function calls.

In the end the analysis that works best would probably involve a
full-fledged shell script parser.  Something like sparse, except for
shell command language.

Sorry I don't have more practical advice in the short term.

My two cents,
Jonathan
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] tests: turn on test-lint-shell-syntax by default

2013-01-27 Thread Torsten Bögershausen 
On 27.01.13 10:31, Jonathan Nieder wrote:
 Hi,
 
 Torsten Bögershausen wrote:
 On 15.01.13 21:38, Junio C Hamano wrote:
 Torsten Bögershausen tbo...@web.de writes:
 
 What do we think about something like this for fishing for which:
 [...]
 +which () {
 +   echo 2 which is not portable (please use type)
 +   exit 1
 +}
 [...]
 if (
 which frotz 
 test $(frobonitz --version -le 2.0
)
 
 With the above definition of which, the only sign of a mistake would
 be some extra output to stderr (which is quelled when running tests in
 the normal way).  The exit is caught by the subshell and just makes
 the if condition false.
 
 That's not so terrible --- it could still dissuade new test authors
 from using which.  The downside I'd worry about is that it provides
 a false sense of security despite not catching problems like
 
   write_script x -EOF 
   # Use foo if possible.  Otherwise use bar.
   if which foo  test $(foo --version) -le 2.0
   then
   ...
   ...
   EOF
   ./x
 
 That's not a great tradeoff relative to the impact of the problem
 being solved.
 
 Don't get me wrong.  I really do want to see more static or dynamic
 analysis of git's shell scripts in the future.  I fear that for the
 tradeoffs to make sense, though, the analysis needs to be more
 sophisticated:
 
  * A very common error in test scripts is leaving out the 
connecting adjacent statements, which causes early errors
in a test assertion to be missed and tests to pass by mistake.
Unfortunately the grammar of the dialect of shell used in tests is
not regular enough to make this easily detectable using regexps.
 
  * Another common mistake is using cd without entering a subshell.
Detecting this requires counting nested parentheses and noticing
when a parenthesis is quoted.
 
  * Another common mistake is relying on the semantics of variable
assignments in front of function calls.  Detecting this requires
recognizing which commands are function calls.
 
 In the end the analysis that works best would probably involve a
 full-fledged shell script parser.  Something like sparse, except for
 shell command language.
 
 Sorry I don't have more practical advice in the short term.
 
 My two cents,
 Jonathan

Jonathan,
thanks for the review.

My ambition is to get the check-non-portable-shell.pl into a shape
that we can enable it by default.

This may be with or without checking for which.
As a first step we will hopefully see less breakage for e.g. Mac OS
caused by echo -n or sed -i usage.

On the longer run, we may be able to have something more advanced.

Back to the which:

Writing a t0009-no-which.sh like this:

#!/bin/sh
test_description='Test the which'

. ./test-lib.sh

which () {
   echo 2 which is not portable (please use type)
   exit 1
}

test_expect_success 'which is not portable' '
if  which frotz
then
say frotz does not exist
else
say frotz does exist
fi

'
test_done
--
and running make test gives the following, at least in my system:

[snip]

*** t0009-no-which.sh ***
FATAL: Unexpected exit with code 1
make[2]: *** [t0009-no-which.sh] Error 1
make[1]: *** [test] Error 2
make: *** [test] Error 2

---
running inside t/
./t0009-no-which.sh --verbose

Initialized empty Git repository in /Users/tb/projects/git/tb/t/trash 
directory.t0009-no-which/.git/
expecting success: 
if  which frotz
then
say frotz does not exist
else
say frotz does exist
fi


which is not portable (please use type)
FATAL: Unexpected exit with code 1

/Torsten








--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] tests: turn on test-lint-shell-syntax by default

2013-01-27 Thread Junio C Hamano 
Jonathan Nieder jrnie...@gmail.com writes:

 ...
 With the above definition of which, the only sign of a mistake would
 be some extra output to stderr (which is quelled when running tests in
 the normal way).  The exit is caught by the subshell and just makes
 the if condition false.

 That's not so terrible --- it could still dissuade new test authors
 from using which.  The downside I'd worry about is that it provides
 a false sense of security despite not catching problems ...
 ...
 In the end the analysis that works best would probably involve a
 full-fledged shell script parser.  Something like sparse, except for
 shell command language.

Exactly.

That is why I keep saying that whole test-lint-shell-syntax should
stay outside the default until it gets more robust by becoming a
reasonable shell parser; it may not necessarily have to become
full parser though.

As we discourage the use of tricky features of the language like
eval in individual test scripts to implement their own mini test
framework, the something like sparse parser may initialy start
small and still be useful; for example it can learn to exclude
anything inside HERE_DOCUMENT from getting inspected.

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] tests: turn on test-lint-shell-syntax by default

2013-01-27 Thread Junio C Hamano 
Torsten Bögershausen tbo...@web.de writes:

 Back to the which:
 ...
 and running make test gives the following, at least in my system:
 ...

I think everybody involved in this discussion already knows that;
the point is that it can easily give false negative, without the
scripts working very hard to do so.

If we did not care about incurring runtime performance cost, we
could arrange:

 - the test framework to define a variable $TEST_ABORT that has a
   full path to a file that is in somewhere test authors cannot
   touch unless they really try hard to (i.e. preferrably outside
   $TRASH_DIRECTORY, as it is not uncommon for to tests to do rm *
   there). This location should be per $(basename $0 .sh) to allow
   running multiple tests in paralell;

 - the test framework to rm -f $TEST_ABORT at the beginning of
   test_expect_success/failure;

 - test_expect_success/failure to check $TEST_ABORT and if it
   exists, abort the execution, showing the contents of the file as
   an error message.

Then you can wrap commands whose use we want to limit, perhaps like
this, in the test framework:

which () {
cat $TEST_ABORT -\EOF
Do not use unportable 'which' in the test script.
if type $cmd is a good way to see if $cmd exists.
EOF
}

sed () {
saw_wantarg= must_abort=
for arg
do
if test -n $saw_wantarg
then
saw_wantarg=
continue
fi
case $arg in
--) break ;; # end of options
-i) echo $TEST_ABORT Do not use 'sed -i'
must_abort=yes
break ;;
-e) saw_wantarg=yes ;; # skip next arg
-*) continue ;; # options without arg
*)  break ;; # filename
esac
done
if test -z $must_abort
sed $@
fi
}

Then you can check that TEST_ABORT does not appear in test scripts
(ensuring that they do not attempt to circumvent the mechanis) and
catch use of unwanted commands or unwanted extended features of
commands at runtime.

But this will incur runtime performace hit, so I am not sure it
would be worth it.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] tests: turn on test-lint-shell-syntax by default

2013-01-27 Thread Junio C Hamano 
Junio C Hamano gits...@pobox.com writes:

 If we did not care about incurring runtime performance cost, we
 could arrange:
 ...
 Then you can wrap commands whose use we want to limit, perhaps like
 this, in the test framework:
 ...
   sed () {
 ...
   done
   if test -z $must_abort
   sed $@
   fi
   }

Of course, aside from missing then, this needs to use the
real sed, so this has to be

if test -z $must_abort
then
command sed $@
fi

or something like that.

An approach along this line may reduce both the false negatives and
false positives down to an acceptable level, but I doubt the result
would be efficient enough for us to tolerate the runtime penalty.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] tests: turn on test-lint-shell-syntax by default

2013-01-26 Thread Junio C Hamano 
Torsten Bögershausen tbo...@web.de writes:

 Do we really need  which to detect if frotz is installed?

I think we all know the answer to that question is no, but why is
that a relevant question in the context of this discussion?  One of
us may be very confused.

I thought the topic of this discussion was that, already knowing
that which should never be used anywhere in our scripts, you are
trying to devise a mechanical way to catch newcomers' attempts to
use it in their changes, in order to prevent patches that add use of
which to be sent for review to waste our time.  I was illustrating
that the approach to override which in a shell function for test
scripts will not be a useful solution for that goal.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] tests: turn on test-lint-shell-syntax by default

2013-01-25 Thread Torsten Bögershausen 
On 15.01.13 21:38, Junio C Hamano wrote:
 Torsten Bögershausen tbo...@web.de writes:
 
 What do we think about something like this for fishing for which:

 --- a/t/test-lib.sh
 +++ b/t/test-lib.sh
 @@ -644,6 +644,10 @@ yes () {
 :
 done
  }
 +which () {
 +   echo 2 which is not portable (please use type)
 +   exit 1
 +}


 This will happen in runtime, which might be good enough ?
 
   if (
   which frotz 
 test $(frobonitz --version -le 2.0
  )
 then
   test_set_prereq FROTZ_FROBONITZ
   else
   echo 2 suitable Frotz/Frobonitz combo not available;
 echo 2 some tests may be skipped
   fi
 
 I somehow think this is a lost cause.

I found different ways to detect if frotz is installed in the test suite:
a) use type(Should be the fastest ?)
b) call the command directly, check the exit code
c) ! test_have_prereq (easy to understand, propably most expensive ?)

Do we really need  which to detect if frotz is installed?


/Torsten



=
if ! type cvs /dev/null 21
then
skip_all='skipping cvsimport tests, cvs not found'
test_done
fi

===
if test -n $BASH  test -z $POSIXLY_CORRECT; then
# we are in full-on bash mode
true
elif type bash /dev/null 21; then
# execute in full-on bash mode
unset POSIXLY_CORRECT
exec bash $0 $@
else
echo '1..0 #SKIP skipping bash completion tests; bash not available'
exit 0
fi
===
svn /dev/null 21
if test $? -ne 1
then
skip_all='skipping git svn tests, svn not found'
test_done
fi
===
( p4 -h  p4d -h ) /dev/null 21 || {
skip_all='skipping git p4 tests; no p4 or p4d'
test_done
}
===
if ! test_have_prereq PERL; then
skip_all='skipping gitweb tests, perl not available'
test_done
fi



--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] tests: turn on test-lint-shell-syntax by default

2013-01-15 Thread Torsten Bögershausen 
On 13.01.13 23:38, Junio C Hamano wrote:
 Jonathan Nieder jrnie...@gmail.com writes:
 
 Hi,

 Torsten Bögershausen wrote:

 -   /^\s*[^#]\s*which\s/ and err 'which is not portable (please use type)';
 +   /^\s*[^#]\s*which\s+[-a-zA-Z0-9]+$/ and err 'which is not portable 
 (please use type)';

 Hmm.  Neither the old version nor the new one matches what seem to
 be typical uses of 'which', based on a quick code search:

  if which sl /dev/null 21
  then
  sl -l
  ...
  fi

 or

  if test -x $(which sl 2/dev/null)
  then
  sl -l
  ...
  fi
 
 Yes, these two misuses are what we want it to trigger on, so the
 test is very easy to trigger and produce a false positive, but does
 not trigger on what we really want to catch.
 
 That does not sound like a good benefit/cost ratio to me.
 
Thanks for comments, I think writing a regexp for which is difficult.
What do we think about something like this for fishing for which:

--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -644,6 +644,10 @@ yes () {
:
done
 }
+which () {
+   echo 2 which is not portable (please use type)
+   exit 1
+}


This will happen in runtime, which might be good enough ?


@Matt:
The [^#] appears to ensure that there's at least one character
before the which and that it's not a pound sign.  Why is this done?
This is simply wrong.



--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] tests: turn on test-lint-shell-syntax by default

2013-01-15 Thread Junio C Hamano 
Torsten Bögershausen tbo...@web.de writes:

 What do we think about something like this for fishing for which:

 --- a/t/test-lib.sh
 +++ b/t/test-lib.sh
 @@ -644,6 +644,10 @@ yes () {
 :
 done
  }
 +which () {
 +   echo 2 which is not portable (please use type)
 +   exit 1
 +}


 This will happen in runtime, which might be good enough ?

if (
which frotz 
test $(frobonitz --version -le 2.0
   )
then
test_set_prereq FROTZ_FROBONITZ
else
echo 2 suitable Frotz/Frobonitz combo not available;
echo 2 some tests may be skipped
fi

I somehow think this is a lost cause.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] tests: turn on test-lint-shell-syntax by default

2013-01-13 Thread Torsten Bögershausen 
On 12.01.13 07:00, Junio C Hamano wrote:
 Torsten Bögershausen tbo...@web.de writes:
 
 The test Makefile has a default set of lint tests which are run
 as part of make test.

 The macro TEST_LINT defaults to test-lint-duplicates test-lint-executable.

 Add test-lint-shell-syntax here, to detect non-portable shell syntax early.

 Signed-off-by: Torsten Bögershausen tbo...@web.de
 ---
 
 As I said already, I do not want to do this yet without further
 reduction of false positives.
Which reinds me that the expression fishing for which is really poor.

How about something like the following:

-- 8 --
Subject: [PATCH] Reduce false positive in check-non-portable-shell.pl

check-non-portable-shell.pl is using simple regular expressions to
find illegal shell syntax.
Improve the expressions and reduce the chance for false positves:

sed -i must be followed by 1..n whitespace and 1 non whitespace
declare must be followed by 1..n whitespace and 1 non whitespace
echo -n must be followed by 1..n whitespace and 1 non whitespace
which must be followed by 1..n whitespace, a string, end of line



diff --git a/t/check-non-portable-shell.pl b/t/check-non-portable-shell.pl
index 8b5a71d..7151dd6 100755
--- a/t/check-non-portable-shell.pl
+++ b/t/check-non-portable-shell.pl
@@ -16,10 +16,10 @@ sub err {
 
 while () {
chomp;
-   /^\s*sed\s+-i/ and err 'sed -i is not portable';
-   /^\s*echo\s+-n/ and err 'echo -n is not portable (please use printf)';
-   /^\s*declare\s+/ and err 'arrays/declare not portable';
-   /^\s*[^#]\s*which\s/ and err 'which is not portable (please use type)';
+   /^\s*sed\s+-i\s+\S/ and err 'sed -i is not portable';
+   /^\s*echo\s+-n\s+\S/ and err 'echo -n is not portable (please use 
printf)';
+   /^\s*declare\s+\S/ and err 'arrays/declare not portable';
+   /^\s*[^#]\s*which\s+[-a-zA-Z0-9]+$/ and err 'which is not portable 
(please use type)';
/test\s+[^=]*==/ and err 'test a == b is not portable (please use =)';
# this resets our $. for each file
close ARGV if eof;

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] tests: turn on test-lint-shell-syntax by default

2013-01-13 Thread Matt Kraai 
On Sun, Jan 13, 2013 at 11:25:57AM +0100, Torsten Bögershausen wrote:
 @@ -16,10 +16,10 @@ sub err {
  
  while () {
   chomp;
 - /^\s*sed\s+-i/ and err 'sed -i is not portable';
 - /^\s*echo\s+-n/ and err 'echo -n is not portable (please use printf)';
 - /^\s*declare\s+/ and err 'arrays/declare not portable';
 - /^\s*[^#]\s*which\s/ and err 'which is not portable (please use type)';
 + /^\s*sed\s+-i\s+\S/ and err 'sed -i is not portable';
 + /^\s*echo\s+-n\s+\S/ and err 'echo -n is not portable (please use 
 printf)';
 + /^\s*declare\s+\S/ and err 'arrays/declare not portable';
 + /^\s*[^#]\s*which\s+[-a-zA-Z0-9]+$/ and err 'which is not portable 
 (please use type)';

The [^#] appears to ensure that there's at least one character
before the which and that it's not a pound sign.  Why is this done?
Why isn't it done for the other commands?
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] tests: turn on test-lint-shell-syntax by default

2013-01-11 Thread Junio C Hamano 
Torsten Bögershausen tbo...@web.de writes:

 The test Makefile has a default set of lint tests which are run
 as part of make test.

 The macro TEST_LINT defaults to test-lint-duplicates test-lint-executable.

 Add test-lint-shell-syntax here, to detect non-portable shell syntax early.

 Signed-off-by: Torsten Bögershausen tbo...@web.de
 ---

As I said already, I do not want to do this yet without further
reduction of false positives.

Addition of the shell script test was a good starting point, but as
it stands, it still is too brittle and will trigger on something
even trivially innouous, like this:

test_expect_success 'no issues' '
cat test.file -\EOF 
which is the right way?
EOF
'

  t/Makefile | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

 diff --git a/t/Makefile b/t/Makefile
 index 1923cc1..6fa2b80 100644
 --- a/t/Makefile
 +++ b/t/Makefile
 @@ -13,7 +13,7 @@ TAR ?= $(TAR)
  RM ?= rm -f
  PROVE ?= prove
  DEFAULT_TEST_TARGET ?= test
 -TEST_LINT ?= test-lint-duplicates test-lint-executable
 +TEST_LINT ?= test-lint-duplicates test-lint-executable test-lint-shell-syntax
  
  # Shell quote;
  SHELL_PATH_SQ = $(subst ','\'',$(SHELL_PATH))
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html