You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Bert Huijben <be...@qqmail.nl> on 2016/02/22 17:13:45 UTC
RE: (unknown)
[Moving thread to dev@s.a.o from users@]
> -----Original Message-----
> From: Bert Huijben [mailto:bert@qqmail.nl]
> Sent: maandag 22 februari 2016 13:21
> To: 'Daniel Shahaf' <d....@daniel.shahaf.name>; 'Michal Matyl'
> <Mi...@zf.com>
> Cc: users@subversion.apache.org
> Subject: RE: (unknown)
>
<snip>
> I just reviewed most of the code that is responsible for this behavior... and I
> have to conclude it works 'as designed'.
>
> 1) We determine the changes on both sides.
> * On one side we see a replacement of line 'C', by a different set of tokens.
> * And on the other side we see an insertion of a set of tokens after 'C'.
> (This code is in subversion/diff/lcs.c)
>
> 2) Then we combine the changes of both sides.
> * We can apply the first change as there are no overlapping regions
> * We can apply the second change as there no overlapping regions
> (This code is in subversion/diff/diff3.c)
>
> In our very abstract implementation things work as intended... so now we
> have to determine how we want to fix things. -> back to design phase.
>
>
> I have a working patch that determines that these changes touch each other
> and then marks them as conflict, but that still doesn't produce the kind of
> conflict that you would really want here. And I'm not sure
>
> In the optimal case we would flag one conflict containing both changes *as
> one*, but that will take more work.
>
>
>
> Note that the 'whitespace' (noted in original report) is completely unrelated
> to this issue. Our diff code works with tokens, while the whitespace is
> handled in the tokenizer. I can easily reproduce this issue without any
> whitespace changes.
I completed the patch to a form, where I like the result.
[[
Index: subversion/libsvn_diff/diff3.c
===================================================================
--- subversion/libsvn_diff/diff3.c (revision 1731660)
+++ subversion/libsvn_diff/diff3.c (working copy)
@@ -320,16 +320,17 @@ svn_diff_diff3_2(svn_diff_t **diff,
suffix_lines, subpool);
lcs_ol = svn_diff__lcs(position_list[0], position_list[2], token_counts[0],
token_counts[2], num_tokens, prefix_lines,
suffix_lines, subpool);
/* Produce a merged diff */
{
svn_diff_t **diff_ref = diff;
+ svn_diff_t *diff_last = NULL;
apr_off_t original_start = 1;
apr_off_t modified_start = 1;
apr_off_t latest_start = 1;
apr_off_t original_sync;
apr_off_t modified_sync;
apr_off_t latest_sync;
apr_off_t common_length;
@@ -429,16 +430,17 @@ svn_diff_diff3_2(svn_diff_t **diff,
is_modified = lcs_om->position[0]->offset - original_start > 0
|| lcs_om->position[1]->offset - modified_start > 0;
is_latest = lcs_ol->position[0]->offset - original_start > 0
|| lcs_ol->position[1]->offset - latest_start > 0;
if (is_modified || is_latest)
{
+ svn_boolean_t add_diff = TRUE;
modified_length = modified_sync - modified_start;
latest_length = latest_sync - latest_start;
(*diff_ref) = apr_palloc(pool, sizeof(**diff_ref));
(*diff_ref)->original_start = original_start - 1;
(*diff_ref)->original_length = original_sync - original_start;
(*diff_ref)->modified_start = modified_start - 1;
@@ -450,26 +452,47 @@ svn_diff_diff3_2(svn_diff_t **diff,
if (is_modified && is_latest)
{
svn_diff__resolve_conflict(*diff_ref,
&position_list[1],
&position_list[2],
num_tokens,
pool);
}
- else if (is_modified)
+ else if (is_modified
+ && (!diff_last
+ || diff_last->type != svn_diff__type_diff_latest))
{
(*diff_ref)->type = svn_diff__type_diff_modified;
}
- else
+ else if (is_latest
+ && (!diff_last
+ || diff_last->type != svn_diff__type_diff_modified))
{
(*diff_ref)->type = svn_diff__type_diff_latest;
}
+ else
+ {
+ /* We have a latest and a modified region that touch each other,
+ but not directly change the same location. Create a single
+ conflict region to properly mark a conflict, and to ease
+ resolving. */
+ diff_last->type = svn_diff__type_conflict;
+ diff_last->original_length += (*diff_ref)->original_length;
+ diff_last->modified_length += (*diff_ref)->modified_length;
+ diff_last->latest_length += (*diff_ref)->latest_length;
- diff_ref = &(*diff_ref)->next;
+ add_diff = FALSE;
+ }
+
+ if (add_diff)
+ {
+ diff_last = *diff_ref;
+ diff_ref = &(*diff_ref)->next;
+ }
}
/* Detect EOF */
if (lcs_om->length == 0 || lcs_ol->length == 0)
break;
modified_length = lcs_om->length
- (original_sync - lcs_om->position[0]->offset);
@@ -485,16 +508,17 @@ svn_diff_diff3_2(svn_diff_t **diff,
(*diff_ref)->original_start = original_sync - 1;
(*diff_ref)->original_length = common_length;
(*diff_ref)->modified_start = modified_sync - 1;
(*diff_ref)->modified_length = common_length;
(*diff_ref)->latest_start = latest_sync - 1;
(*diff_ref)->latest_length = common_length;
(*diff_ref)->resolved_diff = NULL;
+ diff_last = *diff_ref;
diff_ref = &(*diff_ref)->next;
}
/* Set the new offsets */
original_start = original_sync + common_length;
modified_start = modified_sync + common_length;
latest_start = latest_sync + common_length;
Index: subversion/tests/libsvn_diff/diff-diff3-test.c
===================================================================
--- subversion/tests/libsvn_diff/diff-diff3-test.c (revision 1731660)
+++ subversion/tests/libsvn_diff/diff-diff3-test.c (working copy)
@@ -2991,30 +2991,32 @@ three_way_double_add(apr_pool_t *pool)
will be a PASS. */
"A\n"
"B\n"
"<<<<<<< doubleadd2\n"
"C\n"
"D\n" /* New line 1a */
"E\n" /* New line 2a */
"F\n" /* New line 3a*/
+ "||||||| doubleadd1\n"
+ "C\n"
"=======\n"
"O\n"
"P\n" /* New line 1b */
"Q\n" /* New line 2b */
"R\n" /* New line 3b */
">>>>>>> doubleadd3\n"
"J\n"
"K\n"
"L",
NULL,
svn_diff_conflict_display_modified_original_latest,
pool));
- SVN_ERR(three_way_merge("doubleadd1", "doubleadd2", "doubleadd3",
+ SVN_ERR(three_way_merge("doubleadd4", "doubleadd5", "doubleadd6",
"A\n"
"B\n"
"C\n"
"J\n"
"K\n"
"L",
"A\n"
@@ -3039,28 +3041,31 @@ three_way_double_add(apr_pool_t *pool)
/* With s/C/O/ we expect something like this,
but the current (1.9/trunk) result is a
succeeded merge to a combined result.
### I'm guessing this result needs tweaks before it
will be a PASS. */
"A\n"
"B\n"
- "<<<<<<< doubleadd2\n"
+ "<<<<<<< doubleadd5\n"
"C\n"
"D\n" /* New line 1a */
"E\n" /* New line 2a */
"F\n" /* New line 3a*/
+ "||||||| doubleadd4\n"
+ "C\n"
+ "J\n"
"=======\n"
"O\n"
"P\n" /* New line 1b */
"Q\n" /* New line 2b */
"R\n" /* New line 3b */
"J\n"
- ">>>>>>> doubleadd3\n"
+ ">>>>>>> doubleadd6\n"
"K\n"
"L",
NULL,
svn_diff_conflict_display_modified_original_latest,
pool));
return SVN_NO_ERROR;
}
@@ -3102,14 +3107,14 @@ static struct svn_test_descriptor_t test_funcs[] =
SVN_TEST_PASS2(test_identical_suffix,
"identical suffix starts at the boundary of a chunk"),
SVN_TEST_PASS2(test_token_compare,
"compare tokens at the chunk boundary"),
SVN_TEST_PASS2(two_way_issue_3362_v1,
"2-way issue #3362 test v1"),
SVN_TEST_PASS2(two_way_issue_3362_v2,
"2-way issue #3362 test v2"),
- SVN_TEST_XFAIL2(three_way_double_add,
+ SVN_TEST_PASS2(three_way_double_add,
"3-way merge, double add"),
SVN_TEST_NULL
};
SVN_TEST_MAIN
]]
But the result triggered an interesting regression test failure, with an even more interesting comment
/* Merge is more "aggressive" about resolving conflicts than traditional
* patch or diff3. Some people consider this behaviour to be a bug, see
* http://subversion.tigris.org/servlets/ReadMsg?list=dev&msgNo=35014
*/
This comment was added in 2003 after the discussion on that url.
I really don't know what we should do here now.
Personally I agree that I would like to see a text-conflict raised in this specific case where the change regions touch each other. But then there is that old discussion, ....
Bert
RE: merging adjacent changes
Posted by Michal Matyl <Mi...@zf.com>.
Hello,
Thanks for paying attention to this. Unfortunately I can't submit new item in SVN's JIRA. Apart from discussion raised by my example can someone please fill an issue for this ? It's already identified by Daniel as a bug. Per his suggestion I have also rewritten the example into Win batch script.
Regards
Michal
-----Original Message-----
From: Philip Martin [mailto:philip.martin@wandisco.com]
Sent: Monday, February 22, 2016 6:24 PM
To: Bert Huijben
Cc: 'Daniel Shahaf'; 'Michal Matyl'; dev@subversion.apache.org; jcorvel@apache.org; philip@apache.org
Subject: merging adjacent changes
"Bert Huijben" <be...@qqmail.nl> writes:
> Personally I agree that I would like to see a text-conflict raised in
> this specific case where the change regions touch each other. But then
> there is that old discussion, ....
I added the test because we had already, perhaps inadvertently, implemented the behaviour.
Back in 2003 we didn't have 'svn resolve' so resolving a text conflict required the user to edit the file. Now we have 'svn resolved' which may make resolving text conflicts easier. Perhaps that allows us to change Subversion to create more conflicts?
We know merge is essentially imperfect or fuzzy, the question is where to draw the line. Which, if any, of these cases should merge without conflict and what should the merged result be?
1. common first second
ancestor branch branch
A A A
B Bbr1 B
C C Cbr2
D D D
2. common first second
ancestor branch branch
A A A
B Bbr1 B
C new new
D C C
D D
3. common first second
ancestor branch branch
A A A
B Bbr1 B
C new new
D C Cbr2
D D
4. common first second
ancestor branch branch
A A A
B Bbr1 B
C newbr1 newbr2
D C Cbr2
D D
There are probably more cases.
--
Philip Martin
WANdisco
merging adjacent changes
Posted by Philip Martin <ph...@wandisco.com>.
"Bert Huijben" <be...@qqmail.nl> writes:
> Personally I agree that I would like to see a text-conflict raised in
> this specific case where the change regions touch each other. But then
> there is that old discussion, ....
I added the test because we had already, perhaps inadvertently,
implemented the behaviour.
Back in 2003 we didn't have 'svn resolve' so resolving a text conflict
required the user to edit the file. Now we have 'svn resolved' which
may make resolving text conflicts easier. Perhaps that allows us to
change Subversion to create more conflicts?
We know merge is essentially imperfect or fuzzy, the question is where
to draw the line. Which, if any, of these cases should merge without
conflict and what should the merged result be?
1. common first second
ancestor branch branch
A A A
B Bbr1 B
C C Cbr2
D D D
2. common first second
ancestor branch branch
A A A
B Bbr1 B
C new new
D C C
D D
3. common first second
ancestor branch branch
A A A
B Bbr1 B
C new new
D C Cbr2
D D
4. common first second
ancestor branch branch
A A A
B Bbr1 B
C newbr1 newbr2
D C Cbr2
D D
There are probably more cases.
--
Philip Martin
WANdisco