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

CLN: Remove obsolete AnnexRepo.commit() #4168

Merged
merged 2 commits into from Feb 20, 2020
Merged

Conversation

mih
Copy link
Member

@mih mih commented Feb 20, 2020

The GitRepo implementation should be capable of doing what is needed in
the rest of the codebase.

Removing it helps with gh-2970 and makes gh-4127 obsolete by fixing
gh-4126 too.

The GitRepo implementation should be capable of doing what is needed in
the rest of the codebase.

Removing it helps with dataladgh-2970 and makes dataladgh-4127 obsolete by fixing
dataladgh-4126 too.
@mih
Copy link
Member Author

mih commented Feb 20, 2020

I do not see this Travis error locally with git-annex 7.20191114-1, nor with 7.20191230

FAIL: datalad.support.tests.test_annexrepo.test_commit_annex_commit_changed(True,)
1185----------------------------------------------------------------------
1186Traceback (most recent call last):
1187  File "/home/travis/virtualenv/python3.5.6/lib/python3.5/site-packages/nose/case.py", line 198, in runTest
1188    self.test(*self.arg)
1189  File "/home/travis/virtualenv/python3.5.6/lib/python3.5/site-packages/datalad/tests/utils.py", line 586, in newfunc
1190    return t(*(arg + (d,)), **kw)
1191  File "/home/travis/virtualenv/python3.5.6/lib/python3.5/site-packages/datalad/support/tests/test_annexrepo.py", line 2169, in check_commit_annex_commit_changed
1192    , untracked=['untracked']
1193  File "/home/travis/virtualenv/python3.5.6/lib/python3.5/site-packages/datalad/tests/utils.py", line 363, in ok_clean_git
1194    eq_(sorted(index_modified_), sorted(index_modified))
1195AssertionError: ['tobechanged-annex', 'tobechanged-git', 'willgetshort'] != ['tobechanged-annex', 'tobechanged-git']

nor this one:

ERROR: datalad.support.tests.test_annexrepo.test_commit_annex_commit_changed(True,)
1187----------------------------------------------------------------------
1188Traceback (most recent call last):
1189  File "/home/travis/virtualenv/python3.7.1/lib/python3.7/site-packages/nose/case.py", line 198, in runTest
1190    self.test(*self.arg)
1191  File "/home/travis/virtualenv/python3.7.1/lib/python3.7/site-packages/datalad/tests/utils.py", line 586, in newfunc
1192    return t(*(arg + (d,)), **kw)
1193  File "/home/travis/virtualenv/python3.7.1/lib/python3.7/site-packages/datalad/support/tests/test_annexrepo.py", line 2165, in check_commit_annex_commit_changed
1194    ar.commit("message", files=['alwaysbig', 'willgetshort'])
1195  File "/home/travis/virtualenv/python3.7.1/lib/python3.7/site-packages/datalad/support/gitrepo.py", line 1787, in commit
1196    stdin=None,
1197  File "/home/travis/virtualenv/python3.7.1/lib/python3.7/site-packages/datalad/cmd.py", line 405, in run
1198    stderr=output[1],
1199datalad.support.exceptions.CommandError: CommandError: command '['git', 'commit', '-m', 'message', '--', 'alwaysbig', 'willgetshort']' failed with exitcode 1
1200Failed to run ['git', 'commit', '-m', 'message', '--', 'alwaysbig', 'willgetshort'] at '/tmp/datalad_temp_tree_check_commit_annex_commit_changedeckr_12e'.
1201stdout=
1202stderr=git-annex: Cannot make a partial commit with unlocked annexed files. You should `git annex add` the files you want to commit, and then run git commit.

Instead of digging up the reasons, I have first RF'ed the test to use Repo.save() instead of commit -- which is what all top-level "save" code goes through.

The former was removed, and the latter is what is used by top-level
code. It makes sense to me to test the more relevant code paths.
@codecov
Copy link

codecov bot commented Feb 20, 2020

Codecov Report

Merging #4168 into master will decrease coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4168      +/-   ##
==========================================
- Coverage   89.13%   89.12%   -0.02%     
==========================================
  Files         275      275              
  Lines       35847    35814      -33     
==========================================
- Hits        31953    31918      -35     
- Misses       3894     3896       +2     
Impacted Files Coverage Δ
datalad/support/gitrepo.py 90.22% <0.00%> (-0.44%) ⬇️
datalad/support/tests/utils.py 96.15% <0.00%> (+11.53%) ⬆️

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 bc05be3...abcb402. Read the comment docs.

@mih mih added the merge-if-ok OP considers this work done, and requests review/merge label Feb 20, 2020
@kyleam
Copy link
Collaborator

kyleam commented Feb 20, 2020

Instead of digging up the reasons, I have first RF'ed the test to use Repo.save() instead of commit -- which is what all top-level "save" code goes through.

The reason is in this comment in gh-4126, which points to 1a609d4 (BF: annexrepo: Avoid typechange when using temporary index, 2019-04-30) and 205b2ca (TST: rev-save: Demonstrate partial commit failure, 2019-04-30).

It fails for the reason that AnnexRepo.commit continued to use the temporary index after direct mode was gone. The new save partially deals with this, but 205b2ca (TST: rev-save: Demonstrate partial commit failure, 2019-04-30) shows where interface.save/AnnexRepo.commit would pass and the new approach wouldn't. Though none of this will be relevant once the minimum git-annex version is a version that auto-upgrades to v7+ repos.

kyleam
kyleam approved these changes Feb 20, 2020
@mih
Copy link
Member Author

mih commented Feb 20, 2020

Thanks for the clarification and the review @kyleam !

@mih mih merged commit 0f814ab into datalad:master Feb 20, 2020
3 of 4 checks passed
@mih mih deleted the cln-annexcommit branch Feb 20, 2020
@yarikoptic yarikoptic added this to the 0.13.0 milestone Feb 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge-if-ok OP considers this work done, and requests review/merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants