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

1.7.x backport nomination for log --diff on moved files, issue #4153 etc.

From: Julian Foad <julianfoad_at_btopenworld.com>
Date: Thu, 31 Oct 2013 16:27:48 +0000

Ben encouraged me to review the 1.7.x backport nomination for
^/subversion/branches/1.7.x-issue4153. The nomination says:

 * r1306275, and other revisions
   New diff support code for added/deleted files and directories.
   Fixes issue #4153, "svn log --diff" on moved file gives "not found",
   and other problems such as issue #4421, "diff --summarize of a child of
   a newly added child causes crash" (over HTTP only).
     The current code (introduced in 1.7.x in the r1207351 backport commit)
     produces inconsistent paths in diff output (see
     http://svn.haxx.se/dev/archive-2012-03/0385.shtml), and it also breaks
     with the paradigm that the target of an editor drive may never have
     than one path component (which according to cmpilato is an API

So I tried out log --diff with moved files. All the scenarios I tried seem
to work now, so that's very good.

As for the "diff --summarize" fix and "other problems", I wasn't really
able to confirm that. I think at least some summarize cases now work
without crashing that crashed before, but several other cases still crash,
for example:

   svn diff --summarize ^/DIR ^/file
      => seg-fault
   svn diff --summarize ^/f_at_1 ^/f2_at_1 --notice-ancestry
      => seg-fault

Seg-faults are bad, but a failure to fix these cases doesn't invalidate the
backport nomination as they don't seem to be regressions.

I wasn't sure what other cases were supposed to be fixed.

In order to get a feel for the code, I started drafting a combined log
message since the branch log messages don't really explain a lot. What I
came up with so far is (incomplete):

Fix issue #4153, "svn log --diff" on moved file gives "not found".
Fix issue #4421, "diff --summarize of a child of a newly added child causes
crash" (over HTTP only).

The following revisions from trunk are merged to the backport branch.

  r1164116 (part) -- rename repos_diff_summarize.c
  r1230798 -- in log tests, ignore ordering of diff output
  r1306275 -- new test for issue #4153
  r1309865 -- fix resolving URLs in diff_prepare_repos_repos()
  r1338291 -- rewrite, fixing path notifications
  r1338297 -- tweak pool usage
  r1338314 -- tweak stream and file handling
  r1338688 -- filter out non-regular props
  r1338708 -- fix "svn diff -cN foo"
  r1338713 -- add a test: diff_deleted_url()
  r1338739 -- fix path notifications
  r1338748 -- fix test expectations
  r1339159 -- add a test for summarizing diff with added/deleted targets
  r1341544 -- fix repos-wc diff of a copied or moved file
  r1341560 -- fix text normalization in repos-wc diffs
  r1535551 -- tweak comments
  r1535591 -- add a test for issue #4421: deep adds in diff --summarize

Copy the file libsvn_client/diff_summarize.c from trunk. On trunk, it
was renamed from libsvn_client/repos_diff_summarize.c, but here we keep
keep both versions. The new file contains summarize diff callbacks which
can be driven by the code that handles added and deleted diff targets.
On trunk, svn_client__get_diff_summarize_callbacks() replaced the function
svn_client__get_diff_summarize_editor(), but in 1.7 we keep both to minimize
changes made to the summarize diff code.

* subversion/libsvn_client/client.h
  (svn_client__get_diff_summarize_callbacks): Declare.

* subversion/libsvn_client/diff.c
  (resolve_pegged_diff_target_url): New function.
  (diff_prepare_repos_repos): Output the corresponding node kinds, and
    verify that at least one of the diff targets exists.
    When resolving URLs for pegged diff targets,
    resolve each URL separately. This is required to properly handle the
    where the diff target lives at a different location in the operative
    revision range _and_ only exists at one side of the operative range.
  (make_regular_props_array): New function: transform a prop-hash retrieved
   from the RA layer into a changed-props array expected by diff callbacks.
  (make_regular_props_hash): New function ...
   diff_repos_repos_added_or_deleted_target): New functions that handle
    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 callbacks. 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
    function instead of using the diff editor.
  (diff_repos_wc_file_target): New function.
  (diff_repos_wc): If both diff targets can be diffed as files, fetch the
    file from the repository and generate a diff against the local version
    of the file. Otherwise, ###?

* subversion/libsvn_client/diff_summarize.c
  New file, copied from trunk. This is similar to and rather duplicates
  the existing 'repos_diff_summarize.c'. (On trunk, it was renamed, but
  here it is copied.)

* subversion/tests/cmdline/diff_tests.py
  (diff_renamed_file): Remove the XFail decoration.
  (basic_diff_summarize): Adjust...
  (diff_deleted_url): New test.
  (test_list): Run it.

* subversion/tests/cmdline/log_tests.py
  (check_log_chain, setify, compare_diff_output): New functions.
  (log_diff): Adjust...
  (log_diff_moved): New test.
  (test_list): Run it.


Overall, my impression is it seems to do what it promises. I can't claim to
have understood the implications of the way it's written -- especially any
potential issues (such as inconsistencies) that might be introduced by the
way it adds a bunch of diff-summarize code from trunk in parallel with the
older 1.7.0-era summarize code.

But for serving the purpose at hand, it seems to be good enough. I think I
can +1 it.

- Julian
Received on 2013-10-31 17:34:22 CET

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

This site is subject to the Apache Privacy Policy and the Apache Public Forum Archive Policy.