On Fri, Aug 21, 2009 at 01:29:50AM +0200, Stephen Butler wrote:
> Regarding your comment in 38848,
>
> Failing test merge_test 132:
> There is nothing that guarantees that the delete will come
> before the add. If you trace the merge done in merge_test 132
> and break at subversion/libsvn_client/merge.c:merge_file_added
> and subversion/libsvn_client/merge.c:merge_file_deleted, you
> can see that the add gets called first, then the delete (for
> the file "mu").
>
> That sounds to me like the delta editor driver is broken. Rule
> number one in svn_delta.h says,
>
> 1. The producer may call open_directory, add_directory, open_file,
> add_file at most once on any given directory entry. delete_entry may
> be called at most once on any given directory entry and may later be
> followed by add_directory or add_file on the same directory
> entry. delete_entry may not be called on any directory entry after
> open_directory, add_directory, open_file or add_file has been called
> on that directory entry.
>
> Is somebody calling add_file(mu), then delete_entry(mu)?
Yes! I checked it in the debugger, the add happens before the
delete. I think the culprit is in libsvn_repos/reporter.c, at
least for replacements. It has two lists, one for entries in
the source and one for entries in the target, and issues deletes
for items (identified by path) which are in source but not in target,
and then goes on to do a normal directory diff (during which the order
is, probably random, I haven't checked). That strategy breaks in the
replace case, I think, because the path appears on both sides
if there has been a replace.
I've tried the patch below but it doesn't seem to fix anything.
I don't know this code well so I might have missed something
or the approach I'm taking is totally wrong for a reason I don't
know. Will dig deeper into this tomorrow. I'm certain that merge_test
132 traverses this code path while replacing mu.
Any comments/help appreciated.
BTW, Greg told me that the convention about deletes coming first was
a convention in the docs but there was nothing in the code which
enforces it.
Stefan
Index: subversion/libsvn_repos/reporter.c
===================================================================
--- subversion/libsvn_repos/reporter.c (revision 38887)
+++ subversion/libsvn_repos/reporter.c (working copy)
@@ -1043,7 +1043,8 @@ delta_dirs(report_baton_t *b, svn_revnum_t s_rev,
}
/* Remove any deleted entries. Do this before processing the
- target, for graceful handling of case-only renames. */
+ target, for graceful handling of case-only renames, and
+ replacements. */
if (s_entries)
{
for (hi = apr_hash_first(pool, s_entries);
@@ -1051,33 +1052,29 @@ delta_dirs(report_baton_t *b, svn_revnum_t s_rev,
hi = apr_hash_next(hi))
{
const svn_fs_dirent_t *s_entry;
+ svn_revnum_t deleted_rev;
svn_pool_clear(subpool);
s_entry = svn_apr_hash_index_val(hi);
- if (apr_hash_get(t_entries, s_entry->name,
- APR_HASH_KEY_STRING) == NULL)
- {
- svn_revnum_t deleted_rev;
+ if (s_entry->kind == svn_node_file
+ && wc_depth < svn_depth_files)
+ continue;
- if (s_entry->kind == svn_node_file
- && wc_depth < svn_depth_files)
- continue;
+ if (s_entry->kind == svn_node_dir
+ && (wc_depth < svn_depth_immediates
+ || requested_depth == svn_depth_files))
+ continue;
- if (s_entry->kind == svn_node_dir
- && (wc_depth < svn_depth_immediates
- || requested_depth == svn_depth_files))
- continue;
-
- /* There is no corresponding target entry, so delete. */
- e_fullpath = svn_path_join(e_path, s_entry->name, subpool);
- SVN_ERR(svn_repos_deleted_rev(svn_fs_root_fs(b->t_root),
- svn_path_join(t_path,
- s_entry->name,
- subpool),
- s_rev, b->t_rev,
- &deleted_rev, subpool));
-
+ e_fullpath = svn_path_join(e_path, s_entry->name, subpool);
+ SVN_ERR(svn_repos_deleted_rev(svn_fs_root_fs(b->t_root),
+ svn_path_join(t_path,
+ s_entry->name,
+ subpool),
+ s_rev, b->t_rev,
+ &deleted_rev, subpool));
+ if (deleted_rev != SVN_INVALID_REVNUM)
+ {
SVN_ERR(b->editor->delete_entry(e_fullpath,
deleted_rev,
dir_baton, subpool));
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2385854
Received on 2009-08-21 02:04:31 CEST