Re: [PATCH v3 2/6] notes: replace pseudorefs with real refs

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 28, 2015, 19:44 UTC
Message-ID: <xmqqegjsdq3n.fsf@gitster.dls.corp.google.com>
In-Reply-To: <xmqqpp3cds44.fsf@gitster.dls.corp.google.com>

Junio C Hamano <gitster@pobox.com> writes:

Show 22 quoted lines

> David Turner <dturner@twopensource.com> writes:
>
>> All-caps files like NOTES_MERGE_REF are pseudorefs, and thus are
>> per-worktree.  We don't want multiple notes merges happening at once
>> (in different worktrees), so we want to make these refs true refs.
>> ...
> The two "rev-parse --verify" looks semi-sensible [*1*];
> notes_merge_partial is all lowercase and it refers to
> $GIT_DIR/notes_merge_partial, because they are shared across working
> tree. 
>
> But then why are $GIT_DIR/notes_merge_worktree/* still checked with
> "test -f"?  If they are not refs or ref-like things, why should they
> be downcased?  If they are, why not "rev-parse --verify"?
>
> [Footnote]
>
> *1* I say "semi-" sensible, because it looks ugly.  All ref-like
>     things immediately below $GIT_DIR/ are all-caps by convention.
>     Perhaps it is a better idea to move it under refs/; "everything
>     under refs/ is shared across working trees" is probably a much
>     better rule than "all caps but HEAD is special".

I am not sure if "we don't want multiple notes merges at once" is a valid cause for this inconsistency in the first place. When you try to start merging a notes tree in one place (leaving NOTES_MERGE_REF and friends in the ref storage shared across worktrees), should you be prevented from merging a different notes tree in another? Isn't the whole point of having refs/notes/ hierarchy to allow different notes to hang underneath there, and isn't NOTES_MERGE_REF symref about keeping track of which one of them is being worked on in this working tree?

We don't want multiple usual merges at once in different worktrees, either; rather, we don't want conflicting updates to the same branch, be it done by a merge or a single-parent commit, from different worktrees. And the way we protect ourselves is by forbidding two checkout of the same branch by controlling what HEAD can point at.

Making NOTES_MERGE_REF shared across working trees feels like solving that "no multiple checkouts" problem by making HEAD shared across working trees, ensuring that only one of them can have usable HEAD. Instead, shouldn't we be solving the issue by allowing NOTES_MERGE_REF in multiple working trees point at different refs but ensuring that there is at most one working tree that updates one particular ref in refs/notes/ hierarchy? Can't we learn from (and even better, reuse) the logic that controls HEAD for this purpose?

I think it is OK for us to admit that the "notes" subsystem is not quite ready to work well with multiple working tree world yet [*1*], and move this series forward without worrying about them.

[Footnote]

*1* I am not saying that the notes subsystem can forever stay to be
    second class citizen that does not know about multiple worktrees
    or pluggable ref backends.  But my brief scan of builtin/notes.c
    seems to indicate that there are quite a lot of work to be done
    to prepare it for these two big changes.  My gut feeling is
    that:

    - NOTES_MERGE_REF symref is like HEAD, that is per working tree
      but is controlled across working trees---no two working trees
      can "checkout" the same refs/notes/* tree for conflict
      resolution at the same time.  It should be all-caps and live
      directly under $GIT_DIR/.

    - NOTES_MERGE_PARTIAL is like MERGE_HEAD or the index in that it
      is merely a state of in-progress merge that is private to a
      single working tree.  $GIT_DIR/NOTES_MERGE_PARTIAL is just
      fine, and we do not have to change it.

    - NOTES_MERGE_WORKTREE is a temporary area in the filesystem
      that houses bunch of blob files (i.e. notes contents).  It
      should be per-working tree but it is not even refs.
      $GIT_DIR/NOTES_MERGE_WORKTREE is just fine, and we do not have
      to change it.

    And as to adjusting to the "ref backend with pseudo ref" world,
    I think the code is more-or-less ready, especially because your
    recent round of this series teaches the "pseudorefs private to
    working tree" to update_ref() and delete_ref(), relieving the
    ref API users like notes subsystem from having to worry about
    them.

    So the only potential issue in the notes subsystem is to ensure
    NOTES_MERGE_REF from multiple working trees do not point at the
    same thing at the same time, with a similar protection we have
    for HEAD, I think.