doc/rcs/cvs/discussion.mdwn

   1 I've started reviewing this, and the main thing I don't like is the
   2 post-commit wrapper wrapper that ikiwiki-makerepo is patched to set up.
   3 That just seems unnecessarily complicated. Why can't ikiwiki itself detect
   4 the "cvs add <directory>" call and avoid doing anything in that case?
   5 --[[Joey]]
   6
   7 > The wrapper wrapper does three things:
   8 >
   9 > 7. It ignores `cvs add <directory>`, since this is a weird CVS
  10 > behavior that ikiwiki gets confused by and doesn't need to act on.
  11 > 7. It prevents `cvs` locking against itself: `cvs commit` takes a
  12 > write lock and runs the post-commit hook, which runs `cvs update`,
  13 > which wants a read lock and sleeps forever -- unless the post-commit
  14 > hook runs in the background so the commit can "finish".
  15 > 7. It fails silently if the ikiwiki post-commit hook is missing.
  16 > CVS doesn't have any magic post-commit filenames, so hooks have to
  17 > be configured explicitly. I don't think a commit will actually fail
  18 > if a configured post-commit hook is missing (though I can't test
  19 > this at the moment).
  20 >
  21 > Thing 1 can probably be handled within ikiwiki, if that seems less
  22 > gross to you.
  23
  24 >> It seems like it might be. You can use a `getopt` hook to check
  25 >> `@ARGV` to see how it was called. --[[Joey]]
  26
  27 >>> This does the trick iff the post-commit wrapper passes its args
  28 >>> along. Committed on my branch. This seems potentially dangerous,
  29 >>> since the args passed to ikiwiki are influenced by web commits.
  30 >>> I don't see an exploit, but for paranoia's sake, maybe the wrapper
  31 >>> should only be built with execv() if the cvs plugin is loaded?
  32 >>> --[[schmonz]]
  33
  34 >>>> Hadn't considered that. While in wrapper mode the normal getopt is not
  35 >>>> done, plugin getopt still runs, and so any unsafe options that
  36 >>>> other plugins support could be a problem if another user runs
  37 >>>> the setuid wrapper and passes those options through. --[[Joey]]
  38
  39 >>>>> I've tried compiling the argument check into the wrapper as
  40 >>>>> the first thing main() does, and was surprised to find that
  41 >>>>> this doesn't prevent the `cvs add <dir>` deadlock in a web
  42 >>>>> commit. I was convinced this'd be a reasonable solution,
  43 >>>>> especially if conditionalized on the cvs plugin being loaded,
  44 >>>>> but it doesn't work. And I stuck debug printfs at the beginning
  45 >>>>> of all the rcs_foo() subs, and whatever `cvs add <dir>` is
  46 >>>>> doing to ikiwiki isn't visible to my plugin, because none of
  47 >>>>> those subs are getting called. Nuts. Can you think of anything
  48 >>>>> else that might solve the problem, or should I go back to
  49 >>>>> generating a minimal wrapper wrapper that checks for just
  50 >>>>> this one thing? --[[schmonz]]
  51
  52 >>>>>> I don't see how there could possibly be a difference between
  53 >>>>>> ikiwiki's C wrapper and your shell wrapper wrapper here. --[[Joey]]
  54
  55 >>>>>>> I was comparing strings overly precisely. Fixed on my branch.
  56 >>>>>>> I've also knocked off the two most pressing to-do items. I
  57 >>>>>>> think the plugin's ready for prime time. --[[schmonz]]
  58
  59 > Thing 2 I'm less sure of. (I'd like to see the web UI return
  60 > immediately on save anyway, to a temporary "rebuilding, please wait
  61 > if you feel like knowing when it's done" page, but this problem
  62 > with CVS happens with any kind of commit, and could conceivably
  63 > happen with some other VCS.)
  64
  65 >> None of the other VCSes let a write lock block a read lock, apparently.
  66 >>
  67 >> Anyway, re the backgrounding, when committing via the web, the
  68 >> post-commit hook doesn't run anyway; the rendering is done via the
  69 >> ikiwiki CGI. It would certianly be nice if it popped up a quick "working"
  70 >> page and replaced it with the updated page when done, but that's
  71 >> unrelated;  the post-commit
  72 >> hook only does rendering when committing using the VCS directly. The
  73 >> backgrounding you do actually seems safe enough -- but tacking
  74 >> on a " &" to the ikiwiki wrapper call doesn't need a wrapper script,
  75 >> does it? --[[Joey]]
  76
  77 >>> Nope, it works fine to append it to the `CVSROOT/loginfo` line.
  78 >>> Fixed on my branch. --[[schmonz]]
  79
  80 > Thing 3 I think I did in order to squelch the error messages that
  81 > were bollixing up the CGI. It was easy to do this in the wrapper
  82 > wrapper, but if that's going away, it can be done just as easily
  83 > with output redirection in `CVSROOT/loginfo`.
  84 >
  85 > --[[schmonz]]
  86
  87 >> If the error messages screw up the CGI they must go to stdout.
  88 >> I thought we had stderr even in the the CVS dark ages. ;-) --[[Joey]]
  89
  90 >>> Some messages go to stderr, but definitely not all. That's why
  91 >>> I wound up reaching for IPC::Cmd, to execute the command line
  92 >>> safely while shutting CVS up. Anyway, I've tested what happens
  93 >>> if a configured post-commit hook is missing, and it seems fine,
  94 >>> probably also thanks to IPC::Cmd.
  95 >>> --[[schmonz]]
  96
  97 ----
  98
  99
 100 Further review.. --[[Joey]]
 101
 102 I don't understand what `cvs_shquote_commit` is
 103 trying to do with the test message, but it seems
 104 highly likely to be insecure; I never trust anything
 105 that relies on safely quoting user input passed to the shell.
 106
 107 (As an aside, `shell_quote` can die on certian inputs.)
 108
 109 Seems to me that, if `IPC::Cmd` exposes input to the shell
 110 (which I have not verified but its docs don't specify; a bad sign)
 111 you chose the wrong tool and ended up doing down the wrong
 112 route, dragging in shell quoting problems and fixes. Since you
 113 chose to use `IPC::Cmd` just because you wanted to shut
 114 up CVS stderr, my suggestion would be to use plain `system`
 115 to run the command, with stderr temporarily sent to /dev/null:
 116
 117         open(my $savederr, ">&STDERR");
 118         open(STDERR, ">", "/dev/null");
 119         my $ret=system("cvs", "-Q", @_);
 120         open(STDERR, ">$savederr");
 121
 122 `cvs_runcvs` should not take an array reference. It's
 123 usual for this type of function to take a list of parameters
 124 to pass to the command.
 125
 126 > Thanks for reading carefully. I've tested your suggestions and
 127 > applied them on my branch. --[[schmonz]]
 128
 129 ----
 130
 131 I've abstracted out CVS's involvement in the wrapper, adding a new
 132 "wrapperargcheck" hook to examine `argc/argv` and return success or
 133 failure (failure causes the wrapper to terminate) and implementing
 134 this hook in the plugin. In the non-CVS case, the check immediately
 135 returns success, so the added overhead is just a function call.
 136
 137 Given how rarely anything should need to reach in and modify the
 138 wrapper -- I'd go so far as to say we shouldn't make it too easy
 139 -- I don't think it's worth the effort to try and design a more
 140 general-purpose way to do so. If and when some other problem thinks
 141 it wants to be solved by a new wrapper hook, it's easy enough to add
 142 one. Until then, I'd say it's more important to keep the wrapper as
 143 short and clear as possible. --[[schmonz]]
 144
 145 > I've committed a slightly different hook, which should be general enough
 146 > that `IkiWiki::Receive` can also use it, so please adapt your code to
 147 > that. --[[Joey]]
 148
 149 >> Done. --[[schmonz]].
 150
 151 ----
 152
 153 I'm attempting to bring some polish to this plugin, starting with
 154 fuller test coverage. In preparation, I've refactored the tests a
 155 bunch (and shuffled the code a bit) in my branch. I'm worried,
 156 however, that my misunderstanding of `git rebase` may have made my
 157 branch harder for you to pull.
 158
 159 Before I go writing a whole swack of test cases, could you merge
 160 my latest? Through at least ad0e56cdcaaf76bc68d1b5c56e6845307b51c44a
 161 there should be no functional change. --[[schmonz]]
 162
 163 Never mind, I was able to convince myself (by cloning `origin`
 164 afresh and merging from `schmonz/cvs`). The history is a little
 165 gross but the before-and-after diff looks right.
 166
 167 Bugs found and fixed so far:
 168
 169 * Stop treating text files as binary (`-kb`) on `rcs_add()`
 170    (ac8eab29e8394aca4c0b23a6687ec947ea1ac869)
 171
 172 > Merged to current head. --[[Joey]]
 173
 174 ----
 175
 176 Hi! Bugfixes in `schmonz/cvs` I'd like to see merged:
 177
 178 * `6753235d`: Return bounded output from `rcs_diff()` when asked, as
 179   the API states.
 180
 181 * `e45175d5`: Always explicitly set CVS keyword substitution behavior.
 182   Fixes behavior when a text file is added under a name formerly
 183   used for a binary file.
 184
 185 * `b30cacdf`: If the previous working directory no longer exists after
 186   a CVS operation, don't try to `chdir()` back to it afterward.
 187
 188 These are all the diffs that exist on the branch, so if the changes
 189 are acceptable you should be able to simply merge the branch.
 190 --[[schmonz]]