From: Karthik Nayak Date: Thu, 30 Oct 2025 10:34:46 GMT Subject: Re: [PATCH 01/13] odb: fix subtle logic to check whether an alternate is usable Message-ID: In-Reply-To: <20251024-b4-pks-odb-loose-backend-v1-1-1a4202273c38@pks.im> Patrick Steinhardt writes: > When adding an alternate to the object database we first check whether > or not the path is usable. A path is usable if: > > - It actually exists. > > - We don't have it in our object sources yet. > > While the former check is trivial enough, the latter part is somewhat > subtle and prone for bugs. This is because the function doesn't only > check whether or not the given path is usable. But if it _is_ usable, we > also store that path in the map of object sources immediately. > > The tricky part here is that the path that gets stored in the map is > _not_ copied. Instead, we rely on the fact that subsequent code uses > `strbuf_detach()` to store the exact same allocated memory in the > created object source. Consequently, the memory is owned by the source > but _also_ stored in the map. This subtlety is easy to miss, so if one > decides to refactor this code one can easily end up breaking this > mechanism. > > Make the relationship more explicit by not storing the path as part of > `alt_odb_usable()`. Instead, we store the path after we have created the > source now so that we can use the source's path pointer directly. > Nit: The last sentence would read a little better with s/we// and s/now// > Signed-off-by: Patrick Steinhardt > --- > odb.c | 30 +++++++++++++++++------------- > 1 file changed, 17 insertions(+), 13 deletions(-) > > diff --git a/odb.c b/odb.c > index 00a6e71568b..57d85ed9505 100644 > --- a/odb.c > +++ b/odb.c > @@ -86,17 +86,16 @@ int odb_mkstemp(struct object_database *odb, > /* > * Return non-zero iff the path is usable as an alternate object database. > */ > -static int alt_odb_usable(struct object_database *o, > - struct strbuf *path, > - const char *normalized_objdir, khiter_t *pos) > +static int alt_odb_usable(struct object_database *o, const char *path, > + const char *normalized_objdir) > { > int r; > > /* Detect cases where alternate disappeared */ > - if (!is_directory(path->buf)) { > + if (!is_directory(path)) { > error(_("object directory %s does not exist; " > "check .git/objects/info/alternates"), > - path->buf); > + path); > return 0; > } > > @@ -113,11 +112,14 @@ static int alt_odb_usable(struct object_database *o, > assert(r == 1); /* never used */ > kh_value(o->source_by_path, p) = o->sources; > } > - if (fspatheq(path->buf, normalized_objdir)) > + > + if (fspatheq(path, normalized_objdir)) > + return 0; > + > + if (kh_get_odb_path_map(o->source_by_path, path) < kh_end(o->source_by_path)) > return 0; > - *pos = kh_put_odb_path_map(o->source_by_path, path->buf, &r); > - /* r: 0 = exists, 1 = never used, 2 = deleted */ > - return r == 0 ? 0 : 1; > + > + return 1; > } > Okay as part of `alt_obd_usable()` we no longer add the path to the list of alternates. > /* > @@ -148,6 +150,7 @@ static struct odb_source *link_alt_odb_entry(struct object_database *odb, > struct strbuf pathbuf = STRBUF_INIT; > struct strbuf tmp = STRBUF_INIT; > khiter_t pos; > + int ret; > > if (!is_absolute_path(dir) && relative_base) { > strbuf_realpath(&pathbuf, relative_base, 1); > @@ -172,20 +175,21 @@ static struct odb_source *link_alt_odb_entry(struct object_database *odb, > strbuf_reset(&tmp); > strbuf_realpath(&tmp, odb->sources->path, 1); > > - if (!alt_odb_usable(odb, &pathbuf, tmp.buf, &pos)) > + if (!alt_odb_usable(odb, pathbuf.buf, tmp.buf)) > goto error; > > CALLOC_ARRAY(alternate, 1); > alternate->odb = odb; > alternate->local = false; > - /* pathbuf.buf is already in r->objects->source_by_path */ > alternate->path = strbuf_detach(&pathbuf, NULL); > > /* add the alternate entry */ > *odb->sources_tail = alternate; > odb->sources_tail = &(alternate->next); > - alternate->next = NULL; > - assert(odb->source_by_path); > + > + pos = kh_put_odb_path_map(odb->source_by_path, alternate->path, &ret); > + if (!ret) > + BUG("source must not yet exist"); > kh_value(odb->source_by_path, pos) = alternate; > > /* recursively add alternates */ > Instead we now add it as part of `link_alt_obd_entry()`. This makes a lot more sense. > -- > 2.51.1.930.gacf6e81ea2.dirty