From 3834e571da171844efecc4e26fd89082419079a1 Mon Sep 17 00:00:00 2001 From: Taru Karttunen Date: Tue, 12 Sep 2017 19:06:47 +0300 Subject: Use optionally locking when updating refs --- storage/filesystem/internal/dotgit/dotgit.go | 62 +++++++++++++++++++---- storage/filesystem/internal/dotgit/dotgit_test.go | 6 +-- storage/filesystem/reference.go | 6 ++- 3 files changed, 61 insertions(+), 13 deletions(-) (limited to 'storage/filesystem') diff --git a/storage/filesystem/internal/dotgit/dotgit.go b/storage/filesystem/internal/dotgit/dotgit.go index 0c9cd33..e60ffca 100644 --- a/storage/filesystem/internal/dotgit/dotgit.go +++ b/storage/filesystem/internal/dotgit/dotgit.go @@ -5,6 +5,7 @@ import ( "bufio" "errors" "fmt" + "io" stdioutil "io/ioutil" "os" "strings" @@ -239,7 +240,39 @@ func (d *DotGit) Object(h plumbing.Hash) (billy.File, error) { return d.fs.Open(file) } -func (d *DotGit) SetRef(r *plumbing.Reference) error { +func (d *DotGit) readReferenceFrom(rd io.Reader, name string) (ref *plumbing.Reference, err error) { + b, err := stdioutil.ReadAll(rd) + if err != nil { + return nil, err + } + + line := strings.TrimSpace(string(b)) + return plumbing.NewReferenceFromStrings(name, line), nil +} + +func (d *DotGit) checkReferenceAndTruncate(f billy.File, old *plumbing.Reference) error { + if old == nil { + return nil + } + ref, err := d.readReferenceFrom(f, old.Name().String()) + if err != nil { + return err + } + if ref.Hash() != old.Hash() { + return fmt.Errorf("reference has changed concurrently") + } + _, err = f.Seek(0, io.SeekStart) + if err != nil { + return err + } + err = f.Truncate(0) + if err != nil { + return err + } + return nil +} + +func (d *DotGit) SetRef(r, old *plumbing.Reference) error { var content string switch r.Type() { case plumbing.SymbolicReference: @@ -248,13 +281,30 @@ func (d *DotGit) SetRef(r *plumbing.Reference) error { content = fmt.Sprintln(r.Hash().String()) } - f, err := d.fs.Create(r.Name().String()) + // If we are not checking an old ref, just truncate the file. + mode := os.O_RDWR | os.O_CREATE + if old == nil { + mode |= os.O_TRUNC + } + + f, err := d.fs.OpenFile(r.Name().String(), mode, 0666) if err != nil { return err } defer ioutil.CheckClose(f, &err) + err = f.Lock() + if err != nil { + return err + } + + // this is a no-op to call even when old is nil. + err = d.checkReferenceAndTruncate(f, old) + if err != nil { + return err + } + _, err = f.Write([]byte(content)) return err } @@ -493,13 +543,7 @@ func (d *DotGit) readReferenceFile(path, name string) (ref *plumbing.Reference, } defer ioutil.CheckClose(f, &err) - b, err := stdioutil.ReadAll(f) - if err != nil { - return nil, err - } - - line := strings.TrimSpace(string(b)) - return plumbing.NewReferenceFromStrings(name, line), nil + return d.readReferenceFrom(f, name) } // Module return a billy.Filesystem poiting to the module folder diff --git a/storage/filesystem/internal/dotgit/dotgit_test.go b/storage/filesystem/internal/dotgit/dotgit_test.go index 7d327c8..119d952 100644 --- a/storage/filesystem/internal/dotgit/dotgit_test.go +++ b/storage/filesystem/internal/dotgit/dotgit_test.go @@ -58,21 +58,21 @@ func (s *SuiteDotGit) TestSetRefs(c *C) { err = dir.SetRef(plumbing.NewReferenceFromStrings( "refs/heads/foo", "e8d3ffab552895c19b9fcf7aa264d277cde33881", - )) + ), nil) c.Assert(err, IsNil) err = dir.SetRef(plumbing.NewReferenceFromStrings( "refs/heads/symbolic", "ref: refs/heads/foo", - )) + ), nil) c.Assert(err, IsNil) err = dir.SetRef(plumbing.NewReferenceFromStrings( "bar", "e8d3ffab552895c19b9fcf7aa264d277cde33881", - )) + ), nil) c.Assert(err, IsNil) refs, err := dir.Refs() diff --git a/storage/filesystem/reference.go b/storage/filesystem/reference.go index 49627d3..54cdf56 100644 --- a/storage/filesystem/reference.go +++ b/storage/filesystem/reference.go @@ -11,7 +11,11 @@ type ReferenceStorage struct { } func (r *ReferenceStorage) SetReference(ref *plumbing.Reference) error { - return r.dir.SetRef(ref) + return r.dir.SetRef(ref, nil) +} + +func (r *ReferenceStorage) CheckAndSetReference(ref, old *plumbing.Reference) error { + return r.dir.SetRef(ref, old) } func (r *ReferenceStorage) Reference(n plumbing.ReferenceName) (*plumbing.Reference, error) { -- cgit From c4766ff1a7227de77bba488a8481f332ea8c1667 Mon Sep 17 00:00:00 2001 From: Taru Karttunen Date: Tue, 19 Sep 2017 14:27:05 +0300 Subject: Document Lock+Close usage --- storage/filesystem/internal/dotgit/dotgit.go | 4 ++++ 1 file changed, 4 insertions(+) (limited to 'storage/filesystem') diff --git a/storage/filesystem/internal/dotgit/dotgit.go b/storage/filesystem/internal/dotgit/dotgit.go index e60ffca..cb3adc8 100644 --- a/storage/filesystem/internal/dotgit/dotgit.go +++ b/storage/filesystem/internal/dotgit/dotgit.go @@ -294,6 +294,10 @@ func (d *DotGit) SetRef(r, old *plumbing.Reference) error { defer ioutil.CheckClose(f, &err) + // Lock is unlocked by the deferred Close above. This is because Unlock + // does not imply a fsync and thus there would be a race between + // Unlock+Close and other concurrent writers. Adding Sync to go-billy + // could work, but this is better (and avoids superfluous syncs). err = f.Lock() if err != nil { return err -- cgit From cbab840ef28888c2e85112b3b48294f7333ec187 Mon Sep 17 00:00:00 2001 From: Jeremy Stribling Date: Tue, 28 Nov 2017 11:14:09 -0800 Subject: dotgit: add CheckAndSetReference tests --- storage/filesystem/internal/dotgit/dotgit_test.go | 21 ++++++++++++++++++--- 1 file changed, 18 insertions(+), 3 deletions(-) (limited to 'storage/filesystem') diff --git a/storage/filesystem/internal/dotgit/dotgit_test.go b/storage/filesystem/internal/dotgit/dotgit_test.go index d1b04a7..446a204 100644 --- a/storage/filesystem/internal/dotgit/dotgit_test.go +++ b/storage/filesystem/internal/dotgit/dotgit_test.go @@ -55,10 +55,11 @@ func (s *SuiteDotGit) TestSetRefs(c *C) { fs := osfs.New(tmp) dir := New(fs) - err = dir.SetRef(plumbing.NewReferenceFromStrings( + firstFoo := plumbing.NewReferenceFromStrings( "refs/heads/foo", "e8d3ffab552895c19b9fcf7aa264d277cde33881", - ), nil) + ) + err = dir.SetRef(firstFoo, nil) c.Assert(err, IsNil) @@ -105,6 +106,20 @@ func (s *SuiteDotGit) TestSetRefs(c *C) { c.Assert(ref, NotNil) c.Assert(ref.Hash().String(), Equals, "e8d3ffab552895c19b9fcf7aa264d277cde33881") + // Check that SetRef with a non-nil `old` works. + err = dir.SetRef(plumbing.NewReferenceFromStrings( + "refs/heads/foo", + "6ecf0ef2c2dffb796033e5a02219af86ec6584e5", + ), firstFoo) + c.Assert(err, IsNil) + + // `firstFoo` is no longer the right `old` reference, so this + // should fail. + err = dir.SetRef(plumbing.NewReferenceFromStrings( + "refs/heads/foo", + "6ecf0ef2c2dffb796033e5a02219af86ec6584e5", + ), firstFoo) + c.Assert(err, NotNil) } func (s *SuiteDotGit) TestRefsFromPackedRefs(c *C) { @@ -192,7 +207,7 @@ func (s *SuiteDotGit) TestRemoveRefFromReferenceFileAndPackedRefs(c *C) { err := dir.SetRef(plumbing.NewReferenceFromStrings( "refs/remotes/origin/branch", "e8d3ffab552895c19b9fcf7aa264d277cde33881", - )) + ), nil) // Make sure it only appears once in the refs list. refs, err := dir.Refs() -- cgit