[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

RE: (unknown)

From: Bert Huijben <bert_at_qqmail.nl>
Date: Mon, 22 Feb 2016 17:13:45 +0100

[Moving thread to dev_at_s.a.o from users@]

> -----Original Message-----
> From: Bert Huijben [mailto:bert_at_qqmail.nl]
> Sent: maandag 22 februari 2016 13:21
> To: 'Daniel Shahaf' <d.s_at_daniel.shahaf.name>; 'Michal Matyl'
> <Michal.Matyl_at_zf.com>
> Cc: users_at_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
Received on 2016-02-22 17:24:21 CET

This is an archived mail posted to the Subversion Dev mailing list.

This site is subject to the Apache Privacy Policy and the Apache Public Forum Archive Policy.