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

[David Turner <dturner@twopensource.com>]

On Fri, 2015-07-10 at 06:30 +0200, Michael Haggerty wrote:
> On 07/10/2015 12:06 AM, Junio C Hamano wrote:
> > 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.

All of this is worth fixing, but I don't know that it needs to be fixed
before ref backends hit.  What do you think?

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

Interesting concept.  I think the semantics could get rather
complicated, but maybe it's worth thinking about.

But for now, I think it would be better to special-case pseudorefs, with
the option to expand that to full layering later if we see a need.