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 <ptbu...@gmail.com> wrote: > On Fri, Apr 23, 2010 at 5:22 PM, <gst...@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. > ]]] >
