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.
Index: subversion/libsvn_wc/conflicts.c
===================================================================
--- subversion/libsvn_wc/conflicts.c (revision 1595523)
+++ subversion/libsvn_wc/conflicts.c (working copy)
@@ -1342,6 +1342,18 @@ generate_propconflict(svn_boolean_t *conflict_rema
whichever older-value happens to be defined, so that the
conflict-callback can still attempt a 3-way merge. */
+ /* ### Why are we choosing "whichever value happens to exist"?
+ ### If Alice changes "cat" to "dog" and Bob adds "green", we'd pass "cat" as the common ancestor;
+ ### If Alice adds "dog" and Bob changes "red" to "green", we'd pass "red" as the common ancestor.
+ ### API consumers can't tell the difference between these two situations.
+ ###
+ ### Right now, we are arbitrarily/lossily coercing a 4-way conflict into a 3-way one.
+ ### Instead, we should either be passing all four versions to the API consumer and let them sort it out,
+ ### or clearly clarify whether it is WORKING_VAL or INCOMING_NEW_VAL that CDESC->BASE_ABSPATH is associated with.
+ ###
+ ### I vote for the former (with clear documentation about 4-way conflicts, to help API consumers get it right).
+ */
+
const svn_string_t *conflict_base_val = base_val ? base_val
: incoming_old_val;
const char *file_name;
@@ -1375,14 +1387,26 @@ generate_propconflict(svn_boolean_t *conflict_rema
compare. */
if (working_val && svn_string_compare(base_val, working_val))
+ /* ### Why the condition on working_val != NULL?
+ ### NULL is a legitimate value here; even if one of the four values
+ ### is NULL, that's no reason to prefer one of the other values over it. */
conflict_base_val = incoming_old_val;
else
+ /* ### Here, would be better to just set CONFLICT_BASE_VAL to INCOMING_OLD_VAL.
+ ### (which allows folding both this if/else and the if/else containing it
+ ### into the single line "conflict_base_val = incoming_old_val;"),
+ ### since BASE_VAL is already known to the user and already available
+ ### via the BASE tree. */
conflict_base_val = base_val;
}
else
{
conflict_base_val = base_val;
}
+#if 0
+ /* ### I suggest to replace the above if/else by: */
+ conflict_base_val = base_val;
+#endif
SVN_ERR(svn_io_write_unique(&file_name, dirpath, conflict_base_val->data,
conflict_base_val->len,
@@ -1406,7 +1430,9 @@ generate_propconflict(svn_boolean_t *conflict_rema
SVN_ERR(svn_diff_mem_string_output_merge2(mergestream, diff,
conflict_base_val, working_val,
incoming_new_val, NULL, NULL, NULL, NULL,
- svn_diff_conflict_display_modified_latest, scratch_pool));
+ /* ### The effect of this can't be seen from the command-line client,
+ ### but prop_tests.py 36 triggers this codepath. */
+ svn_diff_conflict_display_modified_original_latest, scratch_pool));
SVN_ERR(svn_stream_close(mergestream));
}
}
