From 8280bbebd1ecc4633b969a439ed4ca653d1bc958 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Mon, 2 Mar 2015 10:29:51 +0100 Subject: write_ref_sha1(): remove check for lock == NULL None of the callers pass NULL to this function, and there doesn't seem to be any usefulness to allowing them to do so. Signed-off-by: Michael Haggerty Reviewed-by: Stefan Beller Signed-off-by: Junio C Hamano --- refs.c | 4 ---- 1 file changed, 4 deletions(-) (limited to 'refs.c') diff --git a/refs.c b/refs.c index c5fa70947f..d1130e2259 100644 --- a/refs.c +++ b/refs.c @@ -3080,10 +3080,6 @@ static int write_ref_sha1(struct ref_lock *lock, static char term = '\n'; struct object *o; - if (!lock) { - errno = EINVAL; - return -1; - } if (!lock->force_write && !hashcmp(lock->old_sha1, sha1)) { unlock_ref(lock); return 0; -- cgit v1.3 From 706d5f816fbcb1299c19f4d41bea60f1e229165e Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Mon, 2 Mar 2015 10:29:52 +0100 Subject: write_ref_sha1(): move write elision test to callers 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. The last caller, in ref_transaction_commit(), still needs the test. Signed-off-by: Michael Haggerty Reviewed-by: Stefan Beller Signed-off-by: Junio C Hamano --- refs.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) (limited to 'refs.c') diff --git a/refs.c b/refs.c index d1130e2259..651e37e127 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 */ } } -- cgit v1.3 From 074336e5ed4ff10577f22c6812e092e3f6607405 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Mon, 2 Mar 2015 10:29:53 +0100 Subject: lock_ref_sha1_basic(): do not set force_write for missing references If a reference is missing, its SHA-1 will be null_sha1, which can't possibly match a new value that ref_transaction_commit() is trying to update it to. So there is no need to set force_write in this scenario. Signed-off-by: Michael Haggerty Reviewed-by: Stefan Beller Signed-off-by: Junio C Hamano --- refs.c | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-) (limited to 'refs.c') diff --git a/refs.c b/refs.c index 651e37e127..b6a2535fd0 100644 --- a/refs.c +++ b/refs.c @@ -2259,7 +2259,6 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname, int type, lflags; int mustexist = (old_sha1 && !is_null_sha1(old_sha1)); int resolve_flags = 0; - int missing = 0; int attempts_remaining = 3; lock = xcalloc(1, sizeof(struct ref_lock)); @@ -2298,13 +2297,13 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname, orig_refname, strerror(errno)); goto error_return; } - missing = is_null_sha1(lock->old_sha1); - /* When the ref did not exist and we are creating it, - * make sure there is no existing ref that is packed - * whose name begins with our refname, nor a ref whose - * name is a proper prefix of our refname. + /* + * If the ref did not exist and we are creating it, make sure + * there is no existing packed ref whose name begins with our + * refname, nor a packed ref whose name is a proper prefix of + * our refname. */ - if (missing && + if (is_null_sha1(lock->old_sha1) && !is_refname_available(refname, skip, get_packed_refs(&ref_cache))) { last_errno = ENOTDIR; goto error_return; @@ -2320,8 +2319,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 (missing) - lock->force_write = 1; if ((flags & REF_NODEREF) && (type & REF_ISSYMREF)) lock->force_write = 1; -- cgit v1.3 From 5a6f47077b31be45bfadd6cef3b8b1a79ad57de5 Mon Sep 17 00:00:00 2001 From: Stefan Beller Date: Tue, 3 Mar 2015 12:43:14 +0100 Subject: struct ref_lock: delete the force_write member Instead, compute the value when it is needed. Signed-off-by: Stefan Beller Edited-by: Michael Haggerty Signed-off-by: Michael Haggerty Reviewed-by: Stefan Beller Signed-off-by: Junio C Hamano --- refs.c | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) (limited to 'refs.c') diff --git a/refs.c b/refs.c index b6a2535fd0..5d8d57d349 100644 --- a/refs.c +++ b/refs.c @@ -12,7 +12,6 @@ struct ref_lock { struct lock_file *lk; unsigned char old_sha1[20]; int lock_fd; - int force_write; }; /* @@ -2319,8 +2318,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)) { @@ -3788,8 +3785,15 @@ 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)) { + 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, -- cgit v1.3 From 5e6f003ca8aed546d50a4f3fbf1e011186047bc0 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Tue, 3 Mar 2015 12:43:16 +0100 Subject: reflog_expire(): ignore --updateref for symbolic references If we are expiring reflog entries for a symbolic reference, then how should --updateref be handled if the newest reflog entry is expired? Option 1: Update the referred-to reference. (This is what the current code does.) This doesn't make sense, because the referred-to reference has its own reflog, which hasn't been rewritten. Option 2: Update the symbolic reference itself (as in, REF_NODEREF). This would convert the symbolic reference into a non-symbolic reference (e.g., detaching HEAD), which is surely not what a user would expect. Option 3: Error out. This is plausible, but it would make the following usage impossible: git reflog expire ... --updateref --all Option 4: Ignore --updateref for symbolic references. We choose to implement option 4. Note: another problem in this code will be fixed in a moment. Signed-off-by: Michael Haggerty Reviewed-by: Stefan Beller Signed-off-by: Junio C Hamano --- Documentation/git-reflog.txt | 3 ++- refs.c | 15 ++++++++++++--- 2 files changed, 14 insertions(+), 4 deletions(-) (limited to 'refs.c') diff --git a/Documentation/git-reflog.txt b/Documentation/git-reflog.txt index 730106c90a..5e7908e4f7 100644 --- a/Documentation/git-reflog.txt +++ b/Documentation/git-reflog.txt @@ -88,7 +88,8 @@ Options for `expire` --updateref:: Update the reference to the value of the top reflog entry (i.e. - @\{0\}) if the previous top entry was pruned. + @\{0\}) if the previous top entry was pruned. (This + option is ignored for symbolic references.) --rewrite:: If a reflog entry's predecessor is pruned, adjust its "old" diff --git a/refs.c b/refs.c index 5d8d57d349..4684ffe2a6 100644 --- a/refs.c +++ b/refs.c @@ -4029,6 +4029,7 @@ int reflog_expire(const char *refname, const unsigned char *sha1, struct ref_lock *lock; char *log_file; int status = 0; + int type; memset(&cb, 0, sizeof(cb)); cb.flags = flags; @@ -4040,7 +4041,7 @@ int reflog_expire(const char *refname, const unsigned char *sha1, * reference itself, plus we might need to update the * reference if --updateref was specified: */ - lock = lock_ref_sha1_basic(refname, sha1, NULL, 0, NULL); + lock = lock_ref_sha1_basic(refname, sha1, NULL, 0, &type); if (!lock) return error("cannot lock ref '%s'", refname); if (!reflog_exists(refname)) { @@ -4077,10 +4078,18 @@ int reflog_expire(const char *refname, const unsigned char *sha1, (*cleanup_fn)(cb.policy_cb); if (!(flags & EXPIRE_REFLOGS_DRY_RUN)) { + /* + * It doesn't make sense to adjust a reference pointed + * to by a symbolic ref based on expiring entries in + * the symbolic reference's reflog. + */ + int update = (flags & EXPIRE_REFLOGS_UPDATE_REF) && + !(type & REF_ISSYMREF); + if (close_lock_file(&reflog_lock)) { status |= error("couldn't write %s: %s", log_file, strerror(errno)); - } else if ((flags & EXPIRE_REFLOGS_UPDATE_REF) && + } else if (update && (write_in_full(lock->lock_fd, sha1_to_hex(cb.last_kept_sha1), 40) != 40 || write_str_in_full(lock->lock_fd, "\n") != 1 || @@ -4091,7 +4100,7 @@ int reflog_expire(const char *refname, const unsigned char *sha1, } else if (commit_lock_file(&reflog_lock)) { status |= error("unable to commit reflog '%s' (%s)", log_file, strerror(errno)); - } else if ((flags & EXPIRE_REFLOGS_UPDATE_REF) && commit_ref(lock)) { + } else if (update && commit_ref(lock)) { status |= error("couldn't set %s", lock->ref_name); } } -- cgit v1.3 From 423c688b855c328ecda0b6a79c4b1af78d09a10c Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Tue, 3 Mar 2015 12:43:17 +0100 Subject: reflog_expire(): never update a reference to null_sha1 Currently, if --updateref is specified and the very last reflog entry is expired or deleted, the reference's value is set to 0{40}. This is an invalid state of the repository, and breaks, for example, "git fsck" and "git for-each-ref". The only place we use --updateref in our own code is when dropping stash entries. In that code, the very next step is to check if the reflog has been made empty, and if so, delete the "refs/stash" reference entirely. Thus that code path ultimately leaves the repository in a valid state. But we don't want to the repository in an invalid state even temporarily, and we don't want to leave an invalid state if other callers of "git reflog expire|delete --updateref" don't think to do the extra cleanup step. So, if "git reflog expire|delete" leaves no more entries in the reflog, just leave the reference unchanged. Signed-off-by: Michael Haggerty Reviewed-by: Stefan Beller Signed-off-by: Junio C Hamano --- refs.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) (limited to 'refs.c') diff --git a/refs.c b/refs.c index 4684ffe2a6..05a4be0c06 100644 --- a/refs.c +++ b/refs.c @@ -4081,10 +4081,13 @@ int reflog_expire(const char *refname, const unsigned char *sha1, /* * It doesn't make sense to adjust a reference pointed * to by a symbolic ref based on expiring entries in - * the symbolic reference's reflog. + * the symbolic reference's reflog. Nor can we update + * a reference if there are no remaining reflog + * entries. */ int update = (flags & EXPIRE_REFLOGS_UPDATE_REF) && - !(type & REF_ISSYMREF); + !(type & REF_ISSYMREF) && + !is_null_sha1(cb.last_kept_sha1); if (close_lock_file(&reflog_lock)) { status |= error("couldn't write %s: %s", log_file, -- cgit v1.3