Re: [PATCH v2 4/7] struct ref_lock: delete the force_write member

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

On 03/02/2015 10:44 PM, Junio C Hamano wrote:
> Michael Haggerty <mhagger@alum.mit.edu> writes:
> 
>> Instead, compute the value when it is needed.
> 
>> @@ -2318,8 +2317,6 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,
>>  	lock->ref_name = xstrdup(refname);
>>  	lock->orig_ref_name = xstrdup(orig_refname);
>>  	ref_file = git_path("%s", refname);
>> -	if ((flags & REF_NODEREF) && (type & REF_ISSYMREF))
>> -		lock->force_write = 1;
>>  
>>   retry:
>>  	switch (safe_create_leading_directories(ref_file)) {
>> @@ -3787,8 +3784,13 @@ int ref_transaction_commit(struct ref_transaction *transaction,
>>  		struct ref_update *update = updates[i];
>>  
>>  		if (!is_null_sha1(update->new_sha1)) {
>> -			if (!update->lock->force_write &&
>> -			    !hashcmp(update->lock->old_sha1, update->new_sha1)) {
>> +			if (!((update->type & REF_ISSYMREF)
>> +			      && (update->flags & REF_NODEREF))
>> +			    && !hashcmp(update->lock->old_sha1, update->new_sha1)) {
>> +				/*
>> +				 * The reference already has the desired
>> +				 * value, so we don't need to write it.
>> +				 */
>>  				unlock_ref(update->lock);
>>  				update->lock = NULL;
>>  			} else if (write_ref_sha1(update->lock, update->new_sha1,
> 
> The code before and after the change are equivalent.
> 
> It shouldn't be the case, but somehow I find the original slightly
> easier to understand. [...]

I had the same feeling; thanks for confirming it. How about I introduce
a temporary variable `overwriting_symref` as an aid to the reader? I
think this makes it pretty clear:

>  		if (!is_null_sha1(update->new_sha1)) {
> -			if (!update->lock->force_write &&
> -			    !hashcmp(update->lock->old_sha1, update->new_sha1)) {
> +			int overwriting_symref = ((update->type & REF_ISSYMREF) &&
> +						  (update->flags & REF_NODEREF));
> +
> +			if (!overwriting_symref
> +			    && !hashcmp(update->lock->old_sha1, update->new_sha1)) {
> +				/*
> +				 * The reference already has the desired
> +				 * value, so we don't need to write it.
> +				 */
>  				unlock_ref(update->lock);
>  				update->lock = NULL;
>  			} else if (write_ref_sha1(update->lock, update->new_sha1,

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu