On Sun, Oct 29, 2017 at 12:08 PM, Antoine Beaupré <anarcat@xxxxxxxxxx> wrote:
> From: Kevin <kevin@xxxxxxxxx>
>
> this introduces a new remote.origin.namespaces argument that is a
s/this/This/
> space-separated list of namespaces. the list of pages extract is then
s/the/The/
> taken from all the specified namespaces.
>
> Reviewed-by: Antoine Beaupré <anarcat@xxxxxxxxxx>
> Signed-off-by: Antoine Beaupré <anarcat@xxxxxxxxxx>
> ---
> diff --git a/contrib/mw-to-git/git-remote-mediawiki.perl b/contrib/mw-to-git/git-remote-mediawiki.perl
> @@ -1331,7 +1356,12 @@ sub get_mw_namespace_id {
> sub get_mw_namespace_id_for_page {
> my $namespace = shift;
> if ($namespace =~ /^([^:]*):/) {
This is not a new issue, but why capture if $1 is never referenced in
the code below?
> - return get_mw_namespace_id($namespace);
> + my ($ns, $id) = split(/:/, $namespace);
> + if (Scalar::Util::looks_like_number($id)) {
> + return get_mw_namespace_id($ns);
So, the idea is that if the input has form "something:number", then
you want to look up "something" as a namespace name. Anything else
(such as "something:foobar") is not considered a valid page reference.
Right?
> + } else{
Missing space before open brace.
> + return
Not required, but missing semi-colon.
> + }
> } else {
> return;
> }
The multiple 'return's are a bit messy. Perhaps collapse the entire
function to something like this:
sub get_mw_namespace_id_for_page {
my $arg = shift;
if ($arg =~ /^([^:]+):\d+$/) {
return get_mw_namespace_id($1);
}
return undef;
}
Then, you don't need even need Scalar::Util::looks_like_number()
(unless, I suppose, the incoming number is expected to be something
other than simple digits).
In fact, it may be that the intent of the original code *was* meant to
do exactly the same as shown in my example above, but that the person
who wrote it accidentally typed:
return get_mw_namespace_id($namespace);
instead of the intended:
return get_mw_namespace_id($1);
So, a minimal fix would be simply to change $namespace to $1.
Tightening the regex as I did in my example would be a bonus (though
probably ought to be a separate patch).
