Hi Greg,
If you have a chance, let me know if you were planning on giving any
feedback on this. Just want to be sure I answered your questions
before committing.
Thanks,
Paul
On Fri, May 14, 2010 at 1:46 PM, Paul Burba <ptburba_at_gmail.com> wrote:
> On Fri, Apr 23, 2010 at 5:22 PM, Â <gstein_at_apache.org> 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.
> ]]]
>
Received on 2010-05-20 21:26:45 CEST