Bug: Branch Deletion Doesn't Clean Up

Bug: Branch Deletion Doesn't Clean Up

[Karl Moskowski]

Apologies if this is a known issue; I was unable to find anything similar in the archive.

The problem is, deleting a branch whose name contains slashes doesn’t delete the directories in .git/refs/heads/.

I’ve tried both git 2.5.4 and 2.6.4 on OS X 10.11 with HFS+ (case-insensitive, case-preserving). Here’s the sequence of commands:

% mkdir Foobar
% cd Foobar/
% git init
% git checkout -b master
% touch file.txt
% git add file.txt
% git commit -m "add a file"
% git checkout -b another/branch

At this point, there's file .git/refs/heads/another/branch

% git checkout master
% git branch -d another/branch

At this point, there's an empty directory .git/refs/heads/another/. Only the “leaf” file noted above is deleted, not the directory above it.

% git checkout -b Another/branch

At this point, the file .git/refs/heads/another/branch is re-created. Note the case of its enclosing directory.

% git status
On branch Another/branch
nothing to commit, working directory clean

% git branch -l
  another/branch
  master

Note the difference in case between the last two commands' output. In addition, there's no asterisk next to another/branch indicating it's current.

Because the first branch deletion didn't also delete the enclosing directory tree below .git/refs/heads/, the new branch gets created beneath it. However, there's inconsistent info between the file system and the git database.

It seems like git branch -d ascend the hierarchy (up to .git/refs/heads/), deleting any empty directories.

----
Karl Moskowski <kmoskowski@me.com>
<https://about.me/kolpanic>

Re: Bug: Branch Deletion Doesn't Clean Up

[Jeff King]

On Thu, Jan 07, 2016 at 02:24:41PM -0500, Karl Moskowski wrote:

> The problem is, deleting a branch whose name contains slashes doesn’t
> delete the directories in .git/refs/heads/.

Yes. Git's model is to leave the directories in place, and remove them
only when they get in the way of creating another ref. In theory, the
effect is the same as deleting the directories proactively.

But as you noticed, it does funny things with case-preserving
filesystems. It also can cause performance problems if you have a very
large number of empty directories (because git has to open each of them
just to find out they're empty).

We can into the latter case at GitHub. Michael Haggerty (cc'd) worked up
some patches recently for this, but I don't now if they're yet polished
enough to send upstream.

> It seems like git branch -d ascend the hierarchy (up to
> .git/refs/heads/), deleting any empty directories.

Yes, though it needs to be coupled with making the branch-creation
process more robust to races (since we might create "refs/heads/foo" in
order to make "refs/heads/foo/bar" while somebody else is deleting it to
get rid of "refs/heads/foo/baz").

-Peff

Re: Bug: Branch Deletion Doesn't Clean Up

[Jeff King]

[resend; sorry, I forgot to cc Michael on the first one]

On Thu, Jan 07, 2016 at 02:24:41PM -0500, Karl Moskowski wrote:

> The problem is, deleting a branch whose name contains slashes doesn’t
> delete the directories in .git/refs/heads/.

Yes. Git's model is to leave the directories in place, and remove them
only when they get in the way of creating another ref. In theory, the
effect is the same as deleting the directories proactively.

But as you noticed, it does funny things with case-preserving
filesystems. It also can cause performance problems if you have a very
large number of empty directories (because git has to open each of them
just to find out they're empty).

We can into the latter case at GitHub. Michael Haggerty (cc'd) worked up
some patches recently for this, but I don't now if they're yet polished
enough to send upstream.

> It seems like git branch -d ascend the hierarchy (up to
> .git/refs/heads/), deleting any empty directories.

Yes, though it needs to be coupled with making the branch-creation
process more robust to races (since we might create "refs/heads/foo" in
order to make "refs/heads/foo/bar" while somebody else is deleting it to
get rid of "refs/heads/foo/baz").

-Peff

Re: Bug: Branch Deletion Doesn't Clean Up

[Mike Hommey]

On Thu, Feb 04, 2016 at 02:55:00AM -0500, Jeff King wrote:
> [resend; sorry, I forgot to cc Michael on the first one]
> 
> On Thu, Jan 07, 2016 at 02:24:41PM -0500, Karl Moskowski wrote:
> 
> > The problem is, deleting a branch whose name contains slashes doesn’t
> > delete the directories in .git/refs/heads/.
> 
> Yes. Git's model is to leave the directories in place, and remove them
> only when they get in the way of creating another ref. In theory, the
> effect is the same as deleting the directories proactively.
> 
> But as you noticed, it does funny things with case-preserving
> filesystems. It also can cause performance problems if you have a very
> large number of empty directories (because git has to open each of them
> just to find out they're empty).
> 
> We can into the latter case at GitHub. Michael Haggerty (cc'd) worked up
> some patches recently for this, but I don't now if they're yet polished
> enough to send upstream.
> 
> > It seems like git branch -d ascend the hierarchy (up to
> > .git/refs/heads/), deleting any empty directories.
> 
> Yes, though it needs to be coupled with making the branch-creation
> process more robust to races (since we might create "refs/heads/foo" in
> order to make "refs/heads/foo/bar" while somebody else is deleting it to
> get rid of "refs/heads/foo/baz").

Can't we come up with a system that would update packed-refs directly
instead of creating files?

Mike

Re: Bug: Branch Deletion Doesn't Clean Up

[Jeff King]

On Thu, Feb 04, 2016 at 05:12:20PM +0900, Mike Hommey wrote:

> > > It seems like git branch -d ascend the hierarchy (up to
> > > .git/refs/heads/), deleting any empty directories.
> > 
> > Yes, though it needs to be coupled with making the branch-creation
> > process more robust to races (since we might create "refs/heads/foo" in
> > order to make "refs/heads/foo/bar" while somebody else is deleting it to
> > get rid of "refs/heads/foo/baz").
> 
> Can't we come up with a system that would update packed-refs directly
> instead of creating files?

There are a few reasons not to:

  - it breaks backwards compatibility (unless we continue to create the
    directory in order to put the dot-lock in it, but then I don't think
    we've gained anything)

  - the usual update method for packed-refs is to take a dot-lock, do a
    whole-file update, and then atomically rename into place.  That
    makes writing a ref O(# of refs) instead of O(1), and increases lock
    contention on the packed-refs file.

  - if we abandon atomic renames as the update mechanism and just update
    in place via lseek/write, then we need read-locking, or we need to
    hope that a reader will never see a sheared write

But if we're willing to break compatibility, we should ditch packed-refs
entirely and move to a _real_ concurrent database. And there is work
underway already to do that (see David Turner's ref-backend-lmdb
series).

-Peff

Re: Bug: Branch Deletion Doesn't Clean Up

[Mike Hommey]

On Thu, Feb 04, 2016 at 03:17:59AM -0500, Jeff King wrote:
> On Thu, Feb 04, 2016 at 05:12:20PM +0900, Mike Hommey wrote:
> 
> > > > It seems like git branch -d ascend the hierarchy (up to
> > > > .git/refs/heads/), deleting any empty directories.
> > > 
> > > Yes, though it needs to be coupled with making the branch-creation
> > > process more robust to races (since we might create "refs/heads/foo" in
> > > order to make "refs/heads/foo/bar" while somebody else is deleting it to
> > > get rid of "refs/heads/foo/baz").
> > 
> > Can't we come up with a system that would update packed-refs directly
> > instead of creating files?
> 
> There are a few reasons not to:
> 
>   - it breaks backwards compatibility (unless we continue to create the
>     directory in order to put the dot-lock in it, but then I don't think
>     we've gained anything)

Is that the kind of backwards compatibility that matters, though? I
mean, I won't claim to know all the internals of how refs are used, but
you sound like the theoretical incompatibility would be two different
versions of git racing for update-ref on the same local repository.

Not that it would change anything about the other reasons below.

>   - the usual update method for packed-refs is to take a dot-lock, do a
>     whole-file update, and then atomically rename into place.  That
>     makes writing a ref O(# of refs) instead of O(1), and increases lock
>     contention on the packed-refs file.
> 
>   - if we abandon atomic renames as the update mechanism and just update
>     in place via lseek/write, then we need read-locking, or we need to
>     hope that a reader will never see a sheared write
> 
> But if we're willing to break compatibility, we should ditch packed-refs
> entirely and move to a _real_ concurrent database. And there is work
> underway already to do that (see David Turner's ref-backend-lmdb
> series).

Mike

Re: Bug: Branch Deletion Doesn't Clean Up

[Jeff King]

On Thu, Feb 04, 2016 at 05:26:13PM +0900, Mike Hommey wrote:

> > There are a few reasons not to:
> > 
> >   - it breaks backwards compatibility (unless we continue to create the
> >     directory in order to put the dot-lock in it, but then I don't think
> >     we've gained anything)
> 
> Is that the kind of backwards compatibility that matters, though? I
> mean, I won't claim to know all the internals of how refs are used, but
> you sound like the theoretical incompatibility would be two different
> versions of git racing for update-ref on the same local repository.

Yes, that's true. It's a lesser breakage than "you cannot go back to the
old version at all".

-Peff