Re: [PATCH] issue 2154 - svn blame on a directory over DAV does not

[VK Sameer <sa...@collab.net>]

kfogel@collab.net wrote:
> VK Sameer <sa...@collab.net> writes:
>
>>Index: subversion/mod_dav_svn/file_revs.c
>>+      /* 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.

OK, will add that mental check :)

> I'm trying to interpret the comment.  What is the "status" being referred
to?  The HTTP error code?

Yes.

>>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)

> 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".

Yes, except I don't understand why there is a '/' or a '\' being
generated at all.

> 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.

OK, thanks, will add that in future error messages.

> I haven't used the for/else trick in Python before.

I picked it up from an existing test (in commit_tests.py?) - it's a  pretty
nice idiom (and it flashed on me that that's why Perl has an  optional extra
block at the end of loops :)).

> I realize there's a lot of other test suite code that
> could become tighter by using this syntax!

Yes, it's a lot nicer than setting up a variable and testing its value  at
the end of the loop.

>>Index: subversion/libsvn_repos/rev_hunt.c
>
> 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));

Sorry, missed that.

> 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.

Yes, but if a user typed "svn blame A", it makes the error clearer if  the
message is "A is not a file" instead of "/A is not a file".

> 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.

Ah, so that's why svn_path_local_style() didn't work as I expected.

> 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.

Thanks for applying the patch (based on reading a future email :)).

Regards
Sameer

PS: Any xx54 issue is henceforth not considered bite-sized :)




---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org