Re: [PATCH 2/8] write_ref_sha1(): Move write elision test to callers
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Feb 12, 2015, 19:58 UTC
- Message-ID
- <xmqq386asxzj.fsf@gitster.dls.corp.google.com>
- In-Reply-To
- <1423473164-6011-3-git-send-email-mhagger@alum.mit.edu>
Michael Haggerty <mhagger@alum.mit.edu> writes:
Show 7 quoted lines
> 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.
Show 62 quoted lines
> 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 */
> }
> }