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

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

From: Daniel Shahaf <d.s_at_daniel.shahaf.name>
Date: Sat, 17 May 2014 18:24:37 +0000

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

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