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).
Justification:
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
more
than one path component (which according to cmpilato is an API
violation).
[...]
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
case
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_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 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
diff_repos_repos_added_or_deleted_target()
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, ###?
(do_diff):
* 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