Worktrees can be made from bare repositories by tiennou · Pull Request #4630 · libgit2/libgit2

tiennou · 2018-04-18T22:54:01Z

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 :

all those remaining repo->is_bare tests which might now not work against a "worktree repo".
you can have bare 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 to ensure_has_workdir and using that everywhere ? I'm unsure about the 2nd point though. Opinions, as always, welcome.

tiennou · 2018-04-19T08:19:34Z

tests/worktree/worktree.c

+	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));


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.

csware · 2018-04-19T08:06:16Z

tests/worktree/worktree.c

+	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));
+


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.

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).

ethomson · 2018-04-19T08:09:29Z

you can have bare repo (git_repository_is_bare = true) with a workdir, since the bare-ness of the parent repo percolates down to the worktree repoes.

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.

tiennou · 2018-04-19T08:16:05Z

The unexpectedness is shared then. Note that cd $worktree; git config --get core.bare returns "true", so git seems to consider the repository still bare. I expect our config code to do the same, but that is_bare function clearly does not.

I found one broken thing in git, adding a relative submodule to the worktree of a bare repository always "can't find directory $relative_path".

tiennou · 2018-04-19T08:19:34Z

tests/worktree/worktree.c

+	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));


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.

ethomson · 2018-04-19T08:28:36Z

The unexpectedness is shared then. Note that cd $worktree; git config --get core.bare returns "true", so git seems to consider the repository still bare. I expect our config code to do the same, but that is_bare function clearly does not.

git config --get core.bare is really only showing you the configuration option, which remains unchanged. If you query the repository configuration through libgit2, we should also report core.bare = true even when you've configured a working folder.

I think that git_repository_is_bare should report the conceptual / perceived bareness of a repository, not just be a direct proxy to the configuration. I don't think there's a parallel in git.git for this concept.

csware · 2018-04-19T10:45:03Z

src/worktree.c

 	    || (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)) {


This also seems to fix #4625

tiennou · 2018-04-20T08:44:17Z

Updated. A repo created from a worktree will not be reported as bare anymore, and the override is done in load_config_data to repo->bare directly, so all call-sites will see it. I'm still trying to pinpoint what I'm breaking by allowing those worktrees to not have a parent, so it's still WIP.

pks-t

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.

pks-t · 2018-04-20T09:40:39Z

src/repository.c

-	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;


This definitely makes sense.

(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.

pks-t · 2018-04-20T09:43:58Z

tests/worktree/worktree.c

+	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));
+


pks-t · 2018-04-20T09:51:08Z

tests/worktree/worktree.c

+	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"));


You should be able to just use "./worktree-frombare" instead of assembling the path, as we always chdir into the test data directory.

csware · 2018-04-20T11:18:44Z

tests/worktree/worktree.c

+	git_repository *repo, *wtrepo;
+	git_buf path = GIT_BUF_INIT;
+
+	repo = cl_git_sandbox_init("testrepo.git");


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.

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

pks-t · 2018-05-07T12:06:32Z

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 git_worktree_add is relative to cwd, not relative to the repository we're creating the worktree from.

I've fixed the test issue and conflict and pushed both to your branch.

pks-t · 2018-05-07T13:36:58Z

Thanks @tiennou!

tiennou · 2018-08-10T20:16:56Z

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
head around longer). Was that the de-backporting intentional ? It's just that OCGit is currently tracking 0.27 and this one's now MIA 😕.

pks-t · 2018-08-16T07:33:47Z

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

csware suggested changes Apr 19, 2018

View reviewed changes

tiennou commented Apr 19, 2018

View reviewed changes

csware reviewed Apr 19, 2018

View reviewed changes

tiennou force-pushed the fix/worktree-from-bare branch 2 times, most recently from c1ae122 to f08f74d Compare April 20, 2018 08:39

pks-t requested changes Apr 20, 2018

View reviewed changes

pks-t added backport-0.27.1 and removed backport-0.27.1 labels Apr 20, 2018

csware reviewed Apr 20, 2018

View reviewed changes

tiennou force-pushed the fix/worktree-from-bare branch 2 times, most recently from d854991 to 68c0e0b Compare April 20, 2018 17:52

tiennou mentioned this pull request Apr 24, 2018

worktree: Add new git_worktree_name and git_worktree_path functions #4639

Closed

pks-t approved these changes May 7, 2018

View reviewed changes

repository: being a worktree means we're not really bare

c7964c2

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

pks-t force-pushed the fix/worktree-from-bare branch from 68c0e0b to 44a505b Compare May 7, 2018 12:06

worktree: a worktree can be made from a bare repository

a82082d

pks-t force-pushed the fix/worktree-from-bare branch from 44a505b to a82082d Compare May 7, 2018 12:28

pks-t merged commit 81ea995 into libgit2:master May 7, 2018

tiennou deleted the fix/worktree-from-bare branch June 4, 2018 22:46

This was referenced Jun 18, 2018

Fix git_worktree_validate failing on bare repositories #4686

Merged

git_repository_path is incorrect for worktree from bare repo #4684

Closed

pks-t added the backport-0.27.5 label Aug 16, 2018

tiennou mentioned this pull request Oct 1, 2018

segfautlt in git_worktree_lookup on bare repos #4822

Closed

This was referenced Jan 29, 2019

feature request git_worktree_add commitish reference #4823

Closed

git_worktree_lookup fails with "out of memory" on a bare repository #4625

Closed

snyk-bot mentioned this pull request Feb 23, 2020

[Snyk] Upgrade nodegit from 0.4.1 to 0.26.4 saurabharch/Breezeblocks#1

Open

snyk-bot mentioned this pull request Apr 22, 2020

[Snyk] Upgrade nodegit from 0.24.3 to 0.26.5 aminatakonate000/Graviton-App#4

Open

snyk-bot mentioned this pull request May 5, 2020

[Snyk] Upgrade nodegit from 0.24.3 to 0.26.5 Barnstorm-Online/ngp-openapi-generator#1

Open

Conversation

tiennou commented Apr 18, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ethomson commented Apr 19, 2018

Uh oh!

tiennou commented Apr 19, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ethomson commented Apr 19, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tiennou commented Apr 20, 2018

Uh oh!

pks-t left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

csware Apr 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pks-t commented May 7, 2018

Uh oh!

pks-t commented May 7, 2018

Uh oh!

tiennou commented Aug 10, 2018

Uh oh!

pks-t commented Aug 16, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

csware Apr 20, 2018 •

edited

Loading