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

[Taylor Blau <me@ttaylorr.com>]

On Sat, Oct 19, 2024 at 10:40:22PM -0400, Jeff King wrote:
> diff --git a/http.c b/http.c
> index d59e59f66b..6df032e40f 100644
> --- a/http.c
> +++ b/http.c
> @@ -19,6 +19,7 @@
>  #include "string-list.h"
>  #include "object-file.h"
>  #include "object-store-ll.h"
> +#include "tempfile.h"
>
>  static struct trace_key trace_curl = TRACE_KEY_INIT(CURL);
>  static int trace_curl_data = 1;
> @@ -2388,8 +2389,24 @@ static char *fetch_pack_index(unsigned char *hash, const char *base_url)
>  	strbuf_addf(&buf, "objects/pack/pack-%s.idx", hash_to_hex(hash));
>  	url = strbuf_detach(&buf, NULL);
>
> -	strbuf_addf(&buf, "%s.temp", sha1_pack_index_name(hash));
> -	tmp = strbuf_detach(&buf, NULL);
> +	/*
> +	 * Don't put this into packs/, since it's just temporary and we don't
> +	 * want to confuse it with our local .idx files.  We'll generate our
> +	 * own index if we choose to download the matching packfile.
> +	 *
> +	 * It's tempting to use xmks_tempfile() here, but it's important that
> +	 * the file not exist, otherwise http_get_file() complains. So we
> +	 * create a filename that should be unique, and then just register it
> +	 * as a tempfile so that it will get cleaned up on exit.
> +	 *
> +	 * Arguably it would be better to hold on to the tempfile handle so
> +	 * that we can remove it as soon as we download the pack and generate
> +	 * the real index, but that might need more surgery.
> +	 */
> +	tmp = xstrfmt("%s/tmp_pack_%s.idx",
> +		      repo_get_object_directory(the_repository),
> +		      hash_to_hex(hash));
> +	register_tempfile(tmp);

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.

>  	if (http_get_file(url, tmp, NULL) != HTTP_OK) {
>  		error("Unable to get pack index %s", url);
> @@ -2427,10 +2444,8 @@ static int fetch_and_setup_pack_index(struct packed_git **packs_head,
>  	}
>
>  	ret = verify_pack_index(new_pack);
> -	if (!ret) {
> +	if (!ret)
>  		close_pack_index(new_pack);
> -		ret = finalize_object_file(tmp_idx, sha1_pack_index_name(sha1));

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

> -	}
>  	free(tmp_idx);
>  	if (ret)
>  		return -1;
> diff --git a/packfile.c b/packfile.c
> index df4ba67719..16d3bcf7f7 100644
> --- a/packfile.c
> +++ b/packfile.c
> @@ -237,13 +237,22 @@ static struct packed_git *alloc_packed_git(int extra)
>  	return p;
>  }
>
> +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.

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