-
Notifications
You must be signed in to change notification settings - Fork 96
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
base: master
Are you sure you want to change the base?
Conversation
try { | ||
workspaceInfo = determineCachedWorkspaceImpl(pathFreeArguments, ignoreWorkspaceOptionValue); | ||
|
||
if (LocalHost.getShortName().equalsIgnoreCase(workspaceInfo.getComputer())) { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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$ |
There was a problem hiding this comment.
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"
There was a problem hiding this 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$ |
There was a problem hiding this comment.
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() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use the same way to figure out the short name like JetBrains
https://github.com/JetBrains/tfsIntegration/blob/0f127c1b10acd3f985920246f43dfbe170d69960/src/org/jetbrains/tfsIntegration/core/tfs/Workstation.java#L234
There was a problem hiding this 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
I' seeing an issue with the workspaces command though. Investigating now... |
No description provided.