Re: [PATCH] packfile: close multi-pack-index in close_all_packs

From: Derrick Stolee <stolee@gmail.com>
Date: Oct 29, 2018, 13:15 UTC
Message-ID: <cf5a9e5e-ac75-1ba2-fda4-9776a90f6968@gmail.com>
In-Reply-To: <20181029111041.GQ30222@szeder.dev>

On 10/29/2018 7:10 AM, SZEDER Gábor wrote:

Show 28 quoted lines

> On Thu, Oct 25, 2018 at 12:54:05PM +0000, Derrick Stolee wrote:
>> Whenever we delete pack-files from the object directory, we need
>> to also delete the multi-pack-index that may refer to those
>> objects. Sometimes, this connection is obvious, like during a
>> repack. Other times, this is less obvious, like when gc calls
>> a repack command and then does other actions on the objects, like
>> write a commit-graph file.
>>
>> The pattern we use to avoid out-of-date in-memory packed_git
>> structs is to call close_all_packs(). This should also call
>> close_midx(). Since we already pass an object store to
>> close_all_packs(), this is a nicely scoped operation.
>>
>> This fixes a test failure when running t6500-gc.sh with
>> GIT_TEST_MULTI_PACK_INDEX=1.
>>
>> Reported-by: Szeder Gábor <szeder.dev@gmail.com>
>> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
>> ---
>>
>> Thanks for the report, Szeder! I think this is the correct fix,
>> and more likely to be permanent across all builtins that run
>> auto-GC. I based it on ds/test-multi-pack-index so it could easily
>> be added on top.
> OK, avoiding those errors in the first place is good, of course...
> However, I still find it disconcerting that those errors didn't cause
> 'git gc' to error out, and, consequently, what other MIDX-related
> errors/bugs might do the same.

When I added GIT_TEST_MULTI_PACK_INDEX, one of the important pieces was to delete the multi-pack-index file whenever deleting the pack-files it contains. This only happens during a 'git repack'.

The thing I had missed (and is covered by this patch) is when we run a subcommand that can remove pack-files while we have a multi-pack-index open.

The reason this wasn't caught is that the test suite did not include any cases where the following things happened in order:

1. Open pack-files and multi-pack-index.
2. Delete pack-files, but keep multi-pack-index open.
3. Read objects (from the multi-pack-index).

This step 3 was added to 'git gc' by the commit-graph write, hence the break. The pack-file deletion happens in the repack subcommand.

Since close_all_packs() is the way we guarded against this problem in the traditional pack-file environment, this is the right place to insert a close_midx() call, and will avoid cases like this in the future (at least, the multi-pack-index will not be the reason on its own).

Thanks, -Stolee