-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Worktrees can be made from bare repositories #4630
Conversation
tests/worktree/worktree.c
Outdated
repo = cl_git_sandbox_init("testrepo.git"); | ||
|
||
cl_assert_equal_i(1, git_repository_is_bare(repo)); | ||
cl_assert_equal_i(0, git_repository_is_worktree(repo)); |
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.
I am aware of that. Right now I feel that we have bigger concerns than testing a single bool OR-ing function vs. all its others users that can now work against those bare-worktrees. Especially since the semantics are unclear.
tests/worktree/worktree.c
Outdated
cl_git_pass(git_repository_open(&wtrepo, path.ptr)); | ||
cl_assert_equal_i(1, git_repository_is_bare(wtrepo)); | ||
cl_assert_equal_i(1, git_repository_is_worktree(wtrepo)); | ||
|
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.
git_repository__ensure_not_bare
should be tested again on repo
to make sure that adding a worktree to a bare repository does not change the return value on the bare repository.
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.
Agreed
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.
Note that because of the semantic change in git_repository_is_bare
, git_repository__ensure_not_bare
now has the same semantics by default. So apart from the special error handling, it's equivalent (and I think it's better to test the public low-level API part vs. a slightly higher-up private one).
To clarify: you're saying that if I have a bare parent repository, the subsequent worktree repository also identifies itself as bare? I don't think that's what I would have expected... if you add a working directory to a bare repository, it reports itself (correctly) as non-bare. I would expect worktree repositories created from a bare parent to do the same. |
The unexpectedness is shared then. Note that I found one broken thing in |
tests/worktree/worktree.c
Outdated
repo = cl_git_sandbox_init("testrepo.git"); | ||
|
||
cl_assert_equal_i(1, git_repository_is_bare(repo)); | ||
cl_assert_equal_i(0, git_repository_is_worktree(repo)); |
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.
I am aware of that. Right now I feel that we have bigger concerns than testing a single bool OR-ing function vs. all its others users that can now work against those bare-worktrees. Especially since the semantics are unclear.
I think that |
src/worktree.c
Outdated
@@ -139,7 +139,7 @@ static int open_worktree_dir(git_worktree **out, const char *parent, const char | |||
if ((wt->name = git__strdup(name)) == NULL | |||
|| (wt->commondir_path = git_worktree__read_link(dir, "commondir")) == NULL | |||
|| (wt->gitlink_path = git_worktree__read_link(dir, "gitdir")) == NULL | |||
|| (wt->parent_path = git__strdup(parent)) == NULL) { | |||
|| (parent && (wt->parent_path = git__strdup(parent)) == NULL)) { |
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.
This also seems to fix #4625
c1ae122
to
f08f74d
Compare
Updated. A repo created from a worktree will not be reported as bare anymore, and the override is done in |
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.
Thanks for this PR. This was an obvious oversight by myself, a working tree shouldn't be considered as bare. It might be that the Windows failures stem from your use of joinpath with relative directories and Unix path separators, even though I think it unlikely. I can't dive into that right now, but could do so later.
if (git_config_get_bool(&is_bare, config, "core.bare") < 0) | ||
repo->is_bare = 0; | ||
if (err != GIT_ENOTFOUND) | ||
repo->is_bare = is_bare && !repo->is_worktree; |
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.
This definitely makes sense.
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.
(Note to myself) the commit message should be We were previously conflating any error into GIT_ENOTFOUND, which might or might not be correct. This fixes the code so a config error is bubbled up, as well as preserving the semantics in the face of worktree-repositories
.
tests/worktree/worktree.c
Outdated
cl_git_pass(git_repository_open(&wtrepo, path.ptr)); | ||
cl_assert_equal_i(1, git_repository_is_bare(wtrepo)); | ||
cl_assert_equal_i(1, git_repository_is_worktree(wtrepo)); | ||
|
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.
Agreed
tests/worktree/worktree.c
Outdated
cl_assert_equal_i(1, git_repository_is_bare(repo)); | ||
cl_assert_equal_i(0, git_repository_is_worktree(repo)); | ||
|
||
cl_git_pass(git_buf_joinpath(&path, repo->gitdir, "../worktree-frombare")); |
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.
You should be able to just use "./worktree-frombare" instead of assembling the path, as we always chdir into the test data directory.
tests/worktree/worktree.c
Outdated
git_repository *repo, *wtrepo; | ||
git_buf path = GIT_BUF_INIT; | ||
|
||
repo = cl_git_sandbox_init("testrepo.git"); |
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.
Use another bare repository here (e.g., short_tag.git
). This fixes the test issue on win32. I suppose the reason is that testrepo is already set up by setup_fixture_worktree
.
d854991
to
68c0e0b
Compare
We were previously conflating any error into GIT_ENOTFOUND, which might or might not be correct. This fixes the code so a config error is bubbled up, as well as preserving the semantics in the face of worktree-repositories
68c0e0b
to
44a505b
Compare
I've rebased this branch and just wanted to push it when I ran the tests for a second time, but this time they failed. The problem is that you're actually adding the worktree-frombare repo outside of our sandbox -- the path of I've fixed the test issue and conflict and pushed both to your branch. |
44a505b
to
a82082d
Compare
Thanks @tiennou! |
I've just hit that case locally, and was mightily confused 😉. So I looked this PR up and got even more confused by the backport-0.27.2 addition 👆(with the deletion only registering after I banged my |
I honestly can't remember anymore. I'd be happy to include it in another bugfix release, though, so I'm tagging it for backport |
Supersedes #4628, fixes #4626.
It fixes the issue by globally disabling the check in
ensure_not_bare
when working against a "worktree repo". I haven't given any thought to what this actually can provoke.My gut feeling says it's not clean enough, as I don't like :
repo->is_bare
tests which might now not work against a "worktree repo".git_repository_is_bare = true
) with a workdir, since the bare-ness of the parent repo percolates down to the worktree repoes.Maybe renaming
ensure_not_bare
toensure_has_workdir
and using that everywhere ? I'm unsure about the 2nd point though. Opinions, as always, welcome.