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

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

On 04/07/2014 09:13 PM, Junio C Hamano wrote:
> 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/