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

[David Turner <dturner@twopensource.com>]

On Wed, 2015-12-23 at 07:27 +0100, Michael Haggerty wrote:
> > +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.

> >  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?