Re: [PATCH/RFC 4/4] git-mw: Adding preview tool in git-mw.perl

From: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
Date: Jun 13, 2013, 12:13 UTC
Message-ID: <vpq61xiryxm.fsf@anie.imag.fr>
In-Reply-To: <1371118039-18925-5-git-send-email-benoit.person@ensimag.fr>

benoit.person@ensimag.fr writes:

Show 25 quoted lines

> From: Benoit Person <benoit.person@ensimag.fr>
>
> This final commit adds the preview subcommand to git mw. It works as such:
> 1- Find the remote name of the current branch's upstream and check if it's a
> mediawiki one.
> 1b- If it's not found or if it's not a mediawiki one. It will list all the
> mediawiki remotes configured and ask the user to replay the command with the
> --remote option set.
> 2- Parse the content of the local file (or blob) (given as argument) using
> the distant mediawiki's API
> 3- Retrieve the current page on the distant mediawiki
> 4- Replaces all content in that page with the newly parsed one
> 5- Convert relative links into absolute
> 6- Save the result on disk
>
> The command accepts those options:
>   --autoload | -a tries to launch the newly generated file in the user's
>                   default browser (using git web--browse)
>   --remote | -r provides a way to select the distant mediawiki in which
>                 the user wants to preview his file (or blob)
>   --output | -o enables the user to choose the output filename. Default
>                 output filename is based on the input filename in which
>                 the extension '.mw' is replaced with '.html'
>   --blob | -b tells the script that the last argument is a blob and not
>               a filename

A commit messages that answers the "what?" and "how?" questions (as opposed to "why?") is always suspicious: doesn't the message belong elsewhere?

Here, you have a nice user documentation for command-line options, and the actual user doc is much poorer:

Show 9 quoted lines

> +sub preview_help {
> +	print <<'END';
> +usage: git mw preview [--remote|-r <remote name>] [--autoload|-a]
> +                      [--output|-o <output filename>] <filename>
> +
> +    -r, --remote    Specify which mediawiki should be used
> +    -o, --output    Name of the output file
> +    -a, --autoload  Autoload the page in your default web browser
> +END

(shorter description, missing --blob)

> +	} else { # file mode
> +		if (! -e $file_name) {
> +			die "File $file_name does not exists \n";

We're just setting a convention to use ${var} in string interpolation (Celestin's perlcritic patch series), so better do it right now ;-).

Did you try "make perlcritic" on your code?

> +	# Default preview_file_name is file_name with .html ext
> +	if ($preview_file_name eq '') {

EMPTY ?

> +	if ($remote_name eq '') {

EMPTY ?

> +	# Load template page
> +	$template = get("$remote_url/index.php?title=$wiki_page_name")
> +		or die "You need to create $wiki_page_name before previewing it";

I got hit again by the HTTPS certificate validation failure. It would make sense to have a more detailed error message, including the URL, because having the same error:

You need to create Accueil before previewing it at /home/moy/local/usr-wheezy/libexec/git-core/git-mw line 182.

for any kind of HTTP failure is a painful. Doesn't "get" return an HTTP code? If so, your message would make sense for 404 errors, but not for the others.

> +	$mw_content_text = $html_tree->look_down('id', 'mw-content-text');

Unfortunately, this doesn't seem standard. It doesn't work on https://ensiwiki.ensimag.fr/index.php/Accueil at least (which is my main use-case :-( ).

At least, you should check $mw_content_text and have a nice error message here. As much as possible, you should allow a way to solve it (make the lookup configurable in .git/config, or allow the user to specify an arbitrary HTML template to plug onto, or display the raw, incomplete, HTML).

I replaced 'mw-content-text' with 'bodyContent' and it worked.

Then I got

Wide character in print at /usr/lib/perl/5.14/IO/Handle.pm line 159.

but the file was generated. There are encoding problems: the title says "Le Wiki des Ã©tudiants et enseignants" (it should be a É).

I guess you fed the API with an improper encoding (double UTF-8 encoding, or UTF-8 announced as latin-1 or so), and the API returned you some hard-coded, badly encoded, rendered HTML.

Show 6 quoted lines

> @@ -41,6 +241,7 @@ usage: git mw <command> <args>
>  
>  git mw commands are:
>      Help        Display help information about git mw
> +    Preview 	Parse and render local file into HTML
>  END

Lower-case help and preview.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/