On Wed, Jun 22 2022, haoyurenzhuxia@xxxxxxxxx wrote:
> From: Xia XiaoWen <haoyurenzhuxia@xxxxxxxxx>
>
> 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@xxxxxxxxx>
> ---
> 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?
