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

[Taylor Blau <me@ttaylorr.com>]

On Sat, Oct 19, 2024 at 09:24:55PM -0400, Jeff King wrote:
> There are a few things I don't like there, though:
>
>   - obviously reversing the tempfile name back to the idx is hacky. We
>     could probably store the original idx that led us to this pack and
>     use that.

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

>   - I don't _think_ there is any case where that .idx file is precious.
>     We wouldn't be indexing the .pack file if we didn't just download
>     it, and we wouldn't have downloaded it if we already had a
>     .idx/.pack pair. OTOH the name we got from the other side isn't
>     necessarily the same one we'll use locally; we're feeding the pack
>     via "index-pack --stdin", so it will do its own hash to come up with
>     the name. The other side could have used a different scheme, or even
>     be trying to intentionally trick us.
>
> So I feel like the root of the problem is that we moved the other side's
> "pack-1234abcd.idx" into place at all! We do not plan to use it, but
> only need to access it via our custom packed_git structs to see whether
> we want to download the matching pack. And in fact I'd suspect there are
> other possible bugs/trickery lurking, if the other side gives us a
> broken .idx that does not match its pack (our odb would now have the
> broken file with no matching pack).
>
> So IMHO a cleaner fix is that we should keep the stuff we download from
> the remote marked as temporary files, and then throw them away when we
> complete the fetch (rather than just expecting index-pack to overwrite
> them).

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