-
Notifications
You must be signed in to change notification settings - Fork 2.4k
fix: ensure symlinked git directories work #2394
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -238,6 +238,85 @@ describe('git-auth-helper tests', () => { | |||||
| expect(setSecretSpy).toHaveBeenCalledWith(expectedSecret) | ||||||
| }) | ||||||
|
|
||||||
| const configureAuth_resolvesSymlinksInIncludeIfGitdir = | ||||||
| 'configureAuth resolves symlinks in includeIf gitdir' | ||||||
| it(configureAuth_resolvesSymlinksInIncludeIfGitdir, async () => { | ||||||
| if (isWindows) { | ||||||
| process.stdout.write( | ||||||
| `Skipped test "${configureAuth_resolvesSymlinksInIncludeIfGitdir}". Symlink creation requires admin privileges on Windows.\n` | ||||||
| ) | ||||||
| return | ||||||
| } | ||||||
|
|
||||||
| // Arrange | ||||||
| await setup(configureAuth_resolvesSymlinksInIncludeIfGitdir) | ||||||
|
|
||||||
| const symlinkPath = path.join(path.dirname(workspace), 'workspace-symlink') | ||||||
|
|
||||||
| try { | ||||||
| // Ensure no pre-existing symlink or file remains at this path | ||||||
| await fs.promises.rm(symlinkPath, {force: true}) | ||||||
|
|
||||||
| // Create a symlink pointing to the real workspace directory | ||||||
| await fs.promises.symlink(workspace, symlinkPath) | ||||||
|
|
||||||
| // Make git appear to be operating from the symlink path | ||||||
| ;(git.getWorkingDirectory as jest.Mock).mockReturnValue(symlinkPath) | ||||||
| process.env['GITHUB_WORKSPACE'] = symlinkPath | ||||||
|
|
||||||
| const authHelper = gitAuthHelper.createAuthHelper(git, settings) | ||||||
|
|
||||||
| // Act | ||||||
| await authHelper.configureAuth() | ||||||
|
|
||||||
| // Assert the host includeIf uses the real resolved path, not the symlink path | ||||||
| const localConfigContent = ( | ||||||
| await fs.promises.readFile(localGitConfigPath) | ||||||
| ).toString() | ||||||
| const realGitDir = ( | ||||||
| await fs.promises.realpath(path.join(symlinkPath, '.git')) | ||||||
| ).replace(/\\/g, '/') | ||||||
| const symlinkGitDir = path.join(symlinkPath, '.git').replace(/\\/g, '/') | ||||||
|
Comment on lines
+276
to
+279
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Personally, I'd be inclined to have a function: function convertBackslashes(file: string) {
return file.replace(/\\/g, '/')
}... const realGitDir = convertBackslashes(
await fs.promises.realpath(path.join(symlinkPath, '.git'))
)
const symlinkGitDir = convertBackslashes(path.join(symlinkPath, '.git'))... const fallbackGitDir = convertBackslashes(
path.join(nonexistentPath, '.git')
)I'm not entirely sure about house style, but I occasionally encourage DRY. It helps understand that strings of code are the same. |
||||||
|
|
||||||
| expect(realGitDir).not.toBe(symlinkGitDir) // sanity check: paths differ | ||||||
| expect( | ||||||
| localConfigContent.indexOf(`includeIf.gitdir:${realGitDir}.path`) | ||||||
| ).toBeGreaterThanOrEqual(0) | ||||||
| expect(localConfigContent.indexOf(symlinkGitDir)).toBeLessThan(0) | ||||||
| } finally { | ||||||
| // Clean up symlink (or any file) at the symlink path | ||||||
| await fs.promises.rm(symlinkPath, {force: true}) | ||||||
| } | ||||||
| }) | ||||||
|
|
||||||
| const configureAuth_fallsBackWhenRealpathSyncFails = | ||||||
| 'configureAuth falls back to constructed path when realpathSync fails' | ||||||
| it(configureAuth_fallsBackWhenRealpathSyncFails, async () => { | ||||||
| // Arrange | ||||||
| await setup(configureAuth_fallsBackWhenRealpathSyncFails) | ||||||
|
|
||||||
| // Use a non-existent path so realpathSync throws ENOENT naturally, | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
(This matches your variable in the const below and is a real word) |
||||||
| // exercising the catch fallback in configureToken() | ||||||
| const nonexistentPath = path.join(runnerTemp, 'does-not-exist') | ||||||
| ;(git.getWorkingDirectory as jest.Mock).mockReturnValue(nonexistentPath) | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
? |
||||||
|
|
||||||
| const authHelper = gitAuthHelper.createAuthHelper(git, settings) | ||||||
|
|
||||||
| // Act - should not throw despite realpathSync failure | ||||||
| await authHelper.configureAuth() | ||||||
|
|
||||||
| // Assert the fallback constructed path is used in the includeIf entry | ||||||
| const localConfigContent = ( | ||||||
| await fs.promises.readFile(localGitConfigPath) | ||||||
| ).toString() | ||||||
| const fallbackGitDir = path | ||||||
| .join(nonexistentPath, '.git') | ||||||
| .replace(/\\/g, '/') | ||||||
| expect( | ||||||
| localConfigContent.indexOf(`includeIf.gitdir:${fallbackGitDir}.path`) | ||||||
| ).toBeGreaterThanOrEqual(0) | ||||||
| }) | ||||||
|
|
||||||
| const setsSshCommandEnvVarWhenPersistCredentialsFalse = | ||||||
| 'sets SSH command env var when persist-credentials false' | ||||||
| it(setsSshCommandEnvVarWhenPersistCredentialsFalse, async () => { | ||||||
|
|
||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the
;here needed?