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.

Author: strib

storage/filesystem/internal/dotgit/dotgit.go

@@ -460,20 +460,26 @@ func (d *DotGit) addRefsFromPackedRefs(refs *[]*plumbing.Reference, seen map[plu
 	return nil
 }
 
-func (d *DotGit) openAndLockPackedRefs() (pr billy.File, err error) {
+func (d *DotGit) openAndLockPackedRefs(doCreate bool) (
+	pr billy.File, err error) {
 	var f billy.File
 	defer func() {
 		if err != nil && f != nil {
 			ioutil.CheckClose(f, &err)
 		}
 	}()
 
+	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,31 +513,25 @@ 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.
 	tmp, err := d.fs.TempFile("", tmpPackedRefsPrefix)
 	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 +558,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 +675,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
 	}
@@ -722,12 +701,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 +720,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
 	}

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())
+}

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
+}