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

fix: fix issue with retrieving repository URL #291

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

marshallku
Copy link

@marshallku marshallku commented Jun 26, 2024

- gconf = gconf.split(/\r?\n/)
+ const gconf = await fs.readFile('.git/config', 'utf8').catch(() => '')
+ const lines = gconf.split(/\r?\n/)

In #186, there was breaking change in retrieving repository URL from .git/config.

Although the split lines were assigned to lines, the code still accessed gconf to get the file lines, causing the repository field to never auto-complete.

References

@marshallku marshallku requested a review from a team as a code owner June 26, 2024 01:23
@wraithgar
Copy link
Member

We will definitely want a regression test on this one.

@marshallku
Copy link
Author

Hi, @wraithgar
The logic is almost same as previous version(before breaking changes), and I just tested locally with my git config file.
Should I write test code for this case?

@wraithgar
Copy link
Member

@marshallku yes. The fact that tests didn't fail w/ this bug means the tests are lacking here. We need some test that would fail w/ the old code but succeed w/ the new code.

@wraithgar
Copy link
Member

It may be as simple as an assertion in an existing test that wasn't looking at the affected attributes. I haven't dug in to be sure.

@marshallku
Copy link
Author

@wraithgar I add test case for retriving URL of remote repository.
I used git config --get remote.origin.url in runtime to get the expected output.
Let me know if it would be better to hard-code https://github.com/npm/init-package-json.git.

@wraithgar
Copy link
Member

wraithgar commented Jun 27, 2024

We don't need to hard code it, but the test should probably be doing the same thing that the code itself is doing, namely looking in the git config file to find the right value.

We'll have to hope that file is present in CI. We've had tests in the past that were coupled to the actual .git content of the source, which failed in some test setups. If that happens we'll have to generate a new test fixture directory for this test and manually write that file.

ETA: Yep, tests fail in CI cause the remote is subtly different than you have locally. We'll want to use a tap testdir here so we can control what goes into that file.

@marshallku
Copy link
Author

marshallku commented Jun 28, 2024

Thanks for leading @wraithgar.
I used testdir, and add more cases to verify logics.

@marshallku
Copy link
Author

Hi @wraithgar

I wanted to kindly remind you about the pull request I submitted. Your feedback is highly valuable, and I’d appreciate it if you could take some time to review it.

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.

2 participants