Bug report: v2.47.0 cannot fetch version 1 pack indexes

What did you do before the bug happened? (Steps to reproduce your issue)

1. Run git clone https://www.townlong-yak.com/test.git

What did you expect to happen? (Expected behavior)

A local "test" folder to be created with a clone of the remote repository.

What happened instead? (Actual behavior)

The command produces the following output: Cloning into 'test'... error: files '/Users/me/test/.git/objects/pack/tmp_idx_WT81vv' and '/Users/me/test/.git/objects/pack/pack-427331d91391b00844273eeb3879cb479ce2c995.idx' differ in contents fatal: unable to rename temporary '*.idx' file to '/Users/me/test/.git/objects/pack/pack-427331d91391b00844273eeb3879cb479ce2c995.idx' error: Unable to find 6261a9d9f7704c02a5421ff733919ab18793aa7d under https://www.townlong-yak.com/test.git Cannot obtain needed object 6261a9d9f7704c02a5421ff733919ab18793aa7d error: fetch failed.

What's different between what you expected and what actually happened?

The clone fails and no test folder exists after the command completes.

Anything else you want to add:

This scenario works correctly with git v2.46.2 and earlier versions, and began failing with v2.47.0.

Running git-bisect identifies b1b8dfde6929ec9463eca0a858c4adb9786d7c93 as the first bad commit, suggesting that the .idx file downloaded from the remote is now expected to be byte-for-byte identical with a locally-generated version; due to format differences, they are not.

The remote idx is in the original (version 1) format, and git verify-pack seems satisfied with it. Did v2.47.0 intend to block fetching such indices?

[System Info] git version: git version 2.47.0.163.g1226f6d8fa cpu: x86_64 built from commit: 1226f6d8faf60d03cbb3b021c68d48364bf67ac0 sizeof-long: 8 sizeof-size_t: 8 shell-path: /bin/sh feature: fsmonitor--daemon libcurl: 8.7.1 zlib: 1.2.12 uname: Darwin 24.0.0 Darwin Kernel Version 24.0.0: Tue Sep 24 23:36:30 PDT 2024; root:xnu-11215.1.12~1/RELEASE_X86_64 x86_64 compiler info: clang: 16.0.0 (clang-1600.0.26.3) libc info: no libc information available $SHELL (typically, interactive shell): /bin/bash

[Enabled Hooks]

On Sat, Oct 19, 2024 at 7:31 PM fox <fox.gbr@townlong-yak.com> wrote:

I can reproduce this problem.

Cc:'ing the authors of that commit.

On Sat, Oct 19, 2024 at 08:37:08PM -0400, Eric Sunshine wrote:

Thanks for reproducing. This is definitely unintentional breakage, which I see that Peff has already diagnosed and proposed a fix for. Let's read on ...

Thanks, Taylor

On Sun, Oct 20, 2024 at 01:22:34AM +0200, fox wrote:

Interesting case. No, we did not intend to block fetching those indices.

We knew there was a potential problem with dumb http here, but the idea was that if we had a local file "foo.idx" (or "foo.pack"), that we'd never try to download the other side in the first place, preferring ours.

And AFAICT that does hold. But here we have the opposite problem! We download the pack idx from the remote first, so that we know what it claims to have (which tells us whether we want to grab the matching pack). And then we grab the matching pack, but instead of using the .idx we downloaded, we index it again ourselves.

Which is probably a good idea to make sure that we prefer our locally-checked copy versus what the other side has. But obviously there is no guarantee that we'll come up with the same bytes as the other side if there are version or configuration differences.

One option is to just discard the remote version before re-indexing, like this:

On Sat, Oct 19, 2024 at 09:24:55PM -0400, Jeff King wrote:

And here's what that might look like. It stores the temporary index outside of the pack/ directory, and then we don't "finalize" it into place. We have to teach parse_pack to retain the original name, but that's OK. It's used only by the http walker anyway.

On Sat, Oct 19, 2024 at 10:40:22PM -0400, Jeff King wrote:

Makes perfect sense, and the comment above here is much appreciated.

I thought about trying to use some intermediate state of the strbuf here to avoid an extra xstrfmt() call, but couldn't come up with anything I didn't think was awkward.

And here's where we stop calling finalize_object_file(). Good.

Huh. I would have thought we have such a helper function already. I guess we probably do, but that it's also defined statically because it's so easy to write.

In any case, this looks like the right thing to do here. It would be nice to have a corresponding test here, since unlike the other finalize_object_file() changes, this one can be provoked deterministically.

Would you mind submitting this as a bona-fide patch, which I can then pick up and start merging down?

In any event, I appreciate you figuring out what was going on here and outlining a couple of paths forward.

Thanks, Taylor

On Mon, Oct 21, 2024 at 04:33:15PM -0400, Taylor Blau wrote:

I don't think there's any useful intermediate state. The earlier %s is the base url, but here it's our local directory.

We could continue to re-use the scratch strbuf as the existing code did (and which xstrfmt() is doing under the hood). It wasn't really intentional for me to change that, but I went through a lot of attempts to get here (using mks_tempfile(), and so on).

I thought so, too, but couldn't find one. We have pack_bitmap_filename() (and so on for .rev and .midx files) that goes from .pack to those extensions. But here we want to go from .idx to .pack. I think most stuff goes from ".pack" because that's what we store in the packed_git struct.

There's also sha1_pack_index_name(), but that goes from a csum-file hash to a filename.

I grepped around and strip_suffix() seems to be par for the course in similar situations within pack/repack code, so I think it's OK here.

Yeah, the test is easy:

On Tue, Oct 22, 2024 at 01:14:02AM -0400, Jeff King wrote:

It would kind of be nice to have a convenient function like:

    const char *pack_ext_name(const struct packed_git *p,
                              const char *ext);

in our codebase, but that is well outside the scope of this patch ;-).

Beautifully written. I wondered why you didn't use `test_commit -C $server`, but it's because that repository is bare, and so needs `--work-tree=.`, so what you wrote here makes sense.

Sounds all good. Thanks!

Thanks, Taylor

On Tue, Oct 22, 2024 at 01:14:02AM -0400, Jeff King wrote:

This took me a little longer than I expected, mostly due to triggering an (existing) bug related to midx. So here's the series I came up with.

The first patch fixes the midx bug, which otherwise causes spurious test failures in patch 3.

The second patch beefs up the existing tests for the area we're touching.

Patch 3 is the actual regression fix. I apologize in advance for the length of the commit message. ;) I tried to summarize all of the weird dumb-http implications I discovered.

If we wanted to we could stop there for maint, and treat the rest as a separate series. But since it's cleanup with no intended behavior change, it's pretty low-risk.

Patches 4-10 are mostly about cleaning up crufty interfaces that I ran across while working on the earlier patches.

And then the final one is an oddity where I think the code is doing the wrong thing, but it's not possible to actually trigger the bug in practice.

  [01/11]: midx: avoid duplicate packed_git entries
  [02/11]: t5550: count fetches in "previously-fetched .idx" test
  [03/11]: dumb-http: store downloaded pack idx as tempfile
  [04/11]: packfile: drop has_pack_index()
  [05/11]: packfile: drop sha1_pack_name()
  [06/11]: packfile: drop sha1_pack_index_name()
  [07/11]: packfile: warn people away from parse_packed_git()
  [08/11]: http-walker: use object_id instead of bare hash
  [09/11]: packfile: convert find_sha1_pack() to use object_id
  [10/11]: packfile: use object_id in find_pack_entry_one()
  [11/11]: packfile: use oidread() instead of hashcpy() to fill object_id

 builtin/fast-import.c         |  6 ++---
 builtin/pack-objects.c        |  4 +--
 builtin/pack-redundant.c      |  5 +++-
 connected.c                   |  4 +--
 http-push.c                   |  4 +--
 http-walker.c                 | 25 ++++++++---------
 http.c                        | 43 ++++++++++++++++++++---------
 midx.c                        | 22 ++++++++++++---
 pack-bitmap.c                 |  4 +--
 packfile.c                    | 51 ++++++++++++++---------------------
 packfile.h                    | 40 +++++++++++++--------------
 t/helper/test-find-pack.c     |  2 +-
 t/t5200-update-server-info.sh |  8 ++++++
 t/t5550-http-fetch-dumb.sh    | 28 +++++++++++++++++--
 walker.c                      |  4 +--
 walker.h                      |  4 +--
 16 files changed, 153 insertions(+), 101 deletions(-)

