| Message ID | 20181025125405.30351-1-dstolee@microsoft.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | packfile: close multi-pack-index in close_all_packs | expand |
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. > -Stolee > > packfile.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/packfile.c b/packfile.c > index 841b36182f..37fcd8f136 100644 > --- a/packfile.c > +++ b/packfile.c > @@ -339,6 +339,11 @@ void close_all_packs(struct raw_object_store *o) > BUG("want to close pack marked 'do-not-close'"); > else > close_pack(p); > + > + if (o->multi_pack_index) { > + close_midx(o->multi_pack_index); > + o->multi_pack_index = NULL; > + } > } > > /* > > base-commit: 0465a50506023df0932fe0534fe6ac6712c0d854 > -- > 2.17.1 >
On 10/29/2018 7:10 AM, SZEDER Gábor wrote: > 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
diff --git a/packfile.c b/packfile.c index 841b36182f..37fcd8f136 100644 --- a/packfile.c +++ b/packfile.c @@ -339,6 +339,11 @@ void close_all_packs(struct raw_object_store *o) BUG("want to close pack marked 'do-not-close'"); else close_pack(p); + + if (o->multi_pack_index) { + close_midx(o->multi_pack_index); + o->multi_pack_index = NULL; + } } /*
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. -Stolee packfile.c | 5 +++++ 1 file changed, 5 insertions(+) base-commit: 0465a50506023df0932fe0534fe6ac6712c0d854