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

benoit.person@ensimag.fr writes:

From: Benoit Person <redacted>

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.

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/

On Sat, Jun 08, 2013 at 09:00:30PM +0200, Matthieu Moy wrote:

quoted

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

In fact, I think we provide the "git-web--browse" tool for just this
purpose. It takes care of the trickiness of looking at the "web.browser"
config, resolving custom browser tools defined by "browser.<tool>.*",
and handling backgrounding of the browser (which you want to do for
graphical browsers but not for terminal ones).

-Peff

PS I agreed with all of the other comments in your review. :)