Branko Čibej wrote:
> Julian Foad wrote:
>
>> The bug is that in our present implementation the "inherit" argument
>> controls both inheriting the environment and searching the path, where
>> the latter should presumably be an independent, orthogonal feature if
>> it is wanted at all.
>
> That doesn't seem like a bug to me. I don't see why we should complicate
> things. This whole thread looks suspiciously like a storm in a teacup.
Sorry it's turning into a storm. It was just a "Hey, look, this flag seems to
control significantly more than it says it does".
>> I submit that there should be a "path but no env" variant in APR. The
>> fact that it may be difficult to implement is no excuse :-)
>
> What $PATH are you going to search, if the environment is empty?
The same path that is searched when the sub-process inherits our environment:
the one from the caller's environment. That would make the search orthogonal
to the inheritance.
>> Given that there isn't, do we want to simulate it in Subversion, or
>> avoid the problem, just updating our doc to describe what we have
>> implemented, or what?
>
> If you want to be pedantic, just document the exact semantics of the
> inherit flag.
OK - see patch below. (I do want to be pedantic to this extent!)
> Don't try to expose all of APR's functionality in
> svn_io_start_cmd -- it's supposed to be useful for Subversion's use, not
> to be a general-purpose program execution routine.
Sure, I don't want to expose all of the possibilities APR supports. But this
function only has one non-local caller, and that caller (in
subversion/libsvn_repos/hooks.c) passes inherit=FALSE, so in fact the public
interface doesn't need to include that flag at all if we're not intending it to
be "general-purpose".
> It's not broken, so don't fix it.
The idea that the program should be searched for only if it is going to inherit
our environment seems illogical (and so, unless explained in the doc string,
"broken") to me. I would be interested to hear another opinion.
It makes sense if you come to it from first knowing the "execve" family of
functions, and then the APR function that is built on top of them, and then
finding that Subversion's svn_io_start_cmd() provides two of the behaviours
that are easily available from the lower-level functions - but that's not how
we try to write library functions. We try to write them to do a particular,
well-defined job, explained in high-level terms (or, in some cases, in terms of
an APR function).
OK, I won't "fix" it. Splitting the current option into two orthogonal options
("inherit environment" and "search the (our) path") seems logical, but it's
neither trivial nor required by our current code base, so not the right thing
to do if we're not aiming for a general-purpose interface.
I wouldn't be surprised, however, if when working on external diff or external
merge or hook scripts at some point in the future, we find we would like this.
But that's OK: develop when appropriate.
>>> (I'm not sure if the feature is "undocumented" so much as "assumed"
>>> :-) ).
>>
>> Er... what?
>
> What he said. There are lots of things in Subversion that are assumed
That's true, and to some extent it's necessary, but in this case, until I
thought about execve etc. I couldn't comprehend how that behaviour combination
could reasonably be assumed. That's why I didn't understand Karl's comment.
> but not documented. One of those is the exact semantics of most of the
> APR functions we rely on. As far as users are concerned, that's not a
> problem. Developers should read the APR docs.
That's fair to an extent. But our doc string should at least mention the
behaviour or refer to APR. In this case, reading the Subversion and APR docs
still wasn't sufficient. I had to read the implementation of our function.
I'm happy to document the actual behaviour and leave it at that. How's this?
Index: subversion/include/svn_io.h
===================================================================
--- subversion/include/svn_io.h (revision 17444)
+++ subversion/include/svn_io.h (working copy)
@@ -736,8 +736,10 @@
* terminated by @c NULL. @a args[0] is the name of the program, though it
* need not be the same as @a cmd.
*
- * @a inherit sets whether the invoked program shall inherit its environment or
- * run "clean".
+ * If @a inherit is true, the invoked program inherits its environment from
+ * the caller and @a cmd, if not absolute, is searched for in PATH.
+ * Otherwise, the invoked program runs with an empty environment and @a cmd
+ * must be an absolute path.
*
* @note On some platforms, failure to execute @a cmd in the child process
* will result in error output being written to @a errfile, if non-NULL, and
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Sat Nov 19 19:50:22 2005