From: Junio C Hamano <gitster@pobox•com>
To: Michael Haggerty <mhagger@alum•mit.edu>
Cc: "Stefan Beller" <sbeller@google•com>,
"Ronnie Sahlberg" <ronniesahlberg@gmail•com>,
"Jonathan Nieder" <jrnieder@gmail•com>,
"Nguyễn Thái Ngọc Duy" <pclouds@gmail•com>,
git@vger•kernel.org
Subject: Re: [PATCH 2/8] write_ref_sha1(): Move write elision test to callers
Date: Thu, 12 Feb 2015 11:58:40 -0800 [thread overview]
Message-ID: <xmqq386asxzj.fsf@gitster.dls.corp.google.com> (raw)
In-Reply-To: <1423473164-6011-3-git-send-email-mhagger@alum.mit.edu> (Michael Haggerty's message of "Mon, 9 Feb 2015 10:12:38 +0100")
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 */
> }
> }
next prev parent reply other threads:[~2015-02-12 19:58 UTC|newest]
Thread overview: 46+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-02-09 9:12 [PATCH 0/8] Fix some problems with reflog expiration Michael Haggerty
2015-02-09 9:12 ` [PATCH 1/8] write_ref_sha1(): remove check for lock == NULL Michael Haggerty
2015-02-10 22:52 ` Stefan Beller
2015-02-11 0:06 ` Jeff King
2015-02-09 9:12 ` [PATCH 2/8] write_ref_sha1(): Move write elision test to callers Michael Haggerty
2015-02-12 19:58 ` Junio C Hamano [this message]
2015-02-09 9:12 ` [PATCH 3/8] lock_ref_sha1_basic(): do not set force_write for missing references Michael Haggerty
2015-02-10 23:24 ` Stefan Beller
2015-02-11 0:05 ` Jeff King
2015-02-11 0:07 ` Stefan Beller
2015-02-12 12:09 ` Michael Haggerty
2015-02-09 9:12 ` [PATCH 4/8] reflog: fix documentation Michael Haggerty
2015-02-10 23:25 ` Stefan Beller
2015-02-09 9:12 ` [PATCH 5/8] reflog: rearrange the manpage Michael Haggerty
2015-02-10 23:42 ` Stefan Beller
2015-02-12 15:17 ` Michael Haggerty
2015-02-12 20:09 ` Junio C Hamano
2015-02-09 9:12 ` [PATCH 6/8] reflog_expire(): ignore --updateref for symbolic references Michael Haggerty
2015-02-11 0:44 ` Stefan Beller
2015-02-12 16:08 ` Michael Haggerty
2015-02-12 17:04 ` Stefan Beller
2015-02-12 20:16 ` Junio C Hamano
2015-02-12 21:54 ` Jeff King
2015-02-13 14:34 ` Michael Haggerty
2015-02-09 9:12 ` [PATCH 7/8] reflog_expire(): never update a reference to null_sha1 Michael Haggerty
2015-02-09 20:55 ` Eric Sunshine
2015-02-12 11:51 ` Michael Haggerty
2015-02-09 9:12 ` [PATCH 8/8] reflog_expire(): lock symbolic refs themselves, not their referent Michael Haggerty
2015-02-11 0:49 ` Stefan Beller
2015-02-11 22:49 ` Junio C Hamano
2015-02-11 23:25 ` Stefan Beller
2015-02-12 16:52 ` Michael Haggerty
2015-02-12 18:04 ` Stefan Beller
2015-02-13 16:26 ` Michael Haggerty
2015-02-13 17:16 ` Stefan Beller
2015-02-13 18:05 ` Junio C Hamano
2015-02-13 18:21 ` Stefan Beller
2015-02-13 18:26 ` Junio C Hamano
2015-02-13 18:32 ` Stefan Beller
2015-02-13 19:12 ` Junio C Hamano
2015-02-13 20:11 ` Michael Haggerty
2015-02-13 21:53 ` Junio C Hamano
2015-02-14 5:58 ` Michael Haggerty
2015-02-09 18:57 ` [PATCH 0/8] Fix some problems with reflog expiration Stefan Beller
2015-02-10 23:12 ` [PATCH] refs.c: get rid of force_write flag Stefan Beller
2015-02-12 15:35 ` Michael Haggerty
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=xmqq386asxzj.fsf@gitster.dls.corp.google.com \
--to=gitster@pobox$(echo .)com \
--cc=git@vger$(echo .)kernel.org \
--cc=jrnieder@gmail$(echo .)com \
--cc=mhagger@alum$(echo .)mit.edu \
--cc=pclouds@gmail$(echo .)com \
--cc=ronniesahlberg@gmail$(echo .)com \
--cc=sbeller@google$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox