[PATCH 00/18] git-remote-mediawiki: Follow perlcritic's recommandations

[PATCH 00/18] git-remote-mediawiki: Follow perlcritic's recommandations

[Célestin Matte]

Hi,
This series of commits intends to follow most of perlcritic's recommandations
in order to make the code more maintainable and readable.
I followed most recommandations from level 5 (most critical ones )to 2, but 
left a great part of the level 1 ones, as they were more about personal choices
of coding style and less about actually improving maintainability and preventing
bugs. Among those I did *not* take into account were:
- ValuesAndExpressions::ProhibitNoisyQuotes: use q(*) instead of '*' for quoting
  some characters
- RegularExpressions::ProhibitEnumeratedClasses: e.g., use [0-9] instead of \d
- CodeLayout::ProhibitParensWithBuiltins: don't use parentheses with builtin
  functions
- RegularExpressions::RequireExtendedFormatting, 
  RegularExpressions::RequireDotMatchAnything and
  RegularExpressions::RequireLineBoundaryMatching: use s, m and x flags for
  *every* regexp
- and some others. Run perlcritic -1 on patched file to so what is left.
Please tell me if you think some of them should be applied anyway.

On the other hand, if quite not sure of the relevance of some commits:
- Add newline in the end of die() error messages [3/18]: are die() messages
  correctly handled already?
- Place the open() call inside the do{} struct and prevent failing close
  [17/18]: current code fails to close the filehandle (try adding a 
  "or warn(..)" after the close() to check this). However, perlcritic complains
  as well if you don't put an explicit close(), which fails when you put it.
  I'm not sure of what's happening and what's the best solution for this.

I checked Documentation/CodingGuidelines and none of these modifications should
contradict it.
This is my first patch, so don't hesitate to point to me anything wrong with it.

Célestin Matte (18):
  Follow perlcritic's recommendations - level 5 and 4
  Change style of some regular expressions to make them clearer
  Add newline in the end of die() error messages
  Prevent local variable $url to have the same name as a global
    variable
  Turn double-negated expressions into simple expressions
  Remove unused variable
  Rename a variable ($last) so that it does not have the name of a
    keyword
  Explicitely assign local variable as undef and make a proper
    one-instruction-by-line indentation
  Check return value of open and remove import of unused open2
  Put long code into a submodule
  Modify strings for a better coding-style
  Brace file handles for print for more clarity
  Remove "unless" statements and replace them by negated "if"
    statements
  Don't use quotes for empty strings
  Put non-trivial numeric values (e.g., different from 0, 1 and 2) in
    constants.
  Fix a typo ("mediwiki" instead of "mediawiki")
  Place the open() call inside the do{} struct and prevent failing
    close
  Clearly rewrite double dereference

 contrib/mw-to-git/git-remote-mediawiki.perl |  558 ++++++++++++++-------------
 1 file changed, 298 insertions(+), 260 deletions(-)

-- 
1.7.9.5

[PATCH 01/18] Follow perlcritic's recommendations - level 5 and 4

[Célestin Matte]

Fix warnings from perlcritic's level 5 and 4. They correspond to the following
cases:
- always end a submodule with a return
- don't use the constant pragma, use the Readonly module instead
- some syntax details for maps, and others.

Signed-off-by: Célestin Matte <celestin.matte@ensimag.fr>
Signed-off-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
---
 contrib/mw-to-git/git-remote-mediawiki.perl |   81 +++++++++++++++++----------
 1 file changed, 51 insertions(+), 30 deletions(-)

diff --git a/contrib/mw-to-git/git-remote-mediawiki.perl b/contrib/mw-to-git/git-remote-mediawiki.perl
index 410eae9..83cf292 100755
--- a/contrib/mw-to-git/git-remote-mediawiki.perl
+++ b/contrib/mw-to-git/git-remote-mediawiki.perl
@@ -15,32 +15,32 @@ use strict;
 use MediaWiki::API;
 use Git;
 use DateTime::Format::ISO8601;
+use warnings;
 
 # By default, use UTF-8 to communicate with Git and the user
-binmode STDERR, ":utf8";
-binmode STDOUT, ":utf8";
+binmode STDERR, ":encoding(UTF-8)";
+binmode STDOUT, ":encoding(UTF-8)";
 
 use URI::Escape;
 use IPC::Open2;
-
-use warnings;
+use Readonly;
 
 # Mediawiki filenames can contain forward slashes. This variable decides by which pattern they should be replaced
-use constant SLASH_REPLACEMENT => "%2F";
+Readonly my $SLASH_REPLACEMENT => "%2F";
 
 # It's not always possible to delete pages (may require some
 # privileges). Deleted pages are replaced with this content.
-use constant DELETED_CONTENT => "[[Category:Deleted]]\n";
+Readonly my $DELETED_CONTENT => "[[Category:Deleted]]\n";
 
 # It's not possible to create empty pages. New empty files in Git are
 # sent with this content instead.
-use constant EMPTY_CONTENT => "<!-- empty page -->\n";
+Readonly my $EMPTY_CONTENT => "<!-- empty page -->\n";
 
 # used to reflect file creation or deletion in diff.
-use constant NULL_SHA1 => "0000000000000000000000000000000000000000";
+Readonly my $NULL_SHA1 => "0000000000000000000000000000000000000000";
 
 # Used on Git's side to reflect empty edit messages on the wiki
-use constant EMPTY_MESSAGE => '*Empty MediaWiki Message*';
+Readonly my $EMPTY_MESSAGE => '*Empty MediaWiki Message*';
 
 if (@ARGV != 2) {
 	exit_error_usage();
@@ -96,6 +96,9 @@ unless ($fetch_strategy) {
 	$fetch_strategy = "by_page";
 }
 
+# Remember the timestamp corresponding to a revision id.
+my %basetimestamps;
+
 # Dumb push: don't update notes and mediawiki ref to reflect the last push.
 #
 # Configurable with mediawiki.dumbPush, or per-remote with
@@ -198,12 +201,14 @@ sub mw_connect_maybe {
 			exit 1;
 		}
 	}
+	return;
 }
 
 ## Functions for listing pages on the remote wiki
 sub get_mw_tracked_pages {
 	my $pages = shift;
 	get_mw_page_list(\@tracked_pages, $pages);
+	return;
 }
 
 sub get_mw_page_list {
@@ -219,6 +224,7 @@ sub get_mw_page_list {
 		get_mw_first_pages(\@slice, $pages);
 		@some_pages = @some_pages[51..$#some_pages];
 	}
+	return;
 }
 
 sub get_mw_tracked_categories {
@@ -241,6 +247,7 @@ sub get_mw_tracked_categories {
 			$pages->{$page->{title}} = $page;
 		}
 	}
+	return;
 }
 
 sub get_mw_all_pages {
@@ -260,6 +267,7 @@ sub get_mw_all_pages {
 	foreach my $page (@{$mw_pages}) {
 		$pages->{$page->{title}} = $page;
 	}
+	return;
 }
 
 # queries the wiki for a set of pages. Meant to be used within a loop
@@ -290,6 +298,7 @@ sub get_mw_first_pages {
 			$pages->{$page->{title}} = $page;
 		}
 	}
+	return;
 }
 
 # Get the list of pages to be fetched according to configuration.
@@ -358,11 +367,12 @@ sub get_all_mediafiles {
 	foreach my $page (@{$mw_pages}) {
 		$pages->{$page->{title}} = $page;
 	}
+	return;
 }
 
 sub get_linked_mediafiles {
 	my $pages = shift;
-	my @titles = map $_->{title}, values(%{$pages});
+	my @titles = map { $_->{title} } values(%{$pages});
 
 	# The query is split in small batches because of the MW API limit of
 	# the number of links to be returned (500 links max).
@@ -390,11 +400,13 @@ sub get_linked_mediafiles {
 		while (my ($id, $page) = each(%{$result->{query}->{pages}})) {
 			my @media_titles;
 			if (defined($page->{links})) {
-				my @link_titles = map $_->{title}, @{$page->{links}};
+				my @link_titles
+				    = map { $_->{title} } @{$page->{links}};
 				push(@media_titles, @link_titles);
 			}
 			if (defined($page->{images})) {
-				my @image_titles = map $_->{title}, @{$page->{images}};
+				my @image_titles
+				    = map { $_->{title} } @{$page->{images}};
 				push(@media_titles, @image_titles);
 			}
 			if (@media_titles) {
@@ -404,6 +416,7 @@ sub get_linked_mediafiles {
 
 		@titles = @titles[($batch+1)..$#titles];
 	}
+	return;
 }
 
 sub get_mw_mediafile_for_page_revision {
@@ -473,9 +486,6 @@ sub get_last_local_revision {
 	return $lastrevision_number;
 }
 
-# Remember the timestamp corresponding to a revision id.
-my %basetimestamps;
-
 # Get the last remote revision without taking in account which pages are
 # tracked or not. This function makes a single request to the wiki thus
 # avoid a loop onto all tracked pages. This is useful for the fetch-by-rev
@@ -538,7 +548,7 @@ sub mediawiki_clean {
 	$string =~ s/\s+$//;
 	if ($string eq "" && $page_created) {
 		# Creating empty pages is forbidden.
-		$string = EMPTY_CONTENT;
+		$string = $EMPTY_CONTENT;
 	}
 	return $string."\n";
 }
@@ -546,7 +556,7 @@ sub mediawiki_clean {
 # Filter applied on MediaWiki data before adding them to Git
 sub mediawiki_smudge {
 	my $string = shift;
-	if ($string eq EMPTY_CONTENT) {
+	if ($string eq $EMPTY_CONTENT) {
 		$string = "";
 	}
 	# This \n is important. This is due to mediawiki's way to handle end of files.
@@ -555,7 +565,7 @@ sub mediawiki_smudge {
 
 sub mediawiki_clean_filename {
 	my $filename = shift;
-	$filename =~ s/@{[SLASH_REPLACEMENT]}/\//g;
+	$filename =~ s{$SLASH_REPLACEMENT}{/}g;
 	# [, ], |, {, and } are forbidden by MediaWiki, even URL-encoded.
 	# Do a variant of URL-encoding, i.e. looks like URL-encoding,
 	# but with _ added to prevent MediaWiki from thinking this is
@@ -569,7 +579,7 @@ sub mediawiki_clean_filename {
 
 sub mediawiki_smudge_filename {
 	my $filename = shift;
-	$filename =~ s/\//@{[SLASH_REPLACEMENT]}/g;
+	$filename =~ s{/}{$SLASH_REPLACEMENT}g;
 	$filename =~ s/ /_/g;
 	# Decode forbidden characters encoded in mediawiki_clean_filename
 	$filename =~ s/_%_([0-9a-fA-F][0-9a-fA-F])/sprintf("%c", hex($1))/ge;
@@ -579,6 +589,7 @@ sub mediawiki_smudge_filename {
 sub literal_data {
 	my ($content) = @_;
 	print STDOUT "data ", bytes::length($content), "\n", $content;
+	return;
 }
 
 sub literal_data_raw {
@@ -588,7 +599,8 @@ sub literal_data_raw {
 	utf8::downgrade($content);
 	binmode STDOUT, ":raw";
 	print STDOUT "data ", bytes::length($content), "\n", $content;
-	binmode STDOUT, ":utf8";
+	binmode STDOUT, ":encoding(UTF-8)";
+	return;
 }
 
 sub mw_capabilities {
@@ -600,6 +612,7 @@ sub mw_capabilities {
 	print STDOUT "list\n";
 	print STDOUT "push\n";
 	print STDOUT "\n";
+	return;
 }
 
 sub mw_list {
@@ -608,11 +621,13 @@ sub mw_list {
 	print STDOUT "? refs/heads/master\n";
 	print STDOUT "\@refs/heads/master HEAD\n";
 	print STDOUT "\n";
+	return;
 }
 
 sub mw_option {
 	print STDERR "remote-helper command 'option $_[0]' not yet implemented\n";
 	print STDOUT "unsupported\n";
+	return;
 }
 
 sub fetch_mw_revisions_for_page {
@@ -707,7 +722,7 @@ sub import_file_revision {
 	if (!$full_import && $n == 1) {
 		print STDOUT "from refs/mediawiki/$remotename/master^0\n";
 	}
-	if ($content ne DELETED_CONTENT) {
+	if ($content ne $DELETED_CONTENT) {
 		print STDOUT "M 644 inline " .
 		    fe_escape_path($title . ".mw") . "\n";
 		literal_data($content);
@@ -734,6 +749,7 @@ sub import_file_revision {
 	print STDOUT "N inline :$n\n";
 	literal_data("mediawiki_revision: " . $commit{mw_revision});
 	print STDOUT "\n\n";
+	return;
 }
 
 # parse a sequence of
@@ -754,6 +770,7 @@ sub get_more_refs {
 			die("Invalid command in a '$cmd' batch: ". $_);
 		}
 	}
+	return;
 }
 
 sub mw_import {
@@ -763,6 +780,7 @@ sub mw_import {
 		mw_import_ref($ref);
 	}
 	print STDOUT "done\n";
+	return;
 }
 
 sub mw_import_ref {
@@ -806,6 +824,7 @@ sub mw_import_ref {
 		# thrown saying that HEAD is referring to unknown object 0000000000000000000
 		# and the clone fails.
 	}
+	return;
 }
 
 sub mw_import_ref_by_pages {
@@ -817,7 +836,7 @@ sub mw_import_ref_by_pages {
 	my ($n, @revisions) = fetch_mw_revisions(\@pages, $fetch_from);
 
 	@revisions = sort {$a->{revid} <=> $b->{revid}} @revisions;
-	my @revision_ids = map $_->{revid}, @revisions;
+	my @revision_ids = map { $_->{revid} } @revisions;
 
 	return mw_import_revids($fetch_from, \@revision_ids, \%pages_hash);
 }
@@ -888,7 +907,7 @@ sub mw_import_revids {
 
 		my %commit;
 		$commit{author} = $rev->{user} || 'Anonymous';
-		$commit{comment} = $rev->{comment} || EMPTY_MESSAGE;
+		$commit{comment} = $rev->{comment} || $EMPTY_MESSAGE;
 		$commit{title} = mediawiki_smudge_filename($page_title);
 		$commit{mw_revision} = $rev->{revid};
 		$commit{content} = mediawiki_smudge($rev->{'*'});
@@ -1006,14 +1025,14 @@ sub mw_push_file {
 	my $oldrevid = shift;
 	my $newrevid;
 
-	if ($summary eq EMPTY_MESSAGE) {
+	if ($summary eq $EMPTY_MESSAGE) {
 		$summary = '';
 	}
 
 	my $new_sha1 = $diff_info_split[3];
 	my $old_sha1 = $diff_info_split[2];
-	my $page_created = ($old_sha1 eq NULL_SHA1);
-	my $page_deleted = ($new_sha1 eq NULL_SHA1);
+	my $page_created = ($old_sha1 eq $NULL_SHA1);
+	my $page_deleted = ($new_sha1 eq $NULL_SHA1);
 	$complete_file_name = mediawiki_clean_filename($complete_file_name);
 
 	my ($title, $extension) = $complete_file_name =~ /^(.*)\.([^\.]*)$/;
@@ -1032,7 +1051,7 @@ sub mw_push_file {
 			# special privileges. A common
 			# convention is to replace the page
 			# with this content instead:
-			$file_content = DELETED_CONTENT;
+			$file_content = $DELETED_CONTENT;
 		} else {
 			$file_content = run_git("cat-file blob $new_sha1");
 		}
@@ -1112,6 +1131,7 @@ sub mw_push {
 		print STDERR "  git pull --rebase\n";
 		print STDERR "\n";
 	}
+	return;
 }
 
 sub mw_push_revision {
@@ -1229,8 +1249,8 @@ sub get_allowed_file_extensions {
 		siprop => 'fileextensions'
 		};
 	my $result = $mediawiki->api($query);
-	my @file_extensions= map $_->{ext},@{$result->{query}->{fileextensions}};
-	my %hashFile = map {$_ => 1}@file_extensions;
+	my @file_extensions = map { $_->{ext}} @{$result->{query}->{fileextensions}};
+	my %hashFile = map { $_ => 1 } @file_extensions;
 
 	return %hashFile;
 }
@@ -1314,7 +1334,8 @@ sub get_mw_namespace_id {
 }
 
 sub get_mw_namespace_id_for_page {
-	if (my ($namespace) = $_[0] =~ /^([^:]*):/) {
+	my $namespace = shift;
+	if ($namespace =~ /^([^:]*):/) {
 		return get_mw_namespace_id($namespace);
 	} else {
 		return;
-- 
1.7.9.5

Re: [PATCH 01/18] Follow perlcritic's recommendations - level 5 and 4

[Eric Sunshine]

On Thu, Jun 6, 2013 at 3:34 PM, Célestin Matte
<celestin.matte@ensimag.fr> wrote:
> Fix warnings from perlcritic's level 5 and 4. They correspond to the following
> cases:
> - always end a submodule with a return
> - don't use the constant pragma, use the Readonly module instead
> - some syntax details for maps, and others.

Although loosely related by being mentioned by perlcritic (4,5), each
bullet point is otherwise unrelated, and mixing such unrelated changes
into a single patch can make review more difficult.

> Signed-off-by: Célestin Matte <celestin.matte@ensimag.fr>
> Signed-off-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
> ---
>  contrib/mw-to-git/git-remote-mediawiki.perl |   81 +++++++++++++++++----------
>  1 file changed, 51 insertions(+), 30 deletions(-)
>
> diff --git a/contrib/mw-to-git/git-remote-mediawiki.perl b/contrib/mw-to-git/git-remote-mediawiki.perl
> index 410eae9..83cf292 100755
> --- a/contrib/mw-to-git/git-remote-mediawiki.perl
> +++ b/contrib/mw-to-git/git-remote-mediawiki.perl
> @@ -15,32 +15,32 @@ use strict;
>  use MediaWiki::API;
>  use Git;
>  use DateTime::Format::ISO8601;
> +use warnings;
>
>  # By default, use UTF-8 to communicate with Git and the user
> -binmode STDERR, ":utf8";
> -binmode STDOUT, ":utf8";
> +binmode STDERR, ":encoding(UTF-8)";
> +binmode STDOUT, ":encoding(UTF-8)";

This change isn't explained or rationalized in the commit message.

> @@ -96,6 +96,9 @@ unless ($fetch_strategy) {
>         $fetch_strategy = "by_page";
>  }
>
> +# Remember the timestamp corresponding to a revision id.
> +my %basetimestamps;

Although this is a simple textual relocation, it's not clear why it's
needed or preferable, and the commit message does not explain it.

> @@ -473,9 +486,6 @@ sub get_last_local_revision {
>         return $lastrevision_number;
>  }
>
> -# Remember the timestamp corresponding to a revision id.
> -my %basetimestamps;
> -
>  # Get the last remote revision without taking in account which pages are
>  # tracked or not. This function makes a single request to the wiki thus
>  # avoid a loop onto all tracked pages. This is useful for the fetch-by-rev
> @@ -555,7 +565,7 @@ sub mediawiki_smudge {
>
>  sub mediawiki_clean_filename {
>         my $filename = shift;
> -       $filename =~ s/@{[SLASH_REPLACEMENT]}/\//g;
> +       $filename =~ s{$SLASH_REPLACEMENT}{/}g;

Although patch 2/18 replaces regex // with {}, the change sneaked into
this patch (1/18) prematurely.

>         # [, ], |, {, and } are forbidden by MediaWiki, even URL-encoded.
>         # Do a variant of URL-encoding, i.e. looks like URL-encoding,
>         # but with _ added to prevent MediaWiki from thinking this is
> @@ -569,7 +579,7 @@ sub mediawiki_clean_filename {
>
>  sub mediawiki_smudge_filename {
>         my $filename = shift;
> -       $filename =~ s/\//@{[SLASH_REPLACEMENT]}/g;
> +       $filename =~ s{/}{$SLASH_REPLACEMENT}g;

Ditto regarding // to {}.

>         $filename =~ s/ /_/g;
>         # Decode forbidden characters encoded in mediawiki_clean_filename
>         $filename =~ s/_%_([0-9a-fA-F][0-9a-fA-F])/sprintf("%c", hex($1))/ge;
> @@ -588,7 +599,8 @@ sub literal_data_raw {
>         utf8::downgrade($content);
>         binmode STDOUT, ":raw";
>         print STDOUT "data ", bytes::length($content), "\n", $content;
> -       binmode STDOUT, ":utf8";
> +       binmode STDOUT, ":encoding(UTF-8)";

Unexplained change.

> +       return;
> }
>
>  sub mw_capabilities {
> @@ -1314,7 +1334,8 @@ sub get_mw_namespace_id {
>  }
>
>  sub get_mw_namespace_id_for_page {
> -       if (my ($namespace) = $_[0] =~ /^([^:]*):/) {
> +       my $namespace = shift;
> +       if ($namespace =~ /^([^:]*):/) {

Another change not mentioned by the commit message.

>                 return get_mw_namespace_id($namespace);
>         } else {
>                 return;
> --

Re: [PATCH 01/18] Follow perlcritic's recommendations - level 5 and 4

[Matthieu Moy]

Célestin Matte <celestin.matte@ensimag.fr> writes:

> Subject: [PATCH 01/18] Follow perlcritic's recommendations - level 5 and 4

It would be better to prefix commit messages with "git-remote-mediawiki: ".

> Fix warnings from perlcritic's level 5 and 4.

It would be cool to have a "make perlcritic" target in the Makefile so
that future developers can easily re-run it and avoid repeating the same
mistakes. As much as possible, "make perlcritic" should produce no
output at the end of your patch series (either the warnings should be
fixed, or they should be disabled).

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

Re: [PATCH 01/18] Follow perlcritic's recommendations - level 5 and 4

[Célestin Matte]

Le 07/06/2013 10:10, Matthieu Moy a écrit :
> It would be cool to have a "make perlcritic" target in the Makefile so
> that future developers can easily re-run it and avoid repeating the same
> mistakes. As much as possible, "make perlcritic" should produce no
> output at the end of your patch series (either the warnings should be
> fixed, or they should be disabled).

The problem is that I took some policies into account for some parts of
the code, but not for all of it. For instance, in commit [15/18], I put
some numeric values in constants, but not all of them, as I think having
"arg[3]" in the code does make sense. Ignoring this policy for future
developement just to prevent the related warnings from appearing would
prevent us to see useful warning messages from this policy.
Therefore, there still are a dozen warning messages appearing in the
current state...

Anyway, should this be integrated in the current patch or added as an
independant patch?

-- 
Célestin Matte

Re: [PATCH 01/18] Follow perlcritic's recommendations - level 5 and 4

[Matthieu Moy]

Célestin Matte <celestin.matte@ensimag.fr> writes:

> The problem is that I took some policies into account for some parts of
> the code, but not for all of it. For instance, in commit [15/18], I put
> some numeric values in constants, but not all of them, as I think having
> "arg[3]" in the code does make sense. Ignoring this policy for future
> developement just to prevent the related warnings from appearing would
> prevent us to see useful warning messages from this policy.

OK. Perhaps a "make sloppy-perlcritic" with zero output and a "make
pedantic-perlcritic" with more policies can make sense. Or just "make
perlcritic", and accept that there are warnings.

> Anyway, should this be integrated in the current patch or added as an
> independant patch?

That could be a new patch, preferably the first of the series.

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

[PATCH 02/18] Change style of some regular expressions to make them clearer

[Célestin Matte]

- Remove m modifier when useless (m// and // was used randomly; this makes the
code more coherent)
- Remove stringy split (split('c', ...) instead of split(/c/, ...))
- Use {}{} instead of /// when slashes or used inside the regexp so as not to
escape it.

Signed-off-by: Célestin Matte <celestin.matte@ensimag.fr>
Signed-off-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
---
 contrib/mw-to-git/git-remote-mediawiki.perl |   16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/contrib/mw-to-git/git-remote-mediawiki.perl b/contrib/mw-to-git/git-remote-mediawiki.perl
index 83cf292..482cd95 100755
--- a/contrib/mw-to-git/git-remote-mediawiki.perl
+++ b/contrib/mw-to-git/git-remote-mediawiki.perl
@@ -121,7 +121,7 @@ chomp($dumb_push);
 $dumb_push = ($dumb_push eq "true");
 
 my $wiki_name = $url;
-$wiki_name =~ s/[^\/]*:\/\///;
+$wiki_name =~ s{[^/]*://}{};
 # If URL is like http://user:password@example.com/, we clearly don't
 # want the password in $wiki_name. While we're there, also remove user
 # and '@' sign, to avoid author like MWUser@HTTPUser@host.com
@@ -762,7 +762,7 @@ sub get_more_refs {
 	my @refs;
 	while (1) {
 		my $line = <STDIN>;
-		if ($line =~ m/^$cmd (.*)$/) {
+		if ($line =~ /^$cmd (.*)$/) {
 			push(@refs, $1);
 		} elsif ($line eq "\n") {
 			return @refs;
@@ -1168,11 +1168,11 @@ sub mw_push_revision {
 		my @local_ancestry = split(/\n/, run_git("rev-list --boundary --parents $local ^$parsed_sha1"));
 		my %local_ancestry;
 		foreach my $line (@local_ancestry) {
-			if (my ($child, $parents) = $line =~ m/^-?([a-f0-9]+) ([a-f0-9 ]+)/) {
-				foreach my $parent (split(' ', $parents)) {
+			if (my ($child, $parents) = $line =~ /^-?([a-f0-9]+) ([a-f0-9 ]+)/) {
+				foreach my $parent (split(/ /, $parents)) {
 					$local_ancestry{$parent} = $child;
 				}
-			} elsif (!$line =~ m/^([a-f0-9]+)/) {
+			} elsif (!$line =~ /^([a-f0-9]+)/) {
 				die "Unexpected output from git rev-list: $line";
 			}
 		}
@@ -1190,10 +1190,10 @@ sub mw_push_revision {
 		# history (linearized with --first-parent)
 		print STDERR "Warning: no common ancestor, pushing complete history\n";
 		my $history = run_git("rev-list --first-parent --children $local");
-		my @history = split('\n', $history);
+		my @history = split(/\n/, $history);
 		@history = @history[1..$#history];
 		foreach my $line (reverse @history) {
-			my @commit_info_split = split(/ |\n/, $line);
+			my @commit_info_split = split(/[ \n]/, $line);
 			push(@commit_pairs, \@commit_info_split);
 		}
 	}
@@ -1272,7 +1272,7 @@ sub get_mw_namespace_id {
 		# Look at configuration file, if the record for that namespace is
 		# already cached. Namespaces are stored in form:
 		# "Name_of_namespace:Id_namespace", ex.: "File:6".
-		my @temp = split(/[\n]/, run_git("config --get-all remote."
+		my @temp = split(/\n/, run_git("config --get-all remote."
 						. $remotename .".namespaceCache"));
 		chomp(@temp);
 		foreach my $ns (@temp) {
-- 
1.7.9.5

Re: [PATCH 02/18] Change style of some regular expressions to make them clearer

[Eric Sunshine]

On Thu, Jun 6, 2013 at 3:34 PM, Célestin Matte
<celestin.matte@ensimag.fr> wrote:
> - Remove m modifier when useless (m// and // was used randomly; this makes the
> code more coherent)
> - Remove stringy split (split('c', ...) instead of split(/c/, ...))
> - Use {}{} instead of /// when slashes or used inside the regexp so as not to
> escape it.
>
> Signed-off-by: Célestin Matte <celestin.matte@ensimag.fr>
> Signed-off-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
> ---
>  contrib/mw-to-git/git-remote-mediawiki.perl |   16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/contrib/mw-to-git/git-remote-mediawiki.perl b/contrib/mw-to-git/git-remote-mediawiki.perl
> index 83cf292..482cd95 100755
> --- a/contrib/mw-to-git/git-remote-mediawiki.perl
> +++ b/contrib/mw-to-git/git-remote-mediawiki.perl
> @@ -121,7 +121,7 @@ chomp($dumb_push);
>  $dumb_push = ($dumb_push eq "true");
>
>  my $wiki_name = $url;
> -$wiki_name =~ s/[^\/]*:\/\///;
> +$wiki_name =~ s{[^/]*://}{};
>  # If URL is like http://user:password@example.com/, we clearly don't
>  # want the password in $wiki_name. While we're there, also remove user
>  # and '@' sign, to avoid author like MWUser@HTTPUser@host.com
> @@ -762,7 +762,7 @@ sub get_more_refs {
>         my @refs;
>         while (1) {
>                 my $line = <STDIN>;
> -               if ($line =~ m/^$cmd (.*)$/) {
> +               if ($line =~ /^$cmd (.*)$/) {
>                         push(@refs, $1);
>                 } elsif ($line eq "\n") {
>                         return @refs;
> @@ -1168,11 +1168,11 @@ sub mw_push_revision {
>                 my @local_ancestry = split(/\n/, run_git("rev-list --boundary --parents $local ^$parsed_sha1"));
>                 my %local_ancestry;
>                 foreach my $line (@local_ancestry) {
> -                       if (my ($child, $parents) = $line =~ m/^-?([a-f0-9]+) ([a-f0-9 ]+)/) {
> -                               foreach my $parent (split(' ', $parents)) {
> +                       if (my ($child, $parents) = $line =~ /^-?([a-f0-9]+) ([a-f0-9 ]+)/) {
> +                               foreach my $parent (split(/ /, $parents)) {

This is a behavior-altering change. split(' ',...) is handled as a
special case[*1*] which strips leading whitespace and then splits on
/\s+/ (run of whitespace). Changing it to split(/ /,...) makes it
match only a single space (rather than a run of whitespace).

[*1*] http://perldoc.perl.org/functions/split.html

>                                         $local_ancestry{$parent} = $child;
>                                 }
> -                       } elsif (!$line =~ m/^([a-f0-9]+)/) {
> +                       } elsif (!$line =~ /^([a-f0-9]+)/) {
>                                 die "Unexpected output from git rev-list: $line";
>                         }
>                 }
> @@ -1190,10 +1190,10 @@ sub mw_push_revision {
>                 # history (linearized with --first-parent)
>                 print STDERR "Warning: no common ancestor, pushing complete history\n";
>                 my $history = run_git("rev-list --first-parent --children $local");
> -               my @history = split('\n', $history);
> +               my @history = split(/\n/, $history);
>                 @history = @history[1..$#history];
>                 foreach my $line (reverse @history) {
> -                       my @commit_info_split = split(/ |\n/, $line);
> +                       my @commit_info_split = split(/[ \n]/, $line);
>                         push(@commit_pairs, \@commit_info_split);
>                 }
>         }
> @@ -1272,7 +1272,7 @@ sub get_mw_namespace_id {
>                 # Look at configuration file, if the record for that namespace is
>                 # already cached. Namespaces are stored in form:
>                 # "Name_of_namespace:Id_namespace", ex.: "File:6".
> -               my @temp = split(/[\n]/, run_git("config --get-all remote."
> +               my @temp = split(/\n/, run_git("config --get-all remote."
>                                                 . $remotename .".namespaceCache"));
>                 chomp(@temp);
>                 foreach my $ns (@temp) {
> --
> 1.7.9.5
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 02/18] Change style of some regular expressions to make them clearer

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

>> -                       if (my ($child, $parents) = $line =~ m/^-?([a-f0-9]+) ([a-f0-9 ]+)/) {
>> -                               foreach my $parent (split(' ', $parents)) {
>> +                       if (my ($child, $parents) = $line =~ /^-?([a-f0-9]+) ([a-f0-9 ]+)/) {
>> +                               foreach my $parent (split(/ /, $parents)) {
>
> This is a behavior-altering change. split(' ',...) is handled as a
> special case[*1*] which strips leading whitespace and then splits on
> /\s+/ (run of whitespace). Changing it to split(/ /,...) makes it
> match only a single space (rather than a run of whitespace).

I initially had the same reaction, but this is reading the output of
the "rev-list --parents" command, whose fields are separated by one
SP each, so there is indeed no behaviour change.

Re: [PATCH 02/18] Change style of some regular expressions to make them clearer

[Eric Sunshine]

On Thu, Jun 6, 2013 at 10:30 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Eric Sunshine <sunshine@sunshineco.com> writes:
>
>>> -                       if (my ($child, $parents) = $line =~ m/^-?([a-f0-9]+) ([a-f0-9 ]+)/) {
>>> -                               foreach my $parent (split(' ', $parents)) {
>>> +                       if (my ($child, $parents) = $line =~ /^-?([a-f0-9]+) ([a-f0-9 ]+)/) {
>>> +                               foreach my $parent (split(/ /, $parents)) {
>>
>> This is a behavior-altering change. split(' ',...) is handled as a
>> special case[*1*] which strips leading whitespace and then splits on
>> /\s+/ (run of whitespace). Changing it to split(/ /,...) makes it
>> match only a single space (rather than a run of whitespace).
>
> I initially had the same reaction, but this is reading the output of
> the "rev-list --parents" command, whose fields are separated by one
> SP each, so there is indeed no behaviour change.

True. This potentially subtle point may deserve mention in the commit
message (or this particular change can be dropped).

Re: [PATCH 02/18] Change style of some regular expressions to make them clearer

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> On Thu, Jun 6, 2013 at 10:30 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Eric Sunshine <sunshine@sunshineco.com> writes:
>>
>>>> -                       if (my ($child, $parents) = $line =~ m/^-?([a-f0-9]+) ([a-f0-9 ]+)/) {
>>>> -                               foreach my $parent (split(' ', $parents)) {
>>>> +                       if (my ($child, $parents) = $line =~ /^-?([a-f0-9]+) ([a-f0-9 ]+)/) {
>>>> +                               foreach my $parent (split(/ /, $parents)) {
>>>
>>> This is a behavior-altering change. split(' ',...) is handled as a
>>> special case[*1*] which strips leading whitespace and then splits on
>>> /\s+/ (run of whitespace). Changing it to split(/ /,...) makes it
>>> match only a single space (rather than a run of whitespace).
>>
>> I initially had the same reaction, but this is reading the output of
>> the "rev-list --parents" command, whose fields are separated by one
>> SP each, so there is indeed no behaviour change.
>
> True. This potentially subtle point may deserve mention in the commit
> message (or this particular change can be dropped).

I agree that the log message should mention it; but the resulting
code is more correct than the original, so I do not see a need to
drop it, or add any in-code comment to it.

Re: [PATCH 02/18] Change style of some regular expressions to make them clearer

[Peter Krefting]

Célestin Matte:

> - Use {}{} instead of /// when slashes or used inside the regexp so as not to
> escape it.

I guess that should read "...when slashes *are* used inside..."?

-- 
\\// Peter - http://www.softwolves.pp.se/

[PATCH 03/18] Add newline in the end of die() error messages

[Célestin Matte]

Signed-off-by: Célestin Matte <celestin.matte@ensimag.fr>
Signed-off-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
---
 contrib/mw-to-git/git-remote-mediawiki.perl |   26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/contrib/mw-to-git/git-remote-mediawiki.perl b/contrib/mw-to-git/git-remote-mediawiki.perl
index 482cd95..4baad95 100755
--- a/contrib/mw-to-git/git-remote-mediawiki.perl
+++ b/contrib/mw-to-git/git-remote-mediawiki.perl
@@ -136,16 +136,16 @@ while (<STDIN>) {
 	if (defined($cmd[0])) {
 		# Line not blank
 		if ($cmd[0] eq "capabilities") {
-			die("Too many arguments for capabilities") unless (!defined($cmd[1]));
+			die("Too many arguments for capabilities\n") unless (!defined($cmd[1]));
 			mw_capabilities();
 		} elsif ($cmd[0] eq "list") {
-			die("Too many arguments for list") unless (!defined($cmd[2]));
+			die("Too many arguments for list\n") unless (!defined($cmd[2]));
 			mw_list($cmd[1]);
 		} elsif ($cmd[0] eq "import") {
-			die("Invalid arguments for import") unless ($cmd[1] ne "" && !defined($cmd[2]));
+			die("Invalid arguments for import\n") unless ($cmd[1] ne "" && !defined($cmd[2]));
 			mw_import($cmd[1]);
 		} elsif ($cmd[0] eq "option") {
-			die("Too many arguments for option") unless ($cmd[1] ne "" && $cmd[2] ne "" && !defined($cmd[3]));
+			die("Too many arguments for option\n") unless ($cmd[1] ne "" && $cmd[2] ne "" && !defined($cmd[3]));
 			mw_option($cmd[1],$cmd[2]);
 		} elsif ($cmd[0] eq "push") {
 			mw_push($cmd[1]);
@@ -242,7 +242,7 @@ sub get_mw_tracked_categories {
 			cmtitle => $category,
 			cmlimit => 'max' } )
 			|| die $mediawiki->{error}->{code} . ': '
-				. $mediawiki->{error}->{details};
+				. $mediawiki->{error}->{details} . "\n";
 		foreach my $page (@{$mw_pages}) {
 			$pages->{$page->{title}} = $page;
 		}
@@ -767,7 +767,7 @@ sub get_more_refs {
 		} elsif ($line eq "\n") {
 			return @refs;
 		} else {
-			die("Invalid command in a '$cmd' batch: ". $_);
+			die("Invalid command in a '$cmd' batch: $_\n");
 		}
 	}
 	return;
@@ -879,7 +879,7 @@ sub mw_import_revids {
 		my $result = $mediawiki->api($query);
 
 		if (!$result) {
-			die "Failed to retrieve modified page for revision $pagerevid";
+			die "Failed to retrieve modified page for revision $pagerevid\n";
 		}
 
 		if (defined($result->{query}->{badrevids}->{$pagerevid})) {
@@ -888,7 +888,7 @@ sub mw_import_revids {
 		}
 
 		if (!defined($result->{query}->{pages})) {
-			die "Invalid revision $pagerevid.";
+			die "Invalid revision $pagerevid.\n";
 		}
 
 		my @result_pages = values(%{$result->{query}->{pages}});
@@ -999,7 +999,7 @@ sub mw_upload_file {
 			}, {
 				skip_encoding => 1
 			} ) || die $mediawiki->{error}->{code} . ':'
-				 . $mediawiki->{error}->{details};
+				 . $mediawiki->{error}->{details} . "\n";
 			my $last_file_page = $mediawiki->get_page({title => $path});
 			$newrevid = $last_file_page->{revid};
 			print STDERR "Pushed file: $new_sha1 - $complete_file_name.\n";
@@ -1079,7 +1079,7 @@ sub mw_push_file {
 				# Other errors. Shouldn't happen => just die()
 				die 'Fatal: Error ' .
 				    $mediawiki->{error}->{code} .
-				    ' from mediwiki: ' . $mediawiki->{error}->{details};
+				    ' from mediwiki: ' . $mediawiki->{error}->{details} . "\n";
 			}
 		}
 		$newrevid = $result->{edit}->{newrevid};
@@ -1101,7 +1101,7 @@ sub mw_push {
 	my $pushed;
 	for my $refspec (@refsspecs) {
 		my ($force, $local, $remote) = $refspec =~ /^(\+)?([^:]*):([^:]*)$/
-		    or die("Invalid refspec for push. Expected <src>:<dst> or +<src>:<dst>");
+		    or die("Invalid refspec for push. Expected <src>:<dst> or +<src>:<dst>\n");
 		if ($force) {
 			print STDERR "Warning: forced push not allowed on a MediaWiki.\n";
 		}
@@ -1173,7 +1173,7 @@ sub mw_push_revision {
 					$local_ancestry{$parent} = $child;
 				}
 			} elsif (!$line =~ /^([a-f0-9]+)/) {
-				die "Unexpected output from git rev-list: $line";
+				die "Unexpected output from git rev-list: $line\n";
 			}
 		}
 		while ($parsed_sha1 ne $HEAD_sha1) {
@@ -1227,7 +1227,7 @@ sub mw_push_revision {
 				return error_non_fast_forward($remote);
 			}
 			if ($status ne "ok") {
-				die("Unknown error from mw_push_file()");
+				die("Unknown error from mw_push_file()\n");
 			}
 		}
 		unless ($dumb_push) {
-- 
1.7.9.5

[PATCH 04/18] Prevent local variable $url to have the same name as a global variable

[Célestin Matte]

There is already a global variable called $url. Changing the name of the local
variable prevents future possible misunderstanding.

Signed-off-by: Célestin Matte <celestin.matte@ensimag.fr>
Signed-off-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
---
 contrib/mw-to-git/git-remote-mediawiki.perl |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/contrib/mw-to-git/git-remote-mediawiki.perl b/contrib/mw-to-git/git-remote-mediawiki.perl
index 4baad95..68fd129 100755
--- a/contrib/mw-to-git/git-remote-mediawiki.perl
+++ b/contrib/mw-to-git/git-remote-mediawiki.perl
@@ -455,14 +455,14 @@ sub get_mw_mediafile_for_page_revision {
 }
 
 sub download_mw_mediafile {
-	my $url = shift;
+	my $download_url = shift;
 
-	my $response = $mediawiki->{ua}->get($url);
+	my $response = $mediawiki->{ua}->get($download_url);
 	if ($response->code == 200) {
 		return $response->decoded_content;
 	} else {
 		print STDERR "Error downloading mediafile from :\n";
-		print STDERR "URL: $url\n";
+		print STDERR "URL: $download_url\n";
 		print STDERR "Server response: " . $response->code . " " . $response->message . "\n";
 		exit 1;
 	}
-- 
1.7.9.5

[PATCH 05/18] Turn double-negated expressions into simple expressions

[Célestin Matte]

Signed-off-by: Célestin Matte <celestin.matte@ensimag.fr>
Signed-off-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
---
 contrib/mw-to-git/git-remote-mediawiki.perl |    8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/contrib/mw-to-git/git-remote-mediawiki.perl b/contrib/mw-to-git/git-remote-mediawiki.perl
index 68fd129..a6c7de2 100755
--- a/contrib/mw-to-git/git-remote-mediawiki.perl
+++ b/contrib/mw-to-git/git-remote-mediawiki.perl
@@ -136,16 +136,16 @@ while (<STDIN>) {
 	if (defined($cmd[0])) {
 		# Line not blank
 		if ($cmd[0] eq "capabilities") {
-			die("Too many arguments for capabilities\n") unless (!defined($cmd[1]));
+			die("Too many arguments for capabilities\n") if (defined($cmd[1]));
 			mw_capabilities();
 		} elsif ($cmd[0] eq "list") {
-			die("Too many arguments for list\n") unless (!defined($cmd[2]));
+			die("Too many arguments for list\n") if (defined($cmd[2]));
 			mw_list($cmd[1]);
 		} elsif ($cmd[0] eq "import") {
-			die("Invalid arguments for import\n") unless ($cmd[1] ne "" && !defined($cmd[2]));
+			die("Invalid arguments for import\n") if ($cmd[1] eq "" || defined($cmd[2]));
 			mw_import($cmd[1]);
 		} elsif ($cmd[0] eq "option") {
-			die("Too many arguments for option\n") unless ($cmd[1] ne "" && $cmd[2] ne "" && !defined($cmd[3]));
+			die("Too many arguments for option\n") if ($cmd[1] eq "" || $cmd[2] eq "" || defined($cmd[3]));
 			mw_option($cmd[1],$cmd[2]);
 		} elsif ($cmd[0] eq "push") {
 			mw_push($cmd[1]);
-- 
1.7.9.5

Re: [PATCH 05/18] Turn double-negated expressions into simple expressions

[Eric Sunshine]

On Thu, Jun 6, 2013 at 3:34 PM, Célestin Matte
<celestin.matte@ensimag.fr> wrote:
> Signed-off-by: Célestin Matte <celestin.matte@ensimag.fr>
> Signed-off-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
> ---
>  contrib/mw-to-git/git-remote-mediawiki.perl |    8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/contrib/mw-to-git/git-remote-mediawiki.perl b/contrib/mw-to-git/git-remote-mediawiki.perl
> index 68fd129..a6c7de2 100755
> --- a/contrib/mw-to-git/git-remote-mediawiki.perl
> +++ b/contrib/mw-to-git/git-remote-mediawiki.perl
> @@ -136,16 +136,16 @@ while (<STDIN>) {
>         if (defined($cmd[0])) {
>                 # Line not blank
>                 if ($cmd[0] eq "capabilities") {
> -                       die("Too many arguments for capabilities\n") unless (!defined($cmd[1]));
> +                       die("Too many arguments for capabilities\n") if (defined($cmd[1]));
>                         mw_capabilities();
>                 } elsif ($cmd[0] eq "list") {
> -                       die("Too many arguments for list\n") unless (!defined($cmd[2]));
> +                       die("Too many arguments for list\n") if (defined($cmd[2]));
>                         mw_list($cmd[1]);
>                 } elsif ($cmd[0] eq "import") {
> -                       die("Invalid arguments for import\n") unless ($cmd[1] ne "" && !defined($cmd[2]));
> +                       die("Invalid arguments for import\n") if ($cmd[1] eq "" || defined($cmd[2]));
>                         mw_import($cmd[1]);
>                 } elsif ($cmd[0] eq "option") {
> -                       die("Too many arguments for option\n") unless ($cmd[1] ne "" && $cmd[2] ne "" && !defined($cmd[3]));
> +                       die("Too many arguments for option\n") if ($cmd[1] eq "" || $cmd[2] eq "" || defined($cmd[3]));

Not new in this patch, but isn't this diagnostic misleading? It will
(falsely) claim "too many arguments" if $cmd[1] or $cmd[2] is an empty
string. Perhaps it should be reworded like the 'import' diagnostic and
say "Invalid arguments for option".

>                         mw_option($cmd[1],$cmd[2]);
>                 } elsif ($cmd[0] eq "push") {
>                         mw_push($cmd[1]);
> --

Re: [PATCH 05/18] Turn double-negated expressions into simple expressions

[Célestin Matte]

Le 07/06/2013 06:12, Eric Sunshine a écrit :
> On Thu, Jun 6, 2013 at 3:34 PM, Célestin Matte
> <celestin.matte@ensimag.fr> wrote:
>> Signed-off-by: Célestin Matte <celestin.matte@ensimag.fr>
>> Signed-off-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
>> ---
>>  contrib/mw-to-git/git-remote-mediawiki.perl |    8 ++++----
>>  1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/contrib/mw-to-git/git-remote-mediawiki.perl b/contrib/mw-to-git/git-remote-mediawiki.perl
>> index 68fd129..a6c7de2 100755
>> --- a/contrib/mw-to-git/git-remote-mediawiki.perl
>> +++ b/contrib/mw-to-git/git-remote-mediawiki.perl
>> @@ -136,16 +136,16 @@ while (<STDIN>) {
>>         if (defined($cmd[0])) {
>>                 # Line not blank
>>                 if ($cmd[0] eq "capabilities") {
>> -                       die("Too many arguments for capabilities\n") unless (!defined($cmd[1]));
>> +                       die("Too many arguments for capabilities\n") if (defined($cmd[1]));
>>                         mw_capabilities();
>>                 } elsif ($cmd[0] eq "list") {
>> -                       die("Too many arguments for list\n") unless (!defined($cmd[2]));
>> +                       die("Too many arguments for list\n") if (defined($cmd[2]));
>>                         mw_list($cmd[1]);
>>                 } elsif ($cmd[0] eq "import") {
>> -                       die("Invalid arguments for import\n") unless ($cmd[1] ne "" && !defined($cmd[2]));
>> +                       die("Invalid arguments for import\n") if ($cmd[1] eq "" || defined($cmd[2]));
>>                         mw_import($cmd[1]);
>>                 } elsif ($cmd[0] eq "option") {
>> -                       die("Too many arguments for option\n") unless ($cmd[1] ne "" && $cmd[2] ne "" && !defined($cmd[3]));
>> +                       die("Too many arguments for option\n") if ($cmd[1] eq "" || $cmd[2] eq "" || defined($cmd[3]));
> 
> Not new in this patch, but isn't this diagnostic misleading? It will
> (falsely) claim "too many arguments" if $cmd[1] or $cmd[2] is an empty
> string. Perhaps it should be reworded like the 'import' diagnostic and
> say "Invalid arguments for option".

We could even be more precise and separate the cases, i.e., die("Too
many arguments") when too many arguments are defined and die("Invalid
arguments") when there are empty strings.
Not sure if I should integrate it in this patch, though.


-- 
Célestin Matte

Re: [PATCH 05/18] Turn double-negated expressions into simple expressions

[Eric Sunshine]

On Fri, Jun 7, 2013 at 1:04 PM, Célestin Matte
<celestin.matte@ensimag.fr> wrote:
> Le 07/06/2013 06:12, Eric Sunshine a écrit :
>> On Thu, Jun 6, 2013 at 3:34 PM, Célestin Matte
>> <celestin.matte@ensimag.fr> wrote:
>>>                 } elsif ($cmd[0] eq "import") {
>>> -                       die("Invalid arguments for import\n") unless ($cmd[1] ne "" && !defined($cmd[2]));
>>> +                       die("Invalid arguments for import\n") if ($cmd[1] eq "" || defined($cmd[2]));
>>>                         mw_import($cmd[1]);
>>>                 } elsif ($cmd[0] eq "option") {
>>> -                       die("Too many arguments for option\n") unless ($cmd[1] ne "" && $cmd[2] ne "" && !defined($cmd[3]));
>>> +                       die("Too many arguments for option\n") if ($cmd[1] eq "" || $cmd[2] eq "" || defined($cmd[3]));
>>
>> Not new in this patch, but isn't this diagnostic misleading? It will
>> (falsely) claim "too many arguments" if $cmd[1] or $cmd[2] is an empty
>> string. Perhaps it should be reworded like the 'import' diagnostic and
>> say "Invalid arguments for option".
>
> We could even be more precise and separate the cases, i.e., die("Too
> many arguments") when too many arguments are defined and die("Invalid
> arguments") when there are empty strings.
> Not sure if I should integrate it in this patch, though.

If you do choose to be more precise, it should be done as a separate
patch. Each conceptually distinct change should have its own patch.
Doing so makes changes easier to review and (generally) easier to
cherry-pick. For example, in this particular case, "simplify
doubly-negated expressions" is quite conceptually distinct from "emit
more precise diagnostics". (Textually the changes may happen to
overlap, but conceptually they are unrelated.)

Re: [PATCH 05/18] Turn double-negated expressions into simple expressions

[Célestin Matte]

Le 07/06/2013 22:25, Eric Sunshine a écrit :
> If you do choose to be more precise, it should be done as a separate
> patch. Each conceptually distinct change should have its own patch.
> Doing so makes changes easier to review and (generally) easier to
> cherry-pick. For example, in this particular case, "simplify
> doubly-negated expressions" is quite conceptually distinct from "emit
> more precise diagnostics". (Textually the changes may happen to
> overlap, but conceptually they are unrelated.)

OK, I will do another patch.

-- 
Célestin Matte

[PATCH 06/18] Remove unused variable

[Célestin Matte]

Signed-off-by: Célestin Matte <celestin.matte@ensimag.fr>
Signed-off-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
---
 contrib/mw-to-git/git-remote-mediawiki.perl |    1 -
 1 file changed, 1 deletion(-)

diff --git a/contrib/mw-to-git/git-remote-mediawiki.perl b/contrib/mw-to-git/git-remote-mediawiki.perl
index a6c7de2..cf8dfc8 100755
--- a/contrib/mw-to-git/git-remote-mediawiki.perl
+++ b/contrib/mw-to-git/git-remote-mediawiki.perl
@@ -128,7 +128,6 @@ $wiki_name =~ s{[^/]*://}{};
 $wiki_name =~ s/^.*@//;
 
 # Commands parser
-my $entry;
 my @cmd;
 while (<STDIN>) {
 	chomp;
-- 
1.7.9.5

[PATCH 07/18] Rename a variable ($last) so that it does not have the name of a keyword

[Célestin Matte]

Signed-off-by: Célestin Matte <celestin.matte@ensimag.fr>
Signed-off-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
---
 contrib/mw-to-git/git-remote-mediawiki.perl |    8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/contrib/mw-to-git/git-remote-mediawiki.perl b/contrib/mw-to-git/git-remote-mediawiki.perl
index cf8dfc8..7fbc998 100755
--- a/contrib/mw-to-git/git-remote-mediawiki.perl
+++ b/contrib/mw-to-git/git-remote-mediawiki.perl
@@ -215,11 +215,11 @@ sub get_mw_page_list {
 	my $pages = shift;
 	my @some_pages = @$page_list;
 	while (@some_pages) {
-		my $last = 50;
-		if ($#some_pages < $last) {
-			$last = $#some_pages;
+		my $last_page = 50;
+		if ($#some_pages < $last_page) {
+			$last_page = $#some_pages;
 		}
-		my @slice = @some_pages[0..$last];
+		my @slice = @some_pages[0..$last_page];
 		get_mw_first_pages(\@slice, $pages);
 		@some_pages = @some_pages[51..$#some_pages];
 	}
-- 
1.7.9.5

[PATCH 08/18] Explicitely assign local variable as undef and make a proper one-instruction-by- line indentation

[Célestin Matte]

Signed-off-by: Célestin Matte <celestin.matte@ensimag.fr>
Signed-off-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
---
 contrib/mw-to-git/git-remote-mediawiki.perl |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/contrib/mw-to-git/git-remote-mediawiki.perl b/contrib/mw-to-git/git-remote-mediawiki.perl
index 7fbc998..ae6dd2e 100755
--- a/contrib/mw-to-git/git-remote-mediawiki.perl
+++ b/contrib/mw-to-git/git-remote-mediawiki.perl
@@ -339,7 +339,10 @@ sub run_git {
 	my $args = shift;
 	my $encoding = (shift || "encoding(UTF-8)");
 	open(my $git, "-|:$encoding", "git " . $args);
-	my $res = do { local $/; <$git> };
+	my $res = do { 
+		local $/ = undef;
+		<$git>
+	};
 	close($git);
 
 	return $res;
-- 
1.7.9.5

Re: [PATCH 08/18] Explicitely assign local variable as undef and make a proper one-instruction-by- line indentation

[Eric Sunshine]

On Thu, Jun 6, 2013 at 3:34 PM, Célestin Matte
<celestin.matte@ensimag.fr> wrote:
> [PATCH 08/18] Explicitely assign local variable as undef and make a proper one-instruction-by- line indentation

s/Explicitely/Explicitly/
s/by- /by-/

> Signed-off-by: Célestin Matte <celestin.matte@ensimag.fr>
> Signed-off-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>

Re: [PATCH 08/18] Explicitely assign local variable as undef and make a proper one-instruction-by- line indentation

[Matthieu Moy]

Célestin Matte <celestin.matte@ensimag.fr> writes:

> Subject: [PATCH 08/18] Explicitely assign local variable as undef and make a proper one-instruction-by- line indentation

Try to keep short subject lines. We usually consider that 80 character
is a strict maximum, and to have e.g. "git log --oneline"'s output fit
on 80 column terminal, we try to stick to 50 chars (man git-commit
documents that).

The subject line can say "what" the patch does, and the "why" can be
deferred to the body (after the blank line in the commit message).

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

[PATCH 09/18] Check return value of open and remove import of unused open2

[Célestin Matte]

Signed-off-by: Célestin Matte <celestin.matte@ensimag.fr>
Signed-off-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
---
 contrib/mw-to-git/git-remote-mediawiki.perl |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/contrib/mw-to-git/git-remote-mediawiki.perl b/contrib/mw-to-git/git-remote-mediawiki.perl
index ae6dd2e..1c34ada 100755
--- a/contrib/mw-to-git/git-remote-mediawiki.perl
+++ b/contrib/mw-to-git/git-remote-mediawiki.perl
@@ -22,7 +22,6 @@ binmode STDERR, ":encoding(UTF-8)";
 binmode STDOUT, ":encoding(UTF-8)";
 
 use URI::Escape;
-use IPC::Open2;
 use Readonly;
 
 # Mediawiki filenames can contain forward slashes. This variable decides by which pattern they should be replaced
@@ -338,7 +337,8 @@ sub get_mw_pages {
 sub run_git {
 	my $args = shift;
 	my $encoding = (shift || "encoding(UTF-8)");
-	open(my $git, "-|:$encoding", "git " . $args);
+	open(my $git, "-|:$encoding", "git " . $args)
+	    or die "Unable to open: $!\n";
 	my $res = do { 
 		local $/ = undef;
 		<$git>
-- 
1.7.9.5

Re: [PATCH 09/18] Check return value of open and remove import of unused open2

[Matthieu Moy]

Célestin Matte <celestin.matte@ensimag.fr> writes:

>  use URI::Escape;
> -use IPC::Open2;
>  use Readonly;

This should have belonged to bp/mediawiki-credential, but it's already
in next so it's OK to fix it here.

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

[PATCH 10/18] Put long code into a submodule

[Célestin Matte]

Signed-off-by: Célestin Matte <celestin.matte@ensimag.fr>
Signed-off-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
---
 contrib/mw-to-git/git-remote-mediawiki.perl |   44 ++++++++++++++++-----------
 1 file changed, 26 insertions(+), 18 deletions(-)

diff --git a/contrib/mw-to-git/git-remote-mediawiki.perl b/contrib/mw-to-git/git-remote-mediawiki.perl
index 1c34ada..1271527 100755
--- a/contrib/mw-to-git/git-remote-mediawiki.perl
+++ b/contrib/mw-to-git/git-remote-mediawiki.perl
@@ -133,24 +133,7 @@ while (<STDIN>) {
 	@cmd = split(/ /);
 	if (defined($cmd[0])) {
 		# Line not blank
-		if ($cmd[0] eq "capabilities") {
-			die("Too many arguments for capabilities\n") if (defined($cmd[1]));
-			mw_capabilities();
-		} elsif ($cmd[0] eq "list") {
-			die("Too many arguments for list\n") if (defined($cmd[2]));
-			mw_list($cmd[1]);
-		} elsif ($cmd[0] eq "import") {
-			die("Invalid arguments for import\n") if ($cmd[1] eq "" || defined($cmd[2]));
-			mw_import($cmd[1]);
-		} elsif ($cmd[0] eq "option") {
-			die("Too many arguments for option\n") if ($cmd[1] eq "" || $cmd[2] eq "" || defined($cmd[3]));
-			mw_option($cmd[1],$cmd[2]);
-		} elsif ($cmd[0] eq "push") {
-			mw_push($cmd[1]);
-		} else {
-			print STDERR "Unknown command. Aborting...\n";
-			last;
-		}
+		parse_commands();
 	} else {
 		# blank line: we should terminate
 		last;
@@ -168,6 +151,31 @@ sub exit_error_usage {
     die "ERROR: git-remote-mediawiki module was not called with a correct number of parameters\n";
 }
 
+sub parse_commands {
+	if ($cmd[0] eq "capabilities") {
+		die("Too many arguments for capabilities\n")
+		    if (defined($cmd[1]));
+		mw_capabilities();
+	} elsif ($cmd[0] eq "list") {
+		die("Too many arguments for list\n") if (defined($cmd[2]));
+		mw_list($cmd[1]);
+	} elsif ($cmd[0] eq "import") {
+		die("Invalid arguments for import\n")
+		    if ($cmd[1] eq "" || defined($cmd[2]));
+		mw_import($cmd[1]);
+	} elsif ($cmd[0] eq "option") {
+		die("Too many arguments for option\n")
+		    if ($cmd[1] eq "" || $cmd[2] eq "" || defined($cmd[3]));
+		mw_option($cmd[1],$cmd[2]);
+	} elsif ($cmd[0] eq "push") {
+		mw_push($cmd[1]);
+	} else {
+		print STDERR "Unknown command. Aborting...\n";
+		last;
+	}
+	return;
+}
+
 # MediaWiki API instance, created lazily.
 my $mediawiki;
 
-- 
1.7.9.5

Re: [PATCH 10/18] Put long code into a submodule

[Eric Sunshine]

On Thu, Jun 6, 2013 at 3:34 PM, Célestin Matte
<celestin.matte@ensimag.fr> wrote:
> Signed-off-by: Célestin Matte <celestin.matte@ensimag.fr>
> Signed-off-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
> ---
>  contrib/mw-to-git/git-remote-mediawiki.perl |   44 ++++++++++++++++-----------
>  1 file changed, 26 insertions(+), 18 deletions(-)
>
> diff --git a/contrib/mw-to-git/git-remote-mediawiki.perl b/contrib/mw-to-git/git-remote-mediawiki.perl
> index 1c34ada..1271527 100755
> --- a/contrib/mw-to-git/git-remote-mediawiki.perl
> +++ b/contrib/mw-to-git/git-remote-mediawiki.perl
> @@ -133,24 +133,7 @@ while (<STDIN>) {
>         @cmd = split(/ /);
>         if (defined($cmd[0])) {
>                 # Line not blank
> -               if ($cmd[0] eq "capabilities") {
> -                       die("Too many arguments for capabilities\n") if (defined($cmd[1]));
> -                       mw_capabilities();
> -               } elsif ($cmd[0] eq "list") {
> -                       die("Too many arguments for list\n") if (defined($cmd[2]));
> -                       mw_list($cmd[1]);
> -               } elsif ($cmd[0] eq "import") {
> -                       die("Invalid arguments for import\n") if ($cmd[1] eq "" || defined($cmd[2]));
> -                       mw_import($cmd[1]);
> -               } elsif ($cmd[0] eq "option") {
> -                       die("Too many arguments for option\n") if ($cmd[1] eq "" || $cmd[2] eq "" || defined($cmd[3]));
> -                       mw_option($cmd[1],$cmd[2]);
> -               } elsif ($cmd[0] eq "push") {
> -                       mw_push($cmd[1]);
> -               } else {
> -                       print STDERR "Unknown command. Aborting...\n";
> -                       last;
> -               }
> +               parse_commands();
>         } else {
>                 # blank line: we should terminate
>                 last;
> @@ -168,6 +151,31 @@ sub exit_error_usage {
>      die "ERROR: git-remote-mediawiki module was not called with a correct number of parameters\n";
>  }
>
> +sub parse_commands {
> +       if ($cmd[0] eq "capabilities") {
> +               die("Too many arguments for capabilities\n")
> +                   if (defined($cmd[1]));
> +               mw_capabilities();
> +       } elsif ($cmd[0] eq "list") {
> +               die("Too many arguments for list\n") if (defined($cmd[2]));
> +               mw_list($cmd[1]);
> +       } elsif ($cmd[0] eq "import") {
> +               die("Invalid arguments for import\n")
> +                   if ($cmd[1] eq "" || defined($cmd[2]));
> +               mw_import($cmd[1]);
> +       } elsif ($cmd[0] eq "option") {
> +               die("Too many arguments for option\n")
> +                   if ($cmd[1] eq "" || $cmd[2] eq "" || defined($cmd[3]));
> +               mw_option($cmd[1],$cmd[2]);
> +       } elsif ($cmd[0] eq "push") {
> +               mw_push($cmd[1]);
> +       } else {
> +               print STDERR "Unknown command. Aborting...\n";
> +               last;

In the original code, 'last' lived directly in the enclosing 'while'
loop, but after this patch, it is inside parse_commands(). With 'use
warnings' enabled, you are going to see an "Exiting subroutine via
last" diagnostic.

> +       }
> +       return;
> +}
> +
>  # MediaWiki API instance, created lazily.
>  my $mediawiki;

Re: [PATCH 10/18] Put long code into a submodule

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> On Thu, Jun 6, 2013 at 3:34 PM, Célestin Matte
> <celestin.matte@ensimag.fr> wrote:
>> Signed-off-by: Célestin Matte <celestin.matte@ensimag.fr>
>> Signed-off-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
>> ---

s/into a submodule/into a sub/; or "subroutine".

>>  contrib/mw-to-git/git-remote-mediawiki.perl |   44 ++++++++++++++++-----------
>>  1 file changed, 26 insertions(+), 18 deletions(-)
>>
>> diff --git a/contrib/mw-to-git/git-remote-mediawiki.perl b/contrib/mw-to-git/git-remote-mediawiki.perl
>> index 1c34ada..1271527 100755
>> --- a/contrib/mw-to-git/git-remote-mediawiki.perl
>> +++ b/contrib/mw-to-git/git-remote-mediawiki.perl
>> @@ -133,24 +133,7 @@ while (<STDIN>) {
>>         @cmd = split(/ /);
>>         if (defined($cmd[0])) {
>>                 # Line not blank
>> -               if ($cmd[0] eq "capabilities") {
>> -                       die("Too many arguments for capabilities\n") if (defined($cmd[1]));
>> -                       mw_capabilities();
>> -               } elsif ($cmd[0] eq "list") {
>> -                       die("Too many arguments for list\n") if (defined($cmd[2]));
>> -                       mw_list($cmd[1]);
>> -               } elsif ($cmd[0] eq "import") {
>> -                       die("Invalid arguments for import\n") if ($cmd[1] eq "" || defined($cmd[2]));
>> -                       mw_import($cmd[1]);
>> -               } elsif ($cmd[0] eq "option") {
>> -                       die("Too many arguments for option\n") if ($cmd[1] eq "" || $cmd[2] eq "" || defined($cmd[3]));
>> -                       mw_option($cmd[1],$cmd[2]);
>> -               } elsif ($cmd[0] eq "push") {
>> -                       mw_push($cmd[1]);
>> -               } else {
>> -                       print STDERR "Unknown command. Aborting...\n";
>> -                       last;
>> -               }
>> +               parse_commands();
>>         } else {
>>                 # blank line: we should terminate
>>                 last;
>> @@ -168,6 +151,31 @@ sub exit_error_usage {
>>      die "ERROR: git-remote-mediawiki module was not called with a correct number of parameters\n";
>>  }
>>
>> +sub parse_commands {
>> +       if ($cmd[0] eq "capabilities") {
>> +               die("Too many arguments for capabilities\n")
>> +                   if (defined($cmd[1]));
>> +               mw_capabilities();
>> +       } elsif ($cmd[0] eq "list") {
>> +               die("Too many arguments for list\n") if (defined($cmd[2]));
>> +               mw_list($cmd[1]);
>> +       } elsif ($cmd[0] eq "import") {
>> +               die("Invalid arguments for import\n")
>> +                   if ($cmd[1] eq "" || defined($cmd[2]));
>> +               mw_import($cmd[1]);
>> +       } elsif ($cmd[0] eq "option") {
>> +               die("Too many arguments for option\n")
>> +                   if ($cmd[1] eq "" || $cmd[2] eq "" || defined($cmd[3]));
>> +               mw_option($cmd[1],$cmd[2]);
>> +       } elsif ($cmd[0] eq "push") {
>> +               mw_push($cmd[1]);
>> +       } else {
>> +               print STDERR "Unknown command. Aborting...\n";
>> +               last;
>
> In the original code, 'last' lived directly in the enclosing 'while'
> loop, but after this patch, it is inside parse_commands(). With 'use
> warnings' enabled, you are going to see an "Exiting subroutine via
> last" diagnostic.
>
>> +       }
>> +       return;
>> +}
>> +
>>  # MediaWiki API instance, created lazily.
>>  my $mediawiki;

[PATCH 11/18] Modify strings for a better coding-style

[Célestin Matte]

- strings which don't need interpolation are single-quoted for more clarity and
slight gain of performance
- interpolation is preferred over concatenation in many cases, for more clarity
- variables are always used with the ${} operator inside strings
- strings including double-quotes are written with qq() so that the quotes do
not have to be escaped

Signed-off-by: Célestin Matte <celestin.matte@ensimag.fr>
Signed-off-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
---
 contrib/mw-to-git/git-remote-mediawiki.perl |  250 +++++++++++++--------------
 1 file changed, 124 insertions(+), 126 deletions(-)

diff --git a/contrib/mw-to-git/git-remote-mediawiki.perl b/contrib/mw-to-git/git-remote-mediawiki.perl
index 1271527..0e2152d 100755
--- a/contrib/mw-to-git/git-remote-mediawiki.perl
+++ b/contrib/mw-to-git/git-remote-mediawiki.perl
@@ -18,14 +18,14 @@ use DateTime::Format::ISO8601;
 use warnings;
 
 # By default, use UTF-8 to communicate with Git and the user
-binmode STDERR, ":encoding(UTF-8)";
-binmode STDOUT, ":encoding(UTF-8)";
+binmode STDERR, ':encoding(UTF-8)';
+binmode STDOUT, ':encoding(UTF-8)';
 
 use URI::Escape;
 use Readonly;
 
 # Mediawiki filenames can contain forward slashes. This variable decides by which pattern they should be replaced
-Readonly my $SLASH_REPLACEMENT => "%2F";
+Readonly my $SLASH_REPLACEMENT => '%2F';
 
 # It's not always possible to delete pages (may require some
 # privileges). Deleted pages are replaced with this content.
@@ -36,7 +36,7 @@ Readonly my $DELETED_CONTENT => "[[Category:Deleted]]\n";
 Readonly my $EMPTY_CONTENT => "<!-- empty page -->\n";
 
 # used to reflect file creation or deletion in diff.
-Readonly my $NULL_SHA1 => "0000000000000000000000000000000000000000";
+Readonly my $NULL_SHA1 => '0000000000000000000000000000000000000000';
 
 # Used on Git's side to reflect empty edit messages on the wiki
 Readonly my $EMPTY_MESSAGE => '*Empty MediaWiki Message*';
@@ -50,35 +50,35 @@ my $url = $ARGV[1];
 
 # Accept both space-separated and multiple keys in config file.
 # Spaces should be written as _ anyway because we'll use chomp.
-my @tracked_pages = split(/[ \n]/, run_git("config --get-all remote.". $remotename .".pages"));
+my @tracked_pages = split(/[ \n]/, run_git("config --get-all remote.${remotename}.pages"));
 chomp(@tracked_pages);
 
 # Just like @tracked_pages, but for MediaWiki categories.
-my @tracked_categories = split(/[ \n]/, run_git("config --get-all remote.". $remotename .".categories"));
+my @tracked_categories = split(/[ \n]/, run_git("config --get-all remote.${remotename}.categories"));
 chomp(@tracked_categories);
 
 # Import media files on pull
-my $import_media = run_git("config --get --bool remote.". $remotename .".mediaimport");
+my $import_media = run_git("config --get --bool remote.${remotename}.mediaimport");
 chomp($import_media);
-$import_media = ($import_media eq "true");
+$import_media = ($import_media eq 'true');
 
 # Export media files on push
-my $export_media = run_git("config --get --bool remote.". $remotename .".mediaexport");
+my $export_media = run_git("config --get --bool remote.${remotename}.mediaexport");
 chomp($export_media);
-$export_media = !($export_media eq "false");
+$export_media = !($export_media eq 'false');
 
-my $wiki_login = run_git("config --get remote.". $remotename .".mwLogin");
+my $wiki_login = run_git("config --get remote.${remotename}.mwLogin");
 # Note: mwPassword is discourraged. Use the credential system instead.
-my $wiki_passwd = run_git("config --get remote.". $remotename .".mwPassword");
-my $wiki_domain = run_git("config --get remote.". $remotename .".mwDomain");
+my $wiki_passwd = run_git("config --get remote.${remotename}.mwPassword");
+my $wiki_domain = run_git("config --get remote.${remotename}.mwDomain");
 chomp($wiki_login);
 chomp($wiki_passwd);
 chomp($wiki_domain);
 
 # Import only last revisions (both for clone and fetch)
-my $shallow_import = run_git("config --get --bool remote.". $remotename .".shallow");
+my $shallow_import = run_git("config --get --bool remote.${remotename}.shallow");
 chomp($shallow_import);
-$shallow_import = ($shallow_import eq "true");
+$shallow_import = ($shallow_import eq 'true');
 
 # Fetch (clone and pull) by revisions instead of by pages. This behavior
 # is more efficient when we have a wiki with lots of pages and we fetch
@@ -86,13 +86,13 @@ $shallow_import = ($shallow_import eq "true");
 # Possible values:
 # - by_rev: perform one query per new revision on the remote wiki
 # - by_page: query each tracked page for new revision
-my $fetch_strategy = run_git("config --get remote.$remotename.fetchStrategy");
+my $fetch_strategy = run_git("config --get remote.${remotename}.fetchStrategy");
 unless ($fetch_strategy) {
-	$fetch_strategy = run_git("config --get mediawiki.fetchStrategy");
+	$fetch_strategy = run_git('config --get mediawiki.fetchStrategy');
 }
 chomp($fetch_strategy);
 unless ($fetch_strategy) {
-	$fetch_strategy = "by_page";
+	$fetch_strategy = 'by_page';
 }
 
 # Remember the timestamp corresponding to a revision id.
@@ -112,12 +112,12 @@ my %basetimestamps;
 # will get the history with information lost). If the import is
 # deterministic, this means everybody gets the same sha1 for each
 # MediaWiki revision.
-my $dumb_push = run_git("config --get --bool remote.$remotename.dumbPush");
+my $dumb_push = run_git("config --get --bool remote.${remotename}.dumbPush");
 unless ($dumb_push) {
-	$dumb_push = run_git("config --get --bool mediawiki.dumbPush");
+	$dumb_push = run_git('config --get --bool mediawiki.dumbPush');
 }
 chomp($dumb_push);
-$dumb_push = ($dumb_push eq "true");
+$dumb_push = ($dumb_push eq 'true');
 
 my $wiki_name = $url;
 $wiki_name =~ s{[^/]*://}{};
@@ -152,22 +152,22 @@ sub exit_error_usage {
 }
 
 sub parse_commands {
-	if ($cmd[0] eq "capabilities") {
+	if ($cmd[0] eq 'capabilities') {
 		die("Too many arguments for capabilities\n")
 		    if (defined($cmd[1]));
 		mw_capabilities();
-	} elsif ($cmd[0] eq "list") {
+	} elsif ($cmd[0] eq 'list') {
 		die("Too many arguments for list\n") if (defined($cmd[2]));
 		mw_list($cmd[1]);
-	} elsif ($cmd[0] eq "import") {
+	} elsif ($cmd[0] eq 'import') {
 		die("Invalid arguments for import\n")
 		    if ($cmd[1] eq "" || defined($cmd[2]));
 		mw_import($cmd[1]);
-	} elsif ($cmd[0] eq "option") {
+	} elsif ($cmd[0] eq 'option') {
 		die("Too many arguments for option\n")
 		    if ($cmd[1] eq "" || $cmd[2] eq "" || defined($cmd[3]));
 		mw_option($cmd[1],$cmd[2]);
-	} elsif ($cmd[0] eq "push") {
+	} elsif ($cmd[0] eq 'push') {
 		mw_push($cmd[1]);
 	} else {
 		print STDERR "Unknown command. Aborting...\n";
@@ -184,7 +184,7 @@ sub mw_connect_maybe {
 		return;
 	}
 	$mediawiki = MediaWiki::API->new;
-	$mediawiki->{config}->{api_url} = "$url/api.php";
+	$mediawiki->{config}->{api_url} = "${url}/api.php";
 	if ($wiki_login) {
 		my %credential = (
 			'url' => $url,
@@ -197,10 +197,10 @@ sub mw_connect_maybe {
 			       lgdomain => $wiki_domain};
 		if ($mediawiki->login($request)) {
 			Git::credential \%credential, 'approve';
-			print STDERR "Logged in mediawiki user \"$credential{username}\".\n";
+			print STDERR qq(Logged in mediawiki user "$credential{username}".\n);
 		} else {
-			print STDERR "Failed to log in mediawiki user \"$credential{username}\" on $url\n";
-			print STDERR "  (error " .
+			print {*STDERR} qq(Failed to log in mediawiki user "$credential{username}" on ${url}\n);
+			print {*STDERR} '  (error ' .
 				$mediawiki->{error}->{code} . ': ' .
 				$mediawiki->{error}->{details} . ")\n";
 			Git::credential \%credential, 'reject';
@@ -240,7 +240,7 @@ sub get_mw_tracked_categories {
 			# Mediawiki requires the Category
 			# prefix, but let's not force the user
 			# to specify it.
-			$category = "Category:" . $category;
+			$category = "Category:${category}";
 		}
 		my $mw_pages = $mediawiki->list( {
 			action => 'query',
@@ -266,8 +266,8 @@ sub get_mw_all_pages {
 	});
 	if (!defined($mw_pages)) {
 		print STDERR "fatal: could not get the list of wiki pages.\n";
-		print STDERR "fatal: '$url' does not appear to be a mediawiki\n";
-		print STDERR "fatal: make sure '$url/api.php' is a valid page.\n";
+		print STDERR "fatal: '${url}' does not appear to be a mediawiki\n";
+		print STDERR "fatal: make sure '${url}/api.php' is a valid page.\n";
 		exit 1;
 	}
 	foreach my $page (@{$mw_pages}) {
@@ -293,8 +293,8 @@ sub get_mw_first_pages {
 	});
 	if (!defined($mw_pages)) {
 		print STDERR "fatal: could not query the list of wiki pages.\n";
-		print STDERR "fatal: '$url' does not appear to be a mediawiki\n";
-		print STDERR "fatal: make sure '$url/api.php' is a valid page.\n";
+		print STDERR "fatal: '${url}' does not appear to be a mediawiki\n";
+		print STDERR "fatal: make sure '${url}/api.php' is a valid page.\n";
 		exit 1;
 	}
 	while (my ($id, $page) = each(%{$mw_pages->{query}->{pages}})) {
@@ -344,10 +344,10 @@ sub get_mw_pages {
 #        $out = run_git("command args", "raw"); # don't interpret output as UTF-8.
 sub run_git {
 	my $args = shift;
-	my $encoding = (shift || "encoding(UTF-8)");
-	open(my $git, "-|:$encoding", "git " . $args)
-	    or die "Unable to open: $!\n";
-	my $res = do { 
+	my $encoding = (shift || 'encoding(UTF-8)');
+	my $res = do {
+		open(my $git, "-|:$encoding", "git ${args}")
+		    or die "Unable to fork: $!\n";
 		local $/ = undef;
 		<$git>
 	};
@@ -365,7 +365,7 @@ sub get_all_mediafiles {
 	my $mw_pages = $mediawiki->list({
 		action => 'query',
 		list => 'allpages',
-		apnamespace => get_mw_namespace_id("File"),
+		apnamespace => get_mw_namespace_id('File'),
 		aplimit => 'max'
 	});
 	if (!defined($mw_pages)) {
@@ -402,7 +402,7 @@ sub get_linked_mediafiles {
 			action => 'query',
 			prop => 'links|images',
 			titles => $mw_titles,
-			plnamespace => get_mw_namespace_id("File"),
+			plnamespace => get_mw_namespace_id('File'),
 			pllimit => 'max'
 		};
 		my $result = $mediawiki->api($query);
@@ -440,7 +440,7 @@ sub get_mw_mediafile_for_page_revision {
 	my $query = {
 		action => 'query',
 		prop => 'imageinfo',
-		titles => "File:" . $filename,
+		titles => "File:${filename}",
 		iistart => $timestamp,
 		iiend => $timestamp,
 		iiprop => 'timestamp|archivename|url',
@@ -471,27 +471,27 @@ sub download_mw_mediafile {
 	if ($response->code == 200) {
 		return $response->decoded_content;
 	} else {
-		print STDERR "Error downloading mediafile from :\n";
-		print STDERR "URL: $download_url\n";
-		print STDERR "Server response: " . $response->code . " " . $response->message . "\n";
+		print {*STDERR} "Error downloading mediafile from :\n";
+		print {*STDERR} "URL: ${download_url}\n";
+		print {*STDERR} 'Server response: ' . $response->code . q{ } . $response->message . "\n";
 		exit 1;
 	}
 }
 
 sub get_last_local_revision {
 	# Get note regarding last mediawiki revision
-	my $note = run_git("notes --ref=$remotename/mediawiki show refs/mediawiki/$remotename/master 2>/dev/null");
+	my $note = run_git("notes --ref=${remotename}/mediawiki show refs/mediawiki/${remotename}/master 2>/dev/null");
 	my @note_info = split(/ /, $note);
 
 	my $lastrevision_number;
-	if (!(defined($note_info[0]) && $note_info[0] eq "mediawiki_revision:")) {
-		print STDERR "No previous mediawiki revision found";
+	if (!(defined($note_info[0]) && $note_info[0] eq 'mediawiki_revision:')) {
+		print STDERR 'No previous mediawiki revision found';
 		$lastrevision_number = 0;
 	} else {
 		# Notes are formatted : mediawiki_revision: #number
 		$lastrevision_number = $note_info[1];
 		chomp($lastrevision_number);
-		print STDERR "Last local mediawiki revision found is $lastrevision_number";
+		print STDERR "Last local mediawiki revision found is ${lastrevision_number}";
 	}
 	return $lastrevision_number;
 }
@@ -570,7 +570,7 @@ sub mediawiki_smudge {
 		$string = "";
 	}
 	# This \n is important. This is due to mediawiki's way to handle end of files.
-	return $string."\n";
+	return "${string}\n";
 }
 
 sub mediawiki_clean_filename {
@@ -592,13 +592,13 @@ sub mediawiki_smudge_filename {
 	$filename =~ s{/}{$SLASH_REPLACEMENT}g;
 	$filename =~ s/ /_/g;
 	# Decode forbidden characters encoded in mediawiki_clean_filename
-	$filename =~ s/_%_([0-9a-fA-F][0-9a-fA-F])/sprintf("%c", hex($1))/ge;
+	$filename =~ s/_%_([0-9a-fA-F][0-9a-fA-F])/sprintf('%c', hex($1))/ge;
 	return $filename;
 }
 
 sub literal_data {
 	my ($content) = @_;
-	print STDOUT "data ", bytes::length($content), "\n", $content;
+	print STDOUT 'data ', bytes::length($content), "\n", $content;
 	return;
 }
 
@@ -607,9 +607,9 @@ sub literal_data_raw {
 	my ($content) = @_;
 	# Avoid confusion between size in bytes and in characters
 	utf8::downgrade($content);
-	binmode STDOUT, ":raw";
-	print STDOUT "data ", bytes::length($content), "\n", $content;
-	binmode STDOUT, ":encoding(UTF-8)";
+	binmode STDOUT, ':raw';
+	print STDOUT 'data ', bytes::length($content), "\n", $content;
+	binmode STDOUT, ':encoding(UTF-8)';
 	return;
 }
 
@@ -617,7 +617,7 @@ sub mw_capabilities {
 	# Revisions are imported to the private namespace
 	# refs/mediawiki/$remotename/ by the helper and fetched into
 	# refs/remotes/$remotename later by fetch.
-	print STDOUT "refspec refs/heads/*:refs/mediawiki/$remotename/*\n";
+	print STDOUT "refspec refs/heads/*:refs/mediawiki/${remotename}/*\n";
 	print STDOUT "import\n";
 	print STDOUT "list\n";
 	print STDOUT "push\n";
@@ -676,7 +676,7 @@ sub fetch_mw_revisions_for_page {
 		@page_revs = sort {$b->{revid} <=> $a->{revid}} (@page_revs);
 		return $page_revs[0];
 	}
-	print STDERR "  Found ", $revnum, " revision(s).\n";
+	print STDERR "  Found ${revnum} revision(s).\n";
 	return @page_revs;
 }
 
@@ -688,8 +688,7 @@ sub fetch_mw_revisions {
 	my $n = 1;
 	foreach my $page (@pages) {
 		my $id = $page->{pageid};
-
-		print STDERR "page $n/", scalar(@pages), ": ". $page->{title} ."\n";
+		print {*STDERR} "page ${n}/", scalar(@pages), ': ', $page->{title}, "\n";
 		$n++;
 		my @page_revs = fetch_mw_revisions_for_page($page, $id, $fetch_from);
 		@revisions = (@page_revs, @revisions);
@@ -703,7 +702,7 @@ sub fe_escape_path {
     $path =~ s/\\/\\\\/g;
     $path =~ s/"/\\"/g;
     $path =~ s/\n/\\n/g;
-    return '"' . $path . '"';
+    return qq("${path}");
 }
 
 sub import_file_revision {
@@ -723,41 +722,41 @@ sub import_file_revision {
 	my $author = $commit{author};
 	my $date = $commit{date};
 
-	print STDOUT "commit refs/mediawiki/$remotename/master\n";
-	print STDOUT "mark :$n\n";
-	print STDOUT "committer $author <$author\@$wiki_name> ", $date->epoch, " +0000\n";
+	print STDOUT "commit refs/mediawiki/${remotename}/master\n";
+	print STDOUT "mark :${n}\n";
+	print STDOUT "committer ${author} <${author}\@${wiki_name}> " . $date->epoch . " +0000\n";
 	literal_data($comment);
 
 	# If it's not a clone, we need to know where to start from
 	if (!$full_import && $n == 1) {
-		print STDOUT "from refs/mediawiki/$remotename/master^0\n";
+		print STDOUT "from refs/mediawiki/${remotename}/master^0\n";
 	}
 	if ($content ne $DELETED_CONTENT) {
-		print STDOUT "M 644 inline " .
-		    fe_escape_path($title . ".mw") . "\n";
+		print {*STDOUT} 'M 644 inline ' .
+		    fe_escape_path("${title}.mw") . "\n";
 		literal_data($content);
 		if (%mediafile) {
-			print STDOUT "M 644 inline "
+			print STDOUT 'M 644 inline '
 			    . fe_escape_path($mediafile{title}) . "\n";
 			literal_data_raw($mediafile{content});
 		}
 		print STDOUT "\n\n";
 	} else {
-		print STDOUT "D " . fe_escape_path($title . ".mw") . "\n";
+		print STDOUT 'D ' . fe_escape_path("${title}.mw") . "\n";
 	}
 
 	# mediawiki revision number in the git note
 	if ($full_import && $n == 1) {
-		print STDOUT "reset refs/notes/$remotename/mediawiki\n";
+		print STDOUT "reset refs/notes/${remotename}/mediawiki\n";
 	}
-	print STDOUT "commit refs/notes/$remotename/mediawiki\n";
-	print STDOUT "committer $author <$author\@$wiki_name> ", $date->epoch, " +0000\n";
-	literal_data("Note added by git-mediawiki during import");
+	print STDOUT "commit refs/notes/${remotename}/mediawiki\n";
+	print STDOUT "committer ${author} <${author}\@${wiki_name}> " . $date->epoch . " +0000\n";
+	literal_data('Note added by git-mediawiki during import');
 	if (!$full_import && $n == 1) {
-		print STDOUT "from refs/notes/$remotename/mediawiki^0\n";
+		print STDOUT "from refs/notes/${remotename}/mediawiki^0\n";
 	}
-	print STDOUT "N inline :$n\n";
-	literal_data("mediawiki_revision: " . $commit{mw_revision});
+	print STDOUT "N inline :${n}\n";
+	literal_data("mediawiki_revision: $commit{mw_revision}");
 	print STDOUT "\n\n";
 	return;
 }
@@ -785,7 +784,7 @@ sub get_more_refs {
 
 sub mw_import {
 	# multiple import commands can follow each other.
-	my @refs = (shift, get_more_refs("import"));
+	my @refs = (shift, get_more_refs('import'));
 	foreach my $ref (@refs) {
 		mw_import_ref($ref);
 	}
@@ -800,7 +799,7 @@ sub mw_import_ref {
 	# Since HEAD is a symbolic ref to master (by convention,
 	# followed by the output of the command "list" that we gave),
 	# we don't need to do anything in this case.
-	if ($ref eq "HEAD") {
+	if ($ref eq 'HEAD') {
 		return;
 	}
 
@@ -816,15 +815,15 @@ sub mw_import_ref {
 	}
 
 	my $n = 0;
-	if ($fetch_strategy eq "by_rev") {
+	if ($fetch_strategy eq 'by_rev') {
 		print STDERR "Fetching & writing export data by revs...\n";
 		$n = mw_import_ref_by_revs($fetch_from);
-	} elsif ($fetch_strategy eq "by_page") {
+	} elsif ($fetch_strategy eq 'by_page') {
 		print STDERR "Fetching & writing export data by pages...\n";
 		$n = mw_import_ref_by_pages($fetch_from);
 	} else {
-		print STDERR "fatal: invalid fetch strategy \"$fetch_strategy\".\n";
-		print STDERR "Check your configuration variables remote.$remotename.fetchStrategy and mediawiki.fetchStrategy\n";
+		print STDERR qq(fatal: invalid fetch strategy "${fetch_strategy}".\n);
+		print STDERR "Check your configuration variables remote.${remotename}.fetchStrategy and mediawiki.fetchStrategy\n";
 		exit 1;
 	}
 
@@ -898,7 +897,7 @@ sub mw_import_revids {
 		}
 
 		if (!defined($result->{query}->{pages})) {
-			die "Invalid revision $pagerevid.\n";
+			die "Invalid revision ${pagerevid}.\n";
 		}
 
 		my @result_pages = values(%{$result->{query}->{pages}});
@@ -908,8 +907,8 @@ sub mw_import_revids {
 		my $page_title = $result_page->{title};
 
 		if (!exists($pages->{$page_title})) {
-			print STDERR "$n/", scalar(@$revision_ids),
-				": Skipping revision #$rev->{revid} of $page_title\n";
+			print STDERR "${n}/", scalar(@$revision_ids),
+				": Skipping revision #$rev->{revid} of ${page_title}\n";
 			next;
 		}
 
@@ -934,14 +933,14 @@ sub mw_import_revids {
 		my %mediafile;
 		if ($namespace) {
 			my $id = get_mw_namespace_id($namespace);
-			if ($id && $id == get_mw_namespace_id("File")) {
+			if ($id && $id == get_mw_namespace_id('File')) {
 				%mediafile = get_mw_mediafile_for_page_revision($filename, $rev->{timestamp});
 			}
 		}
 		# If this is a revision of the media page for new version
 		# of a file do one common commit for both file and media page.
 		# Else do commit only for that page.
-		print STDERR "$n/", scalar(@$revision_ids), ": Revision #$rev->{revid} of $commit{title}\n";
+		print STDERR "${n}/", scalar(@$revision_ids), ": Revision #$rev->{revid} of $commit{title}\n";
 		import_file_revision(\%commit, ($fetch_from == 1), $n_actual, \%mediafile);
 	}
 
@@ -949,9 +948,9 @@ sub mw_import_revids {
 }
 
 sub error_non_fast_forward {
-	my $advice = run_git("config --bool advice.pushNonFastForward");
+	my $advice = run_git('config --bool advice.pushNonFastForward');
 	chomp($advice);
-	if ($advice ne "false") {
+	if ($advice ne 'false') {
 		# Native git-push would show this after the summary.
 		# We can't ask it to display it cleanly, so print it
 		# ourselves before.
@@ -959,7 +958,7 @@ sub error_non_fast_forward {
 		print STDERR "Merge the remote changes (e.g. 'git pull') before pushing again. See the\n";
 		print STDERR "'Note about fast-forwards' section of 'git push --help' for details.\n";
 	}
-	print STDOUT "error $_[0] \"non-fast-forward\"\n";
+	print STDOUT qq(error $_[0] "non-fast-forward"\n);
 	return 0;
 }
 
@@ -970,10 +969,10 @@ sub mw_upload_file {
 	my $file_deleted = shift;
 	my $summary = shift;
 	my $newrevid;
-	my $path = "File:" . $complete_file_name;
+	my $path = "File:${complete_file_name}";
 	my %hashFiles = get_allowed_file_extensions();
 	if (!exists($hashFiles{$extension})) {
-		print STDERR "$complete_file_name is not a permitted file on this wiki.\n";
+		print STDERR "${complete_file_name} is not a permitted file on this wiki.\n";
 		print STDERR "Check the configuration of file uploads in your mediawiki.\n";
 		return $newrevid;
 	}
@@ -993,11 +992,11 @@ sub mw_upload_file {
 		}
 	} else {
 		# Don't let perl try to interpret file content as UTF-8 => use "raw"
-		my $content = run_git("cat-file blob $new_sha1", "raw");
+		my $content = run_git("cat-file blob ${new_sha1}", 'raw');
 		if ($content ne "") {
 			mw_connect_maybe();
 			$mediawiki->{config}->{upload_url} =
-				"$url/index.php/Special:Upload";
+				"${url}/index.php/Special:Upload";
 			$mediawiki->edit({
 				action => 'upload',
 				filename => $complete_file_name,
@@ -1012,9 +1011,9 @@ sub mw_upload_file {
 				 . $mediawiki->{error}->{details} . "\n";
 			my $last_file_page = $mediawiki->get_page({title => $path});
 			$newrevid = $last_file_page->{revid};
-			print STDERR "Pushed file: $new_sha1 - $complete_file_name.\n";
+			print STDERR "Pushed file: ${new_sha1} - ${complete_file_name}.\n";
 		} else {
-			print STDERR "Empty file $complete_file_name not pushed.\n";
+			print STDERR "Empty file ${complete_file_name} not pushed.\n";
 		}
 	}
 	return $newrevid;
@@ -1049,11 +1048,11 @@ sub mw_push_file {
 	if (!defined($extension)) {
 		$extension = "";
 	}
-	if ($extension eq "mw") {
+	if ($extension eq 'mw') {
 		my $ns = get_mw_namespace_id_for_page($complete_file_name);
-		if ($ns && $ns == get_mw_namespace_id("File") && (!$export_media)) {
-			print STDERR "Ignoring media file related page: $complete_file_name\n";
-			return ($oldrevid, "ok");
+		if ($ns && $ns == get_mw_namespace_id('File') && (!$export_media)) {
+			print STDERR "Ignoring media file related page: ${complete_file_name}\n";
+			return ($oldrevid, 'ok');
 		}
 		my $file_content;
 		if ($page_deleted) {
@@ -1063,7 +1062,7 @@ sub mw_push_file {
 			# with this content instead:
 			$file_content = $DELETED_CONTENT;
 		} else {
-			$file_content = run_git("cat-file blob $new_sha1");
+			$file_content = run_git("cat-file blob ${new_sha1}");
 		}
 
 		mw_connect_maybe();
@@ -1084,7 +1083,7 @@ sub mw_push_file {
 				    $mediawiki->{error}->{code} .
 				    ' from mediwiki: ' . $mediawiki->{error}->{details} .
 				    ".\n";
-				return ($oldrevid, "non-fast-forward");
+				return ($oldrevid, 'non-fast-forward');
 			} else {
 				# Other errors. Shouldn't happen => just die()
 				die 'Fatal: Error ' .
@@ -1093,21 +1092,21 @@ sub mw_push_file {
 			}
 		}
 		$newrevid = $result->{edit}->{newrevid};
-		print STDERR "Pushed file: $new_sha1 - $title\n";
+		print STDERR "Pushed file: ${new_sha1} - ${title}\n";
 	} elsif ($export_media) {
 		$newrevid = mw_upload_file($complete_file_name, $new_sha1,
 					   $extension, $page_deleted,
 					   $summary);
 	} else {
-		print STDERR "Ignoring media file $title\n";
+		print STDERR "Ignoring media file ${title}\n";
 	}
 	$newrevid = ($newrevid or $oldrevid);
-	return ($newrevid, "ok");
+	return ($newrevid, 'ok');
 }
 
 sub mw_push {
 	# multiple push statements can follow each other
-	my @refsspecs = (shift, get_more_refs("push"));
+	my @refsspecs = (shift, get_more_refs('push'));
 	my $pushed;
 	for my $refspec (@refsspecs) {
 		my ($force, $local, $remote) = $refspec =~ /^(\+)?([^:]*):([^:]*)$/
@@ -1117,12 +1116,12 @@ sub mw_push {
 		}
 		if ($local eq "") {
 			print STDERR "Cannot delete remote branch on a MediaWiki\n";
-			print STDOUT "error $remote cannot delete\n";
+			print STDOUT "error ${remote} cannot delete\n";
 			next;
 		}
-		if ($remote ne "refs/heads/master") {
+		if ($remote ne 'refs/heads/master') {
 			print STDERR "Only push to the branch 'master' is supported on a MediaWiki\n";
-			print STDOUT "error $remote only master allowed\n";
+			print STDOUT "error ${remote} only master allowed\n";
 			next;
 		}
 		if (mw_push_revision($local, $remote)) {
@@ -1153,9 +1152,10 @@ sub mw_push_revision {
 	my $mw_revision = $last_remote_revid;
 
 	# Get sha1 of commit pointed by local HEAD
-	my $HEAD_sha1 = run_git("rev-parse $local 2>/dev/null"); chomp($HEAD_sha1);
+	my $HEAD_sha1 = run_git("rev-parse ${local} 2>/dev/null");
+	chomp($HEAD_sha1);
 	# Get sha1 of commit pointed by remotes/$remotename/master
-	my $remoteorigin_sha1 = run_git("rev-parse refs/remotes/$remotename/master 2>/dev/null");
+	my $remoteorigin_sha1 = run_git("rev-parse refs/remotes/${remotename}/master 2>/dev/null");
 	chomp($remoteorigin_sha1);
 
 	if ($last_local_revid > 0 &&
@@ -1175,7 +1175,7 @@ sub mw_push_revision {
 		my $parsed_sha1 = $remoteorigin_sha1;
 		# Find a path from last MediaWiki commit to pushed commit
 		print STDERR "Computing path from local to remote ...\n";
-		my @local_ancestry = split(/\n/, run_git("rev-list --boundary --parents $local ^$parsed_sha1"));
+		my @local_ancestry = split(/\n/, run_git("rev-list --boundary --parents ${local} ^${parsed_sha1}"));
 		my %local_ancestry;
 		foreach my $line (@local_ancestry) {
 			if (my ($child, $parents) = $line =~ /^-?([a-f0-9]+) ([a-f0-9 ]+)/) {
@@ -1183,7 +1183,7 @@ sub mw_push_revision {
 					$local_ancestry{$parent} = $child;
 				}
 			} elsif (!$line =~ /^([a-f0-9]+)/) {
-				die "Unexpected output from git rev-list: $line\n";
+				die "Unexpected output from git rev-list: ${line}\n";
 			}
 		}
 		while ($parsed_sha1 ne $HEAD_sha1) {
@@ -1199,7 +1199,7 @@ sub mw_push_revision {
 		# No remote mediawiki revision. Export the whole
 		# history (linearized with --first-parent)
 		print STDERR "Warning: no common ancestor, pushing complete history\n";
-		my $history = run_git("rev-list --first-parent --children $local");
+		my $history = run_git("rev-list --first-parent --children ${local}");
 		my @history = split(/\n/, $history);
 		@history = @history[1..$#history];
 		foreach my $line (reverse @history) {
@@ -1211,12 +1211,12 @@ sub mw_push_revision {
 	foreach my $commit_info_split (@commit_pairs) {
 		my $sha1_child = @{$commit_info_split}[0];
 		my $sha1_commit = @{$commit_info_split}[1];
-		my $diff_infos = run_git("diff-tree -r --raw -z $sha1_child $sha1_commit");
+		my $diff_infos = run_git("diff-tree -r --raw -z ${sha1_child} ${sha1_commit}");
 		# TODO: we could detect rename, and encode them with a #redirect on the wiki.
 		# TODO: for now, it's just a delete+add
 		my @diff_info_list = split(/\0/, $diff_infos);
 		# Keep the subject line of the commit message as mediawiki comment for the revision
-		my $commit_msg = run_git("log --no-walk --format=\"%s\" $sha1_commit");
+		my $commit_msg = run_git(qq(log --no-walk --format="%s" ${sha1_commit}));
 		chomp($commit_msg);
 		# Push every blob
 		while (@diff_info_list) {
@@ -1228,7 +1228,7 @@ sub mw_push_revision {
 			my $info = shift(@diff_info_list);
 			my $file = shift(@diff_info_list);
 			($mw_revision, $status) = mw_push_file($info, $file, $commit_msg, $mw_revision);
-			if ($status eq "non-fast-forward") {
+			if ($status eq 'non-fast-forward') {
 				# we may already have sent part of the
 				# commit to MediaWiki, but it's too
 				# late to cancel it. Stop the push in
@@ -1236,17 +1236,17 @@ sub mw_push_revision {
 				# accurate error message.
 				return error_non_fast_forward($remote);
 			}
-			if ($status ne "ok") {
+			if ($status ne 'ok') {
 				die("Unknown error from mw_push_file()\n");
 			}
 		}
 		unless ($dumb_push) {
-			run_git("notes --ref=$remotename/mediawiki add -f -m \"mediawiki_revision: $mw_revision\" $sha1_commit");
-			run_git("update-ref -m \"Git-MediaWiki push\" refs/mediawiki/$remotename/master $sha1_commit $sha1_child");
+			run_git(qq(notes --ref=${remotename}/mediawiki add -f -m "mediawiki_revision: ${mw_revision}" ${sha1_commit}));
+			run_git(qq(update-ref -m "Git-MediaWiki push" refs/mediawiki/${remotename}/master ${sha1_commit} ${sha1_child}));
 		}
 	}
 
-	print STDOUT "ok $remote\n";
+	print STDOUT "ok ${remote}\n";
 	return 1;
 }
 
@@ -1282,8 +1282,7 @@ sub get_mw_namespace_id {
 		# Look at configuration file, if the record for that namespace is
 		# already cached. Namespaces are stored in form:
 		# "Name_of_namespace:Id_namespace", ex.: "File:6".
-		my @temp = split(/\n/, run_git("config --get-all remote."
-						. $remotename .".namespaceCache"));
+		my @temp = split(/\n/, run_git("config --get-all remote.${remotename}.namespaceCache"));
 		chomp(@temp);
 		foreach my $ns (@temp) {
 			my ($n, $id) = split(/:/, $ns);
@@ -1297,7 +1296,7 @@ sub get_mw_namespace_id {
 	}
 
 	if (!exists $namespace_id{$name}) {
-		print STDERR "Namespace $name not found in cache, querying the wiki ...\n";
+		print STDERR "Namespace ${name} not found in cache, querying the wiki ...\n";
 		# NS not found => get namespace id from MW and store it in
 	        # configuration file.
 	        my $query = {
@@ -1322,7 +1321,7 @@ sub get_mw_namespace_id {
 	my $id;
 
 	unless (defined $ns) {
-		print STDERR "No such namespace $name on MediaWiki.\n";
+		print STDERR "No such namespace ${name} on MediaWiki.\n";
 		$ns = {is_namespace => 0};
 		$namespace_id{$name} = $ns;
 	}
@@ -1336,8 +1335,7 @@ sub get_mw_namespace_id {
 
 	# Store explicitely requested namespaces on disk
 	if (!exists $cached_mw_namespace_id{$name}) {
-		run_git("config --add remote.". $remotename
-			.".namespaceCache \"". $name .":". $store_id ."\"");
+		run_git(qq(config --add remote.${remotename}.namespaceCache "${name}:${store_id}"));
 		$cached_mw_namespace_id{$name} = 1;
 	}
 	return $id;
-- 
1.7.9.5

Re: [PATCH 11/18] Modify strings for a better coding-style

[Eric Sunshine]

On Thu, Jun 6, 2013 at 3:34 PM, Célestin Matte
<celestin.matte@ensimag.fr> wrote:
> - strings which don't need interpolation are single-quoted for more clarity and
> slight gain of performance
> - interpolation is preferred over concatenation in many cases, for more clarity
> - variables are always used with the ${} operator inside strings
> - strings including double-quotes are written with qq() so that the quotes do
> not have to be escaped
>
> Signed-off-by: Célestin Matte <celestin.matte@ensimag.fr>
> Signed-off-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
> ---
>  contrib/mw-to-git/git-remote-mediawiki.perl |  250 +++++++++++++--------------
>  1 file changed, 124 insertions(+), 126 deletions(-)
>
> diff --git a/contrib/mw-to-git/git-remote-mediawiki.perl b/contrib/mw-to-git/git-remote-mediawiki.perl
> index 1271527..0e2152d 100755
> --- a/contrib/mw-to-git/git-remote-mediawiki.perl
> +++ b/contrib/mw-to-git/git-remote-mediawiki.perl
> @@ -344,10 +344,10 @@ sub get_mw_pages {
>  #        $out = run_git("command args", "raw"); # don't interpret output as UTF-8.
>  sub run_git {
>         my $args = shift;
> -       my $encoding = (shift || "encoding(UTF-8)");
> -       open(my $git, "-|:$encoding", "git " . $args)
> -           or die "Unable to open: $!\n";
> -       my $res = do {
> +       my $encoding = (shift || 'encoding(UTF-8)');
> +       my $res = do {
> +               open(my $git, "-|:$encoding", "git ${args}")
> +                   or die "Unable to fork: $!\n";
>                 local $/ = undef;
>                 <$git>
>         };

A change sneaked in here not mentioned by the commit message and not
related to the other changes. open() is now inside the do{} block.

[PATCH 12/18] Brace file handles for print for more clarity

[Célestin Matte]

This follows the following rule:
InputOutput::RequireBracedFileHandleWithPrint (Severity: 1)
    The `print' and `printf' functions have a unique syntax that supports an
    optional file handle argument. Conway suggests wrapping this argument in
    braces to make it visually stand out from the other arguments. When you
    put braces around any of the special package-level file handles like
    `STDOUT', `STDERR', and `DATA', you must the `'*'' sigil or else it
    won't compile under `use strict 'subs''.

      print $FH   "Mary had a little lamb\n";  #not ok
      print {$FH} "Mary had a little lamb\n";  #ok

      print   STDERR   $foo, $bar, $baz;  #not ok
      print  {STDERR}  $foo, $bar, $baz;  #won't compile under 'strict'
      print {*STDERR}  $foo, $bar, $baz;  #perfect!

Signed-off-by: Célestin Matte <celestin.matte@ensimag.fr>
Signed-off-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
---
 contrib/mw-to-git/git-remote-mediawiki.perl |  184 +++++++++++++--------------
 1 file changed, 92 insertions(+), 92 deletions(-)

diff --git a/contrib/mw-to-git/git-remote-mediawiki.perl b/contrib/mw-to-git/git-remote-mediawiki.perl
index 0e2152d..757a7a3 100755
--- a/contrib/mw-to-git/git-remote-mediawiki.perl
+++ b/contrib/mw-to-git/git-remote-mediawiki.perl
@@ -170,7 +170,7 @@ sub parse_commands {
 	} elsif ($cmd[0] eq 'push') {
 		mw_push($cmd[1]);
 	} else {
-		print STDERR "Unknown command. Aborting...\n";
+		print {*STDERR} "Unknown command. Aborting...\n";
 		last;
 	}
 	return;
@@ -197,7 +197,7 @@ sub mw_connect_maybe {
 			       lgdomain => $wiki_domain};
 		if ($mediawiki->login($request)) {
 			Git::credential \%credential, 'approve';
-			print STDERR qq(Logged in mediawiki user "$credential{username}".\n);
+			print {*STDERR} qq(Logged in mediawiki user "$credential{username}".\n);
 		} else {
 			print {*STDERR} qq(Failed to log in mediawiki user "$credential{username}" on ${url}\n);
 			print {*STDERR} '  (error ' .
@@ -265,9 +265,9 @@ sub get_mw_all_pages {
 		aplimit => 'max'
 	});
 	if (!defined($mw_pages)) {
-		print STDERR "fatal: could not get the list of wiki pages.\n";
-		print STDERR "fatal: '${url}' does not appear to be a mediawiki\n";
-		print STDERR "fatal: make sure '${url}/api.php' is a valid page.\n";
+		print {*STDERR} "fatal: could not get the list of wiki pages.\n";
+		print {*STDERR} "fatal: '${url}' does not appear to be a mediawiki\n";
+		print {*STDERR} "fatal: make sure '${url}/api.php' is a valid page.\n";
 		exit 1;
 	}
 	foreach my $page (@{$mw_pages}) {
@@ -292,14 +292,14 @@ sub get_mw_first_pages {
 		titles => $titles,
 	});
 	if (!defined($mw_pages)) {
-		print STDERR "fatal: could not query the list of wiki pages.\n";
-		print STDERR "fatal: '${url}' does not appear to be a mediawiki\n";
-		print STDERR "fatal: make sure '${url}/api.php' is a valid page.\n";
+		print {*STDERR} "fatal: could not query the list of wiki pages.\n";
+		print {*STDERR} "fatal: '${url}' does not appear to be a mediawiki\n";
+		print {*STDERR} "fatal: make sure '${url}/api.php' is a valid page.\n";
 		exit 1;
 	}
 	while (my ($id, $page) = each(%{$mw_pages->{query}->{pages}})) {
 		if ($id < 0) {
-			print STDERR "Warning: page $page->{title} not found on wiki\n";
+			print {*STDERR} "Warning: page $page->{title} not found on wiki\n";
 		} else {
 			$pages->{$page->{title}} = $page;
 		}
@@ -311,7 +311,7 @@ sub get_mw_first_pages {
 sub get_mw_pages {
 	mw_connect_maybe();
 
-	print STDERR "Listing pages on remote wiki...\n";
+	print {*STDERR} "Listing pages on remote wiki...\n";
 
 	my %pages; # hash on page titles to avoid duplicates
 	my $user_defined;
@@ -329,14 +329,14 @@ sub get_mw_pages {
 		get_mw_all_pages(\%pages);
 	}
 	if ($import_media) {
-		print STDERR "Getting media files for selected pages...\n";
+		print {*STDERR} "Getting media files for selected pages...\n";
 		if ($user_defined) {
 			get_linked_mediafiles(\%pages);
 		} else {
 			get_all_mediafiles(\%pages);
 		}
 	}
-	print STDERR (scalar keys %pages) . " pages found.\n";
+	print {*STDERR} (scalar keys %pages) . " pages found.\n";
 	return %pages;
 }
 
@@ -369,9 +369,9 @@ sub get_all_mediafiles {
 		aplimit => 'max'
 	});
 	if (!defined($mw_pages)) {
-		print STDERR "fatal: could not get the list of pages for media files.\n";
-		print STDERR "fatal: '$url' does not appear to be a mediawiki\n";
-		print STDERR "fatal: make sure '$url/api.php' is a valid page.\n";
+		print {*STDERR} "fatal: could not get the list of pages for media files.\n";
+		print {*STDERR} "fatal: '$url' does not appear to be a mediawiki\n";
+		print {*STDERR} "fatal: make sure '$url/api.php' is a valid page.\n";
 		exit 1;
 	}
 	foreach my $page (@{$mw_pages}) {
@@ -458,7 +458,7 @@ sub get_mw_mediafile_for_page_revision {
 		$mediafile{timestamp} = $fileinfo->{timestamp};
 		# Mediawiki::API's download function doesn't support https URLs
 		# and can't download old versions of files.
-		print STDERR "\tDownloading file $mediafile{title}, version $mediafile{timestamp}\n";
+		print {*STDERR} "\tDownloading file $mediafile{title}, version $mediafile{timestamp}\n";
 		$mediafile{content} = download_mw_mediafile($fileinfo->{url});
 	}
 	return %mediafile;
@@ -485,13 +485,13 @@ sub get_last_local_revision {
 
 	my $lastrevision_number;
 	if (!(defined($note_info[0]) && $note_info[0] eq 'mediawiki_revision:')) {
-		print STDERR 'No previous mediawiki revision found';
+		print {*STDERR} 'No previous mediawiki revision found';
 		$lastrevision_number = 0;
 	} else {
 		# Notes are formatted : mediawiki_revision: #number
 		$lastrevision_number = $note_info[1];
 		chomp($lastrevision_number);
-		print STDERR "Last local mediawiki revision found is ${lastrevision_number}";
+		print {*STDERR} "Last local mediawiki revision found is ${lastrevision_number}";
 	}
 	return $lastrevision_number;
 }
@@ -524,7 +524,7 @@ sub get_last_remote_revision {
 
 	my $max_rev_num = 0;
 
-	print STDERR "Getting last revision id on tracked pages...\n";
+	print {*STDERR} "Getting last revision id on tracked pages...\n";
 
 	foreach my $page (@pages) {
 		my $id = $page->{pageid};
@@ -545,7 +545,7 @@ sub get_last_remote_revision {
 		$max_rev_num = ($lastrev->{revid} > $max_rev_num ? $lastrev->{revid} : $max_rev_num);
 	}
 
-	print STDERR "Last remote revision found is $max_rev_num.\n";
+	print {*STDERR} "Last remote revision found is $max_rev_num.\n";
 	return $max_rev_num;
 }
 
@@ -598,7 +598,7 @@ sub mediawiki_smudge_filename {
 
 sub literal_data {
 	my ($content) = @_;
-	print STDOUT 'data ', bytes::length($content), "\n", $content;
+	print {*STDOUT} 'data ', bytes::length($content), "\n", $content;
 	return;
 }
 
@@ -608,7 +608,7 @@ sub literal_data_raw {
 	# Avoid confusion between size in bytes and in characters
 	utf8::downgrade($content);
 	binmode STDOUT, ':raw';
-	print STDOUT 'data ', bytes::length($content), "\n", $content;
+	print {*STDOUT} 'data ', bytes::length($content), "\n", $content;
 	binmode STDOUT, ':encoding(UTF-8)';
 	return;
 }
@@ -617,26 +617,26 @@ sub mw_capabilities {
 	# Revisions are imported to the private namespace
 	# refs/mediawiki/$remotename/ by the helper and fetched into
 	# refs/remotes/$remotename later by fetch.
-	print STDOUT "refspec refs/heads/*:refs/mediawiki/${remotename}/*\n";
-	print STDOUT "import\n";
-	print STDOUT "list\n";
-	print STDOUT "push\n";
-	print STDOUT "\n";
+	print {*STDOUT} "refspec refs/heads/*:refs/mediawiki/${remotename}/*\n";
+	print {*STDOUT} "import\n";
+	print {*STDOUT} "list\n";
+	print {*STDOUT} "push\n";
+	print {*STDOUT} "\n";
 	return;
 }
 
 sub mw_list {
 	# MediaWiki do not have branches, we consider one branch arbitrarily
 	# called master, and HEAD pointing to it.
-	print STDOUT "? refs/heads/master\n";
-	print STDOUT "\@refs/heads/master HEAD\n";
-	print STDOUT "\n";
+	print {*STDOUT} "? refs/heads/master\n";
+	print {*STDOUT} "\@refs/heads/master HEAD\n";
+	print {*STDOUT} "\n";
 	return;
 }
 
 sub mw_option {
-	print STDERR "remote-helper command 'option $_[0]' not yet implemented\n";
-	print STDOUT "unsupported\n";
+	print {*STDERR} "remote-helper command 'option $_[0]' not yet implemented\n";
+	print {*STDOUT} "unsupported\n";
 	return;
 }
 
@@ -672,11 +672,11 @@ sub fetch_mw_revisions_for_page {
 		$query->{rvstartid} = $result->{'query-continue'}->{revisions}->{rvstartid};
 	}
 	if ($shallow_import && @page_revs) {
-		print STDERR "  Found 1 revision (shallow import).\n";
+		print {*STDERR} "  Found 1 revision (shallow import).\n";
 		@page_revs = sort {$b->{revid} <=> $a->{revid}} (@page_revs);
 		return $page_revs[0];
 	}
-	print STDERR "  Found ${revnum} revision(s).\n";
+	print {*STDERR} "  Found ${revnum} revision(s).\n";
 	return @page_revs;
 }
 
@@ -722,42 +722,42 @@ sub import_file_revision {
 	my $author = $commit{author};
 	my $date = $commit{date};
 
-	print STDOUT "commit refs/mediawiki/${remotename}/master\n";
-	print STDOUT "mark :${n}\n";
-	print STDOUT "committer ${author} <${author}\@${wiki_name}> " . $date->epoch . " +0000\n";
+	print {*STDOUT} "commit refs/mediawiki/${remotename}/master\n";
+	print {*STDOUT} "mark :${n}\n";
+	print {*STDOUT} "committer ${author} <${author}\@${wiki_name}> " . $date->epoch . " +0000\n";
 	literal_data($comment);
 
 	# If it's not a clone, we need to know where to start from
 	if (!$full_import && $n == 1) {
-		print STDOUT "from refs/mediawiki/${remotename}/master^0\n";
+		print {*STDOUT} "from refs/mediawiki/${remotename}/master^0\n";
 	}
 	if ($content ne $DELETED_CONTENT) {
 		print {*STDOUT} 'M 644 inline ' .
 		    fe_escape_path("${title}.mw") . "\n";
 		literal_data($content);
 		if (%mediafile) {
-			print STDOUT 'M 644 inline '
+			print {*STDOUT} 'M 644 inline '
 			    . fe_escape_path($mediafile{title}) . "\n";
 			literal_data_raw($mediafile{content});
 		}
-		print STDOUT "\n\n";
+		print {*STDOUT} "\n\n";
 	} else {
-		print STDOUT 'D ' . fe_escape_path("${title}.mw") . "\n";
+		print {*STDOUT} 'D ' . fe_escape_path("${title}.mw") . "\n";
 	}
 
 	# mediawiki revision number in the git note
 	if ($full_import && $n == 1) {
-		print STDOUT "reset refs/notes/${remotename}/mediawiki\n";
+		print {*STDOUT} "reset refs/notes/${remotename}/mediawiki\n";
 	}
-	print STDOUT "commit refs/notes/${remotename}/mediawiki\n";
-	print STDOUT "committer ${author} <${author}\@${wiki_name}> " . $date->epoch . " +0000\n";
+	print {*STDOUT} "commit refs/notes/${remotename}/mediawiki\n";
+	print {*STDOUT} "committer ${author} <${author}\@${wiki_name}> " . $date->epoch . " +0000\n";
 	literal_data('Note added by git-mediawiki during import');
 	if (!$full_import && $n == 1) {
-		print STDOUT "from refs/notes/${remotename}/mediawiki^0\n";
+		print {*STDOUT} "from refs/notes/${remotename}/mediawiki^0\n";
 	}
-	print STDOUT "N inline :${n}\n";
+	print {*STDOUT} "N inline :${n}\n";
 	literal_data("mediawiki_revision: $commit{mw_revision}");
-	print STDOUT "\n\n";
+	print {*STDOUT} "\n\n";
 	return;
 }
 
@@ -788,7 +788,7 @@ sub mw_import {
 	foreach my $ref (@refs) {
 		mw_import_ref($ref);
 	}
-	print STDOUT "done\n";
+	print {*STDOUT} "done\n";
 	return;
 }
 
@@ -805,30 +805,30 @@ sub mw_import_ref {
 
 	mw_connect_maybe();
 
-	print STDERR "Searching revisions...\n";
+	print {*STDERR} "Searching revisions...\n";
 	my $last_local = get_last_local_revision();
 	my $fetch_from = $last_local + 1;
 	if ($fetch_from == 1) {
-		print STDERR ", fetching from beginning.\n";
+		print {*STDERR} ", fetching from beginning.\n";
 	} else {
-		print STDERR ", fetching from here.\n";
+		print {*STDERR} ", fetching from here.\n";
 	}
 
 	my $n = 0;
 	if ($fetch_strategy eq 'by_rev') {
-		print STDERR "Fetching & writing export data by revs...\n";
+		print {*STDERR} "Fetching & writing export data by revs...\n";
 		$n = mw_import_ref_by_revs($fetch_from);
 	} elsif ($fetch_strategy eq 'by_page') {
-		print STDERR "Fetching & writing export data by pages...\n";
+		print {*STDERR} "Fetching & writing export data by pages...\n";
 		$n = mw_import_ref_by_pages($fetch_from);
 	} else {
-		print STDERR qq(fatal: invalid fetch strategy "${fetch_strategy}".\n);
-		print STDERR "Check your configuration variables remote.${remotename}.fetchStrategy and mediawiki.fetchStrategy\n";
+		print {*STDERR} qq(fatal: invalid fetch strategy "${fetch_strategy}".\n);
+		print {*STDERR} "Check your configuration variables remote.${remotename}.fetchStrategy and mediawiki.fetchStrategy\n";
 		exit 1;
 	}
 
 	if ($fetch_from == 1 && $n == 0) {
-		print STDERR "You appear to have cloned an empty MediaWiki.\n";
+		print {*STDERR} "You appear to have cloned an empty MediaWiki.\n";
 		# Something has to be done remote-helper side. If nothing is done, an error is
 		# thrown saying that HEAD is referring to unknown object 0000000000000000000
 		# and the clone fails.
@@ -907,7 +907,7 @@ sub mw_import_revids {
 		my $page_title = $result_page->{title};
 
 		if (!exists($pages->{$page_title})) {
-			print STDERR "${n}/", scalar(@$revision_ids),
+			print {*STDERR} "${n}/", scalar(@$revision_ids),
 				": Skipping revision #$rev->{revid} of ${page_title}\n";
 			next;
 		}
@@ -940,7 +940,7 @@ sub mw_import_revids {
 		# If this is a revision of the media page for new version
 		# of a file do one common commit for both file and media page.
 		# Else do commit only for that page.
-		print STDERR "${n}/", scalar(@$revision_ids), ": Revision #$rev->{revid} of $commit{title}\n";
+		print {*STDERR} "${n}/", scalar(@$revision_ids), ": Revision #$rev->{revid} of $commit{title}\n";
 		import_file_revision(\%commit, ($fetch_from == 1), $n_actual, \%mediafile);
 	}
 
@@ -954,11 +954,11 @@ sub error_non_fast_forward {
 		# Native git-push would show this after the summary.
 		# We can't ask it to display it cleanly, so print it
 		# ourselves before.
-		print STDERR "To prevent you from losing history, non-fast-forward updates were rejected\n";
-		print STDERR "Merge the remote changes (e.g. 'git pull') before pushing again. See the\n";
-		print STDERR "'Note about fast-forwards' section of 'git push --help' for details.\n";
+		print {*STDERR} "To prevent you from losing history, non-fast-forward updates were rejected\n";
+		print {*STDERR} "Merge the remote changes (e.g. 'git pull') before pushing again. See the\n";
+		print {*STDERR} "'Note about fast-forwards' section of 'git push --help' for details.\n";
 	}
-	print STDOUT qq(error $_[0] "non-fast-forward"\n);
+	print {*STDOUT} qq(error $_[0] "non-fast-forward"\n);
 	return 0;
 }
 
@@ -972,8 +972,8 @@ sub mw_upload_file {
 	my $path = "File:${complete_file_name}";
 	my %hashFiles = get_allowed_file_extensions();
 	if (!exists($hashFiles{$extension})) {
-		print STDERR "${complete_file_name} is not a permitted file on this wiki.\n";
-		print STDERR "Check the configuration of file uploads in your mediawiki.\n";
+		print {*STDERR} "${complete_file_name} is not a permitted file on this wiki.\n";
+		print {*STDERR} "Check the configuration of file uploads in your mediawiki.\n";
 		return $newrevid;
 	}
 	# Deleting and uploading a file requires a priviledged user
@@ -985,9 +985,9 @@ sub mw_upload_file {
 			reason => $summary
 		};
 		if (!$mediawiki->edit($query)) {
-			print STDERR "Failed to delete file on remote wiki\n";
-			print STDERR "Check your permissions on the remote site. Error code:\n";
-			print STDERR $mediawiki->{error}->{code} . ':' . $mediawiki->{error}->{details};
+			print {*STDERR} "Failed to delete file on remote wiki\n";
+			print {*STDERR} "Check your permissions on the remote site. Error code:\n";
+			print {*STDERR} $mediawiki->{error}->{code} . ':' . $mediawiki->{error}->{details};
 			exit 1;
 		}
 	} else {
@@ -1011,9 +1011,9 @@ sub mw_upload_file {
 				 . $mediawiki->{error}->{details} . "\n";
 			my $last_file_page = $mediawiki->get_page({title => $path});
 			$newrevid = $last_file_page->{revid};
-			print STDERR "Pushed file: ${new_sha1} - ${complete_file_name}.\n";
+			print {*STDERR} "Pushed file: ${new_sha1} - ${complete_file_name}.\n";
 		} else {
-			print STDERR "Empty file ${complete_file_name} not pushed.\n";
+			print {*STDERR} "Empty file ${complete_file_name} not pushed.\n";
 		}
 	}
 	return $newrevid;
@@ -1051,7 +1051,7 @@ sub mw_push_file {
 	if ($extension eq 'mw') {
 		my $ns = get_mw_namespace_id_for_page($complete_file_name);
 		if ($ns && $ns == get_mw_namespace_id('File') && (!$export_media)) {
-			print STDERR "Ignoring media file related page: ${complete_file_name}\n";
+			print {*STDERR} "Ignoring media file related page: ${complete_file_name}\n";
 			return ($oldrevid, 'ok');
 		}
 		my $file_content;
@@ -1079,7 +1079,7 @@ sub mw_push_file {
 		if (!$result) {
 			if ($mediawiki->{error}->{code} == 3) {
 				# edit conflicts, considered as non-fast-forward
-				print STDERR 'Warning: Error ' .
+				print {*STDERR} 'Warning: Error ' .
 				    $mediawiki->{error}->{code} .
 				    ' from mediwiki: ' . $mediawiki->{error}->{details} .
 				    ".\n";
@@ -1092,13 +1092,13 @@ sub mw_push_file {
 			}
 		}
 		$newrevid = $result->{edit}->{newrevid};
-		print STDERR "Pushed file: ${new_sha1} - ${title}\n";
+		print {*STDERR} "Pushed file: ${new_sha1} - ${title}\n";
 	} elsif ($export_media) {
 		$newrevid = mw_upload_file($complete_file_name, $new_sha1,
 					   $extension, $page_deleted,
 					   $summary);
 	} else {
-		print STDERR "Ignoring media file ${title}\n";
+		print {*STDERR} "Ignoring media file ${title}\n";
 	}
 	$newrevid = ($newrevid or $oldrevid);
 	return ($newrevid, 'ok');
@@ -1112,16 +1112,16 @@ sub mw_push {
 		my ($force, $local, $remote) = $refspec =~ /^(\+)?([^:]*):([^:]*)$/
 		    or die("Invalid refspec for push. Expected <src>:<dst> or +<src>:<dst>\n");
 		if ($force) {
-			print STDERR "Warning: forced push not allowed on a MediaWiki.\n";
+			print {*STDERR} "Warning: forced push not allowed on a MediaWiki.\n";
 		}
 		if ($local eq "") {
-			print STDERR "Cannot delete remote branch on a MediaWiki\n";
-			print STDOUT "error ${remote} cannot delete\n";
+			print {*STDERR} "Cannot delete remote branch on a MediaWiki\n";
+			print {*STDOUT} "error ${remote} cannot delete\n";
 			next;
 		}
 		if ($remote ne 'refs/heads/master') {
-			print STDERR "Only push to the branch 'master' is supported on a MediaWiki\n";
-			print STDOUT "error ${remote} only master allowed\n";
+			print {*STDERR} "Only push to the branch 'master' is supported on a MediaWiki\n";
+			print {*STDOUT} "error ${remote} only master allowed\n";
 			next;
 		}
 		if (mw_push_revision($local, $remote)) {
@@ -1130,15 +1130,15 @@ sub mw_push {
 	}
 
 	# Notify Git that the push is done
-	print STDOUT "\n";
+	print {*STDOUT} "\n";
 
 	if ($pushed && $dumb_push) {
-		print STDERR "Just pushed some revisions to MediaWiki.\n";
-		print STDERR "The pushed revisions now have to be re-imported, and your current branch\n";
-		print STDERR "needs to be updated with these re-imported commits. You can do this with\n";
-		print STDERR "\n";
-		print STDERR "  git pull --rebase\n";
-		print STDERR "\n";
+		print {*STDERR} "Just pushed some revisions to MediaWiki.\n";
+		print {*STDERR} "The pushed revisions now have to be re-imported, and your current branch\n";
+		print {*STDERR} "needs to be updated with these re-imported commits. You can do this with\n";
+		print {*STDERR} "\n";
+		print {*STDERR} "  git pull --rebase\n";
+		print {*STDERR} "\n";
 	}
 	return;
 }
@@ -1147,7 +1147,7 @@ sub mw_push_revision {
 	my $local = shift;
 	my $remote = shift; # actually, this has to be "refs/heads/master" at this point.
 	my $last_local_revid = get_last_local_revision();
-	print STDERR ".\n"; # Finish sentence started by get_last_local_revision()
+	print {*STDERR} ".\n"; # Finish sentence started by get_last_local_revision()
 	my $last_remote_revid = get_last_remote_revision();
 	my $mw_revision = $last_remote_revid;
 
@@ -1174,7 +1174,7 @@ sub mw_push_revision {
 	if ($last_local_revid > 0) {
 		my $parsed_sha1 = $remoteorigin_sha1;
 		# Find a path from last MediaWiki commit to pushed commit
-		print STDERR "Computing path from local to remote ...\n";
+		print {*STDERR} "Computing path from local to remote ...\n";
 		my @local_ancestry = split(/\n/, run_git("rev-list --boundary --parents ${local} ^${parsed_sha1}"));
 		my %local_ancestry;
 		foreach my $line (@local_ancestry) {
@@ -1189,7 +1189,7 @@ sub mw_push_revision {
 		while ($parsed_sha1 ne $HEAD_sha1) {
 			my $child = $local_ancestry{$parsed_sha1};
 			if (!$child) {
-				printf STDERR "Cannot find a path in history from remote commit to last commit\n";
+				print {*STDERR} "Cannot find a path in history from remote commit to last commit\n";
 				return error_non_fast_forward($remote);
 			}
 			push(@commit_pairs, [$parsed_sha1, $child]);
@@ -1198,7 +1198,7 @@ sub mw_push_revision {
 	} else {
 		# No remote mediawiki revision. Export the whole
 		# history (linearized with --first-parent)
-		print STDERR "Warning: no common ancestor, pushing complete history\n";
+		print {*STDERR} "Warning: no common ancestor, pushing complete history\n";
 		my $history = run_git("rev-list --first-parent --children ${local}");
 		my @history = split(/\n/, $history);
 		@history = @history[1..$#history];
@@ -1246,7 +1246,7 @@ sub mw_push_revision {
 		}
 	}
 
-	print STDOUT "ok ${remote}\n";
+	print {*STDOUT} "ok ${remote}\n";
 	return 1;
 }
 
@@ -1296,7 +1296,7 @@ sub get_mw_namespace_id {
 	}
 
 	if (!exists $namespace_id{$name}) {
-		print STDERR "Namespace ${name} not found in cache, querying the wiki ...\n";
+		print {*STDERR} "Namespace ${name} not found in cache, querying the wiki ...\n";
 		# NS not found => get namespace id from MW and store it in
 	        # configuration file.
 	        my $query = {
@@ -1321,7 +1321,7 @@ sub get_mw_namespace_id {
 	my $id;
 
 	unless (defined $ns) {
-		print STDERR "No such namespace ${name} on MediaWiki.\n";
+		print {*STDERR} "No such namespace ${name} on MediaWiki.\n";
 		$ns = {is_namespace => 0};
 		$namespace_id{$name} = $ns;
 	}
-- 
1.7.9.5

[PATCH 13/18] Remove "unless" statements and replace them by negated "if" statements

[Célestin Matte]

Signed-off-by: Célestin Matte <celestin.matte@ensimag.fr>
Signed-off-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
---
 contrib/mw-to-git/git-remote-mediawiki.perl |   12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/contrib/mw-to-git/git-remote-mediawiki.perl b/contrib/mw-to-git/git-remote-mediawiki.perl
index 757a7a3..b7a7012 100755
--- a/contrib/mw-to-git/git-remote-mediawiki.perl
+++ b/contrib/mw-to-git/git-remote-mediawiki.perl
@@ -87,11 +87,11 @@ $shallow_import = ($shallow_import eq 'true');
 # - by_rev: perform one query per new revision on the remote wiki
 # - by_page: query each tracked page for new revision
 my $fetch_strategy = run_git("config --get remote.${remotename}.fetchStrategy");
-unless ($fetch_strategy) {
+if (! $fetch_strategy) {
 	$fetch_strategy = run_git('config --get mediawiki.fetchStrategy');
 }
 chomp($fetch_strategy);
-unless ($fetch_strategy) {
+if (! $fetch_strategy) {
 	$fetch_strategy = 'by_page';
 }
 
@@ -113,7 +113,7 @@ my %basetimestamps;
 # deterministic, this means everybody gets the same sha1 for each
 # MediaWiki revision.
 my $dumb_push = run_git("config --get --bool remote.${remotename}.dumbPush");
-unless ($dumb_push) {
+if (! $dumb_push) {
 	$dumb_push = run_git('config --get --bool mediawiki.dumbPush');
 }
 chomp($dumb_push);
@@ -668,7 +668,7 @@ sub fetch_mw_revisions_for_page {
 			push(@page_revs, $page_rev_ids);
 			$revnum++;
 		}
-		last unless $result->{'query-continue'};
+		last if (! $result->{'query-continue'});
 		$query->{rvstartid} = $result->{'query-continue'}->{revisions}->{rvstartid};
 	}
 	if ($shallow_import && @page_revs) {
@@ -1240,7 +1240,7 @@ sub mw_push_revision {
 				die("Unknown error from mw_push_file()\n");
 			}
 		}
-		unless ($dumb_push) {
+		if (! $dumb_push) {
 			run_git(qq(notes --ref=${remotename}/mediawiki add -f -m "mediawiki_revision: ${mw_revision}" ${sha1_commit}));
 			run_git(qq(update-ref -m "Git-MediaWiki push" refs/mediawiki/${remotename}/master ${sha1_commit} ${sha1_child}));
 		}
@@ -1320,7 +1320,7 @@ sub get_mw_namespace_id {
 	my $ns = $namespace_id{$name};
 	my $id;
 
-	unless (defined $ns) {
+	if (! defined $ns) {
 		print {*STDERR} "No such namespace ${name} on MediaWiki.\n";
 		$ns = {is_namespace => 0};
 		$namespace_id{$name} = $ns;
-- 
1.7.9.5

Re: [PATCH 13/18] Remove "unless" statements and replace them by negated "if" statements

[Eric Sunshine]

On Thu, Jun 6, 2013 at 3:34 PM, Célestin Matte
<celestin.matte@ensimag.fr> wrote:
> Signed-off-by: Célestin Matte <celestin.matte@ensimag.fr>
> Signed-off-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
> ---
>  contrib/mw-to-git/git-remote-mediawiki.perl |   12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/contrib/mw-to-git/git-remote-mediawiki.perl b/contrib/mw-to-git/git-remote-mediawiki.perl
> index 757a7a3..b7a7012 100755
> --- a/contrib/mw-to-git/git-remote-mediawiki.perl
> +++ b/contrib/mw-to-git/git-remote-mediawiki.perl
> @@ -87,11 +87,11 @@ $shallow_import = ($shallow_import eq 'true');
>  # - by_rev: perform one query per new revision on the remote wiki
>  # - by_page: query each tracked page for new revision
>  my $fetch_strategy = run_git("config --get remote.${remotename}.fetchStrategy");
> -unless ($fetch_strategy) {
> +if (! $fetch_strategy) {

Minor style nit: Existing code in git-remote-mediawiki.perl does not
have whitespace following '!'. This nit applies to this entire patch,
so: s/! /!/g

>         $fetch_strategy = run_git('config --get mediawiki.fetchStrategy');
>  }
>  chomp($fetch_strategy);
> -unless ($fetch_strategy) {
> +if (! $fetch_strategy) {
>         $fetch_strategy = 'by_page';
>  }
>
> @@ -113,7 +113,7 @@ my %basetimestamps;
>  # deterministic, this means everybody gets the same sha1 for each
>  # MediaWiki revision.
>  my $dumb_push = run_git("config --get --bool remote.${remotename}.dumbPush");
> -unless ($dumb_push) {
> +if (! $dumb_push) {
>         $dumb_push = run_git('config --get --bool mediawiki.dumbPush');
>  }
>  chomp($dumb_push);
> @@ -668,7 +668,7 @@ sub fetch_mw_revisions_for_page {
>                         push(@page_revs, $page_rev_ids);
>                         $revnum++;
>                 }
> -               last unless $result->{'query-continue'};
> +               last if (! $result->{'query-continue'});
>                 $query->{rvstartid} = $result->{'query-continue'}->{revisions}->{rvstartid};
>         }
>         if ($shallow_import && @page_revs) {
> @@ -1240,7 +1240,7 @@ sub mw_push_revision {
>                                 die("Unknown error from mw_push_file()\n");
>                         }
>                 }
> -               unless ($dumb_push) {
> +               if (! $dumb_push) {
>                         run_git(qq(notes --ref=${remotename}/mediawiki add -f -m "mediawiki_revision: ${mw_revision}" ${sha1_commit}));
>                         run_git(qq(update-ref -m "Git-MediaWiki push" refs/mediawiki/${remotename}/master ${sha1_commit} ${sha1_child}));
>                 }
> @@ -1320,7 +1320,7 @@ sub get_mw_namespace_id {
>         my $ns = $namespace_id{$name};
>         my $id;
>
> -       unless (defined $ns) {
> +       if (! defined $ns) {
>                 print {*STDERR} "No such namespace ${name} on MediaWiki.\n";
>                 $ns = {is_namespace => 0};
>                 $namespace_id{$name} = $ns;
> --
> 1.7.9.5
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 14/18] Don't use quotes for empty strings

[Célestin Matte]

Empty strings are replaced by an $EMPTY constant.

Signed-off-by: Célestin Matte <celestin.matte@ensimag.fr>
Signed-off-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
---
 contrib/mw-to-git/git-remote-mediawiki.perl |   18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/contrib/mw-to-git/git-remote-mediawiki.perl b/contrib/mw-to-git/git-remote-mediawiki.perl
index b7a7012..059bd1a 100755
--- a/contrib/mw-to-git/git-remote-mediawiki.perl
+++ b/contrib/mw-to-git/git-remote-mediawiki.perl
@@ -41,6 +41,8 @@ Readonly my $NULL_SHA1 => '0000000000000000000000000000000000000000';
 # Used on Git's side to reflect empty edit messages on the wiki
 Readonly my $EMPTY_MESSAGE => '*Empty MediaWiki Message*';
 
+Readonly my $EMPTY => q{};
+
 if (@ARGV != 2) {
 	exit_error_usage();
 }
@@ -161,11 +163,11 @@ sub parse_commands {
 		mw_list($cmd[1]);
 	} elsif ($cmd[0] eq 'import') {
 		die("Invalid arguments for import\n")
-		    if ($cmd[1] eq "" || defined($cmd[2]));
+		    if ($cmd[1] eq $EMPTY || defined($cmd[2]));
 		mw_import($cmd[1]);
 	} elsif ($cmd[0] eq 'option') {
 		die("Too many arguments for option\n")
-		    if ($cmd[1] eq "" || $cmd[2] eq "" || defined($cmd[3]));
+		    if ($cmd[1] eq $EMPTY || $cmd[2] eq $EMPTY || defined($cmd[3]));
 		mw_option($cmd[1],$cmd[2]);
 	} elsif ($cmd[0] eq 'push') {
 		mw_push($cmd[1]);
@@ -556,7 +558,7 @@ sub mediawiki_clean {
 	# Mediawiki does not allow blank space at the end of a page and ends with a single \n.
 	# This function right trims a string and adds a \n at the end to follow this rule
 	$string =~ s/\s+$//;
-	if ($string eq "" && $page_created) {
+	if ($string eq $EMPTY && $page_created) {
 		# Creating empty pages is forbidden.
 		$string = $EMPTY_CONTENT;
 	}
@@ -567,7 +569,7 @@ sub mediawiki_clean {
 sub mediawiki_smudge {
 	my $string = shift;
 	if ($string eq $EMPTY_CONTENT) {
-		$string = "";
+		$string = $EMPTY;
 	}
 	# This \n is important. This is due to mediawiki's way to handle end of files.
 	return "${string}\n";
@@ -993,7 +995,7 @@ sub mw_upload_file {
 	} else {
 		# Don't let perl try to interpret file content as UTF-8 => use "raw"
 		my $content = run_git("cat-file blob ${new_sha1}", 'raw');
-		if ($content ne "") {
+		if ($content ne $EMPTY) {
 			mw_connect_maybe();
 			$mediawiki->{config}->{upload_url} =
 				"${url}/index.php/Special:Upload";
@@ -1035,7 +1037,7 @@ sub mw_push_file {
 	my $newrevid;
 
 	if ($summary eq $EMPTY_MESSAGE) {
-		$summary = '';
+		$summary = $EMPTY;
 	}
 
 	my $new_sha1 = $diff_info_split[3];
@@ -1046,7 +1048,7 @@ sub mw_push_file {
 
 	my ($title, $extension) = $complete_file_name =~ /^(.*)\.([^\.]*)$/;
 	if (!defined($extension)) {
-		$extension = "";
+		$extension = $EMPTY;
 	}
 	if ($extension eq 'mw') {
 		my $ns = get_mw_namespace_id_for_page($complete_file_name);
@@ -1114,7 +1116,7 @@ sub mw_push {
 		if ($force) {
 			print {*STDERR} "Warning: forced push not allowed on a MediaWiki.\n";
 		}
-		if ($local eq "") {
+		if ($local eq $EMPTY) {
 			print {*STDERR} "Cannot delete remote branch on a MediaWiki\n";
 			print {*STDOUT} "error ${remote} cannot delete\n";
 			next;
-- 
1.7.9.5

[PATCH 15/18] Put non-trivial numeric values (e.g., different from 0, 1 and 2) in constants.

[Célestin Matte]

Signed-off-by: Célestin Matte <celestin.matte@ensimag.fr>
Signed-off-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
---
 contrib/mw-to-git/git-remote-mediawiki.perl |   20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/contrib/mw-to-git/git-remote-mediawiki.perl b/contrib/mw-to-git/git-remote-mediawiki.perl
index 059bd1a..ff9fd8f 100755
--- a/contrib/mw-to-git/git-remote-mediawiki.perl
+++ b/contrib/mw-to-git/git-remote-mediawiki.perl
@@ -43,6 +43,16 @@ Readonly my $EMPTY_MESSAGE => '*Empty MediaWiki Message*';
 
 Readonly my $EMPTY => q{};
 
+# Number of pages taken into account at once in submodule get_mw_page_list
+Readonly my $SLICE_SIZE => 50;
+
+# Number of linked mediafile to get at once in get_linked_mediafiles
+# The query is split in small batches because of the MW API limit of
+# the number of links to be returned (500 links max).
+Readonly my $BATCH_SIZE => 10;
+
+Readonly my $HTTP_CODE_OK => 200;
+
 if (@ARGV != 2) {
 	exit_error_usage();
 }
@@ -224,13 +234,13 @@ sub get_mw_page_list {
 	my $pages = shift;
 	my @some_pages = @$page_list;
 	while (@some_pages) {
-		my $last_page = 50;
+		my $last_page = $SLICE_SIZE;
 		if ($#some_pages < $last_page) {
 			$last_page = $#some_pages;
 		}
 		my @slice = @some_pages[0..$last_page];
 		get_mw_first_pages(\@slice, $pages);
-		@some_pages = @some_pages[51..$#some_pages];
+		@some_pages = @some_pages[($SLICE_SIZE + 1)..$#some_pages];
 	}
 	return;
 }
@@ -386,9 +396,7 @@ sub get_linked_mediafiles {
 	my $pages = shift;
 	my @titles = map { $_->{title} } values(%{$pages});
 
-	# The query is split in small batches because of the MW API limit of
-	# the number of links to be returned (500 links max).
-	my $batch = 10;
+	my $batch = $BATCH_SIZE;
 	while (@titles) {
 		if ($#titles < $batch) {
 			$batch = $#titles;
@@ -470,7 +478,7 @@ sub download_mw_mediafile {
 	my $download_url = shift;
 
 	my $response = $mediawiki->{ua}->get($download_url);
-	if ($response->code == 200) {
+	if ($response->code == $HTTP_CODE_OK) {
 		return $response->decoded_content;
 	} else {
 		print {*STDERR} "Error downloading mediafile from :\n";
-- 
1.7.9.5

[PATCH 16/18] Fix a typo ("mediwiki" instead of "mediawiki")

[Célestin Matte]

Signed-off-by: Célestin Matte <celestin.matte@ensimag.fr>
Signed-off-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
---
 contrib/mw-to-git/git-remote-mediawiki.perl |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/contrib/mw-to-git/git-remote-mediawiki.perl b/contrib/mw-to-git/git-remote-mediawiki.perl
index ff9fd8f..952ddcc 100755
--- a/contrib/mw-to-git/git-remote-mediawiki.perl
+++ b/contrib/mw-to-git/git-remote-mediawiki.perl
@@ -1091,14 +1091,14 @@ sub mw_push_file {
 				# edit conflicts, considered as non-fast-forward
 				print {*STDERR} 'Warning: Error ' .
 				    $mediawiki->{error}->{code} .
-				    ' from mediwiki: ' . $mediawiki->{error}->{details} .
+				    ' from mediawiki: ' . $mediawiki->{error}->{details} .
 				    ".\n";
 				return ($oldrevid, 'non-fast-forward');
 			} else {
 				# Other errors. Shouldn't happen => just die()
 				die 'Fatal: Error ' .
 				    $mediawiki->{error}->{code} .
-				    ' from mediwiki: ' . $mediawiki->{error}->{details} . "\n";
+				    ' from mediawiki: ' . $mediawiki->{error}->{details} . "\n";
 			}
 		}
 		$newrevid = $result->{edit}->{newrevid};
-- 
1.7.9.5

[PATCH 17/18] Place the open() call inside the do{} struct and prevent failing close

[Célestin Matte]

Placing the open() call inside the do{} struct will automatically close the
filehandle if possible.
Placing the close() call outside the do{} struct is useless and will make it
fail systematically
Change the error message to state that what fails is a fork(), not a file
opening.
Use autodie to properly exit when a print or open call fails.

Signed-off-by: Célestin Matte <celestin.matte@ensimag.fr>
Signed-off-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
---
 contrib/mw-to-git/git-remote-mediawiki.perl |    3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/contrib/mw-to-git/git-remote-mediawiki.perl b/contrib/mw-to-git/git-remote-mediawiki.perl
index 952ddcc..20ddccb 100755
--- a/contrib/mw-to-git/git-remote-mediawiki.perl
+++ b/contrib/mw-to-git/git-remote-mediawiki.perl
@@ -23,6 +23,7 @@ binmode STDOUT, ':encoding(UTF-8)';
 
 use URI::Escape;
 use Readonly;
+use autodie;
 
 # Mediawiki filenames can contain forward slashes. This variable decides by which pattern they should be replaced
 Readonly my $SLASH_REPLACEMENT => '%2F';
@@ -363,8 +364,6 @@ sub run_git {
 		local $/ = undef;
 		<$git>
 	};
-	close($git);
-
 	return $res;
 }
 
-- 
1.7.9.5

Re: [PATCH 17/18] Place the open() call inside the do{} struct and prevent failing close

[Junio C Hamano]

Célestin Matte <celestin.matte@ensimag.fr> writes:

> Placing the open() call inside the do{} struct will automatically close the
> filehandle if possible.
> Placing the close() call outside the do{} struct is useless and will make it
> fail systematically
> Change the error message to state that what fails is a fork(), not a file
> opening.
> Use autodie to properly exit when a print or open call fails.
>
> Signed-off-by: Célestin Matte <celestin.matte@ensimag.fr>
> Signed-off-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
> ---
>  contrib/mw-to-git/git-remote-mediawiki.perl |    3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/contrib/mw-to-git/git-remote-mediawiki.perl b/contrib/mw-to-git/git-remote-mediawiki.perl
> index 952ddcc..20ddccb 100755
> --- a/contrib/mw-to-git/git-remote-mediawiki.perl
> +++ b/contrib/mw-to-git/git-remote-mediawiki.perl
> @@ -23,6 +23,7 @@ binmode STDOUT, ':encoding(UTF-8)';
>  
>  use URI::Escape;
>  use Readonly;
> +use autodie;
>  
>  # Mediawiki filenames can contain forward slashes. This variable decides by which pattern they should be replaced
>  Readonly my $SLASH_REPLACEMENT => '%2F';
> @@ -363,8 +364,6 @@ sub run_git {
>  		local $/ = undef;
>  		<$git>
>  	};
> -	close($git);
> -
>  	return $res;
>  }

Confused.  Which part of this patch moves open inside a do{} block?
This was last touched by [9/18] but it doesn't do any such thing,
either.

Upon leaving this subroutine, $git filehandle will go out of scope,
so in that sense, close may not be necessary, but that does not
match what the proposed log message claims what the patch does.

Also, this patch does not remove "or die" 9/18 added, even though
the proposed log message claims that with autodie it is no longer
necessary.

I am not convinced that using autodie globally, without vetting the
calls the original code make, is a good idea in the first place.
How does this change interact with existing calls to open, close,
etc. that check the return value from them, now these calls throw
exception and will not give a chance for the existing error handling
codepath to intervene?

Re: [PATCH 17/18] Place the open() call inside the do{} struct and prevent failing close

[Célestin Matte]

Le 06/06/2013 23:13, Junio C Hamano a écrit :
> Confused.  Which part of this patch moves open inside a do{} block?
> This was last touched by [9/18] but it doesn't do any such thing,
> either.

I must have failed the rebase, as the first part of the commit moved to
[14/18] because it modifies a part of it:
>@@ -344,10 +344,10 @@ sub get_mw_pages {
> #        $out = run_git("command args", "raw"); # don't interpret
>output as UTF-8.
> sub run_git {
> 	my $args = shift;
>-	my $encoding = (shift || "encoding(UTF-8)");
>-	open(my $git, "-|:$encoding", "git " . $args)
>-	    or die "Unable to open: $!\n";
>-	my $res = do {
>+	my $encoding = (shift || 'encoding(UTF-8)');
>+	my $res = do {
>+		open(my $git, "-|:$encoding", "git ${args}")
>+		    or die "Unable to fork: $!\n";
> 		local $/ = undef;
> 		<$git>
> 	};
I'm not sure how I should correct this. I'll have a look if this commit
actually is useful.

> Upon leaving this subroutine, $git filehandle will go out of scope,
> so in that sense, close may not be necessary, but that does not
> match what the proposed log message claims what the patch does.
> 
> Also, this patch does not remove "or die" 9/18 added, even though
> the proposed log message claims that with autodie it is no longer
> necessary.
> 
> I am not convinced that using autodie globally, without vetting the
> calls the original code make, is a good idea in the first place.
> How does this change interact with existing calls to open, close,
> etc. that check the return value from them, now these calls throw
> exception and will not give a chance for the existing error handling
> codepath to intervene?

So using autodie may not be a good idea.
But the problem is that in the current state, open() return values are
checked, but print ones are not, although it should be. So, either:
- we use autodie and remove checking of existing return values, or
- we don't use autodie and add checking of return value of print calls
- or I'm missing some point :)


-- 
Célestin Matte

Re: [PATCH 17/18] Place the open() call inside the do{} struct and prevent failing close

[Junio C Hamano]

Célestin Matte <celestin.matte@ensimag.fr> writes:

> So using autodie may not be a good idea.
> But the problem is that in the current state, open() return values are
> checked, but print ones are not, although it should be.

I tried "man autodie" and tried to spot 'print' in the categories
list that shows things like ":all covers :default which in turn
covers :io which in turn covers read, seek, ..." and didn't see any.

Running the attached as

	$ perl ad.perl >&-

gave me "Hi" (i.e. no failure from print) but of course killed a
failing syswrite().

So I am not sure your "we must check print" matches well with use of
autodie, either.

-- >8 --
#!/usr/bin/perl -w

use warnings;
use autodie;

for (my $i = 0; $i < 256; $i++) {
	print "Did this die?\n";
}

print STDERR "Hi\n";

for (my $i = 0; $i < 256; $i++) {
	syswrite(1, "Did this die?\n");
}

print STDERR "Lo\n";

exit(0);
--- 8< --

Re: [PATCH 17/18] Place the open() call inside the do{} struct and prevent failing close

[Célestin Matte]

Le 06/06/2013 23:58, Junio C Hamano a écrit :
> Célestin Matte <celestin.matte@ensimag.fr> writes:
> 
>> So using autodie may not be a good idea.
>> But the problem is that in the current state, open() return values are
>> checked, but print ones are not, although it should be.
> 
> I tried "man autodie" and tried to spot 'print' in the categories
> list that shows things like ":all covers :default which in turn
> covers :io which in turn covers read, seek, ..." and didn't see any.
> 
> Running the attached as
> 
> 	$ perl ad.perl >&-
> 
> gave me "Hi" (i.e. no failure from print) but of course killed a
> failing syswrite().

You're right, print is not in the list of function checked by autodie.

> So I am not sure your "we must check print" matches well with use of
> autodie, either.

So I'm not sure what we should do for print calls: should we
systematically check them all? That would be a painful and less-readable
thing to do. We may just not bother about it, after all...

[PATCH 18/18] Clearly rewrite double dereference

[Célestin Matte]

@$var structures are re-written in the following way: @{ $var }
It makes them more readable.

Signed-off-by: Célestin Matte <celestin.matte@ensimag.fr>
Signed-off-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
---
 contrib/mw-to-git/git-remote-mediawiki.perl |   12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/contrib/mw-to-git/git-remote-mediawiki.perl b/contrib/mw-to-git/git-remote-mediawiki.perl
index 20ddccb..06e6f4d 100755
--- a/contrib/mw-to-git/git-remote-mediawiki.perl
+++ b/contrib/mw-to-git/git-remote-mediawiki.perl
@@ -233,7 +233,7 @@ sub get_mw_tracked_pages {
 sub get_mw_page_list {
 	my $page_list = shift;
 	my $pages = shift;
-	my @some_pages = @$page_list;
+	my @some_pages = @{ $page_list };
 	while (@some_pages) {
 		my $last_page = $SLICE_SIZE;
 		if ($#some_pages < $last_page) {
@@ -733,7 +733,7 @@ sub import_file_revision {
 
 	print {*STDOUT} "commit refs/mediawiki/${remotename}/master\n";
 	print {*STDOUT} "mark :${n}\n";
-	print {*STDOUT} "committer ${author} <${author}\@${wiki_name}> " . $date->epoch . " +0000\n";
+	print {*STDOUT} "committer ${author} <${author}\@{ ${wiki_name} }> " . $date->epoch . " +0000\n";
 	literal_data($comment);
 
 	# If it's not a clone, we need to know where to start from
@@ -759,7 +759,7 @@ sub import_file_revision {
 		print {*STDOUT} "reset refs/notes/${remotename}/mediawiki\n";
 	}
 	print {*STDOUT} "commit refs/notes/${remotename}/mediawiki\n";
-	print {*STDOUT} "committer ${author} <${author}\@${wiki_name}> " . $date->epoch . " +0000\n";
+	print {*STDOUT} "committer ${author} <${author}\@{ ${wiki_name} }> " . $date->epoch . " +0000\n";
 	literal_data('Note added by git-mediawiki during import');
 	if (!$full_import && $n == 1) {
 		print {*STDOUT} "from refs/notes/${remotename}/mediawiki^0\n";
@@ -881,7 +881,7 @@ sub mw_import_revids {
 	my $n_actual = 0;
 	my $last_timestamp = 0; # Placeholer in case $rev->timestamp is undefined
 
-	foreach my $pagerevid (@$revision_ids) {
+	foreach my $pagerevid (@{ $revision_ids }) {
 	        # Count page even if we skip it, since we display
 		# $n/$total and $total includes skipped pages.
 		$n++;
@@ -916,7 +916,7 @@ sub mw_import_revids {
 		my $page_title = $result_page->{title};
 
 		if (!exists($pages->{$page_title})) {
-			print {*STDERR} "${n}/", scalar(@$revision_ids),
+			print {*STDERR} "${n}/", scalar(@ { $revision_ids }),
 				": Skipping revision #$rev->{revid} of ${page_title}\n";
 			next;
 		}
@@ -949,7 +949,7 @@ sub mw_import_revids {
 		# If this is a revision of the media page for new version
 		# of a file do one common commit for both file and media page.
 		# Else do commit only for that page.
-		print {*STDERR} "${n}/", scalar(@$revision_ids), ": Revision #$rev->{revid} of $commit{title}\n";
+		print {*STDERR} "${n}/", scalar(@{ $revision_ids }), ": Revision #$rev->{revid} of $commit{title}\n";
 		import_file_revision(\%commit, ($fetch_from == 1), $n_actual, \%mediafile);
 	}
 
-- 
1.7.9.5

Re: [PATCH 18/18] Clearly rewrite double dereference

[Eric Sunshine]

On Thu, Jun 6, 2013 at 3:34 PM, Célestin Matte
<celestin.matte@ensimag.fr> wrote:
> @$var structures are re-written in the following way: @{ $var }
> It makes them more readable.
>
> Signed-off-by: Célestin Matte <celestin.matte@ensimag.fr>
> Signed-off-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
> ---
>  contrib/mw-to-git/git-remote-mediawiki.perl |   12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/contrib/mw-to-git/git-remote-mediawiki.perl b/contrib/mw-to-git/git-remote-mediawiki.perl
> index 20ddccb..06e6f4d 100755
> --- a/contrib/mw-to-git/git-remote-mediawiki.perl
> +++ b/contrib/mw-to-git/git-remote-mediawiki.perl
> @@ -233,7 +233,7 @@ sub get_mw_tracked_pages {
>  sub get_mw_page_list {
>         my $page_list = shift;
>         my $pages = shift;
> -       my @some_pages = @$page_list;
> +       my @some_pages = @{ $page_list };

Minor style nit: Existing code in git-remote-mediawiki.perl does not
have whitespace inside @{}. It prefers @{$foo} rather than @{ $foo }.
This nit applies to this entire patch.

>         while (@some_pages) {
>                 my $last_page = $SLICE_SIZE;
>                 if ($#some_pages < $last_page) {
> @@ -733,7 +733,7 @@ sub import_file_revision {
>
>         print {*STDOUT} "commit refs/mediawiki/${remotename}/master\n";
>         print {*STDOUT} "mark :${n}\n";
> -       print {*STDOUT} "committer ${author} <${author}\@${wiki_name}> " . $date->epoch . " +0000\n";
> +       print {*STDOUT} "committer ${author} <${author}\@{ ${wiki_name} }> " . $date->epoch . " +0000\n";
>         literal_data($comment);
>
>         # If it's not a clone, we need to know where to start from
> @@ -759,7 +759,7 @@ sub import_file_revision {
>                 print {*STDOUT} "reset refs/notes/${remotename}/mediawiki\n";
>         }
>         print {*STDOUT} "commit refs/notes/${remotename}/mediawiki\n";
> -       print {*STDOUT} "committer ${author} <${author}\@${wiki_name}> " . $date->epoch . " +0000\n";
> +       print {*STDOUT} "committer ${author} <${author}\@{ ${wiki_name} }> " . $date->epoch . " +0000\n";
>         literal_data('Note added by git-mediawiki during import');
>         if (!$full_import && $n == 1) {
>                 print {*STDOUT} "from refs/notes/${remotename}/mediawiki^0\n";
> @@ -881,7 +881,7 @@ sub mw_import_revids {
>         my $n_actual = 0;
>         my $last_timestamp = 0; # Placeholer in case $rev->timestamp is undefined
>
> -       foreach my $pagerevid (@$revision_ids) {
> +       foreach my $pagerevid (@{ $revision_ids }) {
>                 # Count page even if we skip it, since we display
>                 # $n/$total and $total includes skipped pages.
>                 $n++;
> @@ -916,7 +916,7 @@ sub mw_import_revids {
>                 my $page_title = $result_page->{title};
>
>                 if (!exists($pages->{$page_title})) {
> -                       print {*STDERR} "${n}/", scalar(@$revision_ids),
> +                       print {*STDERR} "${n}/", scalar(@ { $revision_ids }),
>                                 ": Skipping revision #$rev->{revid} of ${page_title}\n";
>                         next;
>                 }
> @@ -949,7 +949,7 @@ sub mw_import_revids {
>                 # If this is a revision of the media page for new version
>                 # of a file do one common commit for both file and media page.
>                 # Else do commit only for that page.
> -               print {*STDERR} "${n}/", scalar(@$revision_ids), ": Revision #$rev->{revid} of $commit{title}\n";
> +               print {*STDERR} "${n}/", scalar(@{ $revision_ids }), ": Revision #$rev->{revid} of $commit{title}\n";
>                 import_file_revision(\%commit, ($fetch_from == 1), $n_actual, \%mediafile);
>         }
>
> --
> 1.7.9.5
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html