Skip to content

Use .git directory for mutex name#2676

Merged
arturcic merged 2 commits into
GitTools:mainfrom
bording:fix-mutex-name
May 2, 2021
Merged

Use .git directory for mutex name#2676
arturcic merged 2 commits into
GitTools:mainfrom
bording:fix-mutex-name

Conversation

@bording

@bording bording commented May 1, 2021

Copy link
Copy Markdown
Contributor

Follow-up PR to #2669 to fix the mutex name. Based on the command line documentation and my initial investigation of the code, I thought WorkingDirectory would be the actual git working directory, but that turns out to not be true.

Instead, I've switched to using IGitRepositoryInfo.DotGitDirectory, which should always be pointing at the same location for all projects in a solution.

If there are projects that have different DotGitDirectory values in a solution, then that would mean they are operating on completely different repos, but that likely means you have something misconfigured in your solution anyway.

Related Issue

#2669

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@bording

bording commented May 1, 2021

Copy link
Copy Markdown
Contributor Author

I see one test has failed on ubuntu, but there's no real information to go on to understand why it might have failed. That same test seems to have passed on Windows and mac. Any chance that test is flaky and just needs another run? I'm thinking it might be since it looks like it did pass on the Azure run.

@arturcic arturcic added the bug label May 1, 2021
@arturcic

arturcic commented May 1, 2021

Copy link
Copy Markdown
Member

@bording do you think this one is related to the previous fix #2675

private int RunGitVersionTool(GitVersionOptions gitVersionOptions)
{
var mutexName = gitVersionOptions.WorkingDirectory.Replace(Path.DirectorySeparatorChar.ToString(), "");
var mutexName = repositoryInfo.DotGitDirectory.Replace(Path.DirectorySeparatorChar.ToString(), "");

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Perhaps prefixing gitversion to the mutex to avoid anyone else using the same mutex?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It seems a bit unlikely that another mutex with the exact name of the path of the .git folder would exist, but it probably wouldn't hurt to be a bit more defensive.

Looking at the mutex documentation, there is some vague stuff about potential maximum lengths of mutex names, but there are no actual values given, so I guess we'll have to see if there are any bug reports about names being too long and determine what do to if that happens.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The maximum length of mutex names is MAX_PATH, which is defined as 260 characters. So you should be fine here. (Source: CreateMutexW documentation)
Please note that mutex names are case sensitive. Should be ok as long as repositoryInfo.DotGitDirectoryrepositoryInfo.DotGitDirectory has always the same casing.

@bording

bording commented May 1, 2021

Copy link
Copy Markdown
Contributor Author

@bording do you think this one is related to the previous fix #2675

Yes, I suspect it's likely if the path value being passed in from the command line would result in an empty value.

@bording

bording commented May 1, 2021

Copy link
Copy Markdown
Contributor Author

@arturcic Looks like that test failed again, this time on Windows. Is this a known flaky test, or is this something I need to be trying to figure out if the PR is causing it?

@asbjornu

asbjornu commented May 1, 2021

Copy link
Copy Markdown
Member

Looks like that test failed again, this time on Windows. Is this a known flaky test, or is this something I need to be trying to figure out if the PR is causing it?

That error seems to have been with us for some time, IIRC. I think it's a bug in Cake or at least in how we are using Cake, somehow. See cake-build/cake#3164 for details.

@arturcic arturcic merged commit 84be4d8 into GitTools:main May 2, 2021
@mergify

mergify Bot commented May 2, 2021

Copy link
Copy Markdown
Contributor

Thank you @bording for your contribution!

@github-actions

Copy link
Copy Markdown

🎉 This issue has been resolved in version 5.6.10 🎉
The release is available on:

Your GitReleaseManager bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants