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

[Junio C Hamano <gitster@pobox.com>]

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

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