Greg Stein <gstein@lyra.org> writes:
> > * subversion/libsvn_repos/hooks.c
> > (svn_repos__hooks_pre_revprop_change): Remove comment about passing
> > value on hook's stdin. Perhaps that comment was meant for
> > svn_repos__post_revprop_change or something.
>
> Nope... you almost definitely want to pass the new_value to the pre- hook so
> that the hook can examine the value to determine the acceptability of the
> change. For example, you might want to apply a restriction that svn:author
> values must match "[A-Za-z0-9]+".
Oh, ding! I didn't even think of that. Totally agree. Will make
that part of this change.
> Essentially, the pre- hook gets the new_value and the post- hook gets the
> old_value.
Check.
> The comments in the default script text should note that stdin *must* be
> consumed by the script, whether they choose to use the contents or not. If
> the new/old value happens to be larger than the pipe buffer size, then SVN
> will block on writing the data into the pipe. Should the child exit while
> SVN is blocked, then SVN will receive a SIGPIPE, and that would be a Bad
> Thing(tm).
Yes, I've been thinking about that all morning in the shower. It's an
ugly situation -- a hook shouldn't be required to consume, or even
examine, stdin. We're just asking for trouble if we impose
requirements like that.
But even worse, the whole structure of hooks.c:run_hook_cmd() isn't
set up to handle piped data well, as it uses svn_io_run_cmd()
internally. If we feed the data to a pipe, how can we send it all to
a running hook script? We'd either have to write all the data to the
pipe *before* we call svn_io_run_cmd (which we can't do, because the
data could be arbitrarily long), or write it to the pipe *after* we
call svn_io_run_cmd, by which point it's useless because the hook has
already exited.
Which kind of makes you wonder how the current stdout/stderr
apr_file_t's for svn_io_run_cmd work :-). Sure, if they're connected
to real underlying files, then great. But in run_hook_cmd(), we use a
pipe for stderr
apr_file_pipe_create(&read_errhandle, &write_errhandle, pool);
If the subprocess writes too much stderr (more than the pipe can
buffer), I'll bet we lose. Heh.
Also, notice how other callers of svn_io_run_cmd() usually keep the
stdin data in a file somewhere. They open up a read handle on the
file, and pass that handle as `infile' to svn_io_run_cmd(). That's
what I had reluctantly decided to do here too. I'd prefer not to use
real tempfiles, of course. If there were some way to make apr_file_t
behave like an svn_stream_t, such that the subprocess can read an
arbitrary amount of data whose source is an underlying memory
buffer... But I don't see a way to do that.
Of course, if we use tempfiles, the question becomes, why not just
pass the tempfile names to the hooks, and not bother with the stdin
stuff? But maybe it's cleaner to regard the tempfiles as
implementation details, and still use stdin for the hooks themselves.
The *real* problem here is that we only have synchronous subprocesses
in Subversion. We have svn_io_run_cmd(), but we don't have
`svn_io_start_cmd()'. To truly solve this, we have to deal with the
whole cross-platform fork/thread mess, though. I Don't Want To Go
There, not for this change.
Thoughts?
-K
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Fri Mar 14 17:09:53 2003