Re: [PATCH 2/2] files-backend: don't rewrite the `packed-refs` file unnecessarily

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 26, 2017, 06:46 UTC
Message-ID: <xmqqpo9ae79w.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <4c03fc458f619a5f02dd4cae38da8b9b139b850b.1508924577.git.mhagger@alum.mit.edu>

Michael Haggerty <mhagger@alum.mit.edu> writes:

> Even when we are deleting references, we needn't overwrite the
> `packed-refs` file if the references that we are deleting are not
> present in that file. Implement this optimization as follows:

This goal I can understand. files-transaction-prepare may see a deletion and in order to avoid a deletion of a ref from the file-backend to expose a stale entry in the packed-refs file, it prepares a transaction to remove the same ref that might exist in it. If it turns out that there is no such ref in the packed-refs file, then we can leave the packed-refs file as-is without rewriting.

> * Add a function `is_packed_transaction_noop()`, which checks whether
>   a given packed-refs transaction doesn't actually have to do
>   anything. This function must be called while holding the
>   `packed-refs` lock to avoid races.

This checks three things only to cover the most trivial case (I am perfectly happy that it punts on more complex cases). If an update

 - checks the old value,

 - sets a new value, or

 - deletes a ref that is not on the filesystem,

it is not a trivial case. I can understand the latter two (i.e. We are special casing the deletion, and an update with a new value is not. If removing a file from the filesystem is not sufficient to delete, we may have to rewrite the packed-refs), but not the first one. Is it because currently we do not say "delete this ref only when its current value is X"?

Also it is not immediately obvious how the "is this noop" helper is checking the absence of the same-named ref in the current packed-refs file, which is what matters for the correctness of the optimization.