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

[Jeff King <peff@peff.net>]

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

> 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