Re: [PATCHv3 1/2] git-remote-mediawiki: import "File:" attachments
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jun 12, 2012, 21:05 UTC
- Message-ID
- <7vy5nsi6lq.fsf@alter.siamese.dyndns.org>
- In-Reply-To
- <1339532091-25232-1-git-send-email-Pavel.Volek@ensimag.imag.fr>
Pavel Volek <Pavel.Volek@ensimag.imag.fr> writes:
> From: Pavel VOlek <Pavel.Volek@ensimag.imag.fr>
Did you really mean this? It does not match your S-o-b: line below.
Show 9 quoted lines
> > The current version of the git-remote-mediawiki supports only import and export > of the pages, doesn't support import and export of file attachments which are > also exposed by MediaWiki API. This patch adds the functionality to import file > attachments and description pages for these files. > > Chages version2 -> version3: > Fixes in comments. > Variable '$file' -> '$file_content' refactoring to be clearer.
These three lines do not belong here above the three-dash lines, I think.
Show 19 quoted lines
> Signed-off-by: Pavel Volek <Pavel.Volek@ensimag.imag.fr> > Signed-off-by: NGUYEN Kim Thuat <Kim-Thuat.Nguyen@ensimag.imag.fr> > Signed-off-by: ROUCHER IGLESIAS Javier <roucherj@ensimag.imag.fr> > Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr> > --- > contrib/mw-to-git/git-remote-mediawiki | 223 ++++++++++++++++++++++++++++++++- > 1 file changed, 218 insertions(+), 5 deletions(-) > > diff --git a/contrib/mw-to-git/git-remote-mediawiki b/contrib/mw-to-git/git-remote-mediawiki > index c18bfa1..04d3959 100755 > --- a/contrib/mw-to-git/git-remote-mediawiki > +++ b/contrib/mw-to-git/git-remote-mediawiki > @@ -13,9 +13,6 @@ > # > # Known limitations: > # > -# - Only wiki pages are managed, no support for [[File:...]] > -# attachments. > -#
Nice.
Show 18 quoted lines
> @@ -71,6 +68,9 @@ chomp(@tracked_pages);
> my @tracked_categories = split(/[ \n]/, run_git("config --get-all remote.". $remotename .".categories"));
> chomp(@tracked_categories);
>
> +# Import media files too.
> +my $import_media = run_git("config --get --bool remote.". $remotename .".mediaimport");
> +
> my $wiki_login = run_git("config --get remote.". $remotename .".mwLogin");
> # TODO: ideally, this should be able to read from keyboard, but we're
> # inside a remote helper, so our stdin is connect to git, not to a
> @@ -225,6 +225,10 @@ sub get_mw_pages {
> get_mw_first_pages(\@slice, \%pages);
> @some_pages = @some_pages[51..$#some_pages];
> }
> +
> + if ($import_media) {
> + get_mw_pages_for_linked_mediafiles(\@tracked_pages, \%pages);
> + }I am guessing that the loop above is to avoid fetching and processing too many pages at once. Doesn't the call to get_mw_pages_for_linked_mediafiles() need a similar consideration, or what the function does is significantly different from what get_mw_first_pages() does and there is no need to worry?
By the way, does it really have to be that overly long name?
Show 37 quoted lines
> @@ -244,6 +248,11 @@ sub get_mw_pages {
> foreach my $page (@{$mw_pages}) {
> $pages{$page->{title}} = $page;
> }
> +
> + if ($import_media) {
> + my @titles = map $_->{title}, @{$mw_pages};
> + get_mw_pages_for_linked_mediafiles(\@titles, \%pages);
> + }
> }
> }
> if (!$user_defined) {
> @@ -263,10 +272,186 @@ sub get_mw_pages {
> foreach my $page (@{$mw_pages}) {
> $pages{$page->{title}} = $page;
> }
> +
> + if ($import_media) {
> + # Attach list of all pages for media files from the API,
> + # they are in a different namespace, only one namespace
> + # can be queried at the same moment
> + my $mw_pages = $mediawiki->list({
> + action => 'query',
> + list => 'allpages',
> + apnamespace => get_mw_namespace_id("File"),
> + aplimit => 500
> + });
> + 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";
> + exit 1;
> + }
> + foreach my $page (@{$mw_pages}) {
> + $pages{$page->{title}} = $page;
> + }
> + }For categories you need to call pages-for-mediafiles with the titles you learned (the hunk starting at l.224), but there is no need to call pages-for-mediafiles in this hunk?
Not a rhetorical question to suggest that you should; just wondering.
> +sub get_mw_pages_for_linked_mediafiles {
> + my $titles = shift;
> + my @titles = @{$titles};Do you really need to make a copy of this array? Wouldn't it suffice to say
my $mw_titles = join('|', @$titles);at the only location in this function that uses this parameter?
> + my $pages = shift;
> +
> + # pattern 'page1|page2|...' required by the API
> + my $mw_titles = join('|', @titles);Nobody seems to be making sure there won't be more than 500 (I am assuming that this script is considered a 'bot) pages in $mw_titles variable. Shouldn't the API call be split into such batches?
Show 50 quoted lines
> + # Media files could be included or linked from
> + # a page, get all related
> + my $query = {
> + action => 'query',
> + prop => 'links|images',
> + titles => $mw_titles,
> + plnamespace => get_mw_namespace_id("File"),
> + pllimit => 500
> + };
> + my $result = $mediawiki->api($query);
> +
> + while (my ($id, $page) = each(%{$result->{query}->{pages}})) {
> + my @titles;
> + if (defined($page->{links})) {
> + my @link_titles = map $_->{title}, @{$page->{links}};
> + push(@titles, @link_titles);
> + }
> + if (defined($page->{images})) {
> + my @image_titles = map $_->{title}, @{$page->{images}};
> + push(@titles, @image_titles);
> + }
> + if (@titles) {
> + get_mw_first_pages(\@titles, \%{$pages});
> + }
> + }
> +}
> +
> +# Return MediaWiki id for a canonical namespace name.
> +# Ex.: "File", "Project".
> +# Looks for the namespace id in the local configuration
> +# variables, if it is not found asks MW API.
> +sub get_mw_namespace_id {
> + mw_connect_maybe();
> +
> + my $name = shift;
> +
> + # Look at configuration file, if the record
> + # for that namespace is already stored.
> + # Namespaces are stored in form: "Name_of_namespace:Id_namespace",
> + # Ex.: "File:6".
> + my @tracked_namespaces = split(/[ \n]/, run_git("config --get-all remote.". $remotename .".namespaces"));
> + chomp(@tracked_namespaces);
> + if (@tracked_namespaces) {
> + foreach my $ns (@tracked_namespaces) {
> + my @ns_split = split(/:/, $ns);
> + if ($ns_split[0] eq $name) {
> + return $ns_split[1];
> + }
> + }
> + }Does it make sense to call out to run_git() every time this function is called? Shoudln't this part be caching the result in a hash, something like
if (!exists $namespace_id{$name}) {
@temp = ... run_git() ...;
foreach my $ns (@temp) {
my ($n, $s) = split(/:/, $ns);
$namespace_id{$n} = $s;
}
} if (!exists $namespace_id{$name}) {
... similarly, ask MW API and store in %namespace_id{}
} if (exists $namespace_id{$name}) {
return $namespace_id{$name};
}
die "No such namespace $name";