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

Can't use wslgit.exe with SmartGit 18 #37

Closed
rfgamaral opened this issue Jul 18, 2018 · 29 comments · Fixed by #90
Closed

Can't use wslgit.exe with SmartGit 18 #37

rfgamaral opened this issue Jul 18, 2018 · 29 comments · Fixed by #90

Comments

@rfgamaral
Copy link

I'm using SmartGit on Windows 10 and I can't get wslgit.exe working with this application. I get this error:

image

Any idea why this is happening? It works on the command line:

image

Am I doing something wrong?

@andy-5
Copy link
Owner

andy-5 commented Jul 18, 2018

It looks like the backslashes are removed/escaped somewhere in the process of running the command in WSL.

Can you try to set the (Windows) environment variable WSLGIT_USE_INTERACTIVE_SHELL to false and see if that makes any difference? See Advanced Usage in the readme for more details on the flag.

Thinking about it, the reason might be that relative paths with backslashes are passed. Currently, only absolute paths in the arguments are translated from the Windows representation to their Unix representation inside WSL. This is enough for VS Code or the command line, where either absolute paths are used, or you can specify relative paths with forward slashes. I'm not sure if you could configure SmartGit to, e.g., pass absolute paths.

@rfgamaral
Copy link
Author

Tried the env var, didn't change anything. Also tried to find away to use absolute paths in SmartGit, can't find a way to configure that. Will reach to SmartGit support and see what can be done about that.

@andy-5
Copy link
Owner

andy-5 commented Jul 26, 2018

Ok. The other solution, of course, is adding support for relative paths to wslgit. It came up a few times already, but I haven't found the time/need to implement it properly.

To support relative paths, we would need to check every argument (with a backslash in it) if it is actually a file relative to the current working directory. However, this doesn't work with non-existing files, e.g. git rm a file that was already deleted from the file system. On the other hand, not checking the argument at all and just changing backslashes to forward slashes might accidentally modify other arguments like commit messages, etc. So there is no ideal solution.

@rfgamaral
Copy link
Author

@andy-5 I reached to SmartGit about this and it went like this:

Me: Is it possible to configure SmartGit to use absolute paths to files when invoking Git commands? This might help fix this issue: #37

Support: No, this is not possible. Git does not need this and it would be redundant information.

Me: As you can see from the issue I linked, it would not be redundant information... Could you consider adding an option to always use absolute paths instead of relative? It shouldn't be that time consuming to implement such feature.

Support: I still think, this is a problem of wslgit. If it should be a Git replacement, then it should accept the parameters like Git. The absolute path is redundant, because the Git executable is started in the repository's root directory.

Not sure what could be the best course of action here...

Of course none of this would be needed if SmartGit added proper WSL support like VS Code does in some places. Until then, I have to resort to solutions like wslgit, unfortunately it's not working for me with SmartGit.

It does work nicely on the command line. I even configured GPG signing inside WSL and wslgit worked just fine. Kudos, you did an awesome job :)

@andy-5
Copy link
Owner

andy-5 commented Aug 1, 2018

Well, too bad, but I get their point.

As I said, support for relative paths could be added, but it wouldn't work with non-existing files. I'll leave this issue open for now, maybe I, or someone else, finds some time to work on it.

@andy-5
Copy link
Owner

andy-5 commented Aug 10, 2018

I've just added support for relative paths to wslgit, with the restriction mentioned above that they are only recognized if the referenced file exists on the filesystem.

A test build with this feature is available here. Can you please test with this binary and report back if the integration with SmartGit now works?

@rfgamaral
Copy link
Author

@andy-5 I've been on vacation, give me a couple of days to test this and I'll report back as soon as I can. Thanks.

@rfgamaral
Copy link
Author

@andy-5 It almost worked, but now I don't think you can do anything about it...

image

SmartGit is using temporary files created on the host system and path translation between Windows/WSL is not working properly. But now the ball is on their court...

P.S: The test build is again flagged "unsafe" as before.

Feel free to close this issue if there's nothing else for us to discuss. I'll email SmartGit about this. Thanks for your support.

@andy-5
Copy link
Owner

andy-5 commented Aug 16, 2018

Interesting, I didn't know about the --file flag for commit. VS Code passes the commit message via stdin. But it should be possible to fix path translation to work also for this case. I'll try when I have some spare time.

@andy-5
Copy link
Owner

andy-5 commented Aug 21, 2018

There is a new test build available which now also translates paths inside arguments like --file=C:\some\path\message.txt.

This should hopefully fix the issue above and allow you to commit using SmartGit.

