Replace a portion of the "merge a prop-change" logic in the WC with a simpler version that is more correct, IMHO. The replacement is for the logic that merges an incoming prop change (not a prop add or delete) into any property except the mergeinfo property. The old version paid attention to the base version in the working copy in non-obvious ways; the new version simply merges the change into the current working version of that property. This makes merge_tests.py 106 pass, which was the motivation for doing it. One test (merge_tests.py 41) expected a conflict when merging a second change to a property. Adjust that test to expect a successful merge. * subversion/libsvn_wc/props.c (apply_single_mergeinfo_prop_change): New function. The mergeinfo half of the former apply_single_prop_change(). (apply_single_generic_prop_change): New function. (apply_single_prop_change): Replace the body with a wrapper around the two new functions. * subversion/tests/cmdline/merge_tests.py (merge_eolstyle_handling): Make the second prop-change merge expect success rather than a conflict. (test_list): Remove the 'XFail' from merge_two_edits_to_same_prop(). Index: subversion/libsvn_wc/props.c =================================================================== --- subversion/libsvn_wc/props.c (revision 33294) +++ subversion/libsvn_wc/props.c (working copy) @@ -1635,38 +1635,27 @@ apply_single_prop_delete(svn_wc_notify_s } -/* Change the property with name PROPNAME in the set of WORKING_PROPS - * on PATH, setting *STATE or *CONFLICT according to merge outcomes. - * - * *STATE is an input and output parameter, its value is to be - * set using set_merge_prop_state(). - * - * BASE_VAL contains the working copy base property value - * - * OLD_VAL contains the value the of the property the server - * thinks it's overwriting - * - * NEW_VAL contains the value to be set. - * - * CONFLICT_FUNC/BATON is a callback to be called before declaring a - * property conflict; it gives the client a chance to resolve the - * conflict interactively. It uses ADM_ACCESS to possibly examine the - * path's entries. - */ +/* Merge a change to the mergeinfo property. The same as + apply_single_prop_change(), except that the PROPNAME is always + SVN_PROP_MERGEINFO. */ +/* ### This function is extracted straight from the previous all-in-one + version of apply_single_prop_change() by removing the code paths that + were not followed for this property, but with no attempt to rationalize + the remainder. */ static svn_error_t * -apply_single_prop_change(svn_wc_notify_state_t *state, - const char *path, - svn_boolean_t is_dir, - apr_hash_t *working_props, - svn_string_t **conflict, - const char *propname, - const svn_string_t *base_val, - const svn_string_t *old_val, - const svn_string_t *new_val, - svn_wc_conflict_resolver_func_t conflict_func, - void *conflict_baton, - svn_wc_adm_access_t *adm_access, - apr_pool_t *pool) +apply_single_mergeinfo_prop_change(svn_wc_notify_state_t *state, + const char *path, + svn_boolean_t is_dir, + apr_hash_t *working_props, + svn_string_t **conflict, + const char *propname, + const svn_string_t *base_val, + const svn_string_t *old_val, + const svn_string_t *new_val, + svn_wc_conflict_resolver_func_t conflict_func, + void *conflict_baton, + svn_wc_adm_access_t *adm_access, + apr_pool_t *pool) { svn_boolean_t got_conflict = FALSE; svn_string_t *working_val @@ -1681,12 +1670,10 @@ apply_single_prop_change(svn_wc_notify_s if (working_val) { if (svn_string_compare(working_val, new_val)) - /* The new value equals the changed value: a merge */ + /* The new value equals the changed value: a no-op merge */ set_prop_merge_state(state, svn_wc_notify_state_merged); else { - if (strcmp(propname, SVN_PROP_MERGEINFO) == 0) - { /* We have base, WC, and new values. Discover deltas between base <-> WC, and base <-> incoming. Combine those deltas, and apply @@ -1697,42 +1684,12 @@ apply_single_prop_change(svn_wc_notify_s apr_hash_set(working_props, propname, APR_HASH_KEY_STRING, new_val); set_prop_merge_state(state, svn_wc_notify_state_merged); - } - else - { - SVN_ERR(maybe_generate_propconflict(&got_conflict, - path, adm_access, is_dir, - propname, working_props, - old_val, new_val, - base_val, working_val, - conflict_func, - conflict_baton, - pool)); - if (got_conflict) - { - if (base_val) - *conflict = svn_string_createf - (pool, - _("Trying to change property '%s' from '%s' to '%s',\n" - "but property has been locally changed " - "from '%s' to '%s'."), - propname, old_val->data, new_val->data, - base_val->data, working_val->data); - else - *conflict = svn_string_createf - (pool, - _("Trying to change property '%s' from '%s' to '%s',\n" - "but property has been locally added with " - "value '%s'."), - propname, old_val->data, new_val->data, - working_val->data); - } - } } } else { + /* There is a base_val but no working_val */ SVN_ERR(maybe_generate_propconflict(&got_conflict, path, adm_access, is_dir, propname, working_props, old_val, new_val, @@ -1751,8 +1708,6 @@ apply_single_prop_change(svn_wc_notify_s else if (! working_val) /* means !working_val && !base_val due to conditions above: no prop at all */ { - if (strcmp(propname, SVN_PROP_MERGEINFO) == 0) - { /* Discover any mergeinfo additions in the incoming value relative to the base, and "combine" those with the empty WC value. */ @@ -1763,22 +1718,6 @@ apply_single_prop_change(svn_wc_notify_s SVN_ERR(svn_mergeinfo_to_string((svn_string_t **)&new_val, added_mergeinfo, pool)); apr_hash_set(working_props, propname, APR_HASH_KEY_STRING, new_val); - } - else - { - SVN_ERR(maybe_generate_propconflict(&got_conflict, path, adm_access, - is_dir, propname, working_props, - old_val, new_val, - base_val, working_val, - conflict_func, conflict_baton, - pool)); - if (got_conflict) - *conflict = svn_string_createf - (pool, - _("Trying to change property '%s' from '%s' to '%s',\n" - "but the property does not exist."), - propname, old_val->data, new_val->data); - } } else /* means working && base && svn_string_compare(working, base) */ @@ -1788,8 +1727,6 @@ apply_single_prop_change(svn_wc_notify_s else { - if (strcmp(propname, SVN_PROP_MERGEINFO) == 0) - { /* We have base, WC, and new values. Discover deltas between base <-> WC, and base <-> incoming. Combine those deltas, and apply @@ -1800,30 +1737,164 @@ apply_single_prop_change(svn_wc_notify_s apr_hash_set(working_props, propname, APR_HASH_KEY_STRING, new_val); set_prop_merge_state(state, svn_wc_notify_state_merged); - } + } + } + + return SVN_NO_ERROR; +} + +/* Merge a change to a property, using the rule that if the working value + is the same as OLD_VAL then apply the change as a simple update + (replacement), otherwise invoke maybe_generate_propconflict(). + The definition of the arguments and behaviour is the same as + apply_single_prop_change(). */ +static svn_error_t * +apply_single_generic_prop_change(svn_wc_notify_state_t *state, + const char *path, + svn_boolean_t is_dir, + apr_hash_t *working_props, + svn_string_t **conflict, + const char *propname, + const svn_string_t *base_val, + const svn_string_t *old_val, + const svn_string_t *new_val, + svn_wc_conflict_resolver_func_t conflict_func, + void *conflict_baton, + svn_wc_adm_access_t *adm_access, + apr_pool_t *pool) +{ + svn_boolean_t got_conflict = FALSE; + svn_string_t *working_val + = apr_hash_get(working_props, propname, APR_HASH_KEY_STRING); + + /* If working_val is the same as old_val... */ + if ((!working_val && !old_val) + || (working_val && old_val + && svn_string_compare(working_val, old_val))) + { + /* A trivial update: change it to new_val. */ + apr_hash_set(working_props, propname, APR_HASH_KEY_STRING, new_val); + } + else + { + /* Merge the change. */ + SVN_ERR(maybe_generate_propconflict(&got_conflict, path, adm_access, + is_dir, propname, working_props, + old_val, new_val, + base_val, working_val, + conflict_func, conflict_baton, + pool)); + if (got_conflict) + { + /* Describe the conflict, referring to base_val as well as + working_val for the user's convenience. */ + if (working_val && base_val + && svn_string_compare(working_val, base_val)) + *conflict = svn_string_createf + (pool, + _("Trying to change property '%s' from '%s' to '%s',\n" + "but property already exists with value '%s'."), + propname, old_val->data, new_val->data, working_val->data); + else if (working_val && base_val) + *conflict = svn_string_createf + (pool, + _("Trying to change property '%s' from '%s' to '%s',\n" + "but the property has been locally changed from '%s' to " + "'%s'."), + propname, old_val->data, new_val->data, + base_val->data, working_val->data); + else if (working_val) + *conflict = svn_string_createf + (pool, + _("Trying to change property '%s' from '%s' to '%s',\n" + "but property has been locally added with value " + "'%s'."), + propname, old_val->data, new_val->data, working_val->data); + else if (base_val) + *conflict = svn_string_createf + (pool, + _("Trying to change property '%s' from '%s' to '%s',\n" + "but it has been locally deleted."), + propname, old_val->data, new_val->data); else - { - SVN_ERR(maybe_generate_propconflict(&got_conflict, path, - adm_access, is_dir, - propname, working_props, - old_val, new_val, - base_val, working_val, - conflict_func, conflict_baton, - pool)); - if (got_conflict) - *conflict = svn_string_createf - (pool, - _("Trying to change property '%s' from '%s' to '%s',\n" - "but property already exists with value '%s'."), - propname, old_val->data, new_val->data, - working_val->data); - } + *conflict = svn_string_createf + (pool, + _("Trying to change property '%s' from '%s' to '%s',\n" + "but the property does not exist."), + propname, old_val->data, new_val->data); } } return SVN_NO_ERROR; } +/* Change the property with name PROPNAME in the set of WORKING_PROPS + * on PATH, setting *STATE or *CONFLICT according to the merge outcome. + * + * *STATE is an input and output parameter, its value is to be + * set using set_prop_merge_state(). (May be null.). + * + * BASE_VAL contains the working copy base property value. (May be null.) + * + * OLD_VAL contains the value of the property the server + * thinks it's overwriting. (Not null.) + * + * NEW_VAL contains the value to be set. (Not null.) + * + * CONFLICT_FUNC/BATON is a callback to be called before declaring a + * property conflict; it gives the client a chance to resolve the + * conflict interactively. It uses ADM_ACCESS to possibly examine the + * path's entries. + */ +static svn_error_t * +apply_single_prop_change(svn_wc_notify_state_t *state, + const char *path, + svn_boolean_t is_dir, + apr_hash_t *working_props, + svn_string_t **conflict, + const char *propname, + const svn_string_t *base_val, + const svn_string_t *old_val, + const svn_string_t *new_val, + svn_wc_conflict_resolver_func_t conflict_func, + void *conflict_baton, + svn_wc_adm_access_t *adm_access, + apr_pool_t *pool) +{ + /* Note: The purpose is to apply the change (old_val -> new_val) onto + (working_val). There is no need for base_val to be involved in the + process except as a bit of context to help the user understand and + resolve any conflict. */ + + /* Decide how to merge, based on whether we know anything special about + the property. */ + if (strcmp(propname, SVN_PROP_MERGEINFO) == 0) + { + /* We know how to merge any mergeinfo property change. */ + + SVN_ERR(apply_single_mergeinfo_prop_change(state, path, is_dir, + working_props, conflict, + propname, base_val, old_val, + new_val, conflict_func, + conflict_baton, adm_access, + pool)); + } + else + { + /* The standard method: perform a simple update automatically, but + pass any other kind of merge to maybe_generate_propconflict(). */ + + SVN_ERR(apply_single_generic_prop_change(state, path, is_dir, + working_props, conflict, + propname, base_val, old_val, + new_val, conflict_func, + conflict_baton, adm_access, + pool)); + } + + return SVN_NO_ERROR; +} + svn_error_t * svn_wc__merge_props(svn_wc_notify_state_t *state, Index: subversion/tests/cmdline/merge_tests.py =================================================================== --- subversion/tests/cmdline/merge_tests.py (revision 33493) +++ subversion/tests/cmdline/merge_tests.py (working copy) @@ -3929,27 +3929,22 @@ # Test 2: now change the eol-style property to another value and commit, # merge this revision in the still changed mu in the second working copy; - # there should be a property conflict! (Since this will overwrite a - # local change to a versioned resource.) + # there should be no conflict! svntest.main.run_svn(None, 'propset', 'svn:eol-style', "CR", mu_path) svntest.main.run_svn(None, 'commit', '-m', 'set eol-style property', wc_dir) expected_backup_disk = svntest.main.greek_state.copy() expected_backup_disk.add({ - 'A/mu' : Item(contents= "This is the file 'mu'." + crlf + - "Added new line of text." + crlf) + 'A/mu' : Item(contents= "This is the file 'mu'.\015" + + "Added new line of text.\015") }) - expected_backup_disk.add({ - 'A/mu.prej' : Item("Trying to change property 'svn:eol-style' from 'CRLF'" - + " to 'CR',\nbut property has been locally added with" - + " value 'CRLF'.\n")}) expected_backup_output = svntest.wc.State(wc_backup, { - 'A/mu' : Item(status='GC'), + 'A/mu' : Item(status='GU'), }) expected_backup_status = svntest.actions.get_virginal_state(wc_backup, 1) expected_backup_status.tweak('', status=' M') - expected_backup_status.tweak('A/mu', status='MC') + expected_backup_status.tweak('A/mu', status='MM') svntest.actions.run_and_verify_merge(wc_backup, '2', '3', sbox.repo_url, expected_backup_output, expected_backup_disk, @@ -3958,7 +3953,7 @@ # Test 3: now delete the eol-style property and commit, merge this revision # in the still changed mu in the second working copy; there should be no - # conflict! (after marking mu resolved from Test 2) + # conflict! # EOL of mu should be unchanged (=CRLF). svntest.main.run_svn(None, 'propdel', 'svn:eol-style', mu_path) svntest.main.run_svn(None, @@ -3966,8 +3961,8 @@ expected_backup_disk = svntest.main.greek_state.copy() expected_backup_disk.add({ - 'A/mu' : Item(contents= "This is the file 'mu'." + crlf + - "Added new line of text." + crlf) + 'A/mu' : Item(contents= "This is the file 'mu'.\015" + + "Added new line of text.\015") }) expected_backup_output = svntest.wc.State(wc_backup, { 'A/mu' : Item(status=' G'), @@ -3975,7 +3970,6 @@ expected_backup_status = svntest.actions.get_virginal_state(wc_backup, 1) expected_backup_status.tweak('', status=' M') expected_backup_status.tweak('A/mu', status='M ') - svntest.main.run_svn(None, 'resolved', path_backup) svntest.actions.run_and_verify_merge(wc_backup, '3', '4', sbox.repo_url, expected_backup_output, expected_backup_disk, @@ -14238,7 +14232,7 @@ server_has_mergeinfo), SkipUnless(merge_target_and_subtrees_need_nonintersecting_ranges, server_has_mergeinfo), - XFail(merge_two_edits_to_same_prop), + merge_two_edits_to_same_prop, merge_an_eol_unification_and_set_svn_eol_style, SkipUnless(merge_adds_mergeinfo_correctly, server_has_mergeinfo),