You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Bert Huijben <be...@qqmail.nl> on 2014/04/20 17:32:35 UTC
[Review request] RE: svn commit: r1588772 - in /subversion/trunk/subversion: mod_dav_svn/repos.c tests/cmdline/commit_tests.py
> -----Original Message-----
> From: rhuijben@apache.org [mailto:rhuijben@apache.org]
> Sent: zondag 20 april 2014 16:47
> To: commits@subversion.apache.org
> Subject: svn commit: r1588772 - in /subversion/trunk/subversion:
> mod_dav_svn/repos.c tests/cmdline/commit_tests.py
>
> Author: rhuijben
> Date: Sun Apr 20 14:46:42 2014
> New Revision: 1588772
>
> URL: http://svn.apache.org/r1588772
> Log:
> Implement an initial fix for issue #4480. As noted in the code this patch
> might make this code report out of date in a few cases where we should
> allow
> a commit, but this is much safer than not reporting out of date where we
> should.
>
> As far as I can tell this check matches how we do things in the repos/fs
> commit api, so I hope somebody with more knowledge of these apis can
> confirm that this is the right fix to close issue #4480.
Hi all,
This issue has been bothering me for weeks, because I couldn't think of a proper way to fix this.
I think this patch (improved by r1588778) resolves the issue, but I would welcome reviews.
If possible I would like to see this issue fixed in the next 1.8 and 1.7 releases as this issue allows breaking merge tracking by overwriting node properties without a proper out of date check.
Bert
RE: [Review request] RE: svn commit: r1588772 - in /subversion/trunk/subversion: mod_dav_svn/repos.c tests/cmdline/commit_tests.py
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: Bert Huijben [mailto:bert@qqmail.nl]
> Sent: zondag 20 april 2014 17:33
> To: dev@subversion.apache.org
> Subject: [Review request] RE: svn commit: r1588772 - in
> /subversion/trunk/subversion: mod_dav_svn/repos.c
> tests/cmdline/commit_tests.py
>
>
>
> > -----Original Message-----
> > From: rhuijben@apache.org [mailto:rhuijben@apache.org]
> > Sent: zondag 20 april 2014 16:47
> > To: commits@subversion.apache.org
> > Subject: svn commit: r1588772 - in /subversion/trunk/subversion:
> > mod_dav_svn/repos.c tests/cmdline/commit_tests.py
> >
> > Author: rhuijben
> > Date: Sun Apr 20 14:46:42 2014
> > New Revision: 1588772
> >
> > URL: http://svn.apache.org/r1588772
> > Log:
> > Implement an initial fix for issue #4480. As noted in the code this patch
> > might make this code report out of date in a few cases where we should
> > allow
> > a commit, but this is much safer than not reporting out of date where we
> > should.
> >
> > As far as I can tell this check matches how we do things in the repos/fs
> > commit api, so I hope somebody with more knowledge of these apis can
> > confirm that this is the right fix to close issue #4480.
>
> Hi all,
>
> This issue has been bothering me for weeks, because I couldn't think of a
> proper way to fix this.
>
> I think this patch (improved by r1588778) resolves the issue, but I would
> welcome reviews.
>
>
> If possible I would like to see this issue fixed in the next 1.8 and 1.7 releases
> as this issue allows breaking merge tracking by overwriting node properties
> without a proper out of date check.
Pinging this thread, on a workday...
Anybody?
> Bert
Re: [Review request] RE: svn commit: r1588772 - in
/subversion/trunk/subversion: mod_dav_svn/repos.c tests/cmdline/commit_tests.py
Posted by Stefan Fuhrmann <st...@wandisco.com>.
On Wed, Apr 23, 2014 at 11:40 PM, Stefan Fuhrmann <
stefan.fuhrmann@wandisco.com> wrote:
> So, apart from subtle typos, I'd say the patch does what it is
> supposed to do.
>
Read: there might be typos / copy'n'pastos that I missed.
Re: [Review request] RE: svn commit: r1588772 - in
/subversion/trunk/subversion: mod_dav_svn/repos.c tests/cmdline/commit_tests.py
Posted by Stefan Fuhrmann <st...@wandisco.com>.
On Sun, Apr 20, 2014 at 5:32 PM, Bert Huijben <be...@qqmail.nl> wrote:
>
>
> > -----Original Message-----
> > From: rhuijben@apache.org [mailto:rhuijben@apache.org]
> > Sent: zondag 20 april 2014 16:47
> > To: commits@subversion.apache.org
> > Subject: svn commit: r1588772 - in /subversion/trunk/subversion:
> > mod_dav_svn/repos.c tests/cmdline/commit_tests.py
> >
> > Author: rhuijben
> > Date: Sun Apr 20 14:46:42 2014
> > New Revision: 1588772
> >
> > URL: http://svn.apache.org/r1588772
> > Log:
> > Implement an initial fix for issue #4480. As noted in the code this patch
> > might make this code report out of date in a few cases where we should
> > allow
> > a commit, but this is much safer than not reporting out of date where we
> > should.
> >
> > As far as I can tell this check matches how we do things in the repos/fs
> > commit api, so I hope somebody with more knowledge of these apis can
> > confirm that this is the right fix to close issue #4480.
>
> Hi all,
>
> This issue has been bothering me for weeks, because I couldn't think of a
> proper way to fix this.
>
> I think this patch (improved by r1588778) resolves the issue, but I would
> welcome reviews.
>
Assuming that comb->priv.version_name is the BASE revision,
then your patch will actually check whether there has been a
commit to the respective path since BASE. If it is a directory,
it will implicitly detect all changes to the whole sub-tree.
So, apart from subtle typos, I'd say the patch does what it is
supposed to do.
- if (comb->priv.version_name < created_rev)
+ if (SVN_IS_VALID_REVNUM(created_rev))
{
>
Maybe, add a comment saying that you basically test for "node in
txn" vs. "node not in txn, yet".
+ /* Issue #4480: With HTTPv2 we can receive the first change for a
> + directory after it has been made mutable, because one of its
> + descendants was changed before changing the directory.
>
That comment seems odd. Did you intend to say the opposite
- first change *before* making a directory mutable?
If possible I would like to see this issue fixed in the next 1.8 and 1.7
> releases as this issue allows breaking merge tracking by overwriting node
> properties without a proper out of date check.
>
The relatedness check requires another indirection in 1.8-
as you need to get the svn_fs_id_t for each node and then
compare them.
-- Stefan^2.