On Fri, Apr 23, 2010 at 5:22 PM, <[email protected]> wrote:
> Author: gstein
> Date: Fri Apr 23 21:22:52 2010
> New Revision: 937524
>
> URL: http://svn.apache.org/viewvc?rev=937524&view=rev
> Log:
> Begin new infrastructure for generating prop conflict messages. This will
> allow us to (re)generate a property reject file at will, given a record of
> the property conflicts on a given node.
>
> There are two issues for discussion and fixing in a future revision:
> - incoming-delete will remove local-add (it should conflict?)
Hi Greg,
I think the correct behavior is: An incoming-delete removes a local
add only if the incoming base value is the *same* as the added value;
otherwise there is a conflict. This is analogous to how we treat an
incoming file deletion on a local file addition. It's only a tree
conflict if the files differ.
More below...
> - incoming-delete will crash on a local-delete
>
> * subversion/libsvn_wc/props.c:
> (generate_conflict_message): new function to generate a property
> conflict message given the four property values involved in a 4-way
> merge.
> (apply_single_prop_delete): leave two notes about behavior in here (see
> the issues above). fix message generation: use OLD_VAL, not BASE_VAL
> (apply_single_generic_prop_change): the OLD_VAL parameter will always be
> not-NULL, so we can simplify an if condition.
> (svn_wc__merge_props): save away MINE_VAL, and then if we see a conflict
> message returned by the property merging functions, then assert that
> our new function comes up with the same message
>
> * subversion/tests/cmdline/prop_tests.py:
> (prop_reject_grind): new test function to grind thru all the variations
> of property conflicts.
> (test_list): add new test
>
> * subversion/tests/cmdline/svntest/sandbox.py:
> (Sandbox.simple_propset, Sandbox.simple_propdel): new methods
>
> Modified:
> subversion/trunk/subversion/libsvn_wc/props.c
> subversion/trunk/subversion/tests/cmdline/prop_tests.py
> subversion/trunk/subversion/tests/cmdline/svntest/sandbox.py
>
> Modified: subversion/trunk/subversion/libsvn_wc/props.c
> URL:
> http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_wc/props.c?rev=937524&r1=937523&r2=937524&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/libsvn_wc/props.c (original)
> +++ subversion/trunk/subversion/libsvn_wc/props.c Fri Apr 23 21:22:52 2010
> @@ -709,6 +709,136 @@ svn_wc_merge_props3(svn_wc_notify_state_
> }
>
>
> +/* Generate a message to describe the property conflict among these four
> + values.
> +
> + 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 *
> +generate_conflict_message(const char *propname,
> + const svn_string_t *original,
> + const svn_string_t *mine,
> + const svn_string_t *incoming,
> + const svn_string_t *incoming_base,
> + apr_pool_t *result_pool)
> +{
> + if (incoming_base == NULL)
> + {
> + /* Attempting to add the value INCOMING. */
> + SVN_ERR_ASSERT_NO_RETURN(incoming != NULL);
> +
> + if (mine)
> + {
> + /* To have a conflict, these must be different. */
> + SVN_ERR_ASSERT_NO_RETURN(!svn_string_compare(mine, incoming));
> +
> + /* 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);
> + }
> +
> + /* 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);
> + }
> +
> + if (incoming == NULL)
> + {
> + /* Attempting to delete the value INCOMING_BASE. */
> + SVN_ERR_ASSERT_NO_RETURN(incoming_base != NULL);
> +
> + /* A conflict can only occur if we originally had the property;
> + otherwise, we would have merged the property-delete into the
> + non-existent property. */
> + SVN_ERR_ASSERT_NO_RETURN(original != NULL);
> +
> + if (mine && svn_string_compare(original, incoming_base))
> + {
> + /* 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);
> + }
> +
> + /* 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));
> +
> + /* ### wait. what if we had a different property and locally
> + ### deleted it? the statement below is gonna blow up.
> + ### we could have: local-add, local-edit, local-del, or just
> + ### something different (and unchanged). */
<snip>
> @@ -1166,6 +1296,8 @@ apply_single_prop_delete(svn_wc_notify_s
>
> if (! base_val)
> {
> + /* ### what about working_val? what if we locally-added? */
> +
> apr_hash_set(working_props, propname, APR_HASH_KEY_STRING, NULL);
> if (old_val)
> /* This is a merge, merging a delete into non-existent */
> @@ -1216,11 +1348,13 @@ apply_single_prop_delete(svn_wc_notify_s
> cancel_func, cancel_baton,
> dry_run, scratch_pool));
> if (got_conflict)
> + /* ### wait. what if we had a different property and locally
> + ### deleted it? the statement below is gonna blow up. */
Attached is a patch that fixes the segfault and makes an incoming
deletion on a local addition, where the incoming base value differs
from the added value, a conflict, rather than unconditionally deleting
the addition.
I also tweaked prop_test.py 32 to check the results of the *.prej file.
This patch adds two new potential conflicts messages:
Incoming delete on local add of different value:
Trying to delete property 'del.add' with value 'repos',
but property has been locally added with value 'local'.
Incoming delete on local delete of different value:
Trying to delete property 'del.del' with value 'repos',
but property with value 'local' is locally deleted.
What do you think?
Paul
[[[
Fix some property merge conflict bugs.
1) Incoming delete on a local add of a different value is now a
conflict. Previously it was a clean merge and the prop was
deleted.
2) Incoming delete on a local delete where the incoming base value
differs from the local value is now a conflict. Previously
this caused a segfault.
* subversion/libsvn_wc/props.c
(generate_conflict_message): Handle incoming delete on local add and
incoming delete on local delete of a different prop value. Consistently
use a trailing ',' after the first line of each prej conflict message.
(apply_single_prop_delete): Stop considering an incoming delete on a local
add as a merge.
* subversion/tests/cmdline/prop_tests.py
(prop_reject_grind): Start testing incoming delete on local delete of
different prop value. Verify the resulting *.prej file.
]]]
Index: subversion/libsvn_wc/props.c
===================================================================
--- subversion/libsvn_wc/props.c (revision 944335)
+++ subversion/libsvn_wc/props.c (working copy)
@@ -795,34 +795,51 @@
/* Attempting to delete the value INCOMING_BASE. */
SVN_ERR_ASSERT_NO_RETURN(incoming_base != NULL);
+ /* 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);
+
/* A conflict can only occur if we originally had the property;
otherwise, we would have merged the property-delete into the
non-existent property. */
SVN_ERR_ASSERT_NO_RETURN(original != NULL);
- if (mine && svn_string_compare(original, incoming_base))
+ if (svn_string_compare(original, incoming_base))
{
- /* We were trying to delete the correct property, but an edit
- caused the conflict. */
+ 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);
+ }
+ 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 it has been modified "
- "from '%s' to '%s'."),
+ "value '%s',\nbut property with value "
+ "'%s' is locally deleted."),
propname, incoming_base->data,
- original->data, mine->data);
+ original->data);
}
/* 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));
- /* ### wait. what if we had a different property and locally
- ### deleted it? the statement below is gonna blow up.
- ### we could have: local-add, local-edit, local-del, or just
- ### something different (and unchanged). */
return svn_string_createf(result_pool,
_("Trying to delete property '%s' with "
- "value '%s'\nbut the local value is "
+ "value '%s',\nbut the local value is "
"'%s'."),
propname, incoming_base->data, mine->data);
}
@@ -1409,12 +1426,27 @@
if (! base_val)
{
- /* ### what about working_val? what if we locally-added? */
-
- apr_hash_set(working_props, propname, APR_HASH_KEY_STRING, NULL);
- if (old_val)
- /* This is a merge, merging a delete into non-existent */
- set_prop_merge_state(state, svn_wc_notify_state_merged);
+ if (working_val
+ && !svn_string_compare(working_val, old_val))
+ {
+ /* We are trying to delete a locally-added prop. */
+ SVN_ERR(maybe_generate_propconflict(conflict_remains,
+ db, local_abspath,
+ left_version, right_version,
+ is_dir, propname,
+ working_props, old_val, NULL,
+ base_val, working_val,
+ conflict_func, conflict_baton,
+ dry_run, scratch_pool));
+ }
+ else
+ {
+ apr_hash_set(working_props, propname, APR_HASH_KEY_STRING, NULL);
+ if (old_val)
+ /* This is a merge, merging a delete into non-existent
+ property or a local addition of same prop value. */
+ set_prop_merge_state(state, svn_wc_notify_state_merged);
+ }
}
else if (svn_string_compare(base_val, old_val))
@@ -1439,7 +1471,8 @@
}
}
else
- /* The property is locally deleted, so it's a merge */
+ /* The property is locally deleted from the same value, so it's
+ a merge */
set_prop_merge_state(state, svn_wc_notify_state_merged);
}
Index: subversion/tests/cmdline/merge_tests.py
===================================================================
--- subversion/tests/cmdline/merge_tests.py (revision 944335)
+++ subversion/tests/cmdline/merge_tests.py (working copy)
@@ -4244,6 +4244,7 @@
expected_backup_status = svntest.actions.get_virginal_state(wc_backup, 1)
expected_backup_status.tweak('', status=' M')
expected_backup_status.tweak('A/mu', status='M ')
+ #raise svntest.Failure("PTB")
svntest.actions.run_and_verify_merge(wc_backup, '3', '4', sbox.repo_url,
None,
expected_backup_output,
expected_mergeinfo_output,
Index: subversion/tests/cmdline/prop_tests.py
===================================================================
--- subversion/tests/cmdline/prop_tests.py (revision 944335)
+++ subversion/tests/cmdline/prop_tests.py (working copy)
@@ -1780,6 +1780,7 @@
iota_path = sbox.ospath('iota')
mu_path = sbox.ospath('A/mu')
+ mu_prej_path = sbox.ospath('A/mu.prej')
# Create r2 with all the properties we intend to use as incoming-change,
# and as incoming-delete. Also set up our local-edit and local-delete
@@ -1823,8 +1824,7 @@
sbox.simple_propdel('del.edit', iota_path)
sbox.simple_propdel('del.edit2', iota_path)
sbox.simple_propdel('del.diff', iota_path)
- ### don't delete this. causes a segfault :-)
- #sbox.simple_propdel('del.del', iota_path)
+ sbox.simple_propdel('del.del', iota_path)
sbox.simple_propdel('del.add', iota_path)
sbox.simple_commit()
@@ -1845,10 +1845,70 @@
svntest.main.run_svn(False, 'merge', '-r2:3', sbox.repo_url + '/iota',
mu_path)
- ### need to verify mu.prej
- ### note that del.add has been erroneously deleted!
+ # 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)
+
def obstructed_subdirs(sbox):
"""test properties of obstructed subdirectories"""
