Fix several issues related to worktree #1793

ermshiperete · 2019-08-27T15:22:48Z

This PR includes my previous PR (#1792) and generalizes the fix from chuseman (#1759).

Besides fixing the cache key creation when using worktrees it will also fix the location of the GitVersion cache so that the cache directory gets shared between all the worktrees.

asbjornu

Thanks for this! I think it mostly looks good.

asbjornu · 2019-09-04T10:16:03Z

src/GitVersionCore/GitPreparer.cs

@@ -150,6 +150,21 @@ public string GetDotGitDirectory()
            return dotGitDirectory;
        }

+        public string GetDotGitDirectoryOfMain()
+        {
+            var dotGitDir = GetDotGitDirectoryOfWorktreeOrMain();


It feels a bit weird to me that something called "get dot git directory of main" invokes a method called "get dot git directory of worktree or main". It seems like it should be the other way around?

We have two types of directories that can contain .git:

a) a regular git repo with a .git subdirectory. This might be a normal repo, or the main working tree if worktrees are used.
b) a linked working tree repo with a .git file. This file contains the path to the real .git directory which resides under .git/worktrees/<worktreename> of the main working tree.

In some cases it doesn't matter if we're dealing with case A or B, but in other cases we need to get the .git directory of the main working tree.

I tried to name the methods so that this distinction is clear, but maybe you can think of better names?

Well, the outer method uses the words OfMain which makes it sound very specific, but then delegates to a method using the words OrMain, indicating that it won't only fetch the .git directory of main after all. It would make sense for a method with the word Or (non-specific) in it to invoke a method with the word Of (specific) in it, but not the other way around. Does that make sense?

asbjornu · 2019-09-04T10:16:59Z

src/GitVersionCore/GitPreparer.cs

+
+        public string GetDotGitDirectory()
+        {
+            return GetDotGitDirectoryOfMain();


A method just invoking another method feels a bit unnecessary?

I agree. I tried to minimize the changes to existing code, but it would be clearer if we'd replace GetDotGitDirectory with GetDotGitDirectoryOfMain throughout the code base.

Yes, that would be good. Let's just agree on the naming first. Is Main an established word for what you describe in the codebase (or elsewhere) or are there other ways to name it? For me, Main is a bit ambiguous, as we already have Mainline as a versioning mode.

I'm not to fond of these names either. Maybe we can come up with something better.

Main working tree and linked working tree is what is used in the git documentation: "A repository has one main working tree (if it’s not a bare repository) and zero or more linked working trees." (https://git-scm.com/docs/git-worktree).

We could use GetDotGitDirectoryOfRepo (instead of GetDotGitDirectoryOfMain) and GetDotGitDirectoryOfWorktree (instead of GetDotGitDirectoryOfWorktreeOrMain), although the latter method name doesn't make it obvious that it returns .git directory of the main or linked working tree.

That would be an improvement, yes. 👍

src/GitVersionCore/GitVersionCacheKeyFactory.cs

asbjornu · 2019-09-04T10:23:33Z

With this merged, does it mean we can close #1792, #1759 and #1063? Please rebase this on HEAD of master and force-push.

This returns the correct workdir when using worktrees where the git repo resides inside of the main working tree. Partial fix for #1063.

ermshiperete

Yes, with this merged those issues can be closed.

src/GitVersionCore/GitVersionCacheKeyFactory.cs

ermshiperete · 2019-09-04T13:00:56Z

src/GitVersionCore/GitPreparer.cs

+
+        public string GetDotGitDirectory()
+        {
+            return GetDotGitDirectoryOfMain();


I agree. I tried to minimize the changes to existing code, but it would be clearer if we'd replace GetDotGitDirectory with GetDotGitDirectoryOfMain throughout the code base.

ermshiperete · 2019-09-04T13:00:59Z

src/GitVersionCore/GitPreparer.cs

@@ -150,6 +150,21 @@ public string GetDotGitDirectory()
            return dotGitDirectory;
        }

+        public string GetDotGitDirectoryOfMain()
+        {
+            var dotGitDir = GetDotGitDirectoryOfWorktreeOrMain();


We have two types of directories that can contain .git:

a) a regular git repo with a .git subdirectory. This might be a normal repo, or the main working tree if worktrees are used.
b) a linked working tree repo with a .git file. This file contains the path to the real .git directory which resides under .git/worktrees/<worktreename> of the main working tree.

In some cases it doesn't matter if we're dealing with case A or B, but in other cases we need to get the .git directory of the main working tree.

I tried to name the methods so that this distinction is clear, but maybe you can think of better names?

asbjornu · 2019-09-04T13:49:59Z

src/GitVersionCore/GitPreparer.cs

@@ -150,6 +150,21 @@ public string GetDotGitDirectory()
            return dotGitDirectory;
        }

