On Mon, Aug 22, 2011 at 12:42:55PM +0100, Julian Foad wrote:
> I'm noticing all sorts of nasty inconsistencies in diffs. For instance,
> manual testing reveals that in svn_client_diff5(), show_copies_as_adds
> is ignored for a repos-repos diff. Instead, a file is diffed against its
> copyfrom source iff the file is the diff target and not if some parent
> directory is the diff target. For a repos-WC diff, this is ignored if
> the file is the diff target. Ugh. Later.
This reminded me of a patch I have lying around.
It aims at making 'svn diff' display correct copyfrom paths,
which is especially important for the new --git style diffs
which can display copies as first-class operations.
Right now, the copyfrom revision shown in diff output is often incorrect.
The patch reuses the send-copyfrom-args feature the client can use to
request copyfrom information from the server. This was originally added
for updates but was removed from the client side in 1.7 and later.
The patch is not yet finished because it breaks merge tests.
I haven't looked at the merge test failures in detail yet.
The protocol changes for HTTP are also incomplete.
I'm mainly putting this here in case somebody looking at improving
diff is interested and/or wants to comment on the approach.
In any case, I agree that diff needs work in some areas.
Another bad spot is documented here:
http://subversion.tigris.org/issues/show_bug.cgi?id=2873
Index: subversion/libsvn_ra_local/ra_plugin.c
===================================================================
--- subversion/libsvn_ra_local/ra_plugin.c (revision 1156687)
+++ subversion/libsvn_ra_local/ra_plugin.c (working copy)
@@ -844,7 +844,7 @@ svn_ra_local__do_diff(svn_ra_session_t *session,
switch_url,
text_deltas,
depth,
- FALSE,
+ TRUE,
ignore_ancestry,
update_editor,
update_baton,
Index: subversion/libsvn_ra_svn/client.c
===================================================================
--- subversion/libsvn_ra_svn/client.c (revision 1156687)
+++ subversion/libsvn_ra_svn/client.c (working copy)
@@ -1327,10 +1327,11 @@ static svn_error_t *ra_svn_diff(svn_ra_session_t *
svn_boolean_t recurse = DEPTH_TO_RECURSE(depth);
/* Tell the server we want to start a diff. */
- SVN_ERR(svn_ra_svn_write_cmd(conn, pool, "diff", "(?r)cbbcbw", rev,
+ SVN_ERR(svn_ra_svn_write_cmd(conn, pool, "diff", "(?r)cbbcbwb", rev,
target, recurse, ignore_ancestry,
versus_url, text_deltas,
- svn_depth_to_word(depth)));
+ svn_depth_to_word(depth),
+ TRUE /* send_copyfrom_args */));
SVN_ERR(handle_auth_request(sess_baton, pool));
/* Fetch a reporter for the caller to drive. The reporter will drive
Index: subversion/libsvn_ra_svn/protocol
===================================================================
--- subversion/libsvn_ra_svn/protocol (revision 1156687)
+++ subversion/libsvn_ra_svn/protocol (working copy)
@@ -364,7 +364,8 @@ second place for auth-request point as noted below
diff
params: ( [ rev:number ] target:string recurse:bool ignore-ancestry:bool
- url:string ? text-deltas:bool ? depth:word )
+ url:string ? text-deltas:bool ? depth:word
+ ? send_copyfrom_param:bool )
Client switches to report command set.
Upon finish-report, server sends auth-request.
After auth exchange completes, server switches to editor command set.
Index: subversion/libsvn_ra_neon/fetch.c
===================================================================
--- subversion/libsvn_ra_neon/fetch.c (revision 1156687)
+++ subversion/libsvn_ra_neon/fetch.c (working copy)
@@ -2798,7 +2798,7 @@ svn_error_t * svn_ra_neon__do_diff(svn_ra_session_
diff_target,
versus_url,
depth,
- FALSE,
+ TRUE,
ignore_ancestry,
FALSE,
wc_diff,
Index: subversion/libsvn_client/repos_diff.c
===================================================================
--- subversion/libsvn_client/repos_diff.c (revision 1156687)
+++ subversion/libsvn_client/repos_diff.c (working copy)
@@ -209,6 +209,10 @@ struct file_baton {
/* A cache of any property changes (svn_prop_t) received for this file. */
apr_array_header_t *propchanges;
+ /* Copyfrom information for copied files. */
+ const char *copyfrom_path;
+ svn_revnum_t copyfrom_revision;
+
/* The pool passed in by add_file or open_file.
Also, the pool this file_baton is allocated in. */
apr_pool_t *pool;
@@ -253,6 +257,8 @@ make_dir_baton(const char *path,
static struct file_baton *
make_file_baton(const char *path,
svn_boolean_t added,
+ const char *copyfrom_path,
+ svn_revnum_t copyfrom_revision,
struct edit_baton *edit_baton,
apr_pool_t *pool)
{
@@ -268,6 +274,8 @@ make_file_baton(const char *path,
file_baton->wcpath = svn_dirent_join(edit_baton->target, path, file_pool);
file_baton->propchanges = apr_array_make(pool, 1, sizeof(svn_prop_t));
file_baton->base_revision = edit_baton->revision;
+ file_baton->copyfrom_path = copyfrom_path;
+ file_baton->copyfrom_revision = copyfrom_revision;
return file_baton;
}
@@ -536,7 +544,8 @@ diff_deleted_dir(const char *dir,
const char *mimetype1, *mimetype2;
/* Compare a file being deleted against an empty file */
- b = make_file_baton(path, FALSE, eb, iterpool);
+ b = make_file_baton(path, FALSE, NULL, SVN_INVALID_REVNUM,
+ eb, iterpool);
SVN_ERR(get_file_from_ra(b, FALSE, iterpool));
SVN_ERR(get_empty_file(b->edit_baton, &(b->path_end_revision)));
@@ -601,7 +610,8 @@ delete_entry(const char *path,
struct file_baton *b;
/* Compare a file being deleted against an empty file */
- b = make_file_baton(path, FALSE, eb, scratch_pool);
+ b = make_file_baton(path, FALSE, NULL, SVN_INVALID_REVNUM,
+ eb, scratch_pool);
SVN_ERR(get_file_from_ra(b, FALSE, scratch_pool));
SVN_ERR(get_empty_file(b->edit_baton, &(b->path_end_revision)));
@@ -804,9 +814,8 @@ add_file(const char *path,
struct dir_baton *pb = parent_baton;
struct file_baton *b;
- /* ### TODO: support copyfrom? */
-
- b = make_file_baton(path, TRUE, pb->edit_baton, pool);
+ b = make_file_baton(path, TRUE, copyfrom_path, copyfrom_revision,
+ pb->edit_baton, pool);
*file_baton = b;
/* Skip *everything* within a newly tree-conflicted directory.
@@ -833,7 +842,8 @@ open_file(const char *path,
struct dir_baton *pb = parent_baton;
struct file_baton *b;
struct edit_baton *eb = pb->edit_baton;
- b = make_file_baton(path, FALSE, pb->edit_baton, pool);
+ b = make_file_baton(path, FALSE, NULL, SVN_INVALID_REVNUM,
+ pb->edit_baton, pool);
*file_baton = b;
/* Skip *everything* within a newly tree-conflicted directory
@@ -997,26 +1007,75 @@ close_file(void *file_baton,
remove_non_prop_changes(b->pristine_props, b->propchanges);
}
- if (b->path_end_revision || b->propchanges->nelts > 0)
+ if (b->path_end_revision || b->propchanges->nelts > 0 || b->copyfrom_path)
{
const char *mimetype1, *mimetype2;
get_file_mime_types(&mimetype1, &mimetype2, b);
+ if (b->added)
+ {
+ if (b->copyfrom_path && !b->path_end_revision)
+ {
+ svn_stream_t *stream;
+ svn_error_t *err;
- if (b->added)
- SVN_ERR(eb->diff_callbacks->file_added(
- &content_state, &prop_state, &b->tree_conflicted,
- b->wcpath,
- b->path_end_revision ? b->path_start_revision : NULL,
- b->path_end_revision,
- 0,
- b->edit_baton->target_revision,
- mimetype1, mimetype2,
- NULL, SVN_INVALID_REVNUM,
- b->propchanges, b->pristine_props,
- b->edit_baton->diff_cmd_baton,
- scratch_pool));
- else
+ /* The file was copied and its content has not changed since.
+ * Compare the empty file to the copied file.
+ *
+ * ### This behaviour should really depend on the
+ * ### show_copies_as_adds flag. We should show an empty delta
+ * ### if the copy hasn't changed. But it seems that historically
+ * ### repos->repos diffs have always shown copies as adds
+ * ### in this case, so let's keep it this way. */
+ SVN_ERR(get_empty_file(b->edit_baton,
+ &(b->path_start_revision)));
+ SVN_ERR(svn_stream_open_unique(
+ &stream, &b->path_end_revision, NULL,
+ svn_io_file_del_on_pool_cleanup,
+ scratch_pool, scratch_pool));
+ err = svn_ra_get_file(eb->ra_session,
+ b->path,
+ eb->target_revision,
+ stream, NULL, NULL,
+ b->pool);
+ /* Ignore "path does not exist" errors. In some cases the file
+ * has copyfrom but does not exist in the repository (e.g. we
+ * might be merging an addition into a working copy of a branch
+ * where the file doesn't exist yet). */
+ if (err)
+ {
+ if (err->apr_err == SVN_ERR_FS_NOT_FOUND)
+ svn_error_clear(err);
+ else
+ {
+ SVN_ERR(svn_stream_close(stream));
+ return svn_error_trace(err);
+ }
+ }
+ else
+ {
+ /* Show all props as newly added, too. */
+ b->pristine_props = apr_hash_make(b->pool);
+ }
+
+ SVN_ERR(svn_stream_close(stream));
+ }
+
+ if (b->path_end_revision || b->propchanges->nelts > 0)
+ SVN_ERR(eb->diff_callbacks->file_added(
+ &content_state, &prop_state, &b->tree_conflicted,
+ b->wcpath,
+ b->path_end_revision ? b->path_start_revision : NULL,
+ b->path_end_revision,
+ 0,
+ b->edit_baton->target_revision,
+ mimetype1, mimetype2,
+ b->copyfrom_path, b->copyfrom_revision,
+ b->propchanges, b->pristine_props,
+ b->edit_baton->diff_cmd_baton,
+ scratch_pool));
+ }
+ else if (b->path_end_revision || b->propchanges->nelts > 0)
SVN_ERR(eb->diff_callbacks->file_changed(
&content_state, &prop_state,
&b->tree_conflicted, b->wcpath,
Index: subversion/libsvn_client/diff.c
===================================================================
--- subversion/libsvn_client/diff.c (revision 1156687)
+++ subversion/libsvn_client/diff.c (working copy)
@@ -401,23 +401,20 @@ print_git_diff_header_deleted(svn_stream_t *os, co
static svn_error_t *
print_git_diff_header_copied(svn_stream_t *os, const char *header_encoding,
const char *copyfrom_path,
- svn_revnum_t copyfrom_rev,
const char *path,
apr_pool_t *result_pool)
{
SVN_ERR(svn_stream_printf_from_utf8(os, header_encoding, result_pool,
"diff --git a/%s b/%s%s",
copyfrom_path, path, APR_EOL_STR));
- if (copyfrom_rev != SVN_INVALID_REVNUM)
- SVN_ERR(svn_stream_printf_from_utf8(os, header_encoding, result_pool,
- "copy from %s@%ld%s", copyfrom_path,
- copyfrom_rev, APR_EOL_STR));
- else
- SVN_ERR(svn_stream_printf_from_utf8(os, header_encoding, result_pool,
- "copy from %s%s", copyfrom_path,
- APR_EOL_STR));
SVN_ERR(svn_stream_printf_from_utf8(os, header_encoding, result_pool,
+ "copy from %s%s", copyfrom_path,
+ APR_EOL_STR));
+ SVN_ERR(svn_stream_printf_from_utf8(os, header_encoding, result_pool,
"copy to %s%s", path, APR_EOL_STR));
+
+ /* ### need some special git-diff header for copyfrom revision */
+
return SVN_NO_ERROR;
}
@@ -436,6 +433,9 @@ print_git_diff_header_renamed(svn_stream_t *os, co
APR_EOL_STR));
SVN_ERR(svn_stream_printf_from_utf8(os, header_encoding, result_pool,
"rename to %s%s", path, APR_EOL_STR));
+
+ /* ### need some special git-diff header for copyfrom revision */
+
return SVN_NO_ERROR;
}
@@ -473,13 +473,21 @@ print_git_diff_header(svn_stream_t *os,
svn_revnum_t rev1,
svn_revnum_t rev2,
const char *copyfrom_path,
- svn_revnum_t copyfrom_rev,
+ svn_revnum_t copyfrom_revision,
const char *header_encoding,
svn_ra_session_t *ra_session,
svn_wc_context_t *wc_ctx,
const char *wc_root_abspath,
apr_pool_t *scratch_pool)
{
+ if (copyfrom_path)
+ {
+ /* Copyfrom paths are always relative to the repository root.
+ * Strip leading slashes for display. */
+ while (copyfrom_path[0] == '/')
+ copyfrom_path++;
+ }
+
if (operation == svn_diff_op_deleted)
{
SVN_ERR(print_git_diff_header_deleted(os, header_encoding,
@@ -493,11 +501,12 @@ print_git_diff_header(svn_stream_t *os,
else if (operation == svn_diff_op_copied)
{
SVN_ERR(print_git_diff_header_copied(os, header_encoding,
- copyfrom_path, copyfrom_rev,
- repos_relpath2,
+ copyfrom_path, repos_relpath2,
scratch_pool));
+ /* Put the copyfrom revision into the label. It's the only place
+ * we can put it and stay compatible with git. */
*label1 = diff_label(apr_psprintf(scratch_pool, "a/%s", copyfrom_path),
- rev1, scratch_pool);
+ copyfrom_revision, scratch_pool);
*label2 = diff_label(apr_psprintf(scratch_pool, "b/%s", repos_relpath2),
rev2, scratch_pool);
}
@@ -515,8 +524,10 @@ print_git_diff_header(svn_stream_t *os,
SVN_ERR(print_git_diff_header_modified(os, header_encoding,
repos_relpath1, repos_relpath2,
scratch_pool));
+ /* Put the copyfrom revision into the label. It's the only place
+ * we can put it and stay compatible with git. */
*label1 = diff_label(apr_psprintf(scratch_pool, "a/%s", repos_relpath1),
- rev1, scratch_pool);
+ copyfrom_revision, scratch_pool);
*label2 = diff_label(apr_psprintf(scratch_pool, "b/%s", repos_relpath2),
rev2, scratch_pool);
}
@@ -910,7 +921,7 @@ diff_content_changed(const char *path,
const char *mimetype2,
svn_diff_operation_kind_t operation,
const char *copyfrom_path,
- svn_revnum_t copyfrom_rev,
+ svn_revnum_t copyfrom_revision,
void *diff_baton)
{
struct diff_cmd_baton *diff_cmd_baton = diff_baton;
@@ -1040,7 +1051,7 @@ diff_content_changed(const char *path,
SVN_ERR(print_git_diff_header(os, &label1, &label2, operation,
tmp_path1, tmp_path2, rev1, rev2,
copyfrom_path,
- copyfrom_rev,
+ copyfrom_revision,
diff_cmd_baton->header_encoding,
diff_cmd_baton->ra_session,
diff_cmd_baton->wc_ctx,
@@ -1056,6 +1067,20 @@ diff_content_changed(const char *path,
diff_cmd_baton->options.for_internal->show_c_function,
subpool));
+ if (diff_cmd_baton->use_git_diff_format &&
+ !svn_diff_contains_diffs(diff) &&
+ copyfrom_revision != SVN_INVALID_REVNUM)
+ {
+ /* Print the labels even though there is no content to diff.
+ * The labels contain the copyfrom revision.
+ * ### need some special git-diff header for this. */
+ SVN_ERR(svn_stream_printf_from_utf8(
+ os, diff_cmd_baton->header_encoding, subpool,
+ "--- %s" APR_EOL_STR
+ "+++ %s" APR_EOL_STR,
+ label1, label2));
+ }
+
/* We have a printed a diff for this path, mark it as visited. */
apr_hash_set(diff_cmd_baton->visited_paths, path,
APR_HASH_KEY_STRING, path);
@@ -1173,6 +1198,7 @@ diff_file_added(svn_wc_notify_state_t *content_sta
mimetype1, mimetype2,
svn_diff_op_added, NULL, SVN_INVALID_REVNUM,
diff_baton));
+
if (prop_changes->nelts > 0)
SVN_ERR(diff_props_changed(prop_state, tree_conflicted,
path, FALSE, prop_changes,
Index: subversion/libsvn_ra_serf/update.c
===================================================================
--- subversion/libsvn_ra_serf/update.c (revision 1156687)
+++ subversion/libsvn_ra_serf/update.c (working copy)
@@ -2722,7 +2722,7 @@ svn_ra_serf__do_diff(svn_ra_session_t *ra_session,
return make_update_reporter(ra_session, reporter, report_baton,
revision,
session->session_url.path, versus_url, diff_target,
- depth, ignore_ancestry, text_deltas, FALSE,
+ depth, ignore_ancestry, text_deltas, TRUE,
diff_editor, diff_baton, pool);
}
Index: subversion/svnserve/serve.c
===================================================================
--- subversion/svnserve/serve.c (revision 1156687)
+++ subversion/svnserve/serve.c (working copy)
@@ -1781,6 +1781,7 @@ static svn_error_t *diff(svn_ra_svn_conn_t *conn,
/* Default to unknown. Old clients won't send depth, but we'll
handle that by converting recurse if necessary. */
svn_depth_t depth = svn_depth_unknown;
+ svn_boolean_t send_copyfrom_args = FALSE;
/* Parse the arguments. */
if (params->nelts == 5)
@@ -1793,10 +1794,11 @@ static svn_error_t *diff(svn_ra_svn_conn_t *conn,
}
else
{
- SVN_ERR(svn_ra_svn_parse_tuple(params, pool, "(?r)cbbcb?w",
+ SVN_ERR(svn_ra_svn_parse_tuple(params, pool, "(?r)cbbcb?w?b",
&rev, &target, &recurse,
&ignore_ancestry, &versus_url,
- &text_deltas, &depth_word));
+ &text_deltas, &depth_word,
+ &send_copyfrom_args));
}
target = svn_relpath_canonicalize(target, pool);
versus_url = svn_uri_canonicalize(versus_url, pool);
@@ -1819,7 +1821,8 @@ static svn_error_t *diff(svn_ra_svn_conn_t *conn,
svn_revnum_t from_rev;
SVN_ERR(accept_report(NULL, &from_rev,
conn, pool, b, rev, target, versus_path,
- text_deltas, depth, FALSE, ignore_ancestry));
+ text_deltas, depth, send_copyfrom_args,
+ ignore_ancestry));
SVN_ERR(log_command(b, conn, pool, "%s",
svn_log__diff(full_path, from_rev, versus_path,
rev, depth, ignore_ancestry,
Received on 2011-08-23 18:51:47 CEST