git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Michael Haggerty <mhagger@alum.mit.edu>
Date
Apr 15, 2014, 05:41 UTC
Message-ID
<534CC69C.1020503@alum.mit.edu>
In-Reply-To
<xmqqa9bnmwnk.fsf@gitster.dls.corp.google.com>
On 04/14/2014 11:25 PM, Junio C Hamano wrote:
Show 17 quoted lines
> 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.

Show 7 quoted lines
> 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.
Show 23 quoted lines
> 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/
Previous: Junio C HamanoNext: Junio C Hamano
Message 24 of 34 in “Clean up update-refs --stdin and implement ref_transaction”
  1. 00/27 Clean up update-refs --stdin and implement ref_transactionMichael Haggerty, Apr 7, 2014
  2. 01/27 t1400: fix name and expected result of one testMichael Haggerty, Apr 7, 2014
  3. 02/27 t1400: provide more usual input to the commandMichael Haggerty, Apr 7, 2014
  4. 03/27 parse_arg(): really test that argument is properly terminatedMichael Haggerty, Apr 7, 2014
  5. 04/27 t1400: add some more tests involving quoted argumentsMichael Haggerty, Apr 7, 2014
  6. 05/27 refs.h: rename the action_on_err constantsMichael Haggerty, Apr 7, 2014
  7. 06/27 update_refs(): fix constnessMichael Haggerty, Apr 7, 2014
  8. 07/27 update-ref --stdin: read the whole input at onceMichael Haggerty, Apr 7, 2014
  9. 08/27 parse_cmd_verify(): copy old_sha1 instead of evaluating <oldvalue> twiceMichael Haggerty, Apr 7, 2014
  10. 09/27 update-ref.c: extract a new function, parse_refname()Michael Haggerty, Apr 7, 2014
  11. 10/27 update-ref --stdin: improve error messages for invalid valuesMichael Haggerty, Apr 7, 2014
  12. 11/27 update-ref --stdin: make error messages more consistentMichael Haggerty, Apr 7, 2014
  13. 12/27 update-ref --stdin: simplify error messages for missing oldvaluesMichael Haggerty, Apr 7, 2014
  14. 13/27 t1400: test that stdin -z update treats empty <newvalue> as zerosMichael Haggerty, Apr 7, 2014
  15. 14/27 update-ref.c: extract a new function, parse_next_sha1()Michael Haggerty, Apr 7, 2014
  16. 15/27 update-ref --stdin -z: deprecate interpreting the empty string as zerosMichael Haggerty, Apr 7, 2014
  17. 16/27 t1400: test one mistake at a timeMichael Haggerty, Apr 7, 2014
  18. 17/27 update-ref --stdin: improve the error message for unexpected EOFMichael Haggerty, Apr 7, 2014
  19. 18/27 update-ref --stdin: harmonize error messagesMichael Haggerty, Apr 7, 2014
  20. 19/27 refs: add a concept of a reference transactionMichael Haggerty, Apr 7, 2014
  21. Junio C HamanoApr 7, 2014
  22. Michael HaggertyApr 14, 2014
  23. Junio C HamanoApr 14, 2014
  24. Michael HaggertyApr 15, 2014
  25. Junio C HamanoApr 15, 2014
  26. 20/27 update-ref --stdin: reimplement using reference transactionsMichael Haggerty, Apr 7, 2014
  27. 21/27 refs: remove API function update_refs()Michael Haggerty, Apr 7, 2014
  28. 22/27 struct ref_update: rename field "ref_name" to "refname"Michael Haggerty, Apr 7, 2014
  29. 23/27 struct ref_update: store refname as a FLEX_ARRAYMichael Haggerty, Apr 7, 2014
  30. 24/27 ref_transaction_commit(): simplify code using temporary variablesMichael Haggerty, Apr 7, 2014
  31. 25/27 struct ref_update: add a lock fieldMichael Haggerty, Apr 7, 2014
  32. 26/27 struct ref_update: add a type fieldMichael Haggerty, Apr 7, 2014
  33. 27/27 ref_transaction_commit(): work with transaction->updates in placeMichael Haggerty, Apr 7, 2014
  34. Ronnie SahlbergApr 11, 2014

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.