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

[Michael Haggerty <mhagger@alum.mit.edu>]

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.

> [...]
>> Another alternative would be to set
>>
>>     update->flags |= REF_ABNORMAL
>> [...]
> I'm also interested in this idea. Perhaps it would also be nice to
> report *why* they fail (e.g. the conflicting ref name).  I did a
> variant of this with for the journal code, but my way of doing it
> turned out to be a bad idea (long story).  But I want to stay focused
> on the simplest thing possible, for now.

Fair enough.

> [...]
>> Does initial_ref_transaction_commit() need the same treatment?
> 
> We only use that for remote refs -- I'm not sure if those can be
> symrefs.  Wouldn't hurt.

Hmmm, good point. I think the only symrefs that can be set during a
clone are `HEAD` and `refs/remotes/origin/HEAD`. But I guess that no
other references are updated *through* these symrefs, so it's probably
OK. I haven't checked carefully, though.

> [...]

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu