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

Make sure to re-initialise git repo #7095

Merged
merged 3 commits into from
Oct 18, 2022
Merged

Make sure to re-initialise git repo #7095

merged 3 commits into from
Oct 18, 2022

Conversation

mslw
Copy link
Contributor

@mslw mslw commented Oct 17, 2022

The create_sibling_ria command used GitRepo(repo_path, create=True) constructor to initialise bare repositories locally, also when existing=reconfigure was requested. But for existing repositories, GitRepo does not execute git init, it just returns a GitRepo instance, so reconfigure was not actually handled. This commit introduces an aditional check, to make sure that git init is called when reconfiguring.

Partially addresses datalad/datalad-ria#43 (--existing reconfigure --shared not working). However, another problem remains: although we now do handle the reconfiguration, calling git annex init --bare --shared sets the config variable core.sharedRepository (which would presumably affect future commits), but does not change the permissions for the already existing files in the bare git repo.

The `create_sibling_ria` command used `GitRepo(repo_path,
create=True)` constructor to initialise bare repositories locally,
also when `existing=reconfigure` was requested. But if the repository
already exists, `GitRepo` does not execute `git init`, it just returns
a `GitRepo` instance, so `reconfigure` was not actually handled. This
commit introduces an aditional check, to make sure that `reconfigure`
calls `git init`.

Partially addresses #6967 (`--existing reconfigure --shared` not
working). However, another problem remains: although we now do handle
the reconfiguration, calling `git annex init --bare --shared` sets the
config variable `core.sharedRepository` (which would presumably affect
future commits), but does not change the permissions for the already
existing files in the repo.

Co-authored-by: Benjamin Poldrack <benjaminpoldrack@gmail.com>
@mslw mslw added the semver-minor Increment the minor version when merged label Oct 17, 2022
@mslw mslw marked this pull request as ready for review October 17, 2022 16:18
Copy link
Member

@yarikoptic yarikoptic left a comment

Choose a reason for hiding this comment

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

don't know much about ria so can't judge if git init is safe to do upon reconfigure -- left just a single recommendation

datalad/distributed/create_sibling_ria.py Outdated Show resolved Hide resolved
datalad/distributed/create_sibling_ria.py Outdated Show resolved Hide resolved
Co-authored-by: Yaroslav Halchenko <debian@onerussian.com>
@bpoldrack
Copy link
Member

don't know much about ria so can't judge if git init is safe to do

FTR: "safe" doesn't even matter. Re-init is the explicitly described behavior of existing=reconfigure and implemented in the SSH case, it just wasn't done in the local case.

@bpoldrack bpoldrack added semver-patch Increment the patch version when merged and removed semver-minor Increment the minor version when merged labels Oct 18, 2022
@codecov
Copy link

codecov bot commented Oct 18, 2022

Codecov Report

Base: 76.01% // Head: 75.62% // Decreases project coverage by -0.39% ⚠️

Coverage data is based on head (259b8c0) compared to base (dccc798).
Patch coverage: 72.22% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##            maint    datalad/datalad#7095      +/-   ##
==========================================
- Coverage   76.01%   75.62%   -0.40%     
==========================================
  Files         355      355              
  Lines       59101    59320     +219     
  Branches     6318     6322       +4     
==========================================
- Hits        44925    44859      -66     
- Misses      14162    14446     +284     
- Partials       14       15       +1     
Impacted Files Coverage Δ
datalad/core/distributed/tests/test_clone.py 67.38% <63.63%> (-8.93%) ⬇️
datalad/tests/utils_pytest.py 89.91% <69.23%> (-0.22%) ⬇️
datalad/distributed/create_sibling_ria.py 54.00% <75.00%> (+0.91%) ⬆️
datalad/utils.py 73.85% <100.00%> (-2.16%) ⬇️
datalad/cmdline/__init__.py 0.00% <0.00%> (-100.00%) ⬇️
datalad/cmdline/helpers.py 0.00% <0.00%> (-78.27%) ⬇️
datalad/__main__.py 39.65% <0.00%> (-29.32%) ⬇️
datalad/distribution/create_sibling.py 56.62% <0.00%> (-3.86%) ⬇️
datalad/interface/base.py 92.30% <0.00%> (-3.08%) ⬇️
... and 10 more

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.

@bpoldrack
Copy link
Member

Failure in datalad-deprecated happens on maint already - not related to this PR.

Merging.

@bpoldrack bpoldrack merged commit 54e21e3 into datalad:maint Oct 18, 2022
@yarikoptic-gitmate
Copy link
Collaborator

PR released in 0.17.8

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.

4 participants