Regarding wslgit being flagged as "unsafe", I've found their support to be very responsive. Just submit the executable as a false positive on their site and link to this repository/issue and maybe also to the test build itself.

@SBRK
Copy link

SBRK commented Aug 27, 2018

@andy-5 @rfgamaral the commit works with the test build. But now the push does not. Here is the command invoked for push:

C:\bin\wslgit.exe -c 'credential.helper=C:/Program\ Files\ \(x86\)/SmartGit/lib/credentials.cmd' push --porcelain --progress --recurse-submodules=check origin refs/heads/test:refs/heads/test

The command hangs indefinitely

EDIT: after setting [credential] in .gitconfig, it doesn't pass the credential.helper part anymore but it still doesn't work

@andy-5
Copy link
Owner

andy-5 commented Aug 28, 2018

Thanks for the update. At least some progress, but the cmd script credential helper will not work inside WSL and I'm not sure I can do anything about it.

Setting [credential] in .gitconfig sounds interesting. Which settings did you specify there?

Does a manual push using wslgit work? You could test the exact command from SmartGit (minus the credential.helper part) or just something like

C:\bin\wslgit.exe push

If both of these do not work, or require user interaction to enter username/password, you probably need to configure credentials for git inside WSL, independent of SmartGit. For example by setting up a SSH key which is allowed to push to your repository.

@rfgamaral
Copy link
Author

Hey guys, latest version v0.8.0 seems to have fixed all the problems for me, commit, push and pull seem to work fine now. I do not have [credential] section on my .gitconfig.

As soon as possible I'm going to test running npm scripts invoked by Git hooks and report back. If those work (I hope so and I believe they will), we can close this issue; unless someone else still has a problem.

@rfgamaral
Copy link
Author

The Git hooks seem to be working for me. Although I still have a tiny issue with them, which I'll be able to fix with v0.9.0 (due to WSLGIT).

It didn't fix all the problems with Smartagit though... Trying to stage deleted files doesn't work. However, I'm also using wslgit.exe v0.8.0 in VS Code and staging files work fine there. What could possibly be different from both of these applications and when both are using the same wslgit.exe?

@rfgamaral
Copy link
Author

Tried the v0.9.0-alpha10 and wslgit stopped working for me:

image

@andy-5
Copy link
Owner

andy-5 commented Oct 27, 2019

It looks like the space in the credential.helper path is not properly escaped. I guess this is due to the switch to wslpath for translating windows paths to linux. I'll look into it.

@carlolars
Copy link
Contributor

The space should be ok, but the regexes is not considering double-quote character as invalid in a windows path, which it should. Unix filenames can have " but windows can not :) I'll push a PR.

@andy-5
Copy link
Owner

andy-5 commented Oct 28, 2019

Great, thanks @carlolars. I've merged your PR, and there is now a new alpha build for testing.

@rfgamaral The PR automatically closed this issue. If there is still a problem, feel free to reopen it.

@carlolars
Copy link
Contributor

Oops I wrote the magic word fixes in the PR... Didn't mean to close this issue.

@rfgamaral
Copy link
Author

rfgamaral commented Oct 28, 2019

Great, thanks @carlolars. I've merged your PR, and there is now a new alpha build for testing.

@rfgamaral The PR automatically closed this issue. If there is still a problem, feel free to reopen it.

Sorry guys, this doesn't seem to have fixed the issue:

image

Can you please reopen it? I don't have permissions for doing it...

@carlolars
Copy link
Contributor

