[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: svn commit: r937524 - in /subversion/trunk/subversion: libsvn_wc/props.c tests/cmdline/prop_tests.py tests/cmdline/svntest/sandbox.py

From: Paul Burba <ptburba_at_gmail.com>
Date: Fri, 21 May 2010 10:53:47 -0400

On Thu, May 20, 2010 at 4:59 PM, Greg Stein <gstein_at_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 <ptburba_at_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 <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-21 16:54:20 CEST

This is an archived mail posted to the Subversion Dev mailing list.

This site is subject to the Apache Privacy Policy and the Apache Public Forum Archive Policy.