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

From: David Turner <dturner@twopensource.com>
Date: Jul 14, 2015, 04:33 UTC
Message-ID: <1436848402.5074.21.camel@twopensource.com>
In-Reply-To: <559F4A55.1070309@alum.mit.edu>

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

On Fri, 2015-07-10 at 06:30 +0200, Michael Haggerty wrote:

Show 42 quoted lines

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