You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Daniel Shahaf <da...@elego.de> on 2011/03/27 00:30:45 UTC
[PATCH] issue #3469: tree conflict under a directory external
Issue #3469 is about having merge pull in the addition of A/mu when A/
is an external. (File modifications work and raise a tree conflict.)
The following patch attempts to address it by having merge_file_added()
skip the addition of files under a directory external. Thoughts or +1's?
Daniel
[[[
Resolve issue #3469 (tree conflict under a directory external).
* subversion/libsvn_client/merge.c
(merge_file_added):
Skip files that belong to externals or to disjoint working copies.
TODO: wrap the comment
TODO: remove @XFail decorator
(from tree_conflict_tests.py 22)
]]]
[[[
Index: subversion/libsvn_client/merge.c
===================================================================
--- subversion/libsvn_client/merge.c (revision 1085436)
+++ subversion/libsvn_client/merge.c (working copy)
@@ -1531,6 +1531,10 @@ merge_file_added(const char *local_dir_abspath,
int i;
apr_hash_t *file_props;
+ SVN_DBG(("merge_file_added(): local_dir_abspath='%s'\n", local_dir_abspath));
+ SVN_DBG(("merge_file_added(): mine_abspath='%s'\n", mine_abspath));
+ SVN_DBG(("merge_file_added(): target_abspath='%s'\n", merge_b->target_abspath));
+
SVN_ERR_ASSERT(svn_dirent_is_absolute(mine_abspath));
/* Easy out: We are only applying mergeinfo differences. */
@@ -1555,6 +1559,21 @@ merge_file_added(const char *local_dir_abspath,
if (tree_conflicted)
*tree_conflicted = FALSE;
+ /* Easy out: not the same working copy. (So, a disjoint working copy or an external.) */
+ {
+ const char *mine_wcroot_abspath;
+ const char *target_wcroot_abspath;
+ SVN_ERR(svn_wc_get_wc_root(&mine_wcroot_abspath, merge_b->ctx->wc_ctx,
+ mine_abspath, scratch_pool, scratch_pool));
+ SVN_ERR(svn_wc_get_wc_root(&target_wcroot_abspath, merge_b->ctx->wc_ctx,
+ merge_b->target_abspath, scratch_pool, scratch_pool));
+ if (strcmp(mine_wcroot_abspath, target_wcroot_abspath))
+ {
+ *content_state = svn_wc_notify_state_obstructed;
+ return SVN_NO_ERROR;
+ }
+ }
+
/* Apply the prop changes to a new hash table. */
file_props = apr_hash_copy(subpool, original_props);
for (i = 0; i < prop_changes->nelts; ++i)
]]]
Re: [PATCH] issue #3469: tree conflict under a directory external
Posted by Daniel Shahaf <da...@elego.de>.
'Daniel Shahaf' wrote on Mon, Mar 28, 2011 at 00:16:49 +0200:
> Bert Huijben wrote on Sun, Mar 27, 2011 at 19:26:50 +0200:
> > and there are easier (and more
> > performant) ways to check if a node is part of a working copy (e.g. helpers
> > to check if a path is switched/a wc root).
>
> Could you elaborate on these please? I looked through the header files
> and amn't sure which helpers you're referring to.
New patch, including your previous feedbacks modulo the above point:
[[[
Resolve issue #3469 (tree conflict under a directory external).
Review by: rhuijben
* subversion/libsvn_client/merge.c
(merge_cmd_baton_t): Add TARGET_WCROOT_ABSPATH member.
(do_merge): Initialize new member.
(merge_file_added):
Skip files that belong to externals or to disjoint working copies.
Exploit knowledge of libsvn_wc implementations details (queries on
directories are faster) to make the check cheaper.
TODO: remove @XFail decorator
(from tree_conflict_tests.py 22)
]]]
[[[
Index: subversion/libsvn_client/merge.c
===================================================================
--- subversion/libsvn_client/merge.c (revision 1086475)
+++ subversion/libsvn_client/merge.c (working copy)
@@ -249,6 +249,8 @@ typedef struct merge_cmd_baton_t {
versioned dir (dry-run only) */
const char *target_abspath; /* Absolute working copy target of
the merge. */
+ const char *target_wcroot_abspath; /* Absolute path to root of wc that
+ TARGET_ABSPATH belongs to. */
/* The left and right URLs and revs. The value of this field changes to
reflect the merge_source_t *currently* being merged by do_merge(). */
@@ -1555,6 +1557,32 @@ merge_file_added(const char *local_dir_abspath,
if (tree_conflicted)
*tree_conflicted = FALSE;
+ /* Easy out: not the same working copy. (So, a disjoint working copy or
+ an external.) */
+ {
+ const char *mine_wcroot_abspath;
+
+ /* ### White-box optimization:
+
+ The code knows that MINE_ABSPATH is not a directory (it's an added
+ file), and we know that internally libsvn_wc queries are faster for
+ directories (this is an implementation detail). Therefore, query for
+ the wcroot of the containing directory of MINE_ABSPATH.
+
+ (We rely on the implementation detail only for performance, not for
+ correctness; under any implementation it would be valid to query for
+ the parent's wcroot.)
+ */
+ SVN_ERR(svn_wc_get_wc_root(&mine_wcroot_abspath, merge_b->ctx->wc_ctx,
+ svn_dirent_dirname(mine_abspath, scratch_pool),
+ scratch_pool, scratch_pool));
+ if (strcmp(mine_wcroot_abspath, merge_b->target_wcroot_abspath))
+ {
+ *content_state = svn_wc_notify_state_obstructed;
+ return SVN_NO_ERROR;
+ }
+ }
+
/* Apply the prop changes to a new hash table. */
file_props = apr_hash_copy(subpool, original_props);
for (i = 0; i < prop_changes->nelts; ++i)
@@ -8653,6 +8685,9 @@ do_merge(apr_hash_t **modified_subtrees,
merge_cmd_baton.target_missing_child = FALSE;
merge_cmd_baton.reintegrate_merge = reintegrate_merge;
merge_cmd_baton.target_abspath = target_abspath;
+ SVN_ERR(svn_wc_get_wc_root(&merge_cmd_baton.target_wcroot_abspath,
+ ctx->wc_ctx, merge_cmd_baton.target_abspath,
+ pool, subpool));
merge_cmd_baton.pool = subpool;
merge_cmd_baton.merge_options = merge_options;
merge_cmd_baton.diff3_cmd = diff3_cmd;
]]]
Re: [PATCH] issue #3469: tree conflict under a directory external
Posted by 'Daniel Shahaf' <da...@elego.de>.
Bert Huijben wrote on Sun, Mar 27, 2011 at 19:26:50 +0200:
> > -----Original Message-----
> > From: Daniel Shahaf [mailto:danielsh@elego.de]
> > Sent: zondag 27 maart 2011 0:31
> > To: dev@subversion.apache.org
> > Subject: [PATCH] issue #3469: tree conflict under a directory external
> >
> > Issue #3469 is about having merge pull in the addition of A/mu when A/
> > is an external. (File modifications work and raise a tree conflict.)
> >
> > The following patch attempts to address it by having merge_file_added()
> > skip the addition of files under a directory external. Thoughts or +1's?
>
> Your patch contains SVN_DBG() statements
I know, and I intend to remove them before commiting. Thanks for
pointing it out.
> and there are easier (and more
> performant) ways to check if a node is part of a working copy (e.g. helpers
> to check if a path is switched/a wc root).
Could you elaborate on these please? I looked through the header files
and amn't sure which helpers you're referring to.
I think we have to the disk in this case --- since if the file being
added (or any parent thereof) is obstructed, we shouldn't modify the
disk.
> I would drop the term 'disjoint' as it doesn't really apply after moving to
> a single-db working copy system.
>
Not sure what you mean, it's definitely possible to do
svn co URL1 foo && svn co URL2 foo/bar/
in 1.7. If that's not supported, we should make error out (just like we
did for 'svnadmin create' some time ago).
> This patch introduces +- 2 database transactions for every node tested,
> while the wc-root of the merge-target will never change (and will certainly
> be the merge target or an ancestor of it).
>
Fair enough; I'll make the next iteration of the patch cache
target_wcroot_abspath in the baton.
> If you want the cheapest check you should check if the directory of the file
> has a working copy root above (or at) the merge-target.
> (Performing wc_db queries on a directory is generally cheaper than for files
> as we keep a record for each directory in a hashtable and currently a file
> can never be in a diffent working copy then it's directory)
>
In other words: whatever check I do (be it the "get wc root" I have now
or something else, per above), I should do it on the dirname() of the
file being added. Thanks for the tip, I'll make this change in the next
iteration of the patch.
> Bert
>
Thanks for the review. I'll incorporate the two optimizations you
suggested and re-submit the patch once you've clarified the point about
a more performant way to detect a wc root.
Daniel
RE: [PATCH] issue #3469: tree conflict under a directory external
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: Daniel Shahaf [mailto:danielsh@elego.de]
> Sent: zondag 27 maart 2011 0:31
> To: dev@subversion.apache.org
> Subject: [PATCH] issue #3469: tree conflict under a directory external
>
> Issue #3469 is about having merge pull in the addition of A/mu when A/
> is an external. (File modifications work and raise a tree conflict.)
>
> The following patch attempts to address it by having merge_file_added()
> skip the addition of files under a directory external. Thoughts or +1's?
Your patch contains SVN_DBG() statements and there are easier (and more
performant) ways to check if a node is part of a working copy (e.g. helpers
to check if a path is switched/a wc root).
I would drop the term 'disjoint' as it doesn't really apply after moving to
a single-db working copy system.
This patch introduces +- 2 database transactions for every node tested,
while the wc-root of the merge-target will never change (and will certainly
be the merge target or an ancestor of it).
If you want the cheapest check you should check if the directory of the file
has a working copy root above (or at) the merge-target.
(Performing wc_db queries on a directory is generally cheaper than for files
as we keep a record for each directory in a hashtable and currently a file
can never be in a diffent working copy then it's directory)
Bert
>
> Daniel
<snip>