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

Incompatibile with Source Depot #47

Closed
hoovercj opened this issue Apr 7, 2017 · 9 comments
Closed

Incompatibile with Source Depot #47

hoovercj opened this issue Apr 7, 2017 · 9 comments

Comments

@hoovercj
Copy link
Contributor

hoovercj commented Apr 7, 2017

So far I have found two major incompatibilities wiith source depot (aka SD, the internal microsoft fork of perforce).

Problem: SD doesn't support login, it simply uses windows authentication, so the login checks always return false and nothing gets executed.
My workaround: I commented out the body of "isLoggedIn" and just return true instead.
Possible Solution: Provide a "skipLogin" config option. When that is true, "isLoggedIn" always returns true.

Problem: SD doesn't support ztags.
My workaround: I've considered rewriting some regexes as a proof of concept fix, but I haven't done it yet.
Possible Solution: Provide a "useZtags" config option that defaults to true and follows current code paths. When false, it can leave off the ztag parameter and I can write some extra parsing functions to get the output into the correct shape to be compatible with the rest of the program (if possible).

@hoovercj
Copy link
Contributor Author

hoovercj commented Apr 7, 2017

Note: Since this is primarily to help us internally, I'm more than happy to do this work and provide the PR's, I just want to make sure any work I do has minimal impact on the maintainability of the project and sucks up as little of your time as possible.

@stef-levesque
Copy link
Owner

stef-levesque commented Apr 7, 2017

Ok, how about a "SDMode" config option. It would:

  • overwrite the default executable path (unless already specified)
  • skip the "login" checks
  • disable incompatible commands in the UI (with a "setContext" and "when" conditions)
  • allow for a mechanism to provide specialized regexes when needed

As for -ztag, I used it only for scripting efficiency. "change" shouldn't need it, and "info" is only used for a few parameter (i.e. clientName). I don't mind deprecating its usage.

@hoovercj
Copy link
Contributor Author

hoovercj commented Apr 7, 2017

That sounds like a good approach.

As this doesn't affect your users, my suggestion is that you proceed as if this isn't an issue.

I can look at adding that SD compatibility flag and adding the checks that you suggested and doing the extra work to eliminate ztags.

If you like the PR then you can accept it.

Sound like a plan?

@stef-levesque
Copy link
Owner

Yes, it does 👍

I'm currently thinking of something like this:

package.json

  "contributes":{
    "configuration":{
      "type":"object",
      "title":"VS Code Perforce Configuration",
      "properties":{
        "perforce.compatibilityMode": {
          "type": "string",
          "default": "perforce",
          "description": "Specify if we should run in compatibility mode, currently support 'perforce' and 'sourcedepot'"
        }
      }
    },
    //...
    "menus": {
      "scm/title": [
        {
          "command": "perforce.login",
          "group": "2_login",
          "when": "scmProvider == perforce && compatibilityMode != sourcedepot"
        },
        {
          "command": "perforce.logout",
          "group": "2_login",
          "when": "scmProvider == perforce && compatibilityMode != sourcedepot"
        }
      ]
    }
  }

ScmProvider.ts

public Initialize() {
  //...
  const compatibilityMode = workspace.getConfiguration('perforce').get('compatibilityMode', 'perforce');
  commands.executeCommand('setContext', 'compatibilityMode', compatibilityMode);
}

@stef-levesque
Copy link
Owner

All should be fix version 2.0.1. Please let me know if anything else come up!

@vidia
Copy link

vidia commented Nov 23, 2017

Question for @hoovercj, how do you get this working for SD? I have set the configs that you mention here and in the PR, but I am not able to get things working. Any chance I could get some help debugging my config?

@galenelias
Copy link

I tried this with SD and a few commits/features which have gone in since @hoovercj's previous work have broken it again for SD. It's not too much work to get it up and running again for SD, but currently doesn't work.

@stef-levesque
Copy link
Owner

stef-levesque commented Nov 30, 2017

There is unfortunately no way for me to test "SD" functionalities, as this is proprietary.
But I will gladly accept PRs 🙂

@hoovercj
Copy link
Contributor Author

(Un)fortunately my team has switched from SD to git so I don't even know that I have access to an SD enlistment to try this on. I'm happy to review any PRs or give suggestions, but I can't fix and verify this myself.

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

No branches or pull requests

4 participants