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

[David Turner <dturner@twopensource.com>]

[j6t to bcc as it looks like his concerns have been addressed]

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.

On reflection, I'm not sure my approach makes sense.  The problem is
that refs backends internally manage recursive updates to symbolic refs,
so it is not easy to send update for HEAD to one backend while sending
the corresponding refs/heads/master updates to a different one. 

So I am thinking instead that backends should be required to manage
updates to HEAD themselves, and that some functions from refs-be-files
would be made generic to help with this.  

For the LMDB backend, I could put most of that code at the LMDB access
layer (which presently simply converts all LMDB errors other than
NOT_FOUND to calls to die()).  I would intercept reads and writes to
HEAD and logs/HEAD and replace them with calls to the appropriate
functions.  So, for instance, the LMDB backend would still decide
whether to write HEAD, but it would delegate to the files backend code
to actually write it.  Reflog updates are a bit more complicated,
because we might end up using a different reflog format for LMDB vs for
the files backend, but since all reflog updates are controlled by the
backend, it should still be possible to handle this cleanly.

On reflection, I think that this would also be a reasonable approach to
take for stash, which does not fall into any of the listed categories.
It's not a pseudoref because it's not all-caps and it starts with refs/.
Unlike other pseudorefs, it needs a reflog.  But like HEAD and unlike
other refs, it (and its reflog) wants to be per-worktree. Are there
other ref-like-things in this category (which I'll call "per-worktree
refs")?  I guess one set of these is submodules' HEADs.  I have been
planning on punting on these because it looks to me like that's what the
files backend does.  So, let's leave those aside.  I don't think of any
others but maybe I'm missing something.

I'm also not sure how worktrees handle HEAD's reflog -- there doesn't
seem to be anything worktree-specific in refs.c.  But perhaps I'm just
not understanding that bit of the code.