From: Simon.Cathebras <simon.cathebras@ensimag.imag.fr>
Date: Fri, 01 Jun 2012 14:43:13 GMT
Subject: Re: [PATCH 2/3] Test environment of git-remote-mw
Message-ID: <4FC8D501.20207@ensimag.imag.fr>
In-Reply-To: <vpqmx4n9rq6.fsf@bauges.imag.fr>

On 01/06/2012 13:49, Matthieu Moy wrote:
> Guillaume Sasdy<guillaume.sasdy@ensimag.imag.fr>  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.
Got it. With the problem experienced yesterday, we had still this issue 
this morning. It is now fixed :).
>> +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.
The "-p" flag exists to specify if we have to use the admin login on 
wiki to do the command. For instance, here we fetch a page from the wiki 
with Admin privilege.
Others arguments remains the same.
> I'd rather have a more uniform way to wrap calls to test-gitmw.pl in
> shell, like
>
> wiki_<something>  () {
> 	../test-gitmw.pl<something>  "$@"
> }
Do you suggest we include the "-p" flag in <something> ?
I agree for the use of "$@".
>
> 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.
>
Ok.
>> +# 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".
Sorry, we misunderstood what you mean... Update is coming. What about 
git_diff_directories ?
Because this shell function execute a special instance of diff, matching 
two directories and ignoring blank character.
>> +	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?
>
Good idea for the exit status.
>> +# 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).
this function is now erased from our code... Actually, it was really 
useless...
>> +		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.
We were supposed to do it with the previous version.
But we are facing some difficulties using die function outside the test 
harness. We wanted to include the file "test-lib.sh" in our own script 
"test-gitmw-lib.sh" to use die. It doesn't work, because the harness 
expect the file "test-gitmw-lib.sh" to be a test script. Wich is not the 
case.

Do you have any Idea about how to fix this problem ?
>> +fail()
>> +{
> Style: fail () {
>
> Plus, didn't we say "die" was already there for this?
Agree, see above.
>> +        # 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?
Change on the way. renaming this variable "is_login". Is this ok for you ?
> Any reason not to use Perl's option parsing?
Not at all, we juste didn't thought about it.

-- 
CATHEBRAS Simon

2A-ENSIMAG

Filière Ingéniérie des Systèmes d'Information
Membre Bug-Buster