[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: [PATCH] svn diff -r1:BASE URL asserts

From: Julian Foad <julianfoad_at_btopenworld.com>
Date: 2006-03-09 02:58:33 CET

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

This is an archived mail posted to the Subversion Dev mailing list.