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>.
Received on 2014-05-19 19:18:06 CEST