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

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Wed, Jun 22 2022, haoyurenzhuxia@gmail.com wrote:

> From: Xia XiaoWen <haoyurenzhuxia@gmail.com>
>
> The command: `git multi-pack-index write --bitmap` will create 3
> files in `objects/pack/`:
>     * multi-pack-index
>     * multi-pack-index-*.bitmap
>     * multi-pack-index-*.rev
>
> But if the command is terminated by the user (such as Ctl-C) or
> the system, the midx reverse index file (`multi-pack-index-*.rev`)
> is not removed and still exists in `objects/pack/`:
>
>     $ GIT_TEST_MIDX_WRITE_REV=1 git multi-pack-index write --bitmap
>     Selecting bitmap commits: 133020, done.
>     Building bitmaps:   0% (3/331)
>     ^C^C
>
>     $ tree objects/pack/
>     objects/pack/
>     ├── multi-pack-index-3b048d1b965842cd866e10b6ec1a3035dbede0a5.rev
>     ├── pack-b7d425f1b01727d5f364f5d9fbab2d1900fcd5c0.idx
>     └── pack-b7d425f1b01727d5f364f5d9fbab2d1900fcd5c0.pack
>
> This patch resolves this by adding a cleanup handler to the sigchain.
>
> Signed-off-by: Xia XiaoWen <haoyurenzhuxia@gmail.com>
> ---
>  midx.c | 19 +++++++++++++++----
>  1 file changed, 15 insertions(+), 4 deletions(-)
>
> diff --git a/midx.c b/midx.c
> index 5f0dd386b0..6586051a62 100644
> --- a/midx.c
> +++ b/midx.c
> @@ -17,6 +17,7 @@
>  #include "refs.h"
>  #include "revision.h"
>  #include "list-objects.h"
> +#include "sigchain.h"
>  
>  #define MIDX_SIGNATURE 0x4d494458 /* "MIDX" */
>  #define MIDX_VERSION 1
> @@ -41,6 +42,8 @@
>  
>  #define PACK_EXPIRED UINT_MAX
>  
> +static struct strbuf rev_filename = STRBUF_INIT;

Is the rest of this API thread safe, and no longer is because of this?
You're doing this because...

>  const unsigned char *get_midx_checksum(struct multi_pack_index *m)
>  {
>  	return m->data + m->data_len - the_hash_algo->rawsz;
> @@ -884,21 +887,29 @@ static uint32_t *midx_pack_order(struct write_midx_context *ctx)
>  	return pack_order;
>  }
>  
> +static void remove_rev_file_on_signal(int signo)
> +{
> +	if (unlink(rev_filename.buf))
> +		die_errno(_("failed to remove %s"), rev_filename.buf);
> +
> +	sigchain_pop(signo);
> +	raise(signo);

We need to handle this signalling.

I wonder if we could (ab)use the lockfile.c/tempfile.c API instead here,
and get the signal handling, cleanup etc. for free.

Also, the commit message doesn't really say *why*, i.e. in cmd_repack()
we've suffered from this already, but don't we have "git gc" cleaning
these up? Maybe not (I didn't check), but maybe that was the previous
assumption...

I mean, I think it makes sense to clean these up, but are we doing the
same for the other X.* files for the X.pack? Should we?