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

format/packfile: fix bug when the delta depth is equals to 50 #374

Merged
merged 1 commit into from
May 8, 2017
Merged

format/packfile: fix bug when the delta depth is equals to 50 #374

merged 1 commit into from
May 8, 2017

Conversation

ajnavarro
Copy link
Contributor

No description provided.

@@ -197,3 +197,8 @@ func (s *DeltaSelectorSuite) TestObjectsToPack(c *C) {
c.Assert(otp[2].IsDelta(), Equals, true)
c.Assert(otp[2].Depth, Equals, 2)
}

func (s *DeltaSelectorSuite) TestMaxDepth(c *C) {
dsl := s.ds.deltaSizeLimit(0, 0, 50, true)
Copy link
Contributor

Choose a reason for hiding this comment

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

Use maxDepth instead of 50.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we test this on a public function instead? private functions can change (or be removed) at any time, which may remove this test in the future.

Copy link
Contributor Author

@ajnavarro ajnavarro May 3, 2017

Choose a reason for hiding this comment

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

The logic to be able to create a delta with a depth of 50 is really complicated using public methods in deltaSelector (the only one is ObjectsToPack()). That's why I choose to test it using a private method.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@mcuadros
Copy link
Contributor

mcuadros commented May 8, 2017

@ajnavarro @alcortesm ping

@alcortesm
Copy link
Contributor

Changes are still pending, do you want me to ignore them and approve the PR instead?

@codecov
Copy link

codecov bot commented May 8, 2017

Codecov Report

Merging #374 into master will decrease coverage by 0.52%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #374      +/-   ##
==========================================
- Coverage   77.29%   76.76%   -0.53%     
==========================================
  Files         123      122       -1     
  Lines        8751     8604     -147     
==========================================
- Hits         6764     6605     -159     
- Misses       1226     1273      +47     
+ Partials      761      726      -35
Impacted Files Coverage Δ
plumbing/format/packfile/delta_selector.go 91.17% <100%> (+0.26%) ⬆️
plumbing/transport/ssh/common.go 0% <0%> (-50.91%) ⬇️
plumbing/transport/ssh/auth_method.go 32.4% <0%> (-24.08%) ⬇️
options.go 78.57% <0%> (-1.06%) ⬇️
utils/merkletrie/index/node.go 93.54% <0%> (-0.4%) ⬇️
plumbing/object/commit.go 71.3% <0%> (ø) ⬆️
plumbing/object/blob.go 73.58% <0%> (ø) ⬆️
plumbing/memory.go 100% <0%> (ø) ⬆️
worktree_commit.go
worktree_status.go 67.6% <0%> (+0.03%) ⬆️
... and 2 more

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 ced875a...7b3ff58. Read the comment docs.

@ajnavarro
Copy link
Contributor Author

Rebased and using maxDepth instead of 50.

@mcuadros
Copy link
Contributor

mcuadros commented May 8, 2017

@ajnavarro CI is failing

@alcortesm
Copy link
Contributor

plumbing/format/packfile/delta_selector_test.go:202: cannot use maxDepth (type int64) as type int in argument to s.ds.deltaSizeLimit

yes, a go vet issue, it should be easy to fix.

@mcuadros mcuadros merged commit 7cd0215 into src-d:master May 8, 2017
@ajnavarro ajnavarro deleted the fix/delta-encoder-big-deltas branch May 8, 2017 09:30
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.

4 participants