Re: svn commit: r1815799 - in /subversion/trunk/subversion: libsvn_ra_serf/commit.c tests/libsvn_ra/ra-test.c

[Julian Foad]

Evgeny Kotkov wrote:

Julian Foad  writes:

Don't replace it with 'relpath &&' instead, however. If relpath is null then
I think the next line (svn_hash_gets(..., relpath)) would immediately crash
anyway, so allowing it here is useless and therefore confusing. Remove that
condition entirely. That's my suggestion.


The condition is inverted in the sense that it checks for a null relpath
and returns if so — in other words, the svn_hash_gets() won't get called
if the relpath is null.


Oh... oops! I was confused. Right you are.


However, as it turns out, this condition can indeed be simplified by not
checking for null, as the only calling site where the relpath might be
null, setup_proppatch_headers(), already checks for it.  (Among the other
two calling sites, the relpath cannot be null as it would have segfaulted
earlier).

I committed this simplification in r1816061, thanks!


Thanks.

- Julian

Re: svn commit: r1815799 - in /subversion/trunk/subversion: libsvn_ra_serf/commit.c tests/libsvn_ra/ra-test.c

[Evgeny Kotkov]

Evgeny Kotkov  writes:

> (2) I am going to tweak the new test so that it would properly open the
> parent directory and commit to a locked file, to have this case
> covered with a native test.

Committed in r1816060.

Julian Foad  writes:

>> (1) Keep the new simpler check in maybe_set_lock_token_header() — as,
>>  unless I missing something, there should be no reason to explicitly
>>  filter empty relpaths for the lock tokens since they are invalid.
>
> I agree it is good to remove the '*relpath &&' condition if there is no
> reason why it needs to be there.
>
> Don't replace it with 'relpath &&' instead, however. If relpath is null then
> I think the next line (svn_hash_gets(..., relpath)) would immediately crash
> anyway, so allowing it here is useless and therefore confusing. Remove that
> condition entirely. That's my suggestion.

The condition is inverted in the sense that it checks for a null relpath
and returns if so — in other words, the svn_hash_gets() won't get called
if the relpath is null.

However, as it turns out, this condition can indeed be simplified by not
checking for null, as the only calling site where the relpath might be
null, setup_proppatch_headers(), already checks for it.  (Among the other
two calling sites, the relpath cannot be null as it would have segfaulted
earlier).

I committed this simplification in r1816061, thanks!


Regards,
Evgeny Kotkov

Re: svn commit: r1815799 - in /subversion/trunk/subversion: libsvn_ra_serf/commit.c tests/libsvn_ra/ra-test.c

[Julian Foad]

Evgeny Kotkov wrote:

I think that I have missed that open_root() must open a directory and so
it cannot be used when working with a file URL.  This, in turn, means
that the test example is indeed an undocumented/invalid usage of the
delta editor API and that it works by a coincidence.

I can propose doing the following:

(1) Keep the new simpler check in maybe_set_lock_token_header() — as,
 unless I missing something, there should be no reason to explicitly
 filter empty relpaths for the lock tokens since they are invalid.


I agree it is good to remove the '*relpath &&' condition if there is no 
reason why it needs to be there.


Don't replace it with 'relpath &&' instead, however. If relpath is null 
then I think the next line (svn_hash_gets(..., relpath)) would 
immediately crash anyway, so allowing it here is useless and therefore 
confusing. Remove that condition entirely. That's my suggestion.


- Julian


(2) I am going to tweak the new test so that it would properly open the
 parent directory and commit to a locked file, to have this case
 covered with a native test.

Re: svn commit: r1815799 - in /subversion/trunk/subversion: libsvn_ra_serf/commit.c tests/libsvn_ra/ra-test.c

[Evgeny Kotkov]

Bert Huijben  writes:

> I'm not sure if we should really allow this.
>
> The delta editor explicitly describes that you are opening a directory and
> then edit the nodes inside. Only changing properties on the root is allowed
> and other operations are all on nodes within. Allowing to open the node
> itself again may cause all kinds of problems as there are now multiple
> handles pointing to the same thing. How will this be expressed in the
> filesystem/transaction?
>
> I'm surprised that all the other filesystems allow this, so perhaps this
> is a safe change... but the documentation in svn_delta.h doesn't describe
> this as a safe extension. (Which would theoretically allow this as a safe
> extension in later versions... but we must make sure that we are not
> opening new issues this way)
>
> Currently I would guess that making the ra layers provide a proper error
> for this case would not be a bad thing... All our drivers explicitly open
> an existing directory when they want to edit a file...

I think that I have missed that open_root() must open a directory and so
it cannot be used when working with a file URL.  This, in turn, means
that the test example is indeed an undocumented/invalid usage of the
delta editor API and that it works by a coincidence.

I can propose doing the following:

(1) Keep the new simpler check in maybe_set_lock_token_header() — as,
unless I missing something, there should be no reason to explicitly
filter empty relpaths for the lock tokens since they are invalid.

(2) I am going to tweak the new test so that it would properly open the
parent directory and commit to a locked file, to have this case
covered with a native test.


Thanks,
Evgeny Kotkov

RE: svn commit: r1815799 - in /subversion/trunk/subversion: libsvn_ra_serf/commit.c tests/libsvn_ra/ra-test.c

[Bert Huijben]

> -Original Message-
> From: kot...@apache.org [mailto:kot...@apache.org]
> Sent: maandag 20 november 2017 13:44
> To: comm...@subversion.apache.org
> Subject: svn commit: r1815799 - in /subversion/trunk/subversion:
> libsvn_ra_serf/commit.c tests/libsvn_ra/ra-test.c
> 
> Author: kotkov
> Date: Mon Nov 20 12:43:33 2017
> New Revision: 1815799
> 
> URL: http://svn.apache.org/viewvc?rev=1815799=rev
> Log:
> ra_serf: Properly process lock tokens for empty relative paths ("")
> within the commit editor.

I'm not sure if we should really allow this.

The delta editor explicitly describes that you are opening a directory and then 
edit the nodes inside. Only changing properties on the root is allowed and 
other operations are all on nodes within. Allowing to open the node itself 
again may cause all kinds of problems as there are now multiple handles 
pointing to the same thing. How will this be expressed in the 
filesystem/transaction?

I'm surprised that all the other filesystems allow this, so perhaps this is a 
safe change... but the documentation in svn_delta.h doesn't describe this as a 
safe extension. (Which would theoretically allow this as a safe extension in 
later versions... but we must make sure that we are not opening new issues this 
way)


Currently I would guess that making the ra layers provide a proper error for 
this case would not be a bad thing... All our drivers explicitly open an 
existing directory when they want to edit a file...

Bert