You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@subversion.apache.org by st...@apache.org on 2012/05/14 19:03:13 UTC
svn commit: r1338291 - in /subversion/trunk/subversion: libsvn_client/diff.c
tests/cmdline/log_tests.py
Author: stsp
Date: Mon May 14 17:03:13 2012
New Revision: 1338291
URL: http://svn.apache.org/viewvc?rev=1338291&view=rev
Log:
Rewrite the client-side diff code for showing added/deleted diff targets,
fixing inconsistencies with paths shown in diff output.
For related discussion, see this #svn-dev IRC conversation at
http://colabti.org/irclogger/irclogger_log/svn-dev?date=2012-05-03#l726
and also this thread on the dev@ list:
Date: Mon, 19 Mar 2012 21:54:58 +0100
From: Dmitry Pavlenko
To: dev@
Subject: paths in diff output (possible bug)
Message-Id: <20...@tmatesoft.com>
http://svn.haxx.se/dev/archive-2012-03/0385.shtml
* subversion/libsvn_client/diff.c
(diff_prepare_repos_repos): Remove the hack of finding an existing common
ancestor if one target does not exist. Instead, return node kinds of both
diff targets to allow the caller to handle the handle this case instead.
(make_regular_props_array): New helper that transforms a prop-hash retrieved
from the RA layer into a changed-props array expected by diff callbacks.
(diff_repos_repos_added_or_deleted_file,
diff_repos_repos_added_or_deleted_dir,
diff_repos_repos_added_or_deleted_target): New functions that handle added
and deleted diff targets during repos<->repos diffs. Files content is
retrieved from the RA layer and diffed against the empty file via the
diff callabacks. File and directory properties are reported as added or
deleted to the diff callbacks.
(diff_repos_repos, diff_summarize_repos_repos): If one of the diff targets
does not exist, invoke the new diff_repos_repos_added_or_deleted_target()
function instead of using the diff editor.
* subversion/tests/cmdline/log_tests.py
(log_diff_moved): Adjust expected output and remove XFail for DAV RA layers.
This test now passes over all RA layers.
Modified:
subversion/trunk/subversion/libsvn_client/diff.c
subversion/trunk/subversion/tests/cmdline/log_tests.py
Modified: subversion/trunk/subversion/libsvn_client/diff.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_client/diff.c?rev=1338291&r1=1338290&r2=1338291&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_client/diff.c (original)
+++ subversion/trunk/subversion/libsvn_client/diff.c Mon May 14 17:03:13 2012
@@ -1545,6 +1545,8 @@ resolve_pegged_diff_target_url(const cha
* Return URLs and peg revisions in *URL1, *REV1 and in *URL2, *REV2.
* Return suitable anchors in *ANCHOR1 and *ANCHOR2, and targets in
* *TARGET1 and *TARGET2, based on *URL1 and *URL2.
+ * Indicate the corresponding node kinds in *KIND1 and *KIND2, and verify
+ * that at least one of the diff targets exists.
* Set *BASE_PATH corresponding to the URL opened in the new *RA_SESSION
* which is pointing at *ANCHOR1.
* Use client context CTX. Do all allocations in POOL. */
@@ -1558,6 +1560,8 @@ diff_prepare_repos_repos(const char **ur
const char **anchor2,
const char **target1,
const char **target2,
+ svn_node_kind_t *kind1,
+ svn_node_kind_t *kind2,
svn_ra_session_t **ra_session,
svn_client_ctx_t *ctx,
const char *path_or_url1,
@@ -1567,7 +1571,6 @@ diff_prepare_repos_repos(const char **ur
const svn_opt_revision_t *peg_revision,
apr_pool_t *pool)
{
- svn_node_kind_t kind1, kind2;
const char *abspath_or_url2;
const char *abspath_or_url1;
@@ -1644,18 +1647,18 @@ diff_prepare_repos_repos(const char **ur
SVN_ERR(svn_client__get_revision_number(rev2, NULL, ctx->wc_ctx,
(path_or_url2 == *url2) ? NULL : abspath_or_url2,
*ra_session, revision2, pool));
- SVN_ERR(svn_ra_check_path(*ra_session, "", *rev2, &kind2, pool));
+ SVN_ERR(svn_ra_check_path(*ra_session, "", *rev2, kind2, pool));
/* Do the same for the first target. */
SVN_ERR(svn_ra_reparent(*ra_session, *url1, pool));
SVN_ERR(svn_client__get_revision_number(rev1, NULL, ctx->wc_ctx,
(strcmp(path_or_url1, *url1) == 0) ? NULL : abspath_or_url1,
*ra_session, revision1, pool));
- SVN_ERR(svn_ra_check_path(*ra_session, "", *rev1, &kind1, pool));
+ SVN_ERR(svn_ra_check_path(*ra_session, "", *rev1, kind1, pool));
/* Either both URLs must exist at their respective revisions,
* or one of them may be missing from one side of the diff. */
- if (kind1 == svn_node_none && kind2 == svn_node_none)
+ if (*kind1 == svn_node_none && *kind2 == svn_node_none)
{
if (strcmp(*url1, *url2) == 0)
return svn_error_createf(SVN_ERR_FS_NOT_FOUND, NULL,
@@ -1669,9 +1672,9 @@ diff_prepare_repos_repos(const char **ur
"'%ld'"),
*url1, *url2, *rev1, *rev2);
}
- else if (kind1 == svn_node_none)
+ else if (*kind1 == svn_node_none)
SVN_ERR(check_diff_target_exists(*url1, *rev2, *rev1, *ra_session, pool));
- else if (kind2 == svn_node_none)
+ else if (*kind2 == svn_node_none)
SVN_ERR(check_diff_target_exists(*url2, *rev1, *rev2, *ra_session, pool));
/* Choose useful anchors and targets for our two URLs. */
@@ -1679,57 +1682,9 @@ diff_prepare_repos_repos(const char **ur
*anchor2 = *url2;
*target1 = "";
*target2 = "";
- if ((kind1 == svn_node_none) || (kind2 == svn_node_none))
- {
- svn_node_kind_t kind;
- const char *repos_root;
- const char *new_anchor;
- svn_revnum_t rev;
-
- /* The diff target does not exist on one side of the diff.
- * This can happen if the target was added or deleted within the
- * revision range being diffed.
- * However, we don't know how deep within a added/deleted subtree the
- * diff target is. Find a common parent that exists on both sides of
- * the diff and use it as anchor for the diff operation.
- *
- * ### This can fail due to authz restrictions (like in issue #3242).
- * ### But it is the only option we have right now to try to get
- * ### a usable diff in this situation. */
-
- SVN_ERR(svn_ra_get_repos_root2(*ra_session, &repos_root, pool));
-
- /* Since we already know that one of the URLs does exist,
- * look for an existing parent of the URL which doesn't exist. */
- new_anchor = (kind1 == svn_node_none ? *anchor1 : *anchor2);
- rev = (kind1 == svn_node_none ? *rev1 : *rev2);
- do
- {
- if (strcmp(new_anchor, repos_root) != 0)
- {
- new_anchor = svn_path_uri_decode(svn_uri_dirname(new_anchor,
- pool),
- pool);
- if (*base_path)
- *base_path = svn_dirent_dirname(*base_path, pool);
- }
-
- SVN_ERR(svn_ra_reparent(*ra_session, new_anchor, pool));
- SVN_ERR(svn_ra_check_path(*ra_session, "", rev, &kind, pool));
- }
- while (kind != svn_node_dir);
- *anchor1 = *anchor2 = new_anchor;
- /* Diff targets must be relative to the new anchor. */
- *target1 = svn_uri_skip_ancestor(new_anchor, *url1, pool);
- *target2 = svn_uri_skip_ancestor(new_anchor, *url2, pool);
- SVN_ERR_ASSERT(*target1 && *target2);
- if (kind1 == svn_node_none)
- kind1 = svn_node_dir;
- else
- kind2 = svn_node_dir;
- }
- else if ((kind1 == svn_node_file) || (kind2 == svn_node_file))
+ /* If one of the targets is a file, use the parent directory as anchor. */
+ if (*kind1 == svn_node_file || *kind2 == svn_node_file)
{
svn_uri_split(anchor1, target1, *url1, pool);
svn_uri_split(anchor2, target2, *url2, pool);
@@ -2334,6 +2289,276 @@ diff_wc_wc(const char *path1,
return SVN_NO_ERROR;
}
+/* Create an array of regular properties in PROP_HASH, filtering entry-props
+ * and wc-props. Allocate the returned array in RESULT_POOL.
+ * Use SCRATCH_POOL for temporary allocations. */
+static apr_array_header_t *
+make_regular_props_array(apr_hash_t *prop_hash,
+ apr_pool_t *result_pool,
+ apr_pool_t *scratch_pool)
+{
+ apr_array_header_t *regular_props;
+ apr_hash_index_t *hi;
+
+ regular_props = apr_array_make(result_pool, 0, sizeof(svn_prop_t));
+ for (hi = apr_hash_first(scratch_pool, prop_hash); hi;
+ hi = apr_hash_next(hi))
+ {
+ const char *name = svn__apr_hash_index_key(hi);
+ svn_string_t *value = svn__apr_hash_index_val(hi);
+ svn_prop_kind_t prop_kind = svn_property_kind2(name);
+
+ if (prop_kind == svn_prop_regular_kind)
+ {
+ svn_prop_t *prop = apr_palloc(scratch_pool, sizeof(svn_prop_t));
+
+ prop->name = name;
+ prop->value = value;
+ APR_ARRAY_PUSH(regular_props, svn_prop_t) = *prop;
+ }
+ }
+
+ return regular_props;
+}
+
+/* Handle an added or deleted diff target file for a repos<->repos diff.
+ *
+ * Using the provided diff CALLBACKS and the CALLBACK_BATON, show the file
+ * TARGET@PEG_REVISION as added or deleted, depending on SHOW_DELETION.
+ * TARGET is a path relative to RA_SESSION's URL.
+ * REV1 and REV2 are the revisions being compared.
+ * Use SCRATCH_POOL for temporary allocations. */
+static svn_error_t *
+diff_repos_repos_added_or_deleted_file(const char *target,
+ svn_revnum_t peg_revision,
+ svn_revnum_t rev1,
+ svn_revnum_t rev2,
+ svn_boolean_t show_deletion,
+ const char *empty_file,
+ const svn_wc_diff_callbacks4_t
+ *callbacks,
+ struct diff_cmd_baton *callback_baton,
+ svn_ra_session_t *ra_session,
+ apr_pool_t *scratch_pool)
+{
+ apr_file_t *file;
+ const char *file_abspath;
+ svn_stream_t *content;
+ apr_hash_t *prop_hash;
+
+ /* ### There is no way to flush content to disk without access to the bare
+ * ### file handle so we re-implement svn_stream_open_unique() here.
+ * ### Do we need something like svn_stream_flush()? */
+ SVN_ERR(svn_io_open_unique_file3(&file, &file_abspath, NULL,
+ svn_io_file_del_on_close,
+ scratch_pool, scratch_pool));
+ content = svn_stream_from_aprfile2(file, FALSE, scratch_pool);
+ SVN_ERR(svn_ra_get_file(ra_session, target, peg_revision, content, NULL,
+ &prop_hash, scratch_pool));
+ SVN_ERR(svn_io_file_flush_to_disk(file, scratch_pool));
+
+ if (show_deletion)
+ {
+ SVN_ERR(callbacks->file_deleted(NULL, NULL,
+ target, file_abspath, empty_file,
+ apr_hash_get(prop_hash,
+ SVN_PROP_MIME_TYPE,
+ APR_HASH_KEY_STRING),
+ NULL, prop_hash, callback_baton,
+ scratch_pool));
+ }
+ else
+ {
+ SVN_ERR(callbacks->file_added(NULL, NULL, NULL,
+ target, empty_file, file_abspath,
+ rev1, rev2, NULL,
+ apr_hash_get(prop_hash, SVN_PROP_MIME_TYPE,
+ APR_HASH_KEY_STRING),
+ NULL, SVN_INVALID_REVNUM,
+ make_regular_props_array(prop_hash,
+ scratch_pool,
+ scratch_pool),
+ NULL, callback_baton, scratch_pool));
+ }
+
+ SVN_ERR(svn_stream_close(content));
+
+ return SVN_NO_ERROR;
+}
+
+/* Handle an added or deleted diff target directory for a repos<->repos diff.
+ *
+ * Using the provided diff CALLBACKS and the CALLBACK_BATON, show the
+ * directory TARGET@PEG_REVISION, and all of its children, as added or deleted,
+ * depending on SHOW_DELETION. TARGET is a path relative to RA_SESSION's URL.
+ * REV1 and REV2 are the revisions being compared.
+ * Use SCRATCH_POOL for temporary allocations. */
+static svn_error_t *
+diff_repos_repos_added_or_deleted_dir(const char *target,
+ svn_revnum_t revision,
+ svn_revnum_t rev1,
+ svn_revnum_t rev2,
+ svn_boolean_t show_deletion,
+ const char *empty_file,
+ const svn_wc_diff_callbacks4_t
+ *callbacks,
+ struct diff_cmd_baton *callback_baton,
+ svn_ra_session_t *ra_session,
+ apr_pool_t *scratch_pool)
+{
+ apr_hash_t *dirents;
+ apr_hash_t *props;
+ apr_pool_t *iterpool;
+ apr_hash_index_t *hi;
+
+ SVN_ERR(svn_ra_get_dir2(ra_session, &dirents, NULL, &props,
+ target, revision, SVN_DIRENT_KIND,
+ scratch_pool));
+
+ if (show_deletion)
+ SVN_ERR(callbacks->dir_deleted(NULL, NULL, target, callback_baton,
+ scratch_pool));
+ else
+ SVN_ERR(callbacks->dir_added(NULL, NULL, NULL, NULL,
+ target, revision,
+ NULL, SVN_INVALID_REVNUM,
+ callback_baton, scratch_pool));
+ if (props)
+ {
+ if (show_deletion)
+ SVN_ERR(callbacks->dir_props_changed(NULL, NULL, target, FALSE,
+ apr_array_make(scratch_pool, 0,
+ sizeof(svn_prop_t)),
+ props, callback_baton,
+ scratch_pool));
+ else
+ SVN_ERR(callbacks->dir_props_changed(NULL, NULL, target, TRUE,
+ make_regular_props_array(
+ props, scratch_pool,
+ scratch_pool),
+ NULL,
+ callback_baton, scratch_pool));
+ }
+
+ iterpool = svn_pool_create(scratch_pool);
+ for (hi = apr_hash_first(scratch_pool, dirents); hi; hi = apr_hash_next(hi))
+ {
+ const char *name = svn__apr_hash_index_key(hi);
+ svn_dirent_t *dirent = svn__apr_hash_index_val(hi);
+ const char *child_target;
+
+ svn_pool_clear(iterpool);
+
+ child_target = svn_relpath_join(target, name, iterpool);
+
+ if (dirent->kind == svn_node_dir)
+ SVN_ERR(diff_repos_repos_added_or_deleted_dir(child_target,
+ revision, rev1, rev2,
+ show_deletion,
+ empty_file,
+ callbacks,
+ callback_baton,
+ ra_session,
+ iterpool));
+ else if (dirent->kind == svn_node_file)
+ SVN_ERR(diff_repos_repos_added_or_deleted_file(child_target,
+ revision, rev1, rev2,
+ show_deletion,
+ empty_file,
+ callbacks,
+ callback_baton,
+ ra_session,
+ scratch_pool));
+ }
+ svn_pool_destroy(iterpool);
+
+ if (!show_deletion)
+ SVN_ERR(callbacks->dir_closed(NULL, NULL, NULL, target, TRUE,
+ callback_baton, scratch_pool));
+
+ return SVN_NO_ERROR;
+}
+
+
+/* Handle an added or deleted diff target for a repos<->repos diff.
+ *
+ * Using the provided diff CALLBACKS and the CALLBACK_BATON, show
+ * TARGET@PEG_REVISION, and all of its children, if any, as added or deleted.
+ * TARGET is a path relative to RA_SESSION's URL.
+ * REV1 and REV2 are the revisions being compared.
+ * Use SCRATCH_POOL for temporary allocations. */
+static svn_error_t *
+diff_repos_repos_added_or_deleted_target(const char *target1,
+ const char *target2,
+ svn_revnum_t rev1,
+ svn_revnum_t rev2,
+ svn_node_kind_t kind1,
+ svn_node_kind_t kind2,
+ const svn_wc_diff_callbacks4_t
+ *callbacks,
+ struct diff_cmd_baton *callback_baton,
+ svn_ra_session_t *ra_session,
+ apr_pool_t *scratch_pool)
+{
+ const char *existing_target;
+ svn_revnum_t existing_rev;
+ svn_node_kind_t existing_kind;
+ svn_boolean_t show_deletion;
+ const char *empty_file;
+
+ SVN_ERR_ASSERT(kind1 == svn_node_none || kind2 == svn_node_none);
+
+ /* Are we showing an addition or deletion? */
+ show_deletion = (kind2 == svn_node_none);
+
+ /* Which target is being added/deleted? Is it a file or a directory? */
+ if (show_deletion)
+ {
+ existing_target = target1;
+ existing_rev = rev1;
+ existing_kind = kind1;
+ }
+ else
+ {
+ existing_target = target2;
+ existing_rev = rev2;
+ existing_kind = kind2;
+ }
+
+ /* All file content will be diffed against the empty file. */
+ SVN_ERR(svn_io_open_unique_file3(NULL, &empty_file, NULL,
+ svn_io_file_del_on_pool_cleanup,
+ scratch_pool, scratch_pool));
+
+ if (existing_kind == svn_node_file)
+ {
+ /* Get file content and show a diff against the empty file. */
+ SVN_ERR(diff_repos_repos_added_or_deleted_file(existing_target,
+ existing_rev,
+ rev1, rev2,
+ show_deletion,
+ empty_file,
+ callbacks,
+ callback_baton,
+ ra_session,
+ scratch_pool));
+ }
+ else
+ {
+ /* Walk the added/deleted tree and show a diff for each child. */
+ SVN_ERR(diff_repos_repos_added_or_deleted_dir(existing_target,
+ existing_rev,
+ rev1, rev2,
+ show_deletion,
+ empty_file,
+ callbacks,
+ callback_baton,
+ ra_session,
+ scratch_pool));
+ }
+
+ return SVN_NO_ERROR;
+}
/* Perform a diff between two repository paths.
@@ -2370,6 +2595,8 @@ diff_repos_repos(const svn_wc_diff_callb
const char *base_path;
svn_revnum_t rev1;
svn_revnum_t rev2;
+ svn_node_kind_t kind1;
+ svn_node_kind_t kind2;
const char *anchor1;
const char *anchor2;
const char *target1;
@@ -2379,7 +2606,8 @@ diff_repos_repos(const svn_wc_diff_callb
/* Prepare info for the repos repos diff. */
SVN_ERR(diff_prepare_repos_repos(&url1, &url2, &base_path, &rev1, &rev2,
&anchor1, &anchor2, &target1, &target2,
- &ra_session, ctx, path_or_url1, path_or_url2,
+ &kind1, &kind2, &ra_session,
+ ctx, path_or_url1, path_or_url2,
revision1, revision2, peg_revision,
pool));
@@ -2394,6 +2622,21 @@ diff_repos_repos(const svn_wc_diff_callb
callback_baton->ra_session = ra_session;
callback_baton->anchor = base_path;
+ if (kind1 == svn_node_none || kind2 == svn_node_none)
+ {
+ /* One side of the diff does not exist.
+ * Walk the tree that does exist, showing a series of additions
+ * or deletions. */
+ SVN_ERR(diff_repos_repos_added_or_deleted_target(target1, target2,
+ rev1, rev2,
+ kind1, kind2,
+ callbacks,
+ callback_baton,
+ ra_session,
+ pool));
+ return SVN_NO_ERROR;
+ }
+
/* Now, we open an extra RA session to the correct anchor
location for URL1. This is used during the editor calls to fetch file
contents. */
@@ -2741,6 +2984,8 @@ diff_summarize_repos_repos(svn_client_di
const char *base_path;
svn_revnum_t rev1;
svn_revnum_t rev2;
+ svn_node_kind_t kind1;
+ svn_node_kind_t kind2;
const char *anchor1;
const char *anchor2;
const char *target1;
@@ -2752,10 +2997,29 @@ diff_summarize_repos_repos(svn_client_di
/* Prepare info for the repos repos diff. */
SVN_ERR(diff_prepare_repos_repos(&url1, &url2, &base_path, &rev1, &rev2,
&anchor1, &anchor2, &target1, &target2,
- &ra_session, ctx,
- path_or_url1, path_or_url2, revision1, revision2,
+ &kind1, &kind2, &ra_session,
+ ctx, path_or_url1, path_or_url2,
+ revision1, revision2,
peg_revision, pool));
+ if (kind1 == svn_node_none || kind2 == svn_node_none)
+ {
+ /* One side of the diff does not exist.
+ * Walk the tree that does exist, showing a series of additions
+ * or deletions. */
+ SVN_ERR(svn_client__get_diff_summarize_callbacks(
+ &callbacks, &callback_baton, "",
+ summarize_func, summarize_baton, pool));
+ SVN_ERR(diff_repos_repos_added_or_deleted_target(target1, target2,
+ rev1, rev2,
+ kind1, kind2,
+ callbacks,
+ callback_baton,
+ ra_session,
+ pool));
+ return SVN_NO_ERROR;
+ }
+
SVN_ERR(svn_client__get_diff_summarize_callbacks(
&callbacks, &callback_baton,
target1, summarize_func, summarize_baton, pool));
Modified: subversion/trunk/subversion/tests/cmdline/log_tests.py
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/tests/cmdline/log_tests.py?rev=1338291&r1=1338290&r2=1338291&view=diff
==============================================================================
--- subversion/trunk/subversion/tests/cmdline/log_tests.py (original)
+++ subversion/trunk/subversion/tests/cmdline/log_tests.py Mon May 14 17:03:13 2012
@@ -2209,7 +2209,6 @@ def log_xml_old(sbox):
@Issue(4153)
-@XFail(svntest.main.is_ra_type_dav)
def log_diff_moved(sbox):
"log --diff on moved file"
@@ -2225,7 +2224,7 @@ def log_diff_moved(sbox):
mu_at_1 = sbox.repo_url + '/A/mu@1'
mu3_at_3 = sbox.repo_url + '/A/mu3@3'
- r1diff = [make_diff_header('A/mu', 'revision 0', 'revision 1')
+ r1diff = [make_diff_header('mu', 'revision 0', 'revision 1')
+ ["@@ -0,0 +1 @@\n",
"+This is the file 'mu'.\n"]]