Re: [PATCH] refs: cleanup directories when deleting packed ref

From: Jeff King <peff@peff.net>
Date: May 7, 2021, 22:57 UTC
Message-ID: <YJXF4xA0GFx2/2v4@coredump.intra.peff.net>
In-Reply-To: <YJW46fsdKaUv2Fln@coredump.intra.peff.net>

On Fri, May 07, 2021 at 06:02:17PM -0400, Jeff King wrote:

Show 17 quoted lines

> On Fri, May 07, 2021 at 05:56:47PM -0400, Jeff King wrote:
> 
> > > +test_expect_success 'directory not created deleting packed ref' '
> > > +	git branch d1/d2/r1 HEAD &&
> > > +	git pack-refs --all &&
> > > +	test_path_is_missing .git/refs/heads/d1/d2 &&
> > > +	git branch -d d1/d2/r1 &&
> > > +	test_path_is_missing .git/refs/heads/d1/d2 &&
> > > +	test_path_is_missing .git/refs/heads/d1
> > > +'
> > 
> > ...this test passes even without your patch applied. I wonder if there's
> > something else required to trigger the problem.
> 
> If I replace "git branch -d" with "git update-ref -d", then the problem
> does trigger (and your patch does indeed clear it up). I wonder what the
> difference is.

I think this comes down to the interfaces. In update-ref, we call delete_ref(), which creates a transaction to delete the single ref. It realizes the ref is packed and there is no loose file to delete.

Whereas in git-branch, call the plural delete_refs(), which handles the packed and loose stores separately. It first deletes everything from the packed ref store in one go, and then the loose store. And it's actually the deletion from the loose store which gets weird. The ref isn't _anywhere_ at this point. So when we try to read it, we don't set the REF_ISPACKED flag. And thus when it comes time to clean up the loose ref, we say "not packed, so I guess it's worth calling unlink()". Of course that syscall fails, but our unlink_or_msg() wrapper turns ENOENT into success (which is sensible; we want it to be gone, and it is).

And so we think we've deleted a loose ref, and thus call try_remove_empty_parents(), which cleans up the extra directory.

So I'd argue that it's actually delete_refs() which is called from git-branch that is a little confused, or possibly even buggy. But I don't think it's hurting anything, and working around it would probably be awkward and/or inefficient.

Getting back to your patch, though, you are definitely fixing a problem with update-ref (which correctly realizes there is no loose ref to clean up, but forgets that we had to make a lockfile). And the solution you have looks correct. I think you just need to update the test to exercise it with "update-ref -d".

-Peff