On 12.03.2015 14:19, Bert Huijben wrote: > >> -----Original Message----- >> From: Branko Čibej [mailto:br...@wandisco.com] >> Sent: donderdag 12 maart 2015 12:06 >> To: dev@subversion.apache.org >> Subject: Re: svn commit: r1665195 - >> /subversion/trunk/subversion/libsvn_ra_serf/util.c >> >> On 12.03.2015 05:36, Branko Čibej wrote: >>> On 09.03.2015 12:39, rhuij...@apache.org wrote: >>>> Author: rhuijben >>>> Date: Mon Mar 9 11:39:39 2015 >>>> New Revision: 1665195 >>>> >>>> URL: http://svn.apache.org/r1665195 >>>> Log: >>>> In ra-serf: stop handling HTTP status 405 'Method forbidden' as a generic >>>> out of date error. When locking this is the error we get when the resource >>>> does not exist in HEAD, but in general it tells us that there is an >>>> authorization problem. >>>> >>>> As the lock code has its own http status handling now, we can change >>>> the error reported from the generic error handling code path. >>>> >>>> This should give users that accidentally use anonymous 'http' on a >>>> server that uses 'https' for authorized operations a much better response, >>>> than a simple out of date error (with the recommendation to update added >>>> by their client). >>>> >>>> Note that in most cases the detailed error response from the server >>>> is used instead of this generic error code for just the HTTP status. >>>> >>>> * subversion/libsvn_ra_serf/util.c >>>> (svn_ra_serf__error_on_status): Tweak generic error for http status 405. >>>> >>>> Modified: >>>> subversion/trunk/subversion/libsvn_ra_serf/util.c >>>> >>>> Modified: subversion/trunk/subversion/libsvn_ra_serf/util.c >>>> URL: >> http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_ra_serf/util.c >> ?rev=1665195&r1=1665194&r2=1665195&view=diff >> ================================================================ >> ============== >>>> --- subversion/trunk/subversion/libsvn_ra_serf/util.c (original) >>>> +++ subversion/trunk/subversion/libsvn_ra_serf/util.c Mon Mar 9 11:39:39 >> 2015 >>>> @@ -1762,8 +1762,8 @@ svn_ra_serf__error_on_status(serf_status >>>> return svn_error_createf(SVN_ERR_FS_NOT_FOUND, NULL, >>>> _("'%s' path not found"), path); >>>> case 405: >>>> - return svn_error_createf(SVN_ERR_FS_OUT_OF_DATE, NULL, >>>> - _("'%s' is out of date"), path); >>>> + return svn_error_createf(SVN_ERR_RA_DAV_FORBIDDEN, NULL, >>>> + _("HTTP method is not allowed on '%s'"), >>>> path); >>>> case 409: >>>> return svn_error_createf(SVN_ERR_FS_CONFLICT, NULL, >>>> _("'%s' conflicts"), path); >>> -1, please revert this. >>> >>> * SVN_ERR_RA_DAV_FORBIDDEN is the wrong error code to use here. >>> "Forbidden" is 403, not 405; the difference is significant. >>> * Far from being a "better response," the new error message is >>> arguably more confusing to users than the old one. Most Subversion >>> users understand "out of date," but I bet most don't know what an >>> HTTP method is. >>> * Even if we decided that it's OK to confuse users with the details of >>> the HTTP protocol, you'd have to tell them *which* method is not >>> allowed. >> I'm trying to understand how r1666096 makes things better. We're now >> using SVN_ERR_RA_NOT_IMPLEMENTED to flag a 405 error from the server, >> which, IMO, is even worse than before: this error code is used by the RA >> loader when an URL scheme doesn't map to an RA implementation, and in >> libsvn_client when a particular RA implementation doesn't support an RA >> API (which should never happen anyway); neither of these have anything >> whatsoever to do with the 405 HTTP status. >> >> Effectively, that commit only addresses my third point. >> >> Can we please start by understanding when, exactly, we can receive a 405 >> from the server? I can imagine several scenarios: >> >> * LOCK something not at HEAD (IIUC, this is handled separately); >> * Write (PUT, MKCOL, etc.) to a repository on a read-only connection; >> * Invalid server configuration (e.g., a missing method in a <Limit> >> block); >> * Non-conformant proxy between client and server. > Yes... > > And when we receive a 405 from mod_dav, we actually receive a detailed server > error, which contains the actual apr code of the backend. > > The original error handling was added in the initial development of ra_serf, > when the developers were mostly interested in getting the tests running. > Instead of fixing that they actually ignored the server generated error (as > was very common then), they just handled error 405. > > Since then the problem of not detecting the real errors has been fixed, so > the only case where we see it now is on servers and proxies that really don't > implement this (or other) features... Returning 'out of date' (which is > generally only found in detailed server reports), makes libsvn_client's > commit processing trigger specific behavior that we never test for, because > our server never returns this error. > > > We should at least return something else than out of date... Preferably > something that a user reading the error would understand (and doesn't make > 'svn' recommend to simply update when you commit to a URL that isn't > configured to allow commits, such as currently happens on > http://serf.googlecode.com/svn/trunk when you try to commit without rebasing > to https)
Is there any reason not to define a new error code for this condition? Neither ...DAV_FORBIDDEN nor ...RA_NOT_IMPLEMENTED are appropriate. -- Brane
