From: Junio C Hamano <gitster@pobox.com>
Date: Wed, 15 Nov 2017 03:14:27 +0000 (+0900)
Subject: Merge branch 'mh/avoid-rewriting-packed-refs'
X-Git-Tag: v2.16.0-rc0~137
X-Git-Url: https://git.lorimer.id.au/gitweb.git/diff_plain/ffb0b5762e903acc146cb21525f3c643de6facce

Merge branch 'mh/avoid-rewriting-packed-refs'

Recent update to the refs infrastructure implementation started
rewriting packed-refs file more often than before; this has been
optimized again for most trivial cases.

* mh/avoid-rewriting-packed-refs:
files-backend: don't rewrite the `packed-refs` file unnecessarily
t1409: check that `packed-refs` is not rewritten unnecessarily
---

ffb0b5762e903acc146cb21525f3c643de6facce
diff --cc refs/packed-backend.c
index 74f1dea0f4,0b0a17ca8e..6650aac1e8
--- a/refs/packed-backend.c
+++ b/refs/packed-backend.c
@@@ -1261,6 -754,100 +1261,100 @@@ error
  	return -1;
  }
  
+ 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:
+ 	 *
+ 	 * 1. It could be that the old value is being verified without
+ 	 *    setting a new value. In this case, we could verify the
+ 	 *    old value here and skip the update if it agrees. If it
+ 	 *    disagrees, we could either let the update go through
+ 	 *    (the actual commit would re-detect and report the
+ 	 *    problem), or come up with a way of reporting such an
+ 	 *    error to *our* caller.
+ 	 *
+ 	 * 2. It could be that a new value is being set, but that it
+ 	 *    is identical to the current packed value of the
+ 	 *    reference.
+ 	 *
+ 	 * Neither of these cases will come up in the current code,
+ 	 * because the only caller of this function passes to it a
+ 	 * transaction that only includes `delete` updates with no
+ 	 * `old_id`. Even if that ever changes, false positives only
+ 	 * cause an optimization to be missed; they do not affect
+ 	 * correctness.
+ 	 */
+ 
+ 	/*
+ 	 * Start with the cheap checks that don't require old
+ 	 * reference values to be read:
+ 	 */
+ 	for (i = 0; i < transaction->nr; i++) {
+ 		struct ref_update *update = transaction->updates[i];
+ 
+ 		if (update->flags & REF_HAVE_OLD)
+ 			/* Have to check the old value -> needed. */
+ 			return 1;
+ 
+ 		if ((update->flags & REF_HAVE_NEW) && !is_null_oid(&update->new_oid))
+ 			/* Have to set a new value -> needed. */
+ 			return 1;
+ 	}
+ 
+ 	/*
+ 	 * The transaction isn't checking any old values nor is it
+ 	 * setting any nonzero new values, so it still might be able
+ 	 * to be skipped. Now do the more expensive check: the update
+ 	 * is needed if any of the updates is a delete, and the old
+ 	 * `packed-refs` file contains a value for that reference.
+ 	 */
+ 	ret = 0;
+ 	for (i = 0; i < transaction->nr; i++) {
+ 		struct ref_update *update = transaction->updates[i];
+ 		unsigned int type;
+ 		struct object_id oid;
+ 
+ 		if (!(update->flags & REF_HAVE_NEW))
+ 			/*
+ 			 * This reference isn't being deleted -> not
+ 			 * needed.
+ 			 */
+ 			continue;
+ 
+ 		if (!refs_read_raw_ref(ref_store, update->refname,
 -				       oid.hash, &referent, &type) ||
++				       &oid, &referent, &type) ||
+ 		    errno != ENOENT) {
+ 			/*
+ 			 * We have to actually delete that reference
+ 			 * -> this transaction is needed.
+ 			 */
+ 			ret = 1;
+ 			break;
+ 		}
+ 	}
+ 
+ 	strbuf_release(&referent);
+ 	return ret;
+ }
+ 
  struct packed_transaction_backend_data {
  	/* True iff the transaction owns the packed-refs lock. */
  	int own_lock;