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

internal/gps: Parse abbreviated git revisions #1027

Merged
merged 8 commits into from
Aug 28, 2017
Merged

internal/gps: Parse abbreviated git revisions #1027

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
531fb9c
Parse abbreviated git revisions
spenczar Aug 17, 2017
dfb8719
Change commitInfo to disambiguateRevision
spenczar Aug 17, 2017
e0f7e1f
Remove heuristic git hash detection
spenczar Aug 17, 2017
6977b26
Move revision disambiguation into a private method
spenczar Aug 17, 2017
2d4b3c2
Correctly document (*SourceMgr).InferConstraint
spenczar Aug 28, 2017
960d21a
Handle bazaar revisions by talking to the actual repo
spenczar Aug 28, 2017
7f19f8b
Use structs for subtests instead of a function literal
spenczar Aug 28, 2017
49bf340
Don't try to test constraint inference for svn
spenczar Aug 28, 2017
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
14 changes: 14 additions & 0 deletions internal/gps/source.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import (
"fmt"
"sync"

"github.com/Masterminds/vcs"
"github.com/golang/dep/internal/gps/pkgtree"
)

Expand Down Expand Up @@ -450,6 +451,18 @@ func (sg *sourceGateway) revisionPresentIn(ctx context.Context, r Revision) (boo
return present, err
}

func (sg *sourceGateway) commitInfo(ctx context.Context, r Revision) (*vcs.CommitInfo, error) {
sg.mu.Lock()
defer sg.mu.Unlock()

_, err := sg.require(ctx, sourceIsSetUp|sourceExistsLocally)
if err != nil {
return nil, err
}

return sg.src.commitInfo(ctx, r)
}

func (sg *sourceGateway) sourceURL(ctx context.Context) (string, error) {
sg.mu.Lock()
defer sg.mu.Unlock()
Expand Down Expand Up @@ -550,6 +563,7 @@ type source interface {
getManifestAndLock(context.Context, ProjectRoot, Revision, ProjectAnalyzer) (Manifest, Lock, error)
listPackages(context.Context, ProjectRoot, Revision) (pkgtree.PackageTree, error)
revisionPresentIn(Revision) (bool, error)
commitInfo(context.Context, Revision) (*vcs.CommitInfo, error)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Perhaps this should return a Revision, instead of types from vcs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Works for me 👌

exportRevisionTo(context.Context, Revision, string) error
sourceType() string
}
12 changes: 12 additions & 0 deletions internal/gps/source_manager.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -578,6 +578,18 @@ func (sm *SourceMgr) InferConstraint(s string, pi ProjectIdentifier) (Constraint
return version.Unpair(), nil
}

// Abbreviated git revision? If the string is present, there's a good shot of
// this.
if present, _ := sm.RevisionPresentIn(pi, Revision(s)); present {
Copy link
Collaborator

Choose a reason for hiding this comment

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

At the top of this function are a bunch of string checks to validate the revision. I was hoping that we could replace those heuristics with this new logic to lookup the revision from the repo.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, I think we can definitely yoink those out, will do.

srcg, err := sm.srcCoord.getSourceGatewayFor(context.TODO(), pi)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Instead of dipping into the source gateway, this should probably be another function on the source manager, just like RevisionPresentIn which handles getting the value from the source gateway.

dep/internal/gps/source_manager.go

Lines 436 to 448 in ee6fcfc

func (sm *SourceMgr) RevisionPresentIn(id ProjectIdentifier, r Revision) (bool, error) {
if atomic.CompareAndSwapInt32(&sm.releasing, 1, 1) {
return false, smIsReleased{}
}
srcg, err := sm.srcCoord.getSourceGatewayFor(context.TODO(), id)
if err != nil {
// TODO(sdboyer) More-er proper-er errors
return false, err
}
return srcg.revisionPresentIn(context.TODO(), r)
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My concern here was just in adding another public method to the SourceManager interface, which is already very large. What would you think of a private method on the *sourceManager here to encapsulate this behavior?

(*sourceManager).RevisionPresentIn already digs into the source gateway in its implementation, so this would be similar, in my head.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, good catch. A private method for InferConstraint to call would be better.

if err == nil {
ci, err := srcg.commitInfo(context.TODO(), Revision(s))
if err == nil {
return Revision(ci.Commit), nil
}
}
}

return nil, errors.Errorf("%s is not a valid version for the package %s(%s)", s, pi.ProjectRoot, pi.Source)
}

Expand Down
2 changes: 2 additions & 0 deletions internal/gps/source_manager_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,8 @@ func TestSourceManager_InferConstraint(t *testing.T) {
"v0.12.0-12-de4dcafe0": svs,
"master": NewBranch("master"),
"5b3352dc16517996fb951394bcbbe913a2a616e3": Revision("5b3352dc16517996fb951394bcbbe913a2a616e3"),
// abbreviated git ref
"3f4c3bea": Revision("3f4c3bea144e112a69bbe5d8d01c1b09a544253f"),

// valid bzr rev
"jess@linux.com-20161116211307-wiuilyamo9ian0m7": Revision("jess@linux.com-20161116211307-wiuilyamo9ian0m7"),
Expand Down
5 changes: 5 additions & 0 deletions internal/gps/vcs_source.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import (
"time"

"github.com/Masterminds/semver"
"github.com/Masterminds/vcs"
"github.com/golang/dep/internal/fs"
"github.com/golang/dep/internal/gps/pkgtree"
)
Expand All @@ -40,6 +41,10 @@ func (bs *baseVCSSource) upstreamURL() string {
return bs.repo.Remote()
}

func (bs *baseVCSSource) commitInfo(ctx context.Context, r Revision) (*vcs.CommitInfo, error) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is it appropriate to return this type?

We could return (commit, author, message string, timestamp time.Time, err error) to avoid tangling up too tightly with an external project.

Another option is to make this a more targeted method, like commitFor(ctx context.Context, specifier string) (Revision, error).

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah I was thinking that we'd return just a Revision.

return bs.repo.CommitInfo(string(r))
}

func (bs *baseVCSSource) getManifestAndLock(ctx context.Context, pr ProjectRoot, r Revision, an ProjectAnalyzer) (Manifest, Lock, error) {
err := bs.repo.updateVersion(ctx, r.String())
if err != nil {
Expand Down