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 <ptbu...@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 <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. >> ]]] >> >
