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

@@ -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,20 +463,41 @@ 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 {
 			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 +531,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 +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
 	}

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