On 04/14/2014 11:25 PM, Junio C Hamano wrote:
> 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.
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@xxxxxxxxxxxxxxxxxxxxxxxxxxx
--
Michael Haggerty
mhagger@xxxxxxxxxxxx
http://softwareswirl.blogspot.com/
--
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