You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Stephen Butler <sb...@elego.de> on 2010/08/06 10:21:27 UTC
[PATCH] Issue 2333 "diff URL1 URL2 not reverse of diff URL2 URL1"
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
RE: [PATCH] Issue 2333 "diff URL1 URL2 not reverse of diff URL2 URL1"
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: Stephen Butler [mailto:sbutler@elego.de]
> Sent: vrijdag 6 augustus 2010 12:21
> To: dev@subversion.apache.org
> Subject: [PATCH] Issue 2333 "diff URL1 URL2 not reverse of diff URL2
> URL1"
>
> 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.
> ]]]
Review inline.
> 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;
> +
ctx contains wc_ctx. Not sure if it useful to store both.
(But it is probably easier on the rest of the code)
> /* 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));
Instead of using svn_client_list2() which reopens a new ra session, determines the node kind, etc. you could keep an ra session cached in the baton. (If you parent the session at the root of your diff, you never have to reparent it and you don't have to work with an anchor abspath).
This would be *much* faster than svn_client_list2. Probably even if you have only one deleted directory.
Looking back at diff_deleted_tree_cb: Maybe you can just use the existing ra session with a single call to svn_ra_get_dir2().
> + }
> 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,
No need to pass both. wc_ctx is part of ctx and with a libsvn_client internal api I think you can assume that you have a ctx.
(And if you are changing the prototype anyway I would move ctx to before target, to match other new code)
> 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.
> +
You are working in libsvn_client. I think you can just make it always set.
Bert