You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Julian Foad <ju...@btopenworld.com> on 2012/11/28 19:44:56 UTC
Re: r1414810 - dry_run_added_parent_p() - libsvn_client/merge.c
> Author: cmpilato
> URL: http://svn.apache.org/viewvc?rev=1414810&view=rev
> Log:
> Teach the dry-run merge logic to account for additional flavors of
> ra_serf editor drive ordering violations (*sigh*). [...]
>
> This change causes Subversion to check not just the most recently
> added directory, but all added directories (which were already being
> tracked elsewhere, anyway) before deeming "dir1/file" an obstructed
> merge addition.
>
> * subversion/libsvn_client/merge.c
> (merge_cmd_baton_t): Remove the 'added_path' member.
> (dry_run_added_parent_p): New helper function.
> (merge_file_added): Now use dry_run_added_parent_p() instead of
> merge_b->added_path to look for added parent paths.
> (merge_dir_added): Now use dry_run_added_parent_p() instead of
> merge_b->added_path to record added paths and to check for added
> parent paths.
> (do_merge): Don't initialize the now-removed 'added_path' merge
> context baton member.
If the logic is that the list of added paths is not necessarily contain all leaf nodes, then doesn't it follow that dry_run_deleted_p() also needs to be updated to check whether any parent of the path being checked is in the list?
- Julian
> Modified: subversion/trunk/subversion/libsvn_client/merge.c
> ==============================================================================
> +/* Return true iff we're in dry-run mode and a parent of EDIT_RELPATH
> + would have been added by now if we weren't in dry-run mode.
> + Used to avoid spurious notifications (e.g. conflicts) from a merge
> + attempt into an existing target which would have been deleted if we
> + weren't in dry_run mode (issue #2584). Assumes that WCPATH is
> + still versioned (e.g. has an associated entry). */
> +static svn_boolean_t
> +dry_run_added_parent_p(const merge_cmd_baton_t *merge_b,
> + const char *edit_relpath,
> + apr_pool_t *scratch_pool)
> +{
> + int i;
> + const char *abspath;
> +
> + if (!merge_b->dry_run)
> + return FALSE;
> +
> + abspath = svn_dirent_join(merge_b->target->abspath, edit_relpath,
> + scratch_pool);
> + for (i = 0; i < (svn_path_component_count(edit_relpath) - 1); i++)
> + {
> + abspath = svn_dirent_dirname(abspath, scratch_pool);
> + if (apr_hash_get(merge_b->dry_run_added, abspath,
> + APR_HASH_KEY_STRING))
> + return TRUE;
> + }
> + return FALSE;
> +}
> +
> /* Return whether any WC path was put in conflict by the merge
> operation corresponding to MERGE_B. */
> static APR_INLINE svn_boolean_t
> @@ -1857,8 +1883,8 @@ merge_file_added(svn_wc_notify_state_t *
>
> if (obstr_state != svn_wc_notify_state_inapplicable)
> {
> - if (merge_b->dry_run && merge_b->added_path
> - && svn_dirent_is_child(merge_b->added_path,
> mine_abspath, NULL))
> + if (merge_b->dry_run
> + && dry_run_added_parent_p(merge_b, mine_relpath,
> scratch_pool))
> {
> if (content_state)
> *content_state = svn_wc_notify_state_changed;
> @@ -2337,13 +2363,15 @@ merge_dir_added(svn_wc_notify_state_t *s
>
> if (obstr_state != svn_wc_notify_state_inapplicable)
> {
> - if (state && merge_b->dry_run &&
> merge_b->added_path
> - && svn_dirent_is_child(merge_b->added_path,
> local_abspath, NULL))
> + if (state && merge_b->dry_run
> + && dry_run_added_parent_p(merge_b, local_relpath,
> scratch_pool))
> {
> *state = svn_wc_notify_state_changed;
> }
> else if (state)
> - *state = obstr_state;
> + {
> + *state = obstr_state;
> + }
> return SVN_NO_ERROR;
> }
>
> @@ -2358,9 +2386,10 @@ merge_dir_added(svn_wc_notify_state_t *s
> /* Unversioned or schedule-delete */
> if (merge_b->dry_run)
> {
> - merge_b->added_path = apr_pstrdup(merge_b->pool,
> local_abspath);
> - apr_hash_set(merge_b->dry_run_added, merge_b->added_path,
> - APR_HASH_KEY_STRING, merge_b->added_path);
> + const char *added_path = apr_pstrdup(merge_b->pool,
> + local_abspath);
> + apr_hash_set(merge_b->dry_run_added, added_path,
> + APR_HASH_KEY_STRING, added_path);
> }
> else
> {
> @@ -2385,15 +2414,22 @@ merge_dir_added(svn_wc_notify_state_t *s
> /* The dir is not known to Subversion, or is schedule-delete.
> * We will make it schedule-add. */
> if (!merge_b->dry_run)
> - SVN_ERR(svn_wc_add4(merge_b->ctx->wc_ctx, local_abspath,
> - svn_depth_infinity,
> - copyfrom_url, copyfrom_rev,
> - merge_b->ctx->cancel_func,
> - merge_b->ctx->cancel_baton,
> - NULL, NULL, /* no notification func! */
> - scratch_pool));
> + {
> + SVN_ERR(svn_wc_add4(merge_b->ctx->wc_ctx, local_abspath,
> + svn_depth_infinity,
> + copyfrom_url, copyfrom_rev,
> + merge_b->ctx->cancel_func,
> + merge_b->ctx->cancel_baton,
> + NULL, NULL, /* no notification func! */
> + scratch_pool));
> + }
> else
> - merge_b->added_path = apr_pstrdup(merge_b->pool,
> local_abspath);
> + {
> + const char *added_path = apr_pstrdup(merge_b->pool,
> + local_abspath);
> + apr_hash_set(merge_b->dry_run_added, added_path,
> + APR_HASH_KEY_STRING, added_path);
> + }
> if (state)
> *state = svn_wc_notify_state_changed;
> }
> @@ -2431,9 +2467,6 @@ merge_dir_added(svn_wc_notify_state_t *s
> }
> break;
> case svn_node_file:
> - if (merge_b->dry_run)
> - merge_b->added_path = NULL;
> -
> if (is_versioned && dry_run_deleted_p(merge_b, local_abspath))
> {
> /* ### TODO: Retain record of this dir being added to
> @@ -2455,8 +2488,6 @@ merge_dir_added(svn_wc_notify_state_t *s
> }
> break;
> default:
> - if (merge_b->dry_run)
> - merge_b->added_path = NULL;
> if (state)
> *state = svn_wc_notify_state_unknown;
> break;
> @@ -9163,7 +9194,6 @@ do_merge(apr_hash_t **modified_subtrees,
> be reset for each merge source iteration. */
> merge_cmd_baton.merge_source = *source;
> merge_cmd_baton.implicit_src_gap = NULL;
> - merge_cmd_baton.added_path = NULL;
> merge_cmd_baton.add_necessitated_merge = FALSE;
> merge_cmd_baton.dry_run_deletions =
> dry_run ? apr_hash_make(iterpool) : NULL;
>
Re: r1414810 - dry_run_added_parent_p() - libsvn_client/merge.c
Posted by "C. Michael Pilato" <cm...@collab.net>.
On 11/28/2012 02:36 PM, Julian Foad wrote:
> C. Michael Pilato wrote:
>
>> On 11/28/2012 01:44 PM, Julian Foad wrote:
>>> If the logic is that the list of added paths is not necessarily contain
>>> all leaf nodes, then doesn't it follow that dry_run_deleted_p() also
>>> needs to be updated to check whether any parent of the path being checked
>>> is in the list?
>>
>> That's not the logic, but your question about whether dry_run_deleted_p()
>> needs a similar fix is still valid. I don't believe a change is necessary,
>> though, because it not possible with Ev1 to even convey a nested deletion.
>
> Oops, I meant to ask about dry_run_added_p().
Ah. No, that function should remain the same.
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Enterprise Cloud Development
Re: r1414810 - dry_run_added_parent_p() - libsvn_client/merge.c
Posted by Julian Foad <ju...@btopenworld.com>.
C. Michael Pilato wrote:
> On 11/28/2012 01:44 PM, Julian Foad wrote:
>> If the logic is that the list of added paths is not necessarily contain
>> all leaf nodes, then doesn't it follow that dry_run_deleted_p() also
>> needs to be updated to check whether any parent of the path being checked
>> is in the list?
>
> That's not the logic, but your question about whether dry_run_deleted_p()
> needs a similar fix is still valid. I don't believe a change is necessary,
> though, because it not possible with Ev1 to even convey a nested deletion.
Oops, I meant to ask about dry_run_added_p().
- Julian
Re: r1414810 - dry_run_added_parent_p() - libsvn_client/merge.c
Posted by "C. Michael Pilato" <cm...@collab.net>.
On 11/28/2012 01:44 PM, Julian Foad wrote:
> If the logic is that the list of added paths is not necessarily contain
> all leaf nodes, then doesn't it follow that dry_run_deleted_p() also
> needs to be updated to check whether any parent of the path being checked
> is in the list?
That's not the logic, but your question about whether dry_run_deleted_p()
needs a similar fix is still valid. I don't believe a change is necessary,
though, because it not possible with Ev1 to even convey a nested deletion.
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Enterprise Cloud Development