subversion-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Daniel Shahaf <...@daniel.shahaf.name>
Subject The 1.9 svn_wc_conflict_description3_t API (was: Three-way conflict marker for properties (*.prej files))
Date Sat, 17 May 2014 18:24:37 GMT
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.

Mime
View raw message