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

Towards (minimal) support for bare repos #4911

Merged
merged 7 commits into from Oct 19, 2020

Conversation

bpoldrack
Copy link
Member

@bpoldrack bpoldrack commented Sep 9, 2020

This PR aims to add minimal support for bare repositories to GitRepo. We should now be able to properly create one (not just pass --bare to git-init, which is insufficient) and instantiate on an existing bare repo. Added bare property, so GitRepo is aware and we can enhance it to react to that knowledge from within its methods. That property does not only check the config but also does an additional consistency check on pathobj and dot_git (should be cheap enough, I think).

Please have a look, @datalad/developers

datalad/support/gitrepo.py Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Sep 9, 2020

Codecov Report

Merging #4911 into master will decrease coverage by 0.02%.
The diff coverage is 98.03%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4911      +/-   ##
==========================================
- Coverage   89.79%   89.76%   -0.03%     
==========================================
  Files         292      292              
  Lines       41000    41044      +44     
==========================================
+ Hits        36815    36843      +28     
- Misses       4185     4201      +16     
Impacted Files Coverage Δ
datalad/support/gitrepo.py 90.47% <91.66%> (-0.02%) ⬇️
datalad/config.py 96.71% <100.00%> (+0.05%) ⬆️
datalad/core/distributed/clone.py 88.69% <100.00%> (-0.04%) ⬇️
datalad/support/tests/test_gitrepo.py 99.78% <100.00%> (+<0.01%) ⬆️
datalad/tests/test_config.py 100.00% <100.00%> (ø)
datalad/downloaders/http.py 81.85% <0.00%> (-2.71%) ⬇️
datalad/downloaders/tests/test_http.py 88.22% <0.00%> (-1.93%) ⬇️

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 f60bc6a...84ca240. Read the comment docs.

Copy link
Contributor

@kyleam kyleam left a comment

Choose a reason for hiding this comment

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

Thanks for working on this.

Subject: [PATCH 2/7] BF+TST: ephemeral clone to account for bare repo

This patch is in master now (via gh-4899), and that series is presumably the source of the current conflicts.

return True
elif not self.config.getbool("core", "bare") and \
not self.pathobj == self.dot_git:
return False
Copy link
Contributor

@kyleam kyleam Sep 15, 2020

Choose a reason for hiding this comment

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

There's also rev-parse --is-bare-repository. That goes through environment:is_bare_repository(), which looks to boil down to a check that core.bare is not false and that there is no working tree is set.

But I think sticking with the config query here is safe and makes sense to take advantage of the config caching.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, in those checking spots anything that isn't cached would hurt us wrt performance.

else:
# make sure we run the git config calls in the dataset
# to pick up the right config files
run_kwargs['cwd'] = dataset.path
Copy link
Contributor

Choose a reason for hiding this comment

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

I think I'm missing something because this code seems to be covered and didn't error, but doesn't this arm leave _runner undefined?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think you're right, @kyleam. Thanks for spotting that.
It might not error, because this only happens, when we instantiate a Dataset.config for a dataset that wasn't created yet (has no .repo), so we probably never actually call anything on it in the tests.

Copy link
Member Author

Choose a reason for hiding this comment

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

This arm should do the same thing as the outer else.

@mih
Copy link
Member

mih commented Oct 4, 2020

@bpoldrack what is the state of this PR? The fix for #4893 is still urgent, but hasn't made it into the mainline yet, it seems. Can it be pulled from this PR?

@kyleam
Copy link
Contributor

kyleam commented Oct 5, 2020

The fix for #4893 is still urgent, but hasn't made it into the mainline yet, it seems

Didn't that come with gh-4899?

@bpoldrack
Copy link
Member Author

bpoldrack commented Oct 5, 2020

Kyle's right. The immediate fix is done and in maint, @mih.
Will push a fix here for the issue Kyle pointed out in his review. Other than that, this should suffice as a first step, I think, if there's no conceptual objection (which doesn't seem to be the case).

This is a first step to support bare repositories.
For now it just lets us instantiate a GitRepo instance and
introduces GitRepo.bare to make the object aware and possibly
react to it gracefully from within its methods.
Don't force ConfigManager to be instantiated from within
GitRepo.__init__ just to share a GitWitlessRunner. Instead do it the
other way around and grab GitRepo's from within ConfigManager when it's
instantiated anyway.

(Closes datalad#4912)
- test bare repo detection on a dir containing a config file
- explicitly test, that a regular repo is not a bare repo
- better be sure we don't mess with path to origin from within clone and
  therefore call GitRepo w/ create=False explicitly
@kyleam kyleam merged commit af85ad1 into datalad:master Oct 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants