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+TST: ephemeral clone to account for bare repo #4899
Conversation
In context of RIA stores it's important to be able to have an ephemeral clone of a bare repository. clone --reckless=ephemeral didn't account for that and symlinked into '.git/annex' of a bare repo, which is invalid, of course. Added test for correct symlinking of that clone option and another one for an actual workflow involving a RIA store. (Closes datalad#4893)
Codecov Report
@@ Coverage Diff @@
## maint #4899 +/- ##
==========================================
+ Coverage 89.66% 89.67% +0.01%
==========================================
Files 288 289 +1
Lines 40406 40477 +71
==========================================
+ Hits 36230 36298 +68
- Misses 4176 4179 +3
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thx, this will feel a painfull void. Left a few comment
# the path | ||
# Note, that w/o support for bare repos in GitRepo we also | ||
# can't use ConfigManager ATM. | ||
from datalad.cmd import GitWitlessRunner, StdOutErrCapture |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is not true, and tested to be such:
datalad/datalad/tests/test_config.py
Line 524 in 6cbb11a
def test_bare(path): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is true. GitRepo
can only "handle" a bare repo, that it is actively creating. It can't be instantiated on an existing one. And ConfigManager
requires a repo instance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just looked into fixing GitRepo
/ConfigManager
for it, since it seemed to work for creation. Conclusion: It doesn't! The test you pointed to is misleading. Doesn't actually read a bare repo's config. Instead it pretends to by first writing to it and then read out that value (into and from the wrong location, though).
I'll give it shot, but not within this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See PR #4911
datalad/interface/common_opts.py
Outdated
inodes needs to be minimized. | ||
inodes needs to be minimized. Please note, that you might run into issues | ||
when using this mode to link a bare (or any tuned annex) and a non-bare | ||
repository that way, since their annex object trees are organized differently. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe: Using this mode with bare repositories (outside of a RIA store) or tuned repositories is not supported, because their annex object trees are organized differently.
It will not work. There is no uncertainty
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't think so. First of all: It is supported in that --reckless=ephemeral
will do it's thing. Second, there is uncertainty, since you can have a tuned repo, using dirhashlower. It will work in that case. Third: At some point I tested, that a bare repo can actually deal with dirhashmixed keys in its object tree. It just wouldn't put it in there itself. So, if you use an ephemeral clone to write to it you can also read those keys from another clone. But the linked ephemeral clone can't handle existing dirhashlower objects in the bare repo.
All depends on the setup. Can work or not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But will add "outside RIA store" as a hint.
Failure in datalad-container is known and unrelated. And I'm pretty damn sure the one in crippledFS is unrelated, too (added it to the list). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two exclamation marks?! I would still say that revealing the importance of dirhash_mixed or dirhash method equivalence is sufficient and appropriate, but this will do too.
Somehow we managed to ignore the intial usecase leading to the development of RIA stores and ephemeral clones! facepalm
This fixes
clone
to symlink an ephemeral clone's annex correctly, iforigin
is a bare repo.Tests are enhanced in two ways:
test_clone.py
as this is allclone
itself is concerned with.Ping @mih