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

2 messages from 2020-05-02 to 2020-05-04. Participants: Son Luong Ngoc, Derrick Stolee.
Thread: https://gitlist.dev/t/53372

## Son Luong Ngoc, 2020-05-02 07:56

Subject: Re: [PATCH 05/15] run-job: implement pack-files job
Message-ID: <CAL3xRKfcKh=XxGso4DTRfJMAVMtwqyQkO0VUhqVuZUr_aQ5f+A@mail.gmail.com>
URL: https://gitlist.dev/e/CAL3xRKfcKh%3DXxGso4DTRfJMAVMtwqyQkO0VUhqVuZUr_aQ5f%2BA%40mail.gmail.com

```
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

...

> +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?

> +
> + 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)

> +
> + 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, 2020-05-04 14:34

Subject: Re: [PATCH 05/15] run-job: implement pack-files job
Message-ID: <db35867f-bb33-1155-170f-30a895948733@gmail.com>
URL: https://gitlist.dev/e/db35867f-bb33-1155-170f-30a895948733%40gmail.com
In-Reply-To: <CAL3xRKfcKh=XxGso4DTRfJMAVMtwqyQkO0VUhqVuZUr_aQ5f+A@mail.gmail.com>

```
On 5/2/2020 3:56 AM, Son Luong Ngoc wrote:
> 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

>> +
>> + 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

```