On Fri, Mar 3, 2017 at 11:43 PM, Michael Haggerty <mhagger@xxxxxxxxxxxx> wrote: > It's notable that these tests grep around the filesystem, so they won't > be applicable to future refs backends. Of course, "pack-refs" is > intrinsically only applicable to the files backend, so for this test > it's not surprising. But some tests could conceivably be written in a > generic way, so that they should pass for any refs backend. > > Just food for thought; no need to change anything now. I'm a bit on the fence about this. On one hand I think there is room to backend-specific tests (and this one is more about files backend, we just don't have any direct way of getting the backend except through get_main_ref_store()). On the other hand, I can see a need for verifying refs behavior across backends. Submodule backend is unfortunately a bad fit (and probably worktree backend too in early phase) because it cannot fully replace files backend. lmdb does. I guess these tests will have some more restructuring when lmdb joins the party. I imagine we could have something like "ref_expect_success [backend,[backend..]] <title> <body>", which makes it easier to exercise a new backend with the same test, or we could add backend-specific tests as well. Not sure how to do it yet (the devil will be in the body, I think, like dealing with "git -C sub" for submodules). Probably won't do in this series anyway. >> +test_expect_success 'rename_refs(master, new-master)' ' >> + git rev-parse master >expected && >> + $RUN rename-ref refs/heads/master refs/heads/new-master && >> + git rev-parse new-master >actual && >> + test_cmp expected actual && >> + test_commit recreate-master >> +' > > Isn't HEAD set to `refs/heads/master` prior to this test? If so, then I > think it should become detached when you rename `refs/heads/master`. Or > maybe it should be changed to point at `refs/heads/new-master`, I can't > remember. In either case, it might be useful for the test to check that > the behavior matches the status quo, so we notice if the behavior ever > changes inadvertently. You had me worried a bit there, that I broke something. Yes we rename HEAD too. No it's not the backend's job. It's done separately by builtin/branch.c. Probably a good thing because I don't the backend should know about HEAD's special semantics. Front-end might though. >> +test_expect_success 'delete_ref(refs/heads/foo)' ' >> + SHA1=`git rev-parse foo` && >> + git checkout --detach && >> + $RUN delete-ref refs/heads/foo $SHA1 0 && >> + test_must_fail git rev-parse refs/heads/foo -- >> +' > > The last two tests have the same name. > > You might also want to test these functions when the old oid is > incorrect or when the reference is missing. I will. But you probably noticed that I didn't cover refs behavior extensively. I don't know this area well enough to write a conformant test suite. But I think that's ok. We have something to start improving now. -- Duy