{"thread":{"id":"34998","subject":"[PATCH v2] git-remote-mediawiki: bugfix for pages w/ >500 revisions","startedAt":"2013-09-23T17:26:15Z","lastAt":"2013-09-24T08:19:02Z","messageCount":5,"participants":["Benoit Person","Matthieu Moy","Eric Sunshine"],"isPatch":true,"patchVersion":2,"patchTotal":null},"messages":[{"id":"228073","messageId":"1379957175-8606-1-git-send-email-benoit.person@gmail.fr","threadId":"34998","inReplyTo":null,"subject":"[PATCH v2] git-remote-mediawiki: bugfix for pages w/ >500 revisions","fromName":"Benoit Person","fromEmail":"benoit.person@gmail.com","sentAt":"2013-09-23T17:26:15Z","receivedAt":"2013-09-23T17:26:15Z","isPatch":true,"sender":{"key":"benoit.person@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1145270?v=4"},"body":"Mediawiki introduces a new API for queries w/ more than 500 results in\nversion 1.21. That change triggered an infinite loop while cloning a\nmediawiki with such a page.\n\nThe latest API renamed and moved the \"continuing\" information in the\nresponse, necessary to build the next query. The code failed to retrieve\nthat information but still detected that it was in a \"continuing\nquery\". As a result, it launched the same query over and over again.\n\nIf a \"continuing\" information is detected in the response (old or new),\nthe next query is updated accordingly. If not, we quit assuming it's not\na continuing query.\n\nSigned-off-by: Benoit Person \nReported-by: Benjamin Cathey\n---\n contrib/mw-to-git/git-remote-mediawiki.perl | 14 ++++++++++++--\n contrib/mw-to-git/t/t9365-continuing-queries.sh | 24 ++++++++++++++++++++++++\n 2 files changed, 36 insertions(+), 2 deletions(-)\n create mode 100755 contrib/mw-to-git/t/t9365-continuing-queries.sh\n\ndiff --git a/contrib/mw-to-git/git-remote-mediawiki.perl b/contrib/mw-to-git/git-remote-mediawiki.perl\nindex c9a4805..f1db5d2 100755\n--- a/contrib/mw-to-git/git-remote-mediawiki.perl\n+++ b/contrib/mw-to-git/git-remote-mediawiki.perl\n@@ -625,6 +625,9 @@ sub fetch_mw_revisions_for_page {\n \t\trvstartid => $fetch_from,\n \t\trvlimit => 500,\n \t\tpageids => $id,\n+\n+\t\t# let the mediawiki know that we support the latest API\n+\t\tcontinue => '',\n \t};\n \n \tmy $revnum = 0;\n@@ -640,8 +643,15 @@ sub fetch_mw_revisions_for_page {\n \t\t\tpush(@page_revs, $page_rev_ids);\n \t\t\t$revnum++;\n \t\t}\n-\t\tlast if (!$result->{'query-continue'});\n-\t\t$query->{rvstartid} = $result->{'query-continue'}->{revisions}->{rvstartid};\n+\n+\t\tif ($result->{'query-continue'}) { # For legacy APIs\n+\t\t\t$query->{rvstartid} = $result->{'query-continue'}->{revisions}->{rvstartid};\n+\t\t} elsif ($result->{continue}) { # For newer APIs\n+\t\t\t$query->{rvstartid} = $result->{continue}->{rvcontinue};\n+\t\t\t$query->{continue} = $result->{continue}->{continue};\n+\t\t} else {\n+\t\t\tlast;\n+\t\t}\n \t}\n \tif ($shallow_import && @page_revs) {\n \t\tprint {*STDERR} \" Found 1 revision (shallow import).\\n\";\ndiff --git a/contrib/mw-to-git/t/t9365-continuing-queries.sh b/contrib/mw-to-git/t/t9365-continuing-queries.sh\nnew file mode 100755\nindex 0000000..6fb5df4\n--- /dev/null\n+++ b/contrib/mw-to-git/t/t9365-continuing-queries.sh\n@@ -0,0 +1,24 @@\n+#!/bin/sh\n+\n+test_description='Test the Git Mediawiki remote helper: queries w/ more than 500 results'\n+\n+. ./test-gitmw-lib.sh\n+. ./push-pull-tests.sh\n+. $TEST_DIRECTORY/test-lib.sh\n+\n+test_check_precond\n+\n+test_expect_success 'creating page w/ >500 revisions' '\n+\twiki_reset &&\n+\tfor i in $(seq 1 501)\n+\tdo\n+\t\techo \"creating revision $i\"\n+\t\twiki_editpage foo \"revision $i
\" true\n+\tdone\n+'\n+\n+test_expect_success 'cloning page w/ >500 revisions' '\n+\tgit clone mediawiki::'\"$WIKI_URL\"' mw_dir\n+'\n+\n+test_done\n-- \n1.8.4.GIT\n"},{"id":"228074","messageId":"vpqr4cf773c.fsf@anie.imag.fr","threadId":"34998","inReplyTo":"1379957175-8606-1-git-send-email-benoit.person@gmail.fr","subject":"Re: [PATCH v2] git-remote-mediawiki: bugfix for pages w/ >500 revisions","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2013-09-23T17:58:47Z","receivedAt":"2013-09-23T17:58:47Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Benoit Person writes:\n\n> The latest API renamed and moved the \"continuing\" information in the\n> response, necessary to build the next query. The code failed to retrieve\n> that information but still detected that it was in a \"continuing\n> query\". As a result, it launched the same query over and over again.\n\nGood, that's much cleaner.\n\n> +\t\tif ($result->{'query-continue'}) { # For legacy APIs\n> +\t\t\t$query->{rvstartid} = $result->{'query-continue'}->{revisions}->{rvstartid};\n> +\t\t} elsif ($result->{continue}) { # For newer APIs\n\nI'd rather have the comments say \"# API version < X\" and \"# API version\n>= X\". Next time the API change, \"new\" Vs \"old\" will become meaningless.\n\n(But that should not block the patch from inclusion)\n\nThanks,\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"228102","messageId":"CAPig+cSVeJX2wHo7TTJe2oa+_pD7EPdmMo_BGO_Bj-ZKgRD6wA@mail.gmail.com","threadId":"34998","inReplyTo":"1379957175-8606-1-git-send-email-benoit.person@gmail.fr","subject":"Re: [PATCH v2] git-remote-mediawiki: bugfix for pages w/ >500 revisions","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2013-09-23T20:17:46Z","receivedAt":"2013-09-23T20:17:46Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Sep 23, 2013 at 1:26 PM, Benoit Person wrote:\n> diff --git a/contrib/mw-to-git/t/t9365-continuing-queries.sh b/contrib/mw-to-git/t/t9365-continuing-queries.sh\n> new file mode 100755\n> index 0000000..6fb5df4\n> --- /dev/null\n> +++ b/contrib/mw-to-git/t/t9365-continuing-queries.sh\n> @@ -0,0 +1,24 @@\n> +#!/bin/sh\n> +\n> +test_description='Test the Git Mediawiki remote helper: queries w/ more than 500 results'\n> +\n> +. ./test-gitmw-lib.sh\n> +. ./push-pull-tests.sh\n> +. $TEST_DIRECTORY/test-lib.sh\n> +\n> +test_check_precond\n> +\n> +test_expect_success 'creating page w/ >500 revisions' '\n> + wiki_reset &&\n> + for i in $(seq 1 501)\n\ns/seq/test_seq/\n\nd17cf5f3a32f07bf (tests: Introduce test_seq; 2012-08-03)\n\n> + do\n> + echo \"creating revision $i\"\n\nDo you want to end this line with '&&'?\n\n> + wiki_editpage foo \"revision $i
\" true\n> + done\n> +'\n> +\n> +test_expect_success 'cloning page w/ >500 revisions' '\n> + git clone mediawiki::'\"$WIKI_URL\"' mw_dir\n> +'\n> +\n> +test_done\n> --\n"},{"id":"228145","messageId":"CAETqRCjo3_nr0+Jh_0XTPZysfzhtGaeFMGswog--Era31jQo1Q@mail.gmail.com","threadId":"34998","inReplyTo":"CAPig+cSVeJX2wHo7TTJe2oa+_pD7EPdmMo_BGO_Bj-ZKgRD6wA@mail.gmail.com","subject":"Re: [PATCH v2] git-remote-mediawiki: bugfix for pages w/ >500 revisions","fromName":"Benoit Person","fromEmail":"benoit.person@gmail.com","sentAt":"2013-09-24T08:05:34Z","receivedAt":"2013-09-24T08:05:34Z","isPatch":true,"sender":{"key":"benoit.person@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1145270?v=4"},"body":"On 23 September 2013 19:58, Matthieu Moy wrote:\n> I'd rather have the comments say \"# API version < X\" and \"# API version\n>>= X\". Next time the API change, \"new\" Vs \"old\" will become meaningless.\ndone, thanks\n\n\nOn 23 September 2013 20:26, Jonathan Nieder wrote:\n> Some distros (e.g., Debian) occasionally do run the testsuite\n> automatically, but it is still fine since they have a timeout that\n> varies by platform to detect if the test has stalled. I suppose\n> ideally git's test harness could learn to do the same thing some day,\n> but for now it's easier one level above since an appropriate timeout\n> depends on the speed on the platform, what else is creating load on\n> the test machine, and other factors that are probably not easy for us\n> to guess.\ngreat explanation, thanks\n\n\nOn 23 September 2013 22:17, Eric Sunshine wrote:\n> s/seq/test_seq/\ndone, thanks\n\n> d17cf5f3a32f07bf (tests: Introduce test_seq; 2012-08-03)\n>\n>> + do\n>> + echo \"creating revision $i\"\n>\n> Do you want to end this line with '&&'?\nThe way it's intended is that it's more a debug information to see how\nit's going on (creating >500 revs is *quite* long). If I understand it\ncorrectly, using '&&' would mean that the return value of the echo\nstatement will be tested for success ? Anyway, I am not sure it makes\nsense to fail on a \"debug echo\" ?\n\n-- \nBenoit Person\n"},{"id":"228146","messageId":"vpqk3i67hu1.fsf@anie.imag.fr","threadId":"34998","inReplyTo":"CAETqRCjo3_nr0+Jh_0XTPZysfzhtGaeFMGswog--Era31jQo1Q@mail.gmail.com","subject":"Re: [PATCH v2] git-remote-mediawiki: bugfix for pages w/ >500 revisions","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2013-09-24T08:19:02Z","receivedAt":"2013-09-24T08:19:02Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Benoit Person writes:\n\n>> d17cf5f3a32f07bf (tests: Introduce test_seq; 2012-08-03)\n>>\n>>> + do\n>>> + echo \"creating revision $i\"\n>>\n>> Do you want to end this line with '&&'?\n> The way it's intended is that it's more a debug information to see how\n> it's going on (creating >500 revs is *quite* long). If I understand it\n> correctly, using '&&' would mean that the return value of the echo\n> statement will be tested for success ? Anyway, I am not sure it makes\n> sense to fail on a \"debug echo\" ?\n\nI don't think you should bother with such reasoning: just put the &&\neverywhere.\n\nWithout the &&, it's OK in the current version, but what if someone adds\nsomething before the echo? Then it becomes\n\ngit something-important &&\necho debug\ngit something-else-important\n\nand a failure on the first call is unnoticed because of the missing &&\nafterwards.\n\nThe echo is not supposed to fail, so either the && does not change\nanything, or something went terribly wrong and you'd rather notice it.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"}]}