> On 23 Jun 2016, at 09:32, Michael Haggerty <mhagger@xxxxxxxxxxxx> wrote: > > On 06/20/2016 09:57 AM, Lars Schneider wrote: >> >>> On 20 Jun 2016, at 01:51, Junio C Hamano <gitster@xxxxxxxxx> wrote: >>> >>> Junio C Hamano <gitster@xxxxxxxxx> writes: >>> >>>> Michael Haggerty <mhagger@xxxxxxxxxxxx> 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. Although I know git-p4 quite well, I am certainly no expert for the git-p4 branch import. I also pointed out in my previous reply ($gmane/297703) that the git-p4 branch import never worked successfully for me. I always thought the reason is that my P4 branch history is a bit messy. The clean git-p4 branch test cases work fine, though. >>> 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]. Thanks for the pointer! > 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. Agreed! I will post the patch in a few seconds. Cheers, Lars > > Thanks, > Michael > > [1] > https://github.com/git/git/blob/ab7797dbe95fff38d9265869ea367020046db118/refs/files-backend.c#L176-L192 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html