Michael Haggerty <mhagger@xxxxxxxxxxxx> writes:
> I forgot to confirm that the callers *do* verify that they don't pass
> incorrect values to ref_transaction_create() and
> ref_transaction_delete(). But if they wouldn't, then die("BUG:") would
> not be appropriate either, would it? It would have to be die("you silly
> user...").
Having the assert() there gives a confused message to the readers
(at least it did to this reader).
assert() implies that callers that are not buggy should not give
input that triggers the condition, which would mean it is the
callers' responsibility to sanity check the user-input to reject
creating a ref at 0{40} or deleting a ref whose old value is 0{40},
which in turn means these input are errors that need to be diagnosed
and documented.
But as you said below...
> ... even if the preconditions are not met, nothing really crazy
> happens.
I agree that it also is perfectly fine to treat such input as
not-an-input-error.
It is a signal that these checks are not 'if (...) die()' that the
code may take that stance.
I cannot tell which interpretation the code is trying to implement.
Any one of the following I can understand:
(1) drop the assert(), declaring that the user input is perfectly
fine;
(2) keep the assert(), reject such user input at the callers, and
document that these are invalid inputs;
(3) replace the assert() with 'if (...) die("you silly user...")',
and document that these are invalid inputs; or
(4) same as (3) but replace with warn(), declaring such input as
suspicious.
but the current assert() makes the code look "cannot decide" ;-).
I would consider the first two more sensible than the other two, and
am leaning slightly towards (1) over (2).
Thanks.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html