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 2010/04/06 16:24:27 UTC
svn commit: r931164 - /subversion/branches/svn-patch-improvements/
Author: stsp
Date: Tue Apr 6 14:24:27 2010
New Revision: 931164
URL: http://svn.apache.org/viewvc?rev=931164&view=rev
Log:
On the svn-patch-improvements branch, block r931162 from trunk to avoid
a cyclic merge the next time this branch is synced to trunk.
Modified:
subversion/branches/svn-patch-improvements/ (props changed)
Propchange: subversion/branches/svn-patch-improvements/
------------------------------------------------------------------------------
--- svn:mergeinfo (original)
+++ svn:mergeinfo Tue Apr 6 14:24:27 2010
@@ -33,4 +33,4 @@
/subversion/branches/tc_url_rev:874351-874483
/subversion/branches/tree-conflicts:868291-873154
/subversion/branches/tree-conflicts-notify:873926-874008
-/subversion/trunk:918519-930647
+/subversion/trunk:918519-930647,931162
Re: svn commit: r931164 - /subversion/branches/svn-patch-improvements/
Posted by Paul Burba <pt...@gmail.com>.
On Tue, Apr 6, 2010 at 10:32 AM, Hyrum K. Wright <hy...@hyrumwright.org> wrote:
> I'm interested to know why this is needed. Is our merge tracking implementation so broken that it doesn't handle these types of things?
>
> (Forgive me if I'm being naïve here.)
>
> -Hyrum
>
> On Apr 6, 2010, at 9:24 AM, stsp@apache.org wrote:
>
>> Author: stsp
>> Date: Tue Apr 6 14:24:27 2010
>> New Revision: 931164
>>
>> URL: http://svn.apache.org/viewvc?rev=931164&view=rev
>> Log:
>> On the svn-patch-improvements branch, block r931162 from trunk to avoid
>> a cyclic merge the next time this branch is synced to trunk.
>>
>> Modified:
>> subversion/branches/svn-patch-improvements/ (props changed)
>>
>> Propchange: subversion/branches/svn-patch-improvements/
>> ------------------------------------------------------------------------------
>> --- svn:mergeinfo (original)
>> +++ svn:mergeinfo Tue Apr 6 14:24:27 2010
>> @@ -33,4 +33,4 @@
>> /subversion/branches/tc_url_rev:874351-874483
>> /subversion/branches/tree-conflicts:868291-873154
>> /subversion/branches/tree-conflicts-notify:873926-874008
>> -/subversion/trunk:918519-930647
>> +/subversion/trunk:918519-930647,931162
Merge tracking has always had problems with cyclic (or reflective if
your prefer) merges, see
http://subversion.tigris.org/issues/show_bug.cgi?id=2837
The svn-patch-improvements branch illustrates the problem; here is
what we have (excluding r931164!):
---trunk@918518------------------------------------r931162------->
| | | | ^
| | | | |
copy sync merges cherry picked
| keeping branch back to trunk
| up to date with ^ ^
| trunk through | |
| r930647 | |
| | | | | |
V V V V | |
svn-patch-improvements@918521-----------------r929288--r931140-->
What happens when this branch is next synced with trunk? The current
merge logic will look at the mergeinfo on the branch and assume it
needs to merge ^/subversion/trunk -r930647:HEAD.
Problem is, that range includes r931162, which is the cherry pick from
the branch. So it will be included in the applied diff. This *might*
not be a problem if no further changes have occurred on the branch in
the files touched by r929288 and r931140. In fact, in this case they
will currently merge cleanly:
C:\SVN\src-branch-svn-patch-improvements>svn up -r931163
U .
Updated to revision 931163.
C:\SVN\src-branch-svn-patch-improvements>svn merge ^^/subversion/trunk .
--- Merging r930648 through r931175 into '.':
U subversion\libsvn_diff\parse-diff.c
U subversion\libsvn_client\mergeinfo.c
U subversion\tests\libsvn_subr\dirent_uri-test.c
U subversion\tests\libsvn_client\client-test.c
U subversion\tests\cmdline\mergeinfo_tests.py
U subversion\tests\cmdline\patch_tests.py
U tools\buildbot\slaves\win32-SharpSvn\svntest-test.cmd
U tools\buildbot\slaves\win32-SharpSvn\svntest-cleanup.cmd
U tools\buildbot\slaves\win32-SharpSvn
G .
But obviously if more work took place on the branch in the files
touched by r929288 and r931140, then needless conflicts might occur.
Now let's look at the branch with Stefan's --record-only merge applied:
---trunk@918518--------------------------------------r931162------------------->
| | | | ^ |__________
| | | | | |
copy sync merges cherry picked --record-only
| keeping branch back to trunk merge of r931162
| up to date with ^ ^ |
| trunk through | | |
| r930647 | | |
| | | | | | |
V V V V | | V
svn-patch-improvements@918521-----------------r929288--r931140----r931164--->
Now if we sync merge the branch with trunk, the mergeinfo on the
branch causes two discrete diffs to be applied:
^/subversion/trunk -r930647:931161
^/subversion/trunk -r931162:HEAD
C:\SVN\src-branch-svn-patch-improvements>svn revert -R . -q
C:\SVN\src-branch-svn-patch-improvements>svn up
U .
Updated to revision 931188.
C:\SVN\src-branch-svn-patch-improvements>svn merge ^^/subversion/trunk .
--- Merging r930648 through r931161 into '.':
U subversion\libsvn_client\mergeinfo.c
U subversion\tests\libsvn_subr\dirent_uri-test.c
U subversion\tests\libsvn_client\client-test.c
U subversion\tests\cmdline\mergeinfo_tests.py
U subversion\tests\cmdline\patch_tests.py
U tools\buildbot\slaves\win32-SharpSvn\svntest-test.cmd
U tools\buildbot\slaves\win32-SharpSvn\svntest-cleanup.cmd
U tools\buildbot\slaves\win32-SharpSvn
--- Merging r931163 through r931190 into '.':
U subversion\include\private\svn_mergeinfo_private.h
U subversion\libsvn_subr\mergeinfo.c
U subversion\tests\libsvn_subr\mergeinfo-test.c
Now it is probably tempting to say, "Look at the mergeinfo on trunk
created by r931162":
C:\SVN\src-branch-svn-patch-improvements>svn diff -N
^^/subversion/trunk -r931161:931162
Property changes on: .
___________________________________________________________________
Modified: svn:mergeinfo
Merged /subversion/branches/svn-patch-improvements:r929288,931140
"Can't we deduce from that mergeinfo that we don't need to merge this
change back to the svn-patch-improvements branch when we next sync
it?" Not really, because we don't know if r931162 included any other
changes *besides* the merge from svn-patch-improvements. If it did,
do we include or exclude those changes from being merged?
~~~~~
Anyhow, cyclic merges are not handled very well outside of
reintegrate. If anybody out there thinks they have a solution for
issue #2837, by all means, patches welcome.
Paul
reflective merges (was: Re: svn commit: r931164 -
/subversion/branches/svn-patch-improvements/)
Posted by Stefan Sperling <st...@elego.de>.
On Tue, Apr 06, 2010 at 09:32:00AM -0500, Hyrum K. Wright wrote:
> I'm interested to know why this is needed. Is our merge tracking
> implementation so broken that it doesn't handle these types of things?
Short version:
The implementation isn't broken, but the design is broken.
(The problem was known well before 1.5 release btw, it's not new.)
Long version:
Right now, the commit I made in r931164 makes no difference.
Syncing the svn-patch-improvements branch to the trunk works
fine without it:
$ svn merge -c -931164 .
--- Reverse-merging r931164 into '.':
U .
--- Recording mergeinfo for reverse merge of r931164 into '.':
G .
$ svn merge ^/subversion/trunk
--- Merging r930648 through r931168 into '.':
U subversion/libsvn_client/mergeinfo.c
U subversion/tests/libsvn_subr/dirent_uri-test.c
U subversion/tests/libsvn_client/client-test.c
U subversion/tests/cmdline/mergeinfo_tests.py
U subversion/tests/cmdline/patch_tests.py
U tools/buildbot/slaves/win32-SharpSvn/svntest-test.cmd
U tools/buildbot/slaves/win32-SharpSvn/svntest-cleanup.cmd
U tools/buildbot/slaves/win32-SharpSvn
G .
--- Recording mergeinfo for merge of r930648 through r931168 into '.':
G .
But I don't want dannas to run into problems when doing the next
sync merge. Potentially, he could be making more changes on his branch
before he syncs to trunk again. When he does the sync, svn will attempt
to apply the combined diff of r929288 and r931140 to the branch,
even though the diff originally came from the merge target. Subversion
does this because merge-tracking uses PATH:REV as the ID for a change,
and it doesn't realise that trunk:r931164 represents the same semantic
change as branches/svn-patch-improvements:r929288 and
branches/svn-patch-improvements:r931140 do together.
So if dannas makes further changes, they could conflict with the change
which bounces back from trunk to his branch, and that could annoy or
confuse him. And I don't want him to get annoyed or confused :)
Now, in the svn merge output above you do not see the above merge touching
all of the files which were modified in r929288 and r931140 (patch_test.py
is modified because of Bert's 930990).
So maybe I am wrong and Subversion is smart and does not try to merge the
changes again? Or maybe that is just because Subversion attempts to merge
them and discovers that it doesn't have any work to do, so it doesn't
bother providing notification about paths which haven't really changed?
Let's do a test:
$ svn revert -R .
Reverted '.'
Reverted 'subversion/libsvn_client/mergeinfo.c'
Reverted 'subversion/tests/cmdline/mergeinfo_tests.py'
Reverted 'subversion/tests/cmdline/patch_tests.py'
Reverted 'subversion/tests/libsvn_client/client-test.c'
Reverted 'subversion/tests/libsvn_subr/dirent_uri-test.c'
Reverted 'tools/buildbot/slaves/win32-SharpSvn'
Reverted 'tools/buildbot/slaves/win32-SharpSvn/svntest-cleanup.cmd'
Reverted 'tools/buildbot/slaves/win32-SharpSvn/svntest-test.cmd'
$ svn merge -c -931164 .
--- Reverse-merging r931164 into '.':
U .
--- Recording mergeinfo for reverse merge of r931164 into '.':
G .
$ vi subversion/libsvn_diff/parse-diff.c
$ svn diff
Property changes on: .
___________________________________________________________________
Modified: svn:mergeinfo
Reverse-merged /subversion/trunk:r931162
Index: subversion/libsvn_diff/parse-diff.c
===================================================================
--- subversion/libsvn_diff/parse-diff.c (revision 931163)
+++ subversion/libsvn_diff/parse-diff.c (working copy)
@@ -281,7 +281,7 @@
svn_stream_t *original_text;
svn_stream_t *modified_text;
svn_linenum_t original_lines;
- svn_linenum_t modified_lines;
+ svn_linenum_t i_renamed_this_variable;
svn_linenum_t leading_context;
svn_linenum_t trailing_context;
svn_boolean_t changed_line_seen;
@@ -355,12 +355,12 @@
c = line->data[0];
/* Tolerate chopped leading spaces on empty lines. */
- if (original_lines > 0 && modified_lines > 0 &&
+ if (original_lines > 0 && i_renamed_this_variable > 0 &&
(c == ' ' || (! eof && line->len == 0)))
{
hunk_seen = TRUE;
original_lines--;
- modified_lines--;
+ i_renamed_this_variable--;
if (changed_line_seen)
trailing_context++;
else
@@ -378,7 +378,7 @@
original_lines--;
}
- else if (modified_lines > 0 && c == add)
+ else if (i_renamed_this_variable > 0 && c == add)
{
hunk_seen = TRUE;
changed_line_seen = TRUE;
@@ -388,7 +388,7 @@
if (trailing_context > 0)
trailing_context = 0;
- modified_lines--;
+ i_renamed_this_variable--;
}
else
{
@@ -411,7 +411,7 @@
if (in_hunk)
{
original_lines = (*hunk)->original_length;
- modified_lines = (*hunk)->modified_length;
+ i_renamed_this_variable = (*hunk)->modified_length;
}
}
else if (starts_with(line->data, minus))
$ svn merge ^/subversion/trunk
Conflict discovered in '/home/stsp/svn/svn-svn-patch-improvements/subversion/libsvn_diff/parse-diff.c'.
Select: (p) postpone, (df) diff-full, (e) edit,
(mc) mine-conflict, (tc) theirs-conflict,
(s) show all options:
Whoops! The cyclic merge caused a conflict.
Let's see if we get a conflict with the mergeinfo change I made in r931164:
Select: (p) postpone, (df) diff-full, (e) edit,
(mc) mine-conflict, (tc) theirs-conflict,
(s) show all options: p
--- Merging r930648 through r931170 into '.':
C subversion/libsvn_diff/parse-diff.c
U subversion/libsvn_client/mergeinfo.c
U subversion/tests/libsvn_subr/dirent_uri-test.c
U subversion/tests/libsvn_client/client-test.c
U subversion/tests/cmdline/mergeinfo_tests.py
U subversion/tests/cmdline/patch_tests.py
U tools/buildbot/slaves/win32-SharpSvn/svntest-test.cmd
U tools/buildbot/slaves/win32-SharpSvn/svntest-cleanup.cmd
U tools/buildbot/slaves/win32-SharpSvn
G .
--- Recording mergeinfo for merge of r930648 through r931170 into '.':
G .
Summary of conflicts:
Text conflicts: 1
$ svn revert -R .
Reverted '.'
Reverted 'subversion/libsvn_client/mergeinfo.c'
Reverted 'subversion/libsvn_diff/parse-diff.c'
Reverted 'subversion/tests/cmdline/mergeinfo_tests.py'
Reverted 'subversion/tests/cmdline/patch_tests.py'
Reverted 'subversion/tests/libsvn_client/client-test.c'
Reverted 'subversion/tests/libsvn_subr/dirent_uri-test.c'
Reverted 'tools/buildbot/slaves/win32-SharpSvn'
Reverted 'tools/buildbot/slaves/win32-SharpSvn/svntest-cleanup.cmd'
Reverted 'tools/buildbot/slaves/win32-SharpSvn/svntest-test.cmd'
$ vi subversion/libsvn_diff/parse-diff.c
$ svn diff
Index: subversion/libsvn_diff/parse-diff.c
===================================================================
--- subversion/libsvn_diff/parse-diff.c (revision 931163)
+++ subversion/libsvn_diff/parse-diff.c (working copy)
@@ -281,7 +281,7 @@
svn_stream_t *original_text;
svn_stream_t *modified_text;
svn_linenum_t original_lines;
- svn_linenum_t modified_lines;
+ svn_linenum_t i_renamed_this_variable;
svn_linenum_t leading_context;
svn_linenum_t trailing_context;
svn_boolean_t changed_line_seen;
@@ -355,12 +355,12 @@
c = line->data[0];
/* Tolerate chopped leading spaces on empty lines. */
- if (original_lines > 0 && modified_lines > 0 &&
+ if (original_lines > 0 && i_renamed_this_variable > 0 &&
(c == ' ' || (! eof && line->len == 0)))
{
hunk_seen = TRUE;
original_lines--;
- modified_lines--;
+ i_renamed_this_variable--;
if (changed_line_seen)
trailing_context++;
else
@@ -378,7 +378,7 @@
original_lines--;
}
- else if (modified_lines > 0 && c == add)
+ else if (i_renamed_this_variable > 0 && c == add)
{
hunk_seen = TRUE;
changed_line_seen = TRUE;
@@ -388,7 +388,7 @@
if (trailing_context > 0)
trailing_context = 0;
- modified_lines--;
+ i_renamed_this_variable--;
}
else
{
@@ -411,7 +411,7 @@
if (in_hunk)
{
original_lines = (*hunk)->original_length;
- modified_lines = (*hunk)->modified_length;
+ i_renamed_this_variable = (*hunk)->modified_length;
}
}
else if (starts_with(line->data, minus))
$ svn merge ^/subversion/trunk
--- Merging r930648 through r931161 into '.':
U subversion/libsvn_client/mergeinfo.c
U subversion/tests/libsvn_subr/dirent_uri-test.c
U subversion/tests/libsvn_client/client-test.c
U subversion/tests/cmdline/mergeinfo_tests.py
U subversion/tests/cmdline/patch_tests.py
U tools/buildbot/slaves/win32-SharpSvn/svntest-test.cmd
U tools/buildbot/slaves/win32-SharpSvn/svntest-cleanup.cmd
U tools/buildbot/slaves/win32-SharpSvn
--- Recording mergeinfo for merge of r930648 through r931177 into '.':
U .
$
QED. dannas is safe.
Now, you could say that this text conflict was a mere annoyance,
and people should just answer 'mine-full' at the prompt ;-)
But in practice it really hurts, especially when the "cyclic" change
involves renames (because svn does not merge renames properly).
This gets even more complicated (and serious) when multiple merges
are involved. Try to follow this graph:
FIX (FIX merged again)
[feature1]---------------- rX -----------------------o--------
/ /sync | /sync
/ / | /
---------------------------|---------------------rA----------
\ /reint. | cherry /reint.
\ / v pick /
[release-1.x]---rN------------------rM-----------------
rA contains rX as well as rN-M, how to block just rX?
("reint." is a reintegrate merge, and the trick described in
http://svnbook.red-bean.com/nightly/en/svn.branchmerge.advanced.html#svn.branchmerge.advanced.reintegratetwice
is used to keep the release branch alive.)
In the above example, there is no easy way for users to prevent rX from
being merged cyclically while merging changes made between rN and rM.
Had they committed the rX change to trunk or to the release branch
first, there would be no problem. But in practice, people do commit things
to whichever branch they like, and ideally, Subversion should be able
to cope with that. We (elego) have seen the above situation happen.
The users ended up reverting rX and committing the change again to
trunk so that it could propagate without causing a reflective merge.
Stefan
Re: svn commit: r931164 - /subversion/branches/svn-patch-improvements/
Posted by "Hyrum K. Wright" <hy...@hyrumwright.org>.
I'm interested to know why this is needed. Is our merge tracking implementation so broken that it doesn't handle these types of things?
(Forgive me if I'm being naïve here.)
-Hyrum
On Apr 6, 2010, at 9:24 AM, stsp@apache.org wrote:
> Author: stsp
> Date: Tue Apr 6 14:24:27 2010
> New Revision: 931164
>
> URL: http://svn.apache.org/viewvc?rev=931164&view=rev
> Log:
> On the svn-patch-improvements branch, block r931162 from trunk to avoid
> a cyclic merge the next time this branch is synced to trunk.
>
> Modified:
> subversion/branches/svn-patch-improvements/ (props changed)
>
> Propchange: subversion/branches/svn-patch-improvements/
> ------------------------------------------------------------------------------
> --- svn:mergeinfo (original)
> +++ svn:mergeinfo Tue Apr 6 14:24:27 2010
> @@ -33,4 +33,4 @@
> /subversion/branches/tc_url_rev:874351-874483
> /subversion/branches/tree-conflicts:868291-873154
> /subversion/branches/tree-conflicts-notify:873926-874008
> -/subversion/trunk:918519-930647
> +/subversion/trunk:918519-930647,931162
>
>