You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@subversion.apache.org by rh...@apache.org on 2015/01/15 09:51:48 UTC
svn commit: r1651980 - in /subversion/trunk/subversion: libsvn_wc/wc_db.c
tests/libsvn_wc/op-depth-test.c
Author: rhuijben
Date: Thu Jan 15 08:51:47 2015
New Revision: 1651980
URL: http://svn.apache.org/r1651980
Log:
Resolve the issue identified in r1651963, by properly calculating the depth
of the location where the moved to information should be stored after another
move.
* subversion/libsvn_wc/wc_db.c
(delete_update_movedto): Make this assertion maintainer only, like other
similar checks in wc_db.c.
(delete_node): Fix depth calculation.
* subversion/tests/libsvn_wc/op-depth-test.c
(nested_move_delete): Reorder nodes, to make them easier to review.
(test_funcs): Remove XFail assumption from nested_move_delete.
Modified:
subversion/trunk/subversion/libsvn_wc/wc_db.c
subversion/trunk/subversion/tests/libsvn_wc/op-depth-test.c
Modified: subversion/trunk/subversion/libsvn_wc/wc_db.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_wc/wc_db.c?rev=1651980&r1=1651979&r2=1651980&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_wc/wc_db.c (original)
+++ subversion/trunk/subversion/libsvn_wc/wc_db.c Thu Jan 15 08:51:47 2015
@@ -7594,7 +7594,11 @@ delete_update_movedto(svn_wc__db_wcroot_
op_depth,
new_moved_to_relpath));
SVN_ERR(svn_sqlite__update(&affected, stmt));
- assert(affected == 1);
+#ifdef SVN_DEBUG
+ /* Not fatal in release mode. The move recording is broken,
+ but the rest of the working copy can handle this. */
+ SVN_ERR_ASSERT(affected == 1);
+#endif
return SVN_NO_ERROR;
}
@@ -7984,7 +7988,12 @@ delete_node(void *baton,
child_relpath))
child_op_depth = delete_op_depth;
else
- child_op_depth = relpath_depth(child_relpath);
+ {
+ /* Calculate depth of the shadowing at the new location */
+ child_op_depth = child_op_depth
+ - relpath_depth(local_relpath)
+ + relpath_depth(b->moved_to_relpath);
+ }
fixup = TRUE;
}
Modified: subversion/trunk/subversion/tests/libsvn_wc/op-depth-test.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/tests/libsvn_wc/op-depth-test.c?rev=1651980&r1=1651979&r2=1651980&view=diff
==============================================================================
--- subversion/trunk/subversion/tests/libsvn_wc/op-depth-test.c (original)
+++ subversion/trunk/subversion/tests/libsvn_wc/op-depth-test.c Thu Jan 15 08:51:47 2015
@@ -9759,8 +9759,8 @@ nested_move_delete(const svn_test_opts_t
{2, "A/B/E/beta", "base-deleted", NO_COPY_FROM},
{0, "A/B/F", "normal", 1, "A/B/F"},
{2, "A/B/F", "base-deleted", NO_COPY_FROM},
- {2, "A/B/lambda", "base-deleted", NO_COPY_FROM, "A/Z/lambda"},
{0, "A/B/lambda", "normal", 1, "A/B/lambda"},
+ {2, "A/B/lambda", "base-deleted", NO_COPY_FROM, "A/Z/lambda"},
{0}
};
nodes_row_t nodes_AZ[] = {
@@ -9971,7 +9971,7 @@ static struct svn_test_descriptor_t test
"repo_wc_copy"),
SVN_TEST_OPTS_XFAIL(break_move_in_delete,
"break move in delete (issue 4491)"),
- SVN_TEST_OPTS_XFAIL(nested_move_delete,
+ SVN_TEST_OPTS_PASS(nested_move_delete,
"nested move delete"),
SVN_TEST_NULL
};
RE: svn commit: r1651980 - in /subversion/trunk/subversion: libsvn_wc/wc_db.c tests/libsvn_wc/op-depth-test.c
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: Branko Čibej [mailto:brane@wandisco.com]
> Sent: donderdag 15 januari 2015 10:35
> To: dev@subversion.apache.org
> Subject: Re: svn commit: r1651980 - in /subversion/trunk/subversion:
> libsvn_wc/wc_db.c tests/libsvn_wc/op-depth-test.c
>
> On 15.01.2015 09:51, rhuijben@apache.org wrote:
> > Author: rhuijben
> > Date: Thu Jan 15 08:51:47 2015
> > New Revision: 1651980
> >
> > URL: http://svn.apache.org/r1651980
> > Log:
> > Resolve the issue identified in r1651963, by properly calculating the depth
> > of the location where the moved to information should be stored after
> another
> > move.
> >
> > * subversion/libsvn_wc/wc_db.c
> > (delete_update_movedto): Make this assertion maintainer only, like other
> > similar checks in wc_db.c.
> > (delete_node): Fix depth calculation.
>
> If the depth calculation is now correct and the assertion doesn't get
> triggered, the SVN_DEBUG is doubly useless because it masks bugs. If the
> SVN_DEBUG had been there before, we'd never have got the original bug
> report in the first place. Either leave the assertion in or rip it out.
This is a common pattern in the move handling, which really isn't stable yet (especially during updates). We like the information about these missed matches during testing, but the move lookup code verifies whether there is a match ing src and destination before handling a node as moved.
Eventually somebody would have reported that he moved a file, but didn't see it as a move in status (or see it handled as move during update). Yes, that delays the report, but as long as we don't have server side move handling nothing really breaks when move information gets lost.
The 'assert(SOMETHING);' statements in the code are assumed to be left out in release builds, while SVN_ERR_ASSERT() statements are assumed to be left in.
(That is why we didn't get reports from TortoiseSVN users, nor mails from Stefan Kung on abort()s in release code)
SVN_ERR_ASSERT() can either abort() or return an error depending on what the api user wants to happen.
The problem here is that this specific user got an assert() triggering, so I switched it to the same code as at least 3 or 4 other functions have for similar cases.
Bert
Re: svn commit: r1651980 - in /subversion/trunk/subversion: libsvn_wc/wc_db.c tests/libsvn_wc/op-depth-test.c
Posted by Julian Foad <ju...@btopenworld.com>.
Branko Čibej wrote:
> On 15.01.2015 09:51, rhuijben@apache.org wrote:
>> URL: http://svn.apache.org/r1651980
>> Log:
>> Resolve the issue identified in r1651963, by properly calculating the depth
>> of the location where the moved to information should be stored after another
>> move.
>>
>> * subversion/libsvn_wc/wc_db.c
>> (delete_update_movedto): Make this assertion maintainer only, like other
>> similar checks in wc_db.c.
>> (delete_node): Fix depth calculation.
>
> If the depth calculation is now correct and the assertion doesn't get
> triggered, the SVN_DEBUG is doubly useless because it masks bugs. If the
> SVN_DEBUG had been there before, we'd never have got the original bug
> report in the first place. Either leave the assertion in or rip it out.
We should not hesitate to implement alternative kinds of assertion if they are useful.
It seems like what is wanted here is a non-fatal assertion: one that checks a condition, reports if the condition is not met, and then continues execution.
Sometimes when I start up a GUI from the command line, I see the
windowing toolkit library spewing assertion failure messages on the
console:
$ kdiff3
Object::connect: No such signal org::freedesktop::UPower::DeviceAdded(QDBusObjectPath)
Object::connect: No such signal org::freedesktop::UPower::DeviceRemoved(QDBusObjectPath)
...
Presumably we would not wish these to go to the console by default, but to a configurable place (file, gui dialogue box, ...).
- Julian
Re: svn commit: r1651980 - in /subversion/trunk/subversion: libsvn_wc/wc_db.c
tests/libsvn_wc/op-depth-test.c
Posted by Branko Čibej <br...@wandisco.com>.
On 15.01.2015 09:51, rhuijben@apache.org wrote:
> Author: rhuijben
> Date: Thu Jan 15 08:51:47 2015
> New Revision: 1651980
>
> URL: http://svn.apache.org/r1651980
> Log:
> Resolve the issue identified in r1651963, by properly calculating the depth
> of the location where the moved to information should be stored after another
> move.
>
> * subversion/libsvn_wc/wc_db.c
> (delete_update_movedto): Make this assertion maintainer only, like other
> similar checks in wc_db.c.
> (delete_node): Fix depth calculation.
If the depth calculation is now correct and the assertion doesn't get
triggered, the SVN_DEBUG is doubly useless because it masks bugs. If the
SVN_DEBUG had been there before, we'd never have got the original bug
report in the first place. Either leave the assertion in or rip it out.
-- Brane