Re: [PATCH 2/3] Test environment of git-remote-mw

From: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
Date: Jun 1, 2012, 11:49 UTC
Message-ID: <vpqmx4n9rq6.fsf@bauges.imag.fr>
In-Reply-To: <1338547317-26088-2-git-send-email-guillaume.sasdy@ensimag.imag.fr>

Guillaume Sasdy <guillaume.sasdy@ensimag.imag.fr> writes:

Show 15 quoted lines

>  # 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.

Show 6 quoted lines

> +# 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?

Show 8 quoted lines

> +# 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

Show 6 quoted lines

> +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/