Re: [PATCH 4/4] remote-mediawiki: allow using (Main) as a namespace and skip special namespaces

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Oct 29, 2017, 19:49 UTC
Message-ID: <CAPig+cSTp1Udo6xXk5-L6MpWBdiy4sPO__NcND03-89EvRgLHQ@mail.gmail.com>
In-Reply-To: <20171029160857.29460-5-anarcat@debian.org>

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

> Subject: remote-mediawiki: allow using (Main) as a namespace and skip special namespaces

This patch is more difficult to review than it perhaps ought to be since it is making multiple unrelated changes.

It's not clear from the description what special namespaces are and why they need to be skipped. It's also not clear why (Main) is special. Perhaps the commit message(s) could explain these issues in more detail.

To simplify review and make it easier to gauge what it going on, it might make sense to split this patch into at least two: one which skips "special namespaces", and one which gives special treatment to (Main).

More below...

Show 25 quoted lines

> 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
> @@ -264,16 +264,27 @@ sub get_mw_tracked_categories {
>  sub get_mw_tracked_namespaces {
>      my $pages = shift;
> -    foreach my $local_namespace (@tracked_namespaces) {
> -        my $mw_pages = $mediawiki->list( {
> -            action => 'query',
> -            list => 'allpages',
> -            apnamespace => get_mw_namespace_id($local_namespace),
> -            aplimit => 'max' } )
> -            || die $mediawiki->{error}->{code} . ': '
> -                . $mediawiki->{error}->{details} . "\n";
> -        foreach my $page (@{$mw_pages}) {
> -            $pages->{$page->{title}} = $page;
> +    foreach my $local_namespace (sort @tracked_namespaces) {
> +        my ($mw_pages, $namespace_id);
> +        if ($local_namespace eq "(Main)") {
> +            $namespace_id = 0;
> +        } else {
> +            $namespace_id = get_mw_namespace_id($local_namespace);
> +        }
> +        if ($namespace_id >= 0) {

This may be problematic since get_mw_namespace_id() may return undef rather than a number, in which case Perl will complain. Since the code skips the $mediawiki query altogether when it encounters "(Main)", you could fix this problem and simplify the code overall by simply skipping the bulk of the foreach loop body instead of mucking around with $namespace_id. For instance:

    foreach my $local_namespace (sort @tracked_namespaces) {
        next if ($local_namespace eq "(Main)");
        ...normal processing...
    }

Show 6 quoted lines

> +            if ($mw_pages = $mediawiki->list( {
> +                action => 'query',
> +                list => 'allpages',
> +                apnamespace => $namespace_id,
> +                aplimit => 'max' } )) {
> +                print {*STDERR} "$#{$mw_pages} found in namespace $local_namespace ($namespace_id)\n";

The original code did not emit this diagnostic but the new code does so unconditionally. Is this just leftover debugging code or is intended that all users should see this information all the time?

Show 6 quoted lines

> +                foreach my $page (@{$mw_pages}) {
> +                    $pages->{$page->{title}} = $page;
> +                }
> +            } else {
> +                warn $mediawiki->{error}->{code} . ': '
> +                    . $mediawiki->{error}->{details} . "\n";

I guess this is the part which "skips special namespaces". The original code die()'d but this merely warns. Aside from these "special namespaces", are there genuine cases when the $mediawiki query would return an error, and which should indeed die(), or is warning appropriate for all $mediawiki query error cases?

> +            }
>          }
>      }
>      return;