Re: [PATCH v4 05/25] midx: clear auxiliary .rev after replacing the MIDX

On Tue, Aug 24, 2021 at 01:27:34PM -0700, Junio C Hamano wrote:

Taylor Blau [off-list ref] writes:

quoted

When writing a new multi-pack index, write_midx_internal() attempts to
clean up any auxiliary files (currently just the MIDX's `.rev` file, but
soon to include a `.bitmap`, too) corresponding to the MIDX it's
replacing.

This step should happen after the new MIDX is written into place, since
doing so beforehand means that the old MIDX could be read without its
corresponding .rev file.

Signed-off-by: Taylor Blau <redacted>
---
 midx.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/midx.c b/midx.c
index 321c6fdd2f..73b199ca49 100644
--- a/midx.c
+++ b/midx.c

@@ -1086,10 +1086,11 @@ static int write_midx_internal(const char *object_dir, struct multi_pack_index *

 	if (flags & MIDX_WRITE_REV_INDEX)
 		write_midx_reverse_index(midx_name, midx_hash, &ctx);
-	clear_midx_files_ext(the_repository, ".rev", midx_hash);

 	commit_lock_file(&lk);

+	clear_midx_files_ext(the_repository, ".rev", midx_hash);

This needs to take object_dir into account, no?

Yes and no; clear_midx_files_ext() still takes a pointer to a 'struct
repository' until we pick up [1].

I asked for some changes in the latest version that Johannes posted. So
I'd be OK to live with this behavior for the time being, and then I can
send another patch on top that fixes the new and existing callers
(incorporating [1] with some new tests).

Or we can hold one up and expedite the other. I would suggest that we
pick up this series to next if you're otherwise happy with it and then I
can send the trivial fixes on top.

Thanks,
Taylor

[1]: https://lore.kernel.org/git/20210823171011.80588-1-johannes@sipsolutions.net/ (local)