Mailing-List: contact dev-help@subversion.apache.org; run by ezmlm
Precedence: bulk
Received-SPF: pass (athena.apache.org: domain of bert@qqmail.nl designates
 74.125.82.171 as permitted sender)
From: "Bert Huijben" <bert@qqmail.nl>
To: =?utf-8?Q?'Branko_=C4=8Cibej'?= <brane@wandisco.com>,
	<dev@subversion.apache.org>
References: <20150309113940.38AD2AC02F6@hades.apache.org>
 <550117D3.7060808@wandisco.com> <55017306.1090109@wandisco.com>
In-Reply-To: <55017306.1090109@wandisco.com>
Subject: RE: svn commit: r1665195 -
 /subversion/trunk/subversion/libsvn_ra_serf/util.c
Date: Thu, 12 Mar 2015 14:19:50 +0100
Message-ID: <01e601d05cc7$39bd29d0$ad377d70$@qqmail.nl>
MIME-Version: 1.0
Content-Type: text/plain;
	charset="utf-8"
Content-Transfer-Encoding: quoted-printable
Thread-Index: AQJv/sSg0Nf3hZaBlPf9bMfTnclnNgIAo9knAmjc5habtj7U0A==
Content-Language: nl


> -----Original Message-----
> From: Branko =C4=8Cibej [mailto:brane@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
>=20
> On 12.03.2015 05:36, Branko =C4=8Cibej 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/u=
til.c
> ?rev=3D1665195&r1=3D1665194&r2=3D1665195&view=3Ddiff
> >>
> =
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
> >> --- 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.
>=20
> 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.
>=20
> Effectively, that commit only addresses my third point.
>=20
> Can we please start by understanding when, exactly, we can receive a =
405
> from the server? I can imagine several scenarios:
>=20
>   * 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)

	Bert

>=20
> Are there other cases?
>=20
> 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.
>=20
> In any case, I still maintain that "method not allowed" is not a very
> useful message for the user.
>=20
> -- Brane