Julian Foad wrote:
> kfogel@collab.net wrote:
>> Julian Foad <julianfoad@btopenworld.com> writes:
>>
>>> From svn_io_start_cmd():
>>>
>>>> * @a inherit sets whether the invoked program shall inherit its
>>>> environment or
>>>> * run "clean".
>>>
>>>> /* Make sure we invoke cmd directly, not through a shell. */
>>>> apr_err = apr_procattr_cmdtype_set (cmdproc_attr,
>>>>
>>>> inherit?APR_PROGRAM_PATH:APR_PROGRAM);
>>>
>>> That should be "APR_PROGRAM_ENV", not "APR_PROGRAM_PATH", else
>>> path-search is enabled as well as env. But didn't we specifically
>>> want path-search for our external "diff" support? Is that diff
>>> support relying on an undocumented feature (path search)?
>>>
>>> Something's wrong. Anyone care to decide what?
>>
>>
>> Are they bitmasks, and if so, shouldn't we do both? That is:
>>
>> apr_err = apr_procattr_cmdtype_set
>> (cmdproc_attr,
>> inherit ? (APR_PROGRAM_ENV | APR_PROGRAM_PATH) :
>> APR_PROGRAM);
>
> Er... no. They're not bit masks. APR_PROGRAM_PATH already means "env
> + path". APR_PROGRAM, on the other hand, means "no env, no path".
>
> 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.
> There doesn't seem to be a "path but not env" variant available
> directly from APR. From apr/include/apr_thread_proc.h:
>
> typedef enum {
> APR_SHELLCMD, /**< use the shell to invoke the program */
> APR_PROGRAM, /**< invoke the program directly, no
> copied env */
> APR_PROGRAM_ENV, /**< invoke the program, replicating our
> environment */
> APR_PROGRAM_PATH, /**< find program on PATH, use our
> environment */
> APR_SHELLCMD_ENV /**< use the shell to invoke the program,
> * replicating our environment
> */
> } apr_cmdtype_e;
>
> 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?
> 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. 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. It's not broken, so
don't fix it.
>> (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
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.
-- Brane
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Sat Nov 19 15:01:42 2005