You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@subversion.apache.org by Paul Burba <pt...@gmail.com> on 2010/05/14 19:46:00 UTC
Re: svn commit: r937524 - in /subversion/trunk/subversion:
libsvn_wc/props.c tests/cmdline/prop_tests.py tests/cmdline/svntest/sandbox.py
On Fri, Apr 23, 2010 at 5:22 PM, <gs...@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.
]]]
Re: svn commit: r937524 - in /subversion/trunk/subversion:
libsvn_wc/props.c tests/cmdline/prop_tests.py tests/cmdline/svntest/sandbox.py
Posted by Paul Burba <pt...@gmail.com>.
On Fri, Jun 4, 2010 at 2:00 PM, Greg Stein <gs...@gmail.com> wrote:
> On Fri, May 21, 2010 at 10:53, Paul Burba <pt...@gmail.com> wrote:
>> On Thu, May 20, 2010 at 4:59 PM, Greg Stein <gs...@gmail.com> wrote:
>>> Thanks for the ping.
>>>
>>> The patch looks good except for the incoming-delete case.
>>
>> Hi Greg,
>>
>> Which flavor of that case? Incoming delete on a local delete of the
>> same property with the same value? Or something else?
>>
>>> If the
>>> svn_string_compare() succeeds, but mine==NULL, then you get the crash.
>>> I think the mine==NULL needs to remain on the outer-if test.
>>
>> I'm not entirely sure what you are referring to here. Is it this
>> section of generate_conflict_message()?
>
> That section, yeah. But you're right. We should be okay.
>
> Go ahead and commit.
http://svn.apache.org/viewvc?view=revision&revision=951517
> I've got a couple ideas to clarify a few of the
> assertions afterwards, so that (I) won't get confused around this
> stuff again.
>
> Cheers,
> -g
>
Re: svn commit: r937524 - in /subversion/trunk/subversion:
libsvn_wc/props.c tests/cmdline/prop_tests.py tests/cmdline/svntest/sandbox.py
Posted by Greg Stein <gs...@gmail.com>.
On Fri, May 21, 2010 at 10:53, Paul Burba <pt...@gmail.com> wrote:
> On Thu, May 20, 2010 at 4:59 PM, Greg Stein <gs...@gmail.com> wrote:
>> Thanks for the ping.
>>
>> The patch looks good except for the incoming-delete case.
>
> Hi Greg,
>
> Which flavor of that case? Incoming delete on a local delete of the
> same property with the same value? Or something else?
>
>> If the
>> svn_string_compare() succeeds, but mine==NULL, then you get the crash.
>> I think the mine==NULL needs to remain on the outer-if test.
>
> I'm not entirely sure what you are referring to here. Is it this
> section of generate_conflict_message()?
That section, yeah. But you're right. We should be okay.
Go ahead and commit. I've got a couple ideas to clarify a few of the
assertions afterwards, so that (I) won't get confused around this
stuff again.
Cheers,
-g
Re: svn commit: r937524 - in /subversion/trunk/subversion:
libsvn_wc/props.c tests/cmdline/prop_tests.py tests/cmdline/svntest/sandbox.py
Posted by Paul Burba <pt...@gmail.com>.
On Thu, May 20, 2010 at 4:59 PM, Greg Stein <gs...@gmail.com> wrote:
> Thanks for the ping.
>
> The patch looks good except for the incoming-delete case.
Hi Greg,
Which flavor of that case? Incoming delete on a local delete of the
same property with the same value? Or something else?
> If the
> svn_string_compare() succeeds, but mine==NULL, then you get the crash.
> I think the mine==NULL needs to remain on the outer-if test.
I'm not entirely sure what you are referring to here. Is it this
section of generate_conflict_message()?
[[[
if (svn_string_compare(original, incoming_base))
{
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 property with value "
"'%s' is locally deleted."),
propname, incoming_base->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));
]]]
If (ORIGINAL == INCOMING_BASE) && (MINE == INCOMING == NULL) then
we'll trigger that SVN_ERR_ASSERT_NO_RETURN. But we shouldn't be
calling this function in the first place for this case, because the
function assumes there *is* a prop conflict of some kind. It always
produces a conflict message or asserts trying.
At any rate, I'm a bit confused here.
Paul
> Other than that... looks great. Commit!
>
> Cheers,
> -g
>
> On Thu, May 20, 2010 at 15:26, Paul Burba <pt...@gmail.com> wrote:
>> 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 <pt...@gmail.com> wrote:
>>> On Fri, Apr 23, 2010 at 5:22 PM, <gs...@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.
>>> ]]]
>>>
>>
>
Re: svn commit: r937524 - in /subversion/trunk/subversion:
libsvn_wc/props.c tests/cmdline/prop_tests.py tests/cmdline/svntest/sandbox.py
Posted by Paul Burba <pt...@gmail.com>.
On Thu, May 20, 2010 at 4:59 PM, Greg Stein <gs...@gmail.com> wrote:
> Thanks for the ping.
>
> The patch looks good except for the incoming-delete case.
Hi Greg,
Which flavor of that case? Incoming delete on a local delete of the
same property with the same value? Or something else?
> If the
> svn_string_compare() succeeds, but mine==NULL, then you get the crash.
> I think the mine==NULL needs to remain on the outer-if test.
I'm not entirely sure what you are referring to here. Is it this
section of generate_conflict_message()?
[[[
if (svn_string_compare(original, incoming_base))
{
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 property with value "
"'%s' is locally deleted."),
propname, incoming_base->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));
]]]
If (ORIGINAL == INCOMING_BASE) && (MINE == INCOMING == NULL) then
we'll trigger that SVN_ERR_ASSERT_NO_RETURN. But we shouldn't be
calling this function in the first place for this case, because the
function assumes there *is* a prop conflict of some kind. It always
produces a conflict message or asserts trying.
At any rate, I'm a bit confused here.
Paul
> Other than that... looks great. Commit!
>
> Cheers,
> -g
>
> On Thu, May 20, 2010 at 15:26, Paul Burba <pt...@gmail.com> wrote:
>> 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 <pt...@gmail.com> wrote:
>>> On Fri, Apr 23, 2010 at 5:22 PM, <gs...@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.
>>> ]]]
>>>
>>
>
Re: svn commit: r937524 - in /subversion/trunk/subversion:
libsvn_wc/props.c tests/cmdline/prop_tests.py tests/cmdline/svntest/sandbox.py
Posted by Greg Stein <gs...@gmail.com>.
Thanks for the ping.
The patch looks good except for the incoming-delete case. If the
svn_string_compare() succeeds, but mine==NULL, then you get the crash.
I think the mine==NULL needs to remain on the outer-if test.
Other than that... looks great. Commit!
Cheers,
-g
On Thu, May 20, 2010 at 15:26, Paul Burba <pt...@gmail.com> wrote:
> 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 <pt...@gmail.com> wrote:
>> On Fri, Apr 23, 2010 at 5:22 PM, <gs...@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.
>> ]]]
>>
>
Re: svn commit: r937524 - in /subversion/trunk/subversion:
libsvn_wc/props.c tests/cmdline/prop_tests.py tests/cmdline/svntest/sandbox.py
Posted by Greg Stein <gs...@gmail.com>.
Thanks for the ping.
The patch looks good except for the incoming-delete case. If the
svn_string_compare() succeeds, but mine==NULL, then you get the crash.
I think the mine==NULL needs to remain on the outer-if test.
Other than that... looks great. Commit!
Cheers,
-g
On Thu, May 20, 2010 at 15:26, Paul Burba <pt...@gmail.com> wrote:
> 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 <pt...@gmail.com> wrote:
>> On Fri, Apr 23, 2010 at 5:22 PM, <gs...@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.
>> ]]]
>>
>
Re: svn commit: r937524 - in /subversion/trunk/subversion:
libsvn_wc/props.c tests/cmdline/prop_tests.py tests/cmdline/svntest/sandbox.py
Posted by Paul Burba <pt...@gmail.com>.
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 <pt...@gmail.com> wrote:
> On Fri, Apr 23, 2010 at 5:22 PM, <gs...@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.
> ]]]
>
Re: svn commit: r937524 - in /subversion/trunk/subversion:
libsvn_wc/props.c tests/cmdline/prop_tests.py tests/cmdline/svntest/sandbox.py
Posted by Paul Burba <pt...@gmail.com>.
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 <pt...@gmail.com> wrote:
> On Fri, Apr 23, 2010 at 5:22 PM, <gs...@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.
> ]]]
>