summaryrefslogtreecommitdiff
path: root/doc/plugins/contrib/cvs/discussion.mdwn
blob: e1fa6e428cbe6ec876e300333eea388adcb3cfdd (plain)

I've started reviewing this, and the main thing I don't like is the post-commit wrapper wrapper that ikiwiki-makerepo is patched to set up. That just seems unnecessarily complicated. Why can't ikiwiki itself detect the "cvs add " call and avoid doing anything in that case? --[[Joey]]

The wrapper wrapper does three things:

  1. It ignores cvs add <directory>, since this is a weird CVS behavior that ikiwiki gets confused by and doesn't need to act on.
  2. It prevents cvs locking against itself: cvs commit takes a write lock and runs the post-commit hook, which runs cvs update, which wants a read lock and sleeps forever -- unless the post-commit hook runs in the background so the commit can "finish".
  3. It fails silently if the ikiwiki post-commit hook is missing. CVS doesn't have any magic post-commit filenames, so hooks have to be configured explicitly. I don't think a commit will actually fail if a configured post-commit hook is missing (though I can't test this at the moment).

Thing 1 can probably be handled within ikiwiki, if that seems less gross to you.

It seems like it might be. You can use a getopt hook to check @ARGV to see how it was called. --[[Joey]]

This does the trick iff the post-commit wrapper passes its args along. Committed on my branch. This seems potentially dangerous, since the args passed to ikiwiki are influenced by web commits. I don't see an exploit, but for paranoia's sake, maybe the wrapper should only be built with execv() if the cvs plugin is loaded? --[[schmonz]]

Hadn't considered that. While in wrapper mode the normal getopt is not done, plugin getopt still runs, and so any unsafe options that other plugins support could be a problem if another user runs the setuid wrapper and passes those options through. --[[Joey]]

I've tried compiling the argument check into the wrapper as the first thing main() does, and was surprised to find that this doesn't prevent the cvs add <dir> deadlock in a web commit. I was convinced this'd be a reasonable solution, especially if conditionalized on the cvs plugin being loaded, but it doesn't work. And I stuck debug printfs at the beginning of all the rcs_foo() subs, and whatever cvs add <dir> is doing to ikiwiki isn't visible to my plugin, because none of those subs are getting called. Nuts. Can you think of anything else that might solve the problem, or should I go back to generating a minimal wrapper wrapper that checks for just this one thing? --[[schmonz]]

I don't see how there could possibly be a difference between ikiwiki's C wrapper and your shell wrapper wrapper here. --[[Joey]]

Thing 2 I'm less sure of. (I'd like to see the web UI return immediately on save anyway, to a temporary "rebuilding, please wait if you feel like knowing when it's done" page, but this problem with CVS happens with any kind of commit, and could conceivably happen with some other VCS.)

None of the other VCSes let a write lock block a read lock, apparently.

Anyway, re the backgrounding, when committing via the web, the post-commit hook doesn't run anyway; the rendering is done via the ikiwiki CGI. It would certianly be nice if it popped up a quick "working" page and replaced it with the updated page when done, but that's unrelated; the post-commit hook only does rendering when committing using the VCS directly. The backgrounding you do actually seems safe enough -- but tacking on a " &" to the ikiwiki wrapper call doesn't need a wrapper script, does it? --[[Joey]]

Nope, it works fine to append it to the CVSROOT/loginfo line. Fixed on my branch. --[[schmonz]]

Thing 3 I think I did in order to squelch the error messages that were bollixing up the CGI. It was easy to do this in the wrapper wrapper, but if that's going away, it can be done just as easily with output redirection in CVSROOT/loginfo.

--[[schmonz]]

If the error messages screw up the CGI they must go to stdout. I thought we had stderr even in the the CVS dark ages. ;-) --[[Joey]]

Some messages go to stderr, but definitely not all. That's why I wound up reaching for IPC::Cmd, to execute the command line safely while shutting CVS up. Anyway, I've tested what happens if a configured post-commit hook is missing, and it seems fine, probably also thanks to IPC::Cmd. --[[schmonz]]