Add revision implementation #139
Conversation
| @@ -0,0 +1,112 @@ | |||
| package revision | |||
There was a problem hiding this comment.
the package should be in the root of the package inside of a internal folder so:
revision/internal/revision/ -> internal/revision/
There was a problem hiding this comment.
ah ok I thought you meant at the root of a revision package
|
@mcuadros If I read properly "revision spec" a revision is something like this "master", "HEAD" but not "master~2" for instance. So Revision type will only describe "master" part of an expression like "master~2" so how do you want to store the whole thing, a Revision type is not enough ? |
|
Base on the documentation looks like a revision is Just |
|
Ok, it was this kind of sentence that was letting me wondering, "~, e.g. master~3 : A suffix ~ to a revision parameter". |
Codecov Report@@ Coverage Diff @@
## master #139 +/- ##
==========================================
+ Coverage 75.92% 76.66% +0.73%
==========================================
Files 103 106 +3
Lines 6792 7260 +468
==========================================
+ Hits 5157 5566 +409
- Misses 1074 1105 +31
- Partials 561 589 +28
Continue to review full report at Codecov.
|
|
@antham can you tell us, how far are you with the implementation? You did a lot of work! |
|
I have at the moment implemented almost everything, you can have a look here what we can parse : https://github.com/src-d/go-git/pull/139/files#diff-0e4bd274259ee67fba0f3f2541704aefR78 , I used what was defined in git revision page to do this test. There are some missing like the one where we need to parse a relative date, I can do this later. I still need to complete parse method to check validity of a whole revision for some cases. We need to discuss how to integrate this thing. |
|
Don't worry we can miss some of the features, the whole library doesn't implement all the arcane features, we implement only the more relevant and well known. @antham can you elaborate the cases, I can't understand it sorry. |
|
Yep I get it, but it's better to have all bases even if it's no fully completed and not used yet, someone can resume the job if needed instead of starting from scratch which is painful : it's what I wanted to do at least. I'm sorry but I don't understand what you don't understand :-) @mcuadros |
|
|
Ah ok.
I implemented every chunk parser (e.g.: ^3) and I'm almost done with the whole parser (e.g.: master^2~3), we miss some case check to have something complete, it what remains to do.
Parser extract from a revision string like this : "master~1" a list of chunk like that Do you have any precise idea how to integrate revision in your system with that ? |
|
For the parser I think I'm done, we can improve gradually now. A review work remains to have everything consistent like error message for instance. |
|
You deleted, your branch? All you method and structs are private, I can't recognize any interface, can you document the library and make public the methods. At the level of the public API, for sure we will need:
(the name of the functions are placeholders) This is, at least, the minimum implementation, and with this we can merge this work |
|
Nop still here but I rebased it. There is one interface : revisioner. Yep but it would be nice to give the ability to have the result of parsing a revision, it could be useful. |
|
I forgot to precise but I only implemented revision parser not range parser yet. |
|
@antham if you create the method |
|
Yep I will do that. |
|
@mcuadros I added ResolveRevision with tilde, caret, regexp and date reference solver. Do you want me to add more or it could be enough for a first go ? Examples from tests about what this function can solve (from this repository https://github.com/git-fixtures/basic.git) : "refs/heads/master~2^^~": "b029517f6300c2da0f4b651b8642506cd6aaf45d",
"HEAD~2^^~": "b029517f6300c2da0f4b651b8642506cd6aaf45d",
"HEAD~3^2": "a5b8b09e2f8fcb0bb99d3ccb0958157b40890d69",
"HEAD~3^2^0": "a5b8b09e2f8fcb0bb99d3ccb0958157b40890d69",
"HEAD~2^{/binary file}": "35e85108805c84807bc66a02d91535e1e24b38b9",
"HEAD~^{!-some}": "1669dce138d9b841a518c64b10914d88f5e488ea",
"HEAD@{2015-03-31T11:56:18Z}": "918c48b83bd081e863dbe1b80f8998f058cd8294", |
There was a problem hiding this comment.
Have you watched https://www.youtube.com/watch?v=HxaD_trXwRE?
What do you think about it?
internal/revision/parser.go
Outdated
| tok token | ||
| lit string | ||
| n int | ||
| } |
There was a problem hiding this comment.
It is not clear what buf represents here, can you explain it in a comment or give it a better name?
There was a problem hiding this comment.
by the contents of 118-121 it looks like buf holds the last scanned data, with the purpose of returning it in case an unscan is requested.
If so, please give it good names and/or comments, so people don't have to look at code later to make sense of the code they are reading right now.
There was a problem hiding this comment.
Also n should not be an integer if its only values are 0 and 1. It looks like a flag to tell scan whether to reuse the already scanned data or not.
There was a problem hiding this comment.
buf stands for buffer so I don't really get the point about this name it's rather comment that is not clear at the beginning, but I can rename it to buffer if you think it could be better.
You are right about this value it's a boolean.
There was a problem hiding this comment.
Give buf a good name that explains why is it there, what are you going to use it and how, so everybody understand, at a glance what buf is about, without having to read further into the file, otherwise your code is not clear.
If you cannot find a good name, then please write comments explaning all these things.
There was a problem hiding this comment.
For instance, let's say you are using buf to store the last scanned value, with the intention of having a unscan() method that allows to simulate 'inserting back the last scanned value into the stream'.
The reader of your code, when he reads buf, does not know about unscanning (something that is mentioned later) or how are you going to implement it (with the actual buffer).
So a better name for the buf thing will be something like this:
type Parser struct {
 s *scanner
 lastResult struct { // memoization for unscanning
 tok token
 lit string
 valid bool
}
 }
This explains your intention and how are you going to use it from the very beginning, without having to read further into the file. It also helps to detect bugs, as now, everybody will see that valid is clearly a bool, not an int.
internal/revision/parser.go
Outdated
| } | ||
|
|
||
| // scan returns the next token from the underlying scanner. | ||
| // If a token has been unscanned then read that instead. |
There was a problem hiding this comment.
This comment is misleading, tokens cannot be unscanned, for instance, you can not insert back a token that was read a few scans ago. What you can do is request an unscan to the parser, which will "insert back" the last read token.
I think what you mean here is:
"scan returns the next token from the underlying scanner or the last scanned token if an unscan was requested."
There was a problem hiding this comment.
What will scan return if there are no more data in the scanner?
What will scan return if an unscan is requested before scanning anything? is that even possible?
internal/revision/scanner.go
Outdated
| } | ||
|
|
||
| // read reads the next rune from the bufferred reader. | ||
| // Returns the rune(0) if an error occurs (or io.EOF is returned). |
There was a problem hiding this comment.
Why do we want to hide read errors by returning rune(0)?
How do we tell the difference between a read error an io.EOF when scanning?
Why should we use this function instead of just calling s.r.ReadRune, which gives us a much richer behavior (with errors and everything)?
There was a problem hiding this comment.
read wraps the error and through scan, stop if there is a trouble, but indeed we can add more context returning the error that was trigger from scan if something different than EOF occured.
There was a problem hiding this comment.
Yes please, don't hide errors, otherwise, how are we going to know what went wrong when the error occurs?
internal/revision/parser.go
Outdated
| } | ||
|
|
||
| // unscan pushes the previously read token back onto the buffer. | ||
| func (p *Parser) unscan() { p.buf.n = 1 } |
There was a problem hiding this comment.
p.buf.n should probably be a boolean.
There was a problem hiding this comment.
You should add a check to see if something has been scanned before accepting an unscan request.
internal/revision/parser.go
Outdated
| return "Revision invalid : " + e.s | ||
| } | ||
|
|
||
| // Revisioner represents a revision component |
There was a problem hiding this comment.
And what is a revison component?
There was a problem hiding this comment.
A revision component is a part of a revision, ( we can see how it works here : https://github.com/src-d/go-git/pull/139/files/e29e554853a1850f42f649b96797c5394d65f2fa#diff-0e4bd274259ee67fba0f3f2541704aefR86), so it's all structs defined after this interface (Ref, TildePath, and so on)
There was a problem hiding this comment.
I see, thanks. Can you please explain that in the code or expand the comment at line 23 so everybody understand it?
|
|
||
| // ResolveRevision resolves revision to corresponding hash | ||
| func (r *Repository) ResolveRevision(rev plumbing.Revision) (*plumbing.Hash, error) { | ||
| p := revision.NewParserFromString(string(rev)) |
There was a problem hiding this comment.
I don't understand this.
You have a revision, turn it into a string, then call NewParserFromString.
Why don't you create a parser from the revision directly: p := rev.NewParser().
There was a problem hiding this comment.
Also the fact that a revision is not part of the revision package is very confusing:
The revision package does not define a revision (confusing), it defines a parser for a revision instead (confusing).
Nobody should care about revision parsers, as they should be private code, the revision should have methods resolve itself without forcing you to use its internal parser.
So either the name of the package is wrong (revision should be revisionParser) or things have to be refactored to make sense.
There was a problem hiding this comment.
Also all this code should not be part of repository, revisions should have a way to resolve themselves (passing them a repository).
Should this resolver return a slice of hashes instead to cope with range revision (when they are supported in the future)?
There was a problem hiding this comment.
I followed instruction from mcuadro :
Place all the parser, tokenizer, etc in an internal package called revision at the root of the package. This package deals with strings, not with the type Revision
Something is not clear, can you describe more how you want to organize revision ?
I added this method to repository cause I don't see why you would have to give a repository to solve a revision to a third part method, they are related closely to repository, like references. It seems strange to me to call it somewhere else, or at least if you do so it's great to have something to resolve them directly from repository as a convenience.
There was a problem hiding this comment.
(editted: Github move this comment out of context: this is the context; #139 (comment))
I'm sorry, I don't understand @mcuadros comment either.
Maybe you and he have something in mind about how this is going to be used in the future that I ignore.
Just do what he says.
PS: independently of that, if a revision will represents revisons ranges in the future, this method should return a slice of strings or of hashes, just to take that into account future improvements.
There was a problem hiding this comment.
So you want to keep me thing like they are but add the function he mentionned ?
| // Parse explode a revision string into revisioner chunks | ||
| func (p *Parser) Parse() ([]Revisioner, error) { | ||
| var rev Revisioner | ||
| var revs []Revisioner |
There was a problem hiding this comment.
What is the difference between these two values (rev and revs)?
There was a problem hiding this comment.
rev is a single revision component and revs is a list of revision components
There was a problem hiding this comment.
of course, that is pretty clear by its name, what it is not clear is their purpose, how are you going to use them, etc.
By calling something rev, I can imaging it is a Revision, and by calling something revs I can imaging it is a collections of revisions. But why do they do? for instance, it looks like revs is swhat you are planning to return from this method, so maybe you should call it ret because line 171 is so far away that nobody will see it when reading the revs daclaration.
rev is just a temporal value where you store the current revisioner being returned, so there is not much to explain, but declaring it outside of the loop is very confusing (the same happens with err) and why is rev outside the loop but tok is declared inside every iteration? all those things are confusing, so you have to explain them or do them in a way that are easy to understand without explanations.
There was a problem hiding this comment.
I don't see the point here about revs, revs is declared at the top of the functions close to the return definition it's easy to understand it would be returned at the end, it's subjective but this function is not that big you can easily check the function in one look, I think it's quite understandable. We could name return parameters but I don't really like that it looks a bit magical to me when you find a return and it would fail against what you've just told about revs.
internal/revision/parser.go
Outdated
|
|
||
| switch tok { | ||
| case at: | ||
| p.unscan() |
There was a problem hiding this comment.
Why are you unreading the @ here and then discarding it at parseAt()? (the same for all the parse... functions below)
There was a problem hiding this comment.
It's easier for composition and understanding in my opinion to have functions parsing whole chunks, when I call parseTilde for instance it makes much sense to me to give that "~3" instead of "3"
There was a problem hiding this comment.
You are right, a parseTilde function makes more sense if it parse "~3" than just "3".
But that does not justify the extra work and complexity and the surprise everybody get when they realize you are inserting back a character you just extracted, just to immediately extract it again.
The correct solution is not to call parseTilde at all, you already know you are in a "tilde" thing, because you already read a "~", so you just call parseInteger instead to read the remaining "3".
There was a problem hiding this comment.
I started step by step and with the idea to reuse component but that has no sense so we can drop those things indeed. But you won't call parseTilde, parseInteger, because in this function you parse and return something related to tilde and a tilde could be follow by nothing.
|
|
||
| return revs, nil | ||
| default: | ||
| p.unscan() |
There was a problem hiding this comment.
Why not use the string from the p.scan call instead of unscaning the whole thing and reading it back again in parse Ref?
There was a problem hiding this comment.
For others expressions it's ok because as you enter in a sub function you know what kind of object you will have at the end and you only have to analyze the rest of the expression, unscanning the character is unecessary you don't need this information, but here the current token is part of the whole expression you have to unscan it.
|
|
||
| return &ErrInvalidRevision{`":" statement is not valid, could be : :<n>:<path>`} | ||
| } | ||
| } |
There was a problem hiding this comment.
This is quite thoughtful, thanks for the detailed work involved in making this method!
There was a problem hiding this comment.
It's an iterator on a component revision list to validate a full revision, you want me to add more details in function comment ?
There was a problem hiding this comment.
No, the function is just nice as it is, I was just complimenting your work here.
There was a problem hiding this comment.
Ah ok sorry I thought you meant the opposite.
|
@antham I see you are fixing some of the issues, but I cannot tell if you have finished. Please let me know when you are finished and your code is ready for another review, by leaving a comment here. |
|
@alcortesm sorry I'm quite slow, I try to do few but on a regular basis. I wanted to do this, i will ping you when everything is over |
|
@antham , you are doing an excellent job here and we are so grateful for it. Take your time, work at your own pace, there is not hurry, enjoy it. Just let me know when it is ready for another review by explicitly requesting it on this thread, so I don't have to check your code every morning and guess if it is ready or not. |
|
Ok thank you that's nice. I will but maybe you would be pinged if I answer to your comments spread over this PR as I go so don't be surprised to receive notifications. I will ping you when everything will be done. |
|
👍 |
|
@alcortesm can you come back to have a look please ? I re-reviewed all your comments to be sure I adressed them but it's possible I missed some, let me know. Sorry for this big delay :-( |
|
Thanks for the notice. According to Github, your code has conflicts with the master branch and can not be merged. I will review your changes once you solve the conflicts. |
* add ^{/regexp} revision resolver
* add <rev>@{date} revision resolver
* negation form of regexp solver : <rev>@{/!-regexp}
* use a boolean instead of an integer * rename variable
* merge functionalities in scanner method in main scan method * use directly Reader api * rename char to tokenError * propagate errors triggered in scanner in parser
|
@alcortesm done |
|
Thanks, I'm taking a look at it. |
internal/revision/scanner.go
Outdated
| return word, string(data), nil | ||
| } | ||
|
|
||
| if unicode.IsNumber(ch) { |
There was a problem hiding this comment.
There are two problems with this code:
- the
scanfunction is already too long - the code from line 102-128 looks the same as the code from 71-99, the only difference seems to be one is calling
unicode.IsLetterthe otherunicode.IsNumber.
You can solve both issues by creating a new private function that does all the things you want to do, one of its arguments will be a function from the unicode package (unicode.IsLetter or unicode.IsNumber). Then you will call the new function on both cases reducing the overall scan length, something like this:
// you need a better name for this function
type kindCheckFn func(r rune) bool
// you need a better name for this function.
// Something like `scanKind`, suggesting it scans numbers, letters, or any
// other thing the kindCheckFn thinks it is appropriate.
func yourNewFunction(check kindCheckFn, r bufio.Reader) (...){
... // here goes all your repeated code, but only once
}
func (s *scanner) scan() (token, string, error) {
...
if unicode.IsLetter(ch) {
yourNewFunction(unicode.IsLetter, s.r) // you will need some processing of its errors and returned values too
}
if unicode.IsNumber(ch) {
yourNewFunction(unicode.IsNumber, s.r)
}
I think something along these lines will improve the scan function a lot.
There was a problem hiding this comment.
I was thinking to do a thing like that, but I wa thinking at the end this function won't move much so I changed my mind, I will do this.
|
|
||
| // ResolveRevision resolves revision to corresponding hash | ||
| func (r *Repository) ResolveRevision(rev plumbing.Revision) (*plumbing.Hash, error) { | ||
| p := revision.NewParserFromString(string(rev)) |
There was a problem hiding this comment.
(editted: Github move this comment out of context: this is the context; #139 (comment))
I'm sorry, I don't understand @mcuadros comment either.
Maybe you and he have something in mind about how this is going to be used in the future that I ignore.
Just do what he says.
PS: independently of that, if a revision will represents revisons ranges in the future, this method should return a slice of strings or of hashes, just to take that into account future improvements.
|
@alcortesm I wanted to make changes you requested (except those tied to my last comment) but I have trouble with billy, I opened an issue on the repository. |
|
Ok @alcortesm with the trick of @mcuadros I was able to have everything working I rewrote scanner according to what you stated. For the second comment, I wait for your reply. |
* move agreggation function out of scanner to improve readability
|
Thanks @antham , I'm working on your revision right now. |
mcuadros
changed the title
|
Just merged, will be great having the range revision 👍 |
|
It was long job but we finally succeeded to do it. Yep now we can work on range revision, is it possible to open an issue to have some spec about what you want at the end @mcuadros . |
|
@mcuadros have you seen my last comment ? |
|
Sorry about the radio silence, nowadays I don't have many time to thing about the API, but you can raise a issue or a PR, with your suggest implementation, if you are still interested on contribute. |
|
@mcuadros sure I'm still interested, I was thinking to open an issue in next few days. I'm facing currently an issue with range revision, so I will open a PR when I will have something interesting as starting point. |
Implement Revision entity as type Revision string and not as plain string
Add a method ResolveReference(r *git.Repository, rev Revision) (plumbing.Hash, error)
Place all the parser, tokenizer, etc in an internal package called revision at the root of the package. This package deals with strings, not with the type Revision
Define a revision parser/tokenizer to comply with the """spec""" described here https://www.kernel.org/pub/software/scm/git/docs/gitrevisions.html (if something is hard and arcane this can be put in a TODO, is not required implement all the functionality)
Resource for valid reference names : https://git-scm.com/docs/git-check-ref-format