Skip to content
This repository was archived by the owner on Sep 11, 2020. It is now read-only.

Add Stats() to Commit #613

Merged
merged 2 commits into from
Nov 8, 2017
Merged

Add Stats() to Commit #613

merged 2 commits into from
Nov 8, 2017

Conversation

darkowlzz
Copy link
Contributor

@darkowlzz darkowlzz commented Oct 6, 2017

Stats() is similar to git show --stat <hash>.

This introduces a new type FileStat which stores the stats of a file patch. It can be printed to have

common.go | 4 ++++
main.go | 26 ++++++++++++++++++++------

git cli style printing of addition and deletion.

Need help with:

  1. Figuring out when a file is renamed. I couldn't find any existing code that does it. I came across merkletrie.Action which supports "Insert", "Delete" and "Modify" only. And it made me assume file rename is not supported.
  2. How to get patch for the first commit without any parent? To get a patch, I see 2 commits are used. Any pointers on what to do for patch of the first commit?

fixes #482

@codecov
Copy link

codecov bot commented Oct 6, 2017

Codecov Report

Merging #613 into master will decrease coverage by 0.62%.
The diff coverage is 70.42%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #613      +/-   ##
==========================================
- Coverage    78.3%   77.67%   -0.63%     
==========================================
  Files         130      130              
  Lines       10231    10302      +71     
==========================================
- Hits         8011     8002       -9     
- Misses       1358     1448      +90     
+ Partials      862      852      -10
Impacted Files Coverage Δ
plumbing/object/commit.go 67.72% <33.33%> (-3.61%) ⬇️
plumbing/object/patch.go 82.39% <80.35%> (-1.33%) ⬇️
plumbing/transport/ssh/common.go 20.54% <0%> (-45.21%) ⬇️
plumbing/transport/ssh/auth_method.go 31.57% <0%> (-22.81%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0672868...11162e1. Read the comment docs.

@mcuadros
Copy link
Contributor

mcuadros commented Oct 7, 2017

@ajnavarro you was working with diff, maybe you can give a hand here.

@mcuadros
Copy link
Contributor

mcuadros commented Oct 7, 2017

@darkowlzz about comparing the parent tree against void, just use a empty Noder

@ajnavarro ajnavarro self-requested a review October 9, 2017 07:30
Copy link
Contributor

@ajnavarro ajnavarro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe is worth it to add this Stats() method to the Patch object. Doing this we can get Stats between any Commit that we want, or even between trees or files.


for _, chunk := range fp.Chunks() {
switch chunk.Type() {
case 1:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should use the Operation constants, just in case that they will change in the future:

https://github.com/darkowlzz/go-git/blob/9f101c0093390406b5fbfe9809e1cdfcbfba9ee9/plumbing/format/diff/patch.go#L13

}

// FileStats is a collection of FileStat.
type FileStats []FileStat
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe FileStats should have a String() method too

totalChanges := fs.Addition + fs.Deletion
adds := strings.Repeat("+", fs.Addition)
dels := strings.Repeat("-", fs.Deletion)
return fmt.Sprintf(" %s | %d %s%s", fs.Name, totalChanges, adds, dels)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This output is not correct. totalChanges is correct, but the number of + and - is not. Should be the ratio of added and removed lines. See man diffstat for more info.

Also: https://stackoverflow.com/questions/7024848/git-diff-stat-explanation

@ajnavarro
Copy link
Contributor

Figuring out when a file is renamed. I couldn't find any existing code that does it. I came across merkletrie.Action which supports "Insert", "Delete" and "Modify" only. And it made me assume file rename is not supported.

Yes, you're right. Rename is not actually supported.

@mcuadros
Copy link
Contributor

We can move it to Patch but keep this method, as a shortcut.

emptyNoder := treeNoder{}
parentCommit = &Commit{
Hash: emptyNoder.hash,
// TreeHash: emptyNoder.parent.Hash,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mcuadros I think I'm not using it the way you meant.
This is for the first commit scenario, no parent commit.
Without assigning a TreeHash I get panic: object not found.
And if I uncomment the above line, it panics with invalid memory address error.

Missing something really simple? 😅

@darkowlzz
Copy link
Contributor Author

darkowlzz commented Oct 11, 2017

Thanks for the review.

I have moved FileStat & FileStats to patch.go and added Stats() to Patch as well.

Also, created printStat() function to print FileStats considering line length to be 72-80. I read that diffstat result varies based on the screen size. Should we too do that?
And, have I made the +/- output too complex? 😅

@mcuadros
Copy link
Contributor

mcuadros commented Nov 2, 2017

Can you rebase @darkowlzz, thanks!

@darkowlzz
Copy link
Contributor Author

Rebased!

@mcuadros
Copy link
Contributor

mcuadros commented Nov 4, 2017

@ajnavarro PTAL

@ajnavarro ajnavarro self-requested a review November 6, 2017 17:08
@mcuadros mcuadros merged commit e2791ac into src-d:master Nov 8, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

retrieving the stats on a commit
3 participants