Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adapted non-force fetching with locking #1

Closed
wants to merge 12 commits into from
Closed
1 change: 1 addition & 0 deletions plumbing/storer/reference.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ var ErrMaxResolveRecursion = errors.New("max. recursion level reached")
// ReferenceStorer is a generic storage of references.
type ReferenceStorer interface {
SetReference(*plumbing.Reference) error
CheckAndSetReference(new, old *plumbing.Reference) error
Reference(plumbing.ReferenceName) (*plumbing.Reference, error)
IterReferences() (ReferenceIter, error)
RemoveReference(plumbing.ReferenceName) error
Expand Down
2 changes: 1 addition & 1 deletion remote.go
Original file line number Diff line number Diff line change
Expand Up @@ -720,7 +720,7 @@ func (r *Remote) updateLocalReferenceStorage(
}
}

refUpdated, err := updateReferenceStorerIfNeeded(r.s, new)
refUpdated, err := checkAndUpdateReferenceStorerIfNeeded(r.s, new, old)
if err != nil {
return updated, err
}
Expand Down
13 changes: 9 additions & 4 deletions repository.go
Original file line number Diff line number Diff line change
Expand Up @@ -625,17 +625,17 @@ func (r *Repository) calculateRemoteHeadReference(spec []config.RefSpec,
return refs
}

func updateReferenceStorerIfNeeded(
s storer.ReferenceStorer, r *plumbing.Reference) (updated bool, err error) {

func checkAndUpdateReferenceStorerIfNeeded(
s storer.ReferenceStorer, r, old *plumbing.Reference) (
updated bool, err error) {
p, err := s.Reference(r.Name())
if err != nil && err != plumbing.ErrReferenceNotFound {
return false, err
}

// we use the string method to compare references, is the easiest way
if err == plumbing.ErrReferenceNotFound || r.String() != p.String() {
if err := s.SetReference(r); err != nil {
if err := s.CheckAndSetReference(r, old); err != nil {
return false, err
}

Expand All @@ -645,6 +645,11 @@ func updateReferenceStorerIfNeeded(
return false, nil
}

func updateReferenceStorerIfNeeded(
s storer.ReferenceStorer, r *plumbing.Reference) (updated bool, err error) {
return checkAndUpdateReferenceStorerIfNeeded(s, r, nil)
}

// Fetch fetches references along with the objects necessary to complete
// their histories, from the remote named as FetchOptions.RemoteName.
//
Expand Down
62 changes: 53 additions & 9 deletions storage/filesystem/internal/dotgit/dotgit.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (
"bufio"
"errors"
"fmt"
"io"
stdioutil "io/ioutil"
"os"
"strings"
Expand Down Expand Up @@ -242,7 +243,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:
Expand All @@ -251,13 +284,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()
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't there be a corresponding f.Unlock() somewhere, maybe as a defer?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually Close takes care of it, but should probably document that. (If we were unlocking the file it would probably(?) need a fsync before the unlock, but go-billy lacks fsync too).

Copy link

@strib strib Sep 14, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@taruti: I don't understand how Close() takes care of it. Is that a new requirement of the billy.File.Close() method? Our current implementation doesn't do that, though I guess maybe for posfix flock and Windows, Close() handles it automatically? We will need something that explicitly calls Unlock() on our own libfs.File implementation; if libfs.File.Close() is supposed to do that, we can make that happen. Was just confused. A comment here would definitely help, and maybe one over in go-billy as well if that's a new assumption.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But flocks are released by closing the file? From e.g. Linux Furthermore, the lock is released either by an explicit LOCK_UN operation on any of these duplicate file descriptors, or when all such file descriptors have been closed.. But yes more documentation would be good.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Our implementation of file locking will run into trouble with other applications if we keep the locks after all file descriptors are closed.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The implementation of our KBFS locks won't be flock-based, so we'll need some way to know to release them. Doing it as part of the Close() call is totally fine, we should just document it.

(@songgao -- we should change our libfs.File.Close() method to call Unlock()).

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
}
Expand Down Expand Up @@ -512,13 +562,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
Expand Down
6 changes: 5 additions & 1 deletion storage/filesystem/reference.go
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
15 changes: 15 additions & 0 deletions storage/memory/storage.go
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import (
)

var ErrUnsupportedObjectType = fmt.Errorf("unsupported object type")
var ErrRefHasChanged = fmt.Errorf("reference has changed concurrently")

// Storage is an implementation of git.Storer that stores data on memory, being
// ephemeral. The use of this storage should be done in controlled envoriments,
Expand Down Expand Up @@ -202,6 +203,20 @@ func (r ReferenceStorage) SetReference(ref *plumbing.Reference) error {
return nil
}

func (r ReferenceStorage) CheckAndSetReference(ref, old *plumbing.Reference) error {
if ref != nil {
if old != nil {
tmp := r[ref.Name()]
if tmp != nil && tmp.Hash() != old.Hash() {
return ErrRefHasChanged
}
}
r[ref.Name()] = ref
}

return nil
}

func (r ReferenceStorage) Reference(n plumbing.ReferenceName) (*plumbing.Reference, error) {
ref, ok := r[n]
if !ok {
Expand Down