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
Received on 2010-04-06 17:27:58 CEST