You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by VK Sameer <sa...@collab.net> on 2005/02/25 09:41:49 UTC
[PATCH] issue 2154 - svn blame on a directory over DAV does not
return gracefully
Attached is a patch, including a test case, for issue 2154. make check
passes.
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.
Also, svn_path_local_style in svn_repos_get_file_revs doesn't seem to
convert "/A" to "A".
Thanks to a bunch of people, Karl, Madan, and Ben, for their help with
questions and testing.
Regards
Sameer
Re: [PATCH] issue 2154 - svn blame on a directory over DAV does
not return gracefully
Posted by VK Sameer <sa...@collab.net>.
Attached is a patch based on Justin's comment.
'make davcheck' gave some errors in different test cases, merge_tests.py
once, but they didn't recur when run separately, leading me to think it
may have been due to my httpd performance configuration. 'make check'
passed.
Regards
Sameer
Re: [PATCH] issue 2154 - svn blame on a directory over DAV does
not return gracefully
Posted by Justin Erenkrantz <ju...@erenkrantz.com>.
VK Sameer wrote:
> Checking for !derr first would let 'goto cleanup:' be retained in
> file_revs.c, but I don't know if that is a reasonable solution in all
> cases. version.c doesn't use the goto idiom, so it doesn't seem to need
> this fix. Comments?
I believe that checking for !derr first is reasonable as the flush is only
an optimization anyway. So, if we got an error that we're going to
return, then it isn't critical for us to flush as its not likely we wrote
anything at that point. -- justin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] issue 2154 - svn blame on a directory over DAV does
not return gracefully
Posted by VK Sameer <sa...@collab.net>.
On Mon, 2005-02-28 at 03:09, Peter N. Lundblad wrote:
>
> FYI, this code was shamelessly stolen (guess by whome:-) from another
> report implementation. I think this bug is present in several places (the
> log code, get_locations), which will lead to ugly errors if some of the
> underlying repos functions fail for some reason. It would be great if you
> fixed those in the same way!
Adding the fix is not a problem, but checking this out showed
inconsistencies in the if expression used at the 'cleanup:' label.
update.c has this:
if ((!derr) && ap_fflush ...)
while file_revs.c, version.c, and log.c have this:
if ((ap_fflush(...) && (!derr))
Checking for !derr first would let 'goto cleanup:' be retained in
file_revs.c, but I don't know if that is a reasonable solution in all
cases. version.c doesn't use the goto idiom, so it doesn't seem to need
this fix. Comments?
Regards
Sameer
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] issue 2154 - svn blame on a directory over DAV does not
return gracefully
Posted by "Peter N. Lundblad" <pe...@famlundblad.se>.
On Sat, 26 Feb 2005, VK Sameer wrote:
> On Fri, 2005-02-25 at 21:28, Justin Erenkrantz wrote:
> > --On Friday, February 25, 2005 3:11 PM +0530 VK Sameer <sa...@collab.net>
> > wrote:
> >
> > > 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.
> >
> > No, it's that the cleanup branch calls ap_fflush. flush will tell httpd to
> > write the HTTP headers out and that includes whatever r->status is at that
> > particular time. When we call convert_err, we don't immediately set r->status
> > right then - so r->status remains 0.
>
> Ah, I see.
>
> > Hence, HTTP status 200 is returned.
> >
> > So, your patch is fine as we don't want to call flush. -- justin
>
> OK, thanks for the explanation.
FYI, this code was shamelessly stolen (guess by whome:-) from another
report implementation. I think this bug is present in several places (the
log code, get_locations), which will lead to ugly errors if some of the
underlying repos functions fail for some reason. It would be great if you
fixed those in the same way!
(BTW, thanks for debuggin the get_file_revs rpbolem!)
Regards,
//Peter
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] issue 2154 - svn blame on a directory over DAV does
not return gracefully
Posted by VK Sameer <sa...@collab.net>.
On Fri, 2005-02-25 at 21:28, Justin Erenkrantz wrote:
> --On Friday, February 25, 2005 3:11 PM +0530 VK Sameer <sa...@collab.net>
> wrote:
>
> > 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.
>
> No, it's that the cleanup branch calls ap_fflush. flush will tell httpd to
> write the HTTP headers out and that includes whatever r->status is at that
> particular time. When we call convert_err, we don't immediately set r->status
> right then - so r->status remains 0.
Ah, I see.
> Hence, HTTP status 200 is returned.
>
> So, your patch is fine as we don't want to call flush. -- justin
OK, thanks for the explanation.
Sameer
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] issue 2154 - svn blame on a directory over DAV does
not return gracefully
Posted by Justin Erenkrantz <ju...@erenkrantz.com>.
--On Friday, February 25, 2005 3:11 PM +0530 VK Sameer <sa...@collab.net>
wrote:
> Attached is a patch, including a test case, for issue 2154. make check
> passes.
>
> 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.
No, it's that the cleanup branch calls ap_fflush. flush will tell httpd to
write the HTTP headers out and that includes whatever r->status is at that
particular time. When we call convert_err, we don't immediately set r->status
right then - so r->status remains 0. Hence, HTTP status 200 is returned.
So, your patch is fine as we don't want to call flush. -- justin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] issue 2154 - svn blame on a directory over DAV does not return gracefully
Posted by kf...@collab.net.
Okay, committed in r13158.
-Karl
VK Sameer <sa...@collab.net> writes:
> Attached is a patch, including a test case, for issue 2154. make check
> passes.
>
> 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.
>
> Also, svn_path_local_style in svn_repos_get_file_revs doesn't seem to
> convert "/A" to "A".
>
> Thanks to a bunch of people, Karl, Madan, and Ben, for their help with
> questions and testing.
>
> Regards
> Sameer
>
> 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
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
> For additional commands, e-mail: dev-help@subversion.tigris.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] issue 2154 - svn blame on a directory over DAV does not return gracefully
Posted by kf...@collab.net.
VK Sameer <sa...@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