threads / patch / 34998

v2git-remote-mediawiki: bugfix for pages w/ >500 revisions

Subject: [PATCH v2] git-remote-mediawiki: bugfix for pages w/ >500 revisions

## tl;dr

5 messages between Sep 23, 2013 and Sep 24, 2013. Diffs are folded; open one to read it.

replies: 4people: 3as markdown or json

Benoit Person· Sep 23, 2013, 17:26 UTC · lore

Mediawiki introduces a new API for queries w/ more than 500 results in version 1.21. That change triggered an infinite loop while cloning a mediawiki with such a page.

The latest API renamed and moved the "continuing" information in the response, necessary to build the next query. The code failed to retrieve that information but still detected that it was in a "continuing query". As a result, it launched the same query over and over again.

If a "continuing" information is detected in the response (old or new), the next query is updated accordingly. If not, we quit assuming it's not a continuing query.

Signed-off-by: Benoit Person <benoit.person@gmail.fr>
Reported-by: Benjamin Cathey
---
 contrib/mw-to-git/git-remote-mediawiki.perl     | 14 ++++++++++++--
 contrib/mw-to-git/t/t9365-continuing-queries.sh | 24 ++++++++++++++++++++++++
 2 files changed, 36 insertions(+), 2 deletions(-)
 create mode 100755 contrib/mw-to-git/t/t9365-continuing-queries.sh
Show changes to 2 files +36 −2

contrib/mw-to-git/git-remote-mediawiki.perl, contrib/mw-to-git/t/t9365-continuing-queries.sh

diff --git a/contrib/mw-to-git/git-remote-mediawiki.perl b/contrib/mw-to-git/git-remote-mediawiki.perl
index c9a4805..f1db5d2 100755
--- a/contrib/mw-to-git/git-remote-mediawiki.perl
+++ b/contrib/mw-to-git/git-remote-mediawiki.perl
@@ -625,6 +625,9 @@ sub fetch_mw_revisions_for_page {
 		rvstartid => $fetch_from,
 		rvlimit => 500,
 		pageids => $id,
+
+		# let the mediawiki know that we support the latest API
+		continue => '',
 	};
 
 	my $revnum = 0;
@@ -640,8 +643,15 @@ sub fetch_mw_revisions_for_page {
 			push(@page_revs, $page_rev_ids);
 			$revnum++;
 		}
