Skip to content
This repository was archived by the owner on Sep 11, 2020. It is now read-only.

remote: support for non-force, fast-forward-only fetches #665

Merged
merged 7 commits into from
Nov 29, 2017
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions options.go
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,9 @@ type PullOptions struct {
// stored, if nil nothing is stored and the capability (if supported)
// no-progress, is sent to the server to avoid send this information.
Progress sideband.Progress
// Force allows the pull to update a local branch even when the remote
// branch does not descend from it.
Force bool
}

// Validate validates the fields and sets the default values.
Expand Down Expand Up @@ -142,6 +145,9 @@ type FetchOptions struct {
// Tags describe how the tags will be fetched from the remote repository,
// by default is TagFollowing.
Tags TagMode
// Force allows the fetch to update a local branch even when the remote
// branch does not descend from it.
Force bool
}

// Validate validates the fields and sets the default values.
Expand Down
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
Copy link
Contributor

Choose a reason for hiding this comment

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

I know that the order methods are updocumented too, I belive because looks like obviuos (even with that we should document the behaviour at some point), but we should document this one now, sinse for me it`s not a obviuos the goal.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe it is a good time to add doc comments to the other methods in this interface?

Reference(plumbing.ReferenceName) (*plumbing.Reference, error)
IterReferences() (ReferenceIter, error)
RemoveReference(plumbing.ReferenceName) error
Expand Down
30 changes: 27 additions & 3 deletions remote.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import (
var (
NoErrAlreadyUpToDate = errors.New("already up-to-date")
ErrDeleteRefNotSupported = errors.New("server does not support delete-refs")
ErrForceNeeded = errors.New("some refs were not updated")
)

const (
Expand Down Expand Up @@ -302,7 +303,7 @@ func (r *Remote) fetch(ctx context.Context, o *FetchOptions) (storer.ReferenceSt
}
}

updated, err := r.updateLocalReferenceStorage(o.RefSpecs, refs, remoteRefs, o.Tags)
updated, err := r.updateLocalReferenceStorage(o.RefSpecs, refs, remoteRefs, o.Tags, o.Force)
if err != nil {
return nil, err
}
Expand Down Expand Up @@ -773,8 +774,11 @@ func (r *Remote) updateLocalReferenceStorage(
specs []config.RefSpec,
fetchedRefs, remoteRefs memory.ReferenceStorage,
tagMode TagMode,
force bool,
) (updated bool, err error) {
isWildcard := true
forceNeeded := false

for _, spec := range specs {
if !spec.IsWildcard() {
isWildcard = false
Expand All @@ -789,9 +793,25 @@ func (r *Remote) updateLocalReferenceStorage(
continue
}

new := plumbing.NewHashReference(spec.Dst(ref.Name()), ref.Hash())
localName := spec.Dst(ref.Name())
old, _ := storer.ResolveReference(r.s, localName)
new := plumbing.NewHashReference(localName, ref.Hash())

// If the ref exists locally as a branch and force is not specified,
// only update if the new ref is an ancestor of the old
if old != nil && old.Name().IsBranch() && !force && !spec.IsForceUpdate() {
ff, err := isFastForward(r.s, old.Hash(), new.Hash())
if err != nil {
return updated, err
}

if !ff {
forceNeeded = true
continue
}
}

refUpdated, err := updateReferenceStorerIfNeeded(r.s, new)
refUpdated, err := checkAndUpdateReferenceStorerIfNeeded(r.s, new, old)
if err != nil {
return updated, err
}
Expand Down Expand Up @@ -819,6 +839,10 @@ func (r *Remote) updateLocalReferenceStorage(
updated = true
}

if err == nil && forceNeeded {
err = ErrForceNeeded
}

return
}

Expand Down
42 changes: 42 additions & 0 deletions remote_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -308,6 +308,48 @@ func (s *RemoteSuite) doTestFetchNoErrAlreadyUpToDate(c *C, url string) {
c.Assert(err, Equals, NoErrAlreadyUpToDate)
}

func (s *RemoteSuite) TestFetchFastForward(c *C) {
r := newRemote(memory.NewStorage(), &config.RemoteConfig{
URLs: []string{s.GetBasicLocalRepositoryURL()},
})

s.testFetch(c, r, &FetchOptions{
RefSpecs: []config.RefSpec{
config.RefSpec("+refs/heads/master:refs/heads/master"),
},
}, []*plumbing.Reference{
plumbing.NewReferenceFromStrings("refs/heads/master", "6ecf0ef2c2dffb796033e5a02219af86ec6584e5"),
})

// First make sure that we error correctly when a force is required.
err := r.Fetch(&FetchOptions{
RefSpecs: []config.RefSpec{
config.RefSpec("refs/heads/branch:refs/heads/master"),
},
})
c.Assert(err, Equals, ErrForceNeeded)

// And that forcing it fixes the problem.
err = r.Fetch(&FetchOptions{
RefSpecs: []config.RefSpec{
config.RefSpec("+refs/heads/branch:refs/heads/master"),
},
})
c.Assert(err, IsNil)

// Now test that a fast-forward, non-force fetch works.
r.s.SetReference(plumbing.NewReferenceFromStrings(
"refs/heads/master", "918c48b83bd081e863dbe1b80f8998f058cd8294",
))
s.testFetch(c, r, &FetchOptions{
RefSpecs: []config.RefSpec{
config.RefSpec("refs/heads/master:refs/heads/master"),
},
}, []*plumbing.Reference{
plumbing.NewReferenceFromStrings("refs/heads/master", "6ecf0ef2c2dffb796033e5a02219af86ec6584e5"),
})
}

func (s *RemoteSuite) TestString(c *C) {
r := newRemote(nil, &config.RemoteConfig{
Name: "foo",
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
66 changes: 57 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 @@ -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())
Copy link
Contributor

Choose a reason for hiding this comment

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

This function is not covered by the test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

readReferenceFrom is definitely covered by the tests, since it's called by readReferenceFile below. But if you're referring to checkReferenceAndTruncate, sure I'll add a test for it.

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 @@ -248,13 +281,34 @@ 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)

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

// 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 @@ -493,13 +547,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: 3 additions & 3 deletions storage/filesystem/internal/dotgit/dotgit_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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()
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 {
Copy link
Contributor

Choose a reason for hiding this comment

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

This function is not covered by the tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, I didn't notice that the remote tests don't use filesystem storage. I'll add a test.

if ref != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can be change this to:

if ref == nil {
    return 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
1 change: 1 addition & 0 deletions worktree.go
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,7 @@ func (w *Worktree) PullContext(ctx context.Context, o *PullOptions) error {
Depth: o.Depth,
Auth: o.Auth,
Progress: o.Progress,
Force: o.Force,
})

updated := true
Expand Down