You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Daniel Shahaf <d....@daniel.shahaf.name> on 2013/07/09 22:16:19 UTC
Re: svn commit: r1501371 -
/subversion/trunk/subversion/libsvn_ra_serf/util_error.c
Bert Huijben wrote on Tue, Jul 09, 2013 at 20:11:57 +0200:
> Same as for all previous patches in line:
>
"All previous patches"? There was one previous patch along these lines.
> How is this going to help?
I already told you how: it is going to help because API users can't do
anything with the value 120171 that they presently receive. The
outermost error must be defined by APR or by Subversion. 120171 is
neither.
The impact on users is (a) another error number to search for, (b) the
text "While handling serf response:" in the error message. That's not
a big change, it's a very small improvement.
Beyond that, I give up. I've already said everything I have to say.
I *still* haven't heard a valid reason for your veto (can you name an
API consumer whose code would be broken by this?)
And to be honest, I get the impression that you cast a -1 partly because
of what you *assumed* the patch does, even though what it does and what
you assumed it does are not the same thing.
Re: Semantics of svn_error_t->apr_err
Posted by 'Daniel Shahaf' <da...@apache.org>.
Bert Huijben wrote on Mon, Aug 12, 2013 at 14:16:40 +0200:
>
>
> > -----Original Message-----
> > From: Ivan Zhakov [mailto:ivan@visualsvn.com]
> > Sent: maandag 15 juli 2013 18:35
> > To: Subversion Development; Daniel Shahaf
> > Cc: Bert Huijben
> > Subject: Re: Semantics of svn_error_t->apr_err
> >
> > On Wed, Jul 10, 2013 at 12:40 AM, Daniel Shahaf <d....@daniel.shahaf.name>
> > wrote:
> > > Bert Huijben wrote on Tue, Jul 09, 2013 at 22:32:00 +0200:
> > >> > -----Original Message-----
> > >> > From: Daniel Shahaf [mailto:d.s@daniel.shahaf.name]
> > >> > Bert Huijben wrote on Tue, Jul 09, 2013 at 20:11:57 +0200:
> > >> > > How is this going to help?
> > >> >
> > >> > I already told you how: it is going to help because API users can't do
> > >> > anything with the value 120171 that they presently receive. The
> > >> > outermost error must be defined by APR or by Subversion. 120171 is
> > >> > neither.
> > >>
> > >> *Why* ?
> > >>
> > >> There is no rule that apr_err must be set to something that is defined by
> > >> APR or Subversion.
> > >>
> > >
> > > I've nothing new to say. I think every svn_error_t we return from our
> > > APIs should have an APR_ERR member which is either:
> > >
> > > - equal to SVN_WARNING;
> > > or
> > > - in some SVN_ERR_CATEGORY_*_START, in the sense defined by
> > > SVN_ERROR_IN_CATEGORY();
> > > or
> > > - outside the [APR_OS_START_USERERR,
> > APR_OS_START_USERERR+500000]
> > > range, *and* valid according to APR's concept of validity for
> > > apr_status_t values outside that range (which in particular means:
> > > be non-zero).
> > >
> > > Can someone please weigh in on this issue?
> > For client apr_err is just error code. Client uses
> > svn_err_best_message() to get error message and there're some known
> > APR, Subversion *and* serf error codes.
> >
> > So please revert back old behavior with propagating serf error codes
> > to the caller. It's very important for clients to have distinguished
> > error codes for different cases. Client could use to provide special
> > suggestions how to fix this error or something. Error code 'some error
> > from serf' does not help in such cases.
> >
> > Original code in ra_serf constructed all svn_error_t with serf error
> > codes with custom error message, so user should never get 'APR doesn't
> > understand this error code' message. It should be fixed if we
> > construct svn_error_t with serf error code without custom error
> > message.
>
> Ping?
Thanks for this. It was on my radar to get back to this thread, but
I had it mentally classed as "things to look at before 1.9 is branched"
so didn't treat it as urgent. In the meantime I reverted the change
on trunk. Sorry for the delay.
RE: Semantics of svn_error_t->apr_err
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: Ivan Zhakov [mailto:ivan@visualsvn.com]
> Sent: maandag 15 juli 2013 18:35
> To: Subversion Development; Daniel Shahaf
> Cc: Bert Huijben
> Subject: Re: Semantics of svn_error_t->apr_err
>
> On Wed, Jul 10, 2013 at 12:40 AM, Daniel Shahaf <d....@daniel.shahaf.name>
> wrote:
> > Bert Huijben wrote on Tue, Jul 09, 2013 at 22:32:00 +0200:
> >> > -----Original Message-----
> >> > From: Daniel Shahaf [mailto:d.s@daniel.shahaf.name]
> >> > Bert Huijben wrote on Tue, Jul 09, 2013 at 20:11:57 +0200:
> >> > > How is this going to help?
> >> >
> >> > I already told you how: it is going to help because API users can't do
> >> > anything with the value 120171 that they presently receive. The
> >> > outermost error must be defined by APR or by Subversion. 120171 is
> >> > neither.
> >>
> >> *Why* ?
> >>
> >> There is no rule that apr_err must be set to something that is defined by
> >> APR or Subversion.
> >>
> >
> > I've nothing new to say. I think every svn_error_t we return from our
> > APIs should have an APR_ERR member which is either:
> >
> > - equal to SVN_WARNING;
> > or
> > - in some SVN_ERR_CATEGORY_*_START, in the sense defined by
> > SVN_ERROR_IN_CATEGORY();
> > or
> > - outside the [APR_OS_START_USERERR,
> APR_OS_START_USERERR+500000]
> > range, *and* valid according to APR's concept of validity for
> > apr_status_t values outside that range (which in particular means:
> > be non-zero).
> >
> > Can someone please weigh in on this issue?
> For client apr_err is just error code. Client uses
> svn_err_best_message() to get error message and there're some known
> APR, Subversion *and* serf error codes.
>
> So please revert back old behavior with propagating serf error codes
> to the caller. It's very important for clients to have distinguished
> error codes for different cases. Client could use to provide special
> suggestions how to fix this error or something. Error code 'some error
> from serf' does not help in such cases.
>
> Original code in ra_serf constructed all svn_error_t with serf error
> codes with custom error message, so user should never get 'APR doesn't
> understand this error code' message. It should be fixed if we
> construct svn_error_t with serf error code without custom error
> message.
Ping?
Bert
>
> --
> Ivan Zhakov
Re: Semantics of svn_error_t->apr_err
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On Wed, Jul 10, 2013 at 12:40 AM, Daniel Shahaf <d....@daniel.shahaf.name> wrote:
> Bert Huijben wrote on Tue, Jul 09, 2013 at 22:32:00 +0200:
>> > -----Original Message-----
>> > From: Daniel Shahaf [mailto:d.s@daniel.shahaf.name]
>> > Bert Huijben wrote on Tue, Jul 09, 2013 at 20:11:57 +0200:
>> > > How is this going to help?
>> >
>> > I already told you how: it is going to help because API users can't do
>> > anything with the value 120171 that they presently receive. The
>> > outermost error must be defined by APR or by Subversion. 120171 is
>> > neither.
>>
>> *Why* ?
>>
>> There is no rule that apr_err must be set to something that is defined by
>> APR or Subversion.
>>
>
> I've nothing new to say. I think every svn_error_t we return from our
> APIs should have an APR_ERR member which is either:
>
> - equal to SVN_WARNING;
> or
> - in some SVN_ERR_CATEGORY_*_START, in the sense defined by
> SVN_ERROR_IN_CATEGORY();
> or
> - outside the [APR_OS_START_USERERR, APR_OS_START_USERERR+500000]
> range, *and* valid according to APR's concept of validity for
> apr_status_t values outside that range (which in particular means:
> be non-zero).
>
> Can someone please weigh in on this issue?
For client apr_err is just error code. Client uses
svn_err_best_message() to get error message and there're some known
APR, Subversion *and* serf error codes.
So please revert back old behavior with propagating serf error codes
to the caller. It's very important for clients to have distinguished
error codes for different cases. Client could use to provide special
suggestions how to fix this error or something. Error code 'some error
from serf' does not help in such cases.
Original code in ra_serf constructed all svn_error_t with serf error
codes with custom error message, so user should never get 'APR doesn't
understand this error code' message. It should be fixed if we
construct svn_error_t with serf error code without custom error
message.
--
Ivan Zhakov
Re: Semantics of svn_error_t->apr_err
Posted by Greg Stein <gs...@gmail.com>.
On Wed, Jul 10, 2013 at 1:08 PM, Daniel Shahaf <d....@daniel.shahaf.name> wrote:
>...
> Okay. Suppose an API caller gets 120171 as the error code from
> svn_ra_foo(). I ignore the question of how to recognise that 120171 is
> not a Subversion error[1].
>
> 1 - How does he know that 120171 is a serf error?
>
> 2 - Given that 120171 is a serf error, how does he know that it is "SSL
> communications related errors" (docstring of SERF_ERROR_SSL_COMM_FAILED)?
>
> 3 - How does such an API caller get an error string for that error code?
>
> What are your answers to these questions?
You asked for background. Done. Now, I'm going to artfully dodge
answering/responding those questions. :-D
I think my real answer is that the system wasn't very well-defined,
and nobody bothered to really push against it in the intervening
years. You're now attempting to correct for those problems. Fair
enough.
It might be easier to get consensus by addressing it in that light.
>...
> For 1 again, I suggest alternatively: add
> #define SVN_ERROR_IS_SERF_ERROR(status) \
> ((status) > APR_OS_START_USERERR+100 && \
> (status) < APR_OS_START_USERERR+200)
> to our public API. (I'm working off your statement that the reservation
> is one hundred codes.)
>= ... but yeah. I like this one. Our API is quite clear that serf is in there (ref: SVN_ERR_RA_SERF_CATEGORY_START). Or Berkeley DB. Or SQLite. I think it is fair to surface those aspects from svn.
> I don't have a specific idea for 2. Requiring API consumers to include
> serf.h in their builds is less than ideal:
I think an API client can say "I don't know this error. I'll treat it
like APR_EGENERAL and just throw the message at the user." With
SVN_ERR_IS_SERF_ERROR(), they can classify the error some more.
>...
> [2] As an aside:
> subversion/include/svn_error_codes.h:74: SVN_WARNING = APR_OS_START_USERERR + 1,
> modules/proxy/ajp.h:86:#define AJP_EOVERFLOW (APR_OS_START_USERERR + 1)
> include/apreq_error.h:53:#define APREQ_ERROR_GENERAL APR_OS_START_USERERR
> include/apreq_error.h:55:#define APREQ_ERROR_TAINTED (APREQ_ERROR_GENERAL + 1)
I might suggest a patch to apr_errno.h to state these users and their
ranges. And maybe it would be APR_ERR_IS_SERF_ERROR() and
APR_ERR_IS_SVN_ERROR().
And fix the error_codes doc to release its claim on the
[USERERR,USERERR+4999] range. And possibly reduce its claim of "100
categories". We use 23 right now. And none of those use 100 or more
codes, so we could change our definition to 100-code ranges (but we
need to consider whether people use range/category tests; that might
throw off compat). Maybe we say *new* categories are 100 codes, and
only claim (say) 27 more, for a total of 50 categories ever.
Cheers,
-g
Re: Semantics of svn_error_t->apr_err
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Greg Stein wrote on Tue, Jul 09, 2013 at 20:59:48 -0400:
> On Tue, Jul 9, 2013 at 4:40 PM, Daniel Shahaf <d....@daniel.shahaf.name> wrote:
> > Bert Huijben wrote on Tue, Jul 09, 2013 at 22:32:00 +0200:
> >...
> >> There is no rule that apr_err must be set to something that is defined by
> >> APR or Subversion.
>
> Correct.
>
...
> apr_status_t is intended to hold all of these errors, so we shouldn't
> attempt to remap them. Handle the ones that you recognize, and
> propagate everything else (maybe somebody up the call chain knows how
> to handle it). And that caller may be outside of our APIs. We publish
> an apr_status_t to them for review.
Okay. Suppose an API caller gets 120171 as the error code from
svn_ra_foo(). I ignore the question of how to recognise that 120171 is
not a Subversion error[1].
1 - How does he know that 120171 is a serf error?
2 - Given that 120171 is a serf error, how does he know that it is "SSL
communications related errors" (docstring of SERF_ERROR_SSL_COMM_FAILED)?
3 - How does such an API caller get an error string for that error code?
What are your answers to these questions?
Today the answers are: 1 - by assuming that Subversion doesn't/won't
link to another apr_status_t library which assigns a meaning to 120171;
2 - by hardcoding the values, or including serf.h in their build; 3 - by
inspecting err->message.
For 1, I suggest to remove the need for that assumption by fronting the
serf error code by a Subversion error code which declares
"The ->apr_err values in children of this error are libserf-generated
(so should be passed to serf_error_message() or apr_strerror(), for example)".
The rationale here is to let every (current and not-yet-existing)
library use the entire APR_OS_START_USERERR range (just like Private Use
codepoints in Unicode), rather than having to have APR maintain
a registry.
For 1 again, I suggest alternatively: add
#define SVN_ERROR_IS_SERF_ERROR(status) \
((status) > APR_OS_START_USERERR+100 && \
(status) < APR_OS_START_USERERR+200)
to our public API. (I'm working off your statement that the reservation
is one hundred codes.)
I don't have a specific idea for 2. Requiring API consumers to include
serf.h in their builds is less than ideal: how non-C API users deal with
this? I think their options are to roll their own serf.h -> $lang error
codes extractor or to consider 120171 an "unrecognized" error code
(since no SVN_ERROR_IN_CATEGORY() returns TRUE for it).
For 3 I think the existing situation is fine.
Daniel
[1] See svn_error_codes.h:95 for the problem: we reserve the right to
define more SVN_ERR_* codes in the [120001, 125000] range, so future
compatibility considerations kick in.
[2] As an aside:
subversion/include/svn_error_codes.h:74: SVN_WARNING = APR_OS_START_USERERR + 1,
modules/proxy/ajp.h:86:#define AJP_EOVERFLOW (APR_OS_START_USERERR + 1)
include/apreq_error.h:53:#define APREQ_ERROR_GENERAL APR_OS_START_USERERR
include/apreq_error.h:55:#define APREQ_ERROR_TAINTED (APREQ_ERROR_GENERAL + 1)
Re: Semantics of svn_error_t->apr_err
Posted by Greg Stein <gs...@gmail.com>.
On Tue, Jul 9, 2013 at 4:40 PM, Daniel Shahaf <d....@daniel.shahaf.name> wrote:
> Bert Huijben wrote on Tue, Jul 09, 2013 at 22:32:00 +0200:
>...
>> There is no rule that apr_err must be set to something that is defined by
>> APR or Subversion.
Correct.
> I've nothing new to say. I think every svn_error_t we return from our
> APIs should have an APR_ERR member which is either:
>
> - equal to SVN_WARNING;
> or
> - in some SVN_ERR_CATEGORY_*_START, in the sense defined by
> SVN_ERROR_IN_CATEGORY();
> or
> - outside the [APR_OS_START_USERERR, APR_OS_START_USERERR+500000]
> range, *and* valid according to APR's concept of validity for
> apr_status_t values outside that range (which in particular means:
> be non-zero).
>
> Can someone please weigh in on this issue?
The apr_status_t type was designed/intended to hold multiple ranges of
errors. It can hold errors from apr, apr-util, httpd, svn, serf, and
third-party/unknown libraries.
The *problem* is that a central registry of who gets what range was
never codified. There was a lot of discussion way back when, but no
consensus. We added svn's range into the apr_errno.h header, and were
willing to add other libraries/products. Dunno whether the current APR
folks still feel that way.
And because it never got codified, Subversion just "used up" all the
space. Or maybe APR defined it based on svn's use. I don't recall the
exact ordering.
Note that Apache HTTPD defines errors in there, at USERERR+1000
(weirdly, it only defines a single error; it does say that it reserves
1000 error codes).
When we went to define some serf errors, we recognized that Subversion
(by design or accident) begins its errors at USERERR+5000. So serf
decided to take USERERR+100 to stay away from svn and httpd. It
doesn't make it explicit in the header, but there is an implicit
assumption of a 100 slot reservation.
apr_status_t is intended to hold all of these errors, so we shouldn't
attempt to remap them. Handle the ones that you recognize, and
propagate everything else (maybe somebody up the call chain knows how
to handle it). And that caller may be outside of our APIs. We publish
an apr_status_t to them for review.
Cheers,
-g
Semantics of svn_error_t->apr_err
Posted by 'Daniel Shahaf' <d....@daniel.shahaf.name>.
Bert Huijben wrote on Tue, Jul 09, 2013 at 22:32:00 +0200:
> > -----Original Message-----
> > From: Daniel Shahaf [mailto:d.s@daniel.shahaf.name]
> > Bert Huijben wrote on Tue, Jul 09, 2013 at 20:11:57 +0200:
> > > How is this going to help?
> >
> > I already told you how: it is going to help because API users can't do
> > anything with the value 120171 that they presently receive. The
> > outermost error must be defined by APR or by Subversion. 120171 is
> > neither.
>
> *Why* ?
>
> There is no rule that apr_err must be set to something that is defined by
> APR or Subversion.
>
I've nothing new to say. I think every svn_error_t we return from our
APIs should have an APR_ERR member which is either:
- equal to SVN_WARNING;
or
- in some SVN_ERR_CATEGORY_*_START, in the sense defined by
SVN_ERROR_IN_CATEGORY();
or
- outside the [APR_OS_START_USERERR, APR_OS_START_USERERR+500000]
range, *and* valid according to APR's concept of validity for
apr_status_t values outside that range (which in particular means:
be non-zero).
Can someone please weigh in on this issue?
Re: svn commit: r1501371 -
/subversion/trunk/subversion/libsvn_ra_serf/util_error.c
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Bert Huijben wrote on Tue, Jul 09, 2013 at 23:00:31 +0200:
> But note that the original patch this discussion started with, did
> exactly the opposite: It decoupled the message and the error code,
> which made svn produce the warning that it didn't understand the
> error.
>
> So: how about agree-ing that introducing this intermediate,
> untranslatable error was not the right thing to do?
I already acknowledged that the original patch had a bug (it caused
a "APR doesn't know this error code" to appear in the chain), and fixed
that bug in r1501371.
Re: svn commit: r1501371 - /subversion/trunk/subversion/libsvn_ra_serf/util_error.c
Posted by Ben Reser <be...@reser.org>.
On Tue, Jul 9, 2013 at 8:04 PM, Joseph Schaefer <jo...@yahoo.com> wrote:
> You've made some interesting and educational points on what veto rights are all about Greg, and I appreciate that quite a bit. I do think that it'd be worthwhile to document this on the foundation website because it's a common point of frustration for many projects, from well-established ones to incubator podlings.
Well this exists:
http://www.apache.org/foundation/voting.html
Maybe it needs to be improved.
Re: svn commit: r1501371 - /subversion/trunk/subversion/libsvn_ra_serf/util_error.c
Posted by Joseph Schaefer <jo...@yahoo.com>.
You've made some interesting and educational points on what veto rights are all about Greg, and I appreciate that quite a bit. I do think that it'd be worthwhile to document this on the foundation website because it's a common point of frustration for many projects, from well-established ones to incubator podlings.
YMMV
Sent from my iPhone
On Jul 9, 2013, at 10:54 PM, Greg Stein <gs...@gmail.com> wrote:
> On Tue, Jul 9, 2013 at 9:48 PM, Daniel Shahaf <da...@apache.org> wrote:
>> ...
>> A veto not accompanied by a technical reason is invalid. I am seeking
>> consensus on the dev@ list that no technical reason was given. No one
>> is going to strip anyone's bits.
>
> It's a slippery slope that you want to avoid. Very very much.
>
> One of the things he said, "And API users want to see the most
> interesting/unique value for every error, and wrapping with generic
> codes is exactly working against that."
>
> That's quite technical. Or simply saying "the serf code should remain
> on top of the error stack" is technical.
>
> It doesn't have to make sense to you, and you don't have to
> acknowledge/understand his point. His veto still stands.
>
> Again: you don't get to declare a veto "invalid", and the dev@ list is
> not the place to seek consensus for that either. Take it to private@,
> as I suggested, or work through the discussion. And if you want to
> strip his veto, then you have to toss him from the PMC. As I said, PMC
> Members have unilateral power. A majority or a supermajority doesn't
> get to vote and say "bah. that veto is invalid." That throws out the
> entire point of the veto system. So yes: you *are* talking about
> stripping bits because that is your only path.
>
> Further: this isn't a Rules game. Bert disagrees. Why don't you simply
> work through the disagreement? I've seen PMCs played out as a Rules
> game. It is horrible and destructive.
>
> Cheers,
> -g
Re: svn commit: r1501371 - /subversion/trunk/subversion/libsvn_ra_serf/util_error.c
Posted by Greg Stein <gs...@gmail.com>.
On Tue, Jul 9, 2013 at 9:48 PM, Daniel Shahaf <da...@apache.org> wrote:
>...
> A veto not accompanied by a technical reason is invalid. I am seeking
> consensus on the dev@ list that no technical reason was given. No one
> is going to strip anyone's bits.
It's a slippery slope that you want to avoid. Very very much.
One of the things he said, "And API users want to see the most
interesting/unique value for every error, and wrapping with generic
codes is exactly working against that."
That's quite technical. Or simply saying "the serf code should remain
on top of the error stack" is technical.
It doesn't have to make sense to you, and you don't have to
acknowledge/understand his point. His veto still stands.
Again: you don't get to declare a veto "invalid", and the dev@ list is
not the place to seek consensus for that either. Take it to private@,
as I suggested, or work through the discussion. And if you want to
strip his veto, then you have to toss him from the PMC. As I said, PMC
Members have unilateral power. A majority or a supermajority doesn't
get to vote and say "bah. that veto is invalid." That throws out the
entire point of the veto system. So yes: you *are* talking about
stripping bits because that is your only path.
Further: this isn't a Rules game. Bert disagrees. Why don't you simply
work through the disagreement? I've seen PMCs played out as a Rules
game. It is horrible and destructive.
Cheers,
-g
Re: svn commit: r1501371 - /subversion/trunk/subversion/libsvn_ra_serf/util_error.c
Posted by Ben Reser <be...@reser.org>.
On Tue, Jul 9, 2013 at 6:48 PM, Daniel Shahaf <da...@apache.org> wrote:
> A veto not accompanied by a technical reason is invalid. I am seeking
> consensus on the dev@ list that no technical reason was given. No one
> is going to strip anyone's bits.
Daniel is right on this one:
"To prevent vetos from being used capriciously, they must be
accompanied by a technical justification showing why the change is bad
(opens a security exposure, negatively affects performance, etc. ). A
veto without a justification is invalid and has no weight."
What's not clear is who decides that a veto is without justification.
That said I don't really see how Bert's veto is without justification.
Re: svn commit: r1501371 -
/subversion/trunk/subversion/libsvn_ra_serf/util_error.c
Posted by Daniel Shahaf <da...@apache.org>.
On Tue, Jul 09, 2013 at 09:13:07PM -0400, Greg Stein wrote:
> On Tue, Jul 9, 2013 at 9:10 PM, Greg Stein <gs...@gmail.com> wrote:
> > On Tue, Jul 9, 2013 at 8:15 PM, Daniel Shahaf <da...@apache.org> wrote:
> >>...
> >> So, I submit that your veto lacks a technical basis, and is therefore invalid,
> >> and has no standing.
> >
> > You do not get to make that decision. Certainly not unilaterally. If
> > you feel it is invalid, then your course of action is to bring it to
> > the PMC's private list. Make your case, and then the PMC can take a
> > vote to eject Bert from the PMC, thus making his veto non-binding.
>
> And I should add: you'll need to make a similar argument to the Board,
> since the PMC would merely be recommending removal. The Board passes a
> resolution (or other decision making process) to effect the actual
> removal.
>
> So yeah... you've got a rough hill to climb on this one :-)
Huh?
A veto not accompanied by a technical reason is invalid. I am seeking
consensus on the dev@ list that no technical reason was given. No one
is going to strip anyone's bits.
Daniel
Re: svn commit: r1501371 - /subversion/trunk/subversion/libsvn_ra_serf/util_error.c
Posted by Greg Stein <gs...@gmail.com>.
On Tue, Jul 9, 2013 at 9:10 PM, Greg Stein <gs...@gmail.com> wrote:
> On Tue, Jul 9, 2013 at 8:15 PM, Daniel Shahaf <da...@apache.org> wrote:
>>...
>> So, I submit that your veto lacks a technical basis, and is therefore invalid,
>> and has no standing.
>
> You do not get to make that decision. Certainly not unilaterally. If
> you feel it is invalid, then your course of action is to bring it to
> the PMC's private list. Make your case, and then the PMC can take a
> vote to eject Bert from the PMC, thus making his veto non-binding.
And I should add: you'll need to make a similar argument to the Board,
since the PMC would merely be recommending removal. The Board passes a
resolution (or other decision making process) to effect the actual
removal.
So yeah... you've got a rough hill to climb on this one :-)
Cheers,
-g
Re: svn commit: r1501371 - /subversion/trunk/subversion/libsvn_ra_serf/util_error.c
Posted by Greg Stein <gs...@gmail.com>.
On Tue, Jul 9, 2013 at 8:15 PM, Daniel Shahaf <da...@apache.org> wrote:
>...
> So, I submit that your veto lacks a technical basis, and is therefore invalid,
> and has no standing.
You do not get to make that decision. Certainly not unilaterally. If
you feel it is invalid, then your course of action is to bring it to
the PMC's private list. Make your case, and then the PMC can take a
vote to eject Bert from the PMC, thus making his veto non-binding.
There is no middle-ground vote -- the veto power exists to avoid
mob/majority rule. Consensus wins, rather than majority.
Meta: when you starting debating on whether a veto is "valid" or not,
then you should stop yourself. Think hard. It is very, very rare to
see somebody willy-nilly play the veto card. *Especially* in the
Subversion community (boy, you wouldn't believe how often vetoes
happen in some other communities; we rarely see them at all, here).
Meta: a veto should always be viewed as the discussion starter. Yes,
it is unilateral, but the solution arrives through consensus: keep the
change, revert the change, do more work to resolve the issue.
Cheers,
-g
Re: svn commit: r1501371 -
/subversion/trunk/subversion/libsvn_ra_serf/util_error.c
Posted by Daniel Shahaf <da...@apache.org>.
On Tue, Jul 09, 2013 at 11:51:09PM +0200, Bert Huijben wrote:
> So: What are we talking about here?
>
> An 'svn'-only user wants to see different error codes?
>
> (Perhaps he added patch over patch to make the numbers more visible in MAINTAINER mode?)
First and foremost, can you please quit making it personal. The subject being
discussed is whether r1501049 and r1501371 should be applied to 1.8.x (and, I
guess, whether they should be backed out from trunk). What patches I may or
may not have committed in the past is not relevant to the technical quality of
those two revisions.
Now, to your claim about seeing different error codes in 'svn', I repeatedly
explained to you that this is not about svn's output, in maintainer mode or
otherwise. This is about providing API users error numbers they can do
something with --- without hardcoding 120171 in their source code (not to
mention including serf.h from their source code, which cannot be assumed to
be in C), without hardcoding the assumption that Serf is the only
apr_status_t-using Subversion dependency, and without hardcoding the
assumption that Serf APR_OS_START_USERERR codes and Subversion
APR_OS_START_USERERR are disjoint.
Now, having said that, I'll put my cards on the table. You repeatedly ignored
my requests to name an API user whose code will be broken by this. You
repeatedly ignored my explanations that this has nothing to do with the
maintainer mode stack traces. You claimed that SVN_ERR_RA_SERF_WRAPPED_ERROR
gets unwrapped, failed to corroborate that claim, and I think I have proven it
wrong.
So, I submit that your veto lacks a technical basis, and is therefore invalid,
and has no standing.
Feel free to provide a technical reason, of course.
Cheers,
Daniel
Re: svn commit: r1501371 -
/subversion/trunk/subversion/libsvn_ra_serf/util_error.c
Posted by Daniel Shahaf <da...@apache.org>.
On Wed, Jul 10, 2013 at 02:52:21AM +0400, Ivan Zhakov wrote:
> On Wed, Jul 10, 2013 at 1:51 AM, Bert Huijben <be...@qqmail.nl> wrote:
> >> -----Original Message-----
> >> From: Branko Čibej [mailto:brane@wandisco.com]
> >> Sent: dinsdag 9 juli 2013 23:12
> >> To: dev@subversion.apache.org
> >> Subject: Re: svn commit: r1501371 -
> >> /subversion/trunk/subversion/libsvn_ra_serf/util_error.c
> >>
> >> On 09.07.2013 23:00, Bert Huijben wrote:
> >> > I would welcome a patch to svn_strerror that improves the default
> >> > error for serf specific error codes,
> >>
> >> That would effectively make our make implementation details of
> >> libsvn_ra_serf part of the public API. That's a non-strarter IMO.
> >>
> >> > or a patch that translates all serf error codes to something within a
> >> > subversion error range.
> >>
> >> If we go down that road, we may as well make Serf part of the Subversion
> >> project. Which I don't believe is going to happen.
> >>
> >> By all means wrap the error messages inside libsvn_ra_serf; I wouldn't
> >> object to extracting the Serf error message and adding a "caused-by:
> >> serf error NNNN" in the same error chain.
> >
> > Which requires tools to start message scraping?
> >
> > My Chinese error parser is not so good, and perhaps somebody changes the .po file with the next patch release?
> >
> >
> > TortoiseSVN, VisualSVN, AnkhSVN and probably most other GUI clients prefer error values over texts.
> > (I don't know about our JavaHL users, but I know more than a few SharpSvn users that have checks for specific error codes in their products)
> >
> I completely agree with Bert that having error is very important for
> GUI clients. The source of error code doesn't matter actually. It just
> some stable identifier of particular error and reason. GUI client may
> suggest solution based on this error code. Or ignore it in some cases.
As already mentioned, the original Serf error code remains, it is simply
placed in the second place in the error chain, and the front place is
given to an error that describes the second place's apr_status_t value
as a Serf one.
API users that use svn_error_find_cause() won't even know the
difference. (And given that they are looking for a Serf-generated error
code, looking at the bottom rather than top of the chain is probably a
good idea --- but I admit I'm talking with 20/20 hindsight here.)
Re: svn commit: r1501371 - /subversion/trunk/subversion/libsvn_ra_serf/util_error.c
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On Wed, Jul 10, 2013 at 1:51 AM, Bert Huijben <be...@qqmail.nl> wrote:
>> -----Original Message-----
>> From: Branko Čibej [mailto:brane@wandisco.com]
>> Sent: dinsdag 9 juli 2013 23:12
>> To: dev@subversion.apache.org
>> Subject: Re: svn commit: r1501371 -
>> /subversion/trunk/subversion/libsvn_ra_serf/util_error.c
>>
>> On 09.07.2013 23:00, Bert Huijben wrote:
>> > I would welcome a patch to svn_strerror that improves the default
>> > error for serf specific error codes,
>>
>> That would effectively make our make implementation details of
>> libsvn_ra_serf part of the public API. That's a non-strarter IMO.
>>
>> > or a patch that translates all serf error codes to something within a
>> > subversion error range.
>>
>> If we go down that road, we may as well make Serf part of the Subversion
>> project. Which I don't believe is going to happen.
>>
>> By all means wrap the error messages inside libsvn_ra_serf; I wouldn't
>> object to extracting the Serf error message and adding a "caused-by:
>> serf error NNNN" in the same error chain.
>
> Which requires tools to start message scraping?
>
> My Chinese error parser is not so good, and perhaps somebody changes the .po file with the next patch release?
>
>
> TortoiseSVN, VisualSVN, AnkhSVN and probably most other GUI clients prefer error values over texts.
> (I don't know about our JavaHL users, but I know more than a few SharpSvn users that have checks for specific error codes in their products)
>
I completely agree with Bert that having error is very important for
GUI clients. The source of error code doesn't matter actually. It just
some stable identifier of particular error and reason. GUI client may
suggest solution based on this error code. Or ignore it in some cases.
--
Ivan Zhakov
CTO | VisualSVN | http://www.visualsvn.com
Re: svn commit: r1501371 - /subversion/trunk/subversion/libsvn_ra_serf/util_error.c
Posted by Branko Čibej <br...@wandisco.com>.
On 10.07.2013 02:16, Daniel Shahaf wrote:
> On Tue, Jul 09, 2013 at 11:51:09PM +0200, Bert Huijben wrote:
>> (I don't know about our JavaHL users, but I know more than a few SharpSvn users that have checks for specific error codes in their products)
> Do the codes the check for have svn_error_codes.h names?
Well, I did come across a case where a future JavaHL consumer needs the
symbolic error code names to make heads or tails of the errors. These
were (are) present in the deprecated org.tigris JavaHL API, but were --
for some reason that I've not followed up on yet -- removed during the
migration to org.apache. That is proving to be a bit of a problem, which
I don't know how to address yet.
The upshot is that /if/ I decide it's a good idea to expose symbolic
error names in the JavaHL API, then I certainly would not be happy to
see any non-SVN, non-APR messages at the top of the error stack.
-- Brane
--
Branko Čibej | Director of Subversion
WANdisco // Non-Stop Data
e. brane@wandisco.com
Re: svn commit: r1501371 -
/subversion/trunk/subversion/libsvn_ra_serf/util_error.c
Posted by Daniel Shahaf <da...@apache.org>.
On Tue, Jul 09, 2013 at 11:51:09PM +0200, Bert Huijben wrote:
> (I don't know about our JavaHL users, but I know more than a few SharpSvn users that have checks for specific error codes in their products)
Do the codes the check for have svn_error_codes.h names?
If yes, they are not relevant examples.
If not, why not? And can you please ask them to submit bug reports to users@?
RE: svn commit: r1501371 - /subversion/trunk/subversion/libsvn_ra_serf/util_error.c
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: Branko Čibej [mailto:brane@wandisco.com]
> Sent: dinsdag 9 juli 2013 23:12
> To: dev@subversion.apache.org
> Subject: Re: svn commit: r1501371 -
> /subversion/trunk/subversion/libsvn_ra_serf/util_error.c
>
> On 09.07.2013 23:00, Bert Huijben wrote:
> > I would welcome a patch to svn_strerror that improves the default
> > error for serf specific error codes,
>
> That would effectively make our make implementation details of
> libsvn_ra_serf part of the public API. That's a non-strarter IMO.
>
> > or a patch that translates all serf error codes to something within a
> > subversion error range.
>
> If we go down that road, we may as well make Serf part of the Subversion
> project. Which I don't believe is going to happen.
>
> By all means wrap the error messages inside libsvn_ra_serf; I wouldn't
> object to extracting the Serf error message and adding a "caused-by:
> serf error NNNN" in the same error chain.
Which requires tools to start message scraping?
My Chinese error parser is not so good, and perhaps somebody changes the .po file with the next patch release?
TortoiseSVN, VisualSVN, AnkhSVN and probably most other GUI clients prefer error values over texts.
(I don't know about our JavaHL users, but I know more than a few SharpSvn users that have checks for specific error codes in their products)
The texts are for our users and should tell them enough without them ever looking at the error values.
So: What are we talking about here?
An 'svn'-only user wants to see different error codes?
(Perhaps he added patch over patch to make the numbers more visible in MAINTAINER mode?)
100% agree on: The message should be set before the user sees the error.
But I fail to see why wrapping every error in generic subversion errors is going to help anybody.
Should I handle these error messages by pointing them to specific e-mail addresses?
With errors as WSAECONNFAILED I can at least point users to suggestions on how they should connect to their repository, but with error codes as SVN_ERR_RA_SERF_WRAPPED_ERROR I can only point users to dev{_AT_}subversion.apache.org.
These codes add value to our bindings... It allows more advanced bindings to use this knowledge.
Bert
Re: svn commit: r1501371 - /subversion/trunk/subversion/libsvn_ra_serf/util_error.c
Posted by Branko Čibej <br...@wandisco.com>.
On 09.07.2013 23:00, Bert Huijben wrote:
> I would welcome a patch to svn_strerror that improves the default
> error for serf specific error codes,
That would effectively make our make implementation details of
libsvn_ra_serf part of the public API. That's a non-strarter IMO.
> or a patch that translates all serf error codes to something within a
> subversion error range.
If we go down that road, we may as well make Serf part of the Subversion
project. Which I don't believe is going to happen.
By all means wrap the error messages inside libsvn_ra_serf; I wouldn't
object to extracting the Serf error message and adding a "caused-by:
serf error NNNN" in the same error chain.
-- Brane
--
Branko Čibej | Director of Subversion
WANdisco // Non-Stop Data
e. brane@wandisco.com
RE: svn commit: r1501371 - /subversion/trunk/subversion/libsvn_ra_serf/util_error.c
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: Branko Čibej [mailto:brane@wandisco.com]
> Sent: dinsdag 9 juli 2013 22:48
> To: dev@subversion.apache.org
> Subject: Re: svn commit: r1501371 -
> /subversion/trunk/subversion/libsvn_ra_serf/util_error.c
>
> On 09.07.2013 22:32, Bert Huijben wrote:
> >
> >> -----Original Message-----
> >> From: Daniel Shahaf [mailto:d.s@daniel.shahaf.name]
> >> Sent: dinsdag 9 juli 2013 22:16
> >> To: Bert Huijben
> >> Cc: dev@subversion.apache.org; commits@subversion.apache.org
> >> Subject: Re: svn commit: r1501371 -
> >> /subversion/trunk/subversion/libsvn_ra_serf/util_error.c
> >>
> >> Bert Huijben wrote on Tue, Jul 09, 2013 at 20:11:57 +0200:
> >>> Same as for all previous patches in line:
> >>>
> >> "All previous patches"? There was one previous patch along these lines.
> >>
> >>> How is this going to help?
> >> I already told you how: it is going to help because API users can't do
> >> anything with the value 120171 that they presently receive. The
> >> outermost error must be defined by APR or by Subversion. 120171 is
> >> neither.
> > *Why* ?
> >
> > There is no rule that apr_err must be set to something that is defined by
> > APR or Subversion.
>
> Not as such. But I hope we can agree that there's an implied rule that
> apr_strerror and/or svn_strerror should return a valid error message,
> not complain about not understanding the error code.
Agree:
But I think we should do that by setting the right message before an svn_error_t * exits libsvn_ra_serf, not by wrapping the error again or by introducing yet another item in the error chain.
But note that the original patch this discussion started with, did exactly the opposite: It decoupled the message and the error code, which made svn produce the warning that it didn't understand the error.
So: how about agree-ing that introducing this intermediate, untranslatable error was not the right thing to do?
Note also the replies from Lieven and Ivan that said that the return path in ra_serf should be verified to make sure the right text is provided.
>
>
> > Besides APR *defines* everything in a range to be user defined..
> >
> >
> > This same reasoning applies to wrapping everything to APR_EGENERAL,
> which is
> > just as bad.
> >
> > Every integer value is valid here... And allows checking for it.
> >
> > APR_EGENERAL (=1) doesn't allow checking for by api users. And bindings
> > can't translate textual error messages back to text.
>
> The actual Serf error code is still there, in the error chain, for
> anyone who wants to bother looking for it. But it's really bad user
> experience to see the message "APR can't understand this error" in
> response to something that we /can/ be more specific about.
>
>
> > So *why* do *you* want to change the existing error code to this
> wrapping
> > code that is only used wrapping for non-bucket errors. (Which is a specific
> > set of APR defined error codes that are handled specificly in serf).
>
> Daniel said this a number of times: The Serf API (and, as a consequence,
> Serf's error codes) are not part of our public API. Why then should we
> leave those error codes unwrapped? We wrap everything else, including
> Win32.
We don't wrap them in many cases.
I think about 1 in every 3 error reports I get for AnkhSVN is an OS error wrapped via APR. And these values for different kinds of file-sharing problems and detailed winsock errors certainly aren't mapped by APR values.
(Although APR_EGENERAL is rising in the popularity contest for 1.8.0 :( )
I also return error codes outside the OS / Subversion / Serf range in SharpSvn to have some of my own conditions get through at the other side of wrapping.
And as far as I can tell this is what the apr status (with its ranges) values were designed for.
I would welcome a patch to svn_strerror that improves the default error for serf specific error codes, or a patch that translates all serf error codes to something within a subversion error range.
But I don't want to see the tens of thousands of defined errors in the Windows error header file being mapped to a single unknown Subversion error code for 'I don't understand'. It is better to return the proper error string and the value, so API users that do understand the code can use it.
Bert
>
> -- Brane
>
> --
> Branko Čibej | Director of Subversion
> WANdisco // Non-Stop Data
> e. brane@wandisco.com
Re: svn commit: r1501371 - /subversion/trunk/subversion/libsvn_ra_serf/util_error.c
Posted by Branko Čibej <br...@wandisco.com>.
On 09.07.2013 22:32, Bert Huijben wrote:
>
>> -----Original Message-----
>> From: Daniel Shahaf [mailto:d.s@daniel.shahaf.name]
>> Sent: dinsdag 9 juli 2013 22:16
>> To: Bert Huijben
>> Cc: dev@subversion.apache.org; commits@subversion.apache.org
>> Subject: Re: svn commit: r1501371 -
>> /subversion/trunk/subversion/libsvn_ra_serf/util_error.c
>>
>> Bert Huijben wrote on Tue, Jul 09, 2013 at 20:11:57 +0200:
>>> Same as for all previous patches in line:
>>>
>> "All previous patches"? There was one previous patch along these lines.
>>
>>> How is this going to help?
>> I already told you how: it is going to help because API users can't do
>> anything with the value 120171 that they presently receive. The
>> outermost error must be defined by APR or by Subversion. 120171 is
>> neither.
> *Why* ?
>
> There is no rule that apr_err must be set to something that is defined by
> APR or Subversion.
Not as such. But I hope we can agree that there's an implied rule that
apr_strerror and/or svn_strerror should return a valid error message,
not complain about not understanding the error code.
> Besides APR *defines* everything in a range to be user defined..
>
>
> This same reasoning applies to wrapping everything to APR_EGENERAL, which is
> just as bad.
>
> Every integer value is valid here... And allows checking for it.
>
> APR_EGENERAL (=1) doesn't allow checking for by api users. And bindings
> can't translate textual error messages back to text.
The actual Serf error code is still there, in the error chain, for
anyone who wants to bother looking for it. But it's really bad user
experience to see the message "APR can't understand this error" in
response to something that we /can/ be more specific about.
> So *why* do *you* want to change the existing error code to this wrapping
> code that is only used wrapping for non-bucket errors. (Which is a specific
> set of APR defined error codes that are handled specificly in serf).
Daniel said this a number of times: The Serf API (and, as a consequence,
Serf's error codes) are not part of our public API. Why then should we
leave those error codes unwrapped? We wrap everything else, including Win32.
-- Brane
--
Branko Čibej | Director of Subversion
WANdisco // Non-Stop Data
e. brane@wandisco.com
Semantics of svn_error_t->apr_err
Posted by 'Daniel Shahaf' <d....@daniel.shahaf.name>.
Bert Huijben wrote on Tue, Jul 09, 2013 at 22:32:00 +0200:
> > -----Original Message-----
> > From: Daniel Shahaf [mailto:d.s@daniel.shahaf.name]
> > Bert Huijben wrote on Tue, Jul 09, 2013 at 20:11:57 +0200:
> > > How is this going to help?
> >
> > I already told you how: it is going to help because API users can't do
> > anything with the value 120171 that they presently receive. The
> > outermost error must be defined by APR or by Subversion. 120171 is
> > neither.
>
> *Why* ?
>
> There is no rule that apr_err must be set to something that is defined by
> APR or Subversion.
>
I've nothing new to say. I think every svn_error_t we return from our
APIs should have an APR_ERR member which is either:
- equal to SVN_WARNING;
or
- in some SVN_ERR_CATEGORY_*_START, in the sense defined by
SVN_ERROR_IN_CATEGORY();
or
- outside the [APR_OS_START_USERERR, APR_OS_START_USERERR+500000]
range, *and* valid according to APR's concept of validity for
apr_status_t values outside that range (which in particular means:
be non-zero).
Can someone please weigh in on this issue?
RE: svn commit: r1501371 - /subversion/trunk/subversion/libsvn_ra_serf/util_error.c
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: Daniel Shahaf [mailto:d.s@daniel.shahaf.name]
> Sent: dinsdag 9 juli 2013 22:16
> To: Bert Huijben
> Cc: dev@subversion.apache.org; commits@subversion.apache.org
> Subject: Re: svn commit: r1501371 -
> /subversion/trunk/subversion/libsvn_ra_serf/util_error.c
>
> Bert Huijben wrote on Tue, Jul 09, 2013 at 20:11:57 +0200:
> > Same as for all previous patches in line:
> >
>
> "All previous patches"? There was one previous patch along these lines.
>
> > How is this going to help?
>
> I already told you how: it is going to help because API users can't do
> anything with the value 120171 that they presently receive. The
> outermost error must be defined by APR or by Subversion. 120171 is
> neither.
*Why* ?
There is no rule that apr_err must be set to something that is defined by
APR or Subversion.
Besides APR *defines* everything in a range to be user defined..
This same reasoning applies to wrapping everything to APR_EGENERAL, which is
just as bad.
Every integer value is valid here... And allows checking for it.
APR_EGENERAL (=1) doesn't allow checking for by api users. And bindings
can't translate textual error messages back to text.
So *why* do *you* want to change the existing error code to this wrapping
code that is only used wrapping for non-bucket errors. (Which is a specific
set of APR defined error codes that are handled specificly in serf).
I would reason like Raymond Chen (of the oldnewthing fame) here: every
change starts at -1000 points. And needs a good reason to be implemented as
new feature, and an even better reason to be backported as 'fix' to a
maintenance branch.
Bert
RE: svn commit: r1501371 - /subversion/trunk/subversion/libsvn_ra_serf/util_error.c
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: Daniel Shahaf [mailto:d.s@daniel.shahaf.name]
> Sent: dinsdag 9 juli 2013 22:16
> To: Bert Huijben
> Cc: dev@subversion.apache.org; commits@subversion.apache.org
> Subject: Re: svn commit: r1501371 -
> /subversion/trunk/subversion/libsvn_ra_serf/util_error.c
>
> Bert Huijben wrote on Tue, Jul 09, 2013 at 20:11:57 +0200:
> > Same as for all previous patches in line:
> >
>
> "All previous patches"? There was one previous patch along these lines.
>
> > How is this going to help?
>
> I already told you how: it is going to help because API users can't do
> anything with the value 120171 that they presently receive. The
> outermost error must be defined by APR or by Subversion. 120171 is
> neither.
*Why* ?
There is no rule that apr_err must be set to something that is defined by
APR or Subversion.
Besides APR *defines* everything in a range to be user defined..
This same reasoning applies to wrapping everything to APR_EGENERAL, which is
just as bad.
Every integer value is valid here... And allows checking for it.
APR_EGENERAL (=1) doesn't allow checking for by api users. And bindings
can't translate textual error messages back to text.
So *why* do *you* want to change the existing error code to this wrapping
code that is only used wrapping for non-bucket errors. (Which is a specific
set of APR defined error codes that are handled specificly in serf).
I would reason like Raymond Chen (of the oldnewthing fame) here: every
change starts at -1000 points. And needs a good reason to be implemented as
new feature, and an even better reason to be backported as 'fix' to a
maintenance branch.
Bert