From: "Torsten Bögershausen" <tboegi@web.de>
To: Jonathan Nieder <jrnieder@gmail.com>
Cc: "Torsten Bögershausen" <tboegi@web.de>,
"Junio C Hamano" <gitster@pobox.com>,
git@vger.kernel.org, kraai@ftbfs.org
Subject: Re: [PATCH] tests: turn on test-lint-shell-syntax by default
Date: Sun, 27 Jan 2013 14:13:46 +0100 [thread overview]
Message-ID: <5105280A.80002@web.de> (raw)
In-Reply-To: <20130127093121.GA4228@elie.Belkin>
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 <tboegi@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
next prev parent reply other threads:[~2013-01-27 13:14 UTC|newest]
Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-01-12 5:50 [PATCH] tests: turn on test-lint-shell-syntax by default Torsten Bögershausen
2013-01-12 6:00 ` Junio C Hamano
2013-01-13 10:25 ` Torsten Bögershausen
2013-01-13 16:50 ` Matt Kraai
2013-01-13 17:32 ` Jonathan Nieder
2013-01-13 22:38 ` Junio C Hamano
2013-01-15 20:12 ` Torsten Bögershausen
2013-01-15 20:38 ` Junio C Hamano
2013-01-26 6:57 ` Torsten Bögershausen
2013-01-26 21:43 ` Junio C Hamano
2013-01-27 7:43 ` Torsten Bögershausen
2013-01-27 9:31 ` Jonathan Nieder
2013-01-27 13:13 ` Torsten Bögershausen [this message]
2013-01-27 17:34 ` Junio C Hamano
2013-01-27 20:25 ` Junio C Hamano
2013-02-05 20:36 ` Torsten Bögershausen
2013-02-05 20:52 ` Junio C Hamano
2013-02-05 21:56 ` Junio C Hamano
2013-01-27 17:15 ` Junio C Hamano
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=5105280A.80002@web.de \
--to=tboegi@web.de \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=jrnieder@gmail.com \
--cc=kraai@ftbfs.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
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 read-only IMAP folder(s) and NNTP newsgroup(s).