+        public string GetDotGitDirectoryOfMain()
+        {
+            var dotGitDir = GetDotGitDirectoryOfWorktreeOrMain();


Well, the outer method uses the words OfMain which makes it sound very specific, but then delegates to a method using the words OrMain, indicating that it won't only fetch the .git directory of main after all. It would make sense for a method with the word Or (non-specific) in it to invoke a method with the word Of (specific) in it, but not the other way around. Does that make sense?

asbjornu · 2019-09-04T13:52:28Z

src/GitVersionCore/GitPreparer.cs

+
+        public string GetDotGitDirectory()
+        {
+            return GetDotGitDirectoryOfMain();


Yes, that would be good. Let's just agree on the naming first. Is Main an established word for what you describe in the codebase (or elsewhere) or are there other ways to name it? For me, Main is a bit ambiguous, as we already have Mainline as a versioning mode.

src/GitVersionCore/GitVersionCacheKeyFactory.cs

arturcic · 2019-09-05T10:30:16Z

Looks like there was an issue when running the windows azure pipeline build, but I ran it manually again and it succeeds.

asbjornu

I'm still not quite satisfied with the GetDotGitDirectoryOfWorktree and GetDotGitDirectoryOfRepo methods. Are both needed? Can't we just have a GetDotGitDirectory that does a combination of what the two methods are doing now? Since both methods return different things depending on the state of the repository, I don't think the attempt to clarify with OfRepo and OfWorktree is helpful, since it's misleading. I also don't see the value in having two methods doing almost the same thing. Thoughts?

ermshiperete · 2019-09-06T08:38:07Z

How about now?

arturcic · 2019-09-06T08:50:08Z

@ermshiperete there is a failing test on linux and macos, related to a "/" GetProjectRootDirectory_NoWorktree

When using worktrees we want to get the .git directory of the main repo (main working tree). This fixes calculating system hashs and the location of the GitVersion cache. When determining the root directory we have to use the .git directory of the linked working tree. Fixes #1063.

ermshiperete · 2019-09-06T09:29:07Z

@arturcic Can you trigger the build again please? Doesn't look like the test failures now are related after I fixed the real failure on linux and macos.

arturcic · 2019-09-06T09:32:24Z

It seems like the pipeline has some intermittent issues. I already triggered one and it succeeded

asbjornu

I think this looks good now. Any way to get the build green, or should we just merge regardless, @arturcic?

arturcic · 2019-09-06T20:55:06Z

I think we can merge it. There is a unit test that is randomly failing, but that is not an issue

asbjornu · 2019-09-06T23:31:48Z

Thank you so much @ermshiperete!

ermshiperete mentioned this pull request Aug 27, 2019

Doesn't work with git worktrees #1063

Closed

asbjornu requested changes Sep 4, 2019

View reviewed changes

asbjornu mentioned this pull request Sep 4, 2019

Fix git system hash for worktrees #1759

Closed

ermshiperete and others added 3 commits September 4, 2019 14:37

Properly get project root directory

ddb7c6e

This returns the correct workdir when using worktrees where the git repo resides inside of the main working tree. Partial fix for #1063.

Throw better exception when refs dir is missing

eb7dfed

Add cache key creation test for a worktree

4d30873

ermshiperete commented Sep 4, 2019

View reviewed changes

asbjornu requested changes Sep 4, 2019

View reviewed changes

asbjornu requested changes Sep 5, 2019

View reviewed changes

asbjornu approved these changes Sep 6, 2019

View reviewed changes

asbjornu merged commit ea2a10c into GitTools:master Sep 6, 2019

arturcic added the improvement label Sep 7, 2019

arturcic added this to the 5.1.0 milestone Sep 7, 2019

ermshiperete deleted the ImproveWorktree branch September 26, 2019 14:20

chuseman mentioned this pull request Oct 9, 2019

Fix HEAD for worktrees #1848

Closed

Fix several issues related to worktree #1793

Fix several issues related to worktree #1793

Uh oh!

Conversation

ermshiperete commented Aug 27, 2019

Uh oh!

asbjornu 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!

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!

Uh oh!

asbjornu commented Sep 4, 2019

Uh oh!

ermshiperete left a comment

Choose a reason for hiding this comment

Uh oh!

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!

Uh oh!

arturcic commented Sep 5, 2019

Uh oh!

asbjornu left a comment

Choose a reason for hiding this comment

Uh oh!

ermshiperete commented Sep 6, 2019

Uh oh!

arturcic commented Sep 6, 2019

Uh oh!

ermshiperete commented Sep 6, 2019

Uh oh!

arturcic commented Sep 6, 2019

Uh oh!

asbjornu left a comment

Choose a reason for hiding this comment

Uh oh!

arturcic commented Sep 6, 2019

Uh oh!

asbjornu commented Sep 6, 2019

Uh oh!

Uh oh!