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 4 commits
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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
13 changes: 13 additions & 0 deletions internal/gps/source.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -450,6 +450,18 @@ func (sg *sourceGateway) revisionPresentIn(ctx context.Context, r Revision) (boo
return present, err
}

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

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

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

func (sg *sourceGateway) sourceURL(ctx context.Context) (string, error) {
sg.mu.Lock()
defer sg.mu.Unlock()
Expand Down Expand Up @@ -550,6 +562,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)
disambiguateRevision(context.Context, Revision) (Revision, error)
exportRevisionTo(context.Context, Revision, string) error
sourceType() string
}
38 changes: 23 additions & 15 deletions internal/gps/source_manager.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ package gps

import (
"context"
"encoding/hex"
"fmt"
"os"
"os/signal"
Expand Down Expand Up @@ -519,24 +518,13 @@ func (sm *SourceMgr) InferConstraint(s string, pi ProjectIdentifier) (Constraint
return Any(), nil
}

slen := len(s)
if slen == 40 {
if _, err := hex.DecodeString(s); err == nil {
// Whether or not it's intended to be a SHA1 digest, this is a
// valid byte sequence for that, so go with Revision. This
// covers git and hg
return Revision(s), nil
}
}

// Next, try for bzr, which has a three-component GUID separated by
// dashes. There should be two, but the email part could contain
// internal dashes
// Bazaar has a three-component GUID separated by dashes. There should be two,
Copy link
Collaborator

Choose a reason for hiding this comment

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

What happens when you remove the specific handling for bzr? I was hoping (but don't know for sure!) that repo.CommitInfo would work for all vcs types, including bzr.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was hoping we could too, but doing so makes the current test fail (since the current test operates on a git repo, not a bzr one).

I think this should work for bzr too... but I've never used bzr in my life, really. I'm a little wary of trying to write a realistic test given my lack of experience with bzr. If you feel like you could review it well and guide me, though, I'm game.

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 don't think our CI build runs against any real bzr repos?

Copy link
Member

Choose a reason for hiding this comment

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

the gps tests include some real tests against bzr.

// but the email part could contain internal dashes.
if strings.Contains(s, "@") && strings.Count(s, "-") >= 2 {
// Work from the back to avoid potential confusion from the email
i3 := strings.LastIndex(s, "-")
// Skip if - is last char, otherwise this would panic on bounds err
if slen == i3+1 {
if len(s) == i3+1 {
return NewVersion(s), nil
}

Expand Down Expand Up @@ -578,9 +566,29 @@ func (sm *SourceMgr) InferConstraint(s string, pi ProjectIdentifier) (Constraint
return version.Unpair(), nil
}

// Revision, possibly abbreviated
Copy link
Collaborator

Choose a reason for hiding this comment

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

Originally the function checked if the string given was a revision first (note the comment at the top of the file). Unless we need to change that behavior(?), let's move this back to the top where we are replacing the old code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we do need to change that behavior. If we put it at the top, then we'll dereference branch names and version tags, turning them into pure Revisions.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Okay then let's just update the comment to reflect the new order.

r, err := sm.disambiguateRevision(context.TODO(), pi, Revision(s))
if err == nil {
return r, nil
}

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

// disambiguateRevision looks up a revision in the underlying source, spitting
// it back out in an unabbreviated, disambiguated form.
//
// For example, if pi refers to a git-based project, then rev could be an
// abbreviated git commit hash. disambiguateRevision would return the complete
// hash.
func (sm *SourceMgr) disambiguateRevision(ctx context.Context, pi ProjectIdentifier, rev Revision) (Revision, error) {
srcg, err := sm.srcCoord.getSourceGatewayFor(context.TODO(), pi)
if err != nil {
return "", err
}
return srcg.disambiguateRevision(ctx, rev)
}

type timeCount struct {
count int
start time.Time
Expand Down
3 changes: 2 additions & 1 deletion internal/gps/source_manager_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,8 @@ func TestSourceManager_InferConstraint(t *testing.T) {
"v2": NewBranch("v2"),
"v0.12.0-12-de4dcafe0": svs,
"master": NewBranch("master"),
"5b3352dc16517996fb951394bcbbe913a2a616e3": Revision("5b3352dc16517996fb951394bcbbe913a2a616e3"),
"3f4c3bea144e112a69bbe5d8d01c1b09a544253f": Revision("3f4c3bea144e112a69bbe5d8d01c1b09a544253f"),
"3f4c3bea": Revision("3f4c3bea144e112a69bbe5d8d01c1b09a544253f"),

// valid bzr rev
"jess@linux.com-20161116211307-wiuilyamo9ian0m7": Revision("jess@linux.com-20161116211307-wiuilyamo9ian0m7"),
Expand Down
8 changes: 8 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 @@ -40,6 +40,14 @@ func (bs *baseVCSSource) upstreamURL() string {
return bs.repo.Remote()
}

func (bs *baseVCSSource) disambiguateRevision(ctx context.Context, r Revision) (Revision, error) {
ci, err := bs.repo.CommitInfo(string(r))
if err != nil {
return "", err
}
return Revision(ci.Commit), nil
}

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