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

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 22, 2022, 19:58 UTC
Message-ID: <xmqqtu8c31xp.fsf@gitster.g>
In-Reply-To: <YrNb2x2/7Z31XnFJ@nand.local>

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.

Show 10 quoted lines

>   - 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.

>   - 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?

> So I think there are two things worth doing here:
>
>   - make sure that the temporary file used to stage the .bitmap is a
>     lock_file

Yes.

>   - use a temporary file to stage the .rev file (when forced to write
>     one), and ensure that that too is a lock_file

Yes.

Thanks.