When we scan a pack directory to load the idx entries we find into the packed_git list, we skip any of them that are contained in a midx. We then load them later lazily if we actually need to access the corresponding pack, referencing them both from the midx struct and the packed_git list.

The lazy-load in the midx code checks to see if the midx already mentions the pack, but doesn't otherwise check the packed_git list. This makes sense, since we should have added any pack to both lists.

But there's a loophole! If we call close_object_store(), that frees the midx entirely, but _not_ the packed_git structs, which we must keep around for Reasons[1]. If we then try to look up more objects, we'll auto-load the midx again, which won't realize that we've already loaded those packs, and will create duplicate entries in the packed_git list.

This is possibly inefficient, because it means we may open and map the pack redundantly. But it can also lead to weird user-visible behavior. The case I found is in "git repack", which closes and reopens the midx after repacking and then calls update_server_info(). We end up writing the duplicate entries into objects/info/packs.

We could obviously de-dup them while writing that file, but it seems like a violation of more core assumptions that we end up with these duplicate structs at all.

We can avoid the duplicates reasonably efficiently by checking their names in the pack_map hash. This annoyingly does require a little more than a straight hash lookup due to the naming conventions, but it should only happen when we are about to actually open a pack. I don't think one extra malloc will be noticeable there.

[1] I'm not entirely sure of all the details, except that we generally
    assume the packed_git structs never go away. We noted this
    restriction in the comment added by 6f1e9394e2 (object: fix leaking
    packfiles when closing object store, 2024-08-08), but it's somewhat
    vague. At any rate, if you try freeing the structs in
    close_object_store(), you can observe segfaults all over the test
    suite. So it might be fixable, but it's not trivial.

Signed-off-by: Jeff King <peff@peff.net>
---
+cc Stolee here as the original midx author. I can't think of a good
reason we'd want to avoid this dup-detection here.

 midx.c                        | 20 +++++++++++++++++---
 t/t5200-update-server-info.sh |  8 ++++++++
 2 files changed, 25 insertions(+), 3 deletions(-)

On Fri, Oct 25, 2024 at 02:43:40AM -0400, Jeff King wrote:

:-). I swear I have written this exact patch before. I think this was way back in the early days of MIDX bitmaps, and working around this bug was the reason that we don't generate the MIDX/bitmap at the end of a repack in the same process.

I don't recall all of the details at the time (or even if the above rationale was what I was thinking back then or not), but I think the change below here is obviously correct.

Thanks, Taylor

We have a test in t5550 that looks at index fetching over dumb http. It creates two branches, each of which is completely stored in its own pack, then fetches the branches independently. What should (and does) happen is that the first fetch grabs both .idx files and one .pack file, and then the fetch of the second branch re-uses the previously downloaded .idx files (fetching none) and grabs the now-required .pack file.

Since the next few patches will be touching this area of the code, let's beef up the test a little by checking that we're downloading the expected items at each step.

Signed-off-by: Jeff King <peff@peff.net>
---
 t/t5550-http-fetch-dumb.sh | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

This patch fixes a regression in b1b8dfde69 (finalize_object_file(): implement collision check, 2024-09-26) where fetching a v1 pack idx file over the dumb-http protocol would cause the fetch to fail.

The core of the issue is that dumb-http stores the idx we fetch from the remote at the same path that will eventually hold the idx we generate from "index-pack --stdin". The sequence is something like this:

  0. We realize we need some object X, which we don't have locally, and
     nor does the other side have it as a loose object.

  1. We download the list of remote packs from objects/info/packs.

  2. For each entry in that file, we download each pack index and store
     it locally in .git/objects/pack/pack-$hash.idx (the $hash is not
     something we can verify yet and is given to us by the remote).

  3. We check each pack index we got to see if it has object X. When we
     find a match, we download the matching .pack file from the remote
     to a tempfile. We feed that to "index-pack --stdin", which
     reindexes the pack, rather than trusting that it has what the other
     side claims it does. In most cases, this will end up generating the
     exact same (byte-for-byte) pack index which we'll store at the same
     pack-$hash.idx path, because the index generation and $hash id are
     computed based on what's in the packfile. But:

       a. The other side might have used other options to generate the
          index. For instance we use index v2 by default, but long ago
          it was v1 (and you can still ask for v1 explicitly).

       b. The other side might even use a different mechanism to
          determine $hash. E.g., long ago it was based on the sorted
          list of objects in the packfile, but we switched to using the
          pack checksum in 1190a1acf8 (pack-objects: name pack files
          after trailer hash, 2013-12-05).

