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

BF: Prevent doomed annex calls on files we already know are untracked #7166

Merged
merged 4 commits into from
Nov 14, 2022

Conversation

adswa
Copy link
Member

@adswa adswa commented Nov 10, 2022

Fixes #7032

This change adds a short check if any file in a list of paths given to a datalad status --annex call is untracked. If it is, it is removed from the list of paths given to the annex query, as we already know that this annex call would fail.
It does not prevent the status report of being untracked:

❱ datalad status --annex basic my some moreuntracked 
untracked: moreuntracked (file)
untracked: my (file)
1 annex'd file (7.0 B recorded total size)

If untracked paths are excluded from this query, a trace of it is left in the debug log.

This PR also adds a small test.

Fixes datalad#7032. When datalad status --annex is called on an untracked
file specifically, it will invoke an annex query that we already know
will fail. This commit adds a check if any specifically added file as a
path is untracked, and if so, prevent the annex query from being run on
it.
@adswa adswa added semver-patch Increment the patch version when merged CHANGELOG-missing When a PR's description does not contain a changelog item, yet. labels Nov 10, 2022
@github-actions github-actions bot removed the CHANGELOG-missing When a PR's description does not contain a changelog item, yet. label Nov 10, 2022
@codecov
Copy link

codecov bot commented Nov 10, 2022

Codecov Report

Base: 88.88% // Head: 90.92% // Increases project coverage by +2.03% 🎉

Coverage data is based on head (6013617) compared to base (209bc31).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##            maint    #7166      +/-   ##
==========================================
+ Coverage   88.88%   90.92%   +2.03%     
==========================================
  Files         355      355              
  Lines       46514    46524      +10     
  Branches     6328     6331       +3     
==========================================
+ Hits        41345    42302     +957     
+ Misses       5154     4207     -947     
  Partials       15       15              
Impacted Files Coverage Δ
datalad/core/local/status.py 94.30% <100.00%> (+0.14%) ⬆️
datalad/core/local/tests/test_status.py 97.88% <100.00%> (+0.09%) ⬆️
datalad/_version.py 45.68% <0.00%> (ø)
datalad/cmd.py 94.07% <0.00%> (+0.37%) ⬆️
datalad/__init__.py 98.00% <0.00%> (+16.00%) ⬆️
datalad/tests/utils.py 56.12% <0.00%> (+56.12%) ⬆️
datalad/tests/test_tests_utils.py 92.34% <0.00%> (+92.34%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

v['state'] == 'untracked']
lgr.debug(
'Skipping %s.get_content_annexinfo() for untracked paths: %s',
repo, paths)
Copy link
Member

Choose a reason for hiding this comment

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

even though only for DEBUG I wonder if it is wise to include all paths -- what is there is 1000s of them? do we ever log all paths . I would just log number of them I guess, or may be we have some helper to show like first N and then ... ?

Copy link
Member Author

@adswa adswa Nov 11, 2022

Choose a reason for hiding this comment

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

Oh, I agree that this would be sensible. I guess it never occurred to me because the very next line in that piece of code actually does report all paths:

 lgr.debug('Querying %s.get_content_annexinfo() for paths: %s', repo, paths)

I guess this would probably benefit equally from such a helper. But I'm not aware of one...

Copy link
Member

@bpoldrack bpoldrack left a comment

Choose a reason for hiding this comment

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

Looks alright to me!

Are you planning to address the logging, @adswa? Otherwise I'm fine with merge - logging can be addressed independently.

@adswa
Copy link
Member Author

adswa commented Nov 14, 2022

I don't have a good idea how to do the logging. As it would be necessary also in other parts of that code, I'd be in favour of addressing this outside of this PR.

changelog.d/pr-7166.md Outdated Show resolved Hide resolved
No need for BF mentioned in section 'Bug Fixes'
@bpoldrack
Copy link
Member

Minor change to changelog - will merge w/o rerunning tests.

@bpoldrack bpoldrack merged commit 1e857f6 into datalad:maint Nov 14, 2022
@yarikoptic-gitmate
Copy link
Collaborator

PR released in 0.17.10

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-patch Increment the patch version when merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

status --annex basic <untrackedfile> fails with unnecessary annex call
4 participants