| Message ID | ec153eff7b0d5ca3188ec6f43bc40c38609f6a80.1632859148.git.jonathantanmy@google.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | No more adding submodule ODB as alternate | expand |
Jonathan Tan <jonathantanmy@google.com> writes: > 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 <jonathantanmy@google.com> > --- > refs.c | 3 ++- > refs/files-backend.c | 5 ++++- > refs/packed-backend.c | 6 ++++-- > refs/refs-internal.h | 1 + > 4 files changed, 11 insertions(+), 4 deletions(-) > > diff --git a/refs.c b/refs.c > index 6f7b3447a7..5163e064ae 100644 > --- a/refs.c > +++ b/refs.c > @@ -255,12 +255,13 @@ int refname_is_safe(const char *refname) > * 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) > { > if (flags & REF_ISBROKEN) > return 0; > - if (!has_object_file(oid)) { > + if (!repo_has_object_file(repo, oid)) { > error(_("%s does not point to a valid object!"), refname); > return 0; > } OK. > diff --git a/refs/files-backend.c b/refs/files-backend.c > index f0cbea41c9..4d883d9a89 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; > }; > @@ -776,6 +776,7 @@ struct packed_ref_iterator { > struct object_id oid, peeled; > struct strbuf refname_buf; > > + struct repository *repo; > unsigned int flags; > }; The two steps so far seems to give the necessary information to code paths that want them, so it is not wrong per-se, but this makes me wonder a few things. - There may be multiple ref backends and iterators corresponding to them. Is it reasonable to assume that there are backends that do not need "repo"? Otherwise, shouldn't this be added to the base class "struct ref_iterator base"? - The iterator_begin() and other functions have been taught to take the repository in addition to the ref_store in the previous step, but . Doesn't iterator iterate over a single ref_store? Shouldn't it have a pointer to the ref_store it is iterating over? . Doesn't a ref_store belong to a single repository? Shouldn't it have a pointer to the repository it is part of? If the answers to both are 'yes', then we wouldn't need to add a repository pointer as a new parameter to functions that already took a ref store. In other words, I am wondering if the right pieces of information are stored in the right structure. Thanks.
> The two steps so far seems to give the necessary information to code > paths that want them, so it is not wrong per-se, but this makes me > wonder a few things. > > - There may be multiple ref backends and iterators corresponding to > them. Is it reasonable to assume that there are backends that do > not need "repo"? Otherwise, shouldn't this be added to the base > class "struct ref_iterator base"? All backends need repos, but not all iterators need backends - there is a merge_ref_iterator and a prefix_ref_iterator, for example. > - The iterator_begin() and other functions have been taught to take > the repository in addition to the ref_store in the previous step, > but > > . Doesn't iterator iterate over a single ref_store? Shouldn't it > have a pointer to the ref_store it is iterating over? No - as above, merge_ref_iterator, for example, does not iterate over a ref_store but combines the results of 2 other iterators. > . Doesn't a ref_store belong to a single repository? Shouldn't > it have a pointer to the repository it is part of? > > If the answers to both are 'yes', then we wouldn't need to add a > repository pointer as a new parameter to functions that already > took a ref store. > > In other words, I am wondering if the right pieces of information > are stored in the right structure. > > Thanks. A ref_store does belong to a single repository. The reason why it doesn't have a pointer to that repository is probably because struct ref_store (00eebe351c ("refs: create a base class "ref_store" for files_ref_store", 2016-09-09)) predates struct repository (359efeffc1 ("repository: introduce the repository object", 2017-06-23)). I've been avoiding introducing a pointer to the repository in struct ref_store to avoid unnecessary coupling, but it is looking more and more necessary, as you mention in your reply [1] to another patch about how this would eliminate certain other "user" codepaths needing to know about the repo. I'll take a look at introducing a pointer to the repo in struct ref_store and report back with my findings. [1] https://lore.kernel.org/git/xmqqh7e4iacw.fsf@gitster.g/
diff --git a/refs.c b/refs.c index 6f7b3447a7..5163e064ae 100644 --- a/refs.c +++ b/refs.c @@ -255,12 +255,13 @@ int refname_is_safe(const char *refname) * 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) { if (flags & REF_ISBROKEN) return 0; - if (!has_object_file(oid)) { + if (!repo_has_object_file(repo, oid)) { error(_("%s does not point to a valid object!"), refname); return 0; } diff --git a/refs/files-backend.c b/refs/files-backend.c index f0cbea41c9..4d883d9a89 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; }; @@ -751,6 +752,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; @@ -854,6 +856,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; @@ -1138,7 +1141,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 94fb1042a2..55c8bd3081 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; }; @@ -864,8 +865,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; @@ -954,6 +955,7 @@ static struct ref_iterator *packed_ref_iterator_begin( iter->base.oid = &iter->oid; + iter->repo = repo; iter->flags = flags; if (prefix && *prefix) diff --git a/refs/refs-internal.h b/refs/refs-internal.h index 9440be51da..e7b0a0a658 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);
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 <jonathantanmy@google.com> --- refs.c | 3 ++- refs/files-backend.c | 5 ++++- refs/packed-backend.c | 6 ++++-- refs/refs-internal.h | 1 + 4 files changed, 11 insertions(+), 4 deletions(-)