You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@subversion.apache.org by Daniel Shahaf <da...@elego.de> on 2012/03/04 11:56:31 UTC

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

Posted by Stefan Sperling <st...@elego.de>.
On Tue, Mar 06, 2012 at 12:39:27PM +0000, 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

Posted by Stefan Sperling <st...@elego.de>.
On Tue, Mar 06, 2012 at 12:39:27PM +0000, 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

Posted by Julian Foad <ju...@btopenworld.com>.
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

Posted by Julian Foad <ju...@btopenworld.com>.
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