odb: fix subtle logic to check whether an alternate is usable

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, store the path after we have created the
source so that we can use the source's path pointer directly.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/odb.c b/odb.c
index 00a6e71568b5985c0b344bfebbe92d1e0bff1294..57d85ed9505e3fb2a22beeb663527fe543104f41 100644 (file)
--- 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;
 }
 
 /*
@@ -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 */