refs/files: remove "name exist?" check in lock_ref_oid_basic()

In lock_ref_oid_basic() we'll happily lock a reference that doesn't exist yet. That's normal, and is how references are initially born, but we don't need to retain checks here in lock_ref_oid_basic() about the state of the ref, when what we're checking is either checked already, or something we're about to discover by trying to lock the ref with raceproof_create_file(). The one exception is the caller in files_reflog_expire(), who passes us a "type" to find out if the reference is a symref or not. We can move the that logic over to that caller, which can now defer its discovery of whether or not the ref is a symref until it's needed. In the preceding commit an exhaustive regression test was added for that case in a new test in "t1417-reflog-updateref.sh". The improved diagnostics here were added in 5b2d8d6f218 (lock_ref_sha1_basic(): improve diagnostics for ref D/F conflicts, 2015-05-11), and then much of the surrounding code went away recently in my 245fbba46d6 (refs/files: remove unused "errno == EISDIR" code, 2021-08-23). The refs_resolve_ref_unsafe() code being removed here looks like it should be tasked with doing that, but it's actually redundant to other code. The reason for that is as noted in 245fbba46d6 this once widely used function now only has a handful of callers left, which all handle this case themselves. To the extent that we're racy between their check and ours removing this check actually improves the situation, as we'll be doing fewer things between the not-under-lock initial check and acquiring the lock. Why this is OK for all the remaining callers of lock_ref_oid_basic() is noted below. There are only two of those callers: * "git branch -[cm] <oldbranch> <newbranch>": In files_copy_or_rename_ref() we'll call this when we copy or rename refs via rename_ref() and copy_ref(). but only after we've checked if the refname exists already via its own call to refs_resolve_ref_unsafe() and refs_rename_ref_available(). As the updated comment to the latter here notes neither of those are actually needed. If we delete not only this code but also refs_rename_ref_available() we'll do just fine, we'll just emit a less friendly error message if e.g. "git branch -m A B/C" would have a D/F conflict with a "B" file. Actually we'd probably die before that in case reflogs for the branch existed, i.e. when the try to rename() or copy_file() the relevant reflog, since if we've got a D/F conflict with a branch name we'll probably also have the same with its reflogs (but not necessarily, we might have reflogs, but it might not). As some #leftoverbits that code seems buggy to me, i.e. the reflog "protocol" should be to get a lock on the main ref, and then perform ref and/or reflog operations. That code dates back to c976d415e53 (git-branch: add options and tests for branch renaming, 2006-11-28) and probably pre-dated the solidifying of that convention. But in any case, that edge case is not our bug or problem right now. * "git reflog expire <ref>": In files_reflog_expire() we'll call this without previous ref existence checking in files-backend.c, but that code is in turn called by code that's just finished checking if the refname whose reflog we're expiring exists. See ae35e16cd43 (reflog expire: don't lock reflogs using previously seen OID, 2021-08-23) for the current state of that code, and 5e6f003ca8a (reflog_expire(): ignore --updateref for symbolic references, 2015-03-03) for the code we'd break if we only did a "update = !!ref" here, which is covered by the aforementioned regression test in "t1417-reflog-updateref.sh". Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Ævar Arnfjörð Bjarmason committed Oct 16, 2021 at 11:39 UTC 52106430dc80ea20ec2e00a6079a7bc114d36b70
1 file changed +24 -24
refs/files-backend.c
+24 -24
@@ -1001,7 +1001,7 @@ static int create_reflock(const char *path, void *cb)
1001 * Locks a ref returning the lock on success and NULL on failure.
1002 */
1003 static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs,
1004 - const char *refname, int *type,
1004 + const char *refname,
1005 struct strbuf *err)
1006 {
1007 struct strbuf ref_file = STRBUF_INIT;
@@ -1013,16 +1013,6 @@ static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs,
1013 CALLOC_ARRAY(lock, 1);
1014
1015 files_ref_path(refs, &ref_file, refname);
1016 - if (!refs_resolve_ref_unsafe(&refs->base, refname,
1017 - RESOLVE_REF_NO_RECURSE,
1018 - &lock->old_oid, type)) {
1019 - if (!refs_verify_refname_available(&refs->base, refname,
1020 - NULL, NULL, err))
1021 - strbuf_addf(err, "unable to resolve reference '%s': %s",
1022 - refname, strerror(errno));
1023 -
1024 - goto error_return;
1025 - }
1016
1017 /*
1018 * If the ref did not exist and we are creating it, make sure
@@ -1364,14 +1354,14 @@ static int commit_ref_update(struct files_ref_store *refs,
1354 struct strbuf *err);
1355
1356 /*
1367 - * Check whether an attempt to rename old_refname to new_refname would
1368 - * cause a D/F conflict with any existing reference (other than
1369 - * possibly old_refname). If there would be a conflict, emit an error
1357 + * Emit a better error message than lockfile.c's
1358 + * unable_to_lock_message() would in case there is a D/F conflict with
1359 + * another existing reference. If there would be a conflict, emit an error
1360 * message and return false; otherwise, return true.
1361 *
1362 * Note that this function is not safe against all races with other
1373 - * processes (though rename_ref() catches some races that might get by
1374 - * this check).
1363 + * processes, and that's not its job. We'll emit a more verbose error on D/f
1364 + * conflicts if we get past it into lock_ref_oid_basic().
1365 */
1366 static int refs_rename_ref_available(struct ref_store *refs,
1367 const char *old_refname,
@@ -1492,7 +1482,7 @@ static int files_copy_or_rename_ref(struct ref_store *ref_store,
1482
1483 logmoved = log;
1484
1495 - lock = lock_ref_oid_basic(refs, newrefname, NULL, &err);
1485 + lock = lock_ref_oid_basic(refs, newrefname, &err);
1486 if (!lock) {
1487 if (copy)
1488 error("unable to copy '%s' to '%s': %s", oldrefname, newrefname, err.buf);
@@ -1514,7 +1504,7 @@ static int files_copy_or_rename_ref(struct ref_store *ref_store,
1504 goto out;
1505
1506 rollback:
1517 - lock = lock_ref_oid_basic(refs, oldrefname, NULL, &err);
1507 + lock = lock_ref_oid_basic(refs, oldrefname, &err);
1508 if (!lock) {
1509 error("unable to lock %s for rollback: %s", oldrefname, err.buf);
1510 strbuf_release(&err);
@@ -1921,7 +1911,7 @@ static int files_create_symref(struct ref_store *ref_store,
1911 struct ref_lock *lock;
1912 int ret;
1913
1924 - lock = lock_ref_oid_basic(refs, refname, NULL, &err);
1914 + lock = lock_ref_oid_basic(refs, refname, &err);
1915 if (!lock) {
1916 error("%s", err.buf);
1917 strbuf_release(&err);
@@ -3125,7 +3115,6 @@ static int files_reflog_expire(struct ref_store *ref_store,
3115 struct strbuf log_file_sb = STRBUF_INIT;
3116 char *log_file;
3117 int status = 0;
3128 - int type;
3118 struct strbuf err = STRBUF_INIT;
3119 const struct object_id *oid;
3120
@@ -3139,7 +3128,7 @@ static int files_reflog_expire(struct ref_store *ref_store,
3128 * reference itself, plus we might need to update the
3129 * reference if --updateref was specified:
3130 */
3142 - lock = lock_ref_oid_basic(refs, refname, &type, &err);
3131 + lock = lock_ref_oid_basic(refs, refname, &err);
3132 if (!lock) {
3133 error("cannot lock ref '%s': %s", refname, err.buf);
3134 strbuf_release(&err);
@@ -3201,9 +3190,20 @@ static int files_reflog_expire(struct ref_store *ref_store,
3190 * a reference if there are no remaining reflog
3191 * entries.
3192 */
3204 - int update = (flags & EXPIRE_REFLOGS_UPDATE_REF) &&
3205 - !(type & REF_ISSYMREF) &&
3206 - !is_null_oid(&cb.last_kept_oid);
3193 + int update = 0;
3194 +
3195 + if ((flags & EXPIRE_REFLOGS_UPDATE_REF) &&
3196 + !is_null_oid(&cb.last_kept_oid)) {
3197 + int ignore_errno;
3198 + int type;
3199 + const char *ref;
3200 +
3201 + ref = refs_werrres_ref_unsafe(&refs->base, refname,
3202 + RESOLVE_REF_NO_RECURSE,
3203 + NULL, &type,
3204 + &ignore_errno);
3205 + update = !!(ref && !(type & REF_ISSYMREF));
3206 + }
3207
3208 if (close_lock_file_gently(&reflog_lock)) {
3209 status |= error("couldn't write %s: %s", log_file,