Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix a bug where GitRepository.compareTrees() operation is not cached. #382

Merged
merged 1 commit into from
Apr 4, 2019

Conversation

trustin
Copy link
Member

@trustin trustin commented Apr 3, 2019

Motivation:

Even if we enabled cache, null can be set to GitRepository.cache
depending on how a GitRepository was initialized. If it was newly
created, cache will be set properly. Otherwise, it won't be.

Modifications:

  • Removed the childArgs from DirectoryBasedStorageManager and made
    its subtypes call init() method explicitly, so that it is less
    error-prone.
  • Added a test case that makes sure GitRepository.cache is non-null
    for both code path.

Result:

  • GitRepository.compareTrees() operation is cached properly.

@trustin trustin added the defect label Apr 3, 2019
@trustin trustin added this to the 0.39.0 milestone Apr 3, 2019
Copy link
Contributor

@hyangtack hyangtack left a comment

Choose a reason for hiding this comment

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

Nice work!

Motivation:

Even if we enabled cache, `null` can be set to `GitRepository.cache`
depending on how a `GitRepository` was initialized. If it was newly
created, cache will be set properly. Otherwise, it won't be.

Modifications:

- Removed the `childArgs` from `DirectoryBasedStorageManager` and made
  its subtypes call `init()` method explicitly, so that it is less
  error-prone.
- Added a test case that makes sure `GitRepository.cache` is non-null
  for both code path.

Result:

- `GitRepository.compareTrees()` operation is cached properly.
@codecov
Copy link

codecov bot commented Apr 4, 2019

Codecov Report

Merging #382 into master will decrease coverage by 0.08%.
The diff coverage is 70.96%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #382      +/-   ##
============================================
- Coverage     67.08%   66.99%   -0.09%     
+ Complexity     2851     2848       -3     
============================================
  Files           315      315              
  Lines         11902    11903       +1     
  Branches       1280     1282       +2     
============================================
- Hits           7984     7975       -9     
- Misses         3151     3158       +7     
- Partials        767      770       +3
Impacted Files Coverage Δ Complexity Δ
...r/internal/storage/repository/RepositoryCache.java 73.33% <ø> (ø) 9 <0> (ø) ⬇️
...internal/storage/repository/git/GitRepository.java 81.15% <ø> (ø) 153 <0> (ø) ⬇️
...l/storage/repository/git/GitRepositoryManager.java 67.74% <100%> (-2.41%) 11 <5> (ø)
...nternal/storage/project/DefaultProjectManager.java 94.11% <100%> (ø) 10 <4> (ø) ⬇️
...internal/storage/DirectoryBasedStorageManager.java 64.28% <43.75%> (-3.29%) 33 <4> (-1)
...centraldogma/server/internal/api/WatchService.java 84.84% <0%> (-6.07%) 10% <0%> (-1%)
.../centraldogma/internal/client/AbstractWatcher.java 66.66% <0%> (-0.75%) 27% <0%> (-1%)

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 7c5664c...c068582. Read the comment docs.

@codecov
Copy link

codecov bot commented Apr 4, 2019

Codecov Report

Merging #382 into master will decrease coverage by 0.04%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #382      +/-   ##
============================================
- Coverage     67.08%   67.03%   -0.05%     
+ Complexity     2851     2850       -1     
============================================
  Files           315      315              
  Lines         11902    11903       +1     
  Branches       1280     1280              
============================================
- Hits           7984     7979       -5     
- Misses         3151     3160       +9     
+ Partials        767      764       -3
Impacted Files Coverage Δ Complexity Δ
...r/internal/storage/repository/RepositoryCache.java 73.33% <ø> (ø) 9 <0> (ø) ⬇️
...internal/storage/repository/git/GitRepository.java 81.15% <ø> (ø) 153 <0> (ø) ⬇️
...l/storage/repository/git/GitRepositoryManager.java 70.58% <100%> (+0.43%) 11 <0> (ø) ⬇️
...centraldogma/server/internal/api/WatchService.java 84.84% <0%> (-6.07%) 10% <0%> (-1%)
...rage/repository/git/CacheableCompareTreesCall.java 51.51% <0%> (-3.04%) 6% <0%> (-1%)
.../centraldogma/internal/client/AbstractWatcher.java 65.18% <0%> (-2.23%) 29% <0%> (+1%)

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 7c5664c...c068582. Read the comment docs.

Copy link
Contributor

@hyangtack hyangtack left a comment

Choose a reason for hiding this comment

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

Still LGTM :-)

@trustin trustin merged commit 6c1835e into line:master Apr 4, 2019
@trustin trustin deleted the fix_missing_cache branch April 4, 2019 06:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants