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

git status-related fixes #23

Merged
merged 3 commits into from
Jun 5, 2024
Merged

Conversation

mark-dr
Copy link
Contributor

@mark-dr mark-dr commented Apr 5, 2023

Two fixes for changes in #12:

  1. Use path.join to construct a path (this was failing on POSIX systems)
  2. Avoid calling git status altogether if user has set ignoreGitStatus. I was getting failures in my monorepo project due to it interpreting my package dir as the Git dir. I'm not sure why it passes --git-dir to git status (could just set the exec's cwd to match dir?), but this change means that passing ignoreGitStatus allows users in situations like mine to bypass the issue entirely.

@mark-dr
Copy link
Contributor Author

mark-dr commented Apr 19, 2023

@danay1999, pinging you since this PR changes some of your work from #12. Could I get you to have a look, or to assign some reviewers? 🙂

Copy link

@akwodkiewicz akwodkiewicz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a maintainer, but looks good.

@bennycode
Copy link
Contributor

I can confirm that the path.join(dirPath, ".git") fix is very important. I am using Windows 11 in combination mit WSL2 / Ubuntu. When running the ts-fix without the fix from this PR, I am getting to see the following error:

Error: Command failed: git --git-dir="/home/bennycode/dev/my-project.git" --work-tree="/home/bennycode/dev/bennycode/trading212-api" status --porcelain

fatal: not a git repository: '/home/bennycode/dev/my-project\.git'

It's because of the mix of / and \ in the file path. Using path.join fixes this inconsistency.

@jakebailey and @andrewbranch: Can you have a look at this?

@jakebailey jakebailey merged commit 93e8fac into microsoft:main Jun 5, 2024
2 checks passed
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.

4 participants