[PATCH] Fix failing three-way diff for properties

[Daniel Näslund]

[[[
Fix failing three way diff for properties when invoking diff-full (df)
in the interactive conflict resolutioner.

* subversion/libsvn_wc/util.c
  (svn_wc__cd2_to_cd): All the usual files needed in a three way diff is
    available for properties as well as markers for binary format and
    mime.

* subversion/svn/conflict-callbacks.c
  (svn_cl__conflict_handler): Set diff_allowed only if desc->is_binary
    is false.
]]]

It turned out that when creating svn_wc__cd2_to_cd someone forgot that
we now have the ability to do three way diffs for properties.

I changed the condition for when a diff is allowed in the conflict
handler but I wonder... If no mime-type is set is_binary is set to false
for properties. But how do I set a mime-type for a property? This may be
a bit too simple.

Another thing is that we may end up with those pescious 'No newline at
end of file' :
[[[
Conflict for property 'prop' discovered on '/tmp/branch/foo'.
Select: (p) postpone, (df) diff-full, (e) edit,
        (s) show all options: df
--- /tmp/branch/svn-VLCOnf tor okt 29 23:44:33 2009
+++ /tmp/svn-IUkBeS tor okt 29 23:44:33 2009
@@ -1,4 +1,10 @@
-A
-Whole
-Lotta
-Lines
\ No newline at end of file
+<<<<<<< (modified)
+The
+Brown
+Fox
+Jumped=======
+Some
+Other
+Randomly
+Choosen
+Words>>>>>>> (latest)
]]]

For clarity I could always add a newline to the properties when diffing.
Since they will not be used for resolution by hand no harm done. But the
markers will always be alone on one line increasing readability. Is that
ok?

I haven't run make check yet. Will do it first thing tomorrow.

/Daniel

[diff3-prop-conflict.diff]

---

Index: subversion/svn/conflict-callbacks.c
===================================================================
--- subversion/svn/conflict-callbacks.c (revision 40299)
+++ subversion/svn/conflict-callbacks.c (arbetskopia)
@@ -446,8 +446,8 @@
          markers to the user (this is the typical 3-way merge
          scenario), or if no base is available, we can show a diff
          between mine and theirs. */
-      if ((desc->merged_file && desc->base_file)
-          || (!desc->base_file && desc->my_file && desc->their_file))
+      if (! desc->is_binary && ((desc->merged_file && desc->base_file)
+          || (!desc->base_file && desc->my_file && desc->their_file)))
         diff_allowed = TRUE;
 
       while (TRUE)
Index: subversion/libsvn_wc/util.c
===================================================================
--- subversion/libsvn_wc/util.c (revision 40299)
+++ subversion/libsvn_wc/util.c (arbetskopia)
@@ -527,6 +527,17 @@
       case svn_wc_conflict_kind_property:
         new_conflict->property_name = apr_pstrdup(result_pool,
                                                   conflict->property_name);
+        new_conflict->is_binary = conflict->is_binary;
+        new_conflict->mime_type = apr_pstrdup(result_pool,
+                                              conflict->mime_type);
+        new_conflict->base_file = apr_pstrdup(result_pool,
+                                              conflict->base_file);
+        new_conflict->their_file = apr_pstrdup(result_pool,
+                                               conflict->their_file);
+        new_conflict->my_file = apr_pstrdup(result_pool,
+                                            conflict->my_file);
+        new_conflict->merged_file = apr_pstrdup(result_pool,
+                                                conflict->merged_file);
         break;
 
       case svn_wc_conflict_kind_text:

---

[Daniel Näslund]

On Thu, Oct 29, 2009 at 11:50:56PM +0100, Daniel Näslund wrote:

> [[[
> Fix failing three way diff for properties when invoking diff-full (df)
> in the interactive conflict resolutioner.
>
> * subversion/libsvn_wc/util.c
>   (svn_wc__cd2_to_cd): All the usual files needed in a three way diff is
>     available for properties as well as markers for binary format and
>     mime.
>
> * subversion/svn/conflict-callbacks.c
>   (svn_cl__conflict_handler): Set diff_allowed only if desc->is_binary
>     is false.
> ]]]

[...]
>
> I haven't run make check yet. Will do it first thing tomorrow.

 make check passed!

Still having some doubts about the way I set diff_allowed in
svn_cl__conflict_handler(). It affects text conflicts too.

/Daniel

------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2412950

[Bert Huijben-6]

> -----Original Message-----
> From: Daniel Näslund [mailto:daniel@...]
> Sent: vrijdag 30 oktober 2009 8:51
> To: dev@...
> Subject: Re: [PATCH] Fix failing three-way diff for properties
>
> On Thu, Oct 29, 2009 at 11:50:56PM +0100, Daniel Näslund wrote:
> > [[[
> > Fix failing three way diff for properties when invoking diff-full
> (df)
> > in the interactive conflict resolutioner.
> >
> > * subversion/libsvn_wc/util.c
> >   (svn_wc__cd2_to_cd): All the usual files needed in a three way diff
> is
> >     available for properties as well as markers for binary format and
> >     mime.
> >
> > * subversion/svn/conflict-callbacks.c
> >   (svn_cl__conflict_handler): Set diff_allowed only if desc-
> >is_binary
> >     is false.
> > ]]]
>
> [...]
> >
> > I haven't run make check yet. Will do it first thing tomorrow.
>
>  make check passed!
>
> Still having some doubts about the way I set diff_allowed in
> svn_cl__conflict_handler(). It affects text conflicts too.

In WC-NG we will store all versions of all conflicted properties inside the database.. so you can just handle them one at a time via the resolve apis in your own preferred order and even non interactively. (No need to create 3 * 10 .prej files if 10 properties are conflicted :)

Via the new conflict code you can see that a node is conflicted and you can then retrieve more details, like which properties with their values, if the text is conflicted and if it is tree conflicted. And you can just retrieve the data directly instead of relying on the data stored in the user part of the working copy.

This change will also allow sharing the same conflict resolve handler code for interactive and non interactive handlers.

(For backwards compatibility we will call old resolvers like we always did, but new style resolvers will see all information at once).

We still need to design some way to make all this available in the commandline client. One way would be to make 'svn resolve PATH' run the interactive conflict handler on all the remaining parts of a conflict, but another way to see the conflict-version properties in a non interactive way would be nice.

        Bert

------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2412984

[Julian Foad]

Daniel Näslund wrote:

> [[[
> Fix failing three way diff for properties when invoking diff-full (df)
> in the interactive conflict resolutioner.
>
> * subversion/libsvn_wc/util.c
>   (svn_wc__cd2_to_cd): All the usual files needed in a three way diff is
>     available for properties as well as markers for binary format and
>     mime.
>
> * subversion/svn/conflict-callbacks.c
>   (svn_cl__conflict_handler): Set diff_allowed only if desc->is_binary
>     is false.
> ]]]
>
> It turned out that when creating svn_wc__cd2_to_cd someone forgot that
> we now have the ability to do three way diffs for properties.
>
> I changed the condition for when a diff is allowed in the conflict
> handler but I wonder... If no mime-type is set is_binary is set to false
> for properties. But how do I set a mime-type for a property? This may be
> a bit too simple.

On this point, does the "is_binary" flag refer to a property or to the
versioned file's content? From what you say here, it sounds like it
refers to the versioned file's content. In that case, you cannot use it
to decide whether a property value is diffable. I think we have an API
for empirically determining whether a certain file or a certain text
contains non-text characters. It is used for automatically detecting
"binary" files during import/add. Maybe you could use it.

- Julian

------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2413054

[Daniel Näslund]

Hi Julian,

On Fri, Oct 30, 2009 at 04:39:28PM +0000, Julian Foad wrote:

> Daniel Näslund wrote:
> > [[[
> > Fix failing three way diff for properties when invoking diff-full (df)
> > in the interactive conflict resolutioner.
> >
> > * subversion/libsvn_wc/util.c
> >   (svn_wc__cd2_to_cd): All the usual files needed in a three way diff is
> >     available for properties as well as markers for binary format and
> >     mime.
> >
> > * subversion/svn/conflict-callbacks.c
> >   (svn_cl__conflict_handler): Set diff_allowed only if desc->is_binary
> >     is false.
> > ]]]

> > I changed the condition for when a diff is allowed in the conflict
> > handler but I wonder... If no mime-type is set is_binary is set to false
> > for properties. But how do I set a mime-type for a property? This may be
> > a bit too simple.
>
> On this point, does the "is_binary" flag refer to a property or to the
> versioned file's content? From what you say here, it sounds like it
> refers to the versioned file's content. In that case, you cannot use it
> to decide whether a property value is diffable.

I found this comment in svn_wc_conflict_description2_t:

[[[
  /** Whether svn thinks ('my' version of) @c path is a 'binary' file.
   *  (Only if @c kind is 'text', else undefined.) */
  svn_boolean_t is_binary;
]]]

When looking in libsvn_wc/props.c (maybe_generate_propconflict) the
is_binary flag and mime_type is handled like this:

[[[
  if (!is_dir && working_props)
    mime_propval = apr_hash_get(working_props, SVN_PROP_MIME_TYPE,
                                APR_HASH_KEY_STRING);
  cdesc->mime_type = mime_propval ? mime_propval->data : NULL;
  cdesc->is_binary = mime_propval ?
      svn_mime_type_is_binary(mime_propval->data) : FALSE;
]]]

So the mime_type and is_binary flag refers to the file the properties
belongs to. We were hoping that it would refer to the mime_type of the
property.

Why do we just look at 'mine' for mime_type? If 'theirs' is a binary
file I guess we still can't do a diff? Or is that a tree conflict? If
the mime_type of a file has changed then it's assumed that it's a replaced
file.

> I think we have an API for empirically determining whether a certain
> file or a certain text contains non-text characters. It is used for
> automatically detecting "binary" files during import/add. Maybe you
> could use it.

Found it! svn_io_detect_mimetype2() was just what I was looking for.

BUT it needs a map of mime-types. svn_client_context_t contains one of
those. How can I get a hold of that from deep inside libsvn_wc? There is
currently 18 stack-levels of merge_report_editor-driven function calls
between me and that precious ctx struct!

/Daniel

------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2413237

[Daniel Näslund]

On Fri, Oct 30, 2009 at 12:11:02PM +0100, Bert Huijben wrote:

> > > [[[
> > > Fix failing three way diff for properties when invoking diff-full
> > (df)
> > > in the interactive conflict resolutioner.
> > >
> > > * subversion/libsvn_wc/util.c
> > >   (svn_wc__cd2_to_cd): All the usual files needed in a three way diff
> > is
> > >     available for properties as well as markers for binary format and
> > >     mime.
> > >
> > > * subversion/svn/conflict-callbacks.c
> > >   (svn_cl__conflict_handler): Set diff_allowed only if desc-
> > >is_binary
> > >     is false.
> > > ]]]
> >
> > Still having some doubts about the way I set diff_allowed in
> > svn_cl__conflict_handler(). It affects text conflicts too.
>
> In WC-NG we will store all versions of all conflicted properties
> inside the database.. so you can just handle them one at a time via
> the resolve apis in your own preferred order and even non
> interactively. (No need to create 3 * 10 .prej files if 10 properties
> are conflicted :)

That's nice!

> Via the new conflict code you can see that a node is conflicted and
> you can then retrieve more details, like which properties with their
> values, if the text is conflicted and if it is tree conflicted. And
> you can just retrieve the data directly instead of relying on the data
> stored in the user part of the working copy.

A good thing. Unfortunately this leaves me with nothing left to do. I
was hoping that I could fix the mime_type and is_binary flag of
svn_wc_conflict_description_t. But I assume that the mime_type of a
property will be stored in the database and accessed through the
functions in libsvn_wc/conflicts.c. I took a peek and saw a lot of not
yet implemented functions.

> This change will also allow sharing the same conflict resolve handler
> code for interactive and non interactive handlers.
>
> (For backwards compatibility we will call old resolvers like we always
> did, but new style resolvers will see all information at once).
>
> We still need to design some way to make all this available in the
> commandline client. One way would be to make 'svn resolve PATH' run
> the interactive conflict handler on all the remaining parts of a
> conflict, but another way to see the conflict-version properties in a
> non interactive way would be nice.

What about svn di --conflict PATH?

/Daniel

------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2413262

[Julian Foad]

Daniel Näslund wrote:
> A good thing. Unfortunately this leaves me with nothing left to do. I
> was hoping that I could fix the mime_type and is_binary flag of
> svn_wc_conflict_description_t. But I assume that the mime_type of a
> property will be stored in the database and accessed through the
> functions in libsvn_wc/conflicts.c. I took a peek and saw a lot of not
> yet implemented functions.

I don't think there are any plans to store MIME types of properties.

- Julian

------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2413428

[Julian Foad]

Daniel Näslund wrote:

> I found this comment in svn_wc_conflict_description2_t:
>
> [[[
>   /** Whether svn thinks ('my' version of) @c path is a 'binary' file.
>    *  (Only if @c kind is 'text', else undefined.) */
>   svn_boolean_t is_binary;
> ]]]
>
> When looking in libsvn_wc/props.c (maybe_generate_propconflict) the
> is_binary flag and mime_type is handled like this:
>
> [[[
>   if (!is_dir && working_props)
>     mime_propval = apr_hash_get(working_props, SVN_PROP_MIME_TYPE,
>                                 APR_HASH_KEY_STRING);
>   cdesc->mime_type = mime_propval ? mime_propval->data : NULL;
>   cdesc->is_binary = mime_propval ?
>       svn_mime_type_is_binary(mime_propval->data) : FALSE;
> ]]]
>
> So the mime_type and is_binary flag refers to the file the properties
> belongs to. We were hoping that it would refer to the mime_type of the
> property.
>
> Why do we just look at 'mine' for mime_type?

Probably there is no very good reason; that's just how it was originally
written.

>  If 'theirs' is a binary file I guess we still can't do a diff? Or is
> that a tree conflict?

We still can't do a diff, true. That is not a tree conflict.

>  If the mime_type of a file has changed then it's assumed that it's a
> replaced file.

Where is that assumption coded? It is wrong.

> > I think we have an API for empirically determining whether a certain
> > file or a certain text contains non-text characters. It is used for
> > automatically detecting "binary" files during import/add. Maybe you
> > could use it.
>
> Found it! svn_io_detect_mimetype2() was just what I was looking for.
>
> BUT it needs a map of mime-types. svn_client_context_t contains one of
> those. How can I get a hold of that from deep inside libsvn_wc? There is
> currently 18 stack-levels of merge_report_editor-driven function calls
> between me and that precious ctx struct!

That function uses the map of MIME types only to recognize filename
extensions, which is irrelevant for property values. It is optional, so
just pass NULL for that parameter. The function will then just use its
byte-analysing heuristic which is what you want, I think.

- Julian

------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2413784

[Gavin Baumanis-4]

Hi,

Ping. This patch submission has received no further comments / updates.

Gavin.


On 02/11/2009, at 22:13 , Julian Foad wrote:

> Daniel Näslund wrote:
>> I found this comment in svn_wc_conflict_description2_t:
>>
>> [[[
>>  /** Whether svn thinks ('my' version of) @c path is a 'binary' file.
>>   *  (Only if @c kind is 'text', else undefined.) */
>>  svn_boolean_t is_binary;
>> ]]]
>>
>> When looking in libsvn_wc/props.c (maybe_generate_propconflict) the
>> is_binary flag and mime_type is handled like this:
>>
>> [[[
>>  if (!is_dir && working_props)
>>    mime_propval = apr_hash_get(working_props, SVN_PROP_MIME_TYPE,
>>                                APR_HASH_KEY_STRING);
>>  cdesc->mime_type = mime_propval ? mime_propval->data : NULL;
>>  cdesc->is_binary = mime_propval ?
>>      svn_mime_type_is_binary(mime_propval->data) : FALSE;
>> ]]]
>>
>> So the mime_type and is_binary flag refers to the file the properties
>> belongs to. We were hoping that it would refer to the mime_type of  
>> the
>> property.
>>
>> Why do we just look at 'mine' for mime_type?
>
> Probably there is no very good reason; that's just how it was  
> originally
> written.
>
>> If 'theirs' is a binary file I guess we still can't do a diff? Or is
>> that a tree conflict?
>
> We still can't do a diff, true. That is not a tree conflict.
>
>> If the mime_type of a file has changed then it's assumed that it's a
>> replaced file.
>
> Where is that assumption coded? It is wrong.
>
>>> I think we have an API for empirically determining whether a certain
>>> file or a certain text contains non-text characters. It is used for
>>> automatically detecting "binary" files during import/add. Maybe you
>>> could use it.
>>
>> Found it! svn_io_detect_mimetype2() was just what I was looking for.
>>
>> BUT it needs a map of mime-types. svn_client_context_t contains one  
>> of
>> those. How can I get a hold of that from deep inside libsvn_wc?  
>> There is
>> currently 18 stack-levels of merge_report_editor-driven function  
>> calls
>> between me and that precious ctx struct!
>
> That function uses the map of MIME types only to recognize filename
> extensions, which is irrelevant for property values. It is optional,  
> so
> just pass NULL for that parameter. The function will then just use its
> byte-analysing heuristic which is what you want, I think.
>
> - Julian
>
> ------------------------------------------------------
> http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2413784

------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2415368

[Daniel Näslund]

On Sat, Nov 07, 2009 at 08:07:45PM +1100, Gavin Baumanis wrote:
> Hi,
>
> Ping. This patch submission has received no further comments / updates.
>
> Gavin.
 

> > Daniel Näslund wrote:
> > [[[
> > Fix failing three way diff for properties when invoking diff-full (df)
> > in the interactive conflict resolutioner.
> >
> > * subversion/libsvn_wc/util.c
> >   (svn_wc__cd2_to_cd): All the usual files needed in a three way diff is
> >     available for properties as well as markers for binary format and
> >     mime.
> >
> > * subversion/svn/conflict-callbacks.c
> >   (svn_cl__conflict_handler): Set diff_allowed only if desc->is_binary
> >     is false.
> > ]]]

[...]

That patch didn't properly determine if a property was binary or not. It
checked if the file the property was set on was binary. I will submit a
new patch later. Discard this one.

/Daniel

------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2415882

[Gavin Baumanis-4]

Hi Daniel,

Thanks for the update.
Gavin.


On 10/11/2009, at 06:41 , Daniel Näslund wrote:

> On Sat, Nov 07, 2009 at 08:07:45PM +1100, Gavin Baumanis wrote:
>> Hi,
>>
>> Ping. This patch submission has received no further comments /  
>> updates.
>>
>> Gavin.
>
>>> Daniel Näslund wrote:
>>> [[[
>>> Fix failing three way diff for properties when invoking diff-full  
>>> (df)
>>> in the interactive conflict resolutioner.
>>>
>>> * subversion/libsvn_wc/util.c
>>>  (svn_wc__cd2_to_cd): All the usual files needed in a three way  
>>> diff is
>>>    available for properties as well as markers for binary format and
>>>    mime.
>>>
>>> * subversion/svn/conflict-callbacks.c
>>>  (svn_cl__conflict_handler): Set diff_allowed only if desc-
>>> >is_binary
>>>    is false.
>>> ]]]
>
> [...]
>
> That patch didn't properly determine if a property was binary or  
> not. It
> checked if the file the property was set on was binary. I will  
> submit a
> new patch later. Discard this one.
>
> /Daniel
>
> ------------------------------------------------------
> http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2415882

------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2415886