]> sipb.mit.edu Git - ikiwiki.git/blobdiff - doc/todo/auto-create_tag_pages_according_to_a_template.mdwn
sipb-www / ikiwiki.git / blobdiff
? search:
summary | shortlog | log | commit | commitdiff | tree
raw | inline | side by side
my autotag branch seems ready
[ikiwiki.git] / doc / todo / auto-create_tag_pages_according_to_a_template.mdwn
diff --git a/doc/todo/auto-create_tag_pages_according_to_a_template.mdwn b/doc/todo/auto-create_tag_pages_according_to_a_template.mdwn
index 8c586d7065c24c21d3b7b01a31606d265807dabd..b05e1db3d1cf6ea7d66d5ea52c14667cb3146b6e 100644 (file)
--- a/doc/todo/auto-create_tag_pages_according_to_a_template.mdwn
+++ b/doc/todo/auto-create_tag_pages_according_to_a_template.mdwn
@@ -15,86 +15,7 @@ A new setting is used to enable or disable auto-create tag pages, `tag_autocreat
 The new tag file is created during the preprocess phase. 
 The new tag file is then complied during the change phase.
 
 The new tag file is created during the preprocess phase. 
 The new tag file is then complied during the change phase.
 
-_tag.pm from version 3.01_
-
-
-       --- tag.pm      2009-02-06 10:26:03.000000000 -0700
-       +++ tag_new.pm  2009-02-06 12:17:19.000000000 -0700
-       @@ -14,6 +14,7 @@
-                       hook(type => "preprocess", id => "tag", call => \&preprocess_tag, scan => 1);
-                       hook(type => "preprocess", id => "taglink", call => \&preprocess_taglink, scan => 1);
-                       hook(type => "pagetemplate", id => "tag", call => \&pagetemplate);
-       +       hook(type => "change", id => "tag", call => \&change);
-        }
-        
-        sub getopt () {
-       @@ -36,6 +37,36 @@
-                                                       safe => 1,
-                                                       rebuild => 1,
-                                       },
-       +               tag_autocreate => {
-       +                       type => "boolean",
-       +                       example => 0,
-       +                       description => "Auto-create the new tag pages, uses autotagpage.tmpl ",
-       +                       safe => 1,
-       +                       rebulid => 1,
-       +               },
-       +}
-       +
-       +my $autocreated_page = 0;
-       +
-       +sub gen_tag_page($)    {
-       +       my $tag=shift;
-       +
-       +       my $tag_file=$tag.'.'.$config{default_pageext};
-       +       return if (-f $config{srcdir}.$tag_file);
-       +
-       +       my $template=template("autotagpage.tmpl");
-       +       $template->param(tag => $tag);
-       +       writefile($tag_file, $config{srcdir}, $template->output);
-       +       $autocreated_page = 1;
-       +
-       +       if ($config{rcs}) {
-       +               IkiWiki::disable_commit_hook();
-       +               IkiWiki::rcs_add($tag_file);
-       +               IkiWiki::rcs_commit_staged(
-       +                       gettext("Automatic tag page generation"),
-       +                       undef, undef);
-       +               IkiWiki::enable_commit_hook();
-       +       }
-        }
-        
-        sub tagpage ($) {
-       @@ -47,6 +78,10 @@
-                                       $tag=~y#/#/#s; # squash dups
-                       }
-        
-       +       if (defined $config{tag_autocreate} && $config{tag_autocreate} ) {
-       +               gen_tag_page($tag);
-       +       }
-       +
-                       return $tag;
-        }
-        
-       @@ -125,4 +160,18 @@
-                       }
-        }
-        
-       +sub change(@) {
-       +       return unless($autocreated_page);
-       +       $autocreated_page = 0;
-       +
-       +       # This refresh/saveindex is to complie the autocreated tag pages
-       +       IkiWiki::refresh();
-       +       IkiWiki::saveindex();
-       +
-       +       # This refresh/saveindex is to fix the Tags link
-       +       # With out this additional refresh/saveindex the tag link displays ?tag
-       +       IkiWiki::refresh();
-       +       IkiWiki::saveindex();
-       +}
-       +
-
+*see git history of this page if you want the patch --[[smcv]]*
 
 This uses a [[template|wikitemplates]] called `autotagpage.tmpl`, here is my template file:
 
 
 This uses a [[template|wikitemplates]] called `autotagpage.tmpl`, here is my template file:
 
@@ -126,7 +47,7 @@ is not satisfied for the newly created tag page. I shall put debug msgs into Ren
 
 ---
 
 
 ---
 
-I've made another attempt at fixiing this
+I've made another attempt at fixing this
 
 The current progress can be found at my [git repository][gitweb] on branch
 `autotag`:
 
 The current progress can be found at my [git repository][gitweb] on branch
 `autotag`:
@@ -152,11 +73,177 @@ consider this a feature, not a bug)
 Todo/Bugs:
 
 * Will still create a page even if there's a page other than `$tag` under
 Todo/Bugs:
 
 * Will still create a page even if there's a page other than `$tag` under
-`tagbase` satisfying the tag link.
+`tagbase` satisfying the tag link. (details? --[[Joey]])
 * Call from `IkiWiki.pm` to `Render.pm`, which adds a module dependency in the
 * Call from `IkiWiki.pm` to `Render.pm`, which adds a module dependency in the
-wrong direction.
+wrong direction. (fixed --[[Joey]] )
 * Add files to RCS.
 * Unit tests.
 * Add files to RCS.
 * Unit tests.
-* Proper documentation.
+* Proper documentation. (fixed (mostly) --[[Joey]])
 
 --[[David_Riebenbauer]]
 
 --[[David_Riebenbauer]]
+
+> Starting review of this. Some of your commits are to very delicate,
+> optimised, and security-sensitive ground, so I have to look at them very
+> carefully. --[[Joey]]
+
+>> First of, sorry that it took me so damn long to answer. I didn't lose
+>> interest but it took a while for me to find the time and motivation
+>> to address you suggestions. --[[David_Riebenbauer]]
+
+> * In the refactoring in [f3abeac919c4736429bd3362af6edf51ede8e7fe][],
+>   you introduced at least 2 bugs, one a possible security hole.
+>   Now one part of the code tests `if ($file)` and the other
+>   caller tests `if ($f)`. These two tests both tested `if (! defined $f)`
+>   before. Notice that the variable needs to be the untainted variable
+>   for both. Also notice that `if ($f)` fails if `$f` contains `0`,
+>   which is a very common perl gotcha.
+> * Your refactored code changes `-l $_ || -d _` to `-l $file || -d $file`.
+>   The latter makes one more stat system call; note the use of a
+>   bare `_` in the first to make perl reuse the stat buffer.
+> * (As a matter of style, could you put a space after the commas in your
+>   perl?)
+
+>> The first two points should be addressed in
+>> [da5d29f95f6e693e8c14be1b896cf25cf4fdb3c0][]. And sure, I can add the
+>> spaces. --[[David_Riebenbauer]]
+
+> I'd like to cherry-pick the above commit, once it's in shape, before
+> looking at the rest in detail. So just a few other things that stood out.
+> 
+> * Commit [4af4d26582f0c2b915d7102fb4a604b176385748][] seems unnecessary.
+>   `srcfile($file, 1)` already is documented to return undef if the
+>   file does not exist. (But without the second parameter, it throws
+>   an error.)
+
+>> You're right. I must have been some confused by some other promplem I
+>> introduced then. Reverted. --[[David_Riebenbauer]]
+
+> * Commit [f58f3e1bec41ccf9316f37b014ce0b373c8e49e1][] adds a line
+>   that is intented by a space, not a tab.
+
+>> Sorry, That one was reverted anyway. --[[David_Riebenbauer]]
+
+> * Commit [f58f3e1bec41ccf9316f37b014ce0b373c8e49e1][] says that auto-added
+>   files will be recreated if the user deletes them. That seems bad.
+>   `autoindex` goes to some trouble to not recreate deleted files.
+
+>> I reverted the commit and addressed the issue in
+>> [a358d74bef51dae31332ff27e897fe04834571e6][] and
+>> [981400177d68a279f485727be3f013e68f0bf691][].
+ --[[David_Riebenbauer]]
+
+>>> This doesn't seem to have all the refinements that autoindex has:
+>>>
+>>> * `autoindex` attaches the record of deletions to the `index` page, which
+>>>   is (nearly) guaranteed to exist; this one attaches the record of
+>>>   deletions to the deleted page's page state. Won't that tend to result
+>>>   in losing the record along with the deleted page?
+
+>>>> This is probably on of the harder things to do, 'cause there are (most of the
+>>>> time) several pages that are responsible for the creation of a single tag page.
+>>>> Of course I could attach the info to all of them.
+
+>>>> With current behaviour I think the information in `%pagestate` is kept around
+>>>> regardless whether the corresponding page exists or not.
+>>>> --[[David_Riebenbauer]]
+
+>>>>> Sorry, I'll try to be clearer: `autoindex` hard-codes that the [[index]] page
+>>>>> of the entire wiki is the one responsible for storing the page state. That
+>>>>> page isn't responsible for the creation of the tag page, it's just an
+>>>>> arbitrary page that's (more or less) guaranteed to exist. --[[smcv]]
+
+>>>>> I don't like that [[plugins/autoindex]] has to do that,
+>>>>> but `%pagestate` values are only stored for pages that exist,
+>>>>> so it was necessary. (Another way to look at this is that
+>>>>> `%pagestate` is not the ideal data structure.) --[[Joey]]
+
+>>>>>> Aha! Having looked at [[plugins/write]] again, it turns out that what this
+>>>>>> feature should really use is `%wikistate`, I think? :-) --[[smcv]]
+
+>>>>>>> Ah, indeed, that came after I wrote autoindex. I've fixed autoindex to
+>>>>>>> use it. --[[Joey]]
+
+>>>>> Ok, now I know what you mean. --[[David_Riebenbauer]]
+
+>>> * `autoindex` forgets that a page was deleted when that page is
+>>>   re-created
+
+>>>> Yes, I forgot about that and that is a bug. I'll fix that.
+>>>> --[[David_Riebenbauer]]
+
+>>>>> In my branch, it keeps a list of autofiles that were created,
+>>>>> not deleted. And I think that turns out to be necessary, really.
+>>>>> However, I see no way to clean out that list on deletion and
+>>>>> manual recreation -- it still needs to remember it was once an autofile,
+>>>>> in order to avoid recreating it if it's deleted yet again. --[[Joey]]
+
+>>>>>> Are these really the semantics we want? It seems strange to me
+>>>>>> that this:
+>>>>>>
+>>>>>> * tag a page as foo
+>>>>>> * tags/foo automatically appears
+>>>>>> * delete tags/foo
+>>>>>> * create tags/foo manually
+>>>>>> * delete tags/foo again
+>>>>>> * tags/foo isn't automatically created
+>>>>>>
+>>>>>> isn't the same as this:
+>>>>>>
+>>>>>> * create tags/foo
+>>>>>> * delete tags/foo
+>>>>>> * tag a page as foo
+>>>>>> * tags/foo automatically appears
+>>>>>>
+>>>>>> or even this:
+>>>>>>
+>>>>>> * create tags/foo
+>>>>>> * tag a page as foo
+>>>>>> * delete tags/foo
+>>>>>> * tags/foo automatically appears (?)
+>>>>>>
+>>>>>> --[[smcv]]
+
+>>>>>>> I agree that the last of these is not desired. It could be avoided
+>>>>>>> by extending the list of autofiles to include those that were not
+>>>>>>> created due to the file/page already existing.
+>>>>>>> 
+>>>>>>> Hmm, that would fix the previous scenario too. --[[Joey]] 
+
+>>> * `autoindex` forgets that a page was deleted when it's no longer needed
+>>>   anyway (this may be harder for `autotag`?)
+
+>>>> I don't think so. AFAIK ikiwiki can detect whether there are taglinks to a page
+>>>> anyway, so it should be quite easy. I'll try to implement that too.
+>>>> --[[David_Riebenbauer]]
+
+>>> It'd probably be an interesting test of the core change to port
+>>> `autoindex` to use it? (Adding the file to the RCS would be
+>>> necessary to get parity with `autoindex`.) --[[smcv]]
+
+>>>> Good suggestion. Adding the files to RCS is on my todo list anyway.
+>>>> --[[David_Riebenbauer]]
+
+>>>>> I think it may be better to allow the `add_autofile` caller
+>>>>> to specify if it is added to RCS. In my branch, it can do
+>>>>> so by just making the callback it registers call `rcs_add`; 
+>>>>> and I have tag do this. Other plugins might want autofiles
+>>>>> that do not get checked in, conceivably.
+>>>>> --[[Joey]] 
+
+> Regarding the call from `IkiWiki.pm` to `Render.pm`, wouldn't this be
+> quite easy to solve by moving `verify_src_file` to IkiWiki.pm? --[[smcv]]
+
+>> True. I'll do that. --[[David_Riebenbauer]]
+>> Fixed in my branch --[[Joey]]
+
+[[!template id=gitbranch branch=origin/autotag author="[[Joey]]"]]
+I've pushed an autotag branch of my own, which refactors
+things a bit and fixes bugs around deletion/recreation.
+I've tested it fairly thouroughly. --[[Joey]]
+
+[f3abeac919c4736429bd3362af6edf51ede8e7fe]: http://git.liegesta.at/?p=ikiwiki.git;a=commitdiff;h=f3abeac919c4736429bd3362af6edf51ede8e7fe (commitdiff for f3abeac919c4736429bd3362af6edf51ede8e7fe)
+[4af4d26582f0c2b915d7102fb4a604b176385748]: http://git.liegesta.at/?p=ikiwiki.git;a=commitdiff;h=4af4d26582f0c2b915d7102fb4a604b176385748 (commitdiff for 4af4d26582f0c2b915d7102fb4a604b176385748)
+[f58f3e1bec41ccf9316f37b014ce0b373c8e49e1]: http://git.liegesta.at/?p=ikiwiki.git;a=commitdiff;h=f58f3e1bec41ccf9316f37b014ce0b373c8e49e1 (commitdiff for f58f3e1bec41ccf9316f37b014ce0b373c8e49e1)
+[da5d29f95f6e693e8c14be1b896cf25cf4fdb3c0]: http://git.liegesta.at/?p=ikiwiki.git;a=commitdiff;h=da5d29f95f6e693e8c14be1b896cf25cf4fdb3c0 (commitdiff for da5d29f95f6e693e8c14be1b896cf25cf4fdb3c0)
+[a358d74bef51dae31332ff27e897fe04834571e6]: http://git.liegesta.at/?p=ikiwiki.git;a=commitdiff;h=a358d74bef51dae31332ff27e897fe04834571e6 (commitdiff for a358d74bef51dae31332ff27e897fe04834571e6)
+[981400177d68a279f485727be3f013e68f0bf691]: http://git.liegesta.at/?p=ikiwiki.git;a=commitdiff;h=981400177d68a279f485727be3f013e68f0bf691 (commitdiff for 981400177d68a279f485727be3f013e68f0bf691)
wiki compiler