rev-list command implementation for objects #135
Conversation
ajnavarro
commented
Nov 23, 2016
•
ajnavarro
commented
- Stateless method that with a commit list and a repository object get all the reachable objects, ignoring elements into ignore hash list.
- Added tests using basic repository commit tree.
|
|
||
| // NewObjects returns hashes that are into the serverHas commits histories | ||
| // and not into have commits histories | ||
| func (od *ObjectsDiff) NewObjects(have []*Commit, remoteHas []*Commit) ([]plumbing.Hash, error) { |
There was a problem hiding this comment.
Usually in git, what you have is call our and the "remote" storage is call their
There was a problem hiding this comment.
And the method, maybe is better call it Diff
| // NewObjects returns hashes that are into the serverHas commits histories | ||
| // and not into have commits histories | ||
| func (od *ObjectsDiff) NewObjects(have []*Commit, remoteHas []*Commit) ([]plumbing.Hash, error) { | ||
| set := map[plumbing.Hash]bool{} |
There was a problem hiding this comment.
I really prefer map initialization with make
|
|
||
| // ObjectsDiff provides a way to compare two Commit arrays and gets the differences | ||
| // (new commits, trees or files) | ||
| type ObjectsDiff struct { |
There was a problem hiding this comment.
The intent of the struct is not very clear, can you expose the goals better?
| "gopkg.in/src-d/go-git.v4/plumbing" | ||
| ) | ||
|
|
||
| // ObjectsDiff provides a way to compare two Commit arrays and gets the differences |
|
|
||
| // Diff returns hashes that are into 'their' commits histories | ||
| // and not into 'our' commits histories | ||
| func (od *ObjectsDiff) Diff(our []*Commit, their []*Commit) ([]plumbing.Hash, error) { |
There was a problem hiding this comment.
Just a function Diff, without state. No creation of ObjectsDiff.
There was a problem hiding this comment.
Diff -> ObjectsHistoryDiff?
- Stateless method that with a commit list and a repository object get all the reachable objects, ignoring elements into ignore hash list. - Added tests using basic repository commit tree.
ajnavarro
changed the title
|
I refactor the entire functionality. PTAL. |
|
|
||
| // ObjectsDiff checks the differences between two Commit lists and returns a list | ||
| // of hashes that represents all the objects that are in 'their' history, but not in | ||
| // 'our' history. That hashes could be references to commits, trees or files. |
There was a problem hiding this comment.
not files, blobs. What about the tags?
| return nil, err | ||
| } | ||
|
|
||
| result := map[plumbing.Hash]bool{} |
| // RevListObjects gets all the hashes from all the reachable objects from the | ||
| // given commits. Ignore hashes are objects that you don't want back into the | ||
| // result. All that objects must be accessible from the Repository. | ||
| func RevListObjects( |
There was a problem hiding this comment.
Since r is the context, I prefer having Repository as fist method.
| @@ -0,0 +1,115 @@ | |||
| package git | |||
There was a problem hiding this comment.
I prefer revlist.go instead of rev_list.go
|
|
||
| // RevListObjects gets all the hashes from all the reachable objects from the | ||
| // given commits. Ignore hashes are objects that you don't want back into the | ||
| // result. All that objects must be accessible from the Repository. |
There was a problem hiding this comment.
I really don't understand the phrase "Ignore hashes are objects that you don't want back into the result"
| // RevListObjects gets all the hashes from all the reachable objects from the | ||
| // given commits. Ignore param are object hashes that we want to ignore on the | ||
| // result. All that objects must be accessible from the Repository. | ||
| func RevListObjects( |
There was a problem hiding this comment.
Is the operation performed by this function a complementary set?, maybe we should call it just like that.
There was a problem hiding this comment.
Or at least mention it in the comment instead of explaining what a complementary set operation is without mention it.
| func RevListObjects( | ||
| r *Repository, | ||
| commits []*Commit, | ||
| ignore []plumbing.Hash) ([]plumbing.Hash, error) { |
There was a problem hiding this comment.
I think it will make more sense to pass a set of commits and a set of ignores instead of lists. What do you think?
| ignore []plumbing.Hash) ([]plumbing.Hash, error) { | ||
|
|
||
| seen := hashListToSet(ignore) | ||
| result := make(map[plumbing.Hash]bool) |
There was a problem hiding this comment.
Why is this a set but then return a list. Lets use a list instead directly or return a set.
| return result | ||
| } | ||
|
|
||
| func iterateCommits(commit *Commit, cb func(c *Commit) error) error { |
There was a problem hiding this comment.
I got lost pretty quickly with this, it should be easy to read in a single pass.
Please add comments explaining what iterateCommits, iterateCommitTrees and iterateAll do and refactor for legibility. For example iterateAll is just calling iterateCommits with some arguments, usually you don't write a new function for something so simple unless the name is very descriptive, but in this case it is not ('All' does not transmit any meaning, is iterateAll iterating over more thing than iterateCommits, then why call it All?). Why not just substitute iterateAll with a call to iterateCommits with well explained and named arguments?
| // | |/ | ||
| // * | 35e8510 binary file | ||
| // |/ | ||
| // * b029517 Initial commit |
There was a problem hiding this comment.
This comment is awesome, thanks!
There was a problem hiding this comment.
For the record: git log --graph --oneline --all
There was a problem hiding this comment.
if is for the record, then write this as a comment
* rev-list command implementation for objects - Stateless method that with a commit list and a repository object get all the reachable objects, ignoring elements into ignore hash list. - Added tests using basic repository commit tree.
