{"thread":{"id":"53372","subject":"Re: [PATCH 05/15] run-job: implement pack-files job","startedAt":"2020-05-02T07:57:05Z","lastAt":"2020-05-04T14:34:24Z","messageCount":2,"participants":["Son Luong Ngoc","Derrick Stolee"],"isPatch":true,"patchVersion":1,"patchTotal":15},"messages":[{"id":"396816","messageId":"CAL3xRKfcKh=XxGso4DTRfJMAVMtwqyQkO0VUhqVuZUr_aQ5f+A@mail.gmail.com","threadId":"53372","inReplyTo":null,"subject":"Re: [PATCH 05/15] run-job: implement pack-files job","fromName":"Son Luong Ngoc","fromEmail":"sluongng@gmail.com","sentAt":"2020-05-02T07:56:50Z","receivedAt":"2020-05-02T07:57:05Z","isPatch":true,"sender":{"key":"sluongng@gmail.com","avatar":"https://avatars.githubusercontent.com/u/26684313?v=4"},"body":"Hi Derrick,\n\nSorry for another late reply on this RFC.\nThis time is a question on the multi-pack-index repack process.\n\n\"Derrick Stolee via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n> diff --git a/builtin/run-job.c b/builtin/run-job.c\n> index cecf9058c51..d3543f7ccb9 100644\n> --- a/builtin/run-job.c\n> +++ b/builtin/run-job.c\n\n...\n\n> +static int multi_pack_index_repack(void)\n> +{\n> + int result;\n> + struct argv_array cmd = ARGV_ARRAY_INIT;\n> + argv_array_pushl(&cmd, \"multi-pack-index\", \"repack\",\n> + \"--no-progress\", \"--batch-size=0\", NULL);\n> + result = run_command_v_opt(cmd.argv, RUN_GIT_CMD);\n> +\n> + if (result && multi_pack_index_verify()) {\n> + warning(_(\"multi-pack-index verify failed after repack\"));\n> + result = rewrite_multi_pack_index();\n> + }\n\nIts a bit inconsistent where write() and expire() did not include\nverify() within them\nbut repack does. What make repack() different?\n\n> +\n> + return result;\n> +}\n> +\n> +static int run_pack_files_job(void)\n> +{\n> + if (multi_pack_index_write()) {\n> + error(_(\"failed to write multi-pack-index\"));\n> + return 1;\n> + }\n> +\n> + if (multi_pack_index_verify()) {\n> + warning(_(\"multi-pack-index verify failed after initial write\"));\n> + return rewrite_multi_pack_index();\n> + }\n> +\n> + if (multi_pack_index_expire()) {\n> + error(_(\"multi-pack-index expire failed\"));\n> + return 1;\n> + }\n\nWhy expire *before* repack and not after?\n\nI thought `core.multiPackIndex=true` would prevent old pack files from\nbeing used thus expiring immediately after repack is safe? (on that\nnote, are users\nrequired to set this config prior to running the job?)\n\nIf expiring immediately after repack()+verify() is not safe, then should\nwe have a minimum allowed interval set? (although I would preferred to\nmake expire() safe)\n\n> +\n> + if (multi_pack_index_verify()) {\n> + warning(_(\"multi-pack-index verify failed after expire\"));\n> + return rewrite_multi_pack_index();\n> + }\n> +\n> + if (multi_pack_index_repack()) {\n> + error(_(\"multi-pack-index repack failed\"));\n> + return 1;\n> + }\n\nAgain, I just think the decision to include verify() inside repack()\nmade this part a bit inconsistent.\n\n> +\n> + return 0;\n> +}\n> +\n\nCheers,\nSon Luong\n"},{"id":"396903","messageId":"db35867f-bb33-1155-170f-30a895948733@gmail.com","threadId":"53372","inReplyTo":"CAL3xRKfcKh=XxGso4DTRfJMAVMtwqyQkO0VUhqVuZUr_aQ5f+A@mail.gmail.com","subject":"Re: [PATCH 05/15] run-job: implement pack-files job","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2020-05-04T14:34:17Z","receivedAt":"2020-05-04T14:34:24Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 5/2/2020 3:56 AM, Son Luong Ngoc wrote:\n> Hi Derrick,\n> \n> Sorry for another late reply on this RFC.\n> This time is a question on the multi-pack-index repack process.\n> \n> Why expire *before* repack and not after?\n> \n> I thought `core.multiPackIndex=true` would prevent old pack files from\n> being used thus expiring immediately after repack is safe? (on that\n> note, are users\n> required to set this config prior to running the job?)\n> \n> If expiring immediately after repack()+verify() is not safe, then should\n> we have a minimum allowed interval set? (although I would preferred to\n> make expire() safe)\n\nSuppose we run \"repack\" and then \"expire\".\n\nSuppose that there is a concurrent Git process that has a read handle\non the multi-pack-index from before the repack. This multi-pack-index\nhas a pack-file that was repacked by the \"repack\" command, and hence\nwas expired and deleted by the \"expire\" command (because all of its\nobjects are in the _new_ pack). However, when the Git process with the\nold multi-pack-index reads an object pointing to that pack-file, it\nfinds that the pack does not exist when it tries to load it.\n\nGit is usually pretty resilient to these kinds of things, but it seemed\nprudent to be extra careful here. By spacing out these jobs across a\ntime where we don't expect concurrent Git processes to hold a read handle\non that old multi-pack-index (say, 24 hours) then this method is safe.\n\nIn fact, Scalar ensures that no concurrent Git process is running at the\nstart of the job [1], which means that no Git processes are _still_ running\nsince the last job (but this does not stop concurrent processes from starting\nafter that point and before the 'expire' command).\n\n[1] https://github.com/microsoft/scalar/blob/489764b815dfea32bec5cd4228f2157766012784/Scalar.Common/Maintenance/PackfileMaintenanceStep.cs#L106-L111\n\n>> +\n>> + if (multi_pack_index_verify()) {\n>> + warning(_(\"multi-pack-index verify failed after expire\"));\n>> + return rewrite_multi_pack_index();\n>> + }\n>> +\n>> + if (multi_pack_index_repack()) {\n>> + error(_(\"multi-pack-index repack failed\"));\n>> + return 1;\n>> + }\n> \n> Again, I just think the decision to include verify() inside repack()\n> made this part a bit inconsistent.\n\nYou're right. It is a bit inconsistent. That should be fixed in the\nnext version.\n\nThanks,\n-Stolee\n"}]}