From 1093c7e4b2ec5f3b4052ed5c1b5530560864a920 Mon Sep 17 00:00:00 2001 From: Joey Hess Date: Fri, 26 Mar 2010 00:57:46 -0400 Subject: new bug --- doc/bugs/depends_simple_mixup.mdwn | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) create mode 100644 doc/bugs/depends_simple_mixup.mdwn (limited to 'doc/bugs/depends_simple_mixup.mdwn') diff --git a/doc/bugs/depends_simple_mixup.mdwn b/doc/bugs/depends_simple_mixup.mdwn new file mode 100644 index 000000000..506bef904 --- /dev/null +++ b/doc/bugs/depends_simple_mixup.mdwn @@ -0,0 +1,18 @@ +The [[bugs]] page, at least before I commit this, has a bug at the top that +has been modified to link to done, and ikiwiki's dependency calculations +failed to notice and update the bugs page. Looking at the indexdb, I saw +that the page was not included in the `depends_simple` of the bugs page. + +I was able to replicate the problem locally by starting off with the page +not marked done (when it did appear in the bugs page `depends_simple` +(appropriatly as a link dependency, since a change to the page removing the +link would make it match)), then removing the done link. + +At that point, it vanished from `depends_simple`. Presumably because +the main (pagespec) depends for the bugs page now matched it, as a content +dependency. But, it seems to me it should still be listed in +`depends_simple` here. This, I think, is the cause of the bug. + +Then re-add the done link, and the dependency calc code breaks down, +not noticing that bugs dependeded on the page and needs to be updated. +--[[Joey]] -- cgit v1.2.3 From 4cb464d6e52344b8a0dfd29e82ede78aef1385d3 Mon Sep 17 00:00:00 2001 From: Joey Hess Date: Fri, 26 Mar 2010 01:05:22 -0400 Subject: typo --- doc/bugs/depends_simple_mixup.mdwn | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'doc/bugs/depends_simple_mixup.mdwn') diff --git a/doc/bugs/depends_simple_mixup.mdwn b/doc/bugs/depends_simple_mixup.mdwn index 506bef904..c2845240d 100644 --- a/doc/bugs/depends_simple_mixup.mdwn +++ b/doc/bugs/depends_simple_mixup.mdwn @@ -4,7 +4,7 @@ failed to notice and update the bugs page. Looking at the indexdb, I saw that the page was not included in the `depends_simple` of the bugs page. I was able to replicate the problem locally by starting off with the page -not marked done (when it did appear in the bugs page `depends_simple` +marked done (when it did appear in the bugs page `depends_simple` (appropriatly as a link dependency, since a change to the page removing the link would make it match)), then removing the done link. -- cgit v1.2.3 From 0d524ad672333fd0bafa64e81e261fd297c25580 Mon Sep 17 00:00:00 2001 From: Joey Hess Date: Fri, 26 Mar 2010 01:38:53 -0400 Subject: Fix incorrect influence info returned by a failing link() pagespec, that could lead to bad dependency handling in certian situations. --- IkiWiki.pm | 4 ++-- debian/changelog | 2 ++ doc/bugs/depends_simple_mixup.mdwn | 5 +++++ t/pagespec_match.t | 8 +++++++- 4 files changed, 16 insertions(+), 3 deletions(-) (limited to 'doc/bugs/depends_simple_mixup.mdwn') diff --git a/IkiWiki.pm b/IkiWiki.pm index 022bfe3bd..927d62940 100644 --- a/IkiWiki.pm +++ b/IkiWiki.pm @@ -2215,7 +2215,7 @@ sub match_link ($$;@) { my $from=exists $params{location} ? $params{location} : ''; my $links = $IkiWiki::links{$page}; - return IkiWiki::FailReason->new("$page has no links", "" => 1) + return IkiWiki::FailReason->new("$page has no links", $page => $IkiWiki::DEPEND_LINKS, "" => 1) unless $links && @{$links}; my $bestlink = IkiWiki::bestlink($from, $link); foreach my $p (@{$links}) { @@ -2232,7 +2232,7 @@ sub match_link ($$;@) { if match_glob($p_rel, $link, %params); } } - return IkiWiki::FailReason->new("$page does not link to $link", "" => 1); + return IkiWiki::FailReason->new("$page does not link to $link", $page => $IkiWiki::DEPEND_LINKS, "" => 1); } sub match_backlink ($$;@) { diff --git a/debian/changelog b/debian/changelog index da1ab890e..b9a105552 100644 --- a/debian/changelog +++ b/debian/changelog @@ -17,6 +17,8 @@ ikiwiki (3.20100324) UNRELEASED; urgency=low * Add --set-yaml switch for setting more complex config file options. * filecheck: Fix bugs that prevented the pagespecs from matching when not called by attachment plugin. + * Fix incorrect influence info returned by a failing link() pagespec, + that could lead to bad dependency handling in certian situations. -- Joey Hess Sat, 13 Mar 2010 14:48:10 -0500 diff --git a/doc/bugs/depends_simple_mixup.mdwn b/doc/bugs/depends_simple_mixup.mdwn index c2845240d..2603ff04c 100644 --- a/doc/bugs/depends_simple_mixup.mdwn +++ b/doc/bugs/depends_simple_mixup.mdwn @@ -15,4 +15,9 @@ dependency. But, it seems to me it should still be listed in Then re-add the done link, and the dependency calc code breaks down, not noticing that bugs dependeded on the page and needs to be updated. + +Ok.. Turns out this was not a problem with the actual influences +calculation or dependency calculation code. Whew! `match_link` +just didn't set the influence correctly when failing. [[fixed|done]] + --[[Joey]] diff --git a/t/pagespec_match.t b/t/pagespec_match.t index 8b0be4e8a..ade9bca5a 100755 --- a/t/pagespec_match.t +++ b/t/pagespec_match.t @@ -1,7 +1,7 @@ #!/usr/bin/perl use warnings; use strict; -use Test::More tests => 72; +use Test::More tests => 75; BEGIN { use_ok("IkiWiki"); } @@ -54,6 +54,7 @@ $config{userdir}=""; $links{foo}=[qw{bar baz}]; $links{bar}=[]; $links{baz}=[]; +$links{meh}=[]; $links{"bugs/foo"}=[qw{bugs/done}]; $links{"bugs/done"}=[]; $links{"bugs/bar"}=[qw{done}]; @@ -82,6 +83,7 @@ ok(! pagespec_match("bar", ""), "empty pagespec should match nothing"); ok(! pagespec_match("bar", " "), "blank pagespec should match nothing"); ok(pagespec_match("ook", "link(blog/tags/foo)"), "link internal absolute success"); ok(pagespec_match("ook", "link(/blog/tags/foo)"), "link explicit absolute success"); +ok(pagespec_match("meh", "!link(done)"), "negated failing match is a success"); $IkiWiki::pagectime{foo}=1154532692; # Wed Aug 2 11:26 EDT 2006 $IkiWiki::pagectime{bar}=1154532695; # after @@ -122,3 +124,7 @@ $i=pagespec_match("foo", "link(baz) and created_after(bar)")->influences; is(join(",", sort keys %$i), 'bar,foo', "influences add up over OR"); $i=pagespec_match("foo", "!link(baz) and !created_after(bar)")->influences; is(join(",", sort keys %$i), 'bar,foo', "influences unaffected by negation"); +$i=pagespec_match("foo", "!link(baz) and !created_after(bar)")->influences; +is(join(",", sort keys %$i), 'bar,foo', "influences unaffected by negation"); +$i=pagespec_match("meh", "!link(done)")->influences; +is(join(",", sort keys %$i), 'meh', "a negated, failing link test is successful, so the page is a link influence"); -- cgit v1.2.3 From 6c5f315970b0e7a8473e9a1151229459781192a8 Mon Sep 17 00:00:00 2001 From: Joey Hess Date: Wed, 21 Apr 2010 21:38:58 -0400 Subject: argh. head exploding. --- doc/bugs/depends_simple_mixup.mdwn | 38 +++++++++++++++++++++++++++++++++++++- 1 file changed, 37 insertions(+), 1 deletion(-) (limited to 'doc/bugs/depends_simple_mixup.mdwn') diff --git a/doc/bugs/depends_simple_mixup.mdwn b/doc/bugs/depends_simple_mixup.mdwn index 2603ff04c..2ebd53e85 100644 --- a/doc/bugs/depends_simple_mixup.mdwn +++ b/doc/bugs/depends_simple_mixup.mdwn @@ -18,6 +18,42 @@ not noticing that bugs dependeded on the page and needs to be updated. Ok.. Turns out this was not a problem with the actual influences calculation or dependency calculation code. Whew! `match_link` -just didn't set the influence correctly when failing. [[fixed|done]] +just didn't set the influence correctly when failing. fixed --[[Joey]] + +--- + +Update: Reopening this because the fix for it rather sucks. + +I made `match_link` return on failure an influence of +type DEPEND_LINKS. So, a tag page that inlines `tagged(foo)` +gets a `depends_simple` built up that contains link dependencies for +*every* page in the wiki. A very bloaty way to represent the dependency! + +Per [[dependency_types]], `link(done)` only needs to list in +`depends_simple` the pages that currently match. If a page is modified +to add the link, the regular dependency calculation code notices that +a new page matches. If a page that had the link is modified to remove it, +the `depends_simple` lets ikiwiki remember that the now non-matching page +matched before. + +Where that fell down was `!link(done)`. A page matching that was not added +to `depends_simple`, because the `link(done)` did not match it. If the page +is modified to add the link, the regular dependency calculation code +didn't notice, since the pagespec no longer matched. + +In this case, `depends_simple` needs to contain all pages +that do *not* match `link_done)`, but before my change, it contained +all pages that *do* match. After my change, it contained all pages. + +So, seems what is needed is a way for influence info to be manipulated by +the boolean operations that are applied. One way would be to have two +sets of influences be returned, one for successful matches, and one for +failed matches. Normally, these would be the same. For successful +`match_link`, the successful influence would be the page. +For failed `match_link`, the failed influence would be the page. + +Then, when NOTting a `*Reason`, swap the two sets of influences. +When ANDing/ORing, combine the individual sets. Querying the object for +influences should return only the successful influences. -- cgit v1.2.3 From 8cf6b7abf87818f9063b6ef672f20125de75249c Mon Sep 17 00:00:00 2001 From: Joey Hess Date: Wed, 21 Apr 2010 21:42:18 -0400 Subject: link fix --- doc/bugs/depends_simple_mixup.mdwn | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'doc/bugs/depends_simple_mixup.mdwn') diff --git a/doc/bugs/depends_simple_mixup.mdwn b/doc/bugs/depends_simple_mixup.mdwn index 2ebd53e85..4fe69a90a 100644 --- a/doc/bugs/depends_simple_mixup.mdwn +++ b/doc/bugs/depends_simple_mixup.mdwn @@ -31,7 +31,7 @@ type DEPEND_LINKS. So, a tag page that inlines `tagged(foo)` gets a `depends_simple` built up that contains link dependencies for *every* page in the wiki. A very bloaty way to represent the dependency! -Per [[dependency_types]], `link(done)` only needs to list in +Per [[todo/dependency_types]], `link(done)` only needs to list in `depends_simple` the pages that currently match. If a page is modified to add the link, the regular dependency calculation code notices that a new page matches. If a page that had the link is modified to remove it, -- cgit v1.2.3 From 09ff797682fd89380a4a71564ec02649af99851e Mon Sep 17 00:00:00 2001 From: Joey Hess Date: Wed, 21 Apr 2010 21:55:12 -0400 Subject: more wrongness --- doc/bugs/depends_simple_mixup.mdwn | 5 +++++ 1 file changed, 5 insertions(+) (limited to 'doc/bugs/depends_simple_mixup.mdwn') diff --git a/doc/bugs/depends_simple_mixup.mdwn b/doc/bugs/depends_simple_mixup.mdwn index 4fe69a90a..472de6349 100644 --- a/doc/bugs/depends_simple_mixup.mdwn +++ b/doc/bugs/depends_simple_mixup.mdwn @@ -57,3 +57,8 @@ For failed `match_link`, the failed influence would be the page. Then, when NOTting a `*Reason`, swap the two sets of influences. When ANDing/ORing, combine the individual sets. Querying the object for influences should return only the successful influences. + +In light of this, commit f2b3d1341447cbf29189ab490daae418fbe5d02d seems +thuroughly wrong. So, what about influence info for other matches +like `!author(foo)` etc? Currently, none is returned, but it should +be a content influence. What about backlink influence data? -- cgit v1.2.3 From 13325317a32529e02769baa5e61e6c401c675b27 Mon Sep 17 00:00:00 2001 From: Joey Hess Date: Wed, 21 Apr 2010 22:04:03 -0400 Subject: backlink influence data seems ok --- doc/bugs/depends_simple_mixup.mdwn | 2 +- t/pagespec_match_list.t | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) (limited to 'doc/bugs/depends_simple_mixup.mdwn') diff --git a/doc/bugs/depends_simple_mixup.mdwn b/doc/bugs/depends_simple_mixup.mdwn index 472de6349..e7b48f802 100644 --- a/doc/bugs/depends_simple_mixup.mdwn +++ b/doc/bugs/depends_simple_mixup.mdwn @@ -61,4 +61,4 @@ influences should return only the successful influences. In light of this, commit f2b3d1341447cbf29189ab490daae418fbe5d02d seems thuroughly wrong. So, what about influence info for other matches like `!author(foo)` etc? Currently, none is returned, but it should -be a content influence. What about backlink influence data? +be a content influence. (Backlink influence data is ok.) diff --git a/t/pagespec_match_list.t b/t/pagespec_match_list.t index 05dc012fe..ee5d60f88 100755 --- a/t/pagespec_match_list.t +++ b/t/pagespec_match_list.t @@ -1,7 +1,7 @@ #!/usr/bin/perl use warnings; use strict; -use Test::More tests => 94; +use Test::More tests => 107; BEGIN { use_ok("IkiWiki"); } @@ -80,7 +80,7 @@ foreach my $spec ("* and link(bar)", "* or link(bar)") { } # a pagespec with backlinks() will add as an influence the page with the links -foreach my $spec ("bar or (backlink(foo) and !*.png)", "backlink(foo)") { +foreach my $spec ("bar or (backlink(foo) and !*.png)", "backlink(foo)", "!backlink(foo)") { pagespec_match_list("foo2", $spec, deptype => deptype("presence")); ok($IkiWiki::depends{foo2}{$spec} & $IkiWiki::DEPEND_PRESENCE); ok(! ($IkiWiki::depends{foo2}{$spec} & ($IkiWiki::DEPEND_CONTENT | $IkiWiki::DEPEND_LINKS))); -- cgit v1.2.3 From c98414e192285b2607ee9fcb27f0e8e00db5fb26 Mon Sep 17 00:00:00 2001 From: Joey Hess Date: Wed, 21 Apr 2010 23:08:54 -0400 Subject: added a test case for this bug Further analysis suggests fixing this might not be as dreadful as I first thought! --- doc/bugs/depends_simple_mixup.mdwn | 24 ++++++++++++++++++++++-- t/pagespec_match_list.t | 20 +++++++++++++++++++- 2 files changed, 41 insertions(+), 3 deletions(-) (limited to 'doc/bugs/depends_simple_mixup.mdwn') diff --git a/doc/bugs/depends_simple_mixup.mdwn b/doc/bugs/depends_simple_mixup.mdwn index e7b48f802..79bfa8bdc 100644 --- a/doc/bugs/depends_simple_mixup.mdwn +++ b/doc/bugs/depends_simple_mixup.mdwn @@ -44,9 +44,11 @@ is modified to add the link, the regular dependency calculation code didn't notice, since the pagespec no longer matched. In this case, `depends_simple` needs to contain all pages -that do *not* match `link_done)`, but before my change, it contained +that do *not* match `link(done)`, but before my change, it contained all pages that *do* match. After my change, it contained all pages. +---- + So, seems what is needed is a way for influence info to be manipulated by the boolean operations that are applied. One way would be to have two sets of influences be returned, one for successful matches, and one for @@ -58,7 +60,25 @@ Then, when NOTting a `*Reason`, swap the two sets of influences. When ANDing/ORing, combine the individual sets. Querying the object for influences should return only the successful influences. -In light of this, commit f2b3d1341447cbf29189ab490daae418fbe5d02d seems +---- + +Would it be possible to avoid the complication of maintianing two sets of +influence info? + +Well, notice that the influence of `pagespec_match($page, "link(done)")` +is $page. Iff the match succeeds. + +Also, the influence of `pagespec_match($page, "!link(done)")` is +$page. Iff the (overall) match succeeds. + +Does that hold for all cases? If so, the code that populates +`depends_simple` could just test if the pagespec was successful, and +if not, avoid adding $page influences, while still adding any other, +non-$page influences. + +---- + +Hmm, commit f2b3d1341447cbf29189ab490daae418fbe5d02d seems thuroughly wrong. So, what about influence info for other matches like `!author(foo)` etc? Currently, none is returned, but it should be a content influence. (Backlink influence data is ok.) diff --git a/t/pagespec_match_list.t b/t/pagespec_match_list.t index ee5d60f88..27546e6ca 100755 --- a/t/pagespec_match_list.t +++ b/t/pagespec_match_list.t @@ -1,7 +1,7 @@ #!/usr/bin/perl use warnings; use strict; -use Test::More tests => 107; +use Test::More tests => 115; BEGIN { use_ok("IkiWiki"); } @@ -27,6 +27,8 @@ IkiWiki::checkconfig(); $IkiWiki::pagectime{foo} = 2; $IkiWiki::pagectime{foo2} = 2; $IkiWiki::pagectime{foo3} = 1; +$IkiWiki::pagectime{foo4} = 1; +$IkiWiki::pagectime{foo5} = 1; $IkiWiki::pagectime{bar} = 3; $IkiWiki::pagectime{"post/1"} = 6; $IkiWiki::pagectime{"post/2"} = 6; @@ -69,12 +71,28 @@ foreach my $spec ("* and link(bar)", "* or link(bar)") { ok($IkiWiki::depends{foo2}{$spec} & $IkiWiki::DEPEND_PRESENCE); ok(! ($IkiWiki::depends{foo2}{$spec} & ($IkiWiki::DEPEND_CONTENT | $IkiWiki::DEPEND_LINKS))); ok($IkiWiki::depends_simple{foo2}{foo2} == $IkiWiki::DEPEND_LINKS); + ok($IkiWiki::depends_simple{foo2}{foo} != $IkiWiki::DEPEND_LINKS); %IkiWiki::depends_simple=(); %IkiWiki::depends=(); pagespec_match_list("foo3", $spec, deptype => deptype("links")); ok($IkiWiki::depends{foo3}{$spec} & $IkiWiki::DEPEND_LINKS); ok(! ($IkiWiki::depends{foo3}{$spec} & ($IkiWiki::DEPEND_CONTENT | $IkiWiki::DEPEND_PRESENCE))); ok($IkiWiki::depends_simple{foo3}{foo3} == $IkiWiki::DEPEND_LINKS); + ok($IkiWiki::depends_simple{foo3}{foo} != $IkiWiki::DEPEND_LINKS); + %IkiWiki::depends_simple=(); + %IkiWiki::depends=(); +} +# Above we tested that a link pagespec is influenced +# by the pages that currently contain the link. + +# Oppositely, a pagespec that tests for pages that do not have a link +# is not influenced by pages that currently contain the link, but +# is instead influenced by pages that currently do not (but that +# could be changed to have it). +foreach my $spec ("* and !link(bar)", "* and !(!(!link(bar)))") { + pagespec_match_list("foo2", $spec); + ok($IkiWiki::depends_simple{foo2}{foo2} != $IkiWiki::DEPEND_LINKS); + ok($IkiWiki::depends_simple{foo2}{foo} == $IkiWiki::DEPEND_LINKS); %IkiWiki::depends_simple=(); %IkiWiki::depends=(); } -- cgit v1.2.3 From 2b175d7c1fe997277800c0d501332e96de475c6d Mon Sep 17 00:00:00 2001 From: Joey Hess Date: Thu, 22 Apr 2010 00:12:15 -0400 Subject: improved fix for depends_simple_mixup Avoid adding the page matched against as an influence for currently failing pagespec matches, while still adding any other influences. This avoids bloating depends_simple with lots of bogus influences when matching eg, "!link(done)". It's only necessary for the page being tested to be an influence of that if the page matches. --- IkiWiki.pm | 14 +++++++++++++- doc/bugs/depends_simple_mixup.mdwn | 6 +++++- 2 files changed, 18 insertions(+), 2 deletions(-) (limited to 'doc/bugs/depends_simple_mixup.mdwn') diff --git a/IkiWiki.pm b/IkiWiki.pm index 0791e1e75..509f9ba2e 100644 --- a/IkiWiki.pm +++ b/IkiWiki.pm @@ -1818,10 +1818,12 @@ sub add_depends ($$;$) { foreach my $p (keys %pagesources) { my $r=$sub->($p, location => $page); my $i=$r->influences; + my $static=$r->influences_static; foreach my $k (keys %$i) { + next unless $r || $static || $k eq $page; $depends_simple{$page}{lc $k} |= $i->{$k}; } - last if $r->influences_static; + last if $static; } $depends{$page}{$pagespec} |= $deptype; @@ -2136,6 +2138,9 @@ sub pagespec_match_list ($$;@) { my $r=$sub->($p, %params, location => $page); error(sprintf(gettext("cannot match pages: %s"), $r)) if $r->isa("IkiWiki::ErrorReason"); + unless ($r) { + $r->remove_influence($p); + } $accum |= $r; if ($r) { push @matches, $p; @@ -2232,6 +2237,13 @@ sub merge_influences { } } +sub remove_influence { + my $this=shift; + my $torm=shift; + + delete $this->[1]{$torm}; +} + package IkiWiki::ErrorReason; our @ISA = 'IkiWiki::FailReason'; diff --git a/doc/bugs/depends_simple_mixup.mdwn b/doc/bugs/depends_simple_mixup.mdwn index 79bfa8bdc..a5910d02e 100644 --- a/doc/bugs/depends_simple_mixup.mdwn +++ b/doc/bugs/depends_simple_mixup.mdwn @@ -81,4 +81,8 @@ non-$page influences. Hmm, commit f2b3d1341447cbf29189ab490daae418fbe5d02d seems thuroughly wrong. So, what about influence info for other matches like `!author(foo)` etc? Currently, none is returned, but it should -be a content influence. (Backlink influence data is ok.) +be a content influence. (Backlink influence data seems ok.) + +---- + +[[done]] again! -- cgit v1.2.3