The regression we saw in the real world was (3a). A recent client fetching from a server with a v1 index downloaded that index, then complained about trying to overwrite it with its own v2 index. This collision is otherwise harmless; we know we want to replace the remote version with our local one, but the collision check doesn't realize that.

There are a few options to fix it:

  - we could teach index-pack a command-line option to ignore only pack
    idx collisions, and use it when the dumb-http code invokes
    index-pack. This would be an awkward thing to expose users to and
    would involve a lot of boilerplate to get the option down to the
    collision code.

  - we could delete the remote .idx file right before running
    index-pack. It should be redundant at that point (since we've just
    downloaded the matching pack). But it feels risky to delete
    something from our own .git/objects based on what the other side has
    said. I'm not entirely positive that a malicious server couldn't lie
    about which pack-$hash.idx it has and get us to delete something
    precious.

  - we can stop co-mingling the downloaded idx files in our local
    objects directory. This is a slightly bigger change but I think
    fixes the root of the problem more directly.

This patch implements the third option. The big design questions are: where do we store the downloaded files, and how do we manage their lifetimes?

There are some additional quirks to the dumb-http system we should consider. Remember that in step 2 we downloaded every pack index, but in step 3 we may only download some of the matching packs. What happens to those other idx files now? They sit in the .git/objects/pack directory, possibly waiting to be used at a later date. That may save bandwidth for a subsequent fetch, but it also creates a lot of weird corner cases:

  - our local object directory now has semi-untrusted .idx files sitting
    around, without their matching .pack

  - in case 3b, we noted that we might not generate the same hash as the
    other side. In that case even if we download the matching pack,
    our index-pack invocation will store it in a different
    pack-$hash.idx file. And the unmatched .idx will sit there forever.

  - if the server repacks, it may delete the old packs. Now we have
    these orphaned .idx files sitting around locally that will never be
    used (nor deleted).

  - if we repack locally we may delete our local version of the server's
    pack index and not realize we have it. So we'll download it again,
    even though we have all of the objects it mentions.

I think the right solution here is probably some more complex cache management system: download the remote .idx files to their own storage directory, mark them as "seen" when we get their matching pack (to avoid re-downloading even if we repack), and then delete them when the server's objects/info/refs no longer mentions them.

But since the dumb http protocol is so ancient and so inferior to the smart http protocol, I don't think it's worth spending a lot of time creating such a system. For this patch I'm just downloading the idx files to .git/objects/tmp_pack_*, and marking them as tempfiles to be deleted when we exit (and due to the name, any we miss due to a crash, etc, should eventually be removed by "git gc" runs based on timestamps).

That is slightly worse for one case: if we download an idx but not the matching pack, we won't retain that idx for subsequent runs. But the flip side is that we're making other cases better (we never hold on to useless idx files forever). I suspect that worse case does not even come up often, since it implies that the packs are generated to match distinct parts of history (i.e., in practice even in a repo with many packs you're going to end up grabbing all of those packs to do a clone). If somebody really cares about that, I think the right path forward is a managed cache directory as above, and this patch is providing the first step in that direction anyway (by moving things out of the objects/pack/ directory).

There are two test changes. One demonstrates the broken v1 index case (it double-checks the resulting clone with fsck to be careful, but prior to this patch it actually fails at the clone step). The other tweaks the expectation for a test that covers the "slightly worse" case to accommodate the extra index download.

The code changes are fairly simple. We stop using finalize_object_file() to copy the remote's index file into place, and leave it as a tempfile. We give the tempfile a real ".idx" name, since the packfile code expects that, and thus we make sure it is out of the usual packs/ directory (so we'd never mistake it for a real local .idx).

We also have to change parse_pack_index(), which creates a temporary packed_git to access our index (we need this because all of the pack idx code assumes we have that struct). It reads the index data from the tempfile, but prior to this patch would speculatively write the finalized name into the packed_git struct using the pack-$hash we expect to use.

