Branko Čibej <brane_at_wandisco.com> writes:
>> With that in mind, I am -0 on the corresponding backport proposal.
>
> I removed the backport proposal until we get this sorted out.
Ack.
>> 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?
Right, that's the second known solution for this problem. I believe it was
originally proposed in http://svn.haxx.se/dev/archive-2015-06/0018.shtml, and
I implemented it in the aforementioned patch so that we could make a choice
between two implementations instead of comparing an existing implementation
with a concept.
> 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.
I don't think that this approach actually breaks the SVN_ERR() pattern, as
the operation can still fail itself, say, with SVN_ERR_REPOS_BAD_ARGS or
SVN_ERR_CANCELLED. The difference is in that we start treating errors that
happen, e.g., during revision replay, as the *result* of the operation. In
this design, a successfully completed svn_repos_verify_fs3() call means that
nothing prevented us from executing a set of checks for the revisions in the
[start_rev, end_rev] range and that we shared our findings. A failed verify
call, on the contrary, means that something prevented us from executing this
set of checks and that the reason for that is stated in returned svn_error_t.
The bright side of this approach, as I see it, is that don't put the burden
of guessing the right error to deal with on the caller (like we did with the
b->silent_errors field), as the it now always goes through the notify_func().
Boolean arguments of svn_repos_verify_fs3() such as keep_going, metadata_only
and check_normalization are a hint on where to stop or what to check, i.e.,
they affect the set of performed checks, but don't alter the way of delivering
the result. Furthermore, this approach allows us not to invent an error like
"Verification of 10 out of 200 revisions failed ..." in the API itself — just
because we *need* to return something. We could do that in our UI code
(svnadmin.c), but it's no longer mandatory.
Indeed, there is a drawback of requiring a compatibility wrapper that does
more that a single function call with appropriate arguments, but I see it as
a necessary price for a non-contradicting API.
Does it make sense?
> 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.
Ack.
Regards,
Evgeny Kotkov
Received on 2015-06-16 13:09:15 CEST