Don't assume every git repository has a HEAD #1053

Minnozz · 2017-08-23T22:10:16Z

What does this do / why do we need it?

The code that determines the versions of a git repository assumes that all git repositories have a HEAD. It panics when this is not the case.

What should your reviewer look out for in this PR?

Not sure if the big comment block above the changes should be edited.

Do you need help or clarification on anything?

Is using the zero value of Revision like this okay?

Which issue(s) does this PR fix?

Fixes #1051, Fixes #1112

Fixes golang#1051

sdboyer · 2017-08-25T11:57:08Z

hmm, interesting. could you explain the circumstances under which a git repo might lack a HEAD? the one case I can imagine is cloning an empty repo.

the empty repo case also makes me think...there is currently an undocumented assumption that ListVersions will return at least one item. when that assumption isn't met, the solver has a panic condition.

we've run into that panic before with gopkg.in, but I have, until now, thought that assumption was still an accurate one. this is making me think that, though - and besides, even if it's an accurate assumption, it's still not a good one, as solver code is not resilient against a particular output that's valid with respect to type signatures.

I'll look at this more in depth next week, when I'm back from vacation and not sitting in an oil change shop lobby.

Minnozz · 2017-08-25T16:35:34Z

In my case, the remote repository did have a HEAD on the server side, but it pointed at master by default, which did not exist (the only existing branch was develop). git ls-remote did not return this invalid HEAD ref.

Steps to reproduce:

Create an empty, bare repository (git init --bare)
Note that HEAD points to master, which does not exist (yet)
Clone from the repository
Create and checkout a branch other than master in the clone
Do the initial commit on the branch
Push the branch to the remote
git ls-remote from the clone does not list HEAD

With this PR and the situation above, listVersions() could return the revision from refs/heads/develop even though no HEAD is returned from git ls-remote.

My code hosting platform (Phabricator) has no way of changing the HEAD ref of the hosted repository, so I had to log in on the server hosting Phabricator to make HEAD point at the correct branch. As I understand it, GitHub changes HEAD to point at the default branch.

sdboyer · 2017-08-26T15:43:29Z

ahhhh, yes, that case also makes sense. (i actually created a similar problem to what Phabricator imposes in a git hosting platform i built, now that i think about it 😢). HEAD literally is the default branch in a bare git repository.

so yeah, we'll need to adapt the solver to really do this properly. the same basic issue there is #776.

Minnozz · 2017-08-26T16:48:31Z

I'm don't think that the issue in #776 (with dep version v0.1.0-175-gc79b048) still occurs in the version I based my PR on (v0.3.0-151-g1f6d6bb). With the patch from my PR applied (which only fixes a panic/out of bounds access inside of listVersions()), dep functions correctly on a repository without HEAD but with branches, because listVersions() correctly returns the head of the develop branch as a possible version. In #776, the panic comes from findValidVersion().

Do you agree this PR fixes a problem on its own, and that the case where listVersions() returns 0 versions is a separate issue?

sdboyer · 2017-08-27T18:56:33Z

Do you agree this PR fixes a problem on its own, and that the case where listVersions() returns 0 versions is a separate issue?

mm, i suppose that's fair.

still, we need some test cases to cover this. that may be a bit awkward - probably the easiest thing to do would be to create a git repository on the fly, munge it so that there is no HEAD, and work from there.

note: there are some helpers for working with git in internal/test/test.go, but we can't use them here, as we don't let gps reach back outside of internal/gps.

Minnozz · 2017-08-31T07:53:43Z

probably the easiest thing to do would be to create a git repository on the fly, munge it so that there is no HEAD, and work from there.

I'm not sure I know how to write a test like that; I'm new to Go (and dep) and the other tests in internal/gps/vcs_source_test.go all use "real" remote repositories.

Do you have a suggestion on how to advance this PR?

darkowlzz · 2017-09-01T22:10:37Z

@Minnozz you can start writing the test referring to Test_hgSource_exportRevisionTo_removeVcsFiles, which uses some of our test helpers.

Since we need to create a git repo on the fly in this case, you can create a tempdir using the test helper and then run git commands in the tempdir using RunGit(). Once the repo is in the desired state (without HEAD), you can create a gitSource, refer testGitSourceInteractions, with url prefix file:/// for maybeGitSource. And calling listVersions() on that gitSource would return the required result to test the result of the changes.

You can use TempFile() to create files with some specific content.

I don't think this would require any fixture data, but in case you need them, you can create testdata/ directory and keep the fixtures there. Copy them to the desired git repo dir using test helper TempCopy(). You can read more about testdata fixtures from test-fixtures-in-go blog post.

Let us know if you need any help :)

Minnozz · 2017-09-03T15:45:51Z

@darkowlzz Thanks a lot for the instructions. I gave it a try, let me know what you think.

I'm not sure what qualifies as a slow test. I think this one does not, since there is no network communication.

darkowlzz

Awesome!! 🎉
I tried and it seems to work. No more panics and fetches proper version list.
But the test seems to be failing on Windows https://ci.appveyor.com/project/golang/dep/build/1967 .
My guess is, it's because of file:///. Not sure. @carolynvs any idea about this?

Suggested small changes.

darkowlzz · 2017-09-04T14:41:43Z

internal/gps/vcs_source_test.go

@@ -524,6 +524,62 @@ func testHgSourceInteractions(t *testing.T) {
 	<-donech
 }

+func Test_gitSource_listVersions_noHEAD(t *testing.T) {


I think we decided to not do any more snake case function names, idiomatic go. The ones that you see are old code, we need to change them. So, let's rename this to TestGitSourceListVersionsNoHEAD.

darkowlzz · 2017-09-04T14:48:15Z

internal/gps/vcs_source_test.go

+	un := "file://" + repoPath
+	u, err := url.Parse(un)
+	if err != nil {
+		t.Errorf("URL was bad, lolwut? errtext: %s", err)


LOL! 😄
Let's make this Fatalf.
t.Fatalf("Error parsing URL %s: %s", un, err)

darkowlzz · 2017-09-04T14:54:29Z

internal/gps/vcs_source_test.go

+	}
+
+	if len(pvlist) != 1 {
+		t.Errorf("expected 1 version pair from listVersions(), got %d", len(pvlist))


Let's change this to GOT/WNT style message:
t.Errorf("Unexpected version pair length:\n\t(GOT): %d\n\t(WNT): %d", len(pvlist), 1)

darkowlzz · 2017-09-04T14:57:47Z

I'm not sure what qualifies as a slow test. I think this one does not, since there is no network communication.

No, it's fine. This need not be a slow test. Yeah, network or tests that run for long need this. This one doesn't take any time.

darkowlzz · 2017-09-04T15:33:48Z

@ibrasho ping! Maybe you too can help with windows related issues? 😊

ibrasho · 2017-09-06T14:07:51Z

Most likely this error is because git is not configured with a user.
Can you try this?

mremond · 2017-09-07T12:04:12Z

For the record, this patch also fixes #1112

Is there anything I can do to help with this PR to make it good for integration in master ?

Minnozz · 2017-09-07T13:38:37Z

@mremond I intend to address the suggested changes from @darkowlzz today or tomorrow. After that, we'll have to see if the tests pass on Windows.

@ibrasho I will try adding --author="Test author <[email protected]>" to the git commit invocation (calling git config --global inside the test is very scary).

Minnozz · 2017-09-07T13:47:43Z

Another option is adding the pre-prepared repository (without HEAD) to testdata instead of recreating it every time the test is run. Let me know if that would be better.

ibrasho · 2017-09-07T18:34:55Z

@Minnozz I'm just trying to figure out if that's the actual reason. If it's, we can decide what's the appropriate long-term solution then. 😄

Fixes an error when the git user is not set.

Minnozz · 2017-09-07T20:17:19Z

Adding --author unfortunately did not fix the test on Windows: https://ci.appveyor.com/project/golang/dep/build/2012

Having the output of the failing git command would be very useful; running the tests wit -logs should give us that (but changing RunGit() to always log the output if the command fails might also be a good idea)

Minnozz · 2017-09-08T08:51:04Z

I fixed the test on Windows: https://ci.appveyor.com/project/golang/dep/build/2015

darkowlzz

LGTM!

ibrasho

LGTM

darkowlzz · 2017-09-09T15:57:30Z

Thanks for fixing this. 👍

Don't assume every git repository has a HEAD

41eca4f

Fixes golang#1051

Minnozz requested a review from sdboyer as a code owner August 23, 2017 22:10

googlebot added the cla: yes label Aug 23, 2017

Add test for gitSource.listVersions() without HEAD

89fb479

darkowlzz reviewed Sep 4, 2017

View reviewed changes

This was referenced Sep 4, 2017

Trying to vendor a package with a dependency to gopkg.in #1119

Closed

Incorrect branch taken on n+1 dependency #1112

Closed

BartSchuurmans added 3 commits September 7, 2017 21:56

Fix naming convention for test

308d997

Nicer error reporting in test

6621e9e

Don't rely on local git config in test

9fc18f1

Fixes an error when the git user is not set.

BartSchuurmans added 2 commits September 8, 2017 10:01

Fix test when git is not set up

d3e5403

Fix file:// URI on Windows

e7a41d8

Minnozz force-pushed the fix-1051 branch from 5759fae to e7a41d8 Compare September 8, 2017 08:43

darkowlzz approved these changes Sep 8, 2017

View reviewed changes

ibrasho added area: gps correctness labels Sep 9, 2017

ibrasho approved these changes Sep 9, 2017

View reviewed changes

darkowlzz merged commit 9db233a into golang:master Sep 9, 2017

Minnozz deleted the fix-1051 branch September 10, 2017 14:25

ibrasho mentioned this pull request Sep 21, 2017

"panic: version queue is empty, should not happen" while trying to use "develop" branch of "github.com/gin-gonic/gin" #776

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't assume every git repository has a HEAD #1053

Don't assume every git repository has a HEAD #1053

Minnozz commented Aug 23, 2017 •

edited by darkowlzz

Loading

sdboyer commented Aug 25, 2017

Minnozz commented Aug 25, 2017

sdboyer commented Aug 26, 2017

Minnozz commented Aug 26, 2017 •

edited

Loading

sdboyer commented Aug 27, 2017

Minnozz commented Aug 31, 2017

darkowlzz commented Sep 1, 2017 •

edited

Loading

Minnozz commented Sep 3, 2017

darkowlzz left a comment

darkowlzz Sep 4, 2017

darkowlzz Sep 4, 2017

darkowlzz Sep 4, 2017

darkowlzz commented Sep 4, 2017

darkowlzz commented Sep 4, 2017

ibrasho commented Sep 6, 2017

mremond commented Sep 7, 2017

Minnozz commented Sep 7, 2017

Minnozz commented Sep 7, 2017

ibrasho commented Sep 7, 2017

Minnozz commented Sep 7, 2017

Minnozz commented Sep 8, 2017

darkowlzz left a comment

ibrasho left a comment

darkowlzz commented Sep 9, 2017

Don't assume every git repository has a HEAD #1053

Don't assume every git repository has a HEAD #1053

Conversation

Minnozz commented Aug 23, 2017 • edited by darkowlzz Loading

What does this do / why do we need it?

What should your reviewer look out for in this PR?

Do you need help or clarification on anything?

Which issue(s) does this PR fix?

sdboyer commented Aug 25, 2017

Minnozz commented Aug 25, 2017

sdboyer commented Aug 26, 2017

Minnozz commented Aug 26, 2017 • edited Loading

sdboyer commented Aug 27, 2017

Minnozz commented Aug 31, 2017

darkowlzz commented Sep 1, 2017 • edited Loading

Minnozz commented Sep 3, 2017

darkowlzz left a comment

Choose a reason for hiding this comment

darkowlzz Sep 4, 2017

Choose a reason for hiding this comment

darkowlzz Sep 4, 2017

Choose a reason for hiding this comment

darkowlzz Sep 4, 2017

Choose a reason for hiding this comment

darkowlzz commented Sep 4, 2017

darkowlzz commented Sep 4, 2017

ibrasho commented Sep 6, 2017

mremond commented Sep 7, 2017

Minnozz commented Sep 7, 2017

Minnozz commented Sep 7, 2017

ibrasho commented Sep 7, 2017

Minnozz commented Sep 7, 2017

Minnozz commented Sep 8, 2017

darkowlzz left a comment

Choose a reason for hiding this comment

ibrasho left a comment

Choose a reason for hiding this comment

darkowlzz commented Sep 9, 2017

Minnozz commented Aug 23, 2017 •

edited by darkowlzz

Loading

Minnozz commented Aug 26, 2017 •

edited

Loading

darkowlzz commented Sep 1, 2017 •

edited

Loading