On 15.06.2015 20:54, Evgeny Kotkov wrote:
> Branko Čibej <brane_at_wandisco.com> writes:
>
>> Evgeny, please take a look at r1684940.
>>
>> I don't really like the fact that with this change and 'svnadmin
>> --keep-going --quiet', the text "Error verifying revision N" gets printed
>> to stderr; but I couldn't think of a better way to join the error to the
>> revision it was emitted for. I'd love to find a better solution.
> After giving this change a look, I cannot get rid of the feeling that what we
> are doing here is just adding different workarounds for the API issues, e.g.,
> booleans like b->silent_errors or b->silent_running. I should also say that
> I find the b->feedback_stream manipulations questionable, because now, for
> instance, we write warnings to either stdout or stderr depending on the --quiet
> argument. I always thought that --quiet should only suppress a part of the
> output, but shouldn't really retarget it to a different standard stream.
>
> With that in mind, I am -0 on the corresponding backport proposal.
I removed the backport proposal until we get this sorted out.
> I sketched the other possible option with redesigning svn_repos_verify_fs3()
> API to only report errors via the notify_func(), please see the attached patch.
> Although I won't insist on going this way, I like it better, as it allows us
> to get rid of things like b->silent_errors, b->silent_running, juggling with
> standard streams and API that can yield the same error in two different ways.
>
> What do you think?
IIRC Ivan already proposed something along these lines?
My main objection to this approach is that it breaks all the API
patterns we've ever had: it creates a function that does not return an
error even though it clearly fails, and relies on some notification
callback to report actual errors. This is, IMNSHO, fundamentally broken.
We've relied throughout our code on the fact that it's safe to wrap any
function that returns an svn_error_t* with SVN_ERR() and not have to
worry about missing errors. Your proposal throws this pattern out the
window; if we adopt it, we'll have to start checking API docs to see if
the svn_error_t* actually guarantees that the caller gets an error when
something fails. That's really, really painful.
My minor objection is that the compatibility wrapper suddenly becomes
comples, whereas the usual pattern is just wrapping the new function
with a slightly different set of parameters. This makes maintaining the
compatibility wrappers just that bit more complex. However, I could live
with that if my former objection was resolved.
-- Brane
Received on 2015-06-16 11:24:34 CEST