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

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 30, 2017, 04:52 UTC
Message-ID: <xmqqzi895jab.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <6004e1dea6af33cb41c523855757aa6b04b912bc.1509181545.git.mhagger@alum.mit.edu>

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

Show 21 quoted lines

> +int is_packed_transaction_needed(struct ref_store *ref_store,
> +				 struct ref_transaction *transaction)
> +{
> +	struct packed_ref_store *refs = packed_downcast(
> +			ref_store,
> +			REF_STORE_READ,
> +			"is_packed_transaction_needed");
> +	struct strbuf referent = STRBUF_INIT;
> +	size_t i;
> +	int ret;
> +
> +	if (!is_lock_file_locked(&refs->lock))
> +		BUG("is_packed_transaction_needed() called while unlocked");
> +
> +	/*
> +	 * We're only going to bother returning false for the common,
> +	 * trivial case that references are only being deleted, their
> +	 * old values are not being checked, and the old `packed-refs`
> +	 * file doesn't contain any of those reference(s). This gives
> +	 * false positives for some other cases that could
> +	 * theoretically be optimized away:

The way I understand "the old file does not contain these references" part of the condition is "if there were any of these refs, removing them from the loose ref storage may expose them, which necessitates us to remove them from the packed-refs (and if there is no loose ref for them, we do noeed to remove them from the packed-refs)---so that definitely is not a no-op".

I was confused by the "is_noop?" version, especially about "do we check the old value?" condition. The above does not help me all that much to reach the same level of understanding as I have for the other condition; sorry.

Is the reason why we know we want to play safe when the caller wants to check the old value because that could cause the transaction to abort if it does not match?

Thanks.