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

Try to resolve the issue with JetBrains computer names in TFS workspaces #212

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

AlexRukhlin
Copy link
Contributor

No description provided.

try {
workspaceInfo = determineCachedWorkspaceImpl(pathFreeArguments, ignoreWorkspaceOptionValue);

if (LocalHost.getShortName().equalsIgnoreCase(workspaceInfo.getComputer())) {
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand the need for the comparison here. If the workspace was found, shouldn't we just return it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IMO, we do not check the computer when trying to find a workspace in the cache. If we've found a workspace with an unexpected computer name, I would change the name (by using the system property) and continue with that. I hope that would protect us from unexpected cache refresh, which might remove the workspace.

jbComputerName));

System.setProperty(LocalHost.SHORT_NAME_OVERRIDE_PROPERTY, jbComputerName);
return determineCachedWorkspaceImpl(pathFreeArguments, ignoreWorkspaceOptionValue);
Copy link
Contributor

Choose a reason for hiding this comment

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

If this final attempt using the JetBrains computer name is unsuccessful, should we set the system property back to its original value?


final String jbComputerName = LocalHost.getShortNameJB();
log.info(MessageFormat.format(
"We'll try to find the workspace one more time using the JetBrains name {0}", //$NON-NLS-1$
Copy link
Contributor

Choose a reason for hiding this comment

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

P999: "JetBrains name" would be clearer as: "JetBrains computer name"

Copy link
Member

@jpricket jpricket left a comment

Choose a reason for hiding this comment

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

The changes seem good to me. I had one question, but I trust that the code is correct.
Leah should probably also review them.

@@ -108,10 +110,77 @@ public synchronized static String getShortName() {
name.substring(0, (name.length() > MAX_COMPUTER_NAME_SIZE) ? MAX_COMPUTER_NAME_SIZE : name.length());
}

log.info("Short name resolved to: " + computerNameJB); //$NON-NLS-1$
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be computerName not computerNameJB?

* @return a short identifier (name of this computer). Never
* <code>null</code> or empty.
*/
public synchronized static String getShortNameJB() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

@leantk leantk left a comment

Choose a reason for hiding this comment

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

I verified this worked on my Mac

@leantk
Copy link
Contributor

leantk commented Jun 14, 2017

I' seeing an issue with the workspaces command though. Investigating now...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants