On 11.06.2015 14:33, Evgeny Kotkov wrote:
> Branko ÄŒibej <brane_at_wandisco.com> writes:
>
>>> Should be fixed in r1684325.
>> Meh ... siliness. It's actually r1684344. The correct fix is in
>> svnadmin; always sending notifications is correct from API users'
>> perspective, too.
> Thanks for looking into this.
>
> Frankly, I cannot say that adding a flag like b->silent_errors looks exactly
> right to me. The calling site is now bound to an assumption that the error
> notification is the *same* as the error returned by svn_repos_verify_fs3() —
> otherwise we would be discarding a potentially useful part of the error info.
I'm looking at this from a different perspective; the API should behave
consistently regardless of the --keep-going flag:
* if a notification handler is provided, it will be called for each
error encountered during verification;
* if an error occurs during verification, the API will return an error.
How the notifications are presented to the user is an implementation
detail of 'svnadmin verify' and has nothing to do with the API.
> Maybe this indicates that we could do a better job in designing the API?
> I think that if even we do this sort of silencing in svnadmin.c, potential
> users of svn_repos_verify_fs3() would have to introduce a similar logic and
> this could be confusing for them.
I think it's far more useful that the API is consistent WRT
notifications. How the API user handles notifications, or if she even
provides a notification handler, is up to her.
> Furthermore, I can't get rid of the feeling that --keep-going implementation
> has certain flaws in terms of both the API and the UI design. Another example
> of what's currently wrong would be the behavior of svnadmin verify --keep-going
> --quiet, because right now the output is useless for the end user. Here is a
> quick sample:
>
> svnadmin verify --keep-going -q asf-part
>
> svnadmin: E165011: Verification of 14 out of 108221 revisions failed on
> repository 'C:\asf-part'
>
> We don't output errors with --quiet, and I think that this isn't something an
> administrator wants to see after executing verify for a couple of hours, e.g.,
> for a huge repository. She would have to re-run the same command to get a
> hint of exactly what is wrong, even if the intention was just to suppress the
> progress.
If '--quiet' means 'suppress progress', then the result is exactly as
expected. We could change 'svnadmin verify' to behave differently, e.g.,
only display error notifications in --quiet mode; I agree that would be
better than the current behaviour, but again, this has nothing to do
with the API itself.
> Worth mentioning, the test suite didn't catch this problem or the
> double error reporting bug, and I treat this is an indication that the tests
> probably require more work.
In this particular case it's a feature of our Python tests that the
regex expected results match at least one output line. We could invent a
new expected-result class, but I really don't think it's worth the
effort for catching such a minor issue.
> I didn't yet look into fixing this, but I am thinking that we're now trying to
> close the holes found in the --keep-going implementation through the STATUS
> file — and it doesn't sound quite right to me. Maybe the API was overly bent
> for a particular usage scenario or maybe we just need a fresh look on this, but
> without fixing it we could be shipping a half-baked feature and would have to
> maintain the corresponding API. Fixing this, on the other hand, could mean
> blocking the 1.9.0 release, and I don't like this either.
I think we're at the point where we're trying to polish the UI. It is
rather late in the day for that. The minor tweak required in svnadmin to
emit errors in --quiet mode can be pushed into RC3, but I'd pretty much
stop there; we can always make minor UI changes later on. I wouldn't
change svn_repos_verify_fs3 any more; it's consistent and does what it
advertises.
> A possible (although quite radical) option would be cutting the --keep-going
> feature from 1.9.x and redesigning it in 1.10. I believe that there are some
> possibilities to explore — i.e., maybe, having a separate callback for error
> reporting or introducing svn_repos_notify_func2_t that would be yielding errors
> or would provide the necessary flexibility in another way.
I think the API consumer already has all the flexibility she needs.
Separate callbacks or new notification handler types add complexity, but
not flexibility, IMO.
As a matter of fact, I'd love to actually hear from GUI client
implementers instead of second-guessing their needs.
-- Brane
Received on 2015-06-11 15:29:32 CEST