X-Git-Url: http://git.vanrenterghem.biz/git.ikiwiki.info.git/blobdiff_plain/6d22987b4e82a978bb37229db210fc9ba14f44bd..406a30ad0dd776e108c4088431a5cf8ae597c422:/doc/todo/tracking_bugs_with_dependencies.mdwn diff --git a/doc/todo/tracking_bugs_with_dependencies.mdwn b/doc/todo/tracking_bugs_with_dependencies.mdwn index 8435e9f58..464f68363 100644 --- a/doc/todo/tracking_bugs_with_dependencies.mdwn +++ b/doc/todo/tracking_bugs_with_dependencies.mdwn @@ -11,6 +11,8 @@ I like the idea of [[tips/integrated_issue_tracking_with_ikiwiki]], and I do so > >> I thought about this briefly, and got about that far.. glad you got >> further. :-) --[[Joey]] + +>> Or, one [[!taglink could_also_refer|pagespec_in_DL_style]] to the language of [[!wikipedia description logics]]: their formulas actually define classes of objects through quantified relations to other classes. --Ivan Z. > > Another option would be go with a more functional syntax. The concept here would > be to allow a pagespec to appear in a 'pagespec function' anywhere a page can. e.g. @@ -58,6 +60,7 @@ I like the idea of [[tips/integrated_issue_tracking_with_ikiwiki]], and I do so >> So, equivilant example: `define(bugs, bugs/* and !*/Discussion) and define(openbugs, bugs and !link(done)) and openbugs and !link(openbugs)` >> + >> Re recursion, it is avoided.. but building a pagespec that is O(N^X) where N is the >> number of pages in the wiki is not avoided. Probably need to add DOS prevention. >> --[[Joey]] @@ -67,13 +70,15 @@ I like the idea of [[tips/integrated_issue_tracking_with_ikiwiki]], and I do so >>>> Yeah, guess that'd work. :-) -> One quick further thought. All the above discussion assumes that 'dependency' is the +> One quick further thought. All the above discussion assumes that 'dependency' is the > same as 'links to', which is not really true. For example, you'd like to be able to say > "This bug does not depend upon [ [ link to other bug ] ]" and not have a dependency. > Without having different types of links, I don't see how this would be possible. > > -- [[Will]] +>> I saw that this issue is targeted at by the work on [[structured page data#another_kind_of_links]]. --Ivan Z. + Okie - I've had a quick attempt at this. Initial patch attached. This one doesn't quite work. And there is still a lot of debugging stuff in there. @@ -188,15 +193,83 @@ The following three inlines work for me with this patch: I've lost track of the indent level, so I'm going back to not indented - I think this is a working [[patch]] taking into account all comments above (which doesn't mean it is above reproach :) ). --[[Will]] +> Very belated code review of last version of the patch: +> +> * `is_globlist` is no longer needed + +>> Good :) + +> * I don't understand why the pagespec match regexp is changed +> from having flags `igx` to `ixgs`. Don't see why you +> want `.` to match '\n` in it, and don't see any `.` in the regexp +> anyway? + +>> Because you have to define all the named pagespecs in the pagespec, you sometimes end up with very long pagespecs. I found it useful to split them over multiple lines. That didn't work at one point and I added the 's' to make it work. I may have further altered the regex since then to make the 's' redundant. Remove it and see if multi-line pagespecs still work. :) + +> * Some changes of `@_` to `%params` in `pagespec_makeperl` do not +> make sense to me. I don't see where \%params is defined and populated, +> except with `\$params{specFunc}`. + +>> I'm not a perl hacker. This was a mighty battle for me to get going. There is probably some battlefield carnage from my early struggles learning perl left here. +>> Part of this is that @_ / @params already existed as a way of passing in extra parameters. I didn't want to pollute that top level namespace - just at my own parameter (a hash) which contained the data I needed. + +> * Seems that the only reason `match_glob` has to check for `~` is +> because when a named spec appears in a pagespec, it is translated +> to `match_glob("~foo")`. If, instead, `pagespec_makeperl` checked +> for named specs, it could convert them into `check_named_spec("foo")` +> and avoid that ugliness. + +>> Yeah - I wanted to make named specs syntactically different on my first pass. You are right in that this could be made a fallback - named specs always override pagenames. + +> * The changes to `match_link` seem either unecessary, or incomplete. +> Shouldn't it check for named specs and call +> `check_named_spec_existential`? + +>> An earlier version did. Then I realised it wasn't actually needed in that case - match_link() already included a loop that was like a type of existential matching. Each time through the loop it would +>> call match_glob(). match_glob() in turn will handle the named spec. I tested this version briefly and it seemed to work. I remember looking at this again later and wondering if I had mis-understood +>> some of the logic in match_link(), which might mean there are cases where you would need an explicit call to check_named_spec_existential() - I never checked it properly after having that thought. + +> * Generally, the need to modify `match_*` functions so that they +> check for and handle named pagespecs seems suboptimal, if +> only because there might be others people may want to use named +> pagespecs with. It would be possible to move this check +> to `pagespec_makeperl`, by having it check if the parameter +> passed to a pagespec function looked like a named pagespec. +> The only issue is that some pagespec functions take a parameter +> that is not a page name at all, and it could be weird +> if such a parameter were accidentially interpreted as a named +> pagespec. (But, that seems unlikely to happen.) + +>> Possibly. I'm not sure which I prefer between the current solution and that one. Each have advantages and disadvantages. +>> It really isn't much code for the match functions to add a call to check_named_spec_existential(). + +> * I need to check if your trick to avoid infinite recursion +> works if there are two named specs that recursively +> call one-another. I suspect it does, but will test this +> myself.. + +>> It worked for me. :) + +> --[[Joey]] + +>> There is one issue that I've been thinking about that I haven't raised anywhere (or checked myself), and that is how this all interacts with page dependencies. +>> Firstly, I'm not sure anymore that the `pagespec_merge` function will continue to work in all cases. Secondly, it seems that there are two types of dependency, and ikiwiki +>> currently only handles one of them. The first type is "Rebuild this page when any of these other pages changes" - ikiwiki handles this. The second type is "rebuild this page when +>> set of pages referred to by this pagespec changes" - ikiwiki doesn't seem to handle this. I suspect that named pagespecs would make that second type of dependency more +>> important. I'll try to come up with a good example. -- [[Will]] + +>>> Hrm, I was going to build an example of this with backlinks, but it looks like that is handled as a special case at the moment (line 458 of render.pm). I'll see if I can break +>>> things another way. Fixing this properly would allow removal of that special case. -- [[Will]] + ---- diff --git a/IkiWiki.pm b/IkiWiki.pm - index 4e4da11..2cc5f09 100644 + index 4e4da11..8b3cdfe 100644 --- a/IkiWiki.pm +++ b/IkiWiki.pm - @@ -1550,7 +1550,16 @@ sub globlist_to_pagespec ($) { #{{{ + @@ -1550,7 +1550,16 @@ sub globlist_to_pagespec ($) { - sub is_globlist ($) { #{{{ + sub is_globlist ($) { my $s=shift; - return ( $s =~ /[^\s]+\s+([^\s]+)/ && $1 ne "and" && $1 ne "or" ); + return ! ($s =~ / @@ -208,20 +281,20 @@ account all comments above (which doesn't mean it is above reproach :) ). --[[W + \))?\s*$ + ) | + (\s and \s) | (\s or \s) # or we find 'and' or 'or' somewhere - + /x); - } #}}} + + /xs); + } - sub safequote ($) { #{{{ - @@ -1631,7 +1640,7 @@ sub pagespec_merge ($$) { #{{{ + sub safequote ($) { + @@ -1631,7 +1640,7 @@ sub pagespec_merge ($$) { return "($a) or ($b)"; - } #}}} + } - -sub pagespec_translate ($) { #{{{ - +sub pagespec_makeperl ($) { #{{{ + -sub pagespec_translate ($) { + +sub pagespec_makeperl ($) { my $spec=shift; # Support for old-style GlobLists. - @@ -1650,7 +1659,9 @@ sub pagespec_translate ($) { #{{{ + @@ -1650,12 +1659,14 @@ sub pagespec_translate ($) { | \) # ) | @@ -232,18 +305,25 @@ account all comments above (which doesn't mean it is above reproach :) ). --[[W | [^\s()]+ # any other text ) - @@ -1666,16 +1677,23 @@ sub pagespec_translate ($) { #{{{ + \s* # ignore whitespace + - }igx) { + + }igxs) { + my $word=$1; + if (lc $word eq 'and') { + $code.=' &&'; + @@ -1666,16 +1677,23 @@ sub pagespec_translate ($) { elsif ($word eq "(" || $word eq ")" || $word eq "!") { $code.=' '.$word; } - + elsif ($word =~ /^define\(\s*~(\w+)\s*,(.*)\)$/) { + - elsif ($word =~ /^(\w+)\((.*)\)$/) { + + elsif ($word =~ /^define\(\s*~(\w+)\s*,(.*)\)$/s) { + $code .= " (\$params{specFuncs}->{$1}="; # (exists \$params{specFuncs}) && + $code .= "memoize("; + $code .= &pagespec_makeperl($2); + $code .= ")"; + $code .= ") "; + } - elsif ($word =~ /^(\w+)\((.*)\)$/) { + + elsif ($word =~ /^(\w+)\((.*)\)$/s) { if (exists $IkiWiki::PageSpec::{"match_$1"}) { - $code.="IkiWiki::PageSpec::match_$1(\$page, ".safequote($2).", \@_)"; + $code.="IkiWiki::PageSpec::match_$1(\$page, ".safequote($2).", \%params)"; @@ -258,14 +338,14 @@ account all comments above (which doesn't mean it is above reproach :) ). --[[W } } - @@ -1683,23 +1701,36 @@ sub pagespec_translate ($) { #{{{ + @@ -1683,8 +1701,18 @@ sub pagespec_translate ($) { $code=0; } + return 'sub { my $page=shift; my %params = @_; '.$code.' }'; - +} #}}} + +} + - +sub pagespec_translate ($) { #{{{ + +sub pagespec_translate ($) { + my $spec=shift; + + my $code = pagespec_makeperl($spec); @@ -275,30 +355,19 @@ account all comments above (which doesn't mean it is above reproach :) ). --[[W no warnings; - return eval 'sub { my $page=shift; '.$code.' }'; + return eval $code; - } #}}} + } - sub pagespec_match ($$;@) { #{{{ - my $page=shift; - my $spec=shift; - my @params=@_; - + my %params=@_; - - # Backwards compatability with old calling convention. - if (@params == 1) { - - unshift @params, 'location'; - + %params = { location => $params[1] }; - } + sub pagespec_match ($$;@) { + @@ -1699,7 +1727,7 @@ sub pagespec_match ($$;@) { - + $params{specFuncs} = {} unless defined $params{specFuncs}; - + my $sub=pagespec_translate($spec); return IkiWiki::FailReason->new("syntax error in pagespec \"$spec\"") if $@; - return $sub->($page, @params); - + return $sub->($page, %params); - } #}}} + + return $sub->($page, @params, specFuncs => {}); + } - sub pagespec_valid ($) { #{{{ - @@ -1748,11 +1779,78 @@ sub new { #{{{ + sub pagespec_valid ($) { + @@ -1748,11 +1776,78 @@ sub new { package IkiWiki::PageSpec; @@ -365,7 +434,7 @@ account all comments above (which doesn't mean it is above reproach :) ). --[[W + } +} + - sub match_glob ($$;@) { #{{{ + sub match_glob ($$;@) { my $page=shift; my $glob=shift; my %params=@_; @@ -377,29 +446,33 @@ account all comments above (which doesn't mean it is above reproach :) ). --[[W my $from=exists $params{location} ? $params{location} : ''; # relative matching - @@ -1782,11 +1880,16 @@ sub match_internal ($$;@) { #{{{ + @@ -1782,11 +1877,12 @@ sub match_internal ($$;@) { - sub match_link ($$;@) { #{{{ + sub match_link ($$;@) { my $page=shift; - my $link=lc(shift); + my $fulllink=shift; my %params=@_; + my $link=lc($fulllink); - - my $from=exists $params{location} ? $params{location} : ''; - + if (substr($fulllink, 0, 1) eq '~') { - + return check_named_spec_existential($page, $fulllink, \&match_link, %params); - + } - - + my $from=exists $params{location} ? $params{location} : ''; + my $from=exists $params{location} ? $params{location} : ''; + - + # relative matching if ($link =~ m!^\.! && defined $from) { $from=~s#/?[^/]+$##; - @@ -1811,12 +1914,25 @@ sub match_link ($$;@) { #{{{ - } #}}} + @@ -1804,19 +1900,32 @@ sub match_link ($$;@) { + } + else { + return IkiWiki::SuccessReason->new("$page links to page $p matching $link") + - if match_glob($p, $link, %params); + + if match_glob($p, $fulllink, %params); + } + } + return IkiWiki::FailReason->new("$page does not link to $link"); + } - sub match_backlink ($$;@) { #{{{ + sub match_backlink ($$;@) { - return match_link($_[1], $_[0], @_); + my $page=shift; + my $backlink=shift; @@ -410,9 +483,9 @@ account all comments above (which doesn't mean it is above reproach :) ). --[[W + } + + return match_link($backlink, $page, @params); - } #}}} + } - sub match_created_before ($$;@) { #{{{ + sub match_created_before ($$;@) { my $page=shift; my $testpage=shift; + my @params=@_; @@ -423,8 +496,8 @@ account all comments above (which doesn't mean it is above reproach :) ). --[[W if (exists $IkiWiki::pagectime{$testpage}) { if ($IkiWiki::pagectime{$page} < $IkiWiki::pagectime{$testpage}) { - @@ -1834,6 +1950,11 @@ sub match_created_before ($$;@) { #{{{ - sub match_created_after ($$;@) { #{{{ + @@ -1834,6 +1943,11 @@ sub match_created_before ($$;@) { + sub match_created_after ($$;@) { my $page=shift; my $testpage=shift; + my @params=@_;