Re: [PATCH 11/16] refs: move duplicate check to common code

On Wed, 2015-12-23 at 07:27 +0100, Michael Haggerty wrote:

quoted

+static int ref_update_reject_duplicates(struct ref_transaction
*transaction,
+					struct string_list
*refnames,
+					struct strbuf *err)

I like that you extract this code into a function. Though it feels
awkward to have a function called "ref_update_reject_duplicates()"
that
has a side effect of filling the names into a string list. I think it
would feel more natural to call the function get_affected_refnames(),
and treat the duplicate check as an extra bonus.

Renamed, thanks.

quoted

 int ref_transaction_commit(struct ref_transaction *transaction,
 			   struct strbuf *err)
 {
-	return the_refs_backend->transaction_commit(transaction,
err);
+	int ret = -1;
+	struct string_list affected_refnames =
STRING_LIST_INIT_NODUP;
+
+	assert(err);
+
+	if (transaction->state != REF_TRANSACTION_OPEN)
+		die("BUG: commit called for transaction that is
not open");
+
+	if (!transaction->nr) {
+		transaction->state = REF_TRANSACTION_CLOSED;
+		return 0;
+	}
+
+	if (ref_update_reject_duplicates(transaction,
&affected_refnames, err)) {
+		ret = TRANSACTION_GENERIC_ERROR;
+		goto done;
+	}
+
+	ret = the_refs_backend->transaction_commit(transaction,
+						  
 &affected_refnames, err);
+done:
+	string_list_clear(&affected_refnames, 0);
+	return ret;
 }

Here you are avoiding a small amount of code duplication by calling
ref_update_reject_duplicates() here rather than in the backend
-specific
code. But you are doing so at the cost of having to compute
affected_refnames here and pass it (redundantly) to the backend's
transaction_commit function. This increases the coupling between
these
functions, which in my opinion is worse than the small amount of code
duplication. But maybe it's just me.

Hm.  I'm trying to follow the principle that the backends should do as
little as possible (and that the common code should do as much as
possible).  This reduces the cognitive load when writing new backends -
- a new backend author need not know that the list of ref updates must
be unique.

What do others think?