-		last if (!$result->{'query-continue'});
-		$query->{rvstartid} = $result->{'query-continue'}->{revisions}->{rvstartid};
+
+		if ($result->{'query-continue'}) { # For legacy APIs
+			$query->{rvstartid} = $result->{'query-continue'}->{revisions}->{rvstartid};
+		} elsif ($result->{continue}) { # For newer APIs
+			$query->{rvstartid} = $result->{continue}->{rvcontinue};
+			$query->{continue} = $result->{continue}->{continue};
+		} else {
+			last;
+		}
 	}
 	if ($shallow_import && @page_revs) {
 		print {*STDERR} "  Found 1 revision (shallow import).\n";
diff --git a/contrib/mw-to-git/t/t9365-continuing-queries.sh b/contrib/mw-to-git/t/t9365-continuing-queries.sh
new file mode 100755
index 0000000..6fb5df4
--- /dev/null
+++ b/contrib/mw-to-git/t/t9365-continuing-queries.sh
@@ -0,0 +1,24 @@
+#!/bin/sh
+
+test_description='Test the Git Mediawiki remote helper: queries w/ more than 500 results'
+
+. ./test-gitmw-lib.sh
+. ./push-pull-tests.sh
+. $TEST_DIRECTORY/test-lib.sh
+
+test_check_precond
+
+test_expect_success 'creating page w/ >500 revisions' '
+	wiki_reset &&
+	for i in $(seq 1 501)
+	do
+		echo "creating revision $i"
+		wiki_editpage foo "revision $i<br/>" true
+	done
+'
+
+test_expect_success 'cloning page w/ >500 revisions' '
+	git clone mediawiki::'"$WIKI_URL"' mw_dir
+'
+
+test_done

-- 
1.8.4.GIT
Matthieu Moy· Sep 23, 2013, 17:58 UTC · re: Benoit Person · lore

Re: [PATCH v2] git-remote-mediawiki: bugfix for pages w/ >500 revisions

Benoit Person <benoit.person@gmail.com> writes:
> The latest API renamed and moved the "continuing" information in the
> response, necessary to build the next query. The code failed to retrieve
> that information but still detected that it was in a "continuing
> query". As a result, it launched the same query over and over again.
Good, that's much cleaner.
> +		if ($result->{'query-continue'}) { # For legacy APIs
> +			$query->{rvstartid} = $result->{'query-continue'}->{revisions}->{rvstartid};
> +		} elsif ($result->{continue}) { # For newer APIs
I'd rather have the comments say "# API version < X" and "# API version
>= X". Next time the API change, "new" Vs "old" will become meaningless.
(But that should not block the patch from inclusion)
Thanks,
-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/
Eric Sunshine· Sep 23, 2013, 20:17 UTC · re: Benoit Person · lore

Re: [PATCH v2] git-remote-mediawiki: bugfix for pages w/ >500 revisions

On Mon, Sep 23, 2013 at 1:26 PM, Benoit Person <benoit.person@gmail.com> wrote:
Show 19 quoted lines
> diff --git a/contrib/mw-to-git/t/t9365-continuing-queries.sh b/contrib/mw-to-git/t/t9365-continuing-queries.sh
> new file mode 100755
> index 0000000..6fb5df4
> --- /dev/null
> +++ b/contrib/mw-to-git/t/t9365-continuing-queries.sh
> @@ -0,0 +1,24 @@
> +#!/bin/sh
> +
> +test_description='Test the Git Mediawiki remote helper: queries w/ more than 500 results'
> +
> +. ./test-gitmw-lib.sh
> +. ./push-pull-tests.sh
> +. $TEST_DIRECTORY/test-lib.sh
> +
> +test_check_precond
> +
> +test_expect_success 'creating page w/ >500 revisions' '
> +       wiki_reset &&
> +       for i in $(seq 1 501)
s/seq/test_seq/
d17cf5f3a32f07bf (tests: Introduce test_seq;  2012-08-03)
> +       do
> +               echo "creating revision $i"
Do you want to end this line with '&&'?
Show 10 quoted lines
> +               wiki_editpage foo "revision $i<br/>" true
> +       done
> +'
> +
> +test_expect_success 'cloning page w/ >500 revisions' '
> +       git clone mediawiki::'"$WIKI_URL"' mw_dir
> +'
> +
> +test_done
> --
Benoit Person· Sep 24, 2013, 08:05 UTC · re: Eric Sunshine · lore

Re: [PATCH v2] git-remote-mediawiki: bugfix for pages w/ >500 revisions

On 23 September 2013 19:58, Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> wrote:
> I'd rather have the comments say "# API version < X" and "# API version
>>= X". Next time the API change, "new" Vs "old" will become meaningless.
done, thanks
On 23 September 2013 20:26, Jonathan Nieder <jrnieder@gmail.com> wrote:
Show 8 quoted lines
> Some distros (e.g., Debian) occasionally do run the testsuite
> automatically, but it is still fine since they have a timeout that
> varies by platform to detect if the test has stalled.  I suppose
> ideally git's test harness could learn to do the same thing some day,
> but for now it's easier one level above since an appropriate timeout
> depends on the speed on the platform, what else is creating load on
> the test machine, and other factors that are probably not easy for us
> to guess.
great explanation, thanks
On 23 September 2013 22:17, Eric Sunshine <sunshine@sunshineco.com> wrote:
> s/seq/test_seq/
done, thanks
Show 6 quoted lines
> d17cf5f3a32f07bf (tests: Introduce test_seq;  2012-08-03)
>
>> +       do
>> +               echo "creating revision $i"
>
> Do you want to end this line with '&&'?

The way it's intended is that it's more a debug information to see how it's going on (creating >500 revs is *quite* long). If I understand it correctly, using '&&' would mean that the return value of the echo statement will be tested for success ? Anyway, I am not sure it makes sense to fail on a "debug echo" ?

-- 
Benoit Person
Matthieu Moy· Sep 24, 2013, 08:19 UTC · re: Benoit Person · lore

Re: [PATCH v2] git-remote-mediawiki: bugfix for pages w/ >500 revisions

Benoit Person <benoit.person@gmail.com> writes:
Show 11 quoted lines
>> d17cf5f3a32f07bf (tests: Introduce test_seq;  2012-08-03)
>>
>>> +       do
>>> +               echo "creating revision $i"
>>
>> Do you want to end this line with '&&'?
> The way it's intended is that it's more a debug information to see how
> it's going on (creating >500 revs is *quite* long). If I understand it
> correctly, using '&&' would mean that the return value of the echo
> statement will be tested for success ? Anyway, I am not sure it makes
> sense to fail on a "debug echo" ?

I don't think you should bother with such reasoning: just put the && everywhere.

Without the &&, it's OK in the current version, but what if someone adds something before the echo? Then it becomes

git something-important && echo debug git something-else-important

and a failure on the first call is unnoticed because of the missing && afterwards.

The echo is not supposed to fail, so either the && does not change anything, or something went terribly wrong and you'd rather notice it.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

← back to recent threads