Vladimir Prus <ghost@cs.msu.su> writes:
> Updated patch follows:
>
> Log message:
> The 'diff' command no longer crashes when two revisions of a file differ in
> properties but not in text.
>
> * subversion/libsvn_client/repos_diff.c
>
> (close_file): Check for presense of any change to report was added.
>
> * subversion/tests/clients/cmdline/diff_tests.py
>
> (diff_only_property_change): New function.
Excellent. Fix AND a test!
> Patch:
> Index: ./subversion/libsvn_client/repos_diff.c
> ===================================================================
> --- ./subversion/libsvn_client/repos_diff.c
> +++ ./subversion/libsvn_client/repos_diff.c Fri Jan 18 13:00:08 2002
> @@ -568,7 +568,10 @@
> {
> struct file_baton *b = file_baton;
>
> - SVN_ERR (run_diff_cmd (b));
> + /* It is possible to have only change to properties, without texdelta
> + In this case no difference should be reported and no need to run diff.
> */
> + if (b->path_end_revision)
> + SVN_ERR (run_diff_cmd (b));
>
> svn_pool_destroy (b->pool);
>
This looks basically okay (some grammatical/spelling nits, but
whatever). What's missing is some comment in the definition of the
file baton's path_end_revision member that explains that
path_end_revision is expected to be NULL for all files whose textual
contents did not change over the span of the diff'ed revisions. That
way anyone who need to change that behavior can be reasonably expected
to determine the implications of their change, and come up with some
other method for avoiding diffs on same-text files.
> Index: ./subversion/tests/clients/cmdline/diff_tests.py
> ===================================================================
> --- ./subversion/tests/clients/cmdline/diff_tests.py
> +++ ./subversion/tests/clients/cmdline/diff_tests.py Fri Jan 18 12:55:46 2002
> @@ -547,6 +547,32 @@
>
> return 0
>
> +def diff_only_property_change(sbox):
> + "diff when property was changed but text was not"
> +
> + if sbox.build():
> + return 1
> +
> + wc_dir = sbox.wc_dir
> +
> + current_dir = os.getcwd();
> + os.chdir(sbox.wc_dir);
> +
> + svntest.main.run_svn(None, 'propset', 'svn:eol-style', 'none', "iota")
> + svntest.main.run_svn(None, 'ci')
> +
> + result = 0
> +
> + if os.system(svntest.main.svn_binary + " diff -r1:2"):
> + result = 1
> +
> + if not result and os.system(svntest.main.svn_binary + " diff -r2:1"):
> + result = 1
> +
> + os.chdir(current_dir)
> + return result
> +
> +
>
> ########################################################################
> # Run the tests
> @@ -562,6 +588,7 @@
> diff_non_recursive,
> diff_repo_subset,
> diff_non_version_controlled_file,
> + diff_only_property_change,
> ]
>
> if __name__ == '__main__':
>
>
> Note: svntest.main.run_svn provides to way to detect segfault. I've found
> nothing appropriate, so have used plain os.system.
Well, I was about to freak out reading this portion of the patch until
I saw you comment here. Whew! So, you've hit on something that has
been bothering me for some time about our Python tests -- we don't
detect segfaults. How hard would it be to adjust svntest.main.run_svn()
so that it *could* return an error if a segfault occured (perhaps a -1
instead of a 1, something) ? My feeling is that the Right Way to do
this is to first fix the broken run_svn() function, then use it. That
way you essentially get credit for fixing the entire Python test suite,
and we don't have to revisit this again. :-)
At any rate, the spirit of this patch is wonderful, and I look forward
to the finished patch (which needs only contain the actual Subversion
code changes, I wouldn't expect you to fix the Python tests if that's
not your aim).
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Sat Oct 21 14:36:58 2006