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@xxxxxxxxxxxxxxxx>
>>> ---
>>> 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@xxxxxxxxxxxx
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html