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 Shahaf <d....@daniel.shahaf.name> on 2011/06/15 05:33:46 UTC
Re: svn commit: r1135635 -
/subversion/trunk/subversion/libsvn_wc/props.c
Could we just print a hex dump of the value?
stsp@apache.org wrote on Tue, Jun 14, 2011 at 15:10:50 -0000:
> Author: stsp
> Date: Tue Jun 14 15:10:50 2011
> New Revision: 1135635
>
> URL: http://svn.apache.org/viewvc?rev=1135635&view=rev
> Log:
> * subversion/libsvn_wc/props.c
> (prop_conflict_from_skel): If any property value involved in the conflict
> is binary data do not generate a conflict diff. Also refrain from writing
> binary data to the reject file, and print a readable placeholder instead.
>
> Modified:
> subversion/trunk/subversion/libsvn_wc/props.c
>
> Modified: subversion/trunk/subversion/libsvn_wc/props.c
> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_wc/props.c?rev=1135635&r1=1135634&r2=1135635&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/libsvn_wc/props.c (original)
> +++ subversion/trunk/subversion/libsvn_wc/props.c Tue Jun 14 15:10:50 2011
> @@ -569,6 +569,9 @@ prop_conflict_from_skel(const svn_string
> svn_diff_t *diff;
> svn_diff_file_options_t *diff_opts;
> svn_stringbuf_t *buf;
> + svn_boolean_t original_is_binary;
> + svn_boolean_t mine_is_binary;
> + svn_boolean_t incoming_is_binary;
>
> /* Navigate to the property name. */
> skel = skel->children->next;
> @@ -602,47 +605,64 @@ prop_conflict_from_skel(const svn_string
> else if (incoming_base && svn_string_compare(original, mine))
> original = incoming_base;
>
> - /* ### TODO Do not attempt to generate diffs of binary data. */
> - diff_opts = svn_diff_file_options_create(scratch_pool);
> - diff_opts->ignore_space = FALSE;
> - diff_opts->ignore_eol_style = FALSE;
> - diff_opts->show_c_function = FALSE;
> - SVN_ERR(svn_diff_mem_string_diff3(&diff, original, mine, incoming,
> - diff_opts, scratch_pool));
> - if (svn_diff_contains_conflicts(diff))
> - {
> - svn_stream_t *stream;
> - svn_diff_conflict_display_style_t style;
> - const char *mine_marker = _("<<<<<<< (local property value)");
> - const char *incoming_marker = _(">>>>>>> (incoming property value)");
> - const char *separator = "=======";
> -
> - style = svn_diff_conflict_display_modified_latest;
> - stream = svn_stream_from_stringbuf(buf, scratch_pool);
> - SVN_ERR(svn_stream_skip(stream, buf->len));
> - SVN_ERR(svn_diff_mem_string_output_merge2(stream, diff,
> - original, mine, incoming,
> - NULL, mine_marker,
> - incoming_marker, separator,
> - style, scratch_pool));
> - SVN_ERR(svn_stream_close(stream));
> + /* If any of the property values involved in the diff is binary data,
> + * do not generate a diff. */
> + original_is_binary = svn_io_is_binary_data(original->data, original->len);
> + mine_is_binary = svn_io_is_binary_data(mine->data, mine->len);
> + incoming_is_binary = svn_io_is_binary_data(incoming->data, incoming->len);
> +
> + if (!(original_is_binary || mine_is_binary || incoming_is_binary))
> + {
> + diff_opts = svn_diff_file_options_create(scratch_pool);
> + diff_opts->ignore_space = FALSE;
> + diff_opts->ignore_eol_style = FALSE;
> + diff_opts->show_c_function = FALSE;
> + SVN_ERR(svn_diff_mem_string_diff3(&diff, original, mine, incoming,
> + diff_opts, scratch_pool));
> + if (svn_diff_contains_conflicts(diff))
> + {
> + svn_stream_t *stream;
> + svn_diff_conflict_display_style_t style;
> + const char *mine_marker = _("<<<<<<< (local property value)");
> + const char *incoming_marker = _(">>>>>>> (incoming property value)");
> + const char *separator = "=======";
> +
> + style = svn_diff_conflict_display_modified_latest;
> + stream = svn_stream_from_stringbuf(buf, scratch_pool);
> + SVN_ERR(svn_stream_skip(stream, buf->len));
> + SVN_ERR(svn_diff_mem_string_output_merge2(stream, diff,
> + original, mine, incoming,
> + NULL, mine_marker,
> + incoming_marker, separator,
> + style, scratch_pool));
> + SVN_ERR(svn_stream_close(stream));
> +
> + *conflict_desc = svn_string_create_from_buf(buf, result_pool);
> + return SVN_NO_ERROR;
> + }
> }
> - else
> +
> + /* If we could not print a conflict diff just print full values . */
> + if (mine->len > 0)
> {
> - /* If we cannot print a conflict diff just print full values . */
> - if (mine->len > 0)
> - {
> - svn_stringbuf_appendcstr(buf, _("Local property value:\n"));
> - svn_stringbuf_appendbytes(buf, mine->data, mine->len);
> - svn_stringbuf_appendcstr(buf, "\n");
> - }
> + svn_stringbuf_appendcstr(buf, _("Local property value:\n"));
> + if (mine_is_binary)
> + svn_stringbuf_appendcstr(buf, _("Cannot display: property value is "
> + "binary data\n"));
> + else
> + svn_stringbuf_appendbytes(buf, mine->data, mine->len);
> + svn_stringbuf_appendcstr(buf, "\n");
> + }
>
> - if (incoming->len > 0)
> - {
> - svn_stringbuf_appendcstr(buf, _("Incoming property value:\n"));
> - svn_stringbuf_appendbytes(buf, incoming->data, incoming->len);
> - svn_stringbuf_appendcstr(buf, "\n");
> - }
> + if (incoming->len > 0)
> + {
> + svn_stringbuf_appendcstr(buf, _("Incoming property value:\n"));
> + if (incoming_is_binary)
> + svn_stringbuf_appendcstr(buf, _("Cannot display: property value is "
> + "binary data\n"));
> + else
> + svn_stringbuf_appendbytes(buf, incoming->data, incoming->len);
> + svn_stringbuf_appendcstr(buf, "\n");
> }
>
> *conflict_desc = svn_string_create_from_buf(buf, result_pool);
>
>
Re: svn commit: r1135635 -
/subversion/trunk/subversion/libsvn_wc/props.c
Posted by Stefan Sperling <st...@elego.de>.
On Wed, Jun 15, 2011 at 12:22:13PM +0200, Bert Huijben wrote:
> The problem for application builders is that you can't just get the property
> here.
>
> You can get the new BASE version of the property and the new ACTUAL version
> of the property (which is usually the old actual property on property
> conflicts), but you can't access the other property values (read: ORIGINAL)
> as you don't know the old revision. (Unless you captured it somewhere before
> the operation)
>
I see, but how is this problem solved by putting a hexdump into
the prej file?
I think that property conflicts aren't being represented very well even
in internal data structures. E.g. in svn_wc_conflict_description2_t some
unrelated field is being used to store the path to the prej file (a
comment added by you points this out).
> The new conflict store is designed to fix this problem, but that will have
> to wait for 1.8.
I also think it is too late now to change any of this for 1.7.
But it needs some serious work for 1.8.
RE: svn commit: r1135635 - /subversion/trunk/subversion/libsvn_wc/props.c
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: Stefan Sperling [mailto:stsp@elego.de]
> Sent: woensdag 15 juni 2011 12:12
> To: dev@subversion.apache.org
> Subject: Re: svn commit: r1135635 -
> /subversion/trunk/subversion/libsvn_wc/props.c
>
> On Wed, Jun 15, 2011 at 06:55:19AM +0300, Daniel Shahaf wrote:
> > Daniel Shahaf wrote on Wed, Jun 15, 2011 at 06:33:46 +0300:
> > > stsp@apache.org wrote on Tue, Jun 14, 2011 at 15:10:50 -0000:
> > > > + svn_stringbuf_appendcstr(buf, _("Local property value:\n"));
> > > > + if (mine_is_binary)
> > > > + svn_stringbuf_appendcstr(buf, _("Cannot display: property
value
> is "
> > > > + "binary data\n"));
> > >
> > > Could we just print a hex dump of the value?
> >
> > Concretely:
> >
> > [[[
> > but the property has already been locally changed to a different value.
> > Local property value:
> > mine
> > Incoming property value:
> >
> 0x7E454B4602010100000000000000000002003D0001000000A014400000000000
> 40000000000000
> > ]]]
>
> While I understand your reasoning I think this would be very
> confusing to many users. Not all SVN users are programmers.
>
> And I guess people will in general be using special tools anyway to view
> such properties. E.g. if the prop value was a PNG image, they will want
> to see the image. A string of hex numbers won't help them much.
>
> There is precedence here, too. We don't show diffs of binary files,
> we show a placeholder instead that says a diff cannot be shown.
> Would you also prefer svn diff to print a diff of the hex dumps?
> I'd doubt it.
>
> I'd say let them propget the value and look at it in whatever manner
> they see fit. This is a total edge case anyway. No user using standard
> svn: properties is ever going to run into this.
>
> > Patch to produce that:
The problem for application builders is that you can't just get the property
here.
You can get the new BASE version of the property and the new ACTUAL version
of the property (which is usually the old actual property on property
conflicts), but you can't access the other property values (read: ORIGINAL)
as you don't know the old revision. (Unless you captured it somewhere before
the operation)
The new conflict store is designed to fix this problem, but that will have
to wait for 1.8.
Bert
Re: svn commit: r1135635 -
/subversion/trunk/subversion/libsvn_wc/props.c
Posted by Stefan Sperling <st...@elego.de>.
On Wed, Jun 15, 2011 at 06:55:19AM +0300, Daniel Shahaf wrote:
> Daniel Shahaf wrote on Wed, Jun 15, 2011 at 06:33:46 +0300:
> > stsp@apache.org wrote on Tue, Jun 14, 2011 at 15:10:50 -0000:
> > > + svn_stringbuf_appendcstr(buf, _("Local property value:\n"));
> > > + if (mine_is_binary)
> > > + svn_stringbuf_appendcstr(buf, _("Cannot display: property value is "
> > > + "binary data\n"));
> >
> > Could we just print a hex dump of the value?
>
> Concretely:
>
> [[[
> but the property has already been locally changed to a different value.
> Local property value:
> mine
> Incoming property value:
> 0x7E454B4602010100000000000000000002003D0001000000A01440000000000040000000000000
> ]]]
While I understand your reasoning I think this would be very
confusing to many users. Not all SVN users are programmers.
And I guess people will in general be using special tools anyway to view
such properties. E.g. if the prop value was a PNG image, they will want
to see the image. A string of hex numbers won't help them much.
There is precedence here, too. We don't show diffs of binary files,
we show a placeholder instead that says a diff cannot be shown.
Would you also prefer svn diff to print a diff of the hex dumps?
I'd doubt it.
I'd say let them propget the value and look at it in whatever manner
they see fit. This is a total edge case anyway. No user using standard
svn: properties is ever going to run into this.
> Patch to produce that:
Re: svn commit: r1135635 -
/subversion/trunk/subversion/libsvn_wc/props.c
Posted by Daniel Shahaf <da...@elego.de>.
Daniel Shahaf wrote on Wed, Jun 15, 2011 at 06:33:46 +0300:
> stsp@apache.org wrote on Tue, Jun 14, 2011 at 15:10:50 -0000:
> > + svn_stringbuf_appendcstr(buf, _("Local property value:\n"));
> > + if (mine_is_binary)
> > + svn_stringbuf_appendcstr(buf, _("Cannot display: property value is "
> > + "binary data\n"));
>
> Could we just print a hex dump of the value?
Concretely:
[[[
but the property has already been locally changed to a different value.
Local property value:
mine
Incoming property value:
0x7E454B4602010100000000000000000002003D0001000000A01440000000000040000000000000
]]]
Patch to produce that:
[[[
Index: subversion/libsvn_wc/props.c
===================================================================
--- subversion/libsvn_wc/props.c (revision 1135910)
+++ subversion/libsvn_wc/props.c (working copy)
@@ -545,6 +545,16 @@ maybe_prop_value(const svn_skel_t *skel,
}
+/** Store printf("%x", c) into BUF and return the latter. */
+static APR_INLINE const char *
+hex_of(char *buf, const unsigned char c)
+{
+ buf[0] = "01234567890ABCDEF"[(c >> 4)];
+ buf[1] = "01234567890ABCDEF"[c & 0xf];
+ buf[2] = '\0';
+ return buf;
+}
+
/* Parse a property conflict description from the provided SKEL.
The result includes a descriptive message (see generate_conflict_message)
and maybe a diff of property values containing conflict markers.
@@ -647,8 +657,14 @@ prop_conflict_from_skel(const svn_string_t **confl
{
svn_stringbuf_appendcstr(buf, _("Local property value:\n"));
if (mine_is_binary)
- svn_stringbuf_appendcstr(buf, _("Cannot display: property value is "
- "binary data\n"));
+ {
+ char hex[3] = {0};
+ svn_string_t mine2 = *mine;
+
+ svn_stringbuf_appendcstr(buf, "0x");
+ while (mine2.len--)
+ svn_stringbuf_appendbytes(buf, hex_of(hex, *mine2.data++), 2);
+ }
else
svn_stringbuf_appendbytes(buf, mine->data, mine->len);
svn_stringbuf_appendcstr(buf, "\n");
@@ -658,8 +674,14 @@ prop_conflict_from_skel(const svn_string_t **confl
{
svn_stringbuf_appendcstr(buf, _("Incoming property value:\n"));
if (incoming_is_binary)
- svn_stringbuf_appendcstr(buf, _("Cannot display: property value is "
- "binary data\n"));
+ {
+ char hex[3] = {0};
+ svn_string_t incoming2 = *incoming;
+
+ svn_stringbuf_appendcstr(buf, "0x");
+ while (incoming2.len--)
+ svn_stringbuf_appendbytes(buf, hex_of(hex, *incoming2.data++), 2);
+ }
else
svn_stringbuf_appendbytes(buf, incoming->data, incoming->len);
svn_stringbuf_appendcstr(buf, "\n");
]]]
If the general idea is plausible I'll clean up the patch, fix tests,
commit, etc.
Re: svn commit: r1135635 -
/subversion/trunk/subversion/libsvn_wc/props.c
Posted by Daniel Shahaf <da...@elego.de>.
Daniel Shahaf wrote on Wed, Jun 15, 2011 at 06:33:46 +0300:
> stsp@apache.org wrote on Tue, Jun 14, 2011 at 15:10:50 -0000:
> > + svn_stringbuf_appendcstr(buf, _("Local property value:\n"));
> > + if (mine_is_binary)
> > + svn_stringbuf_appendcstr(buf, _("Cannot display: property value is "
> > + "binary data\n"));
>
> Could we just print a hex dump of the value?
Concretely:
[[[
but the property has already been locally changed to a different value.
Local property value:
mine
Incoming property value:
0x7E454B4602010100000000000000000002003D0001000000A01440000000000040000000000000
]]]
Patch to produce that:
[[[
Index: subversion/libsvn_wc/props.c
===================================================================
--- subversion/libsvn_wc/props.c (revision 1135910)
+++ subversion/libsvn_wc/props.c (working copy)
@@ -545,6 +545,16 @@ maybe_prop_value(const svn_skel_t *skel,
}
+/** Store printf("%x", c) into BUF and return the latter. */
+static APR_INLINE const char *
+hex_of(char *buf, const unsigned char c)
+{
+ buf[0] = "01234567890ABCDEF"[(c >> 4)];
+ buf[1] = "01234567890ABCDEF"[c & 0xf];
+ buf[2] = '\0';
+ return buf;
+}
+
/* Parse a property conflict description from the provided SKEL.
The result includes a descriptive message (see generate_conflict_message)
and maybe a diff of property values containing conflict markers.
@@ -647,8 +657,14 @@ prop_conflict_from_skel(const svn_string_t **confl
{
svn_stringbuf_appendcstr(buf, _("Local property value:\n"));
if (mine_is_binary)
- svn_stringbuf_appendcstr(buf, _("Cannot display: property value is "
- "binary data\n"));
+ {
+ char hex[3] = {0};
+ svn_string_t mine2 = *mine;
+
+ svn_stringbuf_appendcstr(buf, "0x");
+ while (mine2.len--)
+ svn_stringbuf_appendbytes(buf, hex_of(hex, *mine2.data++), 2);
+ }
else
svn_stringbuf_appendbytes(buf, mine->data, mine->len);
svn_stringbuf_appendcstr(buf, "\n");
@@ -658,8 +674,14 @@ prop_conflict_from_skel(const svn_string_t **confl
{
svn_stringbuf_appendcstr(buf, _("Incoming property value:\n"));
if (incoming_is_binary)
- svn_stringbuf_appendcstr(buf, _("Cannot display: property value is "
- "binary data\n"));
+ {
+ char hex[3] = {0};
+ svn_string_t incoming2 = *incoming;
+
+ svn_stringbuf_appendcstr(buf, "0x");
+ while (incoming2.len--)
+ svn_stringbuf_appendbytes(buf, hex_of(hex, *incoming2.data++), 2);
+ }
else
svn_stringbuf_appendbytes(buf, incoming->data, incoming->len);
svn_stringbuf_appendcstr(buf, "\n");
]]]
If the general idea is plausible I'll clean up the patch, fix tests,
commit, etc.