You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@subversion.apache.org by cm...@apache.org on 2012/11/28 17:57:02 UTC
svn commit: r1414810 - /subversion/trunk/subversion/libsvn_client/merge.c
Author: cmpilato
Date: Wed Nov 28 16:57:01 2012
New Revision: 1414810
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*). Specifically,
account for situations where the editor drive looks like so:
add_dir("dir1")
add_dir("dir2")
add_file("dir1/file")
...
Formerly, this code would see the add of "dir1/file" and wish to flag
that as a obstruction because "dir1" wasn't on disk and also wasn't
the most recent directory added via the merge. ("dir2" was.)
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.
Modified:
subversion/trunk/subversion/libsvn_client/merge.c
Modified: subversion/trunk/subversion/libsvn_client/merge.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_client/merge.c?rev=1414810&r1=1414809&r2=1414810&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_client/merge.c (original)
+++ subversion/trunk/subversion/libsvn_client/merge.c Wed Nov 28 16:57:01 2012
@@ -260,9 +260,6 @@ typedef struct merge_cmd_baton_t {
SOURCES_ANCESTRAL is FALSE. */
svn_boolean_t reintegrate_merge; /* Whether this is a --reintegrate
merge or not. */
- const char *added_path; /* Set to the dir path whenever the
- dir is added as a child of a
- versioned dir (dry-run only) */
const merge_target_t *target; /* Description of merge target node */
/* The left and right URLs and revs. The value of this field changes to
@@ -490,6 +487,35 @@ dry_run_added_p(const merge_cmd_baton_t
APR_HASH_KEY_STRING) != NULL);
}
+/* 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
Re: r1414810 - dry_run_added_parent_p() - libsvn_client/merge.c
Posted by Julian Foad <ju...@btopenworld.com>.
> 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: svn commit: r1414810 - /subversion/trunk/subversion/libsvn_client/merge.c
Posted by "C. Michael Pilato" <cm...@collab.net>.
On 11/28/2012 12:19 PM, Bert Huijben wrote:
>> Teach the dry-run merge logic to account for additional flavors of
>> ra_serf editor drive ordering violations (*sigh*). Specifically,
>> account for situations where the editor drive looks like so:
>>
>> add_dir("dir1")
>> add_dir("dir2")
>> add_file("dir1/file")
>
> Are you sure the *editor drive ordering* is like that?
Strictly speaking, I suppose it was more like:
d1 = add_dir("dir1")
d2 = add_dir("dir2")
close_dir(d2)
f1 = add_file("dir1/file")
close_file(f1)
close_dir(d1)
> And this brings us back on the question: should we really drive merges on
> the diff editor.
>
> The v1 editor works, but it is not the ideal situation. And driving it on
> a v2 editor will be even harder as there are no clean open events to
> perform tree conflict tests.
ESCOPE. Great question for a different thread -- knock yourself out, man! :-)
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Enterprise Cloud Development
RE: svn commit: r1414810 - /subversion/trunk/subversion/libsvn_client/merge.c
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: cmpilato@apache.org [mailto:cmpilato@apache.org]
> Sent: woensdag 28 november 2012 17:57
> To: commits@subversion.apache.org
> Subject: svn commit: r1414810 -
> /subversion/trunk/subversion/libsvn_client/merge.c
>
> Author: cmpilato
> Date: Wed Nov 28 16:57:01 2012
> New Revision: 1414810
>
> 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*). Specifically,
> account for situations where the editor drive looks like so:
>
> add_dir("dir1")
> add_dir("dir2")
> add_file("dir1/file")
Are you sure the *editor drive ordering* is like that?
I'm pretty sure the add events are driven on the update report which is identical for all ra layers, while for serf the file and directory modifications may arrive later. It's just that the diff editor only reports the additions after close_file() and close_directory().
The result is the same, but we receive more information on the client, which we might use to avoid these problems...
And this brings us back on the question: should we really drive merges on the diff editor.
The v1 editor works, but it is not the ideal situation. And driving it on a v2 editor will be even harder as there are no clean open events to perform tree conflict tests.
> ...
>
> Formerly, this code would see the add of "dir1/file" and wish to flag
> that as a obstruction because "dir1" wasn't on disk and also wasn't
> the most recent directory added via the merge. ("dir2" was.)
>
> 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.
Bert
<snip>