Re: What's cooking in git.git (Jun 2016, #05; Thu, 16)

[Michael Haggerty <mhagger@alum.mit.edu>]

On 06/20/2016 09:57 AM, Lars Schneider wrote:
> 
>> On 20 Jun 2016, at 01:51, Junio C Hamano <gitster@pobox.com> wrote:
>>
>> Junio C Hamano <gitster@pobox.com> writes:
>>
>>> Michael Haggerty <mhagger@alum.mit.edu> writes:
>>>
>>>> According to [1], something in that test seems to have been trying to run
>>>>
>>>>    git update-ref -d git-p4-tmp/6
>>>>
>>>> Similarly in the other failed test.
>>>
>>> Ah, OK, that would try mucking with .git/git-p4-tmp/6 but that is
>>> not a place to have a ref.  It will not participate in reachability
>>> analysis and will end up losing the referents.
>>>
>>> Perhaps placing it under refs/git-p4-tmp would fix it (both in
>>> git-p4 and in tests)?

I have to say, I'm still confused about how the old code could have
worked at all. For quite some time, Git hasn't allowed references with
weird names like `git-p4-tmp/6` to be created, so how could there be any
references there that need to be deleted? It seems to me that one of the
following must be true:

* This feature (i.e., whatever needs to create the temporary
  references) has been broken for a long time, which would imply that
  there are no tests for it.

* The references are created in some bogus way, like writing files
  directly to the filesystem, rather than using `git update-ref`.
  This is broken and will be even more broken soon when we add
  support for different reference storage backends.

* Such references are never actually created and never actually
  needed. This would imply that the cleanup code is unnecessary.

* The temporary references are created using `git branch`, and are
  thus actually named like `refs/heads/git-p4-tmp/6`. In this case
  the deletion code wasn't cleaning them up right, but the feature
  might have otherwise worked correctly.

* I'm wrong about Git not allowing references like `git-p4-tmp/6` to
  be created, in which case I'd like to know what code path allows it
  so that I can fix it.

Any of these possibilities would mean that your proposed fix is wrong or
incomplete.

>> Oh, another thing.  If these refs are meant to be transient, they
>> are likely to be per worktree, if "git worktree" managed multiple
>> worktrees that share the same set of branches and tags are in use.
>>
>> I recall we carved out one hierarchy under refs/ as "not shared
>> across worktrees" (was that refs/worktree/ hierarchy?  I didn't
>> check but please do so when the patch actually is written), and
>> that hierarchy is the appropriate thing to use for this, I think.
> 
> Thanks for the hint. It looks like as if the "per worktree" decision
> is made in "ref.c:466" "is_per_worktree_ref":
> https://github.com/git/git/blob/3dc84b0c19932ec9947ca4936b6bfd6421ccb1b4/refs.c#L466-L470
> 
> In ce414b3 "refs/bisect" was added to a list of prefixes that are
> per worktree. I could easily add "refs/git-p4-tmp" to this list but
> I don't think that would be a good solution. I would prefer to go with 
> your suggestion and add "refs/worktree" to the prefix list and then use
> "refs/worktree/git-p4-tmp".
> 
> Later on we could move "refs/bisect" to "refs/worktree/bisect" and
> simplify the "is_per_worktree_ref" code, too.

The other part of making `refs/bisect` per-worktree is this horrible
hack here [1].

I'd like to request that the change for the p4 temprefs be made in two
steps:

1. Write references to `refs/git-p4-tmp` or whatever, without
   worrying about making them per-worktree.

2. Carve out a per-worktree namespace, bikeshed about its name and
   semantics, make sure it works correctly, then finally move the
   p4 temporary references and eventually the bisect references there.

The reason is that step 1 is low-risk, could be made quickly, and is
enough to unblock mh/split-under-lock and the other patch series that
depend on it. Step 2, on the other hand, could take quite a while, and
its implementation might benefit from changes to reference handling that
are in the pipeline (e.g., perhaps the horrible hack can be dispensed
with). Meanwhile, as far as I understand these references are transient,
so there are no backwards-compatibility considerations that make
renaming them twice more cumbersome than renaming them once.

Thanks,
Michael

[1]
https://github.com/git/git/blob/ab7797dbe95fff38d9265869ea367020046db118/refs/files-backend.c#L176-L192