Re: [PATCH v3 15/16] pack-revindex: write multi-pack reverse indexes

From: Jeff King <peff@peff.net>
Date: Mar 29, 2021, 12:53 UTC
Message-ID: <YGHNwin9zcrP00dI@coredump.intra.peff.net>
In-Reply-To: <fa3acb5d5af3aab46dd2a9703e4da03928625346.1615482270.git.me@ttaylorr.com>

On Thu, Mar 11, 2021 at 12:05:38PM -0500, Taylor Blau wrote:

> Implement the writing half of multi-pack reverse indexes. This is
> nothing more than the format describe a few patches ago, with a new set
> of helper functions that will be used to clear out stale .rev files
> corresponding to old MIDXs.

Looks good.

Show 7 quoted lines

> +struct clear_midx_data {
> +	char *keep;
> +	const char *ext;
> +};
> +
> +static void clear_midx_file_ext(const char *full_path, size_t full_path_len,
> +				const char *file_name, void *_data)

This will clean up _any_ stale midx .rev file. So even if we miss one when writing a new midx (due to a bug, race, power loss, etc), we'll catch it later.

We _might_ want to also teach various tempfile-cleanup code run by gc to likewise look for unattached midx .rev files, but I don't think we necessarily have to do it now.

Show 10 quoted lines

>  void clear_midx_file(struct repository *r)
>  {
>  	char *midx = get_midx_filename(r->objects->odb->path);
> @@ -1049,6 +1162,8 @@ void clear_midx_file(struct repository *r)
>  	if (remove_path(midx))
>  		die(_("failed to clear multi-pack-index at %s"), midx);
>  
> +	clear_midx_files_ext(r, ".rev", NULL);
> +
>  	free(midx);

The sole caller now doesn't pass the "keep" hash, so we'd always delete all of them. I guess we'll see that change once somebody starts actually writing them.

-Peff