From 881aebffcffb32ffc99c681d9a1f8fb50e2cd9cb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Tue, 20 Jul 2021 12:24:06 +0200 Subject: refs/packet: add missing BUG() invocations to reflog callbacks MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In e0cc8ac8202 (packed_ref_store: make class into a subclass of `ref_store`, 2017-06-23) a die() was added to packed_create_reflog(), but not to any of the other reflog callbacks, let's do that. Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Junio C Hamano --- refs/packed-backend.c | 5 +++++ 1 file changed, 5 insertions(+) (limited to 'refs') diff --git a/refs/packed-backend.c b/refs/packed-backend.c index f8aa97d799..24a360b719 100644 --- a/refs/packed-backend.c +++ b/refs/packed-backend.c @@ -1600,6 +1600,7 @@ static int packed_for_each_reflog_ent(struct ref_store *ref_store, const char *refname, each_reflog_ent_fn fn, void *cb_data) { + BUG("packed reference store does not support reflogs"); return 0; } @@ -1608,12 +1609,14 @@ static int packed_for_each_reflog_ent_reverse(struct ref_store *ref_store, each_reflog_ent_fn fn, void *cb_data) { + BUG("packed reference store does not support reflogs"); return 0; } static int packed_reflog_exists(struct ref_store *ref_store, const char *refname) { + BUG("packed reference store does not support reflogs"); return 0; } @@ -1627,6 +1630,7 @@ static int packed_create_reflog(struct ref_store *ref_store, static int packed_delete_reflog(struct ref_store *ref_store, const char *refname) { + BUG("packed reference store does not support reflogs"); return 0; } @@ -1638,6 +1642,7 @@ static int packed_reflog_expire(struct ref_store *ref_store, reflog_expiry_cleanup_fn cleanup_fn, void *policy_cb_data) { + BUG("packed reference store does not support reflogs"); return 0; } -- cgit v1.2.3 From 212631ed50a32a8f8fe15d4b52684b91affce773 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Tue, 20 Jul 2021 12:24:07 +0200 Subject: refs/files: remove unused REF_DELETING in lock_ref_oid_basic() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The lock_ref_oid_basic() function has gradually been replaced by most callers no longer performing a low-level "acquire lock, update and release", and instead using the ref transaction API. So there are only 4 remaining callers of lock_ref_oid_basic(). None of those callers pass REF_DELETING anymore, the last caller went away in 92b1551b1d (refs: resolve symbolic refs first, 2016-04-25). Before that we'd refactored and moved this code in: - 8df4e511387 (struct ref_update: move "have_old" into "flags", 2015-02-17) - 7bd9bcf372d (refs: split filesystem-based refs code into a new file, 2015-11-09) - 165056b2fc (lock_ref_for_update(): new function, 2016-04-24) We then finally stopped using it in 92b1551b1d (noted above). So let's remove the handling of this parameter. By itself this change doesn't benefit us much, but it's the start of even more removal of unused code in and around this function in subsequent commits. Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Junio C Hamano --- refs/files-backend.c | 2 -- 1 file changed, 2 deletions(-) (limited to 'refs') diff --git a/refs/files-backend.c b/refs/files-backend.c index 677b7e4cdd..326f022421 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -934,8 +934,6 @@ static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs, if (mustexist) resolve_flags |= RESOLVE_REF_READING; - if (flags & REF_DELETING) - resolve_flags |= RESOLVE_REF_ALLOW_BAD_NAME; files_ref_path(refs, &ref_file, refname); resolved = !!refs_resolve_ref_unsafe(&refs->base, -- cgit v1.2.3 From 491ad946b29d8938d12aabe0317a0db73dbda2f2 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Mon, 23 Aug 2021 13:36:04 +0200 Subject: refs: drop unused "flags" parameter to lock_ref_oid_basic() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In the last commit we removed the REF_DELETING flag from lock_ref_oid_basic(). Since then all of the remaining callers do pass REF_NO_DEREF, but that has been ignored completely since 7a418f3a17 (lock_ref_sha1_basic(): only handle REF_NODEREF mode, 2016-04-22). So we can simply get rid of the parameter entirely. Signed-off-by: Jeff King Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Junio C Hamano --- refs/files-backend.c | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) (limited to 'refs') diff --git a/refs/files-backend.c b/refs/files-backend.c index 326f022421..e73458e257 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -917,8 +917,7 @@ static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs, const struct object_id *old_oid, const struct string_list *extras, const struct string_list *skip, - unsigned int flags, int *type, - struct strbuf *err) + int *type, struct strbuf *err) { struct strbuf ref_file = STRBUF_INIT; struct ref_lock *lock; @@ -1413,8 +1412,9 @@ static int files_copy_or_rename_ref(struct ref_store *ref_store, logmoved = log; + lock = lock_ref_oid_basic(refs, newrefname, NULL, NULL, NULL, - REF_NO_DEREF, NULL, &err); + NULL, &err); if (!lock) { if (copy) error("unable to copy '%s' to '%s': %s", oldrefname, newrefname, err.buf); @@ -1437,7 +1437,7 @@ static int files_copy_or_rename_ref(struct ref_store *ref_store, rollback: lock = lock_ref_oid_basic(refs, oldrefname, NULL, NULL, NULL, - REF_NO_DEREF, NULL, &err); + NULL, &err); if (!lock) { error("unable to lock %s for rollback: %s", oldrefname, err.buf); strbuf_release(&err); @@ -1845,7 +1845,7 @@ static int files_create_symref(struct ref_store *ref_store, int ret; lock = lock_ref_oid_basic(refs, refname, NULL, - NULL, NULL, REF_NO_DEREF, NULL, + NULL, NULL, NULL, &err); if (!lock) { error("%s", err.buf); @@ -3064,8 +3064,7 @@ static int files_reflog_expire(struct ref_store *ref_store, * reference if --updateref was specified: */ lock = lock_ref_oid_basic(refs, refname, oid, - NULL, NULL, REF_NO_DEREF, - &type, &err); + NULL, NULL, &type, &err); if (!lock) { error("cannot lock ref '%s': %s", refname, err.buf); strbuf_release(&err); -- cgit v1.2.3 From 11e984da076eeb2fea46f23f573c18bc197edb34 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Mon, 23 Aug 2021 13:36:05 +0200 Subject: refs/files: remove unused "extras/skip" in lock_ref_oid_basic() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The lock_ref_oid_basic() function has gradually been replaced by use of the file transaction API, there are only 4 remaining callers of it. None of those callers pass non-NULL "extras" and "skip" parameters, the last such caller went away in 92b1551b1d4 (refs: resolve symbolic refs first, 2016-04-25), so let's remove the parameters. Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Junio C Hamano --- refs/files-backend.c | 22 +++++++--------------- 1 file changed, 7 insertions(+), 15 deletions(-) (limited to 'refs') diff --git a/refs/files-backend.c b/refs/files-backend.c index e73458e257..69f7f54e03 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -915,8 +915,6 @@ static int create_reflock(const char *path, void *cb) static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs, const char *refname, const struct object_id *old_oid, - const struct string_list *extras, - const struct string_list *skip, int *type, struct strbuf *err) { struct strbuf ref_file = STRBUF_INIT; @@ -949,7 +947,7 @@ static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs, last_errno = errno; if (!refs_verify_refname_available( &refs->base, - refname, extras, skip, err)) + refname, NULL, NULL, err)) strbuf_addf(err, "there are still refs under '%s'", refname); goto error_return; @@ -962,7 +960,7 @@ static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs, last_errno = errno; if (last_errno != ENOTDIR || !refs_verify_refname_available(&refs->base, refname, - extras, skip, err)) + NULL, NULL, err)) strbuf_addf(err, "unable to resolve reference '%s': %s", refname, strerror(last_errno)); @@ -977,7 +975,7 @@ static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs, */ if (is_null_oid(&lock->old_oid) && refs_verify_refname_available(refs->packed_ref_store, refname, - extras, skip, err)) { + NULL, NULL, err)) { last_errno = ENOTDIR; goto error_return; } @@ -1412,9 +1410,7 @@ static int files_copy_or_rename_ref(struct ref_store *ref_store, logmoved = log; - - lock = lock_ref_oid_basic(refs, newrefname, NULL, NULL, NULL, - NULL, &err); + lock = lock_ref_oid_basic(refs, newrefname, NULL, NULL, &err); if (!lock) { if (copy) error("unable to copy '%s' to '%s': %s", oldrefname, newrefname, err.buf); @@ -1436,8 +1432,7 @@ static int files_copy_or_rename_ref(struct ref_store *ref_store, goto out; rollback: - lock = lock_ref_oid_basic(refs, oldrefname, NULL, NULL, NULL, - NULL, &err); + lock = lock_ref_oid_basic(refs, oldrefname, NULL, NULL, &err); if (!lock) { error("unable to lock %s for rollback: %s", oldrefname, err.buf); strbuf_release(&err); @@ -1844,9 +1839,7 @@ static int files_create_symref(struct ref_store *ref_store, struct ref_lock *lock; int ret; - lock = lock_ref_oid_basic(refs, refname, NULL, - NULL, NULL, NULL, - &err); + lock = lock_ref_oid_basic(refs, refname, NULL, NULL, &err); if (!lock) { error("%s", err.buf); strbuf_release(&err); @@ -3063,8 +3056,7 @@ static int files_reflog_expire(struct ref_store *ref_store, * reference itself, plus we might need to update the * reference if --updateref was specified: */ - lock = lock_ref_oid_basic(refs, refname, oid, - NULL, NULL, &type, &err); + lock = lock_ref_oid_basic(refs, refname, oid, &type, &err); if (!lock) { error("cannot lock ref '%s': %s", refname, err.buf); strbuf_release(&err); -- cgit v1.2.3 From 640d9d55c3fd73a103165b6457fb7c50ba9e829e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Mon, 23 Aug 2021 13:36:06 +0200 Subject: refs/files: remove unused "skip" in lock_raw_ref() too MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Remove the unused "skip" parameter to lock_raw_ref(), it was never used. We do use it when passing "skip" to the refs_rename_ref_available() function in files_copy_or_rename_ref(), but not here. This is part of a larger series that modifies lock_ref_oid_basic() extensively, there will be no more modifications of this function in this series, but since the preceding commit removed this unused parameter from lock_ref_oid_basic(), let's do it here too for consistency. Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Junio C Hamano --- refs/files-backend.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) (limited to 'refs') diff --git a/refs/files-backend.c b/refs/files-backend.c index 69f7f54e03..997e021c1c 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -531,7 +531,6 @@ static void unlock_ref(struct ref_lock *lock) static int lock_raw_ref(struct files_ref_store *refs, const char *refname, int mustexist, const struct string_list *extras, - const struct string_list *skip, struct ref_lock **lock_p, struct strbuf *referent, unsigned int *type, @@ -568,7 +567,7 @@ retry: * reason to expect this error to be transitory. */ if (refs_verify_refname_available(&refs->base, refname, - extras, skip, err)) { + extras, NULL, err)) { if (mustexist) { /* * To the user the relevant error is @@ -673,7 +672,7 @@ retry: REMOVE_DIR_EMPTY_ONLY)) { if (refs_verify_refname_available( &refs->base, refname, - extras, skip, err)) { + extras, NULL, err)) { /* * The error message set by * verify_refname_available() is OK. @@ -710,7 +709,7 @@ retry: */ if (refs_verify_refname_available( refs->packed_ref_store, refname, - extras, skip, err)) + extras, NULL, err)) goto error_return; } @@ -2407,7 +2406,7 @@ static int lock_ref_for_update(struct files_ref_store *refs, } ret = lock_raw_ref(refs, update->refname, mustexist, - affected_refnames, NULL, + affected_refnames, &lock, &referent, &update->type, err); if (ret) { -- cgit v1.2.3 From 81bc12258958ad293598840ad28df3817c9f4dfb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Mon, 23 Aug 2021 13:36:07 +0200 Subject: refs/debug: re-indent argument list for "prepare" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Re-indent this argument list that's been mis-indented since it was added in 34c319970d1 (refs/debug: trace into reflog expiry too, 2021-04-23). This makes a subsequent change smaller. Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Junio C Hamano --- refs/debug.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'refs') diff --git a/refs/debug.c b/refs/debug.c index 7db4abccc3..449ac3e6cc 100644 --- a/refs/debug.c +++ b/refs/debug.c @@ -364,8 +364,8 @@ struct debug_reflog_expiry_should_prune { }; static void debug_reflog_expiry_prepare(const char *refname, - const struct object_id *oid, - void *cb_data) + const struct object_id *oid, + void *cb_data) { struct debug_reflog_expiry_should_prune *prune = cb_data; trace_printf_key(&trace_refs, "reflog_expire_prepare: %s\n", refname); -- cgit v1.2.3 From 7aa7829f754ab722d558e0408ec6f3dbf23ba70f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Mon, 23 Aug 2021 13:36:09 +0200 Subject: refs/files: add a comment about refs_reflog_exists() call MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add a comment about why it is that we need to check for the the existence of a reflog we're deleting after we've successfully acquired the lock in files_reflog_expire(). As noted in [1] the lock protocol for reflogs is somewhat intuitive. This early exit code the comment applies to dates all the way back to 4264dc15e19 (git reflog expire, 2006-12-19). 1. https://lore.kernel.org/git/54DCDA42.2060800@alum.mit.edu/ Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Junio C Hamano --- refs/files-backend.c | 13 +++++++++++++ 1 file changed, 13 insertions(+) (limited to 'refs') diff --git a/refs/files-backend.c b/refs/files-backend.c index 997e021c1c..fbcd0c790b 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -3061,6 +3061,19 @@ static int files_reflog_expire(struct ref_store *ref_store, strbuf_release(&err); return -1; } + + /* + * When refs are deleted, their reflog is deleted before the + * ref itself is deleted. This is because there is no separate + * lock for reflog; instead we take a lock on the ref with + * lock_ref_oid_basic(). + * + * If a race happens and the reflog doesn't exist after we've + * acquired the lock that's OK. We've got nothing more to do; + * We were asked to delete the reflog, but someone else + * deleted it! The caller doesn't care that we deleted it, + * just that it is deleted. So we can return successfully. + */ if (!refs_reflog_exists(ref_store, refname)) { unlock_ref(lock); return 0; -- cgit v1.2.3 From ae35e16cd43b83070395327faa2fe0a91aa76c54 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Mon, 23 Aug 2021 13:36:10 +0200 Subject: reflog expire: don't lock reflogs using previously seen OID MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit During reflog expiry, the cmd_reflog_expire() function first iterates over all reflogs in logs/*, and then one-by-one acquires the lock for each one and expires it. This behavior has been with us since this command was implemented in 4264dc15e1 ("git reflog expire", 2006-12-19). Change this to stop calling lock_ref_oid_basic() with the OID we saw when we looped over the logs, instead have it pass the OID it managed to lock. This mostly mitigates a race condition where e.g. "git gc" will fail in a concurrently updated repository because the branch moved since "git reflog expire --all" was started. I.e. with: error: cannot lock ref '': ref '' is at but expected This behavior of passing in an "oid" was needed for an edge-case that I've untangled in this and preceding commits though, namely that we needed this OID because we'd: 1. Lookup the reflog name/OID via dwim_log() 2. With that OID, lock the reflog 3. Later in builtin/reflog.c we use the OID we looked as input to lookup_commit_reference_gently(), assured that it's equal to the OID we got from dwim_log(). We can be sure that this change is safe to make because between dwim_log (step #1) and lock_ref_oid_basic (step #2) there was no other logic relevant to the OID or expiry run in the cmd_reflog_expire() caller. We can thus treat that code as a black box, before and after this change it would get an OID that's been locked, the only difference is that now we mostly won't be failing to get the lock due to the TOCTOU race[0]. That failure was purely an implementation detail in how the "current OID" was looked up, it was divorced from the locking mechanism. What do we mean with "mostly"? It mostly mitigates it because we'll still run into cases where the ref is locked and being updated as we want to expire it, and other git processes wanting to update the refs will in turn race with us as we expire the reflog. That remaining race can in turn be mitigated with the core.filesRefLockTimeout setting, see 4ff0f01cb7 ("refs: retry acquiring reference locks for 100ms", 2017-08-21). In practice if that value is high enough we'll probably never have ref updates or reflog expiry failing, since the clients involved will retry for far longer than the time any of those operations could take. See [1] for an initial report of how this impacted "git gc" and a large discussion about this change in early 2019. In particular patch looked good to Michael Haggerty, see his[2]. That message seems to not have made it to the ML archive, its content is quoted in full in my [3]. I'm leaving behind now-unused code the refs API etc. that takes the now-NULL "unused_oid" argument, and other code that can be simplified now that we never have on OID in that context, that'll be cleaned up in subsequent commits, but for now let's narrowly focus on fixing the "git gc" issue. As the modified assert() shows we always pass a NULL oid to reflog_expire() now. Unfortunately this sort of probabilistic contention is hard to turn into a test. I've tested this by running the following three subshells in concurrent terminals: ( rm -rf /tmp/git && git init /tmp/git && while true do head -c 10 /dev/urandom | hexdump >/tmp/git/out && git -C /tmp/git add out && git -C /tmp/git commit -m"out" done ) ( rm -rf /tmp/git-clone && git clone file:///tmp/git /tmp/git-clone && while git -C /tmp/git-clone pull do date done ) ( while git -C /tmp/git-clone reflog expire --all do date done ) Before this change the "reflog expire" would fail really quickly with the "but expected" error noted above. After this change both the "pull" and "reflog expire" will run for a while, but eventually fail because I get unlucky with core.filesRefLockTimeout (the "reflog expire" is in a really tight loop). As noted above that can in turn be mitigated with higher values of core.filesRefLockTimeout than the 100ms default. As noted in the commentary added in the preceding commit there's also the case of branches being racily deleted, that can be tested by adding this to the above: ( while git -C /tmp/git-clone branch topic master && git -C /tmp/git-clone branch -D topic do date done ) With core.filesRefLockTimeout set to 10 seconds (it can probably be a lot lower) I managed to run all four of these concurrently for about an hour, and accumulated ~125k commits, auto-gc's and all, and didn't have a single failure. The loops visibly stall while waiting for the lock, but that's expected and desired behavior. 0. https://en.wikipedia.org/wiki/Time-of-check_to_time-of-use 1. https://lore.kernel.org/git/87tvg7brlm.fsf@evledraar.gmail.com/ 2. http://lore.kernel.org/git/b870a17d-2103-41b8-3cbc-7389d5fff33a@alum.mit.edu 3. https://lore.kernel.org/git/87pnqkco8v.fsf@evledraar.gmail.com/ Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Junio C Hamano --- refs/files-backend.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) (limited to 'refs') diff --git a/refs/files-backend.c b/refs/files-backend.c index fbcd0c790b..d81bda8bc2 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -3027,7 +3027,7 @@ static int expire_reflog_ent(struct object_id *ooid, struct object_id *noid, } static int files_reflog_expire(struct ref_store *ref_store, - const char *refname, const struct object_id *oid, + const char *refname, const struct object_id *unused_oid, unsigned int flags, reflog_expiry_prepare_fn prepare_fn, reflog_expiry_should_prune_fn should_prune_fn, @@ -3044,6 +3044,7 @@ static int files_reflog_expire(struct ref_store *ref_store, int status = 0; int type; struct strbuf err = STRBUF_INIT; + const struct object_id *oid; memset(&cb, 0, sizeof(cb)); cb.flags = flags; @@ -3055,12 +3056,13 @@ static int files_reflog_expire(struct ref_store *ref_store, * reference itself, plus we might need to update the * reference if --updateref was specified: */ - lock = lock_ref_oid_basic(refs, refname, oid, &type, &err); + lock = lock_ref_oid_basic(refs, refname, NULL, &type, &err); if (!lock) { error("cannot lock ref '%s': %s", refname, err.buf); strbuf_release(&err); return -1; } + oid = &lock->old_oid; /* * When refs are deleted, their reflog is deleted before the @@ -3104,6 +3106,7 @@ static int files_reflog_expire(struct ref_store *ref_store, } } + assert(!unused_oid); (*prepare_fn)(refname, oid, cb.policy_cb); refs_for_each_reflog_ent(ref_store, refname, expire_reflog_ent, &cb); (*cleanup_fn)(cb.policy_cb); -- cgit v1.2.3 From cc40b5ce137d0189aa52ab4c3d5ad3957342da5a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Mon, 23 Aug 2021 13:36:11 +0200 Subject: refs API: remove OID argument to reflog_expire() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Since the the preceding commit the "oid" parameter to reflog_expire() is always NULL, but it was not cleaned up to reduce the size of the diff. Let's do that subsequent API and documentation cleanup now. Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Junio C Hamano --- refs/debug.c | 4 ++-- refs/files-backend.c | 3 +-- refs/packed-backend.c | 2 +- refs/refs-internal.h | 2 +- 4 files changed, 5 insertions(+), 6 deletions(-) (limited to 'refs') diff --git a/refs/debug.c b/refs/debug.c index 449ac3e6cc..7793c9e1a3 100644 --- a/refs/debug.c +++ b/refs/debug.c @@ -391,7 +391,7 @@ static void debug_reflog_expiry_cleanup(void *cb_data) } static int debug_reflog_expire(struct ref_store *ref_store, const char *refname, - const struct object_id *oid, unsigned int flags, + unsigned int flags, reflog_expiry_prepare_fn prepare_fn, reflog_expiry_should_prune_fn should_prune_fn, reflog_expiry_cleanup_fn cleanup_fn, @@ -404,7 +404,7 @@ static int debug_reflog_expire(struct ref_store *ref_store, const char *refname, .should_prune = should_prune_fn, .cb_data = policy_cb_data, }; - int res = drefs->refs->be->reflog_expire(drefs->refs, refname, oid, + int res = drefs->refs->be->reflog_expire(drefs->refs, refname, flags, &debug_reflog_expiry_prepare, &debug_reflog_expiry_should_prune_fn, &debug_reflog_expiry_cleanup, diff --git a/refs/files-backend.c b/refs/files-backend.c index d81bda8bc2..a4adac4644 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -3027,7 +3027,7 @@ static int expire_reflog_ent(struct object_id *ooid, struct object_id *noid, } static int files_reflog_expire(struct ref_store *ref_store, - const char *refname, const struct object_id *unused_oid, + const char *refname, unsigned int flags, reflog_expiry_prepare_fn prepare_fn, reflog_expiry_should_prune_fn should_prune_fn, @@ -3106,7 +3106,6 @@ static int files_reflog_expire(struct ref_store *ref_store, } } - assert(!unused_oid); (*prepare_fn)(refname, oid, cb.policy_cb); refs_for_each_reflog_ent(ref_store, refname, expire_reflog_ent, &cb); (*cleanup_fn)(cb.policy_cb); diff --git a/refs/packed-backend.c b/refs/packed-backend.c index 24a360b719..65ba4214b8 100644 --- a/refs/packed-backend.c +++ b/refs/packed-backend.c @@ -1635,7 +1635,7 @@ static int packed_delete_reflog(struct ref_store *ref_store, } static int packed_reflog_expire(struct ref_store *ref_store, - const char *refname, const struct object_id *oid, + const char *refname, unsigned int flags, reflog_expiry_prepare_fn prepare_fn, reflog_expiry_should_prune_fn should_prune_fn, diff --git a/refs/refs-internal.h b/refs/refs-internal.h index 3155708345..d496c5ed52 100644 --- a/refs/refs-internal.h +++ b/refs/refs-internal.h @@ -593,7 +593,7 @@ typedef int create_reflog_fn(struct ref_store *ref_store, const char *refname, int force_create, struct strbuf *err); typedef int delete_reflog_fn(struct ref_store *ref_store, const char *refname); typedef int reflog_expire_fn(struct ref_store *ref_store, - const char *refname, const struct object_id *oid, + const char *refname, unsigned int flags, reflog_expiry_prepare_fn prepare_fn, reflog_expiry_should_prune_fn should_prune_fn, -- cgit v1.2.3 From ff7a2e4dbb0cf3844fbd0fbab7fba95d8e47c8dd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Mon, 23 Aug 2021 13:36:12 +0200 Subject: refs/files: remove unused "oid" in lock_ref_oid_basic() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In the preceding commit the last caller that passed a non-NULL OID was changed to pass NULL to lock_ref_oid_basic(). As noted in preceding commits use of this API has been going away (we should use ref transactions, or lock_raw_ref()), so we're unlikely to gain new callers that want to pass the "oid". So let's remove it, doing so means we can remove the "mustexist" condition, and therefore anything except the "flags = RESOLVE_REF_NO_RECURSE" case. Furthermore, since the verify_lock() function we called did most of its work when the "oid" was passed (as "old_oid") we can inline the trivial part of it that remains in its only remaining caller. Without a NULL "oid" passed it was equivalent to calling refs_read_ref_full() followed by oidclr(). Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Junio C Hamano --- refs/files-backend.c | 70 +++++++++++----------------------------------------- 1 file changed, 14 insertions(+), 56 deletions(-) (limited to 'refs') diff --git a/refs/files-backend.c b/refs/files-backend.c index a4adac4644..4f2d907597 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -852,42 +852,6 @@ static struct ref_iterator *files_ref_iterator_begin( return ref_iterator; } -/* - * Verify that the reference locked by lock has the value old_oid - * (unless it is NULL). Fail if the reference doesn't exist and - * mustexist is set. Return 0 on success. On error, write an error - * message to err, set errno, and return a negative value. - */ -static int verify_lock(struct ref_store *ref_store, struct ref_lock *lock, - const struct object_id *old_oid, int mustexist, - struct strbuf *err) -{ - assert(err); - - if (refs_read_ref_full(ref_store, lock->ref_name, - mustexist ? RESOLVE_REF_READING : 0, - &lock->old_oid, NULL)) { - if (old_oid) { - int save_errno = errno; - strbuf_addf(err, "can't verify ref '%s'", lock->ref_name); - errno = save_errno; - return -1; - } else { - oidclr(&lock->old_oid); - return 0; - } - } - if (old_oid && !oideq(&lock->old_oid, old_oid)) { - strbuf_addf(err, "ref '%s' is at %s but expected %s", - lock->ref_name, - oid_to_hex(&lock->old_oid), - oid_to_hex(old_oid)); - errno = EBUSY; - return -1; - } - return 0; -} - static int remove_empty_directories(struct strbuf *path) { /* @@ -912,15 +876,12 @@ static int create_reflock(const char *path, void *cb) * On failure errno is set to something meaningful. */ static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs, - const char *refname, - const struct object_id *old_oid, - int *type, struct strbuf *err) + const char *refname, int *type, + struct strbuf *err) { struct strbuf ref_file = STRBUF_INIT; struct ref_lock *lock; int last_errno = 0; - int mustexist = (old_oid && !is_null_oid(old_oid)); - int resolve_flags = RESOLVE_REF_NO_RECURSE; int resolved; files_assert_main_repository(refs, "lock_ref_oid_basic"); @@ -928,12 +889,9 @@ static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs, CALLOC_ARRAY(lock, 1); - if (mustexist) - resolve_flags |= RESOLVE_REF_READING; - files_ref_path(refs, &ref_file, refname); - resolved = !!refs_resolve_ref_unsafe(&refs->base, - refname, resolve_flags, + resolved = !!refs_resolve_ref_unsafe(&refs->base, refname, + RESOLVE_REF_NO_RECURSE, &lock->old_oid, type); if (!resolved && errno == EISDIR) { /* @@ -951,8 +909,8 @@ static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs, refname); goto error_return; } - resolved = !!refs_resolve_ref_unsafe(&refs->base, - refname, resolve_flags, + resolved = !!refs_resolve_ref_unsafe(&refs->base, refname, + RESOLVE_REF_NO_RECURSE, &lock->old_oid, type); } if (!resolved) { @@ -987,10 +945,10 @@ static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs, goto error_return; } - if (verify_lock(&refs->base, lock, old_oid, mustexist, err)) { - last_errno = errno; - goto error_return; - } + if (refs_read_ref_full(&refs->base, lock->ref_name, + 0, + &lock->old_oid, NULL)) + oidclr(&lock->old_oid); goto out; error_return: @@ -1409,7 +1367,7 @@ static int files_copy_or_rename_ref(struct ref_store *ref_store, logmoved = log; - lock = lock_ref_oid_basic(refs, newrefname, NULL, NULL, &err); + lock = lock_ref_oid_basic(refs, newrefname, NULL, &err); if (!lock) { if (copy) error("unable to copy '%s' to '%s': %s", oldrefname, newrefname, err.buf); @@ -1431,7 +1389,7 @@ static int files_copy_or_rename_ref(struct ref_store *ref_store, goto out; rollback: - lock = lock_ref_oid_basic(refs, oldrefname, NULL, NULL, &err); + lock = lock_ref_oid_basic(refs, oldrefname, NULL, &err); if (!lock) { error("unable to lock %s for rollback: %s", oldrefname, err.buf); strbuf_release(&err); @@ -1838,7 +1796,7 @@ static int files_create_symref(struct ref_store *ref_store, struct ref_lock *lock; int ret; - lock = lock_ref_oid_basic(refs, refname, NULL, NULL, &err); + lock = lock_ref_oid_basic(refs, refname, NULL, &err); if (!lock) { error("%s", err.buf); strbuf_release(&err); @@ -3056,7 +3014,7 @@ static int files_reflog_expire(struct ref_store *ref_store, * reference itself, plus we might need to update the * reference if --updateref was specified: */ - lock = lock_ref_oid_basic(refs, refname, NULL, &type, &err); + lock = lock_ref_oid_basic(refs, refname, &type, &err); if (!lock) { error("cannot lock ref '%s': %s", refname, err.buf); strbuf_release(&err); -- cgit v1.2.3 From 245fbba46d603b12300c4ed5858500b23a3f80fb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Mon, 23 Aug 2021 13:36:13 +0200 Subject: refs/files: remove unused "errno == EISDIR" code MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When we lock a reference like "foo" we need to handle the case where "foo" exists, but is an empty directory. That's what this code added in bc7127ef0f (ref locking: allow 'foo' when 'foo/bar' used to exist but not anymore., 2006-09-30) seems like it should be dealing with. Except it doesn't, and we never take this branch. The reason is that when bc7127ef0f was written this looked like: ref = resolve_ref([...]); if (!ref && errno == EISDIR) { [...] And in resolve_ref() we had this code: fd = open(path, O_RDONLY); if (fd < 0) return NULL; I.e. we would attempt to read "foo" with open(), which would fail with EISDIR and we'd return NULL. We'd then take this branch, call remove_empty_directories() and continue. Since a1c1d8170d (refs_resolve_ref_unsafe: handle d/f conflicts for writes, 2017-10-06) we don't. E.g. in the case of files_copy_or_rename_ref() our callstack will look something like: [...] -> files_copy_or_rename_ref() -> lock_ref_oid_basic() -> refs_resolve_ref_unsafe() At that point the first (now only) refs_resolve_ref_unsafe() call in lock_ref_oid_basic() would do the equivalent of this in the resulting call to refs_read_raw_ref() in refs_resolve_ref_unsafe(): /* Via refs_read_raw_ref() */ fd = open(path, O_RDONLY); if (fd < 0) /* get errno == EISDIR */ /* later, in refs_resolve_ref_unsafe() */ if ([...] && errno != EISDIR) return NULL; [...] /* returns the refs/heads/foo to the caller, even though it's a directory */ return refname; I.e. even though we got an "errno == EISDIR" we won't take this branch, since in cases of EISDIR "resolved" is always non-NULL. I.e. we pretend at this point as though everything's OK and there is no "foo" directory. We then proceed with the entire ref update and don't call remove_empty_directories() until we call commit_ref_update(). See 5387c0d883 (commit_ref(): if there is an empty dir in the way, delete it, 2016-05-05) for the addition of that code, and a1c1d8170db (refs_resolve_ref_unsafe: handle d/f conflicts for writes, 2017-10-06) for the commit that changed the original codepath added in bc7127ef0f to use this "EISDIR" handling. Further historical commentary: Before the two preceding commits the caller in files_reflog_expire() was the only one out of our 4 callers that would pass non-NULL as an oid. We would then set a (now gone) "resolve_flags" to "RESOLVE_REF_READING" and just before that "errno != EISDIR" check do: if (resolve_flags & RESOLVE_REF_READING) return NULL; There may have been some case where this ended up mattering and we couldn't safely make this change before we removed the "oid" parameter, but I don't think there was, see [1] for some discussion on that. In any case, now that we've removed the "oid" parameter in a preceding commit we can be sure that this code is redundant, so let's remove it. 1. http://lore.kernel.org/git/871r801yp6.fsf@evledraar.gmail.com Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Junio C Hamano --- refs/files-backend.c | 28 +++------------------------- 1 file changed, 3 insertions(+), 25 deletions(-) (limited to 'refs') diff --git a/refs/files-backend.c b/refs/files-backend.c index 4f2d907597..bed2ab25c3 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -882,7 +882,6 @@ static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs, struct strbuf ref_file = STRBUF_INIT; struct ref_lock *lock; int last_errno = 0; - int resolved; files_assert_main_repository(refs, "lock_ref_oid_basic"); assert(err); @@ -890,30 +889,9 @@ static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs, CALLOC_ARRAY(lock, 1); files_ref_path(refs, &ref_file, refname); - resolved = !!refs_resolve_ref_unsafe(&refs->base, refname, - RESOLVE_REF_NO_RECURSE, - &lock->old_oid, type); - if (!resolved && errno == EISDIR) { - /* - * we are trying to lock foo but we used to - * have foo/bar which now does not exist; - * it is normal for the empty directory 'foo' - * to remain. - */ - if (remove_empty_directories(&ref_file)) { - last_errno = errno; - if (!refs_verify_refname_available( - &refs->base, - refname, NULL, NULL, err)) - strbuf_addf(err, "there are still refs under '%s'", - refname); - goto error_return; - } - resolved = !!refs_resolve_ref_unsafe(&refs->base, refname, - RESOLVE_REF_NO_RECURSE, - &lock->old_oid, type); - } - if (!resolved) { + if (!refs_resolve_ref_unsafe(&refs->base, refname, + RESOLVE_REF_NO_RECURSE, + &lock->old_oid, type)) { last_errno = errno; if (last_errno != ENOTDIR || !refs_verify_refname_available(&refs->base, refname, -- cgit v1.2.3 From 48cdcd9ca0daa7904f299a00433a593a5941a2d3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Mon, 23 Aug 2021 13:36:14 +0200 Subject: refs/files: remove unused "errno != ENOTDIR" condition MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit As a follow-up to the preceding commit where we removed the adjacent "errno == EISDIR" condition in the same function, remove the "last_errno != ENOTDIR" condition here. It's not possible for us to hit this condition added in 5b2d8d6f218 (lock_ref_sha1_basic(): improve diagnostics for ref D/F conflicts, 2015-05-11). Since a1c1d8170db (refs_resolve_ref_unsafe: handle d/f conflicts for writes, 2017-10-06) we've explicitly caught these in refs_resolve_ref_unsafe() before returning NULL: if (errno != ENOENT && errno != EISDIR && errno != ENOTDIR) return NULL; We'd then always return the refname from refs_resolve_ref_unsafe() even if we were in a broken state as explained in the preceding commit. The elided context here is a call to refs_resolve_ref_unsafe(). Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Junio C Hamano --- refs/files-backend.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) (limited to 'refs') diff --git a/refs/files-backend.c b/refs/files-backend.c index bed2ab25c3..ab666af4b7 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -893,8 +893,7 @@ static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs, RESOLVE_REF_NO_RECURSE, &lock->old_oid, type)) { last_errno = errno; - if (last_errno != ENOTDIR || - !refs_verify_refname_available(&refs->base, refname, + if (!refs_verify_refname_available(&refs->base, refname, NULL, NULL, err)) strbuf_addf(err, "unable to resolve reference '%s': %s", refname, strerror(last_errno)); -- cgit v1.2.3 From 3fa2e91d173b260c19f19af8edd3907e65bf85f7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Mon, 23 Aug 2021 13:52:37 +0200 Subject: refs file backend: move raceproof_create_file() here MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Move the raceproof_create_file() API added to cache.h and object-file.c in 177978f56ad (raceproof_create_file(): new function, 2017-01-06) to its only user, refs/files-backend.c. Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Junio C Hamano --- refs/files-backend.c | 109 +++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 109 insertions(+) (limited to 'refs') diff --git a/refs/files-backend.c b/refs/files-backend.c index ab666af4b7..950f9738ae 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -852,6 +852,115 @@ static struct ref_iterator *files_ref_iterator_begin( return ref_iterator; } +/* + * Callback function for raceproof_create_file(). This function is + * expected to do something that makes dirname(path) permanent despite + * the fact that other processes might be cleaning up empty + * directories at the same time. Usually it will create a file named + * path, but alternatively it could create another file in that + * directory, or even chdir() into that directory. The function should + * return 0 if the action was completed successfully. On error, it + * should return a nonzero result and set errno. + * raceproof_create_file() treats two errno values specially: + * + * - ENOENT -- dirname(path) does not exist. In this case, + * raceproof_create_file() tries creating dirname(path) + * (and any parent directories, if necessary) and calls + * the function again. + * + * - EISDIR -- the file already exists and is a directory. In this + * case, raceproof_create_file() removes the directory if + * it is empty (and recursively any empty directories that + * it contains) and calls the function again. + * + * Any other errno causes raceproof_create_file() to fail with the + * callback's return value and errno. + * + * Obviously, this function should be OK with being called again if it + * fails with ENOENT or EISDIR. In other scenarios it will not be + * called again. + */ +typedef int create_file_fn(const char *path, void *cb); + +/* + * Create a file in dirname(path) by calling fn, creating leading + * directories if necessary. Retry a few times in case we are racing + * with another process that is trying to clean up the directory that + * contains path. See the documentation for create_file_fn for more + * details. + * + * Return the value and set the errno that resulted from the most + * recent call of fn. fn is always called at least once, and will be + * called more than once if it returns ENOENT or EISDIR. + */ +static int raceproof_create_file(const char *path, create_file_fn fn, void *cb) +{ + /* + * The number of times we will try to remove empty directories + * in the way of path. This is only 1 because if another + * process is racily creating directories that conflict with + * us, we don't want to fight against them. + */ + int remove_directories_remaining = 1; + + /* + * The number of times that we will try to create the + * directories containing path. We are willing to attempt this + * more than once, because another process could be trying to + * clean up empty directories at the same time as we are + * trying to create them. + */ + int create_directories_remaining = 3; + + /* A scratch copy of path, filled lazily if we need it: */ + struct strbuf path_copy = STRBUF_INIT; + + int ret, save_errno; + + /* Sanity check: */ + assert(*path); + +retry_fn: + ret = fn(path, cb); + save_errno = errno; + if (!ret) + goto out; + + if (errno == EISDIR && remove_directories_remaining-- > 0) { + /* + * A directory is in the way. Maybe it is empty; try + * to remove it: + */ + if (!path_copy.len) + strbuf_addstr(&path_copy, path); + + if (!remove_dir_recursively(&path_copy, REMOVE_DIR_EMPTY_ONLY)) + goto retry_fn; + } else if (errno == ENOENT && create_directories_remaining-- > 0) { + /* + * Maybe the containing directory didn't exist, or + * maybe it was just deleted by a process that is + * racing with us to clean up empty directories. Try + * to create it: + */ + enum scld_error scld_result; + + if (!path_copy.len) + strbuf_addstr(&path_copy, path); + + do { + scld_result = safe_create_leading_directories(path_copy.buf); + if (scld_result == SCLD_OK) + goto retry_fn; + } while (scld_result == SCLD_VANISHED && create_directories_remaining-- > 0); + } + +out: + strbuf_release(&path_copy); + errno = save_errno; + return ret; +} + static int remove_empty_directories(struct strbuf *path) { /* -- cgit v1.2.3 From 20d422cfd7f41df671bf98972ff89d68dfa0ed7b Mon Sep 17 00:00:00 2001 From: Han-Wen Nienhuys Date: Mon, 23 Aug 2021 13:52:38 +0200 Subject: refs: remove EINVAL errno output from specification of read_raw_ref_fn MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This commit does not change code; it documents the fact that an alternate ref backend does not need to return EINVAL from read_raw_ref_fn to function properly. This is correct, because refs_read_raw_ref is only called from; * resolve_ref_unsafe(), which does not care for the EINVAL errno result. * refs_verify_refname_available(), which does not inspect errno. * files-backend.c, where errno is overwritten on failure. * packed-backend.c (is_packed_transaction_needed), which calls it for the packed ref backend, which never emits EINVAL. A grep for EINVAL */*c reveals that no code checks errno against EINVAL after reading references. In addition, the refs.h file does not mention errno at all. A grep over resolve_ref_unsafe() turned up the following callers that inspect errno: * sequencer.c::print_commit_summary, which uses it for die_errno * lock_ref_oid_basic(), which only treats EISDIR and ENOTDIR specially. The files ref backend does use EINVAL. The files backend does not call into the generic API (refs_read_raw), but into the files-specific function (files_read_raw_ref), which we are not changing in this commit. As the errno sideband is unintuitive and error-prone, remove EINVAL value, as a step towards getting rid of the errno sideband altogether. Spotted by Ævar Arnfjörð Bjarmason . Signed-off-by: Han-Wen Nienhuys Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Junio C Hamano --- refs/refs-internal.h | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) (limited to 'refs') diff --git a/refs/refs-internal.h b/refs/refs-internal.h index d496c5ed52..1c6e5ab51d 100644 --- a/refs/refs-internal.h +++ b/refs/refs-internal.h @@ -622,9 +622,9 @@ typedef int reflog_expire_fn(struct ref_store *ref_store, * * Return 0 on success. If the ref doesn't exist, set errno to ENOENT * and return -1. If the ref exists but is neither a symbolic ref nor - * an object ID, it is broken; set REF_ISBROKEN in type, set errno to - * EINVAL, and return -1. If there is another error reading the ref, - * set errno appropriately and return -1. + * an object ID, it is broken; set REF_ISBROKEN in type, and return -1 + * (errno should not be ENOENT) If there is another error reading the + * ref, set errno appropriately and return -1. * * Backend-specific flags might be set in type as well, regardless of * outcome. -- cgit v1.2.3 From 1ae6ed230ae696ceb7c4607e00d94642b416ea1c Mon Sep 17 00:00:00 2001 From: Han-Wen Nienhuys Date: Mon, 23 Aug 2021 13:52:39 +0200 Subject: refs/files-backend: stop setting errno from lock_ref_oid_basic MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit refs/files-backend.c::lock_ref_oid_basic() tries to signal how it failed to its callers using errno. It is safe to stop setting errno here, because the callers of this file-scope static function are * files_copy_or_rename_ref() * files_create_symref() * files_reflog_expire() None of them looks at errno after seeing a negative return from lock_ref_oid_basic() to make any decision, and no caller of these three functions looks at errno after they signal a failure by returning a negative value. In particular, * files_copy_or_rename_ref() - here, calls are followed by error() (which performs I/O) or write_ref_to_lockfile() (which calls parse_object() which may perform I/O) * files_create_symref() - here, calls are followed by error() or create_symref_locked() (which performs I/O and does not inspect errno) * files_reflog_expire() - here, calls are followed by error() or refs_reflog_exists() (which calls a function in a vtable that is not documented to use and/or preserve errno) Signed-off-by: Han-Wen Nienhuys Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Junio C Hamano --- refs/files-backend.c | 11 ++--------- 1 file changed, 2 insertions(+), 9 deletions(-) (limited to 'refs') diff --git a/refs/files-backend.c b/refs/files-backend.c index 950f9738ae..c1794dcd29 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -982,7 +982,6 @@ static int create_reflock(const char *path, void *cb) /* * Locks a ref returning the lock on success and NULL on failure. - * On failure errno is set to something meaningful. */ static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs, const char *refname, int *type, @@ -990,7 +989,6 @@ static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs, { struct strbuf ref_file = STRBUF_INIT; struct ref_lock *lock; - int last_errno = 0; files_assert_main_repository(refs, "lock_ref_oid_basic"); assert(err); @@ -1001,11 +999,10 @@ static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs, if (!refs_resolve_ref_unsafe(&refs->base, refname, RESOLVE_REF_NO_RECURSE, &lock->old_oid, type)) { - last_errno = errno; if (!refs_verify_refname_available(&refs->base, refname, NULL, NULL, err)) strbuf_addf(err, "unable to resolve reference '%s': %s", - refname, strerror(last_errno)); + refname, strerror(errno)); goto error_return; } @@ -1018,15 +1015,12 @@ static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs, */ if (is_null_oid(&lock->old_oid) && refs_verify_refname_available(refs->packed_ref_store, refname, - NULL, NULL, err)) { - last_errno = ENOTDIR; + NULL, NULL, err)) goto error_return; - } lock->ref_name = xstrdup(refname); if (raceproof_create_file(ref_file.buf, create_reflock, &lock->lk)) { - last_errno = errno; unable_to_lock_message(ref_file.buf, errno, err); goto error_return; } @@ -1043,7 +1037,6 @@ static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs, out: strbuf_release(&ref_file); - errno = last_errno; return lock; } -- cgit v1.2.3 From 5b12e16bb134969747eaa983ab8d83d57f41e960 Mon Sep 17 00:00:00 2001 From: Han-Wen Nienhuys Date: Mon, 23 Aug 2021 13:52:40 +0200 Subject: refs: make errno output explicit for read_raw_ref_fn MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This makes it explicit how alternative ref backends should report errors in read_raw_ref_fn. read_raw_ref_fn needs to supply a credible errno for a number of cases. These are primarily: 1) The files backend calls read_raw_ref from lock_raw_ref, and uses the resulting error codes to create/remove directories as needed. 2) ENOENT should be translated in a zero OID, optionally with REF_ISBROKEN set, returning the last successfully resolved symref. This is necessary so read_raw_ref("HEAD") on an empty repo returns refs/heads/main (or the default branch du-jour), and we know on which branch to create the first commit. Make this information flow explicit by adding a failure_errno to the signature of read_raw_ref. All errnos from the files backend are still propagated unchanged, even though inspection suggests only ENOTDIR, EISDIR and ENOENT are relevant. Signed-off-by: Han-Wen Nienhuys Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Junio C Hamano --- refs/debug.c | 7 +++---- refs/files-backend.c | 29 +++++++++++++++-------------- refs/packed-backend.c | 8 ++++---- refs/refs-internal.h | 20 ++++++++++++-------- 4 files changed, 34 insertions(+), 30 deletions(-) (limited to 'refs') diff --git a/refs/debug.c b/refs/debug.c index 7793c9e1a3..dfacf79e94 100644 --- a/refs/debug.c +++ b/refs/debug.c @@ -238,15 +238,14 @@ debug_ref_iterator_begin(struct ref_store *ref_store, const char *prefix, static int debug_read_raw_ref(struct ref_store *ref_store, const char *refname, struct object_id *oid, struct strbuf *referent, - unsigned int *type) + unsigned int *type, int *failure_errno) { struct debug_ref_store *drefs = (struct debug_ref_store *)ref_store; int res = 0; oidcpy(oid, null_oid()); - errno = 0; res = drefs->refs->be->read_raw_ref(drefs->refs, refname, oid, referent, - type); + type, failure_errno); if (res == 0) { trace_printf_key(&trace_refs, "read_raw_ref: %s: %s (=> %s) type %x: %d\n", @@ -254,7 +253,7 @@ static int debug_read_raw_ref(struct ref_store *ref_store, const char *refname, } else { trace_printf_key(&trace_refs, "read_raw_ref: %s: %d (errno %d)\n", refname, - res, errno); + res, *failure_errno); } return res; } diff --git a/refs/files-backend.c b/refs/files-backend.c index c1794dcd29..b3de363396 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -341,9 +341,9 @@ static struct ref_cache *get_loose_ref_cache(struct files_ref_store *refs) return refs->loose; } -static int files_read_raw_ref(struct ref_store *ref_store, - const char *refname, struct object_id *oid, - struct strbuf *referent, unsigned int *type) +static int files_read_raw_ref(struct ref_store *ref_store, const char *refname, + struct object_id *oid, struct strbuf *referent, + unsigned int *type, int *failure_errno) { struct files_ref_store *refs = files_downcast(ref_store, REF_STORE_READ, "read_raw_ref"); @@ -354,7 +354,6 @@ static int files_read_raw_ref(struct ref_store *ref_store, struct stat st; int fd; int ret = -1; - int save_errno; int remaining_retries = 3; *type = 0; @@ -459,10 +458,9 @@ stat_ref: ret = parse_loose_ref_contents(buf, oid, referent, type); out: - save_errno = errno; + *failure_errno = errno; strbuf_release(&sb_path); strbuf_release(&sb_contents); - errno = save_errno; return ret; } @@ -540,6 +538,7 @@ static int lock_raw_ref(struct files_ref_store *refs, struct strbuf ref_file = STRBUF_INIT; int attempts_remaining = 3; int ret = TRANSACTION_GENERIC_ERROR; + int failure_errno; assert(err); files_assert_main_repository(refs, "lock_raw_ref"); @@ -610,7 +609,9 @@ retry: if (hold_lock_file_for_update_timeout( &lock->lk, ref_file.buf, LOCK_NO_DEREF, get_files_ref_lock_timeout_ms()) < 0) { - if (errno == ENOENT && --attempts_remaining > 0) { + int myerr = errno; + errno = 0; + if (myerr == ENOENT && --attempts_remaining > 0) { /* * Maybe somebody just deleted one of the * directories leading to ref_file. Try @@ -618,7 +619,7 @@ retry: */ goto retry; } else { - unable_to_lock_message(ref_file.buf, errno, err); + unable_to_lock_message(ref_file.buf, myerr, err); goto error_return; } } @@ -628,9 +629,9 @@ retry: * fear that its value will change. */ - if (files_read_raw_ref(&refs->base, refname, - &lock->old_oid, referent, type)) { - if (errno == ENOENT) { + if (files_read_raw_ref(&refs->base, refname, &lock->old_oid, referent, + type, &failure_errno)) { + if (failure_errno == ENOENT) { if (mustexist) { /* Garden variety missing reference. */ strbuf_addf(err, "unable to resolve reference '%s'", @@ -654,7 +655,7 @@ retry: * reference named "refs/foo/bar/baz". */ } - } else if (errno == EISDIR) { + } else if (failure_errno == EISDIR) { /* * There is a directory in the way. It might have * contained references that have been deleted. If @@ -692,13 +693,13 @@ retry: goto error_return; } } - } else if (errno == EINVAL && (*type & REF_ISBROKEN)) { + } else if (failure_errno == EINVAL && (*type & REF_ISBROKEN)) { strbuf_addf(err, "unable to resolve reference '%s': " "reference broken", refname); goto error_return; } else { strbuf_addf(err, "unable to resolve reference '%s': %s", - refname, strerror(errno)); + refname, strerror(failure_errno)); goto error_return; } diff --git a/refs/packed-backend.c b/refs/packed-backend.c index 65ba4214b8..47247a1491 100644 --- a/refs/packed-backend.c +++ b/refs/packed-backend.c @@ -724,9 +724,9 @@ static struct snapshot *get_snapshot(struct packed_ref_store *refs) return refs->snapshot; } -static int packed_read_raw_ref(struct ref_store *ref_store, - const char *refname, struct object_id *oid, - struct strbuf *referent, unsigned int *type) +static int packed_read_raw_ref(struct ref_store *ref_store, const char *refname, + struct object_id *oid, struct strbuf *referent, + unsigned int *type, int *failure_errno) { struct packed_ref_store *refs = packed_downcast(ref_store, REF_STORE_READ, "read_raw_ref"); @@ -739,7 +739,7 @@ static int packed_read_raw_ref(struct ref_store *ref_store, if (!rec) { /* refname is not a packed reference. */ - errno = ENOENT; + *failure_errno = ENOENT; return -1; } diff --git a/refs/refs-internal.h b/refs/refs-internal.h index 1c6e5ab51d..ddd6d7f8eb 100644 --- a/refs/refs-internal.h +++ b/refs/refs-internal.h @@ -620,11 +620,15 @@ typedef int reflog_expire_fn(struct ref_store *ref_store, * properly-formatted or even safe reference name. NEITHER INPUT NOR * OUTPUT REFERENCE NAMES ARE VALIDATED WITHIN THIS FUNCTION. * - * Return 0 on success. If the ref doesn't exist, set errno to ENOENT - * and return -1. If the ref exists but is neither a symbolic ref nor - * an object ID, it is broken; set REF_ISBROKEN in type, and return -1 - * (errno should not be ENOENT) If there is another error reading the - * ref, set errno appropriately and return -1. + * Return 0 on success, or -1 on failure. If the ref exists but is neither a + * symbolic ref nor an object ID, it is broken. In this case set REF_ISBROKEN in + * type, and return -1 (failure_errno should not be ENOENT) + * + * failure_errno provides errno codes that are interpreted beyond error + * reporting. The following error codes have special meaning: + * * ENOENT: the ref doesn't exist + * * EISDIR: ref name is a directory + * * ENOTDIR: ref prefix is not a directory * * Backend-specific flags might be set in type as well, regardless of * outcome. @@ -638,9 +642,9 @@ typedef int reflog_expire_fn(struct ref_store *ref_store, * - in all other cases, referent will be untouched, and therefore * refname will still be valid and unchanged. */ -typedef int read_raw_ref_fn(struct ref_store *ref_store, - const char *refname, struct object_id *oid, - struct strbuf *referent, unsigned int *type); +typedef int read_raw_ref_fn(struct ref_store *ref_store, const char *refname, + struct object_id *oid, struct strbuf *referent, + unsigned int *type, int *failure_errno); struct ref_storage_be { struct ref_storage_be *next; -- cgit v1.2.3 From 35cf94eaf6c8bdbed86549c2349aa3c3b40b7b09 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9=20Scharfe?= Date: Thu, 9 Sep 2021 23:45:51 +0200 Subject: refs/files-backend: remove unused open mode parameter MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We only need to provide a mode if we are willing to let open(2) create the file, which is not the case here, so drop the unnecessary parameter. Signed-off-by: René Scharfe Signed-off-by: Junio C Hamano --- refs/files-backend.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'refs') diff --git a/refs/files-backend.c b/refs/files-backend.c index 677b7e4cdd..74c0385873 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -1569,7 +1569,7 @@ static int log_ref_setup(struct files_ref_store *refs, goto error; } } else { - *logfd = open(logfile, O_APPEND | O_WRONLY, 0666); + *logfd = open(logfile, O_APPEND | O_WRONLY); if (*logfd < 0) { if (errno == ENOENT || errno == EISDIR) { /* -- cgit v1.2.3 From bf708add2eaf37d53fa8a908b5d8772806c54d1b Mon Sep 17 00:00:00 2001 From: Jeff King Date: Fri, 24 Sep 2021 14:37:58 -0400 Subject: refs-internal.h: move DO_FOR_EACH_* flags next to each other There are currently two DO_FOR_EACH_* flags, which must not have their bits overlap. Yet they're defined hundreds of lines apart. Let's move them next to each other to make it clear that they are related and are a complete set (which matters if you are adding a new flag and would like to know what the next available bit is). Signed-off-by: Jeff King Reviewed-by: Jonathan Tan Signed-off-by: Junio C Hamano --- refs/refs-internal.h | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) (limited to 'refs') diff --git a/refs/refs-internal.h b/refs/refs-internal.h index 3155708345..7b30910974 100644 --- a/refs/refs-internal.h +++ b/refs/refs-internal.h @@ -248,6 +248,14 @@ int refs_rename_ref_available(struct ref_store *refs, /* Include broken references in a do_for_each_ref*() iteration: */ #define DO_FOR_EACH_INCLUDE_BROKEN 0x01 +/* + * Only include per-worktree refs in a do_for_each_ref*() iteration. + * Normally this will be used with a files ref_store, since that's + * where all reference backends will presumably store their + * per-worktree refs. + */ +#define DO_FOR_EACH_PER_WORKTREE_ONLY 0x02 + /* * Reference iterators * @@ -498,14 +506,6 @@ int do_for_each_repo_ref_iterator(struct repository *r, struct ref_iterator *iter, each_repo_ref_fn fn, void *cb_data); -/* - * Only include per-worktree refs in a do_for_each_ref*() iteration. - * Normally this will be used with a files ref_store, since that's - * where all reference backends will presumably store their - * per-worktree refs. - */ -#define DO_FOR_EACH_PER_WORKTREE_ONLY 0x02 - struct ref_store; /* refs backends */ -- cgit v1.2.3 From 9aab952e855be1a567d4d0585afbdbde624e274f Mon Sep 17 00:00:00 2001 From: Jeff King Date: Fri, 24 Sep 2021 14:39:44 -0400 Subject: refs-internal.h: reorganize DO_FOR_EACH_* flag documentation The documentation for the DO_FOR_EACH_* flags is sprinkled over the refs-internal.h file. We define the two flags in one spot, and then describe them in more detail far away from there, in the definitions of refs_ref_iterator_begin() and ref_iterator_advance_fn(). Let's try to organize this a bit better: - convert the #defines to an enum. This makes it clear that they are related, and that the enum shows the complete set of flags. - combine all descriptions for each flag in a single spot, next to the flag's definition - use the enum rather than a bare int for functions which take the flags. This helps readers realize which flags can be used. - clarify the mention of flags for ref_iterator_advance_fn(). It does not take flags itself, but is meant to depend on ones set up earlier. Signed-off-by: Jeff King Reviewed-by: Jonathan Tan Signed-off-by: Junio C Hamano --- refs/refs-internal.h | 46 +++++++++++++++++++++++++++------------------- 1 file changed, 27 insertions(+), 19 deletions(-) (limited to 'refs') diff --git a/refs/refs-internal.h b/refs/refs-internal.h index 7b30910974..2c4e1739f2 100644 --- a/refs/refs-internal.h +++ b/refs/refs-internal.h @@ -245,16 +245,30 @@ int refs_rename_ref_available(struct ref_store *refs, /* We allow "recursive" symbolic refs. Only within reason, though */ #define SYMREF_MAXDEPTH 5 -/* Include broken references in a do_for_each_ref*() iteration: */ -#define DO_FOR_EACH_INCLUDE_BROKEN 0x01 - /* - * Only include per-worktree refs in a do_for_each_ref*() iteration. - * Normally this will be used with a files ref_store, since that's - * where all reference backends will presumably store their - * per-worktree refs. + * These flags are passed to refs_ref_iterator_begin() (and do_for_each_ref(), + * which feeds it). */ -#define DO_FOR_EACH_PER_WORKTREE_ONLY 0x02 +enum do_for_each_ref_flags { + /* + * Include broken references in a do_for_each_ref*() iteration, which + * would normally be omitted. This includes both refs that point to + * missing objects (a true repository corruption), ones with illegal + * names (which we prefer not to expose to callers), as well as + * dangling symbolic refs (i.e., those that point to a non-existent + * ref; this is not a corruption, but as they have no valid oid, we + * omit them from normal iteration results). + */ + DO_FOR_EACH_INCLUDE_BROKEN = (1 << 0), + + /* + * Only include per-worktree refs in a do_for_each_ref*() iteration. + * Normally this will be used with a files ref_store, since that's + * where all reference backends will presumably store their + * per-worktree refs. + */ + DO_FOR_EACH_PER_WORKTREE_ONLY = (1 << 1), +}; /* * Reference iterators @@ -357,16 +371,12 @@ int is_empty_ref_iterator(struct ref_iterator *ref_iterator); * Return an iterator that goes over each reference in `refs` for * which the refname begins with prefix. If trim is non-zero, then * trim that many characters off the beginning of each refname. - * The output is ordered by refname. The following flags are supported: - * - * DO_FOR_EACH_INCLUDE_BROKEN: include broken references in - * the iteration. - * - * DO_FOR_EACH_PER_WORKTREE_ONLY: only produce REF_TYPE_PER_WORKTREE refs. + * The output is ordered by refname. */ struct ref_iterator *refs_ref_iterator_begin( struct ref_store *refs, - const char *prefix, int trim, int flags); + const char *prefix, int trim, + enum do_for_each_ref_flags flags); /* * A callback function used to instruct merge_ref_iterator how to @@ -454,10 +464,8 @@ void base_ref_iterator_free(struct ref_iterator *iter); /* * backend-specific implementation of ref_iterator_advance. For symrefs, the * function should set REF_ISSYMREF, and it should also dereference the symref - * to provide the OID referent. If DO_FOR_EACH_INCLUDE_BROKEN is set, symrefs - * with non-existent referents and refs pointing to non-existent object names - * should also be returned. If DO_FOR_EACH_PER_WORKTREE_ONLY, only - * REF_TYPE_PER_WORKTREE refs should be returned. + * to provide the OID referent. It should respect do_for_each_ref_flags + * that were passed to refs_ref_iterator_begin(). */ typedef int ref_iterator_advance_fn(struct ref_iterator *ref_iterator); -- cgit v1.2.3 From 8dccb2244c81258cf9f3480c4102d14e30978194 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Fri, 24 Sep 2021 14:41:32 -0400 Subject: refs: add DO_FOR_EACH_OMIT_DANGLING_SYMREFS flag When the DO_FOR_EACH_INCLUDE_BROKEN flag is used, we include both actual corrupt refs (illegal names, missing objects), but also symrefs that point to nothing. This latter is not really a corruption, but just something that may happen normally. For example, the symref at refs/remotes/origin/HEAD may point to a tracking branch which is later deleted. (The local HEAD may also be unborn, of course, but we do not access it through ref iteration). Most callers of for_each_ref() etc, do not care. They don't pass INCLUDE_BROKEN, so don't see it at all. But for those which do pass it, this somewhat-normal state causes extra warnings (e.g., from for-each-ref) or even aborts operations (destructive repacks with GIT_REF_PARANOIA set). This patch just introduces the flag and the mechanism; there are no callers yet (and hence no tests). Two things to note on the implementation: - we actually skip any symref that does not resolve to a ref. This includes ones which point to an invalidly-named ref. You could argue this is a more serious breakage than simple dangling. But the overall effect is the same (we could not follow the symref), as well as the impact on things like REF_PARANOIA (either way, a symref we can't follow won't impact reachability, because we'll see the ref itself during iteration). The underlying resolution function doesn't distinguish these two cases (they both get REF_ISBROKEN). - we change the iterator in refs/files-backend.c where we check INCLUDE_BROKEN. There's a matching spot in refs/packed-backend.c, but we don't know need to do anything there. The packed backend does not support symrefs at all. The resulting set of flags might be a bit easier to follow if we broke this down into "INCLUDE_CORRUPT_REFS" and "INCLUDE_DANGLING_SYMREFS". But there are a few reasons not do so: - adding a new OMIT_DANGLING_SYMREFS flag lets us leave existing callers intact, without changing their behavior (and some of them really do want to see the dangling symrefs; e.g., t5505 has a test which expects us to report when a symref becomes dangling) - they're not actually independent. You cannot say "include dangling symrefs" without also including refs whose objects are not reachable, because dangling symrefs by definition do not have an object. We could tweak the implementation to distinguish this, but in practice nobody wants to ask for that. Adding the OMIT flag keeps the implementation simple and makes sure we don't regress the current behavior. Signed-off-by: Jeff King Reviewed-by: Jonathan Tan Signed-off-by: Junio C Hamano --- refs/files-backend.c | 5 +++++ refs/refs-internal.h | 6 ++++++ 2 files changed, 11 insertions(+) (limited to 'refs') diff --git a/refs/files-backend.c b/refs/files-backend.c index 74c0385873..1148c0cf09 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -744,6 +744,11 @@ static int files_ref_iterator_advance(struct ref_iterator *ref_iterator) ref_type(iter->iter0->refname) != REF_TYPE_PER_WORKTREE) continue; + if ((iter->flags & DO_FOR_EACH_OMIT_DANGLING_SYMREFS) && + (iter->iter0->flags & REF_ISSYMREF) && + (iter->iter0->flags & REF_ISBROKEN)) + continue; + if (!(iter->flags & DO_FOR_EACH_INCLUDE_BROKEN) && !ref_resolves_to_object(iter->iter0->refname, iter->iter0->oid, diff --git a/refs/refs-internal.h b/refs/refs-internal.h index 2c4e1739f2..96911fb26e 100644 --- a/refs/refs-internal.h +++ b/refs/refs-internal.h @@ -268,6 +268,12 @@ enum do_for_each_ref_flags { * per-worktree refs. */ DO_FOR_EACH_PER_WORKTREE_ONLY = (1 << 1), + + /* + * Omit dangling symrefs from output; this only has an effect with + * INCLUDE_BROKEN, since they are otherwise not included at all. + */ + DO_FOR_EACH_OMIT_DANGLING_SYMREFS = (1 << 2), }; /* -- cgit v1.2.3 From 34e8a20d763d46d7424f699c6ded2cf7ef66b04e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Tue, 28 Sep 2021 15:02:21 +0200 Subject: refs/ref-cache.[ch]: remove unused remove_entry_from_dir() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This function was missed in 9939b33d6a3 (packed-backend: rip out some now-unused code, 2017-09-08), and has been orphaned since then. Let's delete it. Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Junio C Hamano --- refs/ref-cache.c | 35 ----------------------------------- refs/ref-cache.h | 16 ---------------- 2 files changed, 51 deletions(-) (limited to 'refs') diff --git a/refs/ref-cache.c b/refs/ref-cache.c index 49d732f6db..e0feebf628 100644 --- a/refs/ref-cache.c +++ b/refs/ref-cache.c @@ -212,41 +212,6 @@ struct ref_entry *find_ref_entry(struct ref_dir *dir, const char *refname) return (entry->flag & REF_DIR) ? NULL : entry; } -int remove_entry_from_dir(struct ref_dir *dir, const char *refname) -{ - int refname_len = strlen(refname); - int entry_index; - struct ref_entry *entry; - int is_dir = refname[refname_len - 1] == '/'; - if (is_dir) { - /* - * refname represents a reference directory. Remove - * the trailing slash; otherwise we will get the - * directory *representing* refname rather than the - * one *containing* it. - */ - char *dirname = xmemdupz(refname, refname_len - 1); - dir = find_containing_dir(dir, dirname, 0); - free(dirname); - } else { - dir = find_containing_dir(dir, refname, 0); - } - if (!dir) - return -1; - entry_index = search_ref_dir(dir, refname, refname_len); - if (entry_index == -1) - return -1; - entry = dir->entries[entry_index]; - - MOVE_ARRAY(&dir->entries[entry_index], - &dir->entries[entry_index + 1], dir->nr - entry_index - 1); - dir->nr--; - if (dir->sorted > entry_index) - dir->sorted--; - free_ref_entry(entry); - return dir->nr; -} - int add_ref_entry(struct ref_dir *dir, struct ref_entry *ref) { dir = find_containing_dir(dir, ref->name, 1); diff --git a/refs/ref-cache.h b/refs/ref-cache.h index 3bfb89d2b3..bd1ff578ea 100644 --- a/refs/ref-cache.h +++ b/refs/ref-cache.h @@ -199,22 +199,6 @@ void free_ref_cache(struct ref_cache *cache); */ void add_entry_to_dir(struct ref_dir *dir, struct ref_entry *entry); -/* - * Remove the entry with the given name from dir, recursing into - * subdirectories as necessary. If refname is the name of a directory - * (i.e., ends with '/'), then remove the directory and its contents. - * If the removal was successful, return the number of entries - * remaining in the directory entry that contained the deleted entry. - * If the name was not found, return -1. Please note that this - * function only deletes the entry from the cache; it does not delete - * it from the filesystem or ensure that other cache entries (which - * might be symbolic references to the removed entry) are updated. - * Nor does it remove any containing dir entries that might be made - * empty by the removal. dir must represent the top-level directory - * and must already be complete. - */ -int remove_entry_from_dir(struct ref_dir *dir, const char *refname); - /* * Add a ref_entry to the ref_dir (unsorted), recursing into * subdirectories as necessary. dir must represent the top-level -- cgit v1.2.3 From 6a99fa2e9eaeb3347f9d129e1231d3e15353105d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Tue, 28 Sep 2021 15:02:22 +0200 Subject: refs/ref-cache.[ch]: remove unused add_ref_entry() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This function has not been used since 9dd389f3d8d (packed_ref_store: get rid of the `ref_cache` entirely, 2017-09-25). Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Junio C Hamano --- refs/ref-cache.c | 9 --------- refs/ref-cache.h | 7 ------- 2 files changed, 16 deletions(-) (limited to 'refs') diff --git a/refs/ref-cache.c b/refs/ref-cache.c index e0feebf628..a28883768c 100644 --- a/refs/ref-cache.c +++ b/refs/ref-cache.c @@ -212,15 +212,6 @@ struct ref_entry *find_ref_entry(struct ref_dir *dir, const char *refname) return (entry->flag & REF_DIR) ? NULL : entry; } -int add_ref_entry(struct ref_dir *dir, struct ref_entry *ref) -{ - dir = find_containing_dir(dir, ref->name, 1); - if (!dir) - return -1; - add_entry_to_dir(dir, ref); - return 0; -} - /* * Emit a warning and return true iff ref1 and ref2 have the same name * and the same oid. Die if they have the same name but different diff --git a/refs/ref-cache.h b/refs/ref-cache.h index bd1ff578ea..580d4038f6 100644 --- a/refs/ref-cache.h +++ b/refs/ref-cache.h @@ -199,13 +199,6 @@ void free_ref_cache(struct ref_cache *cache); */ void add_entry_to_dir(struct ref_dir *dir, struct ref_entry *entry); -/* - * Add a ref_entry to the ref_dir (unsorted), recursing into - * subdirectories as necessary. dir must represent the top-level - * directory. Return 0 on success. - */ -int add_ref_entry(struct ref_dir *dir, struct ref_entry *ref); - /* * Find the value entry with the given name in dir, sorting ref_dirs * and recursing into subdirectories as necessary. If the name is not -- cgit v1.2.3 From 5e4546d599abdd1f3bbff0eaef77fe9a0cfcc5c3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Tue, 28 Sep 2021 15:02:23 +0200 Subject: refs/ref-cache.c: remove "mkdir" parameter from find_containing_dir() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Remove the "mkdir" parameter from the find_containing_dir() function, the add_ref_entry() function removed in the preceding commit was its last user. Since "mkdir" is always "0" we can also remove the parameter from search_for_subdir(), which in turn means that we can delete most of that function. Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Junio C Hamano --- refs/ref-cache.c | 36 ++++++++++++------------------------ 1 file changed, 12 insertions(+), 24 deletions(-) (limited to 'refs') diff --git a/refs/ref-cache.c b/refs/ref-cache.c index a28883768c..73b338f5ff 100644 --- a/refs/ref-cache.c +++ b/refs/ref-cache.c @@ -144,30 +144,19 @@ int search_ref_dir(struct ref_dir *dir, const char *refname, size_t len) /* * Search for a directory entry directly within dir (without * recursing). Sort dir if necessary. subdirname must be a directory - * name (i.e., end in '/'). If mkdir is set, then create the - * directory if it is missing; otherwise, return NULL if the desired + * name (i.e., end in '/'). Returns NULL if the desired * directory cannot be found. dir must already be complete. */ static struct ref_dir *search_for_subdir(struct ref_dir *dir, - const char *subdirname, size_t len, - int mkdir) + const char *subdirname, size_t len) { int entry_index = search_ref_dir(dir, subdirname, len); struct ref_entry *entry; - if (entry_index == -1) { - if (!mkdir) - return NULL; - /* - * Since dir is complete, the absence of a subdir - * means that the subdir really doesn't exist; - * therefore, create an empty record for it but mark - * the record complete. - */ - entry = create_dir_entry(dir->cache, subdirname, len, 0); - add_entry_to_dir(dir, entry); - } else { - entry = dir->entries[entry_index]; - } + + if (entry_index == -1) + return NULL; + + entry = dir->entries[entry_index]; return get_ref_dir(entry); } @@ -176,18 +165,17 @@ static struct ref_dir *search_for_subdir(struct ref_dir *dir, * tree that should hold refname. If refname is a directory name * (i.e., it ends in '/'), then return that ref_dir itself. dir must * represent the top-level directory and must already be complete. - * Sort ref_dirs and recurse into subdirectories as necessary. If - * mkdir is set, then create any missing directories; otherwise, + * Sort ref_dirs and recurse into subdirectories as necessary. Will * return NULL if the desired directory cannot be found. */ static struct ref_dir *find_containing_dir(struct ref_dir *dir, - const char *refname, int mkdir) + const char *refname) { const char *slash; for (slash = strchr(refname, '/'); slash; slash = strchr(slash + 1, '/')) { size_t dirnamelen = slash - refname + 1; struct ref_dir *subdir; - subdir = search_for_subdir(dir, refname, dirnamelen, mkdir); + subdir = search_for_subdir(dir, refname, dirnamelen); if (!subdir) { dir = NULL; break; @@ -202,7 +190,7 @@ struct ref_entry *find_ref_entry(struct ref_dir *dir, const char *refname) { int entry_index; struct ref_entry *entry; - dir = find_containing_dir(dir, refname, 0); + dir = find_containing_dir(dir, refname); if (!dir) return NULL; entry_index = search_ref_dir(dir, refname, strlen(refname)); @@ -478,7 +466,7 @@ struct ref_iterator *cache_ref_iterator_begin(struct ref_cache *cache, dir = get_ref_dir(cache->root); if (prefix && *prefix) - dir = find_containing_dir(dir, prefix, 0); + dir = find_containing_dir(dir, prefix); if (!dir) /* There's nothing to iterate over. */ return empty_ref_iterator_begin(); -- cgit v1.2.3 From 750036c8f71368125b70f907fd369d5316571d01 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Tue, 28 Sep 2021 15:02:24 +0200 Subject: refs/ref-cache.[ch]: remove "incomplete" from create_dir_entry() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Remove the now-unused "incomplete" parameter from create_dir_entry(), all its callers specify it as "1", so let's drop the "incomplete=0" case. The last caller to use it was search_for_subdir(), but that code was removed in the preceding commit. Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Junio C Hamano --- refs/files-backend.c | 6 +++--- refs/ref-cache.c | 7 +++---- refs/ref-cache.h | 3 +-- 3 files changed, 7 insertions(+), 9 deletions(-) (limited to 'refs') diff --git a/refs/files-backend.c b/refs/files-backend.c index 74c0385873..8fa328108a 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -227,7 +227,7 @@ static void add_per_worktree_entries_to_dir(struct ref_dir *dir, const char *dir pos = search_ref_dir(dir, prefix, prefix_len); if (pos >= 0) continue; - child_entry = create_dir_entry(dir->cache, prefix, prefix_len, 1); + child_entry = create_dir_entry(dir->cache, prefix, prefix_len); add_entry_to_dir(dir, child_entry); } } @@ -278,7 +278,7 @@ static void loose_fill_ref_dir(struct ref_store *ref_store, strbuf_addch(&refname, '/'); add_entry_to_dir(dir, create_dir_entry(dir->cache, refname.buf, - refname.len, 1)); + refname.len)); } else { if (!refs_resolve_ref_unsafe(&refs->base, refname.buf, @@ -336,7 +336,7 @@ static struct ref_cache *get_loose_ref_cache(struct files_ref_store *refs) * lazily): */ add_entry_to_dir(get_ref_dir(refs->loose->root), - create_dir_entry(refs->loose, "refs/", 5, 1)); + create_dir_entry(refs->loose, "refs/", 5)); } return refs->loose; } diff --git a/refs/ref-cache.c b/refs/ref-cache.c index 73b338f5ff..a5ad8a39fb 100644 --- a/refs/ref-cache.c +++ b/refs/ref-cache.c @@ -49,7 +49,7 @@ struct ref_cache *create_ref_cache(struct ref_store *refs, ret->ref_store = refs; ret->fill_ref_dir = fill_ref_dir; - ret->root = create_dir_entry(ret, "", 0, 1); + ret->root = create_dir_entry(ret, "", 0); return ret; } @@ -86,14 +86,13 @@ static void clear_ref_dir(struct ref_dir *dir) } struct ref_entry *create_dir_entry(struct ref_cache *cache, - const char *dirname, size_t len, - int incomplete) + const char *dirname, size_t len) { struct ref_entry *direntry; FLEX_ALLOC_MEM(direntry, name, dirname, len); direntry->u.subdir.cache = cache; - direntry->flag = REF_DIR | (incomplete ? REF_INCOMPLETE : 0); + direntry->flag = REF_DIR | REF_INCOMPLETE; return direntry; } diff --git a/refs/ref-cache.h b/refs/ref-cache.h index 580d4038f6..5c042ae718 100644 --- a/refs/ref-cache.h +++ b/refs/ref-cache.h @@ -169,8 +169,7 @@ struct ref_dir *get_ref_dir(struct ref_entry *entry); * "refs/heads/") or "" for the top-level directory. */ struct ref_entry *create_dir_entry(struct ref_cache *cache, - const char *dirname, size_t len, - int incomplete); + const char *dirname, size_t len); struct ref_entry *create_ref_entry(const char *refname, const struct object_id *oid, int flag); -- cgit v1.2.3 From 34224e14d6b50cb04430188332362e6a0327e5ed Mon Sep 17 00:00:00 2001 From: Jonathan Tan Date: Fri, 8 Oct 2021 14:08:14 -0700 Subject: refs: plumb repo into ref stores In preparation for the next 2 patches that adds (partial) support for arbitrary repositories to ref iterators, plumb a repository into all ref stores. There are no changes to program logic. Signed-off-by: Jonathan Tan Signed-off-by: Junio C Hamano --- refs/files-backend.c | 6 ++++-- refs/packed-backend.c | 4 +++- refs/packed-backend.h | 4 +++- refs/refs-internal.h | 10 ++++++++-- 4 files changed, 18 insertions(+), 6 deletions(-) (limited to 'refs') diff --git a/refs/files-backend.c b/refs/files-backend.c index 1148c0cf09..6a481e968f 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -79,13 +79,15 @@ static void clear_loose_ref_cache(struct files_ref_store *refs) * Create a new submodule ref cache and add it to the internal * set of caches. */ -static struct ref_store *files_ref_store_create(const char *gitdir, +static struct ref_store *files_ref_store_create(struct repository *repo, + const char *gitdir, unsigned int flags) { struct files_ref_store *refs = xcalloc(1, sizeof(*refs)); struct ref_store *ref_store = (struct ref_store *)refs; struct strbuf sb = STRBUF_INIT; + ref_store->repo = repo; ref_store->gitdir = xstrdup(gitdir); base_ref_store_init(ref_store, &refs_be_files); refs->store_flags = flags; @@ -93,7 +95,7 @@ static struct ref_store *files_ref_store_create(const char *gitdir, get_common_dir_noenv(&sb, gitdir); refs->gitcommondir = strbuf_detach(&sb, NULL); strbuf_addf(&sb, "%s/packed-refs", refs->gitcommondir); - refs->packed_ref_store = packed_ref_store_create(sb.buf, flags); + refs->packed_ref_store = packed_ref_store_create(repo, sb.buf, flags); strbuf_release(&sb); chdir_notify_reparent("files-backend $GIT_DIR", &refs->base.gitdir); diff --git a/refs/packed-backend.c b/refs/packed-backend.c index f8aa97d799..ea3493b24e 100644 --- a/refs/packed-backend.c +++ b/refs/packed-backend.c @@ -193,13 +193,15 @@ static int release_snapshot(struct snapshot *snapshot) } } -struct ref_store *packed_ref_store_create(const char *path, +struct ref_store *packed_ref_store_create(struct repository *repo, + const char *path, unsigned int store_flags) { struct packed_ref_store *refs = xcalloc(1, sizeof(*refs)); struct ref_store *ref_store = (struct ref_store *)refs; base_ref_store_init(ref_store, &refs_be_packed); + ref_store->repo = repo; ref_store->gitdir = xstrdup(path); refs->store_flags = store_flags; diff --git a/refs/packed-backend.h b/refs/packed-backend.h index a01a0aff9c..f61a73ec25 100644 --- a/refs/packed-backend.h +++ b/refs/packed-backend.h @@ -1,6 +1,7 @@ #ifndef REFS_PACKED_BACKEND_H #define REFS_PACKED_BACKEND_H +struct repository; struct ref_transaction; /* @@ -12,7 +13,8 @@ struct ref_transaction; * even among packed refs. */ -struct ref_store *packed_ref_store_create(const char *path, +struct ref_store *packed_ref_store_create(struct repository *repo, + const char *path, unsigned int store_flags); /* diff --git a/refs/refs-internal.h b/refs/refs-internal.h index 96911fb26e..d28440c9cc 100644 --- a/refs/refs-internal.h +++ b/refs/refs-internal.h @@ -539,7 +539,8 @@ struct ref_store; * should call base_ref_store_init() to initialize the shared part of * the ref_store and to record the ref_store for later lookup. */ -typedef struct ref_store *ref_store_init_fn(const char *gitdir, +typedef struct ref_store *ref_store_init_fn(struct repository *repo, + const char *gitdir, unsigned int flags); typedef int ref_init_db_fn(struct ref_store *refs, struct strbuf *err); @@ -697,7 +698,12 @@ struct ref_store { /* The backend describing this ref_store's storage scheme: */ const struct ref_storage_be *be; - /* The gitdir that this ref_store applies to: */ + struct repository *repo; + + /* + * The gitdir that this ref_store applies to. Note that this is not + * necessarily repo->gitdir if the repo has multiple worktrees. + */ char *gitdir; }; -- cgit v1.2.3 From 9bc45a28026eaea42011e8c1884aa2d9dc30f947 Mon Sep 17 00:00:00 2001 From: Jonathan Tan Date: Fri, 8 Oct 2021 14:08:15 -0700 Subject: refs: teach arbitrary repo support to iterators Note that should_pack_ref() is called when writing refs, which is only supported for the_repository, hence the_repository is hardcoded there. Signed-off-by: Jonathan Tan Signed-off-by: Junio C Hamano --- refs/files-backend.c | 5 ++++- refs/packed-backend.c | 6 ++++-- refs/refs-internal.h | 1 + 3 files changed, 9 insertions(+), 3 deletions(-) (limited to 'refs') diff --git a/refs/files-backend.c b/refs/files-backend.c index 6a481e968f..3f213d24b0 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -732,6 +732,7 @@ struct files_ref_iterator { struct ref_iterator base; struct ref_iterator *iter0; + struct repository *repo; unsigned int flags; }; @@ -753,6 +754,7 @@ static int files_ref_iterator_advance(struct ref_iterator *ref_iterator) if (!(iter->flags & DO_FOR_EACH_INCLUDE_BROKEN) && !ref_resolves_to_object(iter->iter0->refname, + iter->repo, iter->iter0->oid, iter->iter0->flags)) continue; @@ -855,6 +857,7 @@ static struct ref_iterator *files_ref_iterator_begin( base_ref_iterator_init(ref_iterator, &files_ref_iterator_vtable, overlay_iter->ordered); iter->iter0 = overlay_iter; + iter->repo = ref_store->repo; iter->flags = flags; return ref_iterator; @@ -1139,7 +1142,7 @@ static int should_pack_ref(const char *refname, return 0; /* Do not pack broken refs: */ - if (!ref_resolves_to_object(refname, oid, ref_flags)) + if (!ref_resolves_to_object(refname, the_repository, oid, ref_flags)) return 0; return 1; diff --git a/refs/packed-backend.c b/refs/packed-backend.c index ea3493b24e..63f78bbaea 100644 --- a/refs/packed-backend.c +++ b/refs/packed-backend.c @@ -778,6 +778,7 @@ struct packed_ref_iterator { struct object_id oid, peeled; struct strbuf refname_buf; + struct repository *repo; unsigned int flags; }; @@ -866,8 +867,8 @@ static int packed_ref_iterator_advance(struct ref_iterator *ref_iterator) continue; if (!(iter->flags & DO_FOR_EACH_INCLUDE_BROKEN) && - !ref_resolves_to_object(iter->base.refname, &iter->oid, - iter->flags)) + !ref_resolves_to_object(iter->base.refname, iter->repo, + &iter->oid, iter->flags)) continue; return ITER_OK; @@ -956,6 +957,7 @@ static struct ref_iterator *packed_ref_iterator_begin( iter->base.oid = &iter->oid; + iter->repo = ref_store->repo; iter->flags = flags; if (prefix && *prefix) diff --git a/refs/refs-internal.h b/refs/refs-internal.h index d28440c9cc..500d77864d 100644 --- a/refs/refs-internal.h +++ b/refs/refs-internal.h @@ -66,6 +66,7 @@ int refname_is_safe(const char *refname); * referred-to object does not exist, emit a warning and return false. */ int ref_resolves_to_object(const char *refname, + struct repository *repo, const struct object_id *oid, unsigned int flags); -- cgit v1.2.3 From 8788195c8846be949e6cd4bd19c8dc5e6371ffd3 Mon Sep 17 00:00:00 2001 From: Jonathan Tan Date: Fri, 8 Oct 2021 14:08:16 -0700 Subject: refs: peeling non-the_repository iterators is BUG There is currently no support for peeling the current ref of an iterator iterating over a non-the_repository ref store, and none is needed. Thus, for now, BUG() if that happens. Signed-off-by: Jonathan Tan Signed-off-by: Junio C Hamano --- refs/files-backend.c | 5 +++-- refs/packed-backend.c | 3 +++ refs/ref-cache.c | 10 ++++++++++ refs/ref-cache.h | 1 + 4 files changed, 17 insertions(+), 2 deletions(-) (limited to 'refs') diff --git a/refs/files-backend.c b/refs/files-backend.c index 3f213d24b0..8ee6ac2103 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -833,7 +833,7 @@ static struct ref_iterator *files_ref_iterator_begin( */ loose_iter = cache_ref_iterator_begin(get_loose_ref_cache(refs), - prefix, 1); + prefix, ref_store->repo, 1); /* * The packed-refs file might contain broken references, for @@ -1165,7 +1165,8 @@ static int files_pack_refs(struct ref_store *ref_store, unsigned int flags) packed_refs_lock(refs->packed_ref_store, LOCK_DIE_ON_ERROR, &err); - iter = cache_ref_iterator_begin(get_loose_ref_cache(refs), NULL, 0); + iter = cache_ref_iterator_begin(get_loose_ref_cache(refs), NULL, + the_repository, 0); while ((ok = ref_iterator_advance(iter)) == ITER_OK) { /* * If the loose reference can be packed, add an entry diff --git a/refs/packed-backend.c b/refs/packed-backend.c index 63f78bbaea..2161218719 100644 --- a/refs/packed-backend.c +++ b/refs/packed-backend.c @@ -886,6 +886,9 @@ static int packed_ref_iterator_peel(struct ref_iterator *ref_iterator, struct packed_ref_iterator *iter = (struct packed_ref_iterator *)ref_iterator; + if (iter->repo != the_repository) + BUG("peeling for non-the_repository is not supported"); + if ((iter->base.flags & REF_KNOWS_PEELED)) { oidcpy(peeled, &iter->peeled); return is_null_oid(&iter->peeled) ? -1 : 0; diff --git a/refs/ref-cache.c b/refs/ref-cache.c index 49d732f6db..97a6ac349e 100644 --- a/refs/ref-cache.c +++ b/refs/ref-cache.c @@ -435,6 +435,8 @@ struct cache_ref_iterator { * on from there.) */ struct cache_ref_iterator_level *levels; + + struct repository *repo; }; static int cache_ref_iterator_advance(struct ref_iterator *ref_iterator) @@ -491,6 +493,11 @@ static int cache_ref_iterator_advance(struct ref_iterator *ref_iterator) static int cache_ref_iterator_peel(struct ref_iterator *ref_iterator, struct object_id *peeled) { + struct cache_ref_iterator *iter = + (struct cache_ref_iterator *)ref_iterator; + + if (iter->repo != the_repository) + BUG("peeling for non-the_repository is not supported"); return peel_object(ref_iterator->oid, peeled) ? -1 : 0; } @@ -513,6 +520,7 @@ static struct ref_iterator_vtable cache_ref_iterator_vtable = { struct ref_iterator *cache_ref_iterator_begin(struct ref_cache *cache, const char *prefix, + struct repository *repo, int prime_dir) { struct ref_dir *dir; @@ -547,5 +555,7 @@ struct ref_iterator *cache_ref_iterator_begin(struct ref_cache *cache, level->prefix_state = PREFIX_CONTAINS_DIR; } + iter->repo = repo; + return ref_iterator; } diff --git a/refs/ref-cache.h b/refs/ref-cache.h index 3bfb89d2b3..7877bf86ed 100644 --- a/refs/ref-cache.h +++ b/refs/ref-cache.h @@ -238,6 +238,7 @@ struct ref_entry *find_ref_entry(struct ref_dir *dir, const char *refname); */ struct ref_iterator *cache_ref_iterator_begin(struct ref_cache *cache, const char *prefix, + struct repository *repo, int prime_dir); #endif /* REFS_REF_CACHE_H */ -- cgit v1.2.3