From: "Antoine Beaupré" <anarcat@debian.org>
To: Eric Sunshine <sunshine@sunshineco.com>
Cc: Git List <git@vger.kernel.org>
Subject: Re: [PATCH 4/4] remote-mediawiki: allow using (Main) as a namespace and skip special namespaces
Date: Sun, 29 Oct 2017 22:11:00 -0400 [thread overview]
Message-ID: <874lqh8jwr.fsf@curie.anarc.at> (raw)
In-Reply-To: <CAPig+cSTp1Udo6xXk5-L6MpWBdiy4sPO__NcND03-89EvRgLHQ@mail.gmail.com>
On 2017-10-29 15:49:28, Eric Sunshine wrote:
> 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).
Agreed, I'll try to do that.
> More below...
>
>> 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...
> }
Ah yes. I see your point but it doesn't actually skip the query when it
encouters main ($namespace_id >= 0).
>> + 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?
This is a known issue that permeates the whole remote at this point, and
it is quite annoying.
https://github.com/Git-Mediawiki/Git-Mediawiki/issues/30
I have, however, considered it useful to include this to show progress
as it can take a while to fetch all namespace information...
Obviously, once we figure out how to silence this stuff (ie. how to
recognize -q), it should be silenced like everything else, but until
then I think it's quite useful.
>> + 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?
Maybe I didn't get the indentation right, but this } else { is for query
failures, *not* the if ($namespace_id < 0). So < 0 is just silently
skipped.
The original code was die()'ing on failures, but I think that's a
mistake: we should fetch what we can and warn on the failures. That
allows the user to fix multiple problems at once instead of having to
rerun the script repeatedly.
A.
--
Le féminisme n'a jamais tué personne
Le machisme tue tous les jours.
- Benoîte Groulx
next prev parent reply other threads:[~2017-10-30 2:11 UTC|newest]
Thread overview: 78+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-10-29 16:08 [PATCH 0/4] WIP: git-remote-media wiki namespace support Antoine Beaupré
2017-10-29 16:08 ` [PATCH 1/4] remote-mediawiki: add " Antoine Beaupré
2017-10-29 17:24 ` Eric Sunshine
2017-10-29 18:29 ` Antoine Beaupré
2017-10-29 20:07 ` Eric Sunshine
2017-10-29 23:08 ` Kevin
2017-10-30 2:14 ` Antoine Beaupré
2017-10-30 2:51 ` [PATCH v2 0/7] " Antoine Beaupré
2017-10-30 2:51 ` [PATCH 1/7] " Antoine Beaupré
2017-10-30 2:51 ` [PATCH 2/7] remote-mediawiki: allow fetching namespaces with spaces Antoine Beaupré
2017-10-30 2:51 ` [PATCH 3/7] remote-mediawiki: show known namespace choices on failure Antoine Beaupré
2017-10-30 2:51 ` [PATCH 4/7] remote-mediawiki: skip virtual namespaces Antoine Beaupré
2017-11-01 13:52 ` Eric Sunshine
2017-11-01 16:45 ` Antoine Beaupré
2017-11-02 1:24 ` Junio C Hamano
2017-11-02 21:20 ` Antoine Beaupré
2017-11-06 0:38 ` Junio C Hamano
2017-10-30 2:51 ` [PATCH 5/7] remote-mediawiki: support fetching from (Main) namespace Antoine Beaupré
2017-11-01 19:56 ` Eric Sunshine
2017-11-02 21:19 ` Antoine Beaupré
2017-10-30 2:51 ` [PATCH 6/7] remote-mediawiki: process namespaces in order Antoine Beaupré
2017-11-01 19:59 ` Eric Sunshine
2017-10-30 2:51 ` [PATCH 7/7] remote-mediawiki: show progress while fetching namespaces Antoine Beaupré
2017-11-01 20:01 ` Eric Sunshine
2017-11-02 21:25 ` [PATCH v3 0/7] remote-mediawiki: namespace support Antoine Beaupré
2017-11-02 21:25 ` [PATCH v3 1/7] remote-mediawiki: add " Antoine Beaupré
2017-11-02 21:25 ` [PATCH v3 2/7] remote-mediawiki: allow fetching namespaces with spaces Antoine Beaupré
2017-11-02 21:25 ` [PATCH v3 3/7] remote-mediawiki: show known namespace choices on failure Antoine Beaupré
2017-11-02 21:25 ` [PATCH v3 4/7] remote-mediawiki: skip virtual namespaces Antoine Beaupré
2017-11-02 22:43 ` Eric Sunshine
2017-11-02 22:54 ` Antoine Beaupré
2017-11-02 21:25 ` [PATCH v3 5/7] remote-mediawiki: support fetching from (Main) namespace Antoine Beaupré
2017-11-02 22:48 ` Eric Sunshine
2017-11-02 21:25 ` [PATCH v3 6/7] remote-mediawiki: process namespaces in order Antoine Beaupré
2017-11-02 22:49 ` Eric Sunshine
2017-11-02 21:25 ` [PATCH v3 7/7] remote-mediawiki: show progress while fetching namespaces Antoine Beaupré
2017-11-02 22:18 ` Thomas Adam
2017-11-02 22:26 ` Antoine Beaupré
2017-11-02 22:31 ` Thomas Adam
2017-11-02 23:10 ` Antoine Beaupré
2017-11-04 9:57 ` Thomas Adam
2017-11-02 22:50 ` Eric Sunshine
2017-11-06 21:19 ` [PATCH v4 0/7] remote-mediawiki: namespace support Antoine Beaupré
2017-11-06 21:19 ` [PATCH v4 1/7] remote-mediawiki: add " Antoine Beaupré
2017-11-06 21:19 ` [PATCH v4 2/7] remote-mediawiki: allow fetching namespaces with spaces Antoine Beaupré
2017-11-07 7:08 ` Thomas Adam
2017-11-07 16:03 ` Antoine Beaupré
2017-11-06 21:19 ` [PATCH v4 3/7] remote-mediawiki: show known namespace choices on failure Antoine Beaupré
2017-11-07 10:45 ` Thomas Adam
2017-11-07 16:07 ` Antoine Beaupré
2017-11-06 21:19 ` [PATCH v4 4/7] remote-mediawiki: skip virtual namespaces Antoine Beaupré
2017-11-06 21:19 ` [PATCH v4 5/7] remote-mediawiki: support fetching from (Main) namespace Antoine Beaupré
2017-11-06 21:19 ` [PATCH v4 6/7] remote-mediawiki: process namespaces in order Antoine Beaupré
2017-11-06 21:19 ` [PATCH v4 7/7] remote-mediawiki: show progress while fetching namespaces Antoine Beaupré
2017-11-07 16:06 ` [PATCH v5 0/7] namespace support Antoine Beaupré
2017-11-07 16:06 ` [PATCH v5 1/7] remote-mediawiki: add " Antoine Beaupré
2017-11-07 16:06 ` [PATCH v5 2/7] remote-mediawiki: allow fetching namespaces with spaces Antoine Beaupré
2017-11-07 16:06 ` [PATCH v5 3/7] remote-mediawiki: show known namespace choices on failure Antoine Beaupré
2017-11-07 16:06 ` [PATCH v5 4/7] remote-mediawiki: skip virtual namespaces Antoine Beaupré
2017-11-07 16:06 ` [PATCH v5 5/7] remote-mediawiki: support fetching from (Main) namespace Antoine Beaupré
2017-11-07 16:07 ` [PATCH v5 6/7] remote-mediawiki: process namespaces in order Antoine Beaupré
2017-11-07 16:07 ` [PATCH v5 7/7] remote-mediawiki: show progress while fetching namespaces Antoine Beaupré
2017-11-08 2:07 ` [PATCH v5 0/7] namespace support Junio C Hamano
2017-10-30 10:43 ` [PATCH 1/4] remote-mediawiki: add " Matthieu Moy
2017-10-29 16:08 ` [PATCH 2/4] remote-mediawiki: allow fetching namespaces with spaces Antoine Beaupré
2017-10-29 16:08 ` [PATCH 3/4] remote-mediawiki: show known namespace choices on failure Antoine Beaupré
2017-10-29 17:34 ` Eric Sunshine
2017-10-29 18:31 ` Antoine Beaupré
2017-11-04 10:57 ` Thomas Adam
2017-10-29 16:08 ` [PATCH 4/4] remote-mediawiki: allow using (Main) as a namespace and skip special namespaces Antoine Beaupré
2017-10-29 19:49 ` Eric Sunshine
2017-10-30 2:11 ` Antoine Beaupré [this message]
2017-10-30 3:49 ` Eric Sunshine
2017-10-30 2:43 ` Antoine Beaupré
2017-10-30 3:52 ` Eric Sunshine
2017-10-30 12:17 ` Antoine Beaupré
2017-10-30 10:40 ` [PATCH 0/4] WIP: git-remote-media wiki namespace support Matthieu Moy
2017-10-30 12:20 ` Antoine Beaupré
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=874lqh8jwr.fsf@curie.anarc.at \
--to=anarcat@debian.org \
--cc=git@vger.kernel.org \
--cc=sunshine@sunshineco.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).