From patchwork Wed Aug 25 23:23:30 2021 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jonathan Tan X-Patchwork-Id: 12458669 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-26.3 required=3.0 tests=BAYES_00,DKIMWL_WL_MED, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_CR_TRAILER,INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS, USER_AGENT_GIT,USER_IN_DEF_DKIM_WL autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 01039C432BE for ; Wed, 25 Aug 2021 23:23:42 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id C9A0961075 for ; Wed, 25 Aug 2021 23:23:41 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S233551AbhHYXY1 (ORCPT ); Wed, 25 Aug 2021 19:24:27 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:51352 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S230176AbhHYXY0 (ORCPT ); Wed, 25 Aug 2021 19:24:26 -0400 Received: from mail-yb1-xb49.google.com (mail-yb1-xb49.google.com [IPv6:2607:f8b0:4864:20::b49]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 7DB4BC061757 for ; Wed, 25 Aug 2021 16:23:40 -0700 (PDT) Received: by mail-yb1-xb49.google.com with SMTP id m66-20020a257145000000b00598282d96ceso1170502ybc.3 for ; Wed, 25 Aug 2021 16:23:40 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20161025; h=date:in-reply-to:message-id:mime-version:references:subject:from:to :cc; bh=T9YyoqLGWbUJgYIEunSJjS5cBhbcRuH237N8fMXxEhE=; b=fQtkgNlNqqADZNfWbfjIzaEOgU+M9wuNEI6ktEyZvny0pzjgEoPwFsNLhgnbhNvd6O ZObliQB7HlEMxLiuKohFpgXD+sU9vZkaNR2xd3INle3LgvkVOUeMw0XHOARdZ30KTw8Q dFM0SHRfame7r9PChm8gCIvRlag1fWAnl9CD0590HUpG6ZbyoneNozwiMhXvvTfTuGPK mOpQ8Yp37b7o1EcZBsXchnUaRauvj8kdQmfMvZJxc2JY3ANfjBWKLYzw82dqnEueafgj S6PouLGvD0GdlcqFNw6RIykhzFRh3hTP0Glzc+/3/UezaQBOsDWBJmTRYgTEKmjSTIJI y0uw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:in-reply-to:message-id:mime-version :references:subject:from:to:cc; bh=T9YyoqLGWbUJgYIEunSJjS5cBhbcRuH237N8fMXxEhE=; b=gBMFq3+S4HsosuWCGGhxwSAtWVLkXzMYyAvS6NfFcsZoH9E/4uo0i/lcPUvMCiOcOJ zfr6GHmJBiWUtctpuWVjnVGBxGjRWAYtkBGcyC+IwT9xRh6DIBuW0KIu2S1DBOZj4wMa fvJgq66KRjEEz7un4GpxhZZTcG0/szRnjP8qj370uG6HGqKFX/dEAyJLWfX4mjBeszwx TWIcNz7MkaKaayI57Xz3zNV78gZN8G7gjdKVAETb3fYMfo7OeRLQPGP2uR1LXAMqtGuA t4cGDyiG/zqzOZDjgFhC6SjikmjQ1M9aKhAvWR32LLIdbxSEcYmrP2PVNV1Kkp9s1s+f URPw== X-Gm-Message-State: AOAM532WDUwI50HvwfGpc7k5DxqMdEjiqo9Kd5WdB7TWfcVDPyBbHMae q2SxrSbqrKCPZ8IGvUsO4KRKGf5ogy2zjmgvSwqkoL/CWIBT5A4fTrZ9m5z++Ysr5IAA5Y9bt1F lSKHJDW7mmLQFnct97lUloAL/u4ESfi3rjcitBiLa2gaQe/4bXcYW2+70xsy5iCRajD6vzlO5cm BR X-Google-Smtp-Source: ABdhPJxMRMUuAzfveCGUjU1UUlfsjrAftIAbnN0QwZ8anrMUggvogXGP9qHtZ6vsXxc72mbBSqS/v36LBpTDGMsstFt3 X-Received: from twelve4.c.googlers.com ([fda3:e722:ac3:cc00:24:72f4:c0a8:437a]) (user=jonathantanmy job=sendgmr) by 2002:a25:42c2:: with SMTP id p185mr1566207yba.0.1629933819640; Wed, 25 Aug 2021 16:23:39 -0700 (PDT) Date: Wed, 25 Aug 2021 16:23:30 -0700 In-Reply-To: Message-Id: <9187eab52552630863285ef5743a107ccc555495.1629933380.git.jonathantanmy@google.com> Mime-Version: 1.0 References: X-Mailer: git-send-email 2.33.0.rc2.250.ged5fa647cd-goog Subject: [RFC PATCH 1/2] refs: make _advance() check struct repo, not flag From: Jonathan Tan To: git@vger.kernel.org Cc: Jonathan Tan , hanwen@google.com Precedence: bulk List-ID: X-Mailing-List: git@vger.kernel.org Currently, ref iterators access the object store each time they advance if and only if the boolean flag DO_FOR_EACH_INCLUDE_BROKEN is unset. (The iterators access the object store because, if DO_FOR_EACH_INCLUDE_BROKEN is unset, they need to attempt to resolve each ref to determine that it is not broken.) Also, the object store accessed is always that of the_repository, making it impossible to iterate over a submodule's refs without DO_FOR_EACH_INCLUDE_BROKEN (unless add_submodule_odb() is used). As a first step in resolving both these problems, replace the DO_FOR_EACH_INCLUDE_BROKEN flag with a struct repository pointer. This commit is a mechanical conversion - whenever DO_FOR_EACH_INCLUDE_BROKEN is set, a NULL repository (representing access to no object store) is used instead, and whenever DO_FOR_EACH_INCLUDE_BROKEN is unset, a non-NULL repository (representing access to that repository's object store) is used instead. Right now, the locations in which non-the_repository support needs to be added are marked with BUG() statements - in a future patch, these will be replaced. (NEEDSWORK: in this RFC patch set, this has not been done) I have considered and rejected the following design alternatives: - Making all ref stores not access the object store during their _advance() callbacks, and making ref_iterator_advance() be responsible for checking the object store - thus, simplifying the code in that the logic of checking for the flag (current) or the pointer (after the equivalent of this commit) is only in one place instead of in every ref store's callback. However, the ref stores already make use of this flag for another reason - for determining if refs are resolvable when writing (search for "REF_STORE_ODB"). Thus, I decided to retain each ref store's knowledge of this flag. - Teaching the ref iterator mechanism to never skip any ref. This has the same problem as above, and furthermore, all callers now need to handle unresolvable refs. - Change the _advance() callback to also have a repository object parameter, and either skip or not skip depending on whether that parameter is NULL. This burdens callers to have to carry this information along with the iterator, and such calling code may be unclear as to why that parameter can be NULL in some cases and cannot in others. Signed-off-by: Jonathan Tan --- refs.c | 48 +++++++++++++++++++++++-------------------- refs/debug.c | 4 ++-- refs/files-backend.c | 19 +++++++++++++---- refs/packed-backend.c | 16 +++++++++++---- refs/refs-internal.h | 24 ++++++++++++---------- 5 files changed, 68 insertions(+), 43 deletions(-) diff --git a/refs.c b/refs.c index 8b9f7c3a80..35b85f3e79 100644 --- a/refs.c +++ b/refs.c @@ -1413,16 +1413,16 @@ int head_ref(each_ref_fn fn, void *cb_data) struct ref_iterator *refs_ref_iterator_begin( struct ref_store *refs, - const char *prefix, int trim, int flags) + const char *prefix, int trim, struct repository *repo, + int flags) { struct ref_iterator *iter; if (ref_paranoia < 0) ref_paranoia = git_env_bool("GIT_REF_PARANOIA", 0); - if (ref_paranoia) - flags |= DO_FOR_EACH_INCLUDE_BROKEN; - iter = refs->be->iterator_begin(refs, prefix, flags); + iter = refs->be->iterator_begin(refs, prefix, + ref_paranoia ? NULL : repo, flags); /* * `iterator_begin()` already takes care of prefix, but we @@ -1442,13 +1442,16 @@ struct ref_iterator *refs_ref_iterator_begin( * Call fn for each reference in the specified submodule for which the * refname begins with prefix. If trim is non-zero, then trim that * many characters off the beginning of each refname before passing - * the refname to fn. flags can be DO_FOR_EACH_INCLUDE_BROKEN to - * include broken references in the iteration. If fn ever returns a + * the refname to fn. If fn ever returns a * non-zero value, stop the iteration and return that value; * otherwise, return 0. + * + * See the documentation of refs_ref_iterator_begin() for more information on + * the repo parameter. */ static int do_for_each_repo_ref(struct repository *r, const char *prefix, - each_repo_ref_fn fn, int trim, int flags, + each_repo_ref_fn fn, int trim, + struct repository *repo, int flags, void *cb_data) { struct ref_iterator *iter; @@ -1457,7 +1460,7 @@ static int do_for_each_repo_ref(struct repository *r, const char *prefix, if (!refs) return 0; - iter = refs_ref_iterator_begin(refs, prefix, trim, flags); + iter = refs_ref_iterator_begin(refs, prefix, trim, repo, flags); return do_for_each_repo_ref_iterator(r, iter, fn, cb_data); } @@ -1479,7 +1482,8 @@ static int do_for_each_ref_helper(struct repository *r, } static int do_for_each_ref(struct ref_store *refs, const char *prefix, - each_ref_fn fn, int trim, int flags, void *cb_data) + each_ref_fn fn, int trim, struct repository *repo, + int flags, void *cb_data) { struct ref_iterator *iter; struct do_for_each_ref_help hp = { fn, cb_data }; @@ -1487,7 +1491,7 @@ static int do_for_each_ref(struct ref_store *refs, const char *prefix, if (!refs) return 0; - iter = refs_ref_iterator_begin(refs, prefix, trim, flags); + iter = refs_ref_iterator_begin(refs, prefix, trim, repo, flags); return do_for_each_repo_ref_iterator(the_repository, iter, do_for_each_ref_helper, &hp); @@ -1495,7 +1499,7 @@ static int do_for_each_ref(struct ref_store *refs, const char *prefix, int refs_for_each_ref(struct ref_store *refs, each_ref_fn fn, void *cb_data) { - return do_for_each_ref(refs, "", fn, 0, 0, cb_data); + return do_for_each_ref(refs, "", fn, 0, the_repository, 0, cb_data); } int for_each_ref(each_ref_fn fn, void *cb_data) @@ -1506,7 +1510,7 @@ int for_each_ref(each_ref_fn fn, void *cb_data) int refs_for_each_ref_in(struct ref_store *refs, const char *prefix, each_ref_fn fn, void *cb_data) { - return do_for_each_ref(refs, prefix, fn, strlen(prefix), 0, cb_data); + return do_for_each_ref(refs, prefix, fn, strlen(prefix), the_repository, 0, cb_data); } int for_each_ref_in(const char *prefix, each_ref_fn fn, void *cb_data) @@ -1518,10 +1522,10 @@ int for_each_fullref_in(const char *prefix, each_ref_fn fn, void *cb_data, unsig { unsigned int flag = 0; - if (broken) - flag = DO_FOR_EACH_INCLUDE_BROKEN; return do_for_each_ref(get_main_ref_store(the_repository), - prefix, fn, 0, flag, cb_data); + prefix, fn, 0, + broken ? NULL : the_repository, + flag, cb_data); } int refs_for_each_fullref_in(struct ref_store *refs, const char *prefix, @@ -1530,16 +1534,16 @@ int refs_for_each_fullref_in(struct ref_store *refs, const char *prefix, { unsigned int flag = 0; - if (broken) - flag = DO_FOR_EACH_INCLUDE_BROKEN; - return do_for_each_ref(refs, prefix, fn, 0, flag, cb_data); + return do_for_each_ref(refs, prefix, fn, 0, + broken ? NULL : the_repository, + flag, cb_data); } int for_each_replace_ref(struct repository *r, each_repo_ref_fn fn, void *cb_data) { return do_for_each_repo_ref(r, git_replace_ref_base, fn, strlen(git_replace_ref_base), - DO_FOR_EACH_INCLUDE_BROKEN, cb_data); + NULL, 0, cb_data); } int for_each_namespaced_ref(each_ref_fn fn, void *cb_data) @@ -1548,7 +1552,7 @@ int for_each_namespaced_ref(each_ref_fn fn, void *cb_data) int ret; strbuf_addf(&buf, "%srefs/", get_git_namespace()); ret = do_for_each_ref(get_main_ref_store(the_repository), - buf.buf, fn, 0, 0, cb_data); + buf.buf, fn, 0, the_repository, 0, cb_data); strbuf_release(&buf); return ret; } @@ -1556,7 +1560,7 @@ int for_each_namespaced_ref(each_ref_fn fn, void *cb_data) int refs_for_each_rawref(struct ref_store *refs, each_ref_fn fn, void *cb_data) { return do_for_each_ref(refs, "", fn, 0, - DO_FOR_EACH_INCLUDE_BROKEN, cb_data); + NULL, 0, cb_data); } int for_each_rawref(each_ref_fn fn, void *cb_data) @@ -2263,7 +2267,7 @@ int refs_verify_refname_available(struct ref_store *refs, strbuf_addch(&dirname, '/'); iter = refs_ref_iterator_begin(refs, dirname.buf, 0, - DO_FOR_EACH_INCLUDE_BROKEN); + NULL, 0); while ((ok = ref_iterator_advance(iter)) == ITER_OK) { if (skip && string_list_has_string(skip, iter->refname)) diff --git a/refs/debug.c b/refs/debug.c index 1a7a9e11cf..753d5da893 100644 --- a/refs/debug.c +++ b/refs/debug.c @@ -224,11 +224,11 @@ static struct ref_iterator_vtable debug_ref_iterator_vtable = { static struct ref_iterator * debug_ref_iterator_begin(struct ref_store *ref_store, const char *prefix, - unsigned int flags) + struct repository *repo, unsigned int flags) { struct debug_ref_store *drefs = (struct debug_ref_store *)ref_store; struct ref_iterator *res = - drefs->refs->be->iterator_begin(drefs->refs, prefix, flags); + drefs->refs->be->iterator_begin(drefs->refs, prefix, repo, flags); struct debug_ref_iterator *diter = xcalloc(1, sizeof(*diter)); base_ref_iterator_init(&diter->base, &debug_ref_iterator_vtable, 1); diter->iter = res; diff --git a/refs/files-backend.c b/refs/files-backend.c index 677b7e4cdd..4c42db1092 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -730,6 +730,7 @@ struct files_ref_iterator { struct ref_iterator base; struct ref_iterator *iter0; + struct repository *repo; unsigned int flags; }; @@ -744,7 +745,13 @@ 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_INCLUDE_BROKEN) && + if (iter->repo && iter->repo != the_repository) + /* + * NEEDSWORK: make ref_resolves_to_object() support + * arbitrary repositories + */ + BUG("iter->repo must be NULL or the_repository"); + if (iter->repo && !ref_resolves_to_object(iter->iter0->refname, iter->iter0->oid, iter->iter0->flags)) @@ -793,7 +800,7 @@ static struct ref_iterator_vtable files_ref_iterator_vtable = { static struct ref_iterator *files_ref_iterator_begin( struct ref_store *ref_store, - const char *prefix, unsigned int flags) + const char *prefix, struct repository *repo, unsigned int flags) { struct files_ref_store *refs; struct ref_iterator *loose_iter, *packed_iter, *overlay_iter; @@ -801,7 +808,7 @@ static struct ref_iterator *files_ref_iterator_begin( struct ref_iterator *ref_iterator; unsigned int required_flags = REF_STORE_READ; - if (!(flags & DO_FOR_EACH_INCLUDE_BROKEN)) + if (repo) required_flags |= REF_STORE_ODB; refs = files_downcast(ref_store, required_flags, "ref_iterator_begin"); @@ -836,10 +843,13 @@ static struct ref_iterator *files_ref_iterator_begin( * references, and (if needed) do our own check for broken * ones in files_ref_iterator_advance(), after we have merged * the packed and loose references. + * + * Do this by not supplying any repo, regardless of whether a repo was + * supplied to files_ref_iterator_begin(). */ packed_iter = refs_ref_iterator_begin( refs->packed_ref_store, prefix, 0, - DO_FOR_EACH_INCLUDE_BROKEN); + NULL, 0); overlay_iter = overlay_ref_iterator_begin(loose_iter, packed_iter); @@ -848,6 +858,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 = repo; iter->flags = flags; return ref_iterator; diff --git a/refs/packed-backend.c b/refs/packed-backend.c index f8aa97d799..bc2302a6e0 100644 --- a/refs/packed-backend.c +++ b/refs/packed-backend.c @@ -776,6 +776,7 @@ struct packed_ref_iterator { struct object_id oid, peeled; struct strbuf refname_buf; + struct repository *repo; unsigned int flags; }; @@ -863,7 +864,13 @@ static int packed_ref_iterator_advance(struct ref_iterator *ref_iterator) ref_type(iter->base.refname) != REF_TYPE_PER_WORKTREE) continue; - if (!(iter->flags & DO_FOR_EACH_INCLUDE_BROKEN) && + if (iter->repo && iter->repo != the_repository) + /* + * NEEDSWORK: make ref_resolves_to_object() support + * arbitrary repositories + */ + BUG("iter->repo must be NULL or the_repository"); + if (iter->repo && !ref_resolves_to_object(iter->base.refname, &iter->oid, iter->flags)) continue; @@ -913,7 +920,7 @@ static struct ref_iterator_vtable packed_ref_iterator_vtable = { static struct ref_iterator *packed_ref_iterator_begin( struct ref_store *ref_store, - const char *prefix, unsigned int flags) + const char *prefix, struct repository *repo, unsigned int flags) { struct packed_ref_store *refs; struct snapshot *snapshot; @@ -922,7 +929,7 @@ static struct ref_iterator *packed_ref_iterator_begin( struct ref_iterator *ref_iterator; unsigned int required_flags = REF_STORE_READ; - if (!(flags & DO_FOR_EACH_INCLUDE_BROKEN)) + if (repo) required_flags |= REF_STORE_ODB; refs = packed_downcast(ref_store, required_flags, "ref_iterator_begin"); @@ -954,6 +961,7 @@ static struct ref_iterator *packed_ref_iterator_begin( iter->base.oid = &iter->oid; + iter->repo = repo; iter->flags = flags; if (prefix && *prefix) @@ -1137,7 +1145,7 @@ static int write_with_updates(struct packed_ref_store *refs, * of updates is exhausted, leave i set to updates->nr. */ iter = packed_ref_iterator_begin(&refs->base, "", - DO_FOR_EACH_INCLUDE_BROKEN); + NULL, 0); if ((ok = ref_iterator_advance(iter)) != ITER_OK) iter = NULL; diff --git a/refs/refs-internal.h b/refs/refs-internal.h index 3155708345..b20fa1f5cd 100644 --- a/refs/refs-internal.h +++ b/refs/refs-internal.h @@ -245,9 +245,6 @@ 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 - /* * Reference iterators * @@ -349,16 +346,19 @@ 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: + * The output is ordered by refname. + * + * Pass NULL as repo to include broken references in the iteration, or non-NULL + * to skip references that do not resolve to an object in the given repo. * - * DO_FOR_EACH_INCLUDE_BROKEN: include broken references in - * the iteration. + * The following flags are supported: * * DO_FOR_EACH_PER_WORKTREE_ONLY: only produce REF_TYPE_PER_WORKTREE refs. */ struct ref_iterator *refs_ref_iterator_begin( struct ref_store *refs, - const char *prefix, int trim, int flags); + const char *prefix, int trim, struct repository *repo, + int flags); /* * A callback function used to instruct merge_ref_iterator how to @@ -446,8 +446,9 @@ 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 + * to provide the OID referent. If a NULL repo was passed to the _begin() + * function that created this iterator, 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. */ @@ -504,7 +505,7 @@ int do_for_each_repo_ref_iterator(struct repository *r, * where all reference backends will presumably store their * per-worktree refs. */ -#define DO_FOR_EACH_PER_WORKTREE_ONLY 0x02 +#define DO_FOR_EACH_PER_WORKTREE_ONLY 0x01 struct ref_store; @@ -569,7 +570,8 @@ typedef int copy_ref_fn(struct ref_store *ref_store, */ typedef struct ref_iterator *ref_iterator_begin_fn( struct ref_store *ref_store, - const char *prefix, unsigned int flags); + const char *prefix, struct repository *repo, + unsigned int flags); /* reflog functions */ From patchwork Wed Aug 25 23:23:31 2021 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jonathan Tan X-Patchwork-Id: 12458671 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-26.3 required=3.0 tests=BAYES_00,DKIMWL_WL_MED, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_CR_TRAILER,INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS, USER_AGENT_GIT,USER_IN_DEF_DKIM_WL autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 0A94DC432BE for ; Wed, 25 Aug 2021 23:23:46 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id D800A610C7 for ; Wed, 25 Aug 2021 23:23:45 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S233583AbhHYXYb (ORCPT ); Wed, 25 Aug 2021 19:24:31 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:51362 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S233577AbhHYXY2 (ORCPT ); Wed, 25 Aug 2021 19:24:28 -0400 Received: from mail-qv1-xf4a.google.com (mail-qv1-xf4a.google.com [IPv6:2607:f8b0:4864:20::f4a]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 23637C061757 for ; Wed, 25 Aug 2021 16:23:42 -0700 (PDT) Received: by mail-qv1-xf4a.google.com with SMTP id n14-20020a0c9d4e0000b0290354a5f8c800so1082787qvf.17 for ; Wed, 25 Aug 2021 16:23:42 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20161025; h=date:in-reply-to:message-id:mime-version:references:subject:from:to :cc; bh=PBZtc4qU2+YG4WwZoeOjaFOGr7BOZZc2AQZ28FoVr9U=; b=Ka+ZSDmkH+7wD8Hyogeyd4u0B9F8r5RHgVqbQSdtfMTsOTJ5kIMZm4WIHeRdq6q/bQ GNnl5o+E33Z5A+ivfLSKAYLMhclXrTArMZ0fz7WWovhmMfLcNpmMN6ajXwjKLtCaBF1r jNlVN5RFYVQ4qFLSz/YCpAU/7bpnNf2hJJYjQRfcxDPkINNgi5dAjcPTPHF+KRFajRUU SvsgvwZE8cuMxYcw5pH56V3rTZv7JS/isqBaeIt2s6WxDUnpI18XQVip4ECup1mdE1TR AyweHjfRqv34jFLgmfdZKiSnZJfSATDDWPudfCCMAyu88F9VtDnmNuVCuNPjcsCmT9jI 3fSA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:in-reply-to:message-id:mime-version :references:subject:from:to:cc; bh=PBZtc4qU2+YG4WwZoeOjaFOGr7BOZZc2AQZ28FoVr9U=; b=XqFEQCJgZR1reTuIzffMvjS9xnH1hQku0Hcw9gIHW3yHQfQuQhQgOx1xvJfCsBmkUV VD+n1L5tk8mLopFVBVD5/rlDp9nJlSlpJA4DrMycfKJXQk/IZEae9SPVQIDfW77C6X4P O54TSDpHSDGwmJi/rpAKA+OLZoSgD5g6tHXdzAeySKd+IiExaFaEK2ysntDYp3Bg5Aps 2EJAtUtJ8hQsJYHo5l8qnu58Mf/yNQnd+wo7WTY2I7phfQnbuG3L4UBFGJh6NYetORsF fmRw0WOGndoLHi56tQ8/6DVEDCsTff527ceR7Tve8klFWLg0uV2AqVU7lShVMvKoMyy/ ZVWg== X-Gm-Message-State: AOAM532MHLAFlYBz+HMq/edgNSBt6AubaMAIhrOWK6SzjvMJi/wbtKfp fBAbwjf6878OxxBBu1kX4j61zUG53EZJEpi9furqwNDW/fcMt2MuYODkt0lvxIJ6K32N8X+JFif lMThVgL7dSBcitQqYX7EDt0NJGUGqpL/nLmkjpSFKeXuqrWCIiSlESy4xASUuwvsZQcsItNkdVm 5n X-Google-Smtp-Source: ABdhPJzyCRu9g2TP9rTZZdUBjtEI6M4i6R1f1s5UDy0C/05ttPWB/itZGLhuAnGJJy28vInXSi/rLRcUD9cSTaguZv+X X-Received: from twelve4.c.googlers.com ([fda3:e722:ac3:cc00:24:72f4:c0a8:437a]) (user=jonathantanmy job=sendgmr) by 2002:ad4:55eb:: with SMTP id bu11mr1190749qvb.57.1629933821190; Wed, 25 Aug 2021 16:23:41 -0700 (PDT) Date: Wed, 25 Aug 2021 16:23:31 -0700 In-Reply-To: Message-Id: <2e93cc60e959d18909405a1d22c650228bf2fcb9.1629933380.git.jonathantanmy@google.com> Mime-Version: 1.0 References: X-Mailer: git-send-email 2.33.0.rc2.250.ged5fa647cd-goog Subject: [RFC PATCH 2/2] refs: add repo paramater to _iterator_peel() From: Jonathan Tan To: git@vger.kernel.org Cc: Jonathan Tan , hanwen@google.com Precedence: bulk List-ID: X-Mailing-List: git@vger.kernel.org Split the ref_iterator_peel() function into two functions: one that returns information solely based on what the ref store contains (success, failure, inconclusive), and one that takes a repo parameter and accesses the object store if need be. Update the ref store's callbacks to not access the object store, and to return success/failure/inconclusive instead of a binary success/failure. This makes it explicit whether a peel attempt may access the object store of a repository. The approach taken in this commit for peeling is different from the approach taken in the parent commit for advancing: - It is complicated to reuse the repo field (which determines if an object store is ever accessed during advancing, and if yes, which object store) added to ref stores in the parent commit; the files ref store wraps the packed ref store, and it does not want the packed ref store to access any object store during advancing (as described in files_ref_iterator_begin()) - thus repo is NULL - but it wants packed ref store peeling. - Having the repo handy when peeling is not as cumbersome as it is when advancing. Firstly, the repo in this case is always non-NULL, and secondly, peeling is typically followed by reading the object, which requires the repo anyway. Signed-off-by: Jonathan Tan --- refs.c | 2 +- refs/debug.c | 9 +++++---- refs/files-backend.c | 12 +++++++----- refs/iterator.c | 38 +++++++++++++++++++++++++++++--------- refs/packed-backend.c | 14 ++++++++------ refs/ref-cache.c | 7 ++++--- refs/refs-internal.h | 27 ++++++++++++++++++++++----- 7 files changed, 76 insertions(+), 33 deletions(-) diff --git a/refs.c b/refs.c index 35b85f3e79..7bc23bcc3f 100644 --- a/refs.c +++ b/refs.c @@ -2012,7 +2012,7 @@ int peel_iterated_oid(const struct object_id *base, struct object_id *peeled) if (current_ref_iter && (current_ref_iter->oid == base || oideq(current_ref_iter->oid, base))) - return ref_iterator_peel(current_ref_iter, peeled); + return ref_iterator_peel(current_ref_iter, the_repository, peeled); return peel_object(base, peeled) ? -1 : 0; } diff --git a/refs/debug.c b/refs/debug.c index 753d5da893..7291a1a4fb 100644 --- a/refs/debug.c +++ b/refs/debug.c @@ -198,13 +198,14 @@ static int debug_ref_iterator_advance(struct ref_iterator *ref_iterator) return res; } -static int debug_ref_iterator_peel(struct ref_iterator *ref_iterator, - struct object_id *peeled) +static enum ref_iterator_peel_result debug_ref_iterator_peel( + struct ref_iterator *ref_iterator, + struct object_id *peeled) { struct debug_ref_iterator *diter = (struct debug_ref_iterator *)ref_iterator; - int res = diter->iter->vtable->peel(diter->iter, peeled); - trace_printf_key(&trace_refs, "iterator_peel: %s: %d\n", diter->iter->refname, res); + enum ref_iterator_peel_result res = diter->iter->vtable->peel(diter->iter, peeled); + trace_printf_key(&trace_refs, "iterator_peel: %s: %d\n", diter->iter->refname, (int) res); return res; } diff --git a/refs/files-backend.c b/refs/files-backend.c index 4c42db1092..e1930848b1 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -770,13 +770,14 @@ static int files_ref_iterator_advance(struct ref_iterator *ref_iterator) return ok; } -static int files_ref_iterator_peel(struct ref_iterator *ref_iterator, - struct object_id *peeled) +static enum ref_iterator_peel_result files_ref_iterator_peel( + struct ref_iterator *ref_iterator, + struct object_id *peeled) { struct files_ref_iterator *iter = (struct files_ref_iterator *)ref_iterator; - return ref_iterator_peel(iter->iter0, peeled); + return ref_iterator_peel_raw(iter->iter0, peeled); } static int files_ref_iterator_abort(struct ref_iterator *ref_iterator) @@ -2122,8 +2123,9 @@ static int files_reflog_iterator_advance(struct ref_iterator *ref_iterator) return ok; } -static int files_reflog_iterator_peel(struct ref_iterator *ref_iterator, - struct object_id *peeled) +static enum ref_iterator_peel_result files_reflog_iterator_peel( + struct ref_iterator *ref_iterator, + struct object_id *peeled) { BUG("ref_iterator_peel() called for reflog_iterator"); } diff --git a/refs/iterator.c b/refs/iterator.c index a89d132d4f..386e6310ed 100644 --- a/refs/iterator.c +++ b/refs/iterator.c @@ -13,10 +13,27 @@ int ref_iterator_advance(struct ref_iterator *ref_iterator) return ref_iterator->vtable->advance(ref_iterator); } +enum ref_iterator_peel_result ref_iterator_peel_raw( + struct ref_iterator *ref_iterator, + struct object_id *peeled) +{ + return ref_iterator->vtable->peel(ref_iterator, peeled); +} + int ref_iterator_peel(struct ref_iterator *ref_iterator, + struct repository *repo, struct object_id *peeled) { - return ref_iterator->vtable->peel(ref_iterator, peeled); + enum ref_iterator_peel_result result = + ref_iterator_peel_raw(ref_iterator, peeled); + + if (repo != the_repository) + /* NEEDSWORK: make peel_object() work with all repositories */ + BUG("ref_iterator_peel() can only be used with the_repository"); + if (result == REF_ITERATOR_PEEL_INCONCLUSIVE) + return peel_object(ref_iterator->oid, peeled) == PEEL_PEELED ? + 0 : -1; + return result == REF_ITERATOR_PEEL_SUCCESS ? 0 : -1; } int ref_iterator_abort(struct ref_iterator *ref_iterator) @@ -51,8 +68,9 @@ static int empty_ref_iterator_advance(struct ref_iterator *ref_iterator) return ref_iterator_abort(ref_iterator); } -static int empty_ref_iterator_peel(struct ref_iterator *ref_iterator, - struct object_id *peeled) +static enum ref_iterator_peel_result empty_ref_iterator_peel( + struct ref_iterator *ref_iterator, + struct object_id *peeled) { BUG("peel called for empty iterator"); } @@ -170,8 +188,9 @@ static int merge_ref_iterator_advance(struct ref_iterator *ref_iterator) return ITER_ERROR; } -static int merge_ref_iterator_peel(struct ref_iterator *ref_iterator, - struct object_id *peeled) +static enum ref_iterator_peel_result merge_ref_iterator_peel( + struct ref_iterator *ref_iterator, + struct object_id *peeled) { struct merge_ref_iterator *iter = (struct merge_ref_iterator *)ref_iterator; @@ -179,7 +198,7 @@ static int merge_ref_iterator_peel(struct ref_iterator *ref_iterator, if (!iter->current) { BUG("peel called before advance for merge iterator"); } - return ref_iterator_peel(*iter->current, peeled); + return ref_iterator_peel_raw(*iter->current, peeled); } static int merge_ref_iterator_abort(struct ref_iterator *ref_iterator) @@ -355,13 +374,14 @@ static int prefix_ref_iterator_advance(struct ref_iterator *ref_iterator) return ok; } -static int prefix_ref_iterator_peel(struct ref_iterator *ref_iterator, - struct object_id *peeled) +static enum ref_iterator_peel_result prefix_ref_iterator_peel( + struct ref_iterator *ref_iterator, + struct object_id *peeled) { struct prefix_ref_iterator *iter = (struct prefix_ref_iterator *)ref_iterator; - return ref_iterator_peel(iter->iter0, peeled); + return ref_iterator_peel_raw(iter->iter0, peeled); } static int prefix_ref_iterator_abort(struct ref_iterator *ref_iterator) diff --git a/refs/packed-backend.c b/refs/packed-backend.c index bc2302a6e0..ef14390043 100644 --- a/refs/packed-backend.c +++ b/refs/packed-backend.c @@ -884,19 +884,21 @@ static int packed_ref_iterator_advance(struct ref_iterator *ref_iterator) return ok; } -static int packed_ref_iterator_peel(struct ref_iterator *ref_iterator, - struct object_id *peeled) +static enum ref_iterator_peel_result packed_ref_iterator_peel( + struct ref_iterator *ref_iterator, + struct object_id *peeled) { struct packed_ref_iterator *iter = (struct packed_ref_iterator *)ref_iterator; if ((iter->base.flags & REF_KNOWS_PEELED)) { oidcpy(peeled, &iter->peeled); - return is_null_oid(&iter->peeled) ? -1 : 0; + return is_null_oid(&iter->peeled) ? + REF_ITERATOR_PEEL_FAILURE : REF_ITERATOR_PEEL_SUCCESS; } else if ((iter->base.flags & (REF_ISBROKEN | REF_ISSYMREF))) { - return -1; + return REF_ITERATOR_PEEL_FAILURE; } else { - return peel_object(&iter->oid, peeled) ? -1 : 0; + return REF_ITERATOR_PEEL_INCONCLUSIVE; } } @@ -1226,7 +1228,7 @@ static int write_with_updates(struct packed_ref_store *refs, /* Pass the old reference through. */ struct object_id peeled; - int peel_error = ref_iterator_peel(iter, &peeled); + int peel_error = ref_iterator_peel(iter, the_repository, &peeled); if (write_packed_entry(out, iter->refname, iter->oid, diff --git a/refs/ref-cache.c b/refs/ref-cache.c index 49d732f6db..031b613bb2 100644 --- a/refs/ref-cache.c +++ b/refs/ref-cache.c @@ -488,10 +488,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) +static enum ref_iterator_peel_result cache_ref_iterator_peel( + struct ref_iterator *ref_iterator, + struct object_id *peeled) { - return peel_object(ref_iterator->oid, peeled) ? -1 : 0; + return REF_ITERATOR_PEEL_INCONCLUSIVE; } static int cache_ref_iterator_abort(struct ref_iterator *ref_iterator) diff --git a/refs/refs-internal.h b/refs/refs-internal.h index b20fa1f5cd..aea8b0844f 100644 --- a/refs/refs-internal.h +++ b/refs/refs-internal.h @@ -317,11 +317,26 @@ struct ref_iterator { */ int ref_iterator_advance(struct ref_iterator *ref_iterator); +enum ref_iterator_peel_result { + REF_ITERATOR_PEEL_SUCCESS, + REF_ITERATOR_PEEL_FAILURE, + REF_ITERATOR_PEEL_INCONCLUSIVE +}; + +/* + * Peel the reference currently being viewed by the iterator without + * using any information from any object store. + */ +enum ref_iterator_peel_result ref_iterator_peel_raw( + struct ref_iterator *ref_iterator, + struct object_id *peeled); + /* - * If possible, peel the reference currently being viewed by the - * iterator. Return 0 on success. + * Peel the reference currently being viewed by the iterator, using the object + * store if the ref store has insufficient information. Returns 0 upon success. */ int ref_iterator_peel(struct ref_iterator *ref_iterator, + struct repository *repo, struct object_id *peeled); /* @@ -455,10 +470,12 @@ void base_ref_iterator_free(struct ref_iterator *iter); typedef int ref_iterator_advance_fn(struct ref_iterator *ref_iterator); /* - * Peels the current ref, returning 0 for success or -1 for failure. + * Peels the current ref using only information from the ref store. If there is + * not enough information, returns REF_ITERATOR_PEEL_INCONCLUSIVE. */ -typedef int ref_iterator_peel_fn(struct ref_iterator *ref_iterator, - struct object_id *peeled); +typedef enum ref_iterator_peel_result ref_iterator_peel_fn( + struct ref_iterator *ref_iterator, + struct object_id *peeled); /* * Implementations of this function should free any resources specific