Guillaume Sasdy <guillaume.sasdy@xxxxxxxxxxxxxxx> writes:
> # CONFIGURATION VARIABLES
> -# You might want to change those ones ...
> +# You might want to change these ones
> #
> WIKI_DIR_NAME="wiki" # Name of the wiki's directory
> WIKI_DIR_INST="/var/www" # Directory of the web server
> TMP="/tmp" # Temporary directory for downloads
> - # Absolute address needed!
> + # Absolute path required!
> SERVER_ADDR="localhost" # Web server's address
>
> -#
> # CONFIGURATION
> -# You should not change those ones unless you know what you to
> +# You should not change these ones unless you know what you do
I already mentionned in in your v1, but these fixups do not belong to
PATCH 2/3. You do not want reviewers to see your mistakes in PATCH 1/2
and see the fix in PATCH 2/3.
> +wiki_getpage () {
> + ../test-gitmw.pl get_page -p "$1" "$2"
> +}
Any reason why test-gitmw.pl and wiki_getpage have this slightly
different API? The perl version has a "-p" flag, and the shell command
has only positionnal arguments.
I'd rather have a more uniform way to wrap calls to test-gitmw.pl in
shell, like
wiki_<something> () {
../test-gitmw.pl <something> "$@"
}
Then, you probably want to move the API documentation (i.e. comments you
put before the shell functions) in, or next to the Perl script, and
avoid repeating it in the shell.
> +# git_content <dir_1> <dir_2>
> +#
> +# Compare the contents of directories <dir_1> and <dir_2> with diff
> +# and exits with error 1 if they do not match. The program will
> +# not look into .git in the process.
> +git_content () {
Didn't I already say that the naming was strange? A function that
compares something shouldn't be called just "content".
> + result=$(diff -r -b --exclude=".git" "$1" "$2")
> +
> + if echo $result | grep -q ">" ; then
Why grep, when the exit status of diff tells you about the differences
already?
> +# git_exist <rep_name> <file_name>
> +#
> +# Check the existence of <file_name> into the git repository
> +# <rep_name> and exits with error 1 if it is absent.
> +git_exist () {
> + result=$(find "$1" -type f -name "$2")
> +
> + if ! echo $result | grep -q "$2" ; then
Missing quotes around $result.
Why do you need grep again? You just want to check whether "$result" is
empty (test -z).
> + echo "test failed: file $2 does not exist in $1"
> + exit 1
die
?
> +wiki_page_exist () {
> + wiki_getpage "$1" .
> +
> + if test -f "$1".mw ; then
> + echo "test failed: file $1 not found on wiki"
> + exit 1
likewise.
> +fail()
> +{
Style: fail () {
Plus, didn't we say "die" was already there for this?
> + # Replace spaces by underscore in the page name
> + $pagename=~s/\ /_/;
Indent with space (didn't I already mention that?).
> +my $login = $ARGV[0];
> +
> +if ($login eq "-p")
Feels weird. If you're not sure it's a login, why call the variable
$login?
Any reason not to use Perl's option parsing?
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html