From d53264806f0d5ddef259f45f4490a19398a102ba Mon Sep 17 00:00:00 2001 From: Jeremy Stribling Date: Thu, 30 Nov 2017 11:36:50 -0800 Subject: dotgit: rewrite packed-refs while holding lock Windows file system doesn't let us rename over a file while holding that file's lock, so use rewrite as a last resort. It could result in a partially-written file, if there's a failure at the wrong time. --- storage/filesystem/internal/dotgit/dotgit.go | 113 ++++++++++----------- .../dotgit/dotgit_rewrite_packed_refs_nix.go | 11 ++ .../dotgit/dotgit_rewrite_packed_refs_windows.go | 39 +++++++ 3 files changed, 103 insertions(+), 60 deletions(-) create mode 100644 storage/filesystem/internal/dotgit/dotgit_rewrite_packed_refs_nix.go create mode 100644 storage/filesystem/internal/dotgit/dotgit_rewrite_packed_refs_windows.go (limited to 'storage') diff --git a/storage/filesystem/internal/dotgit/dotgit.go b/storage/filesystem/internal/dotgit/dotgit.go index 3a12385..5e23e66 100644 --- a/storage/filesystem/internal/dotgit/dotgit.go +++ b/storage/filesystem/internal/dotgit/dotgit.go @@ -387,17 +387,7 @@ func (d *DotGit) Ref(name plumbing.ReferenceName) (*plumbing.Reference, error) { return d.packedRef(name) } -func (d *DotGit) findPackedRefs() ([]*plumbing.Reference, error) { - f, err := d.fs.Open(packedRefsPath) - if err != nil { - if os.IsNotExist(err) { - return nil, nil - } - return nil, err - } - - defer ioutil.CheckClose(f, &err) - +func (d *DotGit) findPackedRefsInFile(f billy.File) ([]*plumbing.Reference, error) { s := bufio.NewScanner(f) var refs []*plumbing.Reference for s.Scan() { @@ -414,6 +404,19 @@ func (d *DotGit) findPackedRefs() ([]*plumbing.Reference, error) { return refs, s.Err() } +func (d *DotGit) findPackedRefs() ([]*plumbing.Reference, error) { + f, err := d.fs.Open(packedRefsPath) + if err != nil { + if os.IsNotExist(err) { + return nil, nil + } + return nil, err + } + + defer ioutil.CheckClose(f, &err) + return d.findPackedRefsInFile(f) +} + func (d *DotGit) packedRef(name plumbing.ReferenceName) (*plumbing.Reference, error) { refs, err := d.findPackedRefs() if err != nil { @@ -460,7 +463,23 @@ func (d *DotGit) addRefsFromPackedRefs(refs *[]*plumbing.Reference, seen map[plu return nil } -func (d *DotGit) openAndLockPackedRefs() (pr billy.File, err error) { +func (d *DotGit) addRefsFromPackedRefsFile(refs *[]*plumbing.Reference, f billy.File, seen map[plumbing.ReferenceName]bool) (err error) { + packedRefs, err := d.findPackedRefsInFile(f) + if err != nil { + return err + } + + for _, ref := range packedRefs { + if !seen[ref.Name()] { + *refs = append(*refs, ref) + seen[ref.Name()] = true + } + } + return nil +} + +func (d *DotGit) openAndLockPackedRefs(doCreate bool) ( + pr billy.File, err error) { var f billy.File defer func() { if err != nil && f != nil { @@ -468,12 +487,17 @@ func (d *DotGit) openAndLockPackedRefs() (pr billy.File, err error) { } }() + openFlags := os.O_RDWR + if doCreate { + openFlags |= os.O_CREATE + } + // Keep trying to open and lock the file until we're sure the file // didn't change between the open and the lock. for { - f, err = d.fs.Open(packedRefsPath) + f, err = d.fs.OpenFile(packedRefsPath, openFlags, 0600) if err != nil { - if os.IsNotExist(err) { + if os.IsNotExist(err) && !doCreate { return nil, nil } @@ -507,19 +531,14 @@ func (d *DotGit) openAndLockPackedRefs() (pr billy.File, err error) { } func (d *DotGit) rewritePackedRefsWithoutRef(name plumbing.ReferenceName) (err error) { - pr, err := d.openAndLockPackedRefs() + pr, err := d.openAndLockPackedRefs(false) if err != nil { return err } if pr == nil { return nil } - doClosePR := true - defer func() { - if doClosePR { - ioutil.CheckClose(pr, &err) - } - }() + defer ioutil.CheckClose(pr, &err) // Creating the temp file in the same directory as the target file // improves our chances for rename operation to be atomic. @@ -527,11 +546,10 @@ func (d *DotGit) rewritePackedRefsWithoutRef(name plumbing.ReferenceName) (err e if err != nil { return err } - doCloseTmp := true + tmpName := tmp.Name() defer func() { - if doCloseTmp { - ioutil.CheckClose(tmp, &err) - } + ioutil.CheckClose(tmp, &err) + _ = d.fs.Remove(tmpName) // don't check err, we might have renamed it }() s := bufio.NewScanner(pr) @@ -558,26 +576,10 @@ func (d *DotGit) rewritePackedRefsWithoutRef(name plumbing.ReferenceName) (err e } if !found { - 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()) - } - - doClosePR = false - if err := pr.Close(); err != nil { - return err - } - - doCloseTmp = false - if err := tmp.Close(); err != nil { - return err + return nil } - return d.fs.Rename(tmp.Name(), packedRefsPath) + return d.rewritePackedRefsWhileLocked(tmp, pr) } // process lines from a packed-refs file @@ -691,20 +693,15 @@ func (d *DotGit) CountLooseRefs() (int, error) { // packed, plus all tags. func (d *DotGit) PackRefs() (err error) { // Lock packed-refs, and create it if it doesn't exist yet. - f, err := d.fs.OpenFile(packedRefsPath, os.O_RDWR|os.O_CREATE, 0600) + f, err := d.openAndLockPackedRefs(true) if err != nil { return err } defer ioutil.CheckClose(f, &err) - err = f.Lock() - if err != nil { - return err - } - // Gather all refs using addRefsFromRefDir and addRefsFromPackedRefs. var refs []*plumbing.Reference - var seen = make(map[plumbing.ReferenceName]bool) + seen := make(map[plumbing.ReferenceName]bool) if err := d.addRefsFromRefDir(&refs, seen); err != nil { return err } @@ -713,7 +710,7 @@ func (d *DotGit) PackRefs() (err error) { return nil } numLooseRefs := len(refs) - if err := d.addRefsFromPackedRefs(&refs, seen); err != nil { + if err := d.addRefsFromPackedRefsFile(&refs, f, seen); err != nil { return err } @@ -722,12 +719,12 @@ func (d *DotGit) PackRefs() (err error) { if err != nil { return err } - doCloseTmp := true + tmpName := tmp.Name() defer func() { - if doCloseTmp { - ioutil.CheckClose(tmp, &err) - } + ioutil.CheckClose(tmp, &err) + _ = d.fs.Remove(tmpName) // don't check err, we might have renamed it }() + w := bufio.NewWriter(tmp) for _, ref := range refs { _, err := w.WriteString(ref.String() + "\n") @@ -741,11 +738,7 @@ func (d *DotGit) PackRefs() (err error) { } // Rename the temp packed-refs file. - doCloseTmp = false - if err := tmp.Close(); err != nil { - return err - } - err = d.fs.Rename(tmp.Name(), packedRefsPath) + err = d.rewritePackedRefsWhileLocked(tmp, f) if err != nil { return err } diff --git a/storage/filesystem/internal/dotgit/dotgit_rewrite_packed_refs_nix.go b/storage/filesystem/internal/dotgit/dotgit_rewrite_packed_refs_nix.go new file mode 100644 index 0000000..af96196 --- /dev/null +++ b/storage/filesystem/internal/dotgit/dotgit_rewrite_packed_refs_nix.go @@ -0,0 +1,11 @@ +// +build !windows + +package dotgit + +import "gopkg.in/src-d/go-billy.v4" + +func (d *DotGit) rewritePackedRefsWhileLocked( + tmp billy.File, pr billy.File) error { + // On non-Windows platforms, we can have atomic rename. + return d.fs.Rename(tmp.Name(), pr.Name()) +} diff --git a/storage/filesystem/internal/dotgit/dotgit_rewrite_packed_refs_windows.go b/storage/filesystem/internal/dotgit/dotgit_rewrite_packed_refs_windows.go new file mode 100644 index 0000000..bcdb93e --- /dev/null +++ b/storage/filesystem/internal/dotgit/dotgit_rewrite_packed_refs_windows.go @@ -0,0 +1,39 @@ +// +build windows + +package dotgit + +import ( + "io" + + "gopkg.in/src-d/go-billy.v4" +) + +func (d *DotGit) rewritePackedRefsWhileLocked( + tmp billy.File, pr billy.File) error { + // If we aren't using the bare Windows filesystem as the storage + // layer, we might be able to get away with a rename over a locked + // file. + err := d.fs.Rename(tmp.Name(), pr.Name()) + if err == nil { + return nil + } + + // Otherwise, Windows doesn't let us rename over a locked file, so + // we have to do a straight copy. Unfortunately this could result + // in a partially-written file if the process fails before the + // copy completes. + _, err = pr.Seek(0, io.SeekStart) + if err != nil { + return err + } + err = pr.Truncate(0) + if err != nil { + return err + } + _, err = tmp.Seek(0, io.SeekStart) + if err != nil { + return err + } + _, err = io.Copy(pr, tmp) + return err +} -- cgit