Re: [PATCH 1/4] remote-mediawiki: add namespace support

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Oct 29, 2017, 17:24 UTC
Message-ID: <CAPig+cSmfJ2Uv21Q4DgJNoy6Ywj7GWPJa6qq0YL9Kar6Q74a_Q@mail.gmail.com>
In-Reply-To: <20171029160857.29460-2-anarcat@debian.org>

On Sun, Oct 29, 2017 at 12:08 PM, Antoine Beaupré <anarcat@debian.org> wrote:

> From: Kevin <kevin@ki-ai.org>
>
> 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/

Show 10 quoted lines

> taken from all the specified namespaces.
>
> Reviewed-by: Antoine Beaupré <anarcat@debian.org>
> Signed-off-by: Antoine Beaupré <anarcat@debian.org>
> ---
> 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).