Re: [PATCH v4 14/21] refs: always handle non-normal refs in files backend

[David Turner <dturner@twopensource.com>]

On Thu, 2016-02-18 at 13:07 +0100, Michael Haggerty wrote:
> On 02/18/2016 03:44 AM, David Turner wrote:
> > On Fri, 2016-02-12 at 16:07 +0100, Michael Haggerty wrote:
> > > On 02/05/2016 08:44 PM, David Turner wrote:
> > > > Always handle non-normal (per-worktree or pseudo) refs in the
> > > > files
> > > > backend instead of alternate backends.
> > > > 
> > > > Sometimes a ref transaction will update both a per-worktree ref
> > > > and
> > > > a
> > > > normal ref.  For instance, an ordinary commit might update
> > > > refs/heads/master and HEAD (or at least HEAD's reflog).
> > > > 
> > > > Updates to normal refs continue to go through the chosen
> > > > backend.
> > > > 
> > > > Updates to non-normal refs are moved to a separate files
> > > > backend
> > > > transaction.
> > > > 
> > > > Signed-off-by: David Turner <dturner@twopensource.com>
> > > > ---
> > > >  refs.c | 81
> > > > +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> > > > +--
> > > >  1 file changed, 79 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/refs.c b/refs.c
> > > > index 227c018..18ba356 100644
> > > > --- a/refs.c
> > > > +++ b/refs.c
> > > > @@ -9,6 +9,11 @@
> > > >  #include "object.h"
> > > >  #include "tag.h"
> > > >  
> > > > +static const char split_transaction_fail_warning[] = N_(
> > > > +	"A ref transaction was split across two refs backends.
> > > >  Part of the "
> > > > +	"transaction succeeded, but then the update to the per
> > > > -worktree refs "
> > > > +	"failed.  Your repository may be in an inconsistent
> > > > state.");
> > > > +
> > > >  /*
> > > >   * We always have a files backend and it is the default.
> > > >   */
> > > > @@ -791,6 +796,13 @@ void ref_transaction_free(struct
> > > > ref_transaction *transaction)
> > > >  	free(transaction);
> > > >  }
> > > >  
> > > > +static void add_update_obj(struct ref_transaction
> > > > *transaction,
> > > > +			   struct ref_update *update)
> > > > +{
> > > > +	ALLOC_GROW(transaction->updates, transaction->nr + 1,
> > > > transaction->alloc);
> > > > +	transaction->updates[transaction->nr++] = update;
> > > > +}
> > > > +
> > > >  static struct ref_update *add_update(struct ref_transaction
> > > > *transaction,
> > > >  				     const char *refname)
> > > >  {
> > > > @@ -798,8 +810,7 @@ static struct ref_update *add_update(struct
> > > > ref_transaction *transaction,
> > > >  	struct ref_update *update = xcalloc(1, sizeof(*update)
> > > > +
> > > > len);
> > > >  
> > > >  	memcpy((char *)update->refname, refname, len); /*
> > > > includes
> > > > NUL */
> > > > -	ALLOC_GROW(transaction->updates, transaction->nr + 1,
> > > > transaction->alloc);
> > > > -	transaction->updates[transaction->nr++] = update;
> > > > +	add_update_obj(transaction, update);
> > > >  	return update;
> > > >  }
> > > >  
> > > > @@ -1217,11 +1228,38 @@ static int dereference_symrefs(struct
> > > > ref_transaction *transaction,
> > > >  	return 0;
> > > >  }
> > > >  
> > > > +/*
> > > > + * Move all non-normal ref updates into a specially-created
> > > > + * files-backend transaction
> > > > + */
> > > > +static int move_abnormal_ref_updates(struct ref_transaction
> > > > *transaction,
> > > > +				     struct ref_transaction
> > > > *files_transaction,
> > > > +				     struct strbuf *err)
> > > > +{
> > > > +	int i;
> > > > +
> > > > +	for (i = 0; i < transaction->nr; i++) {
> > > > +		int last;
> > > > +		struct ref_update *update = transaction
> > > > ->updates[i];
> > > > +
> > > > +		if (ref_type(update->refname) ==
> > > > REF_TYPE_NORMAL)
> > > > +			continue;
> > > > +
> > > > +		last = --transaction->nr;
> > > > +		transaction->updates[i] = transaction
> > > > ->updates[last];
> > > > +		add_update_obj(files_transaction, update);
> > > > +	}
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > 
> > > I think this function is incorrect. The update that was
> > > previously at
> > > transaction->updates[i] never gets checked for abnormality. 
> > 
> > Yes it does; that's the "update" variable that we just checked.
> 
> Sorry, I meant to say "the update that was previously at
> `transaction->updates[transaction->nr - 1]` never gets checked for
> abnormality". Because it gets moved to `transaction->updates[i]`,
> then
> `i` is incremented without checking it.

Oh, right.