Re: [RFC PATCH] midx.c: clean up .rev file

[Taylor Blau <me@ttaylorr.com>]

On Wed, Jun 22, 2022 at 12:58:42PM -0700, Junio C Hamano wrote:
> Taylor Blau <me@ttaylorr.com> writes:
>
> >   - the MIDX file itself is written using a lock_file, so it is
> >     atomically moved into place, and the temporary file is either
> >     removed, or cleaned up automatically with a sigchain handler on
> >     process death
>
> Good.
>
> >   - the bitmap (written in bitmap_writer_finish(), which is the path for
> >     both single- and multi-pack bitmaps) is written to a temporary file
> >     and moved into place after the bitmaps are written.
> >
> >     ...but this temporary file isn't automatically cleaned up, so it
> >     could stick around after process death. Luckily the race window here
> >     is pretty small, since all of the bitmaps have been computed already
> >     and are held in memory.
> >
> >     This is probably worth a cleanup on its own, too.
>
> As long as the "temporary file" is clearly a temporary file that
> "gc" can recognize and get rid of, it would be OK, I would think.

Yeah, I think either is fine (though it would be slightly nicer to have
the sigchain code clean it up ahead of time when possible). In the
meantime, nothing is broken, though.

> >   - unless GIT_TEST_MIDX_WRITE_REV=1 is in your environment, we won't
> >     *write* a .rev file, hence this is pretty rare to deal with in
> >     practice.
>
> OK, but if we were to write one, we should do the same "write into a
> temporary, rename it in place" dance, right?  Or is a separate .rev
> file is pretty much a thing of last decade that we do not have to
> worry too much about?

Right. Technically we link() it into place, cf. our discussion about it
here:

    https://lore.kernel.org/git/xmqq5yqeghck.fsf@gitster.g/

But in general this is not really at all considered common anymore,
since we expect all new writes to have the reverse index embedded in the
MIDX itself.

Thanks,
Taylor