I was mildly surprised that this worked at all, since we call verify_pack_index() on the packed_git which mentions the final name before moving the file into place! But it works because parse_pack_index() leaves the mmap-ed data in the struct, so the lazy-open in verify_pack_index() never triggers, and we read from the tempfile, ignoring the filename in the struct completely. Hacky, but it works.

After this patch, parse_pack_index() now uses the index filename we pass in to derive a matching .pack name. This is OK to change because there are only two callers, both in the dumb http code (and the other passes in an existing pack-$hash.idx name, so the derived name is going to be pack-$hash.pack, which is what we were using anyway).

I'll follow up with some more cleanups in that area, but this patch is sufficient to fix the regression.

Reported-by: fox <fox.gbr@townlong-yak.com>
Signed-off-by: Jeff King <peff@peff.net>
---
 http.c                     | 25 ++++++++++++++++++++-----
 packfile.c                 | 11 ++++++++++-
 t/t5550-http-fetch-dumb.sh | 12 +++++++++++-
 3 files changed, 41 insertions(+), 7 deletions(-)

On Fri, Oct 25, 2024 at 02:58:06AM -0400, Jeff King wrote:

Makes sense, and matches our discussion from elsewhere on the list sometime last week.

Yep, the third option is definitely the most sensible here.

OK. I think the more robust version of the solution you identified here makes sense and is probably the right thing to do if we wanted to spend more time and effort on fixing this part of the dumb HTTP protocol.

But I'm willing to believe that there are so few users of the protocol at this point that it's almost certainly not worth the effort. So I'm supportive of the stopping point that you chose here.

I actually think putting it in $GIT_DIR/objects/pack is fine and perhaps preferable, since that's where we put existing in-progress temporary files. We'll ignore anything that doesn't end with "*.idx", so I think it would be fine.

I don't feel strongly about it. It just feels a little weird to stick these temporary files in one place, and stick other (similar) temporary flies in another.

Very nicely done.

Thanks, Taylor

On Fri, Oct 25, 2024 at 05:18:51PM -0400, Taylor Blau wrote:

But the new tempfile _does_ end with .idx. And it (kinda) has to, because that's an assumption made by packed_git: it stores path/to/foo.pack, and then assumes it can access /path/to/foo.idx as needed.

I say "kinda" because as I mentioned elsewhere, the prior code cheats a bit and assumes that we can access the idx file through the original mmap we made, even if the ".idx" the packed_git mentions does not yet exist. I _think_ that's pretty foolproof and that we'd never unmap or close an idx (though we certainly do for packs themselves). But it doesn't feel like a great thing to rely on.

One difference is that these are pure tempfiles. We'll never "finalize" them by renaming them into place, so there's no chance of having to do a cross-directory link/rename. They just get created and then deleted[1].

Perhaps something like "objects/temp-remote-index/*.idx" would be a more descriptive name (and certainly what I'd use if making a more full caching system), but there are complications:

  1. We don't have good cleanup routines for directories. Would that
     just become a semi-permanent empty directory in a dumb-http repo?

  2. git-prune cleans up tmp_* in objects/ and objects/pack, but would
     have to learn to look there, too.

So just sticking them in objects/ seemed like the path of least resistance to me.

-Peff

[1] They really could be opened anywhere, like /tmp. Which I was tempted
    to do, but since they're non-trivial in size it seemed like keeping
    them inside the repo directory was the best bet.

    Curiously, if we lean into the "we will never close a mapped idx",
    then we could create them, open and map them, and then delete them
    immediately, which is a somewhat common tempfile idiom. At least on
    Unix systems. I'm not sure how Windows would like that. :)

On Sat, Oct 26, 2024 at 02:02:38AM -0400, Jeff King wrote:

OK, reading it all back, I tend to agree that sticking them in objects/ makes sense. Thanks!

Thanks, Taylor

The has_pack_index() function has several oddities that may make it surprising if you are trying to find out if we have a pack with some $hash:

  - it is not looking for a valid pack that we found while searching
    object directories. It just looks for any pack-$hash.idx file in the
    pack directory.

  - it only looks in the local directory, not any alternates

  - it takes a bare "unsigned char" hash, which we try to avoid these
    days

The only caller it has is in the dumb http code; it wants to know if we already have the pack idx in question. This can happen if we downloaded the pack (and generated its index) during a previous fetch.

Before the previous patch ("dumb-http: store downloaded pack idx as tempfile"), it could also happen if we downloaded the .idx from the remote but didn't get the matching .pack. But since that patch, we don't hold on to those .idx files. So there's no need to look for the .idx file in the filesystem; we can just scan through the packed_git list to see if we have it.

That lets us simplify the dumb http code a bit, as we know that if we have the .idx we have the matching .pack already. And it lets us get rid of this odd function that is unlikely to be needed again.

Signed-off-by: Jeff King <peff@peff.net>
---
 http.c     | 15 ++++++++-------
 packfile.c |  8 --------
 packfile.h |  2 --
 3 files changed, 8 insertions(+), 17 deletions(-)

On Fri, Oct 25, 2024 at 03:00:09AM -0400, Jeff King wrote:

All makes sense. Good, let's keep reading...

Thanks, Taylor

The sha1_pack_name() function has a few ugly bits:

  - it writes into a static strbuf (and not even a ring buffer of them),
    which can lead to subtle invalidation problems

  - it uses the term "sha1", but it's really using the_hash_algo, which
    could be sha256

There's only one caller of it left. And in fact that caller is better off using the underlying odb_pack_name() function itself, since it's just copying the result into its own strbuf anyway.

Converting that caller lets us get rid of this now-obselete function.

Signed-off-by: Jeff King <peff@peff.net>
---
 http.c     | 3 ++-
 packfile.c | 6 ------
 packfile.h | 7 -------
 3 files changed, 2 insertions(+), 14 deletions(-)

Like sha1_pack_name() that we dropped in the previous commit, this function uses an error-prone static strbuf and the somewhat misleading name "sha1".

The only caller left is in pack-redundant.c. While this command is marked for potential removal in our BreakingChanges document, we still have it for now. But it's simple enough to convert it to use its own strbuf with the underlying odb_pack_name() function, letting us drop the otherwise obsolete function.

Note that odb_pack_name() does its own strbuf_reset(), so it's safe to use directly within a loop like this.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/pack-redundant.c | 5 ++++-
 packfile.c               | 6 ------
 packfile.h               | 7 -------
 3 files changed, 4 insertions(+), 14 deletions(-)

With a name like parse_packed_git(), you might think it's the right way to access a local pack index and its associated objects. But not so! It's a one-off used by the dumb-http code to access pack idx files we've downloaded from the remote, but whose packs we might not have.

There's only one caller left for this function, and ideally we'd drop it completely and just inline it there. But that would require exposing other internals from packfile.[ch], like alloc_packed_git() and check_packed_git_idx().

So let's leave it be for now, and just warn people that it's probably not what they're looking for. Perhaps in the long run if we eventually drop dumb-http support, we can remove the function entirely then.

Signed-off-by: Jeff King <peff@peff.net>
---
 packfile.h | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

On Fri, Oct 25, 2024 at 03:02:22AM -0400, Jeff King wrote:

That seems like a good step and the right place to stop rather than exposing more internals from packfile.c. Nicely done.

Thanks, Taylor

We long ago switched most code to using object_id structs instead of bare "unsigned char *" hashes. This gives us more type safety from the compiler, and generally makes it easier to understand what we expect in each parameter.

But the dumb-http code has lagged behind. And indeed, the whole "walker" subsystem interface has the same problem, though http-walker is the only user left.

So let's update the walker interface to pass object_id structs (which we already have anyway at all call sites!), and likewise use those within the http-walker methods that it calls.

This cleans up the dumb-http code a bit, but will also let us fix a few more commonly used helper functions.

Signed-off-by: Jeff King <peff@peff.net>
---
 http-walker.c | 25 +++++++++++++------------
 walker.c      |  4 ++--
 walker.h      |  4 ++--
 3 files changed, 17 insertions(+), 16 deletions(-)

The find_sha1_pack() function has a few problems:

  - it's badly named, since it works with any object hash

  - it takes the hash as a bare pointer rather than an object_id struct

We can fix both of these easily, as all callers actually have a real object_id anyway.

I also found the existence of this function somewhat confusing, as it is about looking in an arbitrary set of linked packed_git structs. It's good for things like dumb-http which are looking in downloaded remote packs, and not our local packs. But despite the name, it is not a good way to find the pack which contains a local object (it skips the use of the midx, the pack mru list, and so on).

So let's also add an explanatory comment above the declaration that may point people in the right direction.

I suspect the calls in fast-import.c, which use the packed_git list from the repository struct, could actually just be using find_pack_entry(). But since we'd need to keep it anyway for dumb-http, I didn't dig further there. If we eventually drop dumb-http support, then it might be worth examining them to see if we can get rid of the function entirely.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/fast-import.c | 6 ++----
 http-push.c           | 4 ++--
 http-walker.c         | 2 +-
 packfile.c            | 6 +++---
 packfile.h            | 9 +++++++--
 5 files changed, 15 insertions(+), 12 deletions(-)

The main function we use to search a pack index for an object is find_pack_entry_one(). That function still takes a bare pointer to the hash, despite the fact that its underlying bsearch_pack() function needs an object_id struct. And so we end up making an extra copy of the hash into the struct just to do a lookup.

As it turns out, all callers but one already have such an object_id. So we can just take a pointer to that struct and use it directly. This avoids the extra copy and provides a more type-safe interface.

The one exception is get_delta_base() in packfile.c, when we are chasing a REF_DELTA from inside the pack (and thus we have a pointer directly to the mmap'd pack memory, not a struct). We can just bump the hashcpy() from inside find_pack_entry_one() to this one caller that needs it.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/pack-objects.c    |  4 ++--
 connected.c               |  4 ++--
 midx.c                    |  2 +-
 pack-bitmap.c             |  4 ++--
 packfile.c                | 16 ++++++++--------
 packfile.h                |  4 ++--
 t/helper/test-find-pack.c |  2 +-
 7 files changed, 18 insertions(+), 18 deletions(-)

On Fri, Oct 25, 2024 at 03:06:06AM -0400, Jeff King wrote:

Makes sense.

This and all of the similar changes that follow it are trivially correct and pleasing to read.

Here's the one that needed to turn its bare hash into an object_id that it can pass a pointer to. And...

...here's the original location of that hashcpy() that moved to its only useful caller in get_delta_base(). Makes sense.

The remaining conversions are trivial.

Thanks, Taylor

When chasing a REF_DELTA, we need to pull the raw hash bytes out of the mmap'd packfile into an object_id struct. We do that with a raw hashcpy() of the appropriate length (that happens directly now, though before the previous commit it happened inside find_pack_entry_one(), also using a hashcpy).

But I think this creates a potentially dangerous situation due to d4d364b2c7 (hash: convert `oidcmp()` and `oideq()` to compare whole hash, 2024-06-14). When using sha1, we'll have uninitialized bytes in the latter part of the object_id.hash buffer, which could fool oideq(), etc.

We should use oidread() instead, which correctly zero-pads the extra bytes, as of c98d762ed9 (global: ensure that object IDs are always padded, 2024-06-14).

As far as I can see, this has not been a problem in practice because the object_id we feed to find_pack_entry_one() is never used with oideq(), etc. It is being compared to the bytes mmap'd from a pack idx file, which of course do not have the extra padding bytes themselves. So there's no bug here, but this just puzzled me while looking at the code. We should do the more obviously safe thing, both for future-proofing and to avoid confusing readers.

Signed-off-by: Jeff King <peff@peff.net>
---
+cc Patrick for any wisdom here. I'd guess that the conversions from
c98d762ed9 were found by using ASan, valgrind, or similar with the new
oideq() implementation.  And so this case, because it actually is safe,
was not flagged.

 packfile.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

On Fri, Oct 25, 2024 at 02:41:48AM -0400, Jeff King wrote:

I was glad that you worked on this, because I figured I might end up ~tricking you into~ getting you to take a closer look at some of the other potential fixups in the dumb http code.

The series was a pleasant read, and it looks quite good to me. Let's wait for some other reviewers to chime in before we start merging this down, but otherwise I think this is good to go.

Thanks, Taylor

On Sat, Oct 19, 2024 at 09:24:55PM -0400, Jeff King wrote:

TBH, I don't think that this is all that hacky, but...

...this seems like a much better idea to me. We shouldn't be keeping the provided *.idx file given that we plan on overwriting it later on in the process.

This feels like more fallout similar to the one described by c177d3dc50 (pack-objects: use finalize_object_file() to rename pack/idx/etc, 2024-09-26), in particularly the paragraph beginning with "This has some test and real-world fallout ..." and the one immediately below it.

So I think that your "cleaner fix" is the way to go.

Thanks, Taylor

On Mon, Oct 21, 2024 at 04:23:57PM -0400, Taylor Blau wrote:

Mostly I was worried that there was some case where the filename would not be the usual pack-$hash.idx that we expected. But having now looked at the code quite a bit to produce the follow-on patch, I think these names are always generated locally from sha1_pack_name(), etc.

Another variant of this approach is to actually teach index-pack a flag to say "it's OK to overwrite an existing .idx file, even if the bytes don't match". That's the simplest fix _conceptually_, in that it takes us back to the pre-2.47 behavior. But plumbing through that option would require a lot of ugly and confusing boilerplate code.

I dug on this a little more, and...I don't love the design of the dumb-http protocol.

A big question here is: what should the life cycle of those downloaded .idx files be? And how much should we try to avoid re-downloading them (keeping in mind that the .idx for linux.git is almost 300MB)?

In a normal dumb-http clone, something like this happens with the current code:

  1. We see we need some object X.

  2. We download _all_ of the .idx files offered by the remote, since we
     have no idea in which one we'll find X. (There's an obvious
     low-hanging optimization to stop when we find X, but in the worst
     case we need all of them anyway).

  3. For each one, we download the matching pack if needed. In most
     cases you need all of them, but there might be some pack P for
     which we only have the .idx. We leave that bare .idx sitting in the
     objects directory.

So now we have all of the other side's .idx files locally, but not necessarily all of their .packs. And here are some interesting follow-on cases:

  a. You do a follow-up fetch, and now you need object Y. The saved idx
     of P is useful, because you can avoid downloading it again. Yay.
     You likewise should avoid re-downloading any for which you did get
     the matching pack, because you'll have pack-$hash.idx locally (just
     your generated copy, not the one you got from the other side).

  b. The other side repacks, and then you fetch. Now all their pack
     names are changed. So your saved idx of P is useless. But also you
     end up downloading all of the new .idx files anyway (even though
     you probably already have 99% of the objects).

  c. You repack, then fetch. Now the other side has the same packs, but
     your local copies all have new names. You re-download every idx
     from the other side, not realizing that you already saw them last
     time.

