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