Re: [PATCH v3 19/27] refs: add a concept of a reference transaction

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 14, 2014, 21:25 UTC
Message-ID: <xmqqa9bnmwnk.fsf@gitster.dls.corp.google.com>
In-Reply-To: <534BBE67.3040100@alum.mit.edu>

Michael Haggerty <mhagger@alum.mit.edu> writes:

Show 5 quoted lines

> 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.