With my patch, case (a) doesn't save P (because now we treat it like a tempfile), so we re-download it unnecessarily. That's a little worse. I think the other two cases are largely the same.

But there's a flip side here. What if we kept the .idx files we got from the other side in objects/pack/remote-idx/, basically as a cache? Now case (c) avoids a lot of pointless downloading, though it comes at the cost of storing those extra .idx files (which, before we repack, are basically duplicates of the .idx files we generated ourselves; if we really wanted to get clever we could probably hard-link or something).

We didn't improve (b). And in fact, we've created a new issue: we're still holding on to the old cached .idx files. We'd have to have some scheme to get rid of them. I suppose that happens when we fetch the other side's objects/info/packs file and see that they're no longer mentioned (though if you want to be pedantic, this really should be a per-remote cache, since in theory two remotes could hold the same pack).

So...what does it all mean for this fix?

I think the patch I posted earlier, to keep the .idx files as separate tempfiles, is only slightly worse than the status quo (it doesn't keep P.idx that we didn't need). And given my general feelings towards the dumb-http protocol, maybe that is the right place to stop.

It just feels like we _could_ be improving things with a better managed .idx cache system. And that would fix the regression at the same time. But I'm not sure it's worth sinking time and complexity into this terribly inefficient protocol.

(Part of why I laid all this out is that until wrote all of the above, I wasn't completely convinced that case (a) was the only one that suffered, and that it was so mild).

-Peff

On Tue, Oct 22, 2024 at 01:00:40AM -0400, Jeff King wrote:

All very well analyzed. I share your feelings about the dumb-HTTP protocol. It would be nice to fix, but I have a feeling that our collective time is probably better spent pursuing a reasonable band-aid and then focusing our attention elsewhere ;-).

Thanks, Taylor