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

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

[haoyurenzhuxia]

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;
+
 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);
+}
+
 static void write_midx_reverse_index(char *midx_name, unsigned char *midx_hash,
 				     struct write_midx_context *ctx)
 {
-	struct strbuf buf = STRBUF_INIT;
 	const char *tmp_file;
 
-	strbuf_addf(&buf, "%s-%s.rev", midx_name, hash_to_hex(midx_hash));
+	strbuf_addf(&rev_filename, "%s-%s.rev", midx_name, hash_to_hex(midx_hash));
 
 	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 (finalize_object_file(tmp_file, rev_filename.buf))
 		die(_("cannot store reverse index file"));
 
-	strbuf_release(&buf);
+	sigchain_push_common(remove_rev_file_on_signal);
 }
 
 static void clear_midx_files_ext(const char *object_dir, const char *ext,
-- 
2.34.1.52.g9efd6505a3.dirty.agit.6.5.6

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

[Ævar Arnfjörð Bjarmason]

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?

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

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

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

Exactly my thought.  Well said.

The _only_ case that might matter is if the next "write --bitmap" gets
confused ir there is a leftover file(s) from a previous run, but then
such a bug should be fixed on the reading side, I would think.

Thanks.

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

[Taylor Blau]

On Wed, Jun 22, 2022 at 10:53:24AM -0700, Junio C Hamano wrote:
> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
> > 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
> >> ...
> > 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...
>
> Exactly my thought.  Well said.

`repack` relies on `git multi-pack-index write --bitmap` to do the
actual work here. A few things worth noting:

  - 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

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

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

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

This is mostly the same as Ævar's original suggestion, just with a
larger scope. But I agree that it's the right direction nonetheless.

> The _only_ case that might matter is if the next "write --bitmap" gets
> confused ir there is a leftover file(s) from a previous run, but then
> such a bug should be fixed on the reading side, I would think.

It shouldn't in practice. We'll recognize that the .rev file is garbage
if it's checksum doesn't match the MIDX's, and we squashed the bug where
changing the object *order* (but not the objects themselves) doesn't
change the MIDX checksum (it does now).

We won't write over an existing .rev file with the right name, but we'll
always prefer to read the .rev data from the MIDX itself, if it exists.

Thanks,
Taylor

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

[Junio C Hamano]

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.

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

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

[Taylor Blau]

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

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

[Teng Long]

On Wed, Wed, 22 Jun 2022 14:13:47 -0400, Taylor Blay wrote:

> It shouldn't in practice. We'll recognize that the .rev file is garbage
> if it's checksum doesn't match the MIDX's, and we squashed the bug where
> changing the object *order* (but not the objects themselves) doesn't
> change the MIDX checksum (it does now).
> 
> We won't write over an existing .rev file with the right name, but we'll
> always prefer to read the .rev data from the MIDX itself, if it exists.

Yes, this is what I want to say.

On the patch itself, I think it may worth to remove the tmp rev file at earlier
time because there may reduce doubt about whether the file remaining after
the interruption will have side effects (but we know it won't for now ,
because regenerating midx and rev will ensure an strict match based on the
checksum).

Thanks.

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

[Xiaowen Xia]

Thanks for your attention.

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

* The `gettext()` and `fputs()` which calls in `die_errno(_())` are seem
highly unlikely async-signal-safety, this may cause problems in signal
handlers, I should remove it.
* This is thread-safe. The *.rev file will no longer be read/written after
creating the signal handler by `sigchain_push_common()`.
* On the other hand, the command: `git multi-pack-index write --bitmap` will
create an lockfile named `multi-pack-index.lock` and the signal handler
will not affect the execution of the same command or git-process.

The purpose of this patch is the same as what Teng Long said, to clean up the
files generated by the process when the process exits abnormally.

Thanks.

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

[Xiaowen Xia]

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

You mean, *.rev file will be embedded in MIDX files in the future, and *.rev
file don't exist anymore?
If this is the plan, the patch is helpless, ignore it.