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

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|

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

Bert Huijben-5


> -----Original Message-----
> From: [hidden email] [mailto:[hidden email]]
> Sent: maandag 20 november 2017 13:44
> To: [hidden email]
> 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&view=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


Reply | Threaded
Open this post in threaded view
|

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

Evgeny Kotkov
Bert Huijben <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

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

Julian Foad-5
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.

Reply | Threaded
Open this post in threaded view
|

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

Evgeny Kotkov
Evgeny Kotkov <[hidden email]> 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 <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

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

Julian Foad-5
Evgeny Kotkov wrote:
> Julian Foad <[hidden email]> 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