Mailing-List: contact dev-help@subversion.apache.org; run by ezmlm
Precedence: bulk
Received-SPF: pass (nike.apache.org: domain of brane@wandisco.com designates
 209.85.212.176 as permitted sender)
Message-ID: <55017306.1090109@wandisco.com>
Date: Thu, 12 Mar 2015 12:05:42 +0100
From: =?UTF-8?B?QnJhbmtvIMSMaWJlag==?= <brane@wandisco.com>
Organization: WANdisco
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.10;
 rv:31.0) Gecko/20100101 Thunderbird/31.5.0
MIME-Version: 1.0
To: dev@subversion.apache.org
Subject: Re: svn commit: r1665195 -
 /subversion/trunk/subversion/libsvn_ra_serf/util.c
References: <20150309113940.38AD2AC02F6@hades.apache.org>
 <550117D3.7060808@wandisco.com>
In-Reply-To: <550117D3.7060808@wandisco.com>
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 8bit

On 12.03.2015 05:36, Branko Čibej wrote:
> On 09.03.2015 12:39, rhuijben@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.

Are there other cases?

Of the above 4 cases, we already handle the first and have no control
over the third and fourth. The question is, can we differentiate the
second from the third and fourth cases? It would be useful to be able to
tell the user that they're committing to a read-only repository.

In any case, I still maintain that "method not allowed" is not a very
useful message for the user.

-- Brane