[0/11] dumb-http pack index v1 regression + cleanups

Message

Jeff King Oct. 25, 2024, 6:41 a.m. UTC

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

> I'd also like to see if I can clean things up around parse_pack_index(),
> whose semantics I'm changing here (and which violates all manner of
> assumptions that we usually have about packed_git structs). It's used
> only by the dumb-http code, and I think we want to refactor it a bit so
> that nobody else is tempted to use it.
> 
> I'll try to send something out tonight or tomorrow.

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(-)

Comments

Taylor Blau Oct. 25, 2024, 9:35 p.m. UTC | #1

On Fri, Oct 25, 2024 at 02:41:48AM -0400, Jeff King wrote:
>  16 files changed, 153 insertions(+), 101 deletions(-)

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