git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Jeff King <peff@peff.net>
To: Will Chandler <wfc@wfchandler.org>
Cc: git@vger.kernel.org, ps@pks.im
Subject: Re: [PATCH] refs: cleanup directories when deleting packed ref
Date: Fri, 7 May 2021 18:57:39 -0400	[thread overview]
Message-ID: <YJXF4xA0GFx2/2v4@coredump.intra.peff.net> (raw)
In-Reply-To: <YJW46fsdKaUv2Fln@coredump.intra.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

  reply	other threads:[~2021-05-07 22:57 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-05-07 14:37 Will Chandler
2021-05-07 21:56 ` Jeff King
2021-05-07 22:02   ` Jeff King
2021-05-07 22:57     ` Jeff King [this message]
2021-05-08  4:27       ` Will Chandler
2021-05-08  5:00         ` Will Chandler
2021-05-08  5:21           ` Bagas Sanjaya
2021-05-08  6:24             ` Junio C Hamano
2021-05-09 18:45               ` Will Chandler
2021-05-10  1:15                 ` Junio C Hamano
2021-05-11  1:35           ` Jeff King
2021-05-11  4:58             ` Junio C Hamano

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=YJXF4xA0GFx2/2v4@coredump.intra.peff.net \
    --to=peff@peff.net \
    --cc=git@vger.kernel.org \
    --cc=ps@pks.im \
    --cc=wfc@wfchandler.org \
    --subject='Re: [PATCH] refs: cleanup directories when deleting packed ref' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).