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.
Received on 2014-05-17 21:00:19 CEST