[Moving thread to dev@s.a.o from users@]
> -----Original Message-----
> From: Bert Huijben [mailto:b...@qqmail.nl]
> Sent: maandag 22 februari 2016 13:21
> To: 'Daniel Shahaf' <d...@daniel.shahaf.name>; 'Michal Matyl'
> <michal.ma...@zf.com>
> Cc: us...@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