But we're getting closer :)
I see what is causing that error, the argument to wslpath is quoted so \ is literary interpreted as backslash and space and \( as backslash and parenthes, not as an escaped space or escaped parenthes.

$ wslpath 'C:/Program\ Files\ \(x86\)/gnupg/bin/gpg.exe'
/c/Program/ Files/ /(x86/)/gnupg/bin/gpg.exe
# Not quoting the argument works correctly:
$ wslpath C:/Program\ Files\ \(x86\)/gnupg/bin/gpg.exe
/c/Program Files (x86)/gnupg/bin/gpg.exe

The quoting was deliberate, but maybe it is not required? (The usage example for wslpath shows quoted argument)
My worries was that if a path contained a space it would only translate up to the space, but maybe all paths that contain spaces will have the space correctly escaped?

.replace_all(argument.as_bytes(), &b"${pre}$(wslpath '${path}')"[..])

The quoting is also present when transating unix-windows:

&b"${pre}$(WINPATH=$(wslpath -w '${path}'); echo -n ${WINPATH//\\\\/\\\\\\\\})"[..],

@carlolars
Copy link
Contributor

The quotes for the wslpath argument should be there, but there is an issue with them described in #91. But that issue won't solve this issue entirely.

The problem is the argument
"credential.helper=C:/Program\ Files\ \(x86\)/SmartGit/lib/credentials.cmd"
The space characters and the parentheses are escaped unix-style, but the cmd.exe shell does not need escaping of space and parentheses. The argument must instead be within quotes, which it is, to keep a string with a space as one argument.
So cmd.exe escapes each backslash with a new backslash, making the backslashes actual backslashes, which are considered as path separators by wslpath.

@rfgamaral
Copy link
Author

I just read about credential.helper and realized this is only used to authenticate with https connections for Git repositories and I personally only use ssh connections. So, I've added the following to my ~/.gitconfig:

[credential]
    helper = cache

SmartGit no longer adds credential.helper to Git commands and the above is no longer a problem. Of course, this is a workaround...

@carlolars
Copy link
Contributor

So cmd.exe escapes each backslash with a new backslash, making the backslashes actual backslashes, which are considered as path separators by wslpath.

This was not quite correct, cmd.exe escapes backslashes but for wslpath they are just several consecutive backslashes which wslpath merges into just one.
And SmartGit likely does not use cmd.exe but invokes the git process directly.

But the main problem is still the same; the backslash when escaping the characters are used as actual backslashes. Is it working if using Git-For-Windows?
I would say that the argument string is incorrectly formatted, the characters should not be escaped in that string.

@rfgamaral How did you make SmartGit use the credential.helper? I downloaded the lastest portable version of SmartGit and it does not add the credential.helper and I don't have it configured elsewhere.

@rfgamaral
Copy link
Author

@rfgamaral How did you make SmartGit use the credential.helper? I downloaded the lastest portable version of SmartGit and it does not add the credential.helper and I don't have it configured elsewhere.

I didn't do anything, it was there by default for me... And I can't find a setting (inside SmartGit) anywhere to disable it. Not sure where it's coming from to be honest. All I found was this: https://www.syntevo.com/doc/display/SG/Using+the+Git+credentials+manager

But helper = manager won't work with wslgit.exe, there's no such thing as manager inside WSL. That's why I used cache. Which will ask for you credentials (for https authentication) every time and store them in memory for a period of time.

@rfgamaral
Copy link
Author

Ignoring the credential manager issue for now, I believe all my SmartGit issues have been fixed with 0.9.0-alpha11. Here's my full configuration:

Environment variables:

WSLGIT_MOUNT_ROOT=/
WSLGIT_USE_INTERACTIVE_SHELL=true

~/.bashrc (inside WSL, only file sourced):

if [ "${WSLGIT}" -eq "1" ]; then
    if [ -d "$HOME/.node/bin" ]; then
        export PATH="$HOME/.node/bin:$PATH"
    fi

    return;
fi

SmartGit Hooks Example:

image

Good job @andy-5 and @carlolars, thank you so much 🙇

@rfgamaral
Copy link
Author

There's still one small issue with the latest wslgit version (0.9.0) and SmartGit. Basically, when trying to stage a deleted file I get the following error:

image

However, using the same wslgit.exe file, VS Code can successfully stage the deleted file.

Do you think this can be fixed somehow?

P.S: I'm already used to build wslgit from source so let me know if you want me to quickly test some change to attempt fixing the compatibility with SmartGit once and for all :)

@carlolars
Copy link
Contributor

What a coincidence, yesterday I was thinking that it might be a good idea to at least document in an issue the problem with relative paths to files that don't exist on the filesystem. See #96 😄

The problems are that wslgit treats each argument separately with no knowledge about which git command it belongs to and therefore don't know what to expect, and it's approach to git arguments is "if it looks like a path then it is a path".
Absolute paths are easiest as they always starts with C:\ or some other letter, relative paths to existing files are also easy since it is possible to check if the file exists.
But relative paths to deleted files are worse since it could also be a RegEx, and changing the slashes would break it. VSCode is using absolute paths to files so that's why it works.

@rfgamaral
Copy link
Author

rfgamaral commented Jan 13, 2020

Hum, interesting, will follow that issue :)

I'm wondering if there's any change with SmartGit 19, I haven't upgrade myself yet...

EDIT: Just download SmartGit 19 portable version for testing and the problem wasn't solved, in other words, SmartGit 19 still uses relative paths. Maybe I could ask SmartGit devs to change relative paths to absolute (unless they have some limitation on their side for doing it relative).

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

Successfully merging a pull request may close this issue.

4 participants