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

Retire obsolete code in Repo #3330

Merged
merged 8 commits into from Apr 15, 2019

Conversation

@mih
Copy link
Member

commented Apr 13, 2019

  • duplicate get_status() functionality
  • Repo.is_dirty() -- hardly used, and replaced by Repo.status()
  • Repo.untracked_files is kept, but reimplemented via Repo.status(), but special AnnexRepo variant is gone
  • tests associated with removed code are gone too

mih added some commits Apr 13, 2019

RF: Reimplement GitRepo.untracked_files without GitPy
```
GitPy
%timeit dl.Dataset('.').repo.untracked_files
10 loops, best of 3: 78.9 ms per loop

DataLad
%timeit dl.Dataset('.').repo.untracked_files
10 loops, best of 3: 69.1 ms per loop
```
RF: Retire `Repo.is_dirty()`
There is still Repo.dirty. Repo.status() replaces both, and dirty is
implemented using status().
RF: Retire `AnnexRepo.get_status()`
Replaced by Repo.status()
@codecov

This comment has been minimized.

Copy link

commented Apr 14, 2019

Codecov Report

Merging #3330 into master will decrease coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3330      +/-   ##
==========================================
- Coverage   91.14%   91.13%   -0.01%     
==========================================
  Files         263      263              
  Lines       34246    34126     -120     
==========================================
- Hits        31212    31100     -112     
+ Misses       3034     3026       -8
Impacted Files Coverage Δ
datalad/plugin/export_to_figshare.py 20.71% <ø> (ø) ⬆️
datalad/interface/tests/test_download_url.py 100% <100%> (ø) ⬆️
datalad/support/annexrepo.py 87.51% <100%> (+0.19%) ⬆️
datalad/interface/run.py 100% <100%> (ø) ⬆️
datalad/distribution/tests/test_uninstall.py 99.68% <100%> (ø) ⬆️
datalad/support/gitrepo.py 89.19% <100%> (+0.07%) ⬆️
datalad/interface/utils.py 95.29% <100%> (ø) ⬆️
datalad/interface/add_archive_content.py 90.38% <100%> (ø) ⬆️
datalad/support/tests/test_annexrepo.py 96% <100%> (-0.39%) ⬇️
datalad/metadata/extractors/xmp.py 92.59% <0%> (-1.41%) ⬇️
... and 15 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 94aadb2...de5a756. Read the comment docs.

@mih

This comment has been minimized.

Copy link
Member Author

commented Apr 14, 2019

I am clueless how these changes affect this, now failing, test: https://travis-ci.org/datalad/datalad/jobs/519927555#L1779

Something in the way that dirty is evaluated, but somehow indirectly. Any hints are most appreciated.

@kyleam

This comment has been minimized.

Copy link
Member

commented Apr 14, 2019

I am clueless how these changes affect this, now failing, test: https://travis-ci.org/datalad/datalad/jobs/519927555#L1779

It has something to do with the addurl batch process state. The old ar.get_status() triggered ar.precommit(). Not a proper solution, but you can see that the test would pass with

diff --git a/datalad/support/gitrepo.py b/datalad/support/gitrepo.py
index 810de4938..54f6b1a6f 100644
--- a/datalad/support/gitrepo.py
+++ b/datalad/support/gitrepo.py
@@ -3038,6 +3038,7 @@ def status(self, paths=None, untracked='all', eval_submodule_state='full'):
           `state`
             Can be 'added', 'untracked', 'clean', 'deleted', 'modified'.
         """
+        self.precommit()
         lgr.debug('Query status of %r for %s paths',
                   self, len(paths) if paths else 'all')
         return self.diffstatus(
@@ -126,6 +126,7 @@ def test_download_url_dataset(toppath, topurl, path):
@with_tempfile(mkdir=True)
def test_download_url_archive(toppath, topurl, path):
ds = Dataset(path).rev_create()
# TODO opj for URLs?!

This comment has been minimized.

Copy link
@kyleam

kyleam Apr 14, 2019

Member

Please silently fix up my stupidity rather than bringing attention to it :>

@mih

This comment has been minimized.

Copy link
Member Author

commented Apr 14, 2019

@kyleam Thx for the pointer! I was blind.

@mih

This comment has been minimized.

Copy link
Member Author

commented Apr 14, 2019

I added the precommit() to GitRepo.get_content_info(ref=None) to catch any query on the work tree state. I hope this catches all relevant cases and, at the same time, minimizes performance impact. Entirely based on gut feeling.

@kyleam

kyleam approved these changes Apr 14, 2019

Copy link
Member

left a comment

Looks good, especially this part:

34 insertions(+), 448 deletions(-)
@mih

This comment has been minimized.

Copy link
Member Author

commented Apr 14, 2019

Looks good, especially this part:

34 insertions(+), 448 deletions(-)

I have high hopes for #3333 too!

@mih mih merged commit a038f0e into datalad:master Apr 15, 2019

5 checks passed

WIP ready for review
Details
codecov/patch 100% of diff hit (target 91.14%)
Details
codecov/project Absolute coverage decreased by -<.01% but relative coverage increased by +8.85% compared to 94aadb2
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@mih mih deleted the mih:rf-repo branch Apr 15, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.