fix Repository.ResolveRevision for branch and tag #660
Conversation
| if c == nil { | ||
| return &plumbing.ZeroHash, fmt.Errorf(`No commit exists prior to date "%s"`, date.String()) | ||
| } | ||
|
|
There was a problem hiding this comment.
I removed this part because it's a bad implementation,I misread the spec, it must follows this : This suffix may only be used immediately following a ref name and the ref must have an existing log ($GIT_DIR/logs/<ref>).
repository.go
Outdated
| "refs/tags/%s", | ||
| "refs/heads/%s", | ||
| "refs/remotes/%s", | ||
| "refs/remotes/%s/HEAD", |
There was a problem hiding this comment.
This part exists already in plumbing, I was wondering if it would be possible to move all this code related to revision in plumbing ?
There was a problem hiding this comment.
Maybe we can make this public:
Lines 18 to 27 in 902347a
There was a problem hiding this comment.
Don't you think we could replace those litteral strings with constants defined just right above to avoid duplication ?
antham
changed the title
example_test.go
Outdated
| fmt.Printf("%s %s\n", h.String(), revision) | ||
| } | ||
|
|
||
| // Output: |
There was a problem hiding this comment.
I saw in other examples, you have // Example Output instead of // Output, so examples are not tested, is it a mistake or something you explicitly did ?
There was a problem hiding this comment.
If the examples are in _examples folder are tested in a different way. But if is a example in other folder is an error
antham
changed the title
| "HEAD~^{/!-some}": "1669dce138d9b841a518c64b10914d88f5e488ea", | ||
| "master": "6ecf0ef2c2dffb796033e5a02219af86ec6584e5", | ||
| "branch": "e8d3ffab552895c19b9fcf7aa264d277cde33881", | ||
| "v1.0.0": "6ecf0ef2c2dffb796033e5a02219af86ec6584e5", |
There was a problem hiding this comment.
This tag exists here but I can't find it on https://github.com/git-fixtures/basic.git
There was a problem hiding this comment.
Extrange... but trust on the fixture.
There was a problem hiding this comment.
maybe you could add it to the original repository ? It's convenient when you need to test on the repository and it will fit what is defined in fixtures.
antham
changed the title
|
@mcuadros can you review this PR pls ? |
example_test.go
Outdated
| @@ -85,6 +85,36 @@ func ExampleRepository_References() { | |||
|
|
|||
| } | |||
|
|
|||
| func ExampleRepository_ResolveRevision() { | |||
There was a problem hiding this comment.
I prefer having this example on the _examples folder, following the format.
repository.go
Outdated
| "refs/tags/%s", | ||
| "refs/heads/%s", | ||
| "refs/remotes/%s", | ||
| "refs/remotes/%s/HEAD", |
There was a problem hiding this comment.
Maybe we can make this public:
Lines 18 to 27 in 902347a
| "HEAD~^{/!-some}": "1669dce138d9b841a518c64b10914d88f5e488ea", | ||
| "master": "6ecf0ef2c2dffb796033e5a02219af86ec6584e5", | ||
| "branch": "e8d3ffab552895c19b9fcf7aa264d277cde33881", | ||
| "v1.0.0": "6ecf0ef2c2dffb796033e5a02219af86ec6584e5", |
There was a problem hiding this comment.
Extrange... but trust on the fixture.
|
@antham ping |
Branch and tag were parsed properly by internal revision parser but not resolved correctly into a git hash cause actual reference parser is based on "absolute path" like for instance to reach a branch named test1 it would be refs/heads/test1, the canonical way of resolving such a revision was not implemented
|
@mcuadros you can come back, AppVeyor crashs but it doesn't seem related to my changes |
|
Now AppVeyor works but there is an error with one of the build with travis.... |
mcuadros
changed the title
|
@mcuadros did you see my last comments in this PR about RefRevParseRules and the tag on basic repository ? |
Branch and tag revision are properly parsed by revision lexer but not properly solved into references.
#599