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

Use .git directory for mutex name #2676

Merged
merged 2 commits into from
May 2, 2021
Merged

Conversation

bording
Copy link
Contributor

@bording bording commented May 1, 2021

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
Copy link
Contributor Author

bording commented May 1, 2021

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
Copy link
Member

arturcic commented May 1, 2021

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

@@ -57,7 +57,7 @@ public int Execute(GitVersionOptions gitVersionOptions)

private int RunGitVersionTool(GitVersionOptions gitVersionOptions)
{
var mutexName = gitVersionOptions.WorkingDirectory.Replace(Path.DirectorySeparatorChar.ToString(), "");
var mutexName = repositoryInfo.DotGitDirectory.Replace(Path.DirectorySeparatorChar.ToString(), "");
Copy link
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
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
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
Copy link
Contributor Author

bording commented May 1, 2021

@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
Copy link
Contributor Author

bording commented May 1, 2021

@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
Copy link
Member

asbjornu commented May 1, 2021

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
Copy link
Contributor

mergify bot commented May 2, 2021

Thank you @bording for your contribution!

@github-actions
Copy link

🎉 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