internal/gps: Parse abbreviated git revisions #1027

spenczar · 2017-08-17T15:21:49Z

What does this do / why do we need it?

This changes the SourceManager's InferConstraint method to look up any revisions which are present in the source for a project. It does this as a last-gasp effort so that it doesn't prematurely expand a tag or a branch name into a precise commit. In practice, this means it will expand any abbreviated git commit identifiers.

While dep doesn't want to encourage the use of abbreviated git commit identifiers, they come up frequently when we parse vendoring specifiers (like glide.yaml) of existing projects. We should give a shot at parsing these and then expanding them to their unabbreviated form.

What should your reviewer look out for in this PR?

It is totally possible that there are unintended consequences here. I don't know all the ways in which InferConstraint's output is used.

I also don't know anything about how caching plays in here. I haven't tried to cache anything.

Which issue(s) does this PR fix?

Fixes #987

While dep doesn't want to encourage the use of abbreviated git commit identifiers, they come up frequently when we parse vendoring specifiers (like glide.yaml) of existing projects. We should give a shot at parsing these and then expanding them to their unabbreviated form.

spenczar · 2017-08-17T15:25:01Z

internal/gps/vcs_source.go

@@ -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) {


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).

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

spenczar · 2017-08-17T15:33:47Z

Codeclimate is upset at context.TODO(), which seems a little draconian to me. Should I make those context.Background()?

carolynvs

I defer on all points to whatever @sdboyer thinks about this, but wanted to comment anyway. 😀

carolynvs · 2017-08-17T18:15:47Z

internal/gps/source.go

@@ -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)


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

Works for me 👌

carolynvs · 2017-08-17T18:17:23Z

internal/gps/source_manager.go

@@ -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 {


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.

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

carolynvs · 2017-08-17T18:21:56Z

internal/gps/source_manager.go

+	// Abbreviated git revision? If the string is present, there's a good shot of
+	// this.
+	if present, _ := sm.RevisionPresentIn(pi, Revision(s)); present {
+		srcg, err := sm.srcCoord.getSourceGatewayFor(context.TODO(), pi)


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)

}

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.

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

carolynvs · 2017-08-17T18:22:25Z

internal/gps/vcs_source.go

@@ -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) {


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

commitInfo returned a struct from an external package. This coupling isn't ideal, and we wouldn't use the extra info for anything. It's better to just have a method for exactly what we want, which is disambiguation of short revision specifiers.

Now that we are disambiguating revisions by going to the source, we don't need the heuristic check of string length and hex parseability anymore.

This just cleans up the SourceMgr implementation a bit.

carolynvs · 2017-08-18T00:36:06Z

internal/gps/source_manager.go

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


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.

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.

Yeah, I don't think our CI build runs against any real bzr repos?

the gps tests include some real tests against bzr.

carolynvs · 2017-08-18T00:41:19Z

internal/gps/source_manager.go

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

+	// Revision, possibly abbreviated


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.

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.

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

The previous commit changed the order of prefernce when inferring constraints. Also, moved the bazaar GUID parsing stuff to make sure that it receives the same preference level as revisions for other repository types.

String parsing was a little error prone, so we'd prefer to ask the bazaar repository whether a particular string is a valid revision ID. To do this, we need a very slightly custom version of disambiguateRevision for *bzrSource. This revision also refactors TestSourceManager_InferConstraint pretty dramatically to let it work on repositories coming from different VCSes.

spenczar · 2017-08-28T15:08:42Z

@carolynvs, please take another look - I'm happy with how this turned out, I was able to get rid of the bizarre bazaar string parsing, and I learned a bit about bzr on the way :)

How would you like me to address codeclimate's complaint about using context.TODO()? I think threading a real context in all the way from package main seems out of scope.

carolynvs · 2017-08-28T15:41:01Z

Don't worry about codeclimate flagging the context.TODO() usage. I'll figure something out and get it merged.

carolynvs

This is looking great! Just a couple nits (ignore codeclimate) and we can .

carolynvs · 2017-08-28T15:42:56Z

internal/gps/source_manager_test.go

-	if err != nil {
-		t.Fatal(err)
-	}
+	testcase := func(str string, pi ProjectIdentifier, want Constraint) func(*testing.T) {


nit: Can these subtests be restructured to match the way we have been doing other subtests? The pattern that we have been following is to create an array or map of testcase data, and then in a for loop call t.Run. It makes it easier to review when all the testcases are together.

dep/context_test.go

Lines 116 to 128 in 6c5ebb9

var testcases = []struct {

name string

lock bool

wd string

}{

{"direct", true, filepath.Join("src", "test1")},

{"ascending", true, filepath.Join("src", "test1", "sub")},

{"without lock", false, filepath.Join("src", "test2")},

{"ascending without lock", false, filepath.Join("src", "test2", "sub")},

}

for _, tc := range testcases {

t.Run(tc.name, func(t *testing.T) {

Sure, done.

carolynvs · 2017-08-28T15:47:12Z

internal/gps/source_manager_test.go

+	var (
+		gitProj = ProjectIdentifier{ProjectRoot: "github.com/carolynvs/deptest"}
+		bzrProj = ProjectIdentifier{ProjectRoot: "launchpad.net/govcstestbzrrepo"}
+		hgProj  = ProjectIdentifier{ProjectRoot: "bitbucket.org/golang-dep/dep-test"}


Let's add a testcase for svn. Here's the test repo info: https://github.com/golang/dep/blob/master/internal/gps/vcs_repo_test.go#L122

Will do. That code is using svn to manually checkout a repo that would normally be cloned with git using the go tools, so I might need to do some plumbing to make it appear to the SourceMgr as a svn repo.

Oh, if it's not a straight case like the others, please don't bother. I didn't look at that too closely, and didn't realize it's not a svn repo... 😊

Okay, works for me 😄

Digging in further, I got the sense that dep might not fully support svn, in fact. It doesn't look like anything references the svnRepo type except one skipped test.

carolynvs

Thank you for fixing this! 👍

spenczar requested a review from sdboyer as a code owner August 17, 2017 15:21

googlebot added the cla: yes label Aug 17, 2017

spenczar commented Aug 17, 2017

View reviewed changes

spenczar changed the title ~~Parse abbreviated git revisions~~ internal/gps: Parse abbreviated git revisions Aug 17, 2017

carolynvs reviewed Aug 17, 2017

View reviewed changes

spenczar mentioned this pull request Aug 17, 2017

Gracefully handle abbreviated revisions during import #987

Closed

spenczar added 3 commits August 17, 2017 14:32

Change commitInfo to disambiguateRevision

dfb8719

commitInfo returned a struct from an external package. This coupling isn't ideal, and we wouldn't use the extra info for anything. It's better to just have a method for exactly what we want, which is disambiguation of short revision specifiers.

Remove heuristic git hash detection

e0f7e1f

Now that we are disambiguating revisions by going to the source, we don't need the heuristic check of string length and hex parseability anymore.

Move revision disambiguation into a private method

6977b26

This just cleans up the SourceMgr implementation a bit.

carolynvs reviewed Aug 18, 2017

View reviewed changes

carolynvs suggested changes Aug 18, 2017

View reviewed changes

spenczar added 2 commits August 28, 2017 09:48

Correctly document (*SourceMgr).InferConstraint

2d4b3c2

The previous commit changed the order of prefernce when inferring constraints. Also, moved the bazaar GUID parsing stuff to make sure that it receives the same preference level as revisions for other repository types.

carolynvs suggested changes Aug 28, 2017

View reviewed changes

spenczar added 2 commits August 28, 2017 13:54

Use structs for subtests instead of a function literal

7f19f8b

Don't try to test constraint inference for svn

49bf340

carolynvs approved these changes Aug 28, 2017

View reviewed changes

carolynvs merged commit 43252e0 into golang:master Aug 28, 2017

carolynvs mentioned this pull request Aug 28, 2017

[WIP] Import transitive constraints from glide #920

Closed

5 tasks

ibrasho added the area: gps label Sep 9, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

internal/gps: Parse abbreviated git revisions #1027

internal/gps: Parse abbreviated git revisions #1027

spenczar commented Aug 17, 2017

spenczar Aug 17, 2017

carolynvs Aug 17, 2017

spenczar commented Aug 17, 2017

carolynvs left a comment

carolynvs Aug 17, 2017

spenczar Aug 17, 2017

carolynvs Aug 17, 2017

spenczar Aug 17, 2017

carolynvs Aug 17, 2017

spenczar Aug 17, 2017

carolynvs Aug 17, 2017

carolynvs Aug 17, 2017

carolynvs Aug 18, 2017

spenczar Aug 21, 2017

carolynvs Aug 25, 2017

sdboyer Aug 27, 2017

carolynvs Aug 18, 2017

spenczar Aug 21, 2017

carolynvs Aug 25, 2017

spenczar commented Aug 28, 2017 •

edited

Loading

carolynvs commented Aug 28, 2017

carolynvs left a comment

carolynvs Aug 28, 2017

spenczar Aug 28, 2017

carolynvs Aug 28, 2017

spenczar Aug 28, 2017

carolynvs Aug 28, 2017

spenczar Aug 28, 2017

carolynvs left a comment

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

	var testcases = []struct {
	name string
	lock bool
	wd string
	}{
	{"direct", true, filepath.Join("src", "test1")},
	{"ascending", true, filepath.Join("src", "test1", "sub")},
	{"without lock", false, filepath.Join("src", "test2")},
	{"ascending without lock", false, filepath.Join("src", "test2", "sub")},
	}

	for _, tc := range testcases {
	t.Run(tc.name, func(t *testing.T) {

internal/gps: Parse abbreviated git revisions #1027

internal/gps: Parse abbreviated git revisions #1027

Conversation

spenczar commented Aug 17, 2017

What does this do / why do we need it?

What should your reviewer look out for in this PR?

Which issue(s) does this PR fix?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spenczar commented Aug 17, 2017

carolynvs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spenczar commented Aug 28, 2017 • edited Loading

carolynvs commented Aug 28, 2017

carolynvs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carolynvs left a comment

Choose a reason for hiding this comment

spenczar commented Aug 28, 2017 •

edited

Loading