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

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 2, 2015, 21:44 UTC
Message-ID: <xmqqk2yz9ir9.fsf@gitster.dls.corp.google.com>
In-Reply-To: <1425288597-20547-5-git-send-email-mhagger@alum.mit.edu>

Michael Haggerty <mhagger@alum.mit.edu> writes:

> Instead, compute the value when it is needed.

Show 25 quoted lines

> @@ -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. The before and after says the same thing, i.e. the code used to be:

 - We say "do the write-out without questioning" when we are
   updating a symbolic ref without dereferencing.

 - Do nothing and unlock if we are not told to "do the write-out
   without questioning" and the update will be a no-op anyway.

while the code after the change says:

 + Do nothing and unlock if we are not handling "update a symbolic
   ref without dereferencing" and the update will be a no-op anyway.

Perhaps the former has the same effect as "avoid a single complex sentence and use two short sentences instead".

The negation in the condition does not help, either.

 * If we are updating a symbolic ref without dereferencing, or if we
   are updating with a different object name, we definitely have to
   write.

would be easier to understand, perhaps?  I.e.

	if (hashcmp(update->lock->old_sha1, update->lock->new_sha1) ||
	    ((update->type & REF_ISSYMREF) && (update->flags & REF_NO_DEREF))) {
		/* do the write-out thing */
	} else {
		/* the request to update from the same to the same is a no-op */
		unlock_ref(update->lock);
                update->lock = NULL;
	}

I dunno.