Anton Khirnov <an...@khirnov.net> writes:
>>
>> I think it would be better to gradually migrate existing code to the new
>> function, since this code is eminently inlinable. That probably means
>> deprecating notmuch_messges_valid
>
> Is breaking existing user code really worth the small size/performance
> benefit of inlining?
It's not about performance, but about reducing the size of the API. By
deprecating (and eventually removing) the old functions, we help
people make the right choices when writing new code.
>> > + * Note that an iterator may become invalid either due to getting
>> > exhausted or
>> > + * due to a runtime error. Use notmuch_messages_status to distinguish
>> > + * between those cases.
>> > */
>>
>> I guess we need to decide if we really expect people to use both
>> functions in new code.
>
> I'd prefer to avoid breaking user code, as I don't expect a lot of it
> will actually implement query re-execution, and having to change
> existing callers for no functionality benefit feels user-hostile.
There's a different between documenting best practices and breaking
existing code. At least for now I'm suggesting the former.
>> > + if (message == NULL) {
>> > + if (status == NOTMUCH_PRIVATE_STATUS_NO_DOCUMENT_FOUND) {
>> > + INTERNAL_ERROR ("a messages iterator contains a non-existent
>> > document ID.\n");
>>
>> As you know (because you sent this patch series!) we really want to
>> avoid the library aborting. Are we confident that this cannot arise from
>> concurrent access (I guess via Xapian snapshotting mechanisms?). Perhaps
>> a brief comment about _why_ this should never happen would be a good idea.
>
> Well, I'm just keeping existing behaviour here. AFAIU it would mean that
> the database is inconsistent, which could happen e.g. through a bug in
> notmuch (or Xapian), or good old database corruption. I guess the code
> could be changed to signal an error status, but that's not really in
> scope of this patchset.
>>
OK, fair enough. I guess the use of "INTERNAL_ERROR" should be enough to
suggest this should never happen. Unfortunately in the past that was not
always the case.
>> > }
>> > + status = notmuch_messages_status(messages_ro);
>> >
>> > - printf("SUCCESS\n");
>> > + if (status == NOTMUCH_STATUS_ITERATOR_EXHAUSTED)
>> > + printf("SUCCESS\n");
>> > + else
>> > + printf("status: %u\n", status);
>>
>> It might be my mistake in rebasing, but this test fails for me with
>> "status: 26". Assuming you update notmuch_status_to_string, that can be
>> made less cryptic, and I'll double check
>
> I also need to change the test to use notmuch_status_to_string(), after
> which it prints
> status: Operation invalidated due to concurrent database modification
>
>> > +test_subtest_known_broken
>>
>> It should not in fact be known broken after this commit, right?
>
> I'm reluctant to mark it as fixed (harcoding
> NOTMUCH_STATUS_OPERATION_INVALIDATED as the correct behaviour), because
> future Xapian changes might make iteration succeed, which is what the
> caller actually wants to happen (and thus the properly correct result
> of this test).
>
> What I could do is re-execute the query on
> NOTMUCH_STATUS_OPERATION_INVALIDATED, and see if the second try exhausts
> the iterator. What do you think?
I think deterministic for the medium term is the best we can hope for.
The notmuch test suite has the (perhaps undocumented) convention that
that known_broken tests should correspond to bugs, so I'd prefer that
the test pass if the code is working as expected.
>From a practical point of view, whether a Xapian change (or some kind of
non-determinism) makes a test go from BROKEN->FIXED is really about the
same amount of hassle as going from PASS->FAIL, both result in broken
(non-zero exit code) builds.
_______________________________________________
notmuch mailing list -- notmuch@notmuchmail.org
To unsubscribe send an email to notmuch-le...@notmuchmail.org
