X-Git-Url: https://sipb.mit.edu/gitweb.cgi/ikiwiki.git/blobdiff_plain/05b99e3cfa8c9044d18d5b07aec24fff555f2889..3b8f4f59d6720e5a77cae30eacc4c46582b1603b:/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 95710ced0..ed681ac4d 100644 --- a/doc/todo/auto-create_tag_pages_according_to_a_template.mdwn +++ b/doc/todo/auto-create_tag_pages_according_to_a_template.mdwn @@ -4,7 +4,7 @@ Tags are mainly specific to the object to which they’re stuck. However, I ofte Also see: and -[[!tag wishlist plugins/tag patch]] +[[!tag wishlist plugins/tag patch patch/core]] I would love to see this as well. -- dato @@ -126,7 +126,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`: @@ -160,3 +160,126 @@ wrong direction. * Proper documentation. --[[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]] + +>>> * `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]] + +> 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]] + +[[!template id=gitbranch branch=origin/autotag author="[[Joey]]"]] +I've pushed an autotag branch of my own, which refactors +things a bit. It is untested so far though. --[[Joey]] + +* `verify_src_file` only called from Render.pm +* Gets rid of `%del_files`. +* Uses `%wikistate`. + +[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)