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

Absolute paths fail on Windows due to backslash #4

Closed
avih opened this issue Sep 13, 2016 · 9 comments
Closed

Absolute paths fail on Windows due to backslash #4

avih opened this issue Sep 13, 2016 · 9 comments

Comments

@avih
Copy link

avih commented Sep 13, 2016

Currently the code at src/git.ts looks as follows, which seem to intend to normalize backslashes to slashes:

    static normalizePath(fileName: string, repoPath: string) {
        fileName = fileName.replace(/\\/g, '/');
        return isAbsolute(fileName) ? relative(repoPath, fileName) : fileName;
    }

However, on Windows path.relative appears to return paths with backslashes - which makes the git commands fail. The node docs seems to support it - https://nodejs.org/api/path.html#path_path_relative_from_to (note that at the example the absolute path is with backslashes, while in your code it's with slashes, however, it still seems to return backslashes).

I'd suggest to rewrite it like so:

    static normalizePath(fileName: string, repoPath: string) {
        if (isAbsolute(fileName))
            fileName = relative(repoPath, fileName);
        return fileName.replace(/\\/g, '/');
    }
@avih avih changed the title Relative paths fail on Windows due to backslash Absolute paths fail on Windows due to backslash Sep 13, 2016
@eamodio
Copy link
Member

eamodio commented Sep 14, 2016

@avih Thanks! Look for this in 0.0.6

@avih
Copy link
Author

avih commented Sep 14, 2016

Thanks, but I think that while 8721585 indeed fixed it, your followup commit on that line (d70a472) broke it again.

The additional condition fileName.startsWith(repoPath) is falsy (apparently repoPath is absolute with forward slashes), and so it ends with an absolute path with forward slashes, which git can't find at the repo since it should be relative to the repo.

@eamodio
Copy link
Member

eamodio commented Sep 14, 2016

@avih Doh -- thanks. If change this line to return gitCommand(cwd, 'rev-parse', '--show-toplevel').then(data => data.replace(/\r?\n|\r/g, '').replace(/\\/g, '/')); does that fix it for you?

@avih
Copy link
Author

avih commented Sep 14, 2016

It looks to me like it should fix it.

Unfortunately, and while it's probably simple, I haven't tried yet to edit the typescript sources (haven't used typescript before), and so my debugging and testing involves editing the generated javascript files, and luckily vscode doesn't recompile the typescript files on every run, so it's sustainable to a degree.

Bottom line, I can't test it yet, but the solution is probably simple: just make sure before and after that both strings have their slashes normalized.

eamodio pushed a commit that referenced this issue Sep 14, 2016
@avih
Copy link
Author

avih commented Sep 15, 2016

... so it ends with an absolute path with forward slashes, which git can't find at the repo since it should be relative to the repo

This still happened on 0.0.7 and still happens with 0.1.0, at least on some cases. Locally I modified normalizePath to also replace the slashes for pathRepo - and it seems to fix it.

I think the correct thing would be that normalizePath doesn't expect anything from its arguments, and instead does whatever it needs with the inputs such that the output is normalized. If this means normalizing both arguments before it does other things, then so be it.

I also think it would be a good idea if you could test on windows too. As it turns out, too many issues seem to slip between the cracks, I'm guessing, because you don't have a system to test it on.

eamodio pushed a commit that referenced this issue Sep 15, 2016
@eamodio
Copy link
Member

eamodio commented Sep 15, 2016

@avih Ugh sorry -- pushed 0.1.1. Yeah, I haven't been testing at all on Windows. I do have access but just haven't had the time to dedicate to getting that spun up and working for testing. :(

@avih
Copy link
Author

avih commented Sep 15, 2016

0.1.1 doesn't fix it either. Recall fileName has backslashes on entry, repoPath has forward slashes (at least with the cases I noticed - I wouldn't count on them to always be like this).

You need to normalize both before you do your thing, and then again at the end (which you do already).

And judging by the past, you really should test also on windows if you intend it for windows users too.

@avih
Copy link
Author

avih commented Sep 15, 2016

0.1.2 works as far as I can tell. Thanks :)

billsedison pushed a commit to noside911/vscode-gitlens that referenced this issue Jan 4, 2019
billsedison added a commit to noside911/vscode-gitlens that referenced this issue Jan 4, 2019
Fixes gitkraken#4  The Markdown script should be rendered on the VSCode side
@github-actions
Copy link

github-actions bot commented Dec 9, 2020

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 9, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants