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

> On 23 Jun 2016, at 09:32, Michael Haggerty <mhagger@alum.mit.edu> wrote:
> 
> 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