RF update (focus: single dataset operations) #1304
Conversation
(according to GitRepo.fetch() docs)
Codecov Report
@@ Coverage Diff @@
## master #1304 +/- ##
==========================================
- Coverage 89.18% 89.16% -0.02%
==========================================
Files 237 237
Lines 24293 24396 +103
==========================================
+ Hits 21666 21753 +87
- Misses 2627 2643 +16
Continue to review full report at Codecov.
|
mih
changed the title
|
Hmm, 'thighting"... sounds good, will do that more |
|
Ok, this works better than what we had before. I will merge this some time this weekend. |
| testfname = opj(ds.path, 'load.dat') | ||
| assert_false(ds.repo.file_has_content(testfname)) | ||
| ds.get('.') | ||
| ok_file_has_content(opj(ds.path, 'load.dat'), 'heavy') |
There was a problem hiding this comment.
I'm a bit confused here. 'load.dat' was originally added to a plain git repo in line 152, right?
Where is the point it became an annexed file, that we are able to annex-get?
There was a problem hiding this comment.
Indeed. Evolution of that test somewhat distorted the original idea. Conveniently, ds.repo.file_has_content(testfname) returns False for a file that is guaranteed to have its content with it. So we can leave it as is!
| # annex, would would be the case when we presently have a git repo | ||
| # and the recent fetch brought evidence for a remote annex | ||
| if not isinstance(repo, AnnexRepo) and knows_annex(repo.path): | ||
| lgr.info("Init annex at '%s' prior merge.", repo.path) |
There was a problem hiding this comment.
I think, this should be done be re-requesting ds.repo, since that property already has a check for whether it became an annex. This check in Dataset.repo should use knows_annex though, since it currently is just looking for a local git-annex' branch.
There was a problem hiding this comment.
I assume you meant something like this:
diff --git a/datalad/distribution/update.py b/datalad/distribution/update.py
index 49a03c28..fe814705 100644
--- a/datalad/distribution/update.py
+++ b/datalad/distribution/update.py
@@ -145,7 +145,9 @@ def _update_repo(ds, remote, merge, fetch_all, reobtain_data):
# and the recent fetch brought evidence for a remote annex
if not isinstance(repo, AnnexRepo) and knows_annex(repo.path):
lgr.info("Init annex at '%s' prior merge.", repo.path)
- repo = AnnexRepo(repo.path, create=False)
+ # re-requesting the repo will notice the type change
+ ds._repo = None
+ repo = ds.repo
lgr.info("Merging updates...")
if isinstance(repo, AnnexRepo):
if reobtain_data:
| ('content', content)): | ||
| args.append('--{}{}'.format('' if arg else 'no-', label)) | ||
| for label, arg in (('all', all), ('fast', fast)): | ||
| if arg: |
There was a problem hiding this comment.
lines 1143 - 1146 are equivalent to:
from datalad.support.gitrepo import to_options
args.extend(to_options(all=all, fast=fast))Just saying - we should use it more often ;-)
There was a problem hiding this comment.
For the more complex thing above (--no-something vs --something) it could also be used:
to_options(push=push, no_push=not push, ...)There was a problem hiding this comment.
Did the first. For the second I would need to be convinced that a causal reader has an easier time decoding with it wants to say compared to the present implementation.
|
Well, that went south quickly https://travis-ci.org/datalad/datalad/jobs/203121067 I will revert the changes and repush. When there is more time, we can investigate why exactly the predicted behavior was not the observed one. |
|
So in summary, I reverted all changes that I made in response to Ben's comments. I will merge this now, as it is still better than what it was. I won't have time to get to this again this week and I don't won't to have it dangling... |
updateis now practically a frontend forannex sync.Please comment @bpoldrack @yarikoptic
--reobtain-dataupdatecannot update a plain git repo to an annex #1307From my PoV this step is complete. There is more TODO, (#1199) but there is no need to wait for it .