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

[Taylor Blau <me@ttaylorr.com>]

On Tue, Oct 22, 2024 at 01:14:02AM -0400, Jeff King wrote:
> > > +static char *pack_path_from_idx(const char *idx_path)
> > > +{
> > > +	size_t len;
> > > +	if (!strip_suffix(idx_path, ".idx", &len))
> > > +		BUG("idx path does not end in .idx: %s", idx_path);
> > > +	return xstrfmt("%.*s.pack", (int)len, idx_path);
> > > +}
> > > +
> > >  struct packed_git *parse_pack_index(unsigned char *sha1, const char *idx_path)
> > >  {
> > > -	const char *path = sha1_pack_name(sha1);
> > > +	char *path = pack_path_from_idx(idx_path);
> >
> > 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.
>
> 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.

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 ;-).

> > 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?
>
> Yeah, the test is easy:
>
> diff --git a/t/t5550-http-fetch-dumb.sh b/t/t5550-http-fetch-dumb.sh
> index 58189c9f7d..50a7b98813 100755
> --- a/t/t5550-http-fetch-dumb.sh
> +++ b/t/t5550-http-fetch-dumb.sh
> @@ -507,4 +507,14 @@ test_expect_success 'fetching via http alternates works' '
>  	git -c http.followredirects=true clone "$HTTPD_URL/dumb/alt-child.git"
>  '
>
> +test_expect_success 'dumb http can fetch index v1' '
> +	server=$HTTPD_DOCUMENT_ROOT_PATH/idx-v1.git &&
> +	git init --bare "$server" &&
> +	git -C "$server" --work-tree=. commit --allow-empty -m foo &&
> +	git -C "$server" -c pack.indexVersion=1 gc &&
> +
> +	git clone "$HTTPD_URL/dumb/idx-v1.git" &&
> +	git -C idx-v1 fsck
> +'
> +
>  test_done

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.

> I raised some other more philosophical issues in the other part of the
> thread, but assuming the answer is "no, let's do the simplest thing",
> then I think this approach is OK.
>
> 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.

Sounds all good. Thanks!

> -Peff

Thanks,
Taylor