Re: [PATCH/RFC] git-remote-mediawiki: new tool to preview local changes without pushing

From: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
Date: Jun 8, 2013, 19:00 UTC
Message-ID: <vpqzjv0tokx.fsf@anie.imag.fr>
In-Reply-To: <1370641831-9115-1-git-send-email-benoit.person@ensimag.fr>

benoit.person@ensimag.fr writes:

Show 8 quoted lines

> From: Benoit Person <benoit.person@ensimag.fr>
>
> The #7 issue on git-mediawiki's issue tracker [1] states that the ability to
> preview content without pushing would be a nice thing to have.
>
> This commit is a first attempt to achieve it. It adds a new git command,
> named `git mw`. This command accepts the subcommands `help` and `preview`
> for now.

Review could be easier if you have PATCH 1/2 introducing the command with only "help" (essentially to check that the build system works), and then focus on "preview".

I won't insist in splitting this particular commit, just take it as an advice for future work.

Show 7 quoted lines

> --- a/contrib/mw-to-git/Makefile
> +++ b/contrib/mw-to-git/Makefile
> @@ -5,13 +5,17 @@
>  ## Build git-remote-mediawiki
>  
>  SCRIPT_PERL=git-remote-mediawiki.perl
> +SCRIPT_PERL_MW=git-mw.perl

Why do you need another variable? Just adding git-mw.perl to SCRIPT_PERL should do it, no? (Well, probably the make SCRIPT_PERL=$(SCRIPT_PERL) lacks quotes around the argument, but then you should fix that).

> +# Constants
> +# Mediawiki filenames can contain forward slashes. This variable decides by which pattern they should be replaced
> +Readonly::scalar $SLASH_REPLACEMENT => "%2F";

This one is copied from git-remote-mediawiki.perl, and is a git-mediawiki specific thing, so it would not make sense to put it into Git.pm. This starts convincing me that we should have a GitMediawiki.pm or so, and both scripts should use it.

Another option would be to put everything in git-remote-mediawiki.perl, and probably to have "git mw" as a wrapper around it (to avoid having to type "git remote-mediawiki" which is a bit long).

But the script starts being a bit long, so I think it makes more sense to split it. So I'd say

PATCH 1/3 : introduce "git mw"
PATCH 2/3 : move sharable code to a new module (and make sure it's
            installed properly by "make install")
PATCH 3/3 : actually implement the preview feature

Perhaps others will have other/better advices.

> +	# Auto-loading in browser
> +	if ($autoload) {
> +		open(my $browser, "-|:encoding(UTF-8)", "xdg-open ".$preview_file_name);

That could be read from Git's configuration, and default to xdg-open. But you don't want to hardcode it in the middle of the code.

Also, why use open if you don't want to communicate with the process? system would be sufficient, and won't need close.

Prefer passing an array of arguments, instead of concatenating strings and then rely on command-line parsing to re-split it.

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