Re: [PATCH v7 2/8] cherry-pick: treat CHERRY_PICK_HEAD and REVERT_HEAD as refs

From: Michael Haggerty <mhagger@alum.mit.edu>
Date: Jul 10, 2015, 04:30 UTC
Message-ID: <559F4A55.1070309@alum.mit.edu>
In-Reply-To: <xmqqbnflugsw.fsf@gitster.dls.corp.google.com>

On 07/10/2015 12:06 AM, Junio C Hamano wrote:

Show 25 quoted lines

> David Turner <dturner@twopensource.com> writes:
> 
>> OK, here's my current best idea:
>>
>> 1. A "pseudoref" is an all-caps file in $GIT_DIR/ that always contains
>> at least a SHA1.  CHERRY_PICK_HEAD and REVERT_HEAD are examples. Because
>> HEAD might be a symbolic ref, it is not a pseudoref. 
>>
>> Refs backends do not manage pseudorefs.  Instead, when a pseudoref (an
>> all-caps ref containing no slashes) is requested (e.g. git rev-parse
>> FETCH_HEAD) the generic refs code checks for the existence of that
>> file and if it exists, returns immediately without hitting the backend.
>> The generic code will refuse to allow updates to pseudorefs.
>>
>> 2. The pluggable refs backend manages all refs other than HEAD.
>>
>> 3. The "files" backend always manages HEAD.  This allows for a reflog
>> and for HEAD to be a symbolic ref.
>>
>> The major complication here is ref transactions -- what if there's a
>> transaction that wants to update e.g. both HEAD and refs/heads/master?
> 
> An update to the current branch (e.g. "git commit") does involve at
> least update to the reflog of HEAD, the current branch somewhere in
> refs/heads/ and its log, so it is not "what if" but is a norm [*1*].

The updating of symlink reflogs in general, and particularly that of HEAD, is not done very cleanly. You can see the code in `commit_ref_update()` (some of it helpfully commented to be a "Special hack"):

* If a reference is modified through a symlink, the symlink is locked
rather than the reference itself.
* If a reference is modified directly, and HEAD points at it, then the
HEAD reflog is amended without locking HEAD.

Aside from the lack of proper locking, which could result in races with other processes, we also have the problem that the same reference that is being changed via one of these implicit updates could *also* be being changed directly in the same transaction. Such an update would evade the `ref_update_reject_duplicates()` check.

Previously my thinking was that the locking should be done differently: when the transaction is being processed, extra ref_update records could be created for the extra reference(s) that have to be modified, then these could be handled more straightforwardly. So supposing that HEAD points at refs/heads/master,

* An update of HEAD would be turned into a reflog update and also add a
synthetic update to refs/heads/master.
* An update of refs/heads/master would add a synthetic update to the
HEAD reflog

The first point would obviously apply to any updates via symbolic refs. The second one should too, thought this is a case that we currently punt on to avoid the need to do reverse symbolic ref lookups.

Show 23 quoted lines

>> It may be the case that this never happens; I have not actually audited
>> the code to figure it out.  If someone knows for sure that it does not
>> happen, please say so. But assuming it does happen, here's my idea:
>>
>> If the refs backend is the files backend, we can simply treat HEAD like
>> any other ref.
>>
>> If the refs backend is different, then the refs code needs to hold a
>> files-backend transaction for HEAD, which it will commit immediately
>> after the other transaction succeeds.  We can stick a pointer to the
>> extra transaction in the generic struct ref_transaction, which (as
>> Michael Haggerty suggests) specific backends will extend.
>>
>> A failure to commit either transaction will be reported as a failure,
>> and we'll give an additional inconsistent state warning if the main
>> transaction succeeds but the HEAD transaction fails.
> 
> Yeah, I was thinking along those lines, too.  Thanks for clearly
> writing it down.
> 
>> What do other folks think?
> 
> Me too ;-)

I don't have an answer right now, and I have to get on an airplane in a few hours so I can't think hard about it at the moment. But let me also braindump another vague plan that I have had for a long time: overlayable reference storage schemes. Think of the way that loose refs are currently overlaid on top of packed refs. I think it might be useful to support overlaying more generally.

In this particular case there could be a workspace-local reference storage that only handles HEAD and perhaps some of the other pseudoreferences. That could be overlaid onto loose reference storage (which would then only concern itself with references under "refs/"), which would in turn be overlaid onto packed refs. The workspace-local reference storage layer would have evil special-cased code for dealing with the references that live outside of "refs/".

A `ref_transaction_commit()` would be broken into phases: first each of the stacked backends would be asked to verify that the transaction is possible and acquire any necessary locks, then each backend would get the final "commit" command.

This construct would make it easy for different backends to share the same implementation for HEAD (and potentially other workspace-local) references, by simply layering that one storage mechanism on top of their own.

That would probably be overengineering if it were only used to deal with HEAD, but I think it is a nice general mechanism that could have other applications.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu