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

[Michael Haggerty <mhagger@alum.mit.edu>]

On 04/14/2014 11:25 PM, Junio C Hamano wrote:
> Michael Haggerty <mhagger@alum.mit.edu> 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.

In v2 of this patch series, I didn't forbid calling create with new_sha
== 0{40}, because, even though it's not what the user would think of as
creating a reference, the code didn't care--it would just verify that
the reference didn't exist before and then leave it undefined.

Then in [1] you commented:

> Sounds a bit crazy that you can ask "create", which verifies the
> absense of the thing, to delete a thing.

I reacted to your comment by changing the documentation to forbid
calling "create" with new_sha1 == 0{40}, and enforced the rule using an
assert().  At the same time I added an analogous restriction that if
"delete" is called with have_old set, then old_sha1 must not be 0{40}.

In retrospect, you might have been objecting more to the misleading
docstring than to the behavior as implemented at the time.  The
docstring implied that create could actually be used to delete a
reference, but that was not true: it always checked that the reference
didn't exist beforehand.  So at worst it could leave a non-existent
reference un-created.  Sorry for the confusion.

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

That was the case in v2.

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

The current status in v3 is that (2) is implemented.

Obviously I don't feel strongly about it, given that I implemented (1)
in v2.  But I think that this restriction makes code at the calling site
easier to understand: "create" now always means "create"; i.e., if the
transaction goes through, then the reference exists afterwards.  And
"delete" always means delete; i.e., afterwards, there is one less
reference in the world.

Michael

[1] http://mid.gmane.org/xmqqtxaczvod.fsf@gitster.dls.corp.google.com

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/