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

[Derrick Stolee <stolee@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