threads / patch / 53372

patch, 15 partsRe: [PATCH 05/15] run-job: implement pack-files job

Subject: Re: [PATCH 05/15] run-job: implement pack-files job

## tl;dr

2 messages between May 2, 2020 and May 4, 2020. Diffs are folded; open one to read it.

replies: 1people: 2as markdown or json

Son Luong Ngoc· May 2, 2020, 07:56 UTC · lore
Hi Derrick,

Sorry for another late reply on this RFC. This time is a question on the multi-pack-index repack process.

"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
> diff --git a/builtin/run-job.c b/builtin/run-job.c
> index cecf9058c51..d3543f7ccb9 100644
> --- a/builtin/run-job.c
> +++ b/builtin/run-job.c
...
Show 12 quoted lines
> +static int multi_pack_index_repack(void)
> +{
> + int result;
> + struct argv_array cmd = ARGV_ARRAY_INIT;
> + argv_array_pushl(&cmd, "multi-pack-index", "repack",
> + "--no-progress", "--batch-size=0", NULL);
> + result = run_command_v_opt(cmd.argv, RUN_GIT_CMD);
> +
> + if (result && multi_pack_index_verify()) {
> + warning(_("multi-pack-index verify failed after repack"));
> + result = rewrite_multi_pack_index();
> + }

Its a bit inconsistent where write() and expire() did not include verify() within them but repack does. What make repack() different?

Show 20 quoted lines
> +
> + return result;
> +}
> +
> +static int run_pack_files_job(void)
> +{
> + if (multi_pack_index_write()) {
> + error(_("failed to write multi-pack-index"));
> + return 1;
> + }
> +
> + if (multi_pack_index_verify()) {
> + warning(_("multi-pack-index verify failed after initial write"));
> + return rewrite_multi_pack_index();
> + }
> +
> + if (multi_pack_index_expire()) {
> + error(_("multi-pack-index expire failed"));
> + return 1;
> + }
Why expire *before* repack and not after?

I thought `core.multiPackIndex=true` would prevent old pack files from being used thus expiring immediately after repack is safe? (on that note, are users required to set this config prior to running the job?)

If expiring immediately after repack()+verify() is not safe, then should we have a minimum allowed interval set? (although I would preferred to make expire() safe)

Show 10 quoted lines
> +
> + if (multi_pack_index_verify()) {
> + warning(_("multi-pack-index verify failed after expire"));
> + return rewrite_multi_pack_index();
> + }
> +
> + if (multi_pack_index_repack()) {
> + error(_("multi-pack-index repack failed"));
> + return 1;
> + }

Again, I just think the decision to include verify() inside repack() made this part a bit inconsistent.

> +
> + return 0;
> +}
> +

Cheers, Son Luong

Derrick Stolee· May 4, 2020, 14:34 UTC · re: Son Luong Ngoc · lore
On 5/2/2020 3:56 AM, Son Luong Ngoc wrote:
Show 15 quoted lines
> Hi Derrick,
> 
> Sorry for another late reply on this RFC.
> This time is a question on the multi-pack-index repack process.
> 
> Why expire *before* repack and not after?
> 
> I thought `core.multiPackIndex=true` would prevent old pack files from
> being used thus expiring immediately after repack is safe? (on that
> note, are users
> required to set this config prior to running the job?)
> 
> If expiring immediately after repack()+verify() is not safe, then should
> we have a minimum allowed interval set? (although I would preferred to
> make expire() safe)
Suppose we run "repack" and then "expire".

Suppose that there is a concurrent Git process that has a read handle on the multi-pack-index from before the repack. This multi-pack-index has a pack-file that was repacked by the "repack" command, and hence was expired and deleted by the "expire" command (because all of its objects are in the _new_ pack). However, when the Git process with the old multi-pack-index reads an object pointing to that pack-file, it finds that the pack does not exist when it tries to load it.

Git is usually pretty resilient to these kinds of things, but it seemed prudent to be extra careful here. By spacing out these jobs across a time where we don't expect concurrent Git processes to hold a read handle on that old multi-pack-index (say, 24 hours) then this method is safe.

In fact, Scalar ensures that no concurrent Git process is running at the start of the job [1], which means that no Git processes are _still_ running since the last job (but this does not stop concurrent processes from starting after that point and before the 'expire' command).

[1] https://github.com/microsoft/scalar/blob/489764b815dfea32bec5cd4228f2157766012784/Scalar.Common/Maintenance/PackfileMaintenanceStep.cs#L106-L111
Show 13 quoted lines
>> +
>> + if (multi_pack_index_verify()) {
>> + warning(_("multi-pack-index verify failed after expire"));
>> + return rewrite_multi_pack_index();
>> + }
>> +
>> + if (multi_pack_index_repack()) {
>> + error(_("multi-pack-index repack failed"));
>> + return 1;
>> + }
> 
> Again, I just think the decision to include verify() inside repack()
> made this part a bit inconsistent.

You're right. It is a bit inconsistent. That should be fixed in the next version.

Thanks, -Stolee

← back to recent threads