You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@subversion.apache.org by st...@apache.org on 2015/06/23 13:18:16 UTC
svn commit: r1687029 -
/subversion/trunk/subversion/tests/cmdline/mergeinfo_tests.py
Author: stsp
Date: Tue Jun 23 11:18:15 2015
New Revision: 1687029
URL: http://svn.apache.org/r1687029
Log:
Add a regression test for issue #4582, "reintegrate complains about missing
ranges from node unrelated to branch"
* subversion/tests/cmdline/mergeinfo_tests.py
(no_mergeinfo_on_tree_conflict_victim, test_list): New test.
Modified:
subversion/trunk/subversion/tests/cmdline/mergeinfo_tests.py
Modified: subversion/trunk/subversion/tests/cmdline/mergeinfo_tests.py
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/tests/cmdline/mergeinfo_tests.py?rev=1687029&r1=1687028&r2=1687029&view=diff
==============================================================================
--- subversion/trunk/subversion/tests/cmdline/mergeinfo_tests.py (original)
+++ subversion/trunk/subversion/tests/cmdline/mergeinfo_tests.py Tue Jun 23 11:18:15 2015
@@ -786,6 +786,166 @@ def mergeinfo_local_move(sbox):
'mergeinfo', sbox.repo_url + '/A',
sbox.ospath('A2'))
+@SkipUnless(server_has_mergeinfo)
+@Issue(4582)
+def no_mergeinfo_on_tree_conflict_victim(sbox):
+ "do not record mergeinfo on tree conflict victims"
+ sbox.build()
+
+ # Create a branch of A called A_copy
+ sbox.simple_copy('A', 'A_copy')
+ sbox.simple_commit()
+
+ # Add a new directory and file on both branches
+ sbox.simple_mkdir('A/dir')
+ sbox.simple_add_text('new file', 'A/dir/f')
+ sbox.simple_commit()
+
+ sbox.simple_mkdir('A_copy/dir')
+ sbox.simple_add_text('new file', 'A_copy/dir/f')
+ sbox.simple_commit()
+
+ # Run a merge from A to A_copy
+ expected_output = wc.State(sbox.ospath('A_copy'), {
+ 'dir' : Item(status=' ', treeconflict='C'),
+ 'dir/f' : Item(status=' ', treeconflict='A'),
+ })
+ expected_mergeinfo_output = wc.State(sbox.ospath('A_copy'), {
+ '' : Item(status=' U'),
+ })
+ expected_elision_output = wc.State(sbox.ospath('A_copy'), {
+ })
+
+ expected_disk = svntest.wc.State('', {
+ 'C' : Item(),
+ 'B/E/beta' : Item(contents="This is the file 'beta'.\n"),
+ 'B/E/alpha' : Item(contents="This is the file 'alpha'.\n"),
+ 'B/lambda' : Item(contents="This is the file 'lambda'.\n"),
+ 'B/F' : Item(),
+ 'D/H/omega' : Item(contents="This is the file 'omega'.\n"),
+ 'D/H/psi' : Item(contents="This is the file 'psi'.\n"),
+ 'D/H/chi' : Item(contents="This is the file 'chi'.\n"),
+ 'D/G/tau' : Item(contents="This is the file 'tau'.\n"),
+ 'D/G/pi' : Item(contents="This is the file 'pi'.\n"),
+ 'D/G/rho' : Item(contents="This is the file 'rho'.\n"),
+ 'D/gamma' : Item(contents="This is the file 'gamma'.\n"),
+ 'dir/f' : Item(contents="new file"),
+ 'mu' : Item(contents="This is the file 'mu'.\n"),
+ })
+
+ # The merge will create an add vs add tree conflict on A_copy/dir
+ expected_status = svntest.wc.State(sbox.ospath('A_copy'), {
+ '' : Item(status=' M', wc_rev='4'),
+ 'D' : Item(status=' ', wc_rev='4'),
+ 'D/G' : Item(status=' ', wc_rev='4'),
+ 'D/G/pi' : Item(status=' ', wc_rev='4'),
+ 'D/G/rho' : Item(status=' ', wc_rev='4'),
+ 'D/G/tau' : Item(status=' ', wc_rev='4'),
+ 'D/H' : Item(status=' ', wc_rev='4'),
+ 'D/H/psi' : Item(status=' ', wc_rev='4'),
+ 'D/H/omega' : Item(status=' ', wc_rev='4'),
+ 'D/H/chi' : Item(status=' ', wc_rev='4'),
+ 'D/gamma' : Item(status=' ', wc_rev='4'),
+ 'B' : Item(status=' ', wc_rev='4'),
+ 'B/F' : Item(status=' ', wc_rev='4'),
+ 'B/E' : Item(status=' ', wc_rev='4'),
+ 'B/E/alpha' : Item(status=' ', wc_rev='4'),
+ 'B/E/beta' : Item(status=' ', wc_rev='4'),
+ 'B/lambda' : Item(status=' ', wc_rev='4'),
+ 'C' : Item(status=' ', wc_rev='4'),
+ 'dir' : Item(status=' ', treeconflict='C', wc_rev='4'),
+ 'dir/f' : Item(status=' ', wc_rev='4'),
+ 'mu' : Item(status=' ', wc_rev='4'),
+ })
+
+ expected_skip = wc.State('', { })
+
+ sbox.simple_update('A_copy')
+ svntest.actions.run_and_verify_merge(sbox.ospath('A_copy'),
+ None, None, # rev1, rev2
+ '^/A',
+ None, # URL2
+ expected_output,
+ expected_mergeinfo_output,
+ expected_elision_output,
+ expected_disk,
+ expected_status,
+ expected_skip)
+
+ # Resolve the tree conflict by accepting the working copy state left
+ # behind by the merge. This preserves the line of history of A_copy/dir,
+ # which originated on the branch 'A_copy', rather than replacing it with
+ # Jthe line f history of A/dir which originated on branch 'A'
+ svntest.actions.run_and_verify_resolve([sbox.ospath('A_copy/dir')],
+ '--accept', 'working',
+ sbox.ospath('A_copy/dir'))
+ sbox.simple_commit('A_copy')
+
+ # Now try to merge the 'A_copy' branch back to 'A"
+ expected_output = wc.State(sbox.ospath('A'), {
+ 'dir' : Item(status='R '), # changes line of history of A/dir
+ 'dir/f' : Item(status='A '),
+ })
+ expected_mergeinfo_output = wc.State(sbox.ospath('A'), {
+ '' : Item(status=' U'),
+ })
+ expected_elision_output = wc.State(sbox.ospath('A'), {
+ })
+
+ expected_disk = svntest.wc.State('', {
+ 'C' : Item(),
+ 'B/E/beta' : Item(contents="This is the file 'beta'.\n"),
+ 'B/E/alpha' : Item(contents="This is the file 'alpha'.\n"),
+ 'B/F' : Item(),
+ 'B/lambda' : Item(contents="This is the file 'lambda'.\n"),
+ 'D/H/omega' : Item(contents="This is the file 'omega'.\n"),
+ 'D/H/psi' : Item(contents="This is the file 'psi'.\n"),
+ 'D/H/chi' : Item(contents="This is the file 'chi'.\n"),
+ 'D/G/tau' : Item(contents="This is the file 'tau'.\n"),
+ 'D/G/pi' : Item(contents="This is the file 'pi'.\n"),
+ 'D/G/rho' : Item(contents="This is the file 'rho'.\n"),
+ 'D/gamma' : Item(contents="This is the file 'gamma'.\n"),
+ 'dir/f' : Item(contents="new file"),
+ 'mu' : Item(contents="This is the file 'mu'.\n"),
+ })
+
+ expected_status = svntest.wc.State(sbox.ospath('A'), {
+ '' : Item(status=' M', wc_rev='5'),
+ 'dir' : Item(status='R ', copied='+', wc_rev='-'),
+ 'dir/f' : Item(status=' ', copied='+', wc_rev='-'),
+ 'D' : Item(status=' ', wc_rev='5'),
+ 'D/H' : Item(status=' ', wc_rev='5'),
+ 'D/H/chi' : Item(status=' ', wc_rev='5'),
+ 'D/H/omega' : Item(status=' ', wc_rev='5'),
+ 'D/H/psi' : Item(status=' ', wc_rev='5'),
+ 'D/G' : Item(status=' ', wc_rev='5'),
+ 'D/G/pi' : Item(status=' ', wc_rev='5'),
+ 'D/G/rho' : Item(status=' ', wc_rev='5'),
+ 'D/G/tau' : Item(status=' ', wc_rev='5'),
+ 'D/gamma' : Item(status=' ', wc_rev='5'),
+ 'B' : Item(status=' ', wc_rev='5'),
+ 'B/E' : Item(status=' ', wc_rev='5'),
+ 'B/E/beta' : Item(status=' ', wc_rev='5'),
+ 'B/E/alpha' : Item(status=' ', wc_rev='5'),
+ 'B/lambda' : Item(status=' ', wc_rev='5'),
+ 'B/F' : Item(status=' ', wc_rev='5'),
+ 'mu' : Item(status=' ', wc_rev='5'),
+ 'C' : Item(status=' ', wc_rev='5'),
+ })
+
+ expected_skip = wc.State('', { })
+ sbox.simple_update('A')
+ svntest.actions.run_and_verify_merge(sbox.ospath('A'),
+ None, None, # rev1, rev2
+ '^/A_copy',
+ None, # URL2
+ expected_output,
+ expected_mergeinfo_output,
+ expected_elision_output,
+ expected_disk,
+ expected_status,
+ expected_skip)
+ sbox.simple_commit('A')
########################################################################
# Run the tests
@@ -806,6 +966,7 @@ test_list = [ None,
natural_history_is_not_eligible_nor_merged,
noninheritable_mergeinfo_not_always_eligible,
mergeinfo_local_move,
+ no_mergeinfo_on_tree_conflict_victim,
]
if __name__ == '__main__':
Re: svn commit: r1687029 - /subversion/trunk/subversion/tests/cmdline/mergeinfo_tests.py
Posted by Julian Foad <ju...@btopenworld.com>.
> Julian Foad wrote:
>> The following patch also appears to fix the bug -- all tests pass.
[...]
Bert Huijben wrote:
[...]
> The problem is that we mix recording of mergeinfo to keep the intermediate
> state… and the recording of the final conflict resolving result, by only
> updating the mergeinfo once… instead of actually doing something to resolve
> the conflict.
I don't quite follow, but maybe we don't need to discuss that further
in this thread. It would be a good topic to discuss and sort out in
another thread some time.
> Your patch makes the behavior change specific to add-add tree conflicts…
> That looks like a much better solution than blindly ignoring all tree
> conflict types as done in the previous patch.
OK, I committed my simplification in r1688258.
- Julian
RE: defining obstructions (was: Re: svn commit: r1687029 - /subversion/trunk/subversion/tests/cmdline/mergeinfo_tests.py)
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: Stefan Sperling [mailto:stsp@elego.de]
> Sent: woensdag 24 juni 2015 08:44
> To: Bert Huijben
> Cc: Julian Foad; dev@subversion.apache.org
> Subject: defining obstructions (was: Re: svn commit: r1687029 -
> /subversion/trunk/subversion/tests/cmdline/mergeinfo_tests.py)
>
> On Tue, Jun 23, 2015 at 11:59:25PM +0000, Bert Huijben wrote:
> > Update and merge report ‘obstructed; for different reasons… But are within
> itself pretty consistent, and the documentation of obstructed in svn_wc.h
> supports both implementations.
>
> Can you explain how the reasons for reporting an obstruction are different?
> I've tried finding the explanation in subversion/include/svn_wc.h but
> could not find any.
On Update/Merge things are easier:
We merge incoming changes compared to a common base in local modifications. (3 trees)
diff(original->right) into diff(original->working)
left=original
But on merge we merge the difference between left and right into a local tree that has modifications. (4 trees)
diff(left->right) into diff(pristine->working)
In the first case we can call something that is not in original, but is in working a local addition. It will also *always* have status 'A'dd.
But in case of the 4 tree merge left and pristine are not necessarily the same thing... On catchup merges they mostly are, but in general they may be completely different.
If there is something in WORKING, while left->right describes an ADD, or a change to a different kind of node, then there is some kind of obstruction.
Whether there already exists something locally, or whatever that local thing is... is not really that relevant.
In the current update/switch processing we specialize those variants, as that is in most cases the most valuable information on how to resolve the conflict.
>
> I believe we should only use 'obstruction' to refer to something that is
> unversioned from the point of view of the working copy we're operating in.
> Note that this includes sub-directories containing unrelated working copies
> which were checked out separately, and thus also externals.
>
But on merge this isn't the case. The most valuable information would be where you would want the merge to be applied.
* Was the node moved locally?
* Was the node moved remotely? (But the move not yet merged)
Calling this a local add, a local X, or a local obstruction doesn't really matter that much... . And I think that is why the merge code describes all these cases as local obstruction... as the node is *unrelated*.
In case of an update/switch the node can never be as unrelated as during a merge, as the nodes at least share the same path at a certain revision.
When resolving merge conflicts you would handle all these obstructions the same way... (Find proper target; reapply merge). While on update you would apply a different strategy.
But all of this comes back to the same problem over and over again:
* We only mark tree conflicts, but we offer no resolve features yet.
The only exception now (1.9+) is move conflicts during update/switch...
In 1.8 we started handling those, but left out many corner cases. For 1.9 I fixed all the corner cases that I could find during extensive testing (Including many cases that would break the working copy state in 1.8)
Bert
defining obstructions (was: Re: svn commit: r1687029 -
/subversion/trunk/subversion/tests/cmdline/mergeinfo_tests.py)
Posted by Stefan Sperling <st...@elego.de>.
On Tue, Jun 23, 2015 at 11:59:25PM +0000, Bert Huijben wrote:
> Update and merge report ‘obstructed; for different reasons… But are within itself pretty consistent, and the documentation of obstructed in svn_wc.h supports both implementations.
Can you explain how the reasons for reporting an obstruction are different?
I've tried finding the explanation in subversion/include/svn_wc.h but
could not find any.
I believe we should only use 'obstruction' to refer to something that is
unversioned from the point of view of the working copy we're operating in.
Note that this includes sub-directories containing unrelated working copies
which were checked out separately, and thus also externals.
If there are instances of a versioned node of some sort causing an
'obstruction' to be reported, I believe we should change that.
Philip pointed out to me yesterday that before 1.7 a locally added directory
or file could 'obstruct' an incoming newly added directory tree in the 1.6
working copy design, since we could not add the incoming subtree meta data
anywhere but in the subtree itself. This problem is gone now, so I think we
should stop referring to this case as a 'local obstruction vs. incoming add'
and instead call it what it is from the user's point of view, a 'local add
vs incoming add'
Re: svn commit: r1687029 - /subversion/trunk/subversion/tests/cmdline/mergeinfo_tests.py
Posted by Bert Huijben <be...@qqmail.nl>.
Update and merge report ‘obstructed; for different reasons… But are within itself pretty consistent, and the documentation of obstructed in svn_wc.h supports both implementations.
(I tried to make this more consistent a few months ago, but found that a lot of code and tests depends on the current implementation).
My change in the specific patch is explained in the thread on users@s.a.o. The most important change is that it properly records the tree conflict on the node that is tree conflicted, instead of on descendants of the conflict. (This fixed a related issue, but not the reported issue on the list). I tried not to change the behavior around that. I tried to ask Paul about the history of this behavior and behavior was discussed on the list. But no final solution was found.
The problem is that we mix recording of mergeinfo to keep the intermediate state… and the recording of the final conflict resolving result, by only updating the mergeinfo once… instead of actually doing something to resolve the conflict.
Your patch makes the behavior change specific to add-add tree conflicts… That looks like a much better solution than blindly ignoring all tree conflict types as done in the previous patch.
Sent from Surface
From: Julian Foad
Sent: Wednesday, June 24, 2015 1:22 AM
To: Bert Huijben, dev@subversion.apache.org
Bert and Stefan,
The following patch also appears to fix the bug -- all tests pass.
However, see my further comments below.
[[[
Index: subversion/libsvn_client/merge.c
===================================================================
--- subversion/libsvn_client/merge.c (revision 1687145)
+++ subversion/libsvn_client/merge.c (working copy)
@@ -2791,18 +2791,12 @@ merge_dir_opened(void **new_dir_baton,
SVN_ERR(svn_wc__node_is_added(&added, merge_b->ctx->wc_ctx,
local_abspath, scratch_pool));
db->tree_conflict_reason = added ? svn_wc_conflict_reason_added
:
svn_wc_conflict_reason_obstructed;
-
- if ((merge_b->merge_source.ancestral ||
merge_b->reintegrate_merge)
- && !(pdb && pdb->shadowed))
- {
- store_path(merge_b->skipped_abspaths, local_abspath);
- }
}
}
/* Handle pending conflicts */
SVN_ERR(mark_dir_edited(merge_b, db, local_abspath, scratch_pool));
@@ -5395,20 +5389,12 @@ record_skips_in_mergeinfo(const char *me
merge_b, skipped_abspath,
iterpool));
if (obstruction_state == svn_wc_notify_state_obstructed
|| obstruction_state == svn_wc_notify_state_missing)
continue;
- /* Make sure this is not a tree-conflicted path either. We don't
- * want to add mergeinfo on such nodes since it causes problems down
- * the line (see issue #4582, "reintegrate complains about missing
- * ranges from node unrelated to branch") */
- if (svn_hash_gets(merge_b->tree_conflicted_abspaths,
- skipped_abspath) != NULL)
- continue;
-
/* Add an empty range list for this path.
### TODO: This works fine for a file path skipped because it is
### missing as long as the file's parent directory is present.
### But missing directory paths skipped are not handled yet,
### see issue #2915.
]]]
Rather than adding code to ignore a 'skipped' path if it is also a
tree conflict path, this patch instead removes the code which recorded
an add-vs-add tree conflict victim as 'skipped'. That code block was
added recently (in http://svn.apache.org/r1666690) by Bert. Bert, can
you comment on it?
[[[
------------------------------------------------------------------------
r1666690 | rhuijben | 2015-03-14 15:30:17 +0100 (Sat, 14 Mar 2015) | 26 lines
In the libsvn_client merge code: When reporting and recording that we skip a
node because it is shadowed on an ancestor, properly record that the
ancestor is skipped instead of the descendant itself.
* subversion/libsvn_client/merge.c
(record_skip): Only record a skip, if we really skipped this node, and
not one of its ancestors. To do this handle this add a parent-baton
argument to this function.
(merge_file_changed,
merge_file_added,
merge_file_deleted): Update caller.
(merge_dir_opened): Not only skip the directory add, but also record it
as skipped.
(merge_dir_changed,
merge_dir_added,
merge_dir_deleted,
merge_node_absent): Update caller.
* subversion/tests/cmdline/merge_tree_conflict_tests.py
(merge_obstruction_recording): New function.
(test_list): Add merge_obstruction_recording.
Found by: Pete Harlan <pchpublic88{_AT_}gmail.com>
]]]
However, neither the current code nor the code resulting from this
patch looks entirely correct. Notice the first statement shown in the
patch context:
SVN_ERR(svn_wc__node_is_added(&added,...));
...reason = added ? ...reason_added : ...reason_obstructed;
That looks at the WC schedule to decide whether to call this an
'added' or an 'obstructed' conflict. For a merge, I think if the
directory exists in the working state, then we should report the
reason as 'added' regardless whether the dir was added in a commit on
the checked-out branch or locally added in the WC. And I'm not
entirely sure when we should call it 'obstructed' -- maybe only when
there is a local unversioned object on disk where the WC working state
expects there is nothing?
- Julian
Re: svn commit: r1687029 - /subversion/trunk/subversion/tests/cmdline/mergeinfo_tests.py
Posted by Julian Foad <ju...@btopenworld.com>.
Bert and Stefan,
The following patch also appears to fix the bug -- all tests pass.
However, see my further comments below.
[[[
Index: subversion/libsvn_client/merge.c
===================================================================
--- subversion/libsvn_client/merge.c (revision 1687145)
+++ subversion/libsvn_client/merge.c (working copy)
@@ -2791,18 +2791,12 @@ merge_dir_opened(void **new_dir_baton,
SVN_ERR(svn_wc__node_is_added(&added, merge_b->ctx->wc_ctx,
local_abspath, scratch_pool));
db->tree_conflict_reason = added ? svn_wc_conflict_reason_added
:
svn_wc_conflict_reason_obstructed;
-
- if ((merge_b->merge_source.ancestral ||
merge_b->reintegrate_merge)
- && !(pdb && pdb->shadowed))
- {
- store_path(merge_b->skipped_abspaths, local_abspath);
- }
}
}
/* Handle pending conflicts */
SVN_ERR(mark_dir_edited(merge_b, db, local_abspath, scratch_pool));
@@ -5395,20 +5389,12 @@ record_skips_in_mergeinfo(const char *me
merge_b, skipped_abspath,
iterpool));
if (obstruction_state == svn_wc_notify_state_obstructed
|| obstruction_state == svn_wc_notify_state_missing)
continue;
- /* Make sure this is not a tree-conflicted path either. We don't
- * want to add mergeinfo on such nodes since it causes problems down
- * the line (see issue #4582, "reintegrate complains about missing
- * ranges from node unrelated to branch") */
- if (svn_hash_gets(merge_b->tree_conflicted_abspaths,
- skipped_abspath) != NULL)
- continue;
-
/* Add an empty range list for this path.
### TODO: This works fine for a file path skipped because it is
### missing as long as the file's parent directory is present.
### But missing directory paths skipped are not handled yet,
### see issue #2915.
]]]
Rather than adding code to ignore a 'skipped' path if it is also a
tree conflict path, this patch instead removes the code which recorded
an add-vs-add tree conflict victim as 'skipped'. That code block was
added recently (in http://svn.apache.org/r1666690) by Bert. Bert, can
you comment on it?
[[[
------------------------------------------------------------------------
r1666690 | rhuijben | 2015-03-14 15:30:17 +0100 (Sat, 14 Mar 2015) | 26 lines
In the libsvn_client merge code: When reporting and recording that we skip a
node because it is shadowed on an ancestor, properly record that the
ancestor is skipped instead of the descendant itself.
* subversion/libsvn_client/merge.c
(record_skip): Only record a skip, if we really skipped this node, and
not one of its ancestors. To do this handle this add a parent-baton
argument to this function.
(merge_file_changed,
merge_file_added,
merge_file_deleted): Update caller.
(merge_dir_opened): Not only skip the directory add, but also record it
as skipped.
(merge_dir_changed,
merge_dir_added,
merge_dir_deleted,
merge_node_absent): Update caller.
* subversion/tests/cmdline/merge_tree_conflict_tests.py
(merge_obstruction_recording): New function.
(test_list): Add merge_obstruction_recording.
Found by: Pete Harlan <pchpublic88{_AT_}gmail.com>
]]]
However, neither the current code nor the code resulting from this
patch looks entirely correct. Notice the first statement shown in the
patch context:
SVN_ERR(svn_wc__node_is_added(&added,...));
...reason = added ? ...reason_added : ...reason_obstructed;
That looks at the WC schedule to decide whether to call this an
'added' or an 'obstructed' conflict. For a merge, I think if the
directory exists in the working state, then we should report the
reason as 'added' regardless whether the dir was added in a commit on
the checked-out branch or locally added in the WC. And I'm not
entirely sure when we should call it 'obstructed' -- maybe only when
there is a local unversioned object on disk where the WC working state
expects there is nothing?
- Julian
Re: svn commit: r1687029 -
/subversion/trunk/subversion/tests/cmdline/mergeinfo_tests.py
Posted by Stefan Sperling <st...@stsp.name>.
On Tue, Jun 23, 2015 at 01:34:59PM +0200, Bert Huijben wrote:
> I don't think this label accurately describes the intended case of this test.
>
> Personally I think it is better to add mergeinfo on the TC-victim then to *add* non-inherital mergeinfo to both the direct ancestor *and* inheritable mergeinfo to *all* siblings, which is usually the only alternative. ("The least nodes with mergeinfo, the better")
>
> The label would say me that this is a problem we are checking, while you are really checking to see if a reintegrate would work.... Or at least that is what the log message says.
>
> We should try to avoid mixing wanted/expected behavior ("reintegrate merge doesn't work") with the implementation details ("unexpected svn:mergeinfo") here.
>
Do you have a better suggestion about what the test description should say?
> If reintegrate is really the problem, I'm missing the reasoning why the test is added to mergeinfo_tests.py, as there is also a merge_reintegrate_tests.py.
Is the question of which file this should live in very important?
I figured the root of the problem lies with creating the mergeinfo.
The reintegrate problem results as a symptom of that.
There is no good reason to create this mergeinfo because once the tree
conflict is resolved somehow the node was merged. So the impilict mergeinfo
inherited to the formerly conflicted node can be considered correct.
RE: svn commit: r1687029 - /subversion/trunk/subversion/tests/cmdline/mergeinfo_tests.py
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: stsp@apache.org [mailto:stsp@apache.org]
> Sent: dinsdag 23 juni 2015 13:18
> To: commits@subversion.apache.org
> Subject: svn commit: r1687029 -
> /subversion/trunk/subversion/tests/cmdline/mergeinfo_tests.py
>
> Author: stsp
> Date: Tue Jun 23 11:18:15 2015
> New Revision: 1687029
>
> URL: http://svn.apache.org/r1687029
> Log:
> Add a regression test for issue #4582, "reintegrate complains about missing
> ranges from node unrelated to branch"
>
> * subversion/tests/cmdline/mergeinfo_tests.py
> (no_mergeinfo_on_tree_conflict_victim, test_list): New test.
>
> Modified:
> subversion/trunk/subversion/tests/cmdline/mergeinfo_tests.py
>
> Modified: subversion/trunk/subversion/tests/cmdline/mergeinfo_tests.py
> URL:
> http://svn.apache.org/viewvc/subversion/trunk/subversion/tests/cmdline/merg
> einfo_tests.py?rev=1687029&r1=1687028&r2=1687029&view=diff
> ================================================================
> ==============
> --- subversion/trunk/subversion/tests/cmdline/mergeinfo_tests.py (original)
> +++ subversion/trunk/subversion/tests/cmdline/mergeinfo_tests.py Tue Jun 23
> 11:18:15 2015
> @@ -786,6 +786,166 @@ def mergeinfo_local_move(sbox):
> 'mergeinfo', sbox.repo_url + '/A',
> sbox.ospath('A2'))
>
> +@SkipUnless(server_has_mergeinfo)
> +@Issue(4582)
> +def no_mergeinfo_on_tree_conflict_victim(sbox):
> + "do not record mergeinfo on tree conflict victims"
> + sbox.build()
I don't think this label accurately describes the intended case of this test.
Personally I think it is better to add mergeinfo on the TC-victim then to *add* non-inherital mergeinfo to both the direct ancestor *and* inheritable mergeinfo to *all* siblings, which is usually the only alternative. ("The least nodes with mergeinfo, the better")
The label would say me that this is a problem we are checking, while you are really checking to see if a reintegrate would work.... Or at least that is what the log message says.
We should try to avoid mixing wanted/expected behavior ("reintegrate merge doesn't work") with the implementation details ("unexpected svn:mergeinfo") here.
If reintegrate is really the problem, I'm missing the reasoning why the test is added to mergeinfo_tests.py, as there is also a merge_reintegrate_tests.py.
Bert