Merge pull request #666 from keybase/strib/delete-from-packed-ref

dotgit: handle refs that exist in both packed-refs and a loose ref file

Author: strib

storage/filesystem/internal/dotgit/dotgit.go

@@ -268,7 +268,7 @@ func (d *DotGit) Refs() ([]*plumbing.Reference, error) {
 		return nil, err
 	}
 
-	if err := d.addRefsFromPackedRefs(&refs); err != nil {
+	if err := d.addRefsFromPackedRefs(&refs, seen); err != nil {
 		return nil, err
 	}
 
@@ -336,7 +336,8 @@ func (d *DotGit) RemoveRef(name plumbing.ReferenceName) error {
 	path := d.fs.Join(".", name.String())
 	_, err := d.fs.Stat(path)
 	if err == nil {
-		return d.fs.Remove(path)
+		err = d.fs.Remove(path)
+		// Drop down to remove it from the packed refs file, too.
 	}
 
 	if err != nil && !os.IsNotExist(err) {
@@ -346,13 +347,18 @@ func (d *DotGit) RemoveRef(name plumbing.ReferenceName) error {
 	return d.rewritePackedRefsWithoutRef(name)
 }
 
-func (d *DotGit) addRefsFromPackedRefs(refs *[]*plumbing.Reference) (err error) {
+func (d *DotGit) addRefsFromPackedRefs(refs *[]*plumbing.Reference, seen map[plumbing.ReferenceName]bool) (err error) {
 	packedRefs, err := d.findPackedRefs()
 	if err != nil {
 		return err
 	}
 
-	*refs = append(*refs, packedRefs...)
+	for _, ref := range packedRefs {
+		if !seen[ref.Name()] {
+			*refs = append(*refs, ref)
+			seen[ref.Name()] = true
+		}
+	}
 	return nil
 }
 
@@ -365,13 +371,30 @@ func (d *DotGit) rewritePackedRefsWithoutRef(name plumbing.ReferenceName) (err e
 
 		return err
 	}
+	doCloseF := true
+	defer func() {
+		if doCloseF {
+			ioutil.CheckClose(f, &err)
+		}
+	}()
+
+	err = f.Lock()
+	if err != nil {
+		return err
+	}
 
 	// Creating the temp file in the same directory as the target file
 	// improves our chances for rename operation to be atomic.
 	tmp, err := d.fs.TempFile("", tmpPackedRefsPrefix)
 	if err != nil {
 		return err
 	}
+	doCloseTmp := true
+	defer func() {
+		if doCloseTmp {
+			ioutil.CheckClose(tmp, &err)
+		}
+	}()
 
 	s := bufio.NewScanner(f)
 	found := false
@@ -397,14 +420,21 @@ func (d *DotGit) rewritePackedRefsWithoutRef(name plumbing.ReferenceName) (err e
 	}
 
 	if !found {
-		return nil
+		doCloseTmp = false
+		ioutil.CheckClose(tmp, &err)
+		if err != nil {
+			return err
+		}
+		// Delete the temp file if nothing needed to be removed.
+		return d.fs.Remove(tmp.Name())
 	}
 
+	doCloseF = false
 	if err := f.Close(); err != nil {
-		ioutil.CheckClose(tmp, &err)
 		return err
 	}
 
+	doCloseTmp = false
 	if err := tmp.Close(); err != nil {
 		return err
 	}

storage/filesystem/internal/dotgit/dotgit_test.go

@@ -184,6 +184,46 @@ func (s *SuiteDotGit) TestRemoveRefFromPackedRefs(c *C) {
 		"e8d3ffab552895c19b9fcf7aa264d277cde33881 refs/remotes/origin/branch\n")
 }
 
+func (s *SuiteDotGit) TestRemoveRefFromReferenceFileAndPackedRefs(c *C) {
+	fs := fixtures.Basic().ByTag(".git").One().DotGit()
+	dir := New(fs)
+
+	// Make a ref file for a ref that's already in `packed-refs`.
+	err := dir.SetRef(plumbing.NewReferenceFromStrings(
+		"refs/remotes/origin/branch",
+		"e8d3ffab552895c19b9fcf7aa264d277cde33881",
+	))
+
+	// Make sure it only appears once in the refs list.
+	refs, err := dir.Refs()
+	c.Assert(err, IsNil)
+	found := false
+	for _, ref := range refs {
+		if ref.Name() == "refs/remotes/origin/branch" {
+			c.Assert(found, Equals, false)
+			found = true
+		}
+	}
+
+	name := plumbing.ReferenceName("refs/remotes/origin/branch")
+	err = dir.RemoveRef(name)
+	c.Assert(err, IsNil)
+
+	b, err := ioutil.ReadFile(filepath.Join(fs.Root(), packedRefsPath))
+	c.Assert(err, IsNil)
+
+	c.Assert(string(b), Equals, ""+
+		"# pack-refs with: peeled fully-peeled \n"+
+		"6ecf0ef2c2dffb796033e5a02219af86ec6584e5 refs/heads/master\n"+
+		"6ecf0ef2c2dffb796033e5a02219af86ec6584e5 refs/remotes/origin/master\n")
+
+	refs, err = dir.Refs()
+	c.Assert(err, IsNil)
+
+	ref := findReference(refs, string(name))
+	c.Assert(ref, IsNil)
+}
+
 func (s *SuiteDotGit) TestRemoveRefNonExistent(c *C) {
 	fs := fixtures.Basic().ByTag(".git").One().DotGit()
 	dir := New(fs)