VK Sameer <sameer@collab.net> writes:
> Resolve issue 2154 - svn blame on a directory. Over DAV, this
> fails with an "invalid XML" error message. This patch fixes that.
>
> * subversion/tests/clients/cmdline/blame_tests.py
> (blame_directory): Add new test to blame a directory
> test_list: Added blame_directory test
>
> * subversion/mod_dav_svn/file_revs.c
> (dav_svn__file_revs_report): Change to return immediately without
> cleanup if svn_repos_get_file_revs() returns an error.
>
> * subversion/libsvn_repos/rev_hunt.c
> (svn_repos_get_file_revs): Add path when generating error message
>
> Index: subversion/mod_dav_svn/file_revs.c
> ===================================================================
> --- subversion/mod_dav_svn/file_revs.c (revision 13145)
> +++ subversion/mod_dav_svn/file_revs.c (working copy)
> @@ -273,9 +273,9 @@
>
> if (serr)
> {
> - derr = dav_svn_convert_err(serr, HTTP_INTERNAL_SERVER_ERROR, serr->message,
> - resource->pool);
> - goto cleanup;
> + /* going to cleanup: causes status to change */
> + return (dav_svn_convert_err(serr, HTTP_INTERNAL_SERVER_ERROR, serr->message,
> + resource->pool));
> }
>
> if ((serr = maybe_send_header(&frb)))
These lines went past 80 columns, even after the patch formatting is
removed. No big deal, just letting you know for next time.
(In fact, the existing code was already over 80 lines, whups.)
I'm trying to interpret the comment. What is the "status" being
referred to? The HTTP error code?
> Index: subversion/tests/clients/cmdline/blame_tests.py
> ===================================================================
> --- subversion/tests/clients/cmdline/blame_tests.py (revision 13145)
> +++ subversion/tests/clients/cmdline/blame_tests.py (working copy)
> @@ -89,6 +89,35 @@
>
>
>
> +# Issue #2154 - annotating a directory should fail
> +# (change needed if the desired behavior is to
> +# run blame recursively on all the files in it)
> +#
> +def blame_directory(sbox):
> + "annotating a directory not allowed"
> +
> + # Issue 2154 - blame on directory fails without error message
> +
> + import re
> +
> + # Setup
> + sbox.build()
> + wc_dir = sbox.wc_dir
> + dir = os.path.join(wc_dir, 'A')
> +
> + # Run blame against directory 'A'
> + expected_error = ".*/A is not a file"
> + outlines, errlines = svntest.main.run_svn(1, 'blame', dir)
> +
> + # Verify expected error message is output
> + for line in errlines:
> + if re.match (expected_error, line):
> + break
> + else:
> + raise svntest.Failure ('Failed to find %s in %s' %
> + (expected_error, str(errlines)))
> +
I would expect this test to fail when the server is running under
Windows, because the call to svn_path_local_style() (in rev_hunt.c
below) would have converted the path to "\A" instead of "/A".
However, I think that call to svn_path_local_style() is probably not
appropriate, see below for why. And if we get rid of it, then this
test is robust after all :-).
By the way, when you raise the failure, it would be a good idea to
quote the %s's in forward single quotes.
I haven't used the for/else trick in Python before. I had to do a
double-take before realizing what you had written. Now that I
understand it, I realize there's a lot of other test suite code that
could become tighter by using this syntax!
> ########################################################################
> # Run the tests
>
> @@ -97,6 +126,7 @@
> test_list = [ None,
> blame_space_in_name,
> blame_binary,
> + blame_directory,
> ]
>
> if __name__ == '__main__':
> Index: subversion/libsvn_repos/rev_hunt.c
> ===================================================================
> --- subversion/libsvn_repos/rev_hunt.c (revision 13145)
> +++ subversion/libsvn_repos/rev_hunt.c (working copy)
> @@ -536,7 +536,9 @@
> the callback before reporting an uglier error below. */
> SVN_ERR (svn_fs_check_path (&kind, root, path, pool));
> if (kind != svn_node_file)
> - return svn_error_create (SVN_ERR_FS_NOT_FILE, NULL, NULL);
> + return svn_error_createf
> + (SVN_ERR_FS_NOT_FILE, NULL, _("%s is not a file"),
> + svn_path_local_style (path, pool));
>
> /* Open a history object. */
> SVN_ERR (svn_fs_node_history (&history, root, path, last_pool));
Note that we have the convention of putting the path in single quotes,
thus:
(SVN_ERR_FS_NOT_FILE, NULL, _("'%s' is not a file"),
svn_path_local_style (path, pool));
Also, I don't think the svn_path_local_style() call is appropriate
here, for two reasons. First, we're talking about a path inside the
repository, and so should refer to it using repository syntax.
Second, the local style of the server may be different from that of
the client, and since the only justification for putting the path in
local style would be to reduce confusion to the user on the client
side, a path-conversion done on the server side is pointless.
All the above comments are minor nits. I'd be happy to apply this
patch and just tweak the things manually. The only problem is, I
don't understand *why* the patch works. As you said:
> There are still some unanswered questions which somebody with greater
> familiarity with mod_dav_svn may be able to answer. Or I'll re-visit
> this once I've looked into a few more mod_dav issues. The question is
> why the goto cleanup: blows away the 500 status and picks up the default
> 200 status. I also don't see why a 0 is being returned in the http body.
Fortunately, Ben Collins-Sussman will be coming in a bit, and
furthermore we happen to have Justin Erenkrantz (Apache httpd
developer) visiting our office today, so maybe he can help.
I'll wait till Ben comes in, then look at it with him and Justin. No
need to repost, I think -- if we commit the patch, then we can make the
minor tweaks as described above ourselves.
Nice work, by the way. The "bite-sized" description on this issue
turned out to be totally inaccurate. It's actually a pretty difficult
DAV/HTTP problem, but no one knew that when the issue was filed. It
was only after you started debugging that the issue's true complexity
was revealed. Sigh, this is often how things go :-).
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Fri Feb 25 16:55:16 2005