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

Adjust git.cwd to use a relative path to git root #587

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

Conversation

H3mul
Copy link

@H3mul H3mul commented Aug 31, 2023

This is a simple adjustment for the git cwd command to use a relative path to the git root instead of an absolute path. It should keep the behavior identical, but it solves an issue I ran into in my setup (described below).

More details on --show-cdup vs --show-toplevel here:
https://git-scm.com/docs/git-rev-parse#Documentation/git-rev-parse.txt---show-cdup

Motivation:
I ran into this issue during my esoteric obsidian-git setup using WSL2 (following the solution in #141). Basically, I want to use WSL's git instead of native windows git.exe, since I set it up with git-crypt and it has my ssh credentials. I ran into the issue that in this case, git.revparse("--show-toplevel") will return the root as an absolute WSL path (eg, /mnt/c/Users/...). git.cwd() checks path existence prior to actually switching the working directory, but does so using fs - which will be using the windows filesystem instead, and, of course, not find the root directory, and throw an error.

A quick solution is to pass a relative path to git.cwd() instead of an absolute path, as those will work the same way in WSL and Windows filesystems for a given repo. This is exactly what the --show-cdup flag returns. Note that it will be an empty string if we are already at the repo root, hence the additional existence check.

Some sources for my deep dive:

https://github.com/steveukx/git-js/blob/d64b31ca8670edd7af5a7fe5658516f5717c79a8/simple-git/src/lib/tasks/change-working-directory.ts#L7
https://github.com/steveukx/git-js/blob/d64b31ca8670edd7af5a7fe5658516f5717c79a8/simple-git/src/lib/utils/util.ts#L74
https://github.com/kwsites/file-exists/blob/13f0789cbd8df52588acdd185cd33de3b77fb342/src/index.ts#L10C13-L10C18

@Vinzent03
Copy link
Collaborator

Looks really promising! Thanks for that fix. Will test later or start next week. Please ignore all tests other than the format test. I've messed them up in my latest commits. But the format seems actually to be wrong.

@Vinzent03
Copy link
Collaborator

Have you tested this on normal windows without wsl? I'm getting the following error:

not a git repository (or any of the parent directores): .git

So I don't think the relative cwd is working properly.

@H3mul
Copy link
Author

H3mul commented Sep 15, 2023

Interesting, I havent, let me do some testing as well. I would expect that in this case, git.checkIsRepo() to also fail.

@H3mul
Copy link
Author

H3mul commented Sep 15, 2023

Looks like you were right, the relative path cwd seems to break. Adjusted this feature to resolve the relative path to an absolute one in the nodeJS filesystem, which works both in my WSL case and the windows git.exe case based on my testing

@H3mul
Copy link
Author

H3mul commented Mar 3, 2024

Apologies for the force push, rebased my fork's master to be up-to-date with this repo and forgot this issue tracks that branch. No new changes have been introduced since my last comment.

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

2 participants