pseudo-merge: fix disk reads from find_pseudo_merge()

The goal of this commit was to fix a const warning when compiling
with new versions of glibc, but ended up untangling a much deeper
problem.

The find_pseudo_merge() function does a bsearch() on the "commits"
pointer of a pseudo_merge_map. This pointer ultimately comes from memory
mapped from the on-disk bitmap file, and is thus not writable.

The "commits" array is correctly marked const, but the result from
bsearch() is returned directly as a non-const pseudo_merge_commit
struct. Since new versions of glibc annotate bsearch() in a way that
detects the implicit loss of const, the compiler now warns.

My first instinct was that we should be returning a const struct. That
requires apply_pseudo_merges_for_commit() to mark its local pointer as
const. But that doesn't work! If the offset field has the high-bit set,
we look it up in the extended table via nth_pseudo_merge_ext(). And that
function then feeds our const struct to read_pseudo_merge_commit_at(),
which writes into it by byte-swapping from the on-disk mmap.

But I think this points to a larger problem with find_pseudo_merge(). It
is not just that the return value is missing const, but it is missing
that byte-swapping! And we know that byte-swapping is needed here,
because the comparator we use for bsearch() also calls our
read_pseudo_merge_commit_at() helper.

So I think the interface is all wrong here. We should not be returning a
pointer to a struct which was cast from on-disk data. We should be
filling in a caller-provided struct using the bytes we found,
byte-swapping the values.

That of course raises the dual question: how did this ever work, and
does it work now? The answer to the first part is: this code does not
seem to be triggered in the test suite at all. If we insert a BUG("foo")
call into apply_pseudo_merges_for_commit(), it never triggers.

So I think there is something wrong or missing from the test setup, and
this bears further investigation. Sadly the answer to the second part
("does it work now") is still "no idea". I _think_ this takes us in a
positive direction, but my goal here is mainly to quiet the compiler
warning. Further bug-hunting on this experimental feature can be done
separately.

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/pseudo-merge.c b/pseudo-merge.c
index a2d5bd85f95ebf269a3887698110d2af08258462..ff18b6c364245ea90a4ab48e676a1213bdfac7f0 100644 (file)
--- a/pseudo-merge.c
+++ b/pseudo-merge.c
@@ -638,14 +638,21 @@ static int pseudo_merge_commit_cmp(const void *va, const void *vb)
        return 0;
 }
 
-static struct pseudo_merge_commit *find_pseudo_merge(const struct pseudo_merge_map *pm,
-                                                    uint32_t pos)
+static int find_pseudo_merge(const struct pseudo_merge_map *pm, uint32_t pos,
+                            struct pseudo_merge_commit *out)
 {
+       const unsigned char *at;
+
        if (!pm->commits_nr)
-               return NULL;
+               return 0;
 
-       return bsearch(&pos, pm->commits, pm->commits_nr,
-                      PSEUDO_MERGE_COMMIT_RAWSZ, pseudo_merge_commit_cmp);
+       at = bsearch(&pos, pm->commits, pm->commits_nr,
+                    PSEUDO_MERGE_COMMIT_RAWSZ, pseudo_merge_commit_cmp);
+       if (!at)
+               return 0;
+
+       read_pseudo_merge_commit_at(out, at);
+       return 1;
 }
 
 int apply_pseudo_merges_for_commit(const struct pseudo_merge_map *pm,
@@ -653,16 +660,15 @@ int apply_pseudo_merges_for_commit(const struct pseudo_merge_map *pm,
                                   struct commit *commit, uint32_t commit_pos)
 {
        struct pseudo_merge *merge;
-       struct pseudo_merge_commit *merge_commit;
+       struct pseudo_merge_commit merge_commit;
        int ret = 0;
 
-       merge_commit = find_pseudo_merge(pm, commit_pos);
-       if (!merge_commit)
+       if (!find_pseudo_merge(pm, commit_pos, &merge_commit))
                return 0;
 
-       if (merge_commit->pseudo_merge_ofs & ((uint64_t)1<<63)) {
+       if (merge_commit.pseudo_merge_ofs & ((uint64_t)1<<63)) {
                struct pseudo_merge_commit_ext ext = { 0 };
-               off_t ofs = merge_commit->pseudo_merge_ofs & ~((uint64_t)1<<63);
+               off_t ofs = merge_commit.pseudo_merge_ofs & ~((uint64_t)1<<63);
                uint32_t i;
 
                if (pseudo_merge_ext_at(pm, &ext, ofs) < -1) {
@@ -673,11 +679,11 @@ int apply_pseudo_merges_for_commit(const struct pseudo_merge_map *pm,
                }
 
                for (i = 0; i < ext.nr; i++) {
-                       if (nth_pseudo_merge_ext(pm, &ext, merge_commit, i) < 0)
+                       if (nth_pseudo_merge_ext(pm, &ext, &merge_commit, i) < 0)
                                return ret;
 
                        merge = pseudo_merge_at(pm, &commit->object.oid,
-                                               merge_commit->pseudo_merge_ofs);
+                                               merge_commit.pseudo_merge_ofs);
 
                        if (!merge)
                                return ret;
@@ -687,7 +693,7 @@ int apply_pseudo_merges_for_commit(const struct pseudo_merge_map *pm,
                }
        } else {
                merge = pseudo_merge_at(pm, &commit->object.oid,
-                                       merge_commit->pseudo_merge_ofs);
+                                       merge_commit.pseudo_merge_ofs);
 
                if (!merge)
                        return ret;