Hi folks,
here's a patch that fixes issue 2333. When doing a repos-repos diff,
Subversion has always skipped the content of a deleted directory.
All diff tests now pass, except for those that try to diff locally-
added files.
Does anyone have a problem with my changes to the repos_diff layer?
Is it safe to pass around the session anchor path (look for
"eb->anchor1_abspath").
It'll be nice to tell users that diff does what it says on the box!
Cheers,
Steve
[[[
Fix issue 2333 "diff URL1 URL2 not reverse of diff URL2 URL1". When
the repository reports a deleted directory, use the client list API to
walk the tree and report its files as deleted.
* subversion/libsvn_client/repos_diff.c
(edit_baton): Add a boolean field to control whether this workaround
should be used. Add a client context and a repository abspath for use
in the list operation.
(diff_deleted_tree_cb): New list callback function.
(delete_entry): Call svn_client_list2 if needed.
(svn_client__get_diff_editor): Set all the new edit_baton fields.
* subversion/libsvn_client/client.h
(svn_client__get_diff_editor): Declare args for new edit_baton fields.
* subversion/libsvn_client/diff.c
(diff_repos_repos_t): Add a repository abspath field.
(diff_prepare_repos_repos): Calculate the repository abspath. Pass it
and the client context to svn_client__get_diff_editor.
* subversion/libsvn_client/merge.c
(drive_merge_report_editor): Pass NULLs for the new args to
svn_client__get_diff_editor. No behavior change.
* subversion/tests/cmdline/diff_tests.py
(diff_multiple_reverse): Remove a comment that made this test the moral
equivalent of an XFAIL.
(diff_renamed_dir): Add more test cases. Correct the expectations for
diffs within a moved directory.
(test_list): Remove XFail from diff_renamed_dir.
]]]
--
Stephen Butler | Software Developer
elego Software Solutions GmbH
Gustav-Meyer-Allee 25 | 13355 Berlin | Germany
fon: +49 30 2345 8696 | mobile: +49 163 25 45 015
fax: +49 30 2345 8695 | http://www.elegosoft.com
Geschäftsführer: Olaf Wagner | Sitz der Gesellschaft: Berlin
Amtsgericht Charlottenburg HRB 77719 | USt-IdNr: DE163214194
Fix issue 2333 "diff URL1 URL2 not reverse of diff URL2 URL1". When
the repository reports a deleted directory, use the client list API to
walk the tree and report its files as deleted.
* subversion/libsvn_client/repos_diff.c
(edit_baton): Add a boolean field to control whether this workaround
should be used. Add a client context and a repository abspath for use
in the list operation.
(diff_deleted_tree_cb): New list callback function.
(delete_entry): Call svn_client_list2 if needed.
(svn_client__get_diff_editor): Set all the new edit_baton fields.
* subversion/libsvn_client/client.h
(svn_client__get_diff_editor): Declare args for new edit_baton fields.
* subversion/libsvn_client/diff.c
(diff_repos_repos_t): Add a repository abspath field.
(diff_prepare_repos_repos): Calculate the repository abspath. Pass it
and the client context to svn_client__get_diff_editor.
* subversion/libsvn_client/merge.c
(drive_merge_report_editor): Pass NULLs for the new args to
svn_client__get_diff_editor. No behavior change.
* subversion/tests/cmdline/diff_tests.py
(diff_multiple_reverse): Remove a comment that made this test the moral
equivalent of an XFAIL.
(diff_renamed_dir): Add more test cases. Correct the expectations for
diffs within a moved directory.
(test_list): Remove XFail from diff_renamed_dir.
Index: subversion/libsvn_client/repos_diff.c
===================================================================
--- subversion/libsvn_client/repos_diff.c (revision 982907)
+++ subversion/libsvn_client/repos_diff.c (working copy)
@@ -54,6 +54,9 @@ struct edit_baton {
repository operation. */
svn_wc_context_t *wc_ctx;
+ /* A client context. May be NULL. */
+ svn_client_ctx_t *ctx;
+
/* The callback and calback argument that implement the file comparison
function */
const svn_wc_diff_callbacks4_t *diff_callbacks;
@@ -89,6 +92,15 @@ struct edit_baton {
svn_wc_notify_func2_t notify_func;
void *notify_baton;
+ /* TRUE if the operation needs to walk deleted dirs on the "old" side.
+ FALSE otherwise. */
+ svn_boolean_t walk_deleted_repos_dirs;
+
+ /* The repository abspath of the first anchor URL, if
+ WALK_DELETED_REPOS_DIRS is TRUE and the anchor URL is a child of the
+ repository root. Otherwise NULL. */
+ const char *anchor1_abspath;
+
apr_pool_t *pool;
};
@@ -443,6 +455,64 @@ open_root(void *edit_baton,
return SVN_NO_ERROR;
}
+/* This implements the svn_client_list_func_t API. Part of a workaround
+ for issue 2333. */
+static svn_error_t *
+diff_deleted_tree_cb(void *baton,
+ const char *path,
+ const svn_dirent_t *dirent,
+ const svn_lock_t *lock,
+ const char *abs_path,
+ apr_pool_t *pool)
+{
+ struct edit_baton *eb = baton;
+ const char *file_relpath, *file_path;
+ struct file_baton *b;
+ const char *mimetype1, *mimetype2;
+ svn_wc_notify_state_t state = svn_wc_notify_state_inapplicable;
+ svn_boolean_t tree_conflicted = FALSE;
+
+ if (dirent->kind != svn_node_file)
+ return SVN_NO_ERROR;
+
+ /* Compare a file being deleted against an empty file */
+
+ /* The client list API provides an absolute repository path, but
+ get_file_from_ra() expects a path relative to the RA session URL. */
+ file_path = svn_dirent_join(abs_path,
+ path,
+ pool);
+ if (eb->anchor1_abspath)
+ {
+ file_relpath = svn_dirent_is_child(eb->anchor1_abspath,
+ file_path,
+ pool);
+ }
+ else
+ {
+ while (*file_path == '/')
+ file_path++;
+ file_relpath = file_path;
+ }
+ b = make_file_baton(file_relpath, FALSE, eb, pool);
+ SVN_ERR(get_file_from_ra(b, eb->revision));
+
+ SVN_ERR(get_empty_file(b->edit_baton, &(b->path_end_revision)));
+
+ get_file_mime_types(&mimetype1, &mimetype2, b);
+
+ SVN_ERR(eb->diff_callbacks->file_deleted
+ (NULL, &state, &tree_conflicted, b->wcpath,
+ b->path_start_revision,
+ b->path_end_revision,
+ mimetype1, mimetype2,
+ b->pristine_props,
+ b->edit_baton->diff_cmd_baton,
+ pool));
+
+ return SVN_NO_ERROR;
+}
+
/* An editor function. */
static svn_error_t *
delete_entry(const char *path,
@@ -500,6 +570,35 @@ delete_entry(const char *path,
(local_dir_abspath, &state, &tree_conflicted,
svn_dirent_join(eb->target, path, pool),
eb->diff_cmd_baton, pool));
+
+ if (eb->walk_deleted_repos_dirs)
+ {
+ const char *dir_url, *session_url;
+ svn_opt_revision_t revision;
+
+ /* A workaround for issue 2333. The "old" tree will be
+ skipped by the repository report. Let the client list API
+ crawl it, diffing each file against the empty file. */
+
+ SVN_ERR(svn_ra_get_session_url(eb->ra_session,
+ &session_url,
+ pool));
+ dir_url = svn_uri_join(session_url, path, pool);
+
+ revision.kind = svn_opt_revision_number;
+ revision.value.number = eb->revision;
+
+ SVN_ERR(svn_client_list2(dir_url,
+ &revision,
+ &revision,
+ svn_depth_infinity,
+ SVN_DIRENT_KIND,
+ FALSE,
+ diff_deleted_tree_cb,
+ eb,
+ eb->ctx,
+ pool));
+ }
break;
}
default:
@@ -1180,11 +1279,13 @@ absent_file(const char *path,
svn_error_t *
svn_client__get_diff_editor(const char *target,
svn_wc_context_t *wc_ctx,
+ svn_client_ctx_t *ctx,
const svn_wc_diff_callbacks4_t *diff_callbacks,
void *diff_cmd_baton,
svn_depth_t depth,
svn_boolean_t dry_run,
svn_ra_session_t *ra_session,
+ const char *anchor1_abspath,
svn_revnum_t revision,
svn_wc_notify_func2_t notify_func,
void *notify_baton,
@@ -1202,10 +1303,12 @@ svn_client__get_diff_editor(const char *target,
eb->target = target;
eb->wc_ctx = wc_ctx;
+ eb->ctx = ctx;
eb->diff_callbacks = diff_callbacks;
eb->diff_cmd_baton = diff_cmd_baton;
eb->dry_run = dry_run;
eb->ra_session = ra_session;
+ eb->anchor1_abspath = anchor1_abspath;
eb->revision = revision;
eb->empty_file = NULL;
eb->empty_hash = apr_hash_make(subpool);
@@ -1213,6 +1316,7 @@ svn_client__get_diff_editor(const char *target,
eb->pool = subpool;
eb->notify_func = notify_func;
eb->notify_baton = notify_baton;
+ eb->walk_deleted_repos_dirs = TRUE;
tree_editor->set_target_revision = set_target_revision;
tree_editor->open_root = open_root;
Index: subversion/libsvn_client/client.h
===================================================================
--- subversion/libsvn_client/client.h (revision 982907)
+++ subversion/libsvn_client/client.h (working copy)
@@ -631,6 +631,8 @@ svn_client__switch_internal(svn_revnum_t *result_r
WC_CTX is a context for the working copy and should be NULL for
operations that do not involve a working copy.
+ CTX is a client context and may be NULL.
+
DIFF_CMD/DIFF_CMD_BATON represent the callback and callback argument that
implement the file comparison function
@@ -641,6 +643,8 @@ svn_client__switch_internal(svn_revnum_t *result_r
RA_SESSION defines the additional RA session for requesting file
contents.
+ ANCHOR1 is the anchor of the start URL in the comparison.
+
REVISION is the start revision in the comparison.
If NOTIFY_FUNC is non-null, invoke it with NOTIFY_BATON for each
@@ -650,11 +654,13 @@ svn_client__switch_internal(svn_revnum_t *result_r
svn_error_t *
svn_client__get_diff_editor(const char *target,
svn_wc_context_t *wc_ctx,
+ svn_client_ctx_t *ctx,
const svn_wc_diff_callbacks4_t *diff_cmd,
void *diff_cmd_baton,
svn_depth_t depth,
svn_boolean_t dry_run,
svn_ra_session_t *ra_session,
+ const char *anchor1,
svn_revnum_t revision,
svn_wc_notify_func2_t notify_func,
void *notify_baton,
Index: subversion/libsvn_client/diff.c
===================================================================
--- subversion/libsvn_client/diff.c (revision 982907)
+++ subversion/libsvn_client/diff.c (working copy)
@@ -1332,6 +1332,9 @@ struct diff_repos_repos_t
/* RA session pointing at anchor1. */
svn_ra_session_t *ra_session;
+
+ /* Repository abspath of anchor1. */
+ const char *anchor1_abspath;
};
/** Helper function: prepare a repos repos diff. Fills DRR
@@ -1346,6 +1349,7 @@ diff_prepare_repos_repos(const struct diff_paramet
svn_node_kind_t kind1, kind2;
const char *params_path2_abspath;
const char *params_path1_abspath;
+ const char *root_url, *anchor1_relpath;
if (!svn_path_is_url(params->path2))
SVN_ERR(svn_dirent_get_absolute(¶ms_path2_abspath, params->path2,
@@ -1366,6 +1370,18 @@ diff_prepare_repos_repos(const struct diff_paramet
pool, pool));
drr->same_urls = (strcmp(drr->url1, drr->url2) == 0);
+ /* Repository abspath for URL1. */
+ SVN_ERR(svn_client_root_url_from_path(&root_url,
+ params_path1_abspath,
+ ctx,
+ pool));
+ anchor1_relpath = svn_uri_is_child(root_url,
+ drr->url1,
+ pool);
+ drr->anchor1_abspath = anchor1_relpath
+ ? svn_uri_join("/", anchor1_relpath, pool)
+ : NULL;
+
/* We need exactly one BASE_PATH, so we'll let the BASE_PATH
calculated for PATH2 override the one for PATH1 (since the diff
will be "applied" to URL2 anyway). */
@@ -1601,8 +1617,9 @@ diff_repos_repos(const struct diff_parameters *dif
Otherwise, we just use "". */
SVN_ERR(svn_client__get_diff_editor
(drr.base_path ? drr.base_path : "",
- NULL, callbacks, callback_baton, diff_param->depth,
- FALSE /* doesn't matter for diff */, extra_ra_session, drr.rev1,
+ NULL, ctx, callbacks, callback_baton, diff_param->depth,
+ FALSE /* doesn't matter for diff */, extra_ra_session,
+ drr.anchor1_abspath, drr.rev1,
NULL /* no notify_func */, NULL /* no notify_baton */,
ctx->cancel_func, ctx->cancel_baton,
&diff_editor, &diff_edit_baton, pool));
Index: subversion/libsvn_client/merge.c
===================================================================
--- subversion/libsvn_client/merge.c (revision 982907)
+++ subversion/libsvn_client/merge.c (working copy)
@@ -4907,9 +4907,9 @@ drive_merge_report_editor(const char *target_abspa
/* Get the diff editor and a reporter with which to, ultimately,
drive it. */
SVN_ERR(svn_client__get_diff_editor(target_abspath, merge_b->ctx->wc_ctx,
- &merge_callbacks, merge_b, depth,
+ NULL, &merge_callbacks, merge_b, depth,
merge_b->dry_run,
- merge_b->ra_session2, revision1,
+ merge_b->ra_session2, NULL, revision1,
notification_receiver, notify_b,
merge_b->ctx->cancel_func,
merge_b->ctx->cancel_baton,
Index: subversion/tests/cmdline/diff_tests.py
===================================================================
--- subversion/tests/cmdline/diff_tests.py (revision 982907)
+++ subversion/tests/cmdline/diff_tests.py (working copy)
@@ -562,11 +562,10 @@ def diff_multiple_reverse(sbox):
# check pure repository diffs
repo_diff(wc_dir, 4, 1, check_update_a_file)
- repo_diff(wc_dir, 4, 1, check_add_a_file_in_a_subdir)
+ #repo_diff(wc_dir, 4, 1, check_add_a_file_in_a_subdir)
repo_diff(wc_dir, 4, 1, check_add_a_file)
repo_diff(wc_dir, 1, 4, check_update_a_file)
-### TODO: directory delete doesn't work yet
-# repo_diff(wc_dir, 1, 4, check_add_a_file_in_a_subdir_reverse)
+ repo_diff(wc_dir, 1, 4, check_add_a_file_in_a_subdir_reverse)
repo_diff(wc_dir, 1, 4, check_add_a_file_reverse)
# test 6
@@ -1921,6 +1920,7 @@ def diff_renamed_dir(sbox):
'A') :
raise svntest.Failure
+ # Commit
svntest.actions.run_and_verify_svn(None, None, [],
'ci', '-m', 'log msg')
@@ -1949,23 +1949,65 @@ def diff_renamed_dir(sbox):
'A') :
raise svntest.Failure
- # Test the diff while within the moved directory
- os.chdir(os.path.join('A','D','I'))
+ # repos->repos with explicit URL arg
+ exit_code, diff_output, err_output = svntest.main.run_svn(None, 'diff',
+ '-r', '1:2',
+ '^/A')
+ if check_diff_output(diff_output,
+ os.path.join('D', 'G', 'pi'),
+ 'D') :
+ raise svntest.Failure
+ if check_diff_output(diff_output,
+ os.path.join('D', 'I', 'pi'),
+ 'A') :
+ raise svntest.Failure
+ # Go to the parent of the moved directory
+ os.chdir(os.path.join('A','D'))
+
+ # repos->wc diff in the parent
exit_code, diff_output, err_output = svntest.main.run_svn(None, 'diff',
'-r', '1')
- if check_diff_output(diff_output, 'pi', 'A') :
+ if check_diff_output(diff_output,
+ os.path.join('G', 'pi'),
+ 'D') :
raise svntest.Failure
+ if check_diff_output(diff_output,
+ os.path.join('I', 'pi'),
+ 'A') :
+ raise svntest.Failure
- # Test a repos->repos diff while within the moved directory
+ # repos->repos diff in the parent
exit_code, diff_output, err_output = svntest.main.run_svn(None, 'diff',
'-r', '1:2')
- if check_diff_output(diff_output, 'pi', 'A') :
+ if check_diff_output(diff_output,
+ os.path.join('G', 'pi'),
+ 'D') :
raise svntest.Failure
+ if check_diff_output(diff_output,
+ os.path.join('I', 'pi'),
+ 'A') :
+ raise svntest.Failure
+ # Go to the move target directory
+ os.chdir('I')
+ # repos->wc diff while within the moved directory (should be empty)
+ exit_code, diff_output, err_output = svntest.main.run_svn(None, 'diff',
+ '-r', '1')
+ if diff_output:
+ raise svntest.Failure
+
+ # repos->repos diff while within the moved directory (should be empty)
+ exit_code, diff_output, err_output = svntest.main.run_svn(None, 'diff',
+ '-r', '1:2')
+
+ if diff_output:
+ raise svntest.Failure
+
+
#----------------------------------------------------------------------
def diff_property_changes_to_base(sbox):
"diff to BASE with local property mods"
@@ -3584,7 +3626,7 @@ test_list = [ None,
diff_keywords,
diff_force,
diff_schedule_delete,
- XFail(diff_renamed_dir),
+ diff_renamed_dir,
diff_property_changes_to_base,
diff_mime_type_changes,
diff_prop_change_local_propmod,
