Three-way conflict marker for properties (*.prej files)

[Daniel Shahaf <d....@daniel.shahaf.name>]

I'm looking into enabling 3-way conflict markers for property conflicts.

Consider the following case:

In ...    the property's value is...
======    ==========================
BASE      'local'
WORKING   (deleted)
Merge-LHS 'repos'
Merge-RHS 'repos.changed'

Right now, if the code is switched to
svn_diff_conflict_display_modified_original_latest, it will generate:

    "Trying to change property 'edit.del'\n"
    "but the property has been locally deleted.\n"
    "<<<<<<< (local property value)\n"
    "||||||| (original,older diff3 version)\n"
    "local=======\n"
    "repos.changed>>>>>>> (incoming property value)\n",

Shouldn't the output in that case be:

    "Trying to change property 'edit.del'\n"
    "but the property has been locally deleted.\n"
    "<<<<<<< (local property value)\n"
    "||||||| (original,older diff3 version)\n"
    "repos=======\n"
    "repos.changed>>>>>>> (incoming property value)\n",

?

Daniel

---

That function has some interesting logic around those BASE and Merge-LHS values:

  /* Pick a suitable base for the conflict diff.
   * The incoming value is always a change,
   * but the local value might not have changed. */
  if (original == NULL)
    {
      if (incoming_base)
        original = incoming_base;
      else
        original = svn_string_create_empty(scratch_pool);
    }
  else if (incoming_base && svn_string_compare(original, mine))
    original = incoming_base;

Here, ORIGINAL is the variable that's eventually passed in the ORIGINAL (aka,
OLDER) version formal parameter of the diff3 API; however, the code sometimes
sets the variable "original" to the ORIGINAL version, and sometimes to the
INCOMING_BASE version.

I don't quite understand this.  Why does it make sense to set the variable
'original' to a different value (other than the empty string) if it is NULL?
If one of the four sides of the merge happened to be a nonexistent value, then
that (a null value) is what should be displayed, no?  i.e., the function should
either always set the variable "original" to the ORIGINAL version,
or always set that variable to the INCOMING_VERSION version.  Does that sound
right?

Daniel

Re: The 1.9 svn_wc_conflict_description3_t API (was: Three-way conflict marker for properties (*.prej files))

[Julian Foad <ju...@btopenworld.com>]

Daniel Shahaf wrote:
> Daniel Shahaf wrote on Tue, May 06, 2014 at 11:08:55 +0000:
>> I'm looking into enabling 3-way conflict markers for property 
>> conflicts.
> ...
>> That function has some interesting logic around those BASE and Merge-LHS 
>> values:
> 
> "That function" is libsvn_wc/props.c:prop_conflict_from_skel().
> 
>>   /* Pick a suitable base for the conflict diff.
>>    * The incoming value is always a change,
>>    * but the local value might not have changed. */
>>   if (original == NULL)
>>     {
>>       if (incoming_base)
>>         original = incoming_base;
>>       else
>>         original = svn_string_create_empty(scratch_pool);
>>     }
>>   else if (incoming_base && svn_string_compare(original, mine))
>>     original = incoming_base;
>> 
>> Here, ORIGINAL is the variable that's eventually passed in the ORIGINAL (aka,
>> OLDER) version formal parameter of the diff3 API; however, the code sometimes
>> sets the variable "original" to the ORIGINAL version, and sometimes to the
>> INCOMING_BASE version.
>> 
>> I don't quite understand this.  Why does it make sense to set the variable
>> 'original' to a different value (other than the empty string) if it is NULL?
>> If one of the four sides of the merge happened to be a nonexistent value, then
>> that (a null value) is what should be displayed, no?  i.e., the function should
>> either always set the variable "original" to the ORIGINAL version,
>> or always set that variable to the INCOMING_VERSION version.  Does that sound
>> right?

That sounds right ... assuming the intent is to be logical. I don't know, I suppose there were 
reasons why this behaviour was chosen, perhaps for display purposes to make the display look like some other tool, or because the author didn't like empty output blocks, or ... 
Whatever the reasons were, it looks like a case where presentation 
decisions were made in the wrong layer, and for reasons that are not apparent to us now.

> Done in r1595522.  It also affects dir_conflicts files.
> 
> Grepping for svn_diff_conflict_display_style_t uses, I found that the
> function generate_propconflict(), which underlies the interactive
> conflicts resolver API svn_wc_conflict_description3_t, provides the API
> consumer with 3 fulltext files and one pre-merged file with conflict
> markers.¹  That function, too, has logic which chooses which three out
> of the four sides of the conflict to pass into the three sides provided
> in the API.
> 
> I have two main issues with that:
> 
> 1. We are encoding information about the 4 sides of the conflict into a
> 3-sides API in an unpredictable manner: the way we map the 4 sides into
> the 3 slots varies depending on the values of the 4 sides. That means
> the API consumer receives ambiguous information (it can't tell whether
> the "common ancestor" value is the BASE value or the INCOMING_BASE_OLD 
> value).
> I think we should always map the 4 sides into the 3 slots in the same
> way (and that's what r1595522 did for the accept=postpone case).
> 
> 2. We are unnecessarily losing a side.  The
> svn_wc_conflict_description3_t API is new in 1.9; we could easily extend
> it to include all four sides, which would provide API consumers with
> more information, while still allowing them to do "only" a diff3 if 
> they
> so wish.
> 
> So, I suggest the following changes:
> 
> 1. Make svn_wc_conflict_description3_t have four "full file" members,
> rather than three right now.
> 
> 2. Make generate_propconflict() (in libsvn-wc/conflicts.c) always map
> the 4 conflict sides into the 3 sides of the svn_diff_mem_string_diff3()
> call the same way, regardless of which sides happen to be NULL --- like
> r1595522 did for the accept=postpone case.
> 
> WDYT?

Yup, +1 (without looking in detail at the code you're talking about).

Note also what I wrote [1] about these four versions not being the only, or even the most important four versions that one could be interested in. I was writing in the context of text conflicts but exactly the same applies to property conflicts. My point is just to keep in mind that the WC base version is not the only "fourth version" that can be interesting. For a cherry-pick merge, the youngest common ancestor version in the merge graph is also (and I would argue more) interesting, so we might want to extend this again at some point in the future. In an update or a 3-way merge (which includes sync and reintegrate merges) I agree the WC base version is the next-most-interesting version. Just be careful about the terminology -- "base" on its own could be confused with "merge base" if this code is shared between update and merge scenarios.

- Julian


[1] Email from Julian Foad on 2014-04-07, subject "Re: Three-way merge markers by default", <http://svn.haxx.se/dev/archive-2014-04/0084.shtml>.

The 1.9 svn_wc_conflict_description3_t API (was: Three-way conflict marker for properties (*.prej files))

[Daniel Shahaf <d....@daniel.shahaf.name>]

Daniel Shahaf wrote on Tue, May 06, 2014 at 11:08:55 +0000:
> I'm looking into enabling 3-way conflict markers for property conflicts.
...
> That function has some interesting logic around those BASE and Merge-LHS values:

"That function" is libsvn_wc/props.c:prop_conflict_from_skel().

>   /* Pick a suitable base for the conflict diff.
>    * The incoming value is always a change,
>    * but the local value might not have changed. */
>   if (original == NULL)
>     {
>       if (incoming_base)
>         original = incoming_base;
>       else
>         original = svn_string_create_empty(scratch_pool);
>     }
>   else if (incoming_base && svn_string_compare(original, mine))
>     original = incoming_base;
> 
> Here, ORIGINAL is the variable that's eventually passed in the ORIGINAL (aka,
> OLDER) version formal parameter of the diff3 API; however, the code sometimes
> sets the variable "original" to the ORIGINAL version, and sometimes to the
> INCOMING_BASE version.
> 
> I don't quite understand this.  Why does it make sense to set the variable
> 'original' to a different value (other than the empty string) if it is NULL?
> If one of the four sides of the merge happened to be a nonexistent value, then
> that (a null value) is what should be displayed, no?  i.e., the function should
> either always set the variable "original" to the ORIGINAL version,
> or always set that variable to the INCOMING_VERSION version.  Does that sound
> right?

Done in r1595522.  It also affects dir_conflicts files.

Grepping for svn_diff_conflict_display_style_t uses, I found that the
function generate_propconflict(), which underlies the interactive
conflicts resolver API svn_wc_conflict_description3_t, provides the API
consumer with 3 fulltext files and one pre-merged file with conflict
markers.�  That function, too, has logic which chooses which three out
of the four sides of the conflict to pass into the three sides provided
in the API.

I have two main issues with that:

1. We are encoding information about the 4 sides of the conflict into a
3-sides API in an unpredictable manner: the way we map the 4 sides into
the 3 slots varies depending on the values of the 4 sides. That means
the API consumer receives ambiguous information (it can't tell whether
the "common ancestor" value is the BASE value or the INCOMING_BASE_OLD value).
I think we should always map the 4 sides into the 3 slots in the same
way (and that's what r1595522 did for the accept=postpone case).

2. We are unnecessarily losing a side.  The
svn_wc_conflict_description3_t API is new in 1.9; we could easily extend
it to include all four sides, which would provide API consumers with
more information, while still allowing them to do "only" a diff3 if they
so wish.

So, I suggest the following changes:

1. Make svn_wc_conflict_description3_t have four "full file" members,
rather than three right now.

2. Make generate_propconflict() (in libsvn-wc/conflicts.c) always map
the 4 conflict sides into the 3 sides of the svn_diff_mem_string_diff3()
call the same way, regardless of which sides happen to be NULL --- like
r1595522 did for the accept=postpone case.

WDYT?

Daniel


� The cmdline client doesn't use the pre-merged file; it independently
constructs a diff3 representation from the other three, regardless of
whether the merged file uses diff3 or not.