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.
Then we'd keep the old .rev (with the old order, but the same name) in
place, if we used link(2) in order to try (and fail) to overwrite the
existing .rev with the new one.
Indeed, the temporary file created by write_rev_file_order() is written
in the pack directory, which is where the .rev file and MIDX will
ultimately live.
This code is going to be hidden behind a test knob in a few patches
anyway, but verifying all of this is good nonetheless (especially if you
just want to apply these first two patches into the v2.35 tree).
Thanks,
Taylor
