Mailing-List: contact dev-help@subversion.apache.org; run by ezmlm
Precedence: bulk
Received-SPF: pass (athena.apache.org: local policy)
Date: Sat, 17 May 2014 18:24:37 +0000
From: Daniel Shahaf <d.s@daniel.shahaf.name>
To: dev@subversion.apache.org
Subject: The 1.9 svn_wc_conflict_description3_t API (was: Three-way conflict
 marker for properties (*.prej files))
Message-ID: <20140517182437.GA3471@tarsus.local2>
References: <20140506110855.GA1948@tarsus.local2>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="ReaqsoxgOBHFXBhH"
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20140506110855.GA1948@tarsus.local2>
User-Agent: Mutt/1.5.21 (2010-09-15)


--ReaqsoxgOBHFXBhH
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit

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.

--ReaqsoxgOBHFXBhH
Content-Type: text/x-patch; charset=us-ascii
Content-Disposition: attachment; filename="conflicts-api-questions.diff"

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));
         }
     }

--ReaqsoxgOBHFXBhH--