Author: stsp
Date: Mon Jun 13 23:41:37 2011
New Revision: 1135340
URL: http://svn.apache.org/viewvc?rev=1135340&view=rev
Log:
Improve presentation of conflicts involving multi-line property values
in the property conflicts reject file.
We used to present property conflicts in the reject file in simple
sentences, like this:
Trying to change property 'foo' from 'bar' to 'baz',
but it has been locally deleted.
This format has obvious shortcomings when the property value spans
multiple lines, such as with svn:externals or svn:ignore.
The new format retains the descriptive messages explaining the conflict,
but uses diff3 output with conflict markers to show conflicts in property
values, if possible. If it falls back to showing full property values,
each value is shown on a line of its own, preceded by a line explaining
which value follows.
Example of the new output (a conflict diff with one trailing line of context):
Trying to change property 'svn:externals'
but the local property value conflicts with the incoming change.
<<<<<<< (local property value)
^/trunk/alpha ta
=======
^/branch/beta bb
>>>>>>> (incoming property value)
^/foo/bar bar-ext
If we fall back to printing full values, it looks like this:
Trying to add new property 'add.diff'
but the property already exists.
Local property value:
foo
Incoming property value:
bar
Note that the new format does not include the local or incoming base
values anymore. We don't show those for normal text conflicts either,
and they can easily be obtained if needed ('svn diff' for the base of
a local prop change, and 'svn propget' or 'svn diff' for the base of
an incoming change).
One remaining problem is that we will happily try to run diff3 on
binary property values. This will be addressed in a future commit.
This problem also existed with the old format.
* subversion/libsvn_wc/props.c
(append_prop_conflict): Adjust for changes to prop_conflict_from_skel().
(generate_conflict_message): Stop including property values in messages.
Rephrase messages slightly for consistency. Return a stringbuf instead of
a string since most of the time the caller will have to append more data.
(message_from_skel): Rename to ...
(prop_conflict_from_skel): ... this, because it will now return more
than just a simple message. Add a new output parameter CONFLICT_DESC
to allow this function to return an svn_error_t. As before, get a conflict
description message from generate_conflict_message(). But also run diff3
on the property values to generate a diff with conflict markers.
If that fails, print available property values in their entirety.
* subversion/tests/cmdline/merge_tests.py
(simple_property_merges, property_merge_undo_redo): Adjust expected output.
* subversion/tests/cmdline/prop_tests.py
(prop_reject_grind): More substantial changes were needed for this test.
The code that checked the output relied on the structure of the old
format (2 lines per message) and had to be rewritten.
Modified:
subversion/trunk/subversion/libsvn_wc/props.c
subversion/trunk/subversion/tests/cmdline/merge_tests.py
subversion/trunk/subversion/tests/cmdline/prop_tests.py
Modified: subversion/trunk/subversion/libsvn_wc/props.c
URL:
http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_wc/props.c?rev=1135340&r1=1135339&r2=1135340&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_wc/props.c (original)
+++ subversion/trunk/subversion/libsvn_wc/props.c Mon Jun 13 23:41:37 2011
@@ -61,15 +61,16 @@
#include "svn_private_config.h"
/* Forward declaration. */
-static const svn_string_t *
-message_from_skel(const svn_skel_t *skel,
- apr_pool_t *result_pool,
- apr_pool_t *scratch_pool);
+static svn_error_t *
+prop_conflict_from_skel(const svn_string_t **conflict_desc,
+ const svn_skel_t *skel,
+ apr_pool_t *result_pool,
+ apr_pool_t *scratch_pool);
-/* Given a *SINGLE* property conflict in PROP_SKEL, generate a message
+/* Given a *SINGLE* property conflict in PROP_SKEL, generate a description
for it, and write it to STREAM, along with a trailing EOL sequence.
- See message_from_skel() for details on PROP_SKEL. */
+ See prop_conflict_from_skel() for details on PROP_SKEL. */
static svn_error_t *
append_prop_conflict(svn_stream_t *stream,
const svn_skel_t *prop_skel,
@@ -77,15 +78,13 @@ append_prop_conflict(svn_stream_t *strea
{
/* TODO: someday, perhaps prefix each conflict_description with a
timestamp or something? */
- const svn_string_t *message = message_from_skel(prop_skel, pool, pool);
+ const svn_string_t *conflict_desc;
apr_size_t len;
- const char *native_text =
- svn_utf_cstring_from_utf8_fuzzy(message->data, pool);
+ const char *native_text;
- len = strlen(native_text);
- SVN_ERR(svn_stream_write(stream, native_text, &len));
+ SVN_ERR(prop_conflict_from_skel(&conflict_desc, prop_skel, pool, pool));
+ native_text = svn_utf_cstring_from_utf8_fuzzy(conflict_desc->data, pool);
- native_text = svn_utf_cstring_from_utf8_fuzzy(APR_EOL_STR, pool);
len = strlen(native_text);
return svn_stream_write(stream, native_text, &len);
}
@@ -391,7 +390,7 @@ svn_wc_merge_props3(svn_wc_notify_state_
Note that this function (currently) interprets the property values as
strings, but they could actually be binary values. We'll keep the
types as svn_string_t in case we fix this in the future. */
-static const svn_string_t *
+static svn_stringbuf_t *
generate_conflict_message(const char *propname,
const svn_string_t *original,
const svn_string_t *mine,
@@ -412,21 +411,20 @@ generate_conflict_message(const char *pr
/* Note that we don't care whether MINE is locally-added or
edited, or just something different that is a copy of the
pristine ORIGINAL. */
- return svn_string_createf(result_pool,
- _("Trying to add new property '%s' with "
- "value '%s',\nbut property already "
- "exists with value '%s'."),
- propname, incoming->data, mine->data);
+ return svn_stringbuf_createf(result_pool,
+ _("Trying to add new property '%s'\n"
+ "but the property already exists.\n"),
+ propname);
}
/* To have a conflict, we must have an ORIGINAL which has been
locally-deleted. */
SVN_ERR_ASSERT_NO_RETURN(original != NULL);
- return svn_string_createf(result_pool,
- _("Trying to create property '%s' with "
- "value '%s',\nbut it has been locally "
- "deleted."),
- propname, incoming->data);
+ return svn_stringbuf_createf(result_pool,
+ _("Trying to add new property '%s'\n"
+ "but the property has been locally "
+ "deleted.\n"),
+ propname);
}
if (incoming == NULL)
@@ -436,12 +434,11 @@ generate_conflict_message(const char *pr
/* Are we trying to delete a local addition? */
if (original == NULL && mine != NULL)
- return svn_string_createf(result_pool,
- _("Trying to delete property '%s' with "
- "value '%s',\nbut property has been "
- "locally added with value '%s'."),
- propname, incoming_base->data,
- mine->data);
+ return svn_stringbuf_createf(result_pool,
+ _("Trying to delete property '%s'\n"
+ "but the property has been locally "
+ "added.\n"),
+ propname);
/* A conflict can only occur if we originally had the property;
otherwise, we would have merged the property-delete into the
@@ -453,34 +450,33 @@ generate_conflict_message(const char *pr
if (mine)
/* We were trying to delete the correct property, but an edit
caused the conflict. */
- return svn_string_createf(result_pool,
- _("Trying to delete property '%s' with "
- "value '%s',\nbut it has been modified
"
- "from '%s' to '%s'."),
- propname, incoming_base->data,
- original->data, mine->data);
+ return svn_stringbuf_createf(result_pool,
+ _("Trying to delete property '%s'\n"
+ "but the property has been locally "
+ "modified.\n"),
+ propname);
}
else if (mine == NULL)
{
/* We were trying to delete the property, but we have locally
deleted the same property, but with a different value. */
- return svn_string_createf(result_pool,
- _("Trying to delete property '%s' with "
- "value '%s',\nbut property with value "
- "'%s' is locally deleted."),
- propname, incoming_base->data,
- original->data);
+ return svn_stringbuf_createf(result_pool,
+ _("Trying to delete property '%s'\n"
+ "but the property has been locally "
+ "deleted and had a different "
+ "value.\n"),
+ propname);
}
/* We were trying to delete INCOMING_BASE but our ORIGINAL is
something else entirely. */
SVN_ERR_ASSERT_NO_RETURN(!svn_string_compare(original, incoming_base));
- return svn_string_createf(result_pool,
- _("Trying to delete property '%s' with "
- "value '%s',\nbut the local value is "
- "'%s'."),
- propname, incoming_base->data, mine->data);
+ return svn_stringbuf_createf(result_pool,
+ _("Trying to delete property '%s'\n"
+ "but the local property value is "
+ "different.\n"),
+ propname);
}
/* Attempting to change the property from INCOMING_BASE to INCOMING. */
@@ -495,40 +491,37 @@ generate_conflict_message(const char *pr
/* We have an unchanged property, so the original values must
have been different. */
SVN_ERR_ASSERT_NO_RETURN(!svn_string_compare(original, incoming_base));
- return svn_string_createf(result_pool,
- _("Trying to change property '%s' from '%s' "
- "to '%s',\nbut property already exists "
- "with value '%s'."),
- propname, incoming_base->data, incoming->data,
- mine->data);
+ return svn_stringbuf_createf(result_pool,
+ _("Trying to change property '%s'\n"
+ "but the local property value conflicts "
+ "with the incoming change.\n"),
+ propname);
}
if (original && mine)
- return svn_string_createf(result_pool,
- _("Trying to change property '%s' from '%s' "
- "to '%s',\nbut the property has been locally "
- "changed from '%s' to '%s'."),
- propname, incoming_base->data, incoming->data,
- original->data, mine->data);
+ return svn_stringbuf_createf(result_pool,
+ _("Trying to change property '%s'\n"
+ "but the property has already been locally "
+ "changed to a different value.\n"),
+ propname);
if (original)
- return svn_string_createf(result_pool,
- _("Trying to change property '%s' from '%s' "
- "to '%s',\nbut it has been locally deleted."),
- propname, incoming_base->data, incoming->data);
+ return svn_stringbuf_createf(result_pool,
+ _("Trying to change property '%s'\nbut "
+ "the property has been locally deleted.\n"),
+ propname);
if (mine)
- return svn_string_createf(result_pool,
- _("Trying to change property '%s' from '%s' "
- "to '%s',\nbut property has been locally "
- "added with value '%s'."),
- propname, incoming_base->data, incoming->data,
- mine->data);
-
- return svn_string_createf(result_pool,
- _("Trying to change property '%s' from '%s' to "
- "'%s',\nbut the property does not exist."),
- propname, incoming_base->data, incoming->data);
+ return svn_stringbuf_createf(result_pool,
+ _("Trying to change property '%s'\nbut the "
+ "property has been locally added with a "
+ "different value.\n"),
+ propname);
+
+ return svn_stringbuf_createf(result_pool,
+ _("Trying to change property '%s'\nbut "
+ "the property does not exist locally.\n"),
+ propname);
}
@@ -552,25 +545,30 @@ maybe_prop_value(const svn_skel_t *skel,
}
-/* Generate a property conflict message (see generate_conflict_message)
- from the data contained in SKEL. The message will be allocated in
- RESULT_POOL.
+/* 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.
+ The result will be allocated in RESULT_POOL.
Note: SKEL is a single property conflict of the form:
("prop" ([ORIGINAL]) ([MINE]) ([INCOMING]) ([INCOMING_BASE]))
See notes/wc-ng/conflict-storage for more information. */
-static const svn_string_t *
-message_from_skel(const svn_skel_t *skel,
- apr_pool_t *result_pool,
- apr_pool_t *scratch_pool)
+static svn_error_t *
+prop_conflict_from_skel(const svn_string_t **conflict_desc,
+ const svn_skel_t *skel,
+ apr_pool_t *result_pool,
+ apr_pool_t *scratch_pool)
{
const svn_string_t *original;
const svn_string_t *mine;
const svn_string_t *incoming;
const svn_string_t *incoming_base;
const char *propname;
+ svn_diff_t *diff;
+ svn_diff_file_options_t *diff_opts;
+ svn_stringbuf_t *buf;
/* Navigate to the property name. */
skel = skel->children->next;
@@ -583,8 +581,72 @@ message_from_skel(const svn_skel_t *skel
incoming = maybe_prop_value(skel->next->next->next, scratch_pool);
incoming_base = maybe_prop_value(skel->next->next->next->next, scratch_pool);
- return generate_conflict_message(propname, original, mine, incoming,
- incoming_base, result_pool);
+ buf = generate_conflict_message(propname, original, mine, incoming,
+ incoming_base, scratch_pool);
+
+ if (mine == NULL)
+ mine = svn_string_create("", scratch_pool);
+ if (incoming == NULL)
+ incoming = svn_string_create("", scratch_pool);
+
+ /* 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("", scratch_pool);
+ }
+ 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));
+ }
+ else
+ {
+ /* 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");
+ }
+
+ 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");
+ }
+ }
+
+ *conflict_desc = svn_string_create_from_buf(buf, result_pool);
+ return SVN_NO_ERROR;
}
Modified: subversion/trunk/subversion/tests/cmdline/merge_tests.py
URL:
http://svn.apache.org/viewvc/subversion/trunk/subversion/tests/cmdline/merge_tests.py?rev=1135340&r1=1135339&r2=1135340&view=diff
==============================================================================
--- subversion/trunk/subversion/tests/cmdline/merge_tests.py (original)
+++ subversion/trunk/subversion/tests/cmdline/merge_tests.py Mon Jun 13
23:41:37 2011
@@ -680,8 +680,10 @@ def simple_property_merges(sbox):
None, None, None, None, None, 1)
def error_message(property, old_value, new_value):
- return "Trying to change property '%s' from '%s' to '%s',\n" \
- "but it has been locally deleted.\n" % (property, old_value,
new_value)
+ return "Trying to change property '%s'\n" \
+ "but the property has been locally deleted.\n" \
+ "<<<<<<< (local property value)\n=======\n" \
+ "%s>>>>>>> (incoming property value)\n" % (property, new_value)
# Merge B 3:4 into B2 now causes a conflict
expected_disk.add({
@@ -3079,8 +3081,9 @@ def property_merge_undo_redo(sbox):
expected_elision_output = wc.State(wc_dir, {})
expected_disk = svntest.main.greek_state.copy()
expected_disk.add({'A/B/E/alpha.prej'
- : Item("Trying to create property 'foo' with value 'foo_val',\n"
- + "but it has been locally deleted.\n")})
+ : Item("Trying to add new property 'foo'\n"
+ + "but the property has been locally deleted.\n"
+ + "Incoming property value:\nfoo_val")})
expected_status = svntest.actions.get_virginal_state(wc_dir, 2)
expected_status.tweak('A/B/E/alpha', status=' C')
Modified: subversion/trunk/subversion/tests/cmdline/prop_tests.py
URL:
http://svn.apache.org/viewvc/subversion/trunk/subversion/tests/cmdline/prop_tests.py?rev=1135340&r1=1135339&r2=1135340&view=diff
==============================================================================
--- subversion/trunk/subversion/tests/cmdline/prop_tests.py (original)
+++ subversion/trunk/subversion/tests/cmdline/prop_tests.py Mon Jun 13 23:41:37
2011
@@ -1883,68 +1883,117 @@ def prop_reject_grind(sbox):
mu_path)
# Check that A/mu.prej reports the expected conflicts:
- expected_prej = svntest.verify.UnorderedOutput([
- "Trying to change property 'edit.none' from 'repos' to 'repos.changed',\n"
- "but the property does not exist.\n",
-
- "Trying to delete property 'del.del' with value 'repos',\n"
- "but property with value 'local' is locally deleted.\n",
-
- "Trying to delete property 'del.edit' with value 'repos',\n"
- "but the local value is 'local.changed'.\n",
-
- "Trying to change property 'edit.del' from 'repos' to 'repos.changed',\n"
- "but it has been locally deleted.\n",
-
- "Trying to change property 'edit.edit' from 'repos' to 'repos.changed',\n"
- "but the property has been locally changed from 'local' to
'local.changed'.\n",
-
- "Trying to delete property 'del.edit2' with value 'repos',\n"
- "but it has been modified from 'repos' to 'repos.changed'.\n",
-
- "Trying to delete property 'del.add' with value 'repos',\n"
- "but property has been locally added with value 'local'.\n",
-
- "Trying to delete property 'del.diff' with value 'repos',\n"
- "but the local value is 'local'.\n",
-
- "Trying to change property 'edit.add' from 'repos' to 'repos.changed',\n"
- "but property has been locally added with value 'local'.\n",
-
- "Trying to change property 'edit.diff' from 'repos' to 'repos.changed',\n"
- "but property already exists with value 'local'.\n",
-
- "Trying to add new property 'add.add' with value 'repos',\n"
- "but property already exists with value 'local'.\n",
-
- "Trying to add new property 'add.diff' with value 'repos',\n"
- "but property already exists with value 'local'.\n",
-
- "Trying to create property 'add.del' with value 'repos',\n"
- "but it has been locally deleted.\n",
-
- "Trying to add new property 'add.edit' with value 'repos',\n"
- "but property already exists with value 'local.changed'.\n",
-
- "\n"
- ])
-
- # Get the contents of mu.prej. The error messages in the prej file are
- # two lines each, but there is no guarantee as to order, so remove the
- # newline between each two line error message and then split the whole
- # thing into a list of strings on the remaining newline...
- raw_prej = open(mu_prej_path,
- 'r').read().replace('\nbut', ' but').split('\n')
- # ...then put the newlines back in each list item. That leaves us with
- # list of two lines strings we can compare to the unordered expected
- # prej file.
- actual_prej = []
- for line in raw_prej:
- repaired_line = line.replace(' but', '\nbut')
- actual_prej.append(repaired_line + '\n')
-
- svntest.verify.verify_outputs("Expected mu.prej doesn't match actual
mu.prej",
- actual_prej, None, expected_prej, None)
+ expected_prej = [
+ "Trying to change property 'edit.none'\n"
+ "but the property does not exist locally.\n"
+ "<<<<<<< (local property value)\n"
+ "=======\n"
+ "repos.changed>>>>>>> (incoming property value)\n",
+
+ "Trying to delete property 'del.del'\n"
+ "but the property has been locally deleted and had a different value.\n",
+
+ "Trying to delete property 'del.edit'\n"
+ "but the local property value is different.\n"
+ "<<<<<<< (local property value)\n"
+ "local.changed=======\n"
+ ">>>>>>> (incoming property value)\n",
+
+ "Trying to change property 'edit.del'\n"
+ "but the property has been locally deleted.\n"
+ "<<<<<<< (local property value)\n"
+ "=======\n"
+ "repos.changed>>>>>>> (incoming property value)\n",
+
+ "Trying to change property 'edit.edit'\n"
+ "but the property has already been locally changed to a different value.\n"
+ "<<<<<<< (local property value)\n"
+ "local.changed=======\n"
+ "repos.changed>>>>>>> (incoming property value)\n",
+
+ "Trying to delete property 'del.edit2'\n"
+ "but the property has been locally modified.\n"
+ "<<<<<<< (local property value)\n"
+ "repos.changed=======\n"
+ ">>>>>>> (incoming property value)\n",
+
+ "Trying to delete property 'del.add'\n"
+ "but the property has been locally added.\n"
+ "<<<<<<< (local property value)\n"
+ "local=======\n"
+ ">>>>>>> (incoming property value)\n",
+
+ "Trying to delete property 'del.diff'\n"
+ "but the local property value is different.\n"
+ "<<<<<<< (local property value)\n"
+ "local=======\n"
+ ">>>>>>> (incoming property value)\n",
+
+ "Trying to change property 'edit.add'\n"
+ "but the property has been locally added with a different value.\n"
+ "<<<<<<< (local property value)\n"
+ "local=======\n"
+ "repos.changed>>>>>>> (incoming property value)\n",
+
+ "Trying to change property 'edit.diff'\n"
+ "but the local property value conflicts with the incoming change.\n"
+ "<<<<<<< (local property value)\n"
+ "local=======\n"
+ "repos.changed>>>>>>> (incoming property value)\n",
+
+ "Trying to add new property 'add.add'\n"
+ "but the property already exists.\n"
+ "<<<<<<< (local property value)\n"
+ "local=======\n"
+ "repos>>>>>>> (incoming property value)\n",
+
+ "Trying to add new property 'add.diff'\n"
+ "but the property already exists.\n"
+ "Local property value:\n"
+ "local\n"
+ "Incoming property value:\n"
+ "repos\n",
+
+ "Trying to add new property 'add.del'\n"
+ "but the property has been locally deleted.\n"
+ "<<<<<<< (local property value)\n"
+ "=======\n"
+ "repos>>>>>>> (incoming property value)\n",
+
+ "Trying to add new property 'add.edit'\n"
+ "but the property already exists.\n"
+ "<<<<<<< (local property value)\n"
+ "local.changed=======\n"
+ "repos>>>>>>> (incoming property value)\n",
+ ]
+
+ # Get the contents of mu.prej. The error messages are in the prej file
+ # but there is no guarantee as to order. So try to locate each message
+ # in the file individually.
+ prej_file = open(mu_prej_path, 'r')
+ n = 0
+ for message in expected_prej:
+ prej_file.seek(0)
+ match = False
+ i = 0
+ j = 0
+ msg_lines = message.split('\n')
+ for file_line in prej_file:
+ line = msg_lines[i] + '\n'
+ match = (line == file_line)
+ if match:
+ # The last line in the list is always an empty string.
+ if msg_lines[i + 1] == "":
+ #print("found message %i in file at line %i" % (n, j))
+ break
+ i += 1
+ else:
+ i = 0
+ j += 1
+ n += 1
+ if not match:
+ raise svntest.main.SVNUnmatchedError(
+ "Expected mu.prej doesn't match actual mu.prej")
def obstructed_subdirs(sbox):
"""test properties of obstructed subdirectories"""
