Julian Foad wrote:
>
> Paul (earlier in the thread) compared the running of diff "BASE:1" with
> "1:BASE" and found where the latter went wrong:
>
> --------------------------------------------------------------------------
>
> (Working Scenario) (Asserting Scenario)
> svn diff BASE:1 URL svn diff 1:BASE URL
> --------------------------------------------------------------------------
> svn/diff-cmd.c svn/diff-cmd.c
> svn__cl_diff() svn__cl_diff()
> pegged_diff is determined only by The first potential problem,
> looking at the start revision pegged_diff == TRUE, so we call:
> kind. It is TRUE iff start
> revision kind is not
> svn_opt_revision_base or
> svn_opt_revision_working.
> So it's FALSE in this case which
> results in call to:
> --------------------------------------------------------------------------
> libsvn_client/diff.c libsvn_client/diff.c
> svn_client_diff3() svn_client_diff_peg3()
If I understand the meaning of "a pegged diff" correctly, it is the left-hand
side that is wrong here. Both of these should be pegged diffs. In svn_cl__diff():
> else
> {
> /* The 'svn diff [-r N[:M]] [TARGET[@REV]...]' case matches. */
>
> /* Here each target is a pegged object. Find out the starting
> and ending paths for each target. */
[...]
> /* Determine if we need to do pegged diffs. */
> if (opt_state->start_revision.kind != svn_opt_revision_base
> && opt_state->start_revision.kind != svn_opt_revision_working)
> pegged_diff = TRUE;
That's wrong. In this section, according to the comment, we should assign
pegged_diff=TRUE unconditionally.
Continuing,
> --------------------------------------------------------------------------
> libsvn_client/diff.c libsvn_client/diff.c
> do_diff() do_diff_peg()
> Call check_paths() and get Our second potential problem, the
> call to check_paths() results in
> the struct diff_paths *diff_paths: the struct diff_paths* diff_paths:
> diff_paths->is_local_rev1 1 diff_paths->is_local_rev1 0
> diff_paths->is_local_rev2 0 diff_paths->is_local_rev2 1
> diff_paths->is_repos_path1 1 diff_paths->is_repos_path1 1
> diff_paths->is_repos_path2 1 diff_paths->is_repos_path2 0
> Here the paths struct accurately But path 2 is a URL at revision
> represents the invalid request BASE, invalid yes, but the paths
> for the BASE rev of an URL. struct doesn't reflect this and
> causes a call to:
OK, here, on the right-hand side, the result is being used wrongly. "...path2"
is inapplicable to a pegged diff, and supposed to be ignored.
I've just checked in a refactoring that removes a fair bit of duplicated code
from libsvn_client/diff.c. I've prepared the attached patch which I believe
fixes these two errors, and thus fixes this bug. It also improves the help
message, which is an unrelated change so ought to go in a separate patch.
It also fixes another bug in which "svn diff -rBASE:1 WC_PATH" generates the
wrong output, but my test for that is not right (it passes with and without the
patch). I have a sandbox in which I can see the effect, but am not sure yet
exactly how to reproduce it from scratch.
Therefore this is not yet ready for commit, but I thought I ought to share it,
especially as it might affect Malcolm who seems to be looking at diff bugs at
the moment.
- Julian
Fix bugs in "svn diff" and svn_client_diff3().
1. "svn diff -r1:BASE URL" would fail an assertion.
2. "svn diff -rBASE:1 WC-PATH" would give wrong diff output.
Also slightly improve the help for "svn diff".
* subversion/libsvn_client/diff.c
(check_paths): Make pegged logic same as non-pegged logic, to avoid
attempting a local diff on a URL target. Don't refuse to do a local
pegged diff.
(struct diff_repos_repos): Rename to diff_repos_repos_t, to differentiate
it from the function of the same name.
(diff_prepare_repos_repos, diff_repos_repos, diff_summarize_repos_repos):
Adjust uses of that type.
* subversion/svn/diff-cmd.c
(svn_cl__diff): Correct the decision whether to do a pegged diff.
* subversion/svn/main.c
(svn_cl__cmd_table): Improve the help for "svn diff".
* subversion/tests/cmdline/diff_tests.py
(diff_repos_base, diff_base_repos): New tests.
(test_list): Add the new tests.
Index: subversion/libsvn_client/diff.c
===================================================================
--- subversion/libsvn_client/diff.c (revision 18782)
+++ subversion/libsvn_client/diff.c (working copy)
@@ -1458,31 +1458,18 @@ check_paths(const struct diff_parameters
((params->revision2->kind == svn_opt_revision_base)
|| (params->revision2->kind == svn_opt_revision_working));
- if (peg)
- {
- if (is_local_rev1 && is_local_rev2)
- return svn_error_create(SVN_ERR_CLIENT_BAD_REVISION, NULL,
- _("At least one revision must be non-local "
- "for a pegged diff"));
-
- paths->is_repos1 = ! is_local_rev1;
- paths->is_repos2 = ! is_local_rev2;
- }
- else
- {
- /* Working copy paths with non-local revisions get turned into
- URLs. We don't do that here, though. We simply record that it
- needs to be done, which is information that helps us choose our
- diff helper function. */
- paths->is_repos1 = ! is_local_rev1 || svn_path_is_url(params->path1);
- paths->is_repos2 = ! is_local_rev2 || svn_path_is_url(params->path2);
- }
+ /* Working copy paths with non-local revisions get turned into
+ URLs. We don't do that here, though. We simply record that it
+ needs to be done, which is information that helps us choose our
+ diff helper function. */
+ paths->is_repos1 = ! is_local_rev1 || svn_path_is_url(params->path1);
+ paths->is_repos2 = ! is_local_rev2 || svn_path_is_url(params->path2);
return SVN_NO_ERROR;
}
/** Helper structure filled by diff_prepare_repos_repos */
-struct diff_repos_repos
+struct diff_repos_repos_t
{
/* URL created from path1 */
const char *url1;
@@ -1522,7 +1509,7 @@ struct diff_repos_repos
* structure. */
static svn_error_t *
diff_prepare_repos_repos(const struct diff_parameters *params,
- struct diff_repos_repos *drr,
+ struct diff_repos_repos_t *drr,
svn_client_ctx_t *ctx,
apr_pool_t *pool)
{
@@ -2011,7 +1998,7 @@ diff_repos_repos(const struct diff_param
const svn_delta_editor_t *diff_editor;
void *diff_edit_baton;
- struct diff_repos_repos drr;
+ struct diff_repos_repos_t drr;
/* Prepare info for the repos repos diff. */
SVN_ERR(diff_prepare_repos_repos(diff_param, &drr, ctx, pool));
@@ -2259,7 +2246,7 @@ diff_summarize_repos_repos(const struct
const svn_delta_editor_t *diff_editor;
void *diff_edit_baton;
- struct diff_repos_repos drr;
+ struct diff_repos_repos_t drr;
/* Prepare info for the repos repos diff. */
SVN_ERR(diff_prepare_repos_repos(diff_param, &drr, ctx, pool));
### do_diff(pegged=1): is_repos1=0, is_repos2=0
diff_wc_wc
Index: subversion/svn/diff-cmd.c
===================================================================
--- subversion/svn/diff-cmd.c (revision 18780)
+++ subversion/svn/diff-cmd.c (working copy)
@@ -218,11 +218,7 @@ svn_cl__diff(apr_getopt_t *os,
opt_state->end_revision.kind = working_copy_present
? svn_opt_revision_working : svn_opt_revision_head;
- /* Determine if we need to do pegged diffs. */
- if (opt_state->start_revision.kind != svn_opt_revision_base
- && opt_state->start_revision.kind != svn_opt_revision_working)
- pegged_diff = TRUE;
-
+ pegged_diff = TRUE;
}
svn_opt_push_implicit_dot_target(targets, pool);
### do_diff(pegged=1): is_repos1=0, is_repos2=0
diff_wc_wc
Index: subversion/svn/main.c
===================================================================
--- subversion/svn/main.c (revision 18780)
+++ subversion/svn/main.c (working copy)
@@ -291,18 +291,16 @@ const svn_opt_subcommand_desc2_t svn_cl_
SVN_CL__LOG_MSG_OPTIONS, SVN_CL__AUTH_OPTIONS, svn_cl__config_dir_opt} },
{ "diff", svn_cl__diff, {"di"}, N_
- ("Display the differences between two paths.\n"
+ ("Display the differences between two revisions or paths.\n"
"usage: 1. diff [-c M | -r N[:M]] [TARGET[@REV]...]\n"
" 2. diff [-c M | -r N[:M]] --old=OLD-TGT[@OLDREV] [--new=NEW-TGT[@NEWREV]] \\\n"
" [PATH...]\n"
" 3. diff OLD-URL[@OLDREV] NEW-URL[@NEWREV]\n"
"\n"
- " 1. Display the changes made to TARGETs as they are seen in REV between\n"
- " two revisions. TARGETs may be working copy paths or URLs.\n"
- "\n"
- " N defaults to BASE if any TARGET is a working copy path, otherwise it\n"
- " must be specified. M defaults to the current working version if any\n"
- " TARGET is a working copy path, otherwise it defaults to HEAD.\n"
+ " 1. Display the changes made to TARGETs (found in REV) between\n"
+ " two revisions. TARGETs may be all working copy paths or all URLs.\n"
+ " If TARGETs are working copy paths, N defaults to BASE and M to the\n"
+ " working version; if URLs, N must be specified and M defaults to HEAD.\n"
" The '-c M' option is equivalent to '-r N:M' where N = M-1.\n"
" Using '-c -M' does the reverse: '-r M:N' where N = M-1.\n"
"\n"
### do_diff(pegged=1): is_repos1=0, is_repos2=0
diff_wc_wc
Index: subversion/tests/cmdline/diff_tests.py
===================================================================
--- subversion/tests/cmdline/diff_tests.py (revision 18780)
+++ subversion/tests/cmdline/diff_tests.py (working copy)
@@ -2359,6 +2359,47 @@
os.chdir(current_dir)
+#----------------------------------------------------------------------
+# Requesting a repos->base diff on a URL is an error but used to cause an assertion failure
+def diff_repos_base(sbox):
+ "repos->base diff on a URL"
+
+ sbox.build()
+
+ current_dir = os.getcwd()
+ os.chdir(sbox.wc_dir)
+ try:
+ # Expect some error output, but not a crash.
+ svntest.actions.run_and_verify_svn(None, None, SVNAnyOutput,
+ 'diff', '-r1:BASE', sbox.repo_url)
+
+ finally:
+ os.chdir(current_dir)
+
+
+#----------------------------------------------------------------------
+# A base->repos diff used to output an all-lines-deleted diff
+def diff_base_repos(sbox):
+ "base->repos diff"
+
+ sbox.build()
+
+ current_dir = os.getcwd()
+ os.chdir(sbox.wc_dir)
+ try:
+ update_a_file()
+ svntest.actions.run_and_verify_svn(None, None, [], 'ci', '-m', '')
+ svntest.actions.run_and_verify_svn(None, None, [], 'up')
+
+ # Check for correct output of diff BASE:1
+ out, err = svntest.actions.run_and_verify_svn(None, SVNAnyOutput, [],
+ 'diff', '-rBASE:1')
+ check_update_a_file(out)
+
+ finally:
+ os.chdir(current_dir)
+
+
########################################################################
#Run the tests
@@ -2399,6 +2440,8 @@
diff_repos_wc_add_with_props,
diff_nonrecursive_checkout_deleted_dir,
diff_repos_working_added_dir,
+ diff_repos_base,
+ diff_base_repos,
]
if __name__ == '__main__':
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Thu Mar 9 02:59:05 2006