You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Daniel Näslund <da...@longitudo.com> on 2009/11/10 18:03:50 UTC
[PATCH v2] Fix failing three way diff for properties
[[[
Fix failing three way diff for properties when invoking diff-full (df)
in the interactive conflict resolver.
* subversion/libsvn_wc/util.c
(svn_wc__cd2_to_cd): All the usual files needed in a three way diff is
made available for properties as well as markers for binary format
and mime.
]]]
After this patch a binary property will be displayed when invoking
diff-full. I intend to fix this in a subsequent patch. Perhaps adding
newlines when there is none to enhance readability. Since we're not
using the diff for manually resolving the conflict.
The three way diffs for the propties are sotred in .prej files in this
format:
[[[
Trying to change property 'prop' from 'dog' to 'horse',
but the property has been locally changed from 'dog' to 'cat'.
]]]
Bert wrote in an earlier mail [1] that in WC-NG there will be no .prej files
but that the information will be stored in the database. That way we can
view the three-way diff any time we want and won't have to resort to
those pescious .prej files.
He continues:
[[[
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.
]]]
Is a suboption to svn di a good way to view the conflict in a
non-interactively way?
/Daniel
[1] http://svn.haxx.se/dev/archive-2009-10/0816.shtml
Re: [PATCH v3] Fix failing three way diff for properties
Posted by Stefan Sperling <st...@elego.de>.
On Tue, Nov 10, 2009 at 09:02:59PM +0000, Julian Foad wrote:
> On Tue, 2009-11-10 at 20:50 +0100, Daniel Näslund wrote:
> > Forgot again! Make check passed.
>
> Great. We tend to assume that "make check" passed whenever you post a
> patch, so please continue to remember to run the check.
>
> One more question: Are you sure the fields you are duplicating are not
> allowed to be NULL? If they are, then I think the apr_pstrdup() is not
> guaranteed to succeed and so you would have to do "thing ?
> apr_pstrdup(...) : NULL" or similar.
>
> I haven't looked for the answer, I'm just asking you.
Just a note:
Quick testing shows that the patch does what it should,
a proper diff is printed when choosing 'df' (diff-full)
during interactive conflict resolution on a property conflict.
And I'd also like to know the answer to Julian's question :)
Stefan
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2416320
Re: [PATCH v3] Fix failing three way diff for properties
Posted by Julian Foad <ju...@btopenworld.com>.
Daniel Näslund wrote:
> On Wed, Nov 11, 2009 at 06:53:52PM +0100, Daniel Näslund wrote:
> >
> > [[[
> > Fix failing three way diff for properties when invoking diff-full (df)
> > in the interactive conflict resolver.
> >
> > * subversion/libsvn_wc/util.c
> > (svn_wc__cd2_to_cd, svn_wc__cd_to_cd2): All the usual files needed
> > in a three way diff is made available for properties as well as
> > markers for binary format and mime.
> >
> > Patch by: Daniel Näslund <daniel{_AT_}longitudo.com>
> > Suggested by: julianf
> > ]]]
>
> Double doh, forgot to update cd2_to_cd(). Fixed.
Committed revision 40469. I took the liberty of editing the log message.
Thanks!
- Julian
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2416704
Re: [PATCH v3] Fix failing three way diff for properties
Posted by Daniel Näslund <da...@longitudo.com>.
On Wed, Nov 11, 2009 at 06:53:52PM +0100, Daniel Näslund wrote:
>
> [[[
> Fix failing three way diff for properties when invoking diff-full (df)
> in the interactive conflict resolver.
>
> * subversion/libsvn_wc/util.c
> (svn_wc__cd2_to_cd, svn_wc__cd_to_cd2): All the usual files needed
> in a three way diff is made available for properties as well as
> markers for binary format and mime.
>
> Patch by: Daniel Näslund <daniel{_AT_}longitudo.com>
> Suggested by: julianf
> ]]]
Double doh, forgot to update cd2_to_cd(). Fixed.
Re: [PATCH v3] Fix failing three way diff for properties
Posted by Julian Foad <ju...@btopenworld.com>.
On Wed, 2009-11-11 at 19:34 +0100, Stefan Sperling wrote:
> On Wed, Nov 11, 2009 at 06:53:52PM +0100, Daniel Näslund wrote:
> > Formatting of long lines can be difficult. Is this ok?
> >
> > [[[
> > new_conflict->mime_type = conflict->mime_type
> > ? apr_pstrdup(result_pool, conflict->mime_type)
> > : NULL;
> > ]]]
>
> I'd mildly prefer:
>
> [[[
> new_conflict->mime_type = conflict->mime_type ? apr_pstrdup(
> result_pool,
> conflict->mime_type)
> : NULL;
> ]]]
I prefer an olive shade of green:
> new_conflict->mime_type
> = conflict->mime_type ? apr_pstrdup(conflict->mime_type) : NULL;
- Julian
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2416688
Re: [PATCH v3] Fix failing three way diff for properties
Posted by Stefan Sperling <st...@elego.de>.
On Wed, Nov 11, 2009 at 06:53:52PM +0100, Daniel Näslund wrote:
> Formatting of long lines can be difficult. Is this ok?
>
> [[[
> new_conflict->mime_type = conflict->mime_type
> ? apr_pstrdup(result_pool, conflict->mime_type)
> : NULL;
> ]]]
I'd mildly prefer:
[[[
new_conflict->mime_type = conflict->mime_type ? apr_pstrdup(
result_pool,
conflict->mime_type)
: NULL;
]]]
Stefan
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2416683
Re: [PATCH v3] Fix failing three way diff for properties
Posted by Daniel Näslund <da...@longitudo.com>.
[[[
Fix failing three way diff for properties when invoking diff-full (df)
in the interactive conflict resolver.
* subversion/libsvn_wc/util.c
(svn_wc__cd2_to_cd, svn_wc__cd_to_cd2): All the usual files needed
in a three way diff is made available for properties as well as
markers for binary format and mime.
Patch by: Daniel Näslund <daniel{_AT_}longitudo.com>
Suggested by: julianf
]]]
On Tue, Nov 10, 2009 at 09:02:59PM +0000, Julian Foad wrote:
> One more question: Are you sure the fields you are duplicating are not
> allowed to be NULL? If they are, then I think the apr_pstrdup() is not
> guaranteed to succeed and so you would have to do "thing ?
> apr_pstrdup(...) : NULL" or similar.
The mime type is not guaranteed to be non NULL so I've changed that.
It was not guarenteed to be non NULL for text only either so that was an
existing bug. merged_file may be NULL for properties if one of the
properties is binary. I've changed the assignment there.
Right now it's possible to inoke an interactive diff on binary
properties. I will fix this in a subsequent patch.
I've checked libsvn_wc/props.c and it's safe there. But as I see it, I
only need to check the declaration of the structs. It's up to the rest
of the code to obey the contract the declaration dictates. Else I will
have to chase down every possible code path.
I have only added checks for NULL so I haven't run any new tests.
Formatting of long lines can be difficult. Is this ok?
[[[
new_conflict->mime_type = conflict->mime_type
? apr_pstrdup(result_pool, conflict->mime_type)
: NULL;
]]]
/Daniel
Re: [PATCH v3] Fix failing three way diff for properties
Posted by Julian Foad <ju...@btopenworld.com>.
On Tue, 2009-11-10 at 20:50 +0100, Daniel Näslund wrote:
> Forgot again! Make check passed.
Great. We tend to assume that "make check" passed whenever you post a
patch, so please continue to remember to run the check.
One more question: Are you sure the fields you are duplicating are not
allowed to be NULL? If they are, then I think the apr_pstrdup() is not
guaranteed to succeed and so you would have to do "thing ?
apr_pstrdup(...) : NULL" or similar.
I haven't looked for the answer, I'm just asking you.
- Julian
> On Tue, Nov 10, 2009 at 07:38:01PM +0100, Daniel Näslund wrote:
> > On Tue, Nov 10, 2009 at 06:12:52PM +0000, Julian Foad wrote:
> > > Daniel Näslund wrote:
> > > > [[[
> > > > Fix failing three way diff for properties when invoking diff-full (df)
> > > > in the interactive conflict resolver.
> > > >
> > > > * subversion/libsvn_wc/util.c
> > > > (svn_wc__cd2_to_cd, svn_wc__cd_to_cd2): All the usual files needed
> > > > in a three way diff is made available for properties as well as
> > > > markers for binary format and mime.
> > >
> > > Please can you fix svn_wc__cd_to_cd2() in the same way at the same time.
> > >
> > > In the implementation, it looks like the code is identical for case
> > > svn_wc_conflict_kind_text and case svn_wc_conflict_kind_property, so
> > > please can you share it rather than duplicating it.
> >
> > Fixed
> > /Daniel
>
> > Index: subversion/libsvn_wc/util.c
> > ===================================================================
> > --- subversion/libsvn_wc/util.c (revision 40442)
> > +++ subversion/libsvn_wc/util.c (arbetskopia)
> > @@ -527,7 +527,7 @@
> > case svn_wc_conflict_kind_property:
> > new_conflict->property_name = apr_pstrdup(result_pool,
> > conflict->property_name);
> > - break;
> > + /* Falling through. */
> >
> > case svn_wc_conflict_kind_text:
> > new_conflict->is_binary = conflict->is_binary;
> > @@ -585,7 +585,7 @@
> > case svn_wc_conflict_kind_property:
> > new_conflict->property_name = apr_pstrdup(result_pool,
> > conflict->property_name);
> > - break;
> > + /* Falling through. */
> >
> > case svn_wc_conflict_kind_text:
> > new_conflict->is_binary = conflict->is_binary;
>
> ------------------------------------------------------
> http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2416274
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2416281
Re: [PATCH v3] Fix failing three way diff for properties
Posted by Daniel Näslund <da...@longitudo.com>.
Forgot again! Make check passed.
On Tue, Nov 10, 2009 at 07:38:01PM +0100, Daniel Näslund wrote:
> On Tue, Nov 10, 2009 at 06:12:52PM +0000, Julian Foad wrote:
> > Daniel Näslund wrote:
> > > [[[
> > > Fix failing three way diff for properties when invoking diff-full (df)
> > > in the interactive conflict resolver.
> > >
> > > * subversion/libsvn_wc/util.c
> > > (svn_wc__cd2_to_cd, svn_wc__cd_to_cd2): All the usual files needed
> > > in a three way diff is made available for properties as well as
> > > markers for binary format and mime.
> >
> > Please can you fix svn_wc__cd_to_cd2() in the same way at the same time.
> >
> > In the implementation, it looks like the code is identical for case
> > svn_wc_conflict_kind_text and case svn_wc_conflict_kind_property, so
> > please can you share it rather than duplicating it.
>
> Fixed
>
> /Daniel
> Index: subversion/libsvn_wc/util.c
> ===================================================================
> --- subversion/libsvn_wc/util.c (revision 40442)
> +++ subversion/libsvn_wc/util.c (arbetskopia)
> @@ -527,7 +527,7 @@
> case svn_wc_conflict_kind_property:
> new_conflict->property_name = apr_pstrdup(result_pool,
> conflict->property_name);
> - break;
> + /* Falling through. */
>
> case svn_wc_conflict_kind_text:
> new_conflict->is_binary = conflict->is_binary;
> @@ -585,7 +585,7 @@
> case svn_wc_conflict_kind_property:
> new_conflict->property_name = apr_pstrdup(result_pool,
> conflict->property_name);
> - break;
> + /* Falling through. */
>
> case svn_wc_conflict_kind_text:
> new_conflict->is_binary = conflict->is_binary;
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2416274
Re: [PATCH v3] Fix failing three way diff for properties
Posted by Daniel Näslund <da...@longitudo.com>.
On Tue, Nov 10, 2009 at 06:12:52PM +0000, Julian Foad wrote:
> Daniel Näslund wrote:
> > [[[
> > Fix failing three way diff for properties when invoking diff-full (df)
> > in the interactive conflict resolver.
> >
> > * subversion/libsvn_wc/util.c
> > (svn_wc__cd2_to_cd, svn_wc__cd_to_cd2): All the usual files needed
> > in a three way diff is made available for properties as well as
> > markers for binary format and mime.
>
> Please can you fix svn_wc__cd_to_cd2() in the same way at the same time.
>
> In the implementation, it looks like the code is identical for case
> svn_wc_conflict_kind_text and case svn_wc_conflict_kind_property, so
> please can you share it rather than duplicating it.
Fixed
/Daniel
Re: [PATCH v2] Fix failing three way diff for properties
Posted by Julian Foad <ju...@wandisco.com>.
Daniel Näslund wrote:
> [[[
> Fix failing three way diff for properties when invoking diff-full (df)
> in the interactive conflict resolver.
>
> * subversion/libsvn_wc/util.c
> (svn_wc__cd2_to_cd): All the usual files needed in a three way diff is
> made available for properties as well as markers for binary format
> and mime.
Please can you fix svn_wc__cd_to_cd2() in the same way at the same time.
In the implementation, it looks like the code is identical for case
svn_wc_conflict_kind_text and case svn_wc_conflict_kind_property, so
please can you share it rather than duplicating it.
- Julian
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2416253