Taylor Blau <me@xxxxxxxxxxxx> writes:
> On Fri, Jan 14, 2022 at 01:43:55PM -0800, Junio C Hamano wrote:
>> Junio C Hamano <gitster@xxxxxxxxx> writes:
>>
>> > Taylor Blau <me@xxxxxxxxxxxx> writes:
>> >
>> >> ... It's likely we were using
>> >> finalize_object_file() instead of a pure rename() because the former
>> >> also adjusts shared permissions.
>> >
>> > I thought the primary reason why we use finalize was because we
>> > ignore EEXIST (and the assumption is that the files with the same
>> > contents get the same name computed from their contents).
>> >
>> >> tmp_file = write_rev_file_order(NULL, ctx->pack_order, ctx->entries_nr,
>> >> midx_hash, WRITE_REV);
>> >>
>> >> - if (finalize_object_file(tmp_file, buf.buf))
>> >> + if (rename(tmp_file, buf.buf))
>> >> die(_("cannot store reverse index file"));
>> >
>> > Doesn't your new code die with it if buf.buf names an existing file?
>>
>> Ah, scratch that. rename() discards the old one atomically, so as
>> long as tmp_file and buf.buf are in the same directory (which I
>> think it is in this case), we wouldn't be affected by the bug that
>> is worked around with "Coda hack" in finalize_object_file(), either.
>
> Exactly. In this case, we really did want to overwrite an existing .rev
> file with the same name. That's because prior to this patch, we didn't
> store the object order in the MIDX itself. That made it possible for us
> to change the object order, but leave the MIDX checksum alone.
The other change in this step is addition of a new chunk type that
records the revindex data. With that, you are guaranteeing that a
new file with the same checksum as an old one must have the
byte-for-byte identical contents, so at that point, it is OK to use
finalize_object_file() and not use rename() to keep the old file, no?
If that is the case, we may rather want to use f-o-f consistently
for stuff inside .git/ directory whose filename is tied with its
contents. I do not think we want to chase a bug that comes from
difference between link-then-unlink vs rename, which affects loose
object files in one way and midx file in another way.
