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 14, 2014, 10:54 UTC
Message-ID
<534BBE67.3040100@alum.mit.edu>
In-Reply-To
<xmqq8urhlzr1.fsf@gitster.dls.corp.google.com>
On 04/07/2014 09:13 PM, Junio C Hamano wrote:
Show 37 quoted lines
> Michael Haggerty <mhagger@alum.mit.edu> writes:
> 
>> +void ref_transaction_create(struct ref_transaction *transaction,
>> +			    const char *refname,
>> +			    unsigned char *new_sha1,
>> +			    int flags)
>> +{
>> +	struct ref_update *update = add_update(transaction, refname);
>> +
>> +	assert(!is_null_sha1(new_sha1));
>> +	hashcpy(update->new_sha1, new_sha1);
>> +	hashclr(update->old_sha1);
>> +	update->flags = flags;
>> +	update->have_old = 1;
>> +}
>> +
>> +void ref_transaction_delete(struct ref_transaction *transaction,
>> +			    const char *refname,
>> +			    unsigned char *old_sha1,
>> +			    int flags, int have_old)
>> +{
>> +	struct ref_update *update = add_update(transaction, refname);
>> +
>> +	update->flags = flags;
>> +	update->have_old = have_old;
>> +	if (have_old) {
>> +		assert(!is_null_sha1(old_sha1));
>> +		hashcpy(update->old_sha1, old_sha1);
>> +	}
>> +}
> 
> These assert()s will often turn into no-op in production builds.  If
> it is a bug in the code (i.e. the callers are responsible for
> catching these conditions and issuing errors, and there are actually
> such checks implemented in the callers), it is fine to have these as
> assert()s, but otherwise these should be 'if (...) die("BUG:")', I
> think.

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

Another reason I'm comfortable with only having assert()s in this case is that even if the preconditions are not met, nothing really crazy happens. If I were guarding against something nastier, like a buffer overflow, then I would more likely have used die("BUG:") instead.

It is not material to this discussion, but I wonder how often production builds use NDEBUG. I just checked that Debian does not; i.e., assertions are enabled for git there. Personally I wouldn't run code built with NDEBUG unless building for a severely resource-constrained device, and even then I'd be pretty nervous about it.

Michael
-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/
Previous: Junio C HamanoNext: Junio C Hamano
Message 22 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.