Re: [PATCH 2/8] write_ref_sha1(): Move write elision test to callers

[Junio C Hamano <gitster@pobox.com>]

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

> write_ref_sha1() previously skipped the write if the reference already
> had the desired value, unless lock->force_write was set. Instead,
> perform that test at the callers.
>
> Two of the callers (in rename_ref()) unconditionally set force_write
> just before calling write_ref_sha1(), so they don't need the extra
> check at all. Nor do they need to set force_write anymore.

Good.  I recall that this convoluted "the callee checks if the
values are the same so we need to tell it not to do that" logic
looked very disturbing.



> The last caller, in ref_transaction_commit(), still needs the test.
>
> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
> ---
>  refs.c | 18 +++++++++---------
>  1 file changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/refs.c b/refs.c
> index d1130e2..651e37e 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -2878,7 +2878,6 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logms
>  		error("unable to lock %s for update", newrefname);
>  		goto rollback;
>  	}
> -	lock->force_write = 1;
>  	hashcpy(lock->old_sha1, orig_sha1);
>  	if (write_ref_sha1(lock, orig_sha1, logmsg)) {
>  		error("unable to write current sha1 into %s", newrefname);
> @@ -2894,7 +2893,6 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logms
>  		goto rollbacklog;
>  	}
>  
> -	lock->force_write = 1;
>  	flag = log_all_ref_updates;
>  	log_all_ref_updates = 0;
>  	if (write_ref_sha1(lock, orig_sha1, NULL))
> @@ -3080,10 +3078,6 @@ static int write_ref_sha1(struct ref_lock *lock,
>  	static char term = '\n';
>  	struct object *o;
>  
> -	if (!lock->force_write && !hashcmp(lock->old_sha1, sha1)) {
> -		unlock_ref(lock);
> -		return 0;
> -	}
>  	o = parse_object(sha1);
>  	if (!o) {
>  		error("Trying to write ref %s with nonexistent object %s",
> @@ -3797,15 +3791,21 @@ int ref_transaction_commit(struct ref_transaction *transaction,
>  		struct ref_update *update = updates[i];
>  
>  		if (!is_null_sha1(update->new_sha1)) {
> -			if (write_ref_sha1(update->lock, update->new_sha1,
> -					   update->msg)) {
> +			if (!update->lock->force_write &&
> +			    !hashcmp(update->lock->old_sha1, update->new_sha1)) {
> +				unlock_ref(update->lock);
> +				update->lock = NULL;
> +			} else if (write_ref_sha1(update->lock, update->new_sha1,
> +						  update->msg)) {
>  				update->lock = NULL; /* freed by write_ref_sha1 */
>  				strbuf_addf(err, "Cannot update the ref '%s'.",
>  					    update->refname);
>  				ret = TRANSACTION_GENERIC_ERROR;
>  				goto cleanup;
> +			} else {
> +				/* freed by write_ref_sha1(): */
> +				update->lock = NULL;
>  			}
> -			update->lock = NULL; /* freed by write_ref_sha1 */
>  		}
>  	}