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
