{"thread":{"id":"49670","subject":"'ds/test-multi-pack-index' vs. 'ab/commit-graph-progress'","startedAt":"2018-10-25T11:16:04Z","lastAt":"2018-10-29T13:15:04Z","messageCount":6,"participants":["SZEDER Gábor","Derrick Stolee","Ævar Arnfjörð Bjarmason"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"361508","messageId":"20181025111557.GL30222@szeder.dev","threadId":"49670","inReplyTo":null,"subject":"'ds/test-multi-pack-index' vs. 'ab/commit-graph-progress'","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-10-25T11:15:57Z","receivedAt":"2018-10-25T11:16:04Z","isPatch":false,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"Hi,\n\nwhen branch 'ds/test-multi-pack-index' is merged with\n'ab/commit-graph-progress', IOW 'master', 'next', or 'pu',\n'GIT_TEST_MULTI_PACK_INDEX=1 ./t6500-gc.sh' fails with:\n\n expecting success:\n git -c gc.writeCommitGraph=true gc --no-quiet >stdout 2>stderr &&\n test_must_be_empty stdout &&\n test_line_count = 1 stderr &&\n test_i18ngrep \"Computing commit graph generation numbers\" stderr\n \n + git -c gc.writeCommitGraph=true gc --no-quiet\n + test_must_be_empty stdout\n + test_path_is_file stdout\n + test -f stdout\n + test -s stdout\n + test_line_count = 1 stderr\n + test 3 != 3\n + wc -l\n + test 16 = 1\n + echo test_line_count: line count for stderr != 1\n test_line_count: line count for stderr != 1\n + cat stderr\n error: packfile .git/objects/pack/pack-c67996b982e718f8e3fa70c5ff7db3cecf688bbb.pack index unavailable\n error: packfile .git/objects/pack/pack-d4f2632c6a37149bb546b8b0cfbc56b8183cd0f8.pack index unavailable\n error: packfile .git/objects/pack/pack-d4f2632c6a37149bb546b8b0cfbc56b8183cd0f8.pack index unavailable\n error: packfile .git/objects/pack/pack-c67996b982e718f8e3fa70c5ff7db3cecf688bbb.pack index unavailable\n error: packfile .git/objects/pack/pack-c67996b982e718f8e3fa70c5ff7db3cecf688bbb.pack index unavailable\n error: packfile .git/objects/pack/pack-c67996b982e718f8e3fa70c5ff7db3cecf688bbb.pack index unavailable\n error: packfile .git/objects/pack/pack-c67996b982e718f8e3fa70c5ff7db3cecf688bbb.pack index unavailable\n error: packfile .git/objects/pack/pack-c67996b982e718f8e3fa70c5ff7db3cecf688bbb.pack index unavailable\n error: packfile .git/objects/pack/pack-d4f2632c6a37149bb546b8b0cfbc56b8183cd0f8.pack index unavailable\n error: packfile .git/objects/pack/pack-d4f2632c6a37149bb546b8b0cfbc56b8183cd0f8.pack index unavailable\n error: packfile .git/objects/pack/pack-d4f2632c6a37149bb546b8b0cfbc56b8183cd0f8.pack index unavailable\n error: packfile .git/objects/pack/pack-d4f2632c6a37149bb546b8b0cfbc56b8183cd0f8.pack index unavailable\n error: packfile .git/objects/pack/pack-d4f2632c6a37149bb546b8b0cfbc56b8183cd0f8.pack index unavailable\n error: packfile .git/objects/pack/pack-d4f2632c6a37149bb546b8b0cfbc56b8183cd0f8.pack index unavailable\n error: packfile .git/objects/pack/pack-c67996b982e718f8e3fa70c5ff7db3cecf688bbb.pack index unavailable\n Computing commit graph generation numbers: 25% (1/4) ^MComputing commit graph generation numbers: 50% (2/4) ^MComputing commit graph generation numbers: 75% (3/4) ^MComputing commit graph generation numbers: 100% (4/4) ^MComputing commit graph generation numbers: 100% (4/4), done.\n + return 1\n error: last command exited with $?=1\n not ok 9 - gc --no-quiet\n\n\nI suspect these \"packfile index unavailable\" errors are a Bad Thing,\nbut I didn't follow the MIDX development closely enough to judge.\nSurprisingly (to me), 'git gc' didn't exit with error despite these\nerrors.\n\nAnyway, this test seems to be too fragile, because that\n\n test_line_count = 1 stderr\n\nline will trigger, when anything else during 'git gc' prints\nsomething. And I find it quite strange that an option called\n'--no-quiet' only shows the commit-graph progress, but not the regular\noutput of 'git gc'.\n\n\n"},{"id":"361511","messageId":"20181025125405.30351-1-dstolee@microsoft.com","threadId":"49670","inReplyTo":"20181025111557.GL30222@szeder.dev","subject":"[PATCH] packfile: close multi-pack-index in close_all_packs","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2018-10-25T12:54:05Z","receivedAt":"2018-10-25T12:54:12Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"Whenever we delete pack-files from the object directory, we need\nto also delete the multi-pack-index that may refer to those\nobjects. Sometimes, this connection is obvious, like during a\nrepack. Other times, this is less obvious, like when gc calls\na repack command and then does other actions on the objects, like\nwrite a commit-graph file.\n\nThe pattern we use to avoid out-of-date in-memory packed_git\nstructs is to call close_all_packs(). This should also call\nclose_midx(). Since we already pass an object store to\nclose_all_packs(), this is a nicely scoped operation.\n\nThis fixes a test failure when running t6500-gc.sh with\nGIT_TEST_MULTI_PACK_INDEX=1.\n\nReported-by: Szeder Gábor \nSigned-off-by: Derrick Stolee \n---\n\nThanks for the report, Szeder! I think this is the correct fix,\nand more likely to be permanent across all builtins that run\nauto-GC. I based it on ds/test-multi-pack-index so it could easily\nbe added on top.\n\n-Stolee\n\n packfile.c | 5 +++++\n 1 file changed, 5 insertions(+)\n\ndiff --git a/packfile.c b/packfile.c\nindex 841b36182f..37fcd8f136 100644\n--- a/packfile.c\n+++ b/packfile.c\n@@ -339,6 +339,11 @@ void close_all_packs(struct raw_object_store *o)\n \t\t\tBUG(\"want to close pack marked 'do-not-close'\");\n \t\telse\n \t\t\tclose_pack(p);\n+\n+\tif (o->multi_pack_index) {\n+\t\tclose_midx(o->multi_pack_index);\n+\t\to->multi_pack_index = NULL;\n+\t}\n }\n \n /*\n\nbase-commit: 0465a50506023df0932fe0534fe6ac6712c0d854\n-- \n2.17.1\n\n"},{"id":"361561","messageId":"87va5plois.fsf@evledraar.gmail.com","threadId":"49670","inReplyTo":"20181025111557.GL30222@szeder.dev","subject":"Re: 'ds/test-multi-pack-index' vs. 'ab/commit-graph-progress'","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-10-25T21:11:55Z","receivedAt":"2018-10-25T21:12:02Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Oct 25 2018, SZEDER Gábor wrote:\n\n> when branch 'ds/test-multi-pack-index' is merged with\n> 'ab/commit-graph-progress', IOW 'master', 'next', or 'pu',\n> 'GIT_TEST_MULTI_PACK_INDEX=1 ./t6500-gc.sh' fails with:\n>\n> expecting success:\n> git -c gc.writeCommitGraph=true gc --no-quiet >stdout 2>stderr &&\n> test_must_be_empty stdout &&\n> test_line_count = 1 stderr &&\n> test_i18ngrep \"Computing commit graph generation numbers\" stderr\n>\n> + git -c gc.writeCommitGraph=true gc --no-quiet\n> + test_must_be_empty stdout\n> + test_path_is_file stdout\n> + test -f stdout\n> + test -s stdout\n> + test_line_count = 1 stderr\n> + test 3 != 3\n> + wc -l\n> + test 16 = 1\n> + echo test_line_count: line count for stderr != 1\n> test_line_count: line count for stderr != 1\n> + cat stderr\n> error: packfile .git/objects/pack/pack-c67996b982e718f8e3fa70c5ff7db3cecf688bbb.pack index unavailable\n> error: packfile .git/objects/pack/pack-d4f2632c6a37149bb546b8b0cfbc56b8183cd0f8.pack index unavailable\n> error: packfile .git/objects/pack/pack-d4f2632c6a37149bb546b8b0cfbc56b8183cd0f8.pack index unavailable\n> error: packfile .git/objects/pack/pack-c67996b982e718f8e3fa70c5ff7db3cecf688bbb.pack index unavailable\n> error: packfile .git/objects/pack/pack-c67996b982e718f8e3fa70c5ff7db3cecf688bbb.pack index unavailable\n> error: packfile .git/objects/pack/pack-c67996b982e718f8e3fa70c5ff7db3cecf688bbb.pack index unavailable\n> error: packfile .git/objects/pack/pack-c67996b982e718f8e3fa70c5ff7db3cecf688bbb.pack index unavailable\n> error: packfile .git/objects/pack/pack-c67996b982e718f8e3fa70c5ff7db3cecf688bbb.pack index unavailable\n> error: packfile .git/objects/pack/pack-d4f2632c6a37149bb546b8b0cfbc56b8183cd0f8.pack index unavailable\n> error: packfile .git/objects/pack/pack-d4f2632c6a37149bb546b8b0cfbc56b8183cd0f8.pack index unavailable\n> error: packfile .git/objects/pack/pack-d4f2632c6a37149bb546b8b0cfbc56b8183cd0f8.pack index unavailable\n> error: packfile .git/objects/pack/pack-d4f2632c6a37149bb546b8b0cfbc56b8183cd0f8.pack index unavailable\n> error: packfile .git/objects/pack/pack-d4f2632c6a37149bb546b8b0cfbc56b8183cd0f8.pack index unavailable\n> error: packfile .git/objects/pack/pack-d4f2632c6a37149bb546b8b0cfbc56b8183cd0f8.pack index unavailable\n> error: packfile .git/objects/pack/pack-c67996b982e718f8e3fa70c5ff7db3cecf688bbb.pack index unavailable\n> Computing commit graph generation numbers: 25% (1/4) ^MComputing commit graph generation numbers: 50% (2/4) ^MComputing commit graph generation numbers: 75% (3/4) ^MComputing commit graph generation numbers: 100% (4/4) ^MComputing commit graph generation numbers: 100% (4/4), done.\n> + return 1\n> error: last command exited with $?=1\n> not ok 9 - gc --no-quiet\n>\n>\n> I suspect these \"packfile index unavailable\" errors are a Bad Thing,\n> but I didn't follow the MIDX development closely enough to judge.\n> Surprisingly (to me), 'git gc' didn't exit with error despite these\n> errors.\n>\n> Anyway, this test seems to be too fragile, because that\n>\n> test_line_count = 1 stderr\n\nYeah maybe it's too fragile, on the other hand it caught what seems to\nbe a bug under GIT_TEST_MULTI_PACK_INDEX=true, and the rest of the test\nsuite passes, so there's that.\n\n> line will trigger, when anything else during 'git gc' prints\n> something. And I find it quite strange that an option called\n> '--no-quiet' only shows the commit-graph progress, but not the regular\n> output of 'git gc'.\n\nIt's confusing, but the reason for this seeming discrepancy is that\nwriting the commit-graph happens in-process, whereas the rest of the\nwork done by git-gc (and its output) comes from subprocesses. Most of\nthat output is from \"git-repack\" / \"git-pack-objects\" which doesn't pay\nthe same attention to --quiet and --no-quiet, instead it checks\nisatty(). See cmd_pack_objects().\n"},{"id":"361889","messageId":"20181029111041.GQ30222@szeder.dev","threadId":"49670","inReplyTo":"20181025125405.30351-1-dstolee@microsoft.com","subject":"Re: [PATCH] packfile: close multi-pack-index in close_all_packs","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-10-29T11:10:41Z","receivedAt":"2018-10-29T12:10:48Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Thu, Oct 25, 2018 at 12:54:05PM +0000, Derrick Stolee wrote:\n> Whenever we delete pack-files from the object directory, we need\n> to also delete the multi-pack-index that may refer to those\n> objects. Sometimes, this connection is obvious, like during a\n> repack. Other times, this is less obvious, like when gc calls\n> a repack command and then does other actions on the objects, like\n> write a commit-graph file.\n> \n> The pattern we use to avoid out-of-date in-memory packed_git\n> structs is to call close_all_packs(). This should also call\n> close_midx(). Since we already pass an object store to\n> close_all_packs(), this is a nicely scoped operation.\n> \n> This fixes a test failure when running t6500-gc.sh with\n> GIT_TEST_MULTI_PACK_INDEX=1.\n> \n> Reported-by: Szeder Gábor \n> Signed-off-by: Derrick Stolee \n> ---\n> \n> Thanks for the report, Szeder! I think this is the correct fix,\n> and more likely to be permanent across all builtins that run\n> auto-GC. I based it on ds/test-multi-pack-index so it could easily\n> be added on top.\n\nOK, avoiding those errors in the first place is good, of course...\nHowever, I still find it disconcerting that those errors didn't cause\n'git gc' to error out, and, consequently, what other MIDX-related\nerrors/bugs might do the same.\n\n> -Stolee\n> \n> packfile.c | 5 +++++\n> 1 file changed, 5 insertions(+)\n> \n> diff --git a/packfile.c b/packfile.c\n> index 841b36182f..37fcd8f136 100644\n> --- a/packfile.c\n> +++ b/packfile.c\n> @@ -339,6 +339,11 @@ void close_all_packs(struct raw_object_store *o)\n> \t\t\tBUG(\"want to close pack marked 'do-not-close'\");\n> \t\telse\n> \t\t\tclose_pack(p);\n> +\n> +\tif (o->multi_pack_index) {\n> +\t\tclose_midx(o->multi_pack_index);\n> +\t\to->multi_pack_index = NULL;\n> +\t}\n> }\n> \n> /*\n> \n> base-commit: 0465a50506023df0932fe0534fe6ac6712c0d854\n> -- \n> 2.17.1\n> \n"},{"id":"361890","messageId":"20181029111632.GR30222@szeder.dev","threadId":"49670","inReplyTo":"87va5plois.fsf@evledraar.gmail.com","subject":"Re: 'ds/test-multi-pack-index' vs. 'ab/commit-graph-progress'","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-10-29T11:16:32Z","receivedAt":"2018-10-29T12:16:38Z","isPatch":false,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Thu, Oct 25, 2018 at 11:11:55PM +0200, Ævar Arnfjörð Bjarmason wrote:\n> > Anyway, this test seems to be too fragile, because that\n> >\n> > test_line_count = 1 stderr\n> \n> Yeah maybe it's too fragile, on the other hand it caught what seems to\n> be a bug under GIT_TEST_MULTI_PACK_INDEX=true, and the rest of the test\n> suite passes, so there's that.\n\nI can image more prudent approaches that would have done the same,\ne.g. '! grep ^error stderr'.\n\n> > line will trigger, when anything else during 'git gc' prints\n> > something. And I find it quite strange that an option called\n> > '--no-quiet' only shows the commit-graph progress, but not the regular\n> > output of 'git gc'.\n> \n> It's confusing, but the reason for this seeming discrepancy is that\n> writing the commit-graph happens in-process, whereas the rest of the\n> work done by git-gc (and its output) comes from subprocesses. Most of\n> that output is from \"git-repack\" / \"git-pack-objects\" which doesn't pay\n> the same attention to --quiet and --no-quiet, instead it checks\n> isatty(). See cmd_pack_objects().\n\nThis explains what implementation details led to the current\nbehaviour, but does not justify the actual inconsistency.\n\n"},{"id":"361893","messageId":"cf5a9e5e-ac75-1ba2-fda4-9776a90f6968@gmail.com","threadId":"49670","inReplyTo":"20181029111041.GQ30222@szeder.dev","subject":"Re: [PATCH] packfile: close multi-pack-index in close_all_packs","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2018-10-29T13:15:01Z","receivedAt":"2018-10-29T13:15:04Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 10/29/2018 7:10 AM, SZEDER Gábor wrote:\n> On Thu, Oct 25, 2018 at 12:54:05PM +0000, Derrick Stolee wrote:\n>> Whenever we delete pack-files from the object directory, we need\n>> to also delete the multi-pack-index that may refer to those\n>> objects. Sometimes, this connection is obvious, like during a\n>> repack. Other times, this is less obvious, like when gc calls\n>> a repack command and then does other actions on the objects, like\n>> write a commit-graph file.\n>>\n>> The pattern we use to avoid out-of-date in-memory packed_git\n>> structs is to call close_all_packs(). This should also call\n>> close_midx(). Since we already pass an object store to\n>> close_all_packs(), this is a nicely scoped operation.\n>>\n>> This fixes a test failure when running t6500-gc.sh with\n>> GIT_TEST_MULTI_PACK_INDEX=1.\n>>\n>> Reported-by: Szeder Gábor \n>> Signed-off-by: Derrick Stolee \n>> ---\n>>\n>> Thanks for the report, Szeder! I think this is the correct fix,\n>> and more likely to be permanent across all builtins that run\n>> auto-GC. I based it on ds/test-multi-pack-index so it could easily\n>> be added on top.\n> OK, avoiding those errors in the first place is good, of course...\n> However, I still find it disconcerting that those errors didn't cause\n> 'git gc' to error out, and, consequently, what other MIDX-related\n> errors/bugs might do the same.\nWhen I added GIT_TEST_MULTI_PACK_INDEX, one of the important pieces was \nto delete the multi-pack-index file whenever deleting the pack-files it \ncontains. This only happens during a 'git repack'.\n\nThe thing I had missed (and is covered by this patch) is when we run a \nsubcommand that can remove pack-files while we have a multi-pack-index open.\n\nThe reason this wasn't caught is that the test suite did not include any \ncases where the following things happened in order:\n\n1. Open pack-files and multi-pack-index.\n2. Delete pack-files, but keep multi-pack-index open.\n3. Read objects (from the multi-pack-index).\n\nThis step 3 was added to 'git gc' by the commit-graph write, hence the \nbreak. The pack-file deletion happens in the repack subcommand.\n\nSince close_all_packs() is the way we guarded against this problem in \nthe traditional pack-file environment, this is the right place to insert \na close_midx() call, and will avoid cases like this in the future (at \nleast, the multi-pack-index will not be the reason on its own).\n\nThanks,\n-Stolee\n"}]}