Re: svn commit: r1241050 - /subversion/trunk/subversion/mod_dav_svn/mod_dav_svn.c
On Tue, Mar 06, 2012 at 12:39:27PM +, Julian Foad wrote:
> Daniel Shahaf wrote:
>
> > Daniel Shahaf wrote on Mon, Feb 06, 2012 at 18:20:28 +0200:
> >> Stefan Sperling wrote on Mon, Feb 06, 2012 at 17:12:32 +0100:
> >> > On Mon, Feb 06, 2012 at 05:59:04PM +0200, Daniel Shahaf wrote:
> >> > > This still strips whitespace around ='s in the value:
> >> > > SVNHooksEnv "name = x = y"
> >> > > will result in
> >> > > setenv("name", "x=y", 1)
> >> > > whereas I believe it should result in
> >> > > setenv("name", "x = y", 1)
> >> > > (and, to be honest, I'd be happy with
> >> > > setenv("name ", " x = y", 1)
> >> > > as well).
> >> > >
> >> > > WDYT? How should it behave?
> >> >
> >> > I agree.
> >> > would telling svn_cstring_split() to no strip whitespace suffice?
> >>
> >> I assume that should result in the third setenv() case above, so +1.
> >
> > Ping? trunk@HEAD still strips whitespace around equal signs in the value.
>
> My tuppence-worth? I agree that the current behaviour as stated above is
> wrong. Unless there is precedent to the contrary, I think it should do no
> stripping at all. If you can find precedent for some stripping in such a
> setting, then follow the precedent. Note that it's not only possible to
> strip spaces before and/or after the first '=' character, but also before
> "name" and/or after "x = y".
This option string is parsed by HTTPD.
The API provided by HTTPD is quite bad for this use case.
This is work in progress, and the above behaviour will be changed.
See http://subversion.tigris.org/issues/show_bug.cgi?id=4113
In the long term, the argument to the SVNHooksEnv option will be changed
anyway. It will become a path to a file which contains a list of environment
variables and their values. That file will then be parsed by our own config
parsing code.
Re: svn commit: r1241050 - /subversion/trunk/subversion/mod_dav_svn/mod_dav_svn.c
Daniel Shahaf wrote:
> Daniel Shahaf wrote on Mon, Feb 06, 2012 at 18:20:28 +0200:
>> Stefan Sperling wrote on Mon, Feb 06, 2012 at 17:12:32 +0100:
>> > On Mon, Feb 06, 2012 at 05:59:04PM +0200, Daniel Shahaf wrote:
>> > > This still strips whitespace around ='s in the value:
>> > > SVNHooksEnv "name = x = y"
>> > > will result in
>> > > setenv("name", "x=y", 1)
>> > > whereas I believe it should result in
>> > > setenv("name", "x = y", 1)
>> > > (and, to be honest, I'd be happy with
>> > > setenv("name ", " x = y", 1)
>> > > as well).
>> > >
>> > > WDYT? How should it behave?
>> >
>> > I agree.
>> > would telling svn_cstring_split() to no strip whitespace suffice?
>>
>> I assume that should result in the third setenv() case above, so +1.
>
> Ping? trunk@HEAD still strips whitespace around equal signs in the value.
My tuppence-worth? I agree that the current behaviour as stated above is
wrong. Unless there is precedent to the contrary, I think it should do no
stripping at all. If you can find precedent for some stripping in such a
setting, then follow the precedent. Note that it's not only possible to strip
spaces before and/or after the first '=' character, but also before "name"
and/or after "x = y".
- Julian
Re: svn commit: r1241050 - /subversion/trunk/subversion/mod_dav_svn/mod_dav_svn.c
Daniel Shahaf wrote on Mon, Feb 06, 2012 at 18:20:28 +0200:
> Stefan Sperling wrote on Mon, Feb 06, 2012 at 17:12:32 +0100:
> > On Mon, Feb 06, 2012 at 05:59:04PM +0200, Daniel Shahaf wrote:
> > > This still strips whitespace around ='s in the value:
> > > SVNHooksEnv "name = x = y"
> > > will result in
> > > setenv("name", "x=y", 1)
> > > whereas I believe it should result in
> > > setenv("name", "x = y", 1)
> > > (and, to be honest, I'd be happy with
> > > setenv("name ", " x = y", 1)
> > > as well).
> > >
> > > WDYT? How should it behave?
> >
> > I agree.
> > would telling svn_cstring_split() to no strip whitespace suffice?
>
> I assume that should result in the third setenv() case above, so +1.
Ping? trunk@HEAD still strips whitespace around equal signs in the value.
Re: svn commit: r1241050 - /subversion/trunk/subversion/mod_dav_svn/mod_dav_svn.c
Stefan Sperling wrote on Mon, Feb 06, 2012 at 17:12:32 +0100:
> On Mon, Feb 06, 2012 at 05:59:04PM +0200, Daniel Shahaf wrote:
> > This still strips whitespace around ='s in the value:
> > SVNHooksEnv "name = x = y"
> > will result in
> > setenv("name", "x=y", 1)
> > whereas I believe it should result in
> > setenv("name", "x = y", 1)
> > (and, to be honest, I'd be happy with
> > setenv("name ", " x = y", 1)
> > as well).
> >
> > WDYT? How should it behave?
>
> I agree.
> would telling svn_cstring_split() to no strip whitespace suffice?
I assume that should result in the third setenv() case above, so +1.
Re: svn commit: r1241050 - /subversion/trunk/subversion/mod_dav_svn/mod_dav_svn.c
On Mon, Feb 06, 2012 at 05:59:04PM +0200, Daniel Shahaf wrote:
> This still strips whitespace around ='s in the value:
> SVNHooksEnv "name = x = y"
> will result in
> setenv("name", "x=y", 1)
> whereas I believe it should result in
> setenv("name", "x = y", 1)
> (and, to be honest, I'd be happy with
> setenv("name ", " x = y", 1)
> as well).
>
> WDYT? How should it behave?
I agree.
would telling svn_cstring_split() to no strip whitespace suffice?
Re: svn commit: r1241050 - /subversion/trunk/subversion/mod_dav_svn/mod_dav_svn.c
This still strips whitespace around ='s in the value:
SVNHooksEnv "name = x = y"
will result in
setenv("name", "x=y", 1)
whereas I believe it should result in
setenv("name", "x = y", 1)
(and, to be honest, I'd be happy with
setenv("name ", " x = y", 1)
as well).
WDYT? How should it behave?
s...@apache.org wrote on Mon, Feb 06, 2012 at 15:46:54 -:
> Author: stsp
> Date: Mon Feb 6 15:46:54 2012
> New Revision: 1241050
>
> URL: http://svn.apache.org/viewvc?rev=1241050&view=rev
> Log:
> * subversion/mod_dav_svn/mod_dav_svn.c
> (SVNHooksEnv_cmd): Handle environment variables with values containing '='.
>While here, dup strings referenced from the hash table into the hash
>table's pool for safety.
>
> Modified:
> subversion/trunk/subversion/mod_dav_svn/mod_dav_svn.c
>
> Modified: subversion/trunk/subversion/mod_dav_svn/mod_dav_svn.c
> URL:
> http://svn.apache.org/viewvc/subversion/trunk/subversion/mod_dav_svn/mod_dav_svn.c?rev=1241050&r1=1241049&r2=1241050&view=diff
> ==
> --- subversion/trunk/subversion/mod_dav_svn/mod_dav_svn.c (original)
> +++ subversion/trunk/subversion/mod_dav_svn/mod_dav_svn.c Mon Feb 6 15:46:54
> 2012
> @@ -538,14 +538,36 @@ SVNHooksEnv_cmd(cmd_parms *cmd, void *co
>apr_array_header_t *var;
>
>var = svn_cstring_split(arg1, "=", TRUE, cmd->pool);
> - if (var && var->nelts == 2)
> + if (var && var->nelts >= 2)
> {
>dir_conf_t *conf = config;
> + const char *name;
> + const char *val;
>
> - apr_hash_set(conf->hooks_env,
> - APR_ARRAY_IDX(var, 0, const char *),
> - APR_HASH_KEY_STRING,
> - APR_ARRAY_IDX(var, 1, const char *));
> + name = apr_pstrdup(apr_hash_pool_get(conf->hooks_env),
> + APR_ARRAY_IDX(var, 0, const char *));
> +
> + /* Special case for values which contain '='. */
> + if (var->nelts > 2)
> +{
> + svn_stringbuf_t *buf;
> + int i;
> +
> + buf = svn_stringbuf_create(APR_ARRAY_IDX(var, 1, const char *),
> + cmd->pool);
> + for (i = 2; i < var->nelts; i++)
> +{
> + svn_stringbuf_appendbyte(buf, '=');
> + svn_stringbuf_appendcstr(buf, APR_ARRAY_IDX(var, i, const char
> *));
> +}
> +
> + val = apr_pstrdup(apr_hash_pool_get(conf->hooks_env), buf->data);
> +}
> + else
> +val = apr_pstrdup(apr_hash_pool_get(conf->hooks_env),
> + APR_ARRAY_IDX(var, 1, const char *));
> +
> + apr_hash_set(conf->hooks_env, name, APR_HASH_KEY_STRING, val);
> }
>
>return NULL;
>
>
