From c8ca7e3d031214b6c0478d62119dfb8a9af1631d Mon Sep 17 00:00:00 2001 From: Jeremy Stribling Date: Thu, 12 Oct 2017 17:08:05 -0700 Subject: dotgit: remove loose ref AND packed ref, if both exist Issue: KBFS-2509 --- storage/filesystem/internal/dotgit/dotgit.go | 31 +++++++++++++++++++++++++--- 1 file changed, 28 insertions(+), 3 deletions(-) (limited to 'storage/filesystem/internal') diff --git a/storage/filesystem/internal/dotgit/dotgit.go b/storage/filesystem/internal/dotgit/dotgit.go index 0c9cd33..43d0e2d 100644 --- a/storage/filesystem/internal/dotgit/dotgit.go +++ b/storage/filesystem/internal/dotgit/dotgit.go @@ -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) { @@ -365,6 +366,17 @@ 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. @@ -372,6 +384,12 @@ func (d *DotGit) rewritePackedRefsWithoutRef(name plumbing.ReferenceName) (err e if err != nil { return err } + doCloseTmp := true + defer func() { + if doCloseTmp { + ioutil.CheckClose(tmp, &err) + } + }() s := bufio.NewScanner(f) found := false @@ -397,14 +415,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 } -- cgit From 129ff16f8686bb74a206cf58000de1d9640e370a Mon Sep 17 00:00:00 2001 From: Jeremy Stribling Date: Mon, 27 Nov 2017 16:06:44 -0800 Subject: dotgit: add a test for removing a ref from a file and packed-refs --- storage/filesystem/internal/dotgit/dotgit_test.go | 29 +++++++++++++++++++++++ 1 file changed, 29 insertions(+) (limited to 'storage/filesystem/internal') diff --git a/storage/filesystem/internal/dotgit/dotgit_test.go b/storage/filesystem/internal/dotgit/dotgit_test.go index 7d327c8..913622a 100644 --- a/storage/filesystem/internal/dotgit/dotgit_test.go +++ b/storage/filesystem/internal/dotgit/dotgit_test.go @@ -184,6 +184,35 @@ 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", + )) + + 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) -- cgit From 9c80677ec0d1778e6d304b235a22f4e636322e74 Mon Sep 17 00:00:00 2001 From: Jeremy Stribling Date: Mon, 27 Nov 2017 16:21:44 -0800 Subject: dotgit: don't list references twice Restore the `seen` map that avoided listing packed-refs twice. --- storage/filesystem/internal/dotgit/dotgit.go | 11 ++++++++--- storage/filesystem/internal/dotgit/dotgit_test.go | 13 ++++++++++++- 2 files changed, 20 insertions(+), 4 deletions(-) (limited to 'storage/filesystem/internal') diff --git a/storage/filesystem/internal/dotgit/dotgit.go b/storage/filesystem/internal/dotgit/dotgit.go index 43d0e2d..8e5381c 100644 --- a/storage/filesystem/internal/dotgit/dotgit.go +++ b/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 } @@ -347,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 } diff --git a/storage/filesystem/internal/dotgit/dotgit_test.go b/storage/filesystem/internal/dotgit/dotgit_test.go index 913622a..a775536 100644 --- a/storage/filesystem/internal/dotgit/dotgit_test.go +++ b/storage/filesystem/internal/dotgit/dotgit_test.go @@ -194,6 +194,17 @@ func (s *SuiteDotGit) TestRemoveRefFromReferenceFileAndPackedRefs(c *C) { "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) @@ -206,7 +217,7 @@ func (s *SuiteDotGit) TestRemoveRefFromReferenceFileAndPackedRefs(c *C) { "6ecf0ef2c2dffb796033e5a02219af86ec6584e5 refs/heads/master\n"+ "6ecf0ef2c2dffb796033e5a02219af86ec6584e5 refs/remotes/origin/master\n") - refs, err := dir.Refs() + refs, err = dir.Refs() c.Assert(err, IsNil) ref := findReference(refs, string(name)) -- cgit