Re: [PATCH] packfile: delete .idx files before .pack files

From: Taylor Blau <me@ttaylorr.com>
Date: Jun 20, 2023, 22:02 UTC
Message-ID: <ZJIiElehrukHlNy2@nand.local>
In-Reply-To: <pull.1547.git.1687287675248.gitgitgadget@gmail.com>

On Tue, Jun 20, 2023 at 07:01:15PM +0000, Derrick Stolee via GitGitGadget wrote:

Show 15 quoted lines

> From: Derrick Stolee <derrickstolee@github.com>
>
> When installing a packfile, we place the .pack file before the .idx
> file. The intention is that Git scans for .idx files in the pack
> directory and then loads the .pack files from that list.
>
> However, when we delete packfiles, we do not do this in the reverse
> order as we should. The unlink_pack_path() method deletes the .pack
> followed by the .idx.
>
> This creates a window where the process could be interrupted between
> the .pack deletion and the .idx deletion, leaving the repository in a
> state that looks strange, but isn't actually too problematic if we
> assume the pack was safe to delete. The .idx without a .pack will cause
> some overhead, but will not interrupt other Git processes.

I think that this specific case wouldn't cause a problem since we were about to delete that pack anyway, but it may be worth emphasizing that having a missing pack with an extant .idx file is not always guaranteed to be safe.

> This ordering was introduced into the 'git repack' builtin by
> a1bbc6c0176 (repack: rewrite the shell script in C, 2013-09-15), though
> we must be careful to track history through the code move in 8434e85d5f9
> (repack: refactor pack deletion for future use, 2019-06-10) to see that.

Thanks for calling this out, I agree that the "regression" dates back to a1bbc6c0176, since we can see that in git-repack.sh, we do:

    # Remove the "old-" files
    for name in $names
    do
      rm -f "$PACKDIR/old-pack-$name.idx"
      rm -f "$PACKDIR/old-pack-$name.pack"
    done

...removing the .idx first, which is the right thing to do here.

Show 13 quoted lines

> diff --git a/packfile.c b/packfile.c
> index fd083c86e00..6b591ddcccf 100644
> --- a/packfile.c
> +++ b/packfile.c
> @@ -381,7 +381,7 @@ void close_object_store(struct raw_object_store *o)
>
>  void unlink_pack_path(const char *pack_name, int force_delete)
>  {
> -	static const char *exts[] = {".pack", ".idx", ".rev", ".keep", ".bitmap", ".promisor", ".mtimes"};
> +	static const char *exts[] = {".idx", ".pack", ".rev", ".keep", ".bitmap", ".promisor", ".mtimes"};
>  	int i;
>  	struct strbuf buf = STRBUF_INIT;
>  	size_t plen;

Looks good, thanks.

Thanks, Taylor