On Thu, Dec 20, 2012 at 9:01 AM, <rhuijben_at_apache.org> wrote:
> Author: rhuijben
> Date: Thu Dec 20 14:01:37 2012
> New Revision: 1424469
>
> URL: http://svn.apache.org/viewvc?rev=1424469&view=rev
> Log:
> In the repository-repository diff handler: Don't retrieve pristine properties
> when we already know that there are no differences to report on them.
>
> By checking whether we really need the properties we avoid about 1000 ra calls
> over running our test suite (ra local). This also resolves many spurious merge
> changes that just change entry props.
>
> On top of that stop reporting entry prop only changes as a file/directory
> change to avoid doing unneeded work in the merge and diff handling.
>
> This fixes some issues identified when the merge code was updated to do a
> better obstruction detection, as originally we just skipped these non-changes
> there in the merge code.
>
> It is possible that this obscures some tree conflicts which were identified by
> entry prop changes that should have been detected by the real change down the
> tree (which might have been skipped).
>
> * subversion/libsvn_client/repos_diff.c
> (dir_baton,
> file_baton): Add has_propchange boolean.
> (close_file,
> close_directory): Only retrieve the pristine properties if we may be going
> to use them in a callback.
>
> (change_file_prop,
> change_dir_prop): Detect if we have a real property change and if we have
> store that information.
>
> * subversion/tests/cmdline/merge_reintegrate_tests.py
> (reintegrate_on_shallow_wc): Don't assume to get spurious change event.
>
> * subversion/tests/cmdline/merge_tests.py
> (merge_to_sparse_directories): Don't expect entry prop change only skips.
>
> * subversion/tests/cmdline/merge_tree_conflict_tests.py
> (tree_conflicts_merge_edit_onto_missing,
> tree_conflicts_merge_del_onto_missing):
> Remove some unexpected change notifications.
>
> Modified:
> subversion/trunk/subversion/libsvn_client/repos_diff.c
> subversion/trunk/subversion/tests/cmdline/merge_reintegrate_tests.py
> subversion/trunk/subversion/tests/cmdline/merge_tests.py
> subversion/trunk/subversion/tests/cmdline/merge_tree_conflict_tests.py
>
> Modified: subversion/trunk/subversion/libsvn_client/repos_diff.c
> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_client/repos_diff.c?rev=1424469&r1=1424468&r2=1424469&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/libsvn_client/repos_diff.c (original)
> +++ subversion/trunk/subversion/libsvn_client/repos_diff.c Thu Dec 20 14:01:37 2012
> @@ -143,6 +143,9 @@ struct dir_baton {
> /* A cache of any property changes (svn_prop_t) received for this dir. */
> apr_array_header_t *propchanges;
>
> + /* Boolean indicating whether a node property was changed */
> + svn_boolean_t has_propchange;
> +
> /* The pool passed in by add_dir, open_dir, or open_root.
> Also, the pool this dir baton is allocated in. */
> apr_pool_t *pool;
> @@ -199,6 +202,9 @@ struct file_baton {
> /* A cache of any property changes (svn_prop_t) received for this file. */
> apr_array_header_t *propchanges;
>
> + /* Boolean indicating whether a node property was changed */
> + svn_boolean_t has_propchange;
> +
> /* The pool passed in by add_file or open_file.
> Also, the pool this file_baton is allocated in. */
> apr_pool_t *pool;
> @@ -971,9 +977,11 @@ close_file(void *file_baton,
> fb->path));
> }
>
> - if (!fb->added && fb->propchanges->nelts > 0)
> + if (fb->path_end_revision || fb->has_propchange)
> {
> - if (!fb->pristine_props)
> + const char *mimetype1, *mimetype2;
> +
> + if (!fb->added && !fb->pristine_props)
> {
> /* We didn't receive a text change, so we have no pristine props.
> Retrieve just the props now. */
> @@ -981,11 +989,7 @@ close_file(void *file_baton,
> }
>
> remove_non_prop_changes(fb->pristine_props, fb->propchanges);
> - }
>
> - if (fb->path_end_revision || fb->propchanges->nelts > 0)
> - {
> - const char *mimetype1, *mimetype2;
> get_file_mime_types(&mimetype1, &mimetype2, fb);
>
>
> @@ -1100,37 +1104,40 @@ close_directory(void *dir_baton,
>
> scratch_pool = db->pool;
>
> - if (db->added)
> + if (db->has_propchange)
> {
> - pristine_props = eb->empty_hash;
> - }
> - else
> - {
> - SVN_ERR(svn_ra_get_dir2(eb->ra_session, NULL, NULL, &pristine_props,
> - db->path, db->base_revision, 0, scratch_pool));
> - }
> -
> - if (db->propchanges->nelts > 0)
> - {
> - remove_non_prop_changes(pristine_props, db->propchanges);
> - }
> + if (db->added)
> + {
> + pristine_props = eb->empty_hash;
> + }
> + else
> + {
> + SVN_ERR(svn_ra_get_dir2(eb->ra_session, NULL, NULL, &pristine_props,
> + db->path, db->base_revision, 0, scratch_pool));
> + }
>
> - if (db->propchanges->nelts > 0)
> - {
> - svn_boolean_t tree_conflicted = FALSE;
> - SVN_ERR(eb->diff_callbacks->dir_props_changed(
> - &prop_state, &tree_conflicted,
> - db->path, db->added,
> - db->propchanges, pristine_props,
> - eb->diff_cmd_baton, scratch_pool));
> - if (tree_conflicted)
> - db->tree_conflicted = TRUE;
> + if (db->propchanges->nelts > 0)
> + {
> + remove_non_prop_changes(pristine_props, db->propchanges);
> + }
>
> - if (prop_state == svn_wc_notify_state_obstructed
> - || prop_state == svn_wc_notify_state_missing)
> + if (db->propchanges->nelts > 0)
> {
> - content_state = prop_state;
> - skipped = TRUE;
> + svn_boolean_t tree_conflicted = FALSE;
> + SVN_ERR(eb->diff_callbacks->dir_props_changed(
> + &prop_state, &tree_conflicted,
> + db->path, db->added,
> + db->propchanges, pristine_props,
> + eb->diff_cmd_baton, scratch_pool));
> + if (tree_conflicted)
> + db->tree_conflicted = TRUE;
> +
> + if (prop_state == svn_wc_notify_state_obstructed
> + || prop_state == svn_wc_notify_state_missing)
> + {
> + content_state = prop_state;
> + skipped = TRUE;
> + }
> }
> }
>
> @@ -1218,6 +1225,9 @@ change_file_prop(void *file_baton,
> if (fb->skip)
> return SVN_NO_ERROR;
>
> + if (!fb->has_propchange && svn_property_kind2(name) == svn_prop_regular_kind)
> + fb->has_propchange = TRUE;
> +
> propchange = apr_array_push(fb->propchanges);
> propchange->name = apr_pstrdup(fb->pool, name);
> propchange->value = value ? svn_string_dup(value, fb->pool) : NULL;
> @@ -1241,6 +1251,9 @@ change_dir_prop(void *dir_baton,
> if (db->skip)
> return SVN_NO_ERROR;
>
> + if (!db->has_propchange && svn_property_kind2(name) == svn_prop_regular_kind)
> + db->has_propchange = TRUE;
> +
> propchange = apr_array_push(db->propchanges);
> propchange->name = apr_pstrdup(db->pool, name);
> propchange->value = value ? svn_string_dup(value, db->pool) : NULL;
>
> Modified: subversion/trunk/subversion/tests/cmdline/merge_reintegrate_tests.py
> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/tests/cmdline/merge_reintegrate_tests.py?rev=1424469&r1=1424468&r2=1424469&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/tests/cmdline/merge_reintegrate_tests.py (original)
> +++ subversion/trunk/subversion/tests/cmdline/merge_reintegrate_tests.py Thu Dec 20 14:01:37 2012
> @@ -825,15 +825,6 @@ def reintegrate_on_shallow_wc(sbox):
> 'Some more work on the A_COPY branch', wc_dir)
> # Reuse the same expectations as the prior merge, except that
> # non-inheritable mergeinfo is set on the root of the missing subtree...
> - expected_mergeinfo_output.add({
> - 'D' : Item(status=' U')
> - })
> - expected_A_status.tweak('D', status=' M')
> - expected_A_disk.tweak('D', props={SVN_PROP_MERGEINFO : '/A_COPY/D:2-4*'})
> - # ... a depth-restricted item is skipped ...
> - expected_A_skip.add({
> - 'D/H' : Item()
> - })
> # ... and the mergeinfo on the target root includes the latest rev on the branch.
> expected_A_disk.tweak('', props={SVN_PROP_MERGEINFO : '/A_COPY:2-4'})
> svntest.actions.run_and_verify_merge(A_path, None, None,
>
> Modified: subversion/trunk/subversion/tests/cmdline/merge_tests.py
> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/tests/cmdline/merge_tests.py?rev=1424469&r1=1424468&r2=1424469&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/tests/cmdline/merge_tests.py (original)
> +++ subversion/trunk/subversion/tests/cmdline/merge_tests.py Thu Dec 20 14:01:37 2012
> @@ -7812,8 +7812,8 @@ def merge_to_sparse_directories(sbox):
> # Merge r4:9 into the immediates WC.
> # The root of the immediates WC should get inheritable r4:9 as should
> # the one file present 'mu'. The three directory children present, 'B',
> - # 'C', and 'D' are checked out at depth empty; the two of these affected
> - # by the merge, 'B' and 'D', get non-inheritable mergeinfo for r4:9.
> + # 'C', and 'D' are checked out at depth empty; the one of these affected
> + # by the merge, 'D', gets non-inheritable mergeinfo for r4:9.
> # The root and 'D' do should also get the changes
> # that affect them directly (the prop adds from r8 and r9).
> expected_output = wc.State(immediates_dir, {
> @@ -7823,14 +7823,14 @@ def merge_to_sparse_directories(sbox):
> })
> expected_mergeinfo_output = wc.State(immediates_dir, {
> '' : Item(status=' U'),
> - 'B' : Item(status=' U'),
> 'D' : Item(status=' U'),
> })
> expected_elision_output = wc.State(immediates_dir, {
> + 'D' : Item(status=' U'),
> })
> expected_status = wc.State(immediates_dir, {
> '' : Item(status=' M', wc_rev=9),
> - 'B' : Item(status=' M', wc_rev=9),
> + 'B' : Item(status=' ', wc_rev=9),
> 'mu' : Item(status='M ', wc_rev=9),
> 'C' : Item(status=' ', wc_rev=9),
> 'D' : Item(status=' M', wc_rev=9),
> @@ -7838,15 +7838,12 @@ def merge_to_sparse_directories(sbox):
> expected_disk = wc.State('', {
> '' : Item(props={SVN_PROP_MERGEINFO : '/A:5-9',
> "prop:name" : "propval"}),
> - 'B' : Item(props={SVN_PROP_MERGEINFO : '/A/B:5-9*'}),
> + 'B' : Item(),
> 'mu' : Item("New content"),
> 'C' : Item(),
> - 'D' : Item(props={SVN_PROP_MERGEINFO : '/A/D:5-9*',
> - "prop:name" : "propval"}),
> + 'D' : Item(props={"prop:name" : "propval"}),
> })
> expected_skip = svntest.wc.State(immediates_dir, {
> - 'D/H' : Item(),
> - 'B/E' : Item(),
> })
Hi Bert,
This change breaks mergetracking. Let's walk-through just the test
change above (please bear with me, this is a somewhat lengthy
explanation, but stsp and julianf seem fine with this, so I want to be
completely clear).
First lets look at how merge worked prior to your change:
### In this part of the test we are merging into the root of a WC of
A_COPY checked
### out at depth immediates. We have no local changes and no prior
merges from ^/A:
trunk_at_1424468>svn st -v
9 2 jrandom .
9 1 jrandom B
9 1 jrandom C
9 1 jrandom D
9 1 jrandom mu
trunk_at_1424468>svn pg svn:mergeinfo -vR
### The merge the test performs (^/A -r4:9) touches two files (beta
and omega) that are
### not present in the WC due to the shallow checkout:
trunk_at_1424468>svn diff --summarize ^^/A -r4:9
M file:///C:/SVN/src-trunk-2/Debug/subversion/tests/cmdline/svn-test-work/repositories/merge_tests-56/A/B/E/beta
M file:///C:/SVN/src-trunk-2/Debug/subversion/tests/cmdline/svn-test-work/repositories/merge_tests-56/A/mu
M file:///C:/SVN/src-trunk-2/Debug/subversion/tests/cmdline/svn-test-work/repositories/merge_tests-56/A/D/H/omega
M file:///C:/SVN/src-trunk-2/Debug/subversion/tests/cmdline/svn-test-work/repositories/merge_tests-56/A/D
M file:///C:/SVN/src-trunk-2/Debug/subversion/tests/cmdline/svn-test-work/repositories/merge_tests-56/A
### This change would be applied to a infinite depth WC:
trunk_at_1424468>svn diff ^^/A/B/E/beta -r4:9
Index: beta
===================================================================
--- beta (revision 4)
+++ beta (revision 9)
@@ -1 +1 @@
-This is the file 'beta'.
+New content
\ No newline at end of file
### Ditto this one:
trunk_at_1424468>svn diff ^^/A/D/H/omega -r4:9
Index: omega
===================================================================
--- omega (revision 4)
+++ omega (revision 9)
@@ -1 +1 @@
-This is the file 'omega'.
+New content
\ No newline at end of file
### mu is present in the WC:
trunk_at_1424468>svn diff ^^/A/mu -r4:9
Index: mu
===================================================================
--- mu (revision 4)
+++ mu (revision 9)
@@ -1 +1 @@
-This is the file 'mu'.
+New content
\ No newline at end of file
### This is the merge the test performs. B/E and D/H are skipped
because obviously
### they are not present in the WC due to the shallow depth:
trunk_at_1424468>svn merge ^^/A . -r4:9
Skipped 'B\E'
--- Merging r5 through r9 into '.':
U mu
Skipped 'D\H'
--- Merging r5 through r9 into 'D':
U D
--- Merging r5 through r9 into '.':
U .
--- Recording mergeinfo for merge of r5 through r9 into '.':
U .
--- Recording mergeinfo for merge of r5 through r9 into 'B':
U B
--- Recording mergeinfo for merge of r5 through r9 into 'D':
U D
Summary of conflicts:
Skipped paths: 2
### The merge sets mergeinfo describing the merge on the target
(nothing unusual here)
### but it also sets non-inheritable mergeinfo on the present parents
of the skipped subtrees
### to signal that the merge wasn't able to reach into those subtrees
to apply the diff.
### Notice that 'C' doesn't get any non-inheritable mergeinfo set
because there were
### no potential changes under that subtree:
trunk_at_1424468>svn pg svn:mergeinfo -vR
Properties on '.':
svn:mergeinfo
/A:5-9
Properties on 'B':
svn:mergeinfo
/A/B:5-9*
Properties on 'D':
svn:mergeinfo
/A/D:5-9*
### Now to show why this non-inheritable subtree mergeinfo is important.
### First we commit the merge:
trunk_at_1424468>svn ci -m ""
Sending .
Sending B
Sending D
Sending mu
Transmitting file data .
Committed revision 10.
### Then we update the WC to infinite depth, bringing in the previously missing
### subtrees that the ^/A -r4:9 had changes to apply:
trunk_at_1424468>svn up --set-depth infinity
Updating '.':
A B\lambda
A B\E
A B\E\alpha
A B\E\beta
A B\F
A D\gamma
A D\G
A D\G\pi
A D\G\rho
A D\G\tau
A D\H
A D\H\chi
A D\H\omega
A D\H\psi
Updated to revision 10.
### Now we repeat the earlier merge. The mergetracking logic notices
that the previously
### "missing" subtrees are now present and merges the changes to
'beta' and 'omega':
trunk_at_1424468>svn merge ^^/A . -r4:9
--- Merging r5 through r6 into 'B\E':
U B\E\beta
--- Merging r5 through r6 into 'D\H':
U D\H\omega
--- Recording mergeinfo for merge of r5 through r9 into '.':
U .
--- Recording mergeinfo for merge of r5 through r9 into 'B':
U B
--- Eliding mergeinfo from 'B':
U B
--- Recording mergeinfo for merge of r5 through r9 into 'B\E':
G B\E
--- Eliding mergeinfo from 'B\E':
U B\E
--- Recording mergeinfo for merge of r5 through r9 into 'D':
U D
--- Eliding mergeinfo from 'D':
U D
--- Recording mergeinfo for merge of r5 through r9 into 'D\H':
G D\H
--- Eliding mergeinfo from 'D\H':
U D\H
trunk_at_1424468>svn st
M B
M B\E\beta
M D
M D\H\omega
trunk_at_1424468>svn diff B\E\beta
Index: B/E/beta
===================================================================
--- B/E/beta (revision 10)
+++ B/E/beta (working copy)
@@ -1 +1 @@
-This is the file 'beta'.
+New content
\ No newline at end of file
trunk_at_1424468>svn diff D\H\omega
Index: D/H/omega
===================================================================
--- D/H/omega (revision 10)
+++ D/H/omega (working copy)
@@ -1 +1 @@
-This is the file 'omega'.
+New content
\ No newline at end of file
### The non-inheritable subtree mergeinfo elides and now the mergeinfo clearly
### shows that the entirety of ^/A -r4:9 has been merged to A_COPY:
trunk_at_1424468>svn pg svn:mergeinfo -vR
Properties on '.':
svn:mergeinfo
/A:5-9
Now lets look at what happens with your patch:
### We perform the same merge, but there are no skips this time:
trunk_at_1424469>svn merge ^^/A -r4:9
--- Merging r5 through r9 into '.':
U mu
--- Merging r5 through r9 into 'D':
U D
--- Merging r5 through r9 into '.':
U .
--- Recording mergeinfo for merge of r5 through r9 into '.':
U .
--- Recording mergeinfo for merge of r5 through r9 into 'D':
U D
--- Eliding mergeinfo from 'D':
U D
### No non-inheritable subtree mergeinfo is set either:
trunk_at_1424469>svn st
M .
M D
M mu
trunk_at_1424469>svn pl -vR
Properties on '.':
prop:name
propval
svn:mergeinfo
/A:5-9
Properties on 'D':
prop:name
propval
trunk_at_1424469>svn pg svn:mergeinfo -vR
Properties on '.':
svn:mergeinfo
/A:5-9
### It's probably obvious what the problem is now, but let's continue.
### We commit the merge:
trunk_at_1424469>svn ci -m ""
Sending .
Sending D
Sending mu
Transmitting file data .
Committed revision 10.
### As before we update the WC to infinite depth:
trunk_at_1424469>svn up --set-depth infinity
Updating '.':
A B\lambda
A B\E
A B\E\alpha
A B\E\beta
A B\F
A D\gamma
A D\G
A D\G\pi
A D\G\rho
A D\G\tau
A D\H
A D\H\psi
A D\H\chi
A D\H\omega
Updated to revision 10.
### But now if we repeat the merge the lack of subtree mergeinfo makes it appear
### that A_COPY has *all* the changes from ^/A -r4:9 and the merge is a no-op:
trunk_at_1424469>svn merge ^^/A -r4:9
--- Recording mergeinfo for merge of r5 through r9 into '.':
U .
This is a fundamental change to the way mergeinfo/mergegracking works.
It has the potential for really serious (and silent) problems during
feature branch merges. Suppose we have a classic case like this:
---trunk---------------o
\ \ ^
\ merge /
\ \ merge ("reintegrate style")
\ V /
\---branch-
Say for some reason the first "sync" merge is done into a shallow WC.
Further, assume some of the incoming changes from trunk apply to
subtrees on branch that are missing due to the shallow WC (I recently
saw a user doing just this, expecting it to work
http://subversion.tigris.org/issues/show_bug.cgi?id=4187)
With your patch there is nothing to indicate anything is wrong during
the first merge, no skip notifications, no non-inheritable mergeinfo,
nothing.
Now when we do the second merge, the mergeinfo on branch seems to
indicate that it is fully synced with trunk (which we know it is not)
and the "reintegrate-style" merge succeeds. But recall the subtree
changes that never made it to branch because of the shallow WC? Those
are removed from trunk too! This is almost certainly *not* what the
user intended! Does this make sense? I can work up a more thorough
example if need be.
Julian & Stefan - Are you on board with this change or were you unaware of it?
--
Paul T. Burba
CollabNet, Inc. -- www.collab.net -- Enterprise Cloud Development
Skype: ptburba
> svntest.actions.run_and_verify_merge(immediates_dir, '4', '9',
> sbox.repo_url + '/A', None,
> @@ -7900,7 +7897,6 @@ def merge_to_sparse_directories(sbox):
> })
> expected_skip = svntest.wc.State(files_dir, {
> 'D' : Item(),
> - 'B' : Item(),
> })
> svntest.actions.run_and_verify_merge(files_dir, '4', '9',
> sbox.repo_url + '/A', None,
> @@ -7944,7 +7940,6 @@ def merge_to_sparse_directories(sbox):
> expected_skip = svntest.wc.State(empty_dir, {
> 'mu' : Item(),
> 'D' : Item(),
> - 'B' : Item(),
> })
> svntest.actions.run_and_verify_merge(empty_dir, '4', '9',
> sbox.repo_url + '/A', None,
>
> Modified: subversion/trunk/subversion/tests/cmdline/merge_tree_conflict_tests.py
> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/tests/cmdline/merge_tree_conflict_tests.py?rev=1424469&r1=1424468&r2=1424469&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/tests/cmdline/merge_tree_conflict_tests.py (original)
> +++ subversion/trunk/subversion/tests/cmdline/merge_tree_conflict_tests.py Thu Dec 20 14:01:37 2012
> @@ -1332,12 +1332,8 @@ def tree_conflicts_merge_edit_onto_missi
> expected_skip = svntest.wc.State('', {
> 'F/alpha' : Item(),
> # BH: After fixing several issues in the obstruction handling
> - # I get the following Skip notifications. Please review!
> + # I get the following Skip notification. Please review!
> 'D/D1' : Item(),
> - 'DD/D1' : Item(),
> - 'DF/D1' : Item(),
> - 'DDD/D1' : Item(),
> - 'DDF/D1' : Item(),
> })
>
>
> @@ -1414,13 +1410,6 @@ def tree_conflicts_merge_del_onto_missin
> expected_skip = svntest.wc.State('', {
> 'F/alpha' : Item(),
> 'D/D1' : Item(),
> - # BH: After fixing several issues in the obstruction handling
> - # I get the following Skip notifications. Please review!
> - 'D/D1' : Item(),
> - 'DD/D1' : Item(),
> - 'DF/D1' : Item(),
> - 'DDD/D1' : Item(),
> - 'DDF/D1' : Item(),
> })
>
> svntest.actions.deep_trees_run_tests_scheme_for_merge(sbox,
Received on 2012-12-21 16:33:49 CET