From 708dcfbff77c353f26f80fa69d59c35039d5386a Mon Sep 17 00:00:00 2001 From: Elizabeth Craig Date: Tue, 28 Apr 2026 23:41:49 -0700 Subject: [PATCH] Fix deepening of shallow clones in latest git --- ...-d27a5175-146e-487e-86e5-d28822fb3467.json | 11 ++ .../src/__fixtures__/repositoryFactory.ts | 8 + .../git/ensureSharedHistory.test.ts | 101 +++++++----- .../src/__functional__/git/fetch.test.ts | 155 +++++++++++------- .../__snapshots__/bumpAndPush.test.ts.snap | 12 +- .../src/__tests__/publish/bumpAndPush.test.ts | 2 +- .../beachball/src/git/ensureSharedHistory.ts | 29 ++-- packages/beachball/src/git/fetch.ts | 40 +++-- 8 files changed, 224 insertions(+), 134 deletions(-) create mode 100644 change/change-d27a5175-146e-487e-86e5-d28822fb3467.json diff --git a/change/change-d27a5175-146e-487e-86e5-d28822fb3467.json b/change/change-d27a5175-146e-487e-86e5-d28822fb3467.json new file mode 100644 index 000000000..8820cc151 --- /dev/null +++ b/change/change-d27a5175-146e-487e-86e5-d28822fb3467.json @@ -0,0 +1,11 @@ +{ + "changes": [ + { + "type": "patch", + "comment": "Fix deepening of shallow clones in latest git by being more specific about the comparison ref being fetched. Also reduce git operations for the case where the branch ref doesn't exist locally.", + "packageName": "beachball", + "email": "elcraig@microsoft.com", + "dependentChangeType": "patch" + } + ] +} \ No newline at end of file diff --git a/packages/beachball/src/__fixtures__/repositoryFactory.ts b/packages/beachball/src/__fixtures__/repositoryFactory.ts index 610dc909f..901be0ed6 100644 --- a/packages/beachball/src/__fixtures__/repositoryFactory.ts +++ b/packages/beachball/src/__fixtures__/repositoryFactory.ts @@ -162,6 +162,14 @@ export class RepositoryFactory { /** Cloned child repos, tracked so we can clean them up */ private childRepos: Repository[] = []; + /** Get the root directory of the origin repository */ + public get originRoot(): string { + if (!this.root) { + throw new Error('Factory was already cleaned'); + } + return this.root; + } + /** * Create the "origin" repo and create+commit fixture files. * If `fixture` is a string, the corresponding default fixture is used. diff --git a/packages/beachball/src/__functional__/git/ensureSharedHistory.test.ts b/packages/beachball/src/__functional__/git/ensureSharedHistory.test.ts index 997a534d4..045ac686f 100644 --- a/packages/beachball/src/__functional__/git/ensureSharedHistory.test.ts +++ b/packages/beachball/src/__functional__/git/ensureSharedHistory.test.ts @@ -18,6 +18,7 @@ describe('ensureSharedHistory', () => { let repositoryFactory: RepositoryFactory; const logs = initMockLogs(); const testBranch = 'test'; + const defaultRefSpec = `+refs/heads/${defaultBranchName}:refs/remotes/${defaultRemoteBranchName}`; const realGit = jest.requireActual('workspace-tools').git; /** @@ -25,8 +26,7 @@ describe('ensureSharedHistory', () => { * (Use this instead of `.mockImplementation()` to avoid interference with other mocks.) */ let gitOverride: typeof workspaceTools.git | undefined; - const gitSpy = jest - .spyOn(workspaceTools, 'git') + const gitSpy = (workspaceTools.git as jest.MockedFunction) // Attempt to force git to use English in logs, so we can check for absence of "warning" strings .mockImplementation((args, opts) => (gitOverride || realGit)(args, opts && optsWithLang(opts))); @@ -77,10 +77,10 @@ describe('ensureSharedHistory', () => { ensureSharedHistory({ path: repo.rootPath, verbose: true, branch: defaultRemoteBranchName, fetch: true }); // Ensure the expected git calls were made - expect(filteredGitCalls()).toEqual(['fetch origin master']); + expect(filteredGitCalls()).toEqual([`fetch origin ${defaultRefSpec}`]); const allLogs = logs.getMockLines('all'); - expect(allLogs).toMatch('Fetching branch "master" from remote "origin"...'); + expect(allLogs).toMatch(`Fetching branch "master" from remote "origin" (${defaultRefSpec})...`); expect(allLogs).toMatch('Fetching branch "master" from remote "origin" completed successfully'); expect(allLogs).not.toMatch('warning'); }); @@ -118,7 +118,7 @@ describe('ensureSharedHistory', () => { gitSpy.mockClear(); ensureSharedHistory({ path: repo.rootPath, verbose: true, branch: defaultRemoteBranchName, fetch: true, depth: 1 }); - expect(filteredGitCalls()).toEqual(['fetch origin master']); + expect(filteredGitCalls()).toEqual([`fetch origin ${defaultRefSpec}`]); }); it('errors if fetching is disabled and target branch does not exist locally', () => { @@ -168,10 +168,10 @@ describe('ensureSharedHistory', () => { expect(() => ensureSharedHistory({ path: repo.rootPath, verbose: true, branch: defaultRemoteBranchName, fetch: true }) ).toThrow('Fetching branch "master" from remote "origin" failed'); - expect(filteredGitCalls()).toContain('fetch origin master'); + expect(filteredGitCalls()).toContain(`fetch origin ${defaultRefSpec}`); const allLogs = logs.getMockLines('all'); - expect(allLogs).toMatch('Fetching branch "master" from remote "origin"...'); + expect(allLogs).toMatch(`Fetching branch "master" from remote "origin" (${defaultRefSpec})...`); expect(allLogs).toMatch('Fetching branch "master" from remote "origin" failed'); }); @@ -183,8 +183,7 @@ describe('ensureSharedHistory', () => { ensureSharedHistory({ path: repo.rootPath, verbose: false, branch: 'origin/fake', fetch: true }) ).toThrow('Fetching branch "fake" from remote "origin" failed'); const gitOps = filteredGitCalls(); - expect(gitOps).not.toContain('remote set-branches --add origin fake'); - expect(gitOps).toContain('fetch origin fake'); + expect(gitOps).toContain('fetch origin +refs/heads/fake:refs/remotes/origin/fake'); expect(logs.getMockLines('all')).toEqual(''); }); @@ -198,12 +197,12 @@ describe('ensureSharedHistory', () => { ).toThrow('Fetching branch "fake" from remote "origin" failed'); const gitOps = filteredGitCalls(); - expect(gitOps).toContain('remote set-branches --add origin fake'); - expect(gitOps).toContain('fetch origin fake'); + expect(gitOps).toContain('fetch origin +refs/heads/fake:refs/remotes/origin/fake'); const allLogs = logs.getMockLines('all'); - expect(allLogs).toMatch('Adding branch "fake" to fetch config for remote "origin"'); - expect(allLogs).toMatch('Fetching branch "fake" from remote "origin"...'); + expect(allLogs).toMatch( + `Fetching branch "fake" from remote "origin" (+refs/heads/fake:refs/remotes/origin/fake)...` + ); }); it('errors if fetching is disabled and adequate history is not available', () => { @@ -237,13 +236,13 @@ describe('ensureSharedHistory', () => { branch: defaultRemoteBranchName, fetch: true, depth: 2, - // Run this with verbose git logs so we can verify arguments were correct (no "warning" logs). - // However, this adds noise including filesystem paths to the output, so we can't snapshot it. + // Run this with verbose git logs so we can verify arguments were correct (no "warning" logs) verbose: true, }); - const allLogs = logs.getMockLines('all'); - expect(allLogs).toMatch('Adding branch "master" to fetch config for remote "origin"'); + const allLogs = logs.getMockLines('all', { + replacePaths: { [repo.rootPath]: '', [repositoryFactory.originRoot]: '' }, + }); expect(allLogs).toMatch('This is a shallow clone. Deepening to check for changes...'); expect(allLogs).toMatch('Deepening by 2 more commits (attempt 1/3)...'); expect(allLogs).toMatch('Deepening by 2 more commits (attempt 2/3)...'); @@ -252,17 +251,40 @@ describe('ensureSharedHistory', () => { expect(logs.mocks.warn).not.toHaveBeenCalled(); expect(logs.mocks.error).not.toHaveBeenCalled(); - expect(filteredGitCalls().filter(line => !line.startsWith('rev-parse') && !line.startsWith('merge-base'))) - .toMatchInlineSnapshot(` - [ - "config --get-all remote.origin.fetch", - "remote set-branches --add origin master", - "fetch --depth=2 origin master", - "fetch --deepen=2 origin master", - "fetch --deepen=2 origin master", - "fetch --deepen=2 origin master", - ] - `); + const deepen = `fetch --deepen=2 origin ${defaultRefSpec}`; + expect(filteredGitCalls()).toEqual([`fetch --depth=2 origin ${defaultRefSpec}`, deepen, deepen, deepen]); + }); + + it('deepens history if needed when tracking ref already exists locally', () => { + // singleBranch: false means origin/master is already fetched at depth 1 before ensureSharedHistory + // runs, so hasBranchRef returns true and the ref already exists — but history is still too shallow. + // This verifies that deepening correctly updates the existing tracking ref (requires explicit refspec). + const repo = repositoryFactory.cloneRepository({ depth: 1, branch: testBranch, singleBranch: false }); + + // Verify the precondition: origin/master already exists locally (at depth 1) + expect(realGit(['rev-parse', '--verify', defaultRemoteBranchName], { cwd: repo.rootPath }).success).toBe(true); + + gitSpy.mockClear(); + + ensureSharedHistory({ + path: repo.rootPath, + branch: defaultRemoteBranchName, + fetch: true, + depth: 2, + verbose: true, + }); + + const allLogs = logs.getMockLines('all', { + replacePaths: { [repo.rootPath]: '', [repositoryFactory.originRoot]: '' }, + }); + expect(allLogs).toMatch('This is a shallow clone. Deepening to check for changes...'); + expect(allLogs).toMatch('Deepening by 2 more commits (attempt 1/3)...'); + expect(allLogs).not.toMatch('warning'); + expect(logs.mocks.warn).not.toHaveBeenCalled(); + expect(logs.mocks.error).not.toHaveBeenCalled(); + + const deepen = `fetch --deepen=2 origin ${defaultRefSpec}`; + expect(filteredGitCalls()).toEqual([`fetch --depth=2 origin ${defaultRefSpec}`, deepen, deepen, deepen]); }); it('unshallows if deepening attempts fail', () => { @@ -279,17 +301,14 @@ describe('ensureSharedHistory', () => { }); expect(logs.getMockLines('all')).toMatch("Still didn't find a common commit after deepening by 3. Unshallowing..."); - expect(filteredGitCalls()).toMatchInlineSnapshot(` - [ - "config --get-all remote.origin.fetch", - "remote set-branches --add origin master", - "fetch --depth=1 origin master", - "fetch --deepen=1 origin master", - "fetch --deepen=1 origin master", - "fetch --deepen=1 origin master", - "fetch --unshallow origin master", - ] - `); + const deepen = `fetch --deepen=1 origin ${defaultRefSpec}`; + expect(filteredGitCalls()).toEqual([ + `fetch --depth=1 origin ${defaultRefSpec}`, + deepen, + deepen, + deepen, + `fetch --unshallow origin ${defaultRefSpec}`, + ]); }); it('errors if there is no common history', () => { @@ -309,12 +328,12 @@ describe('ensureSharedHistory', () => { // The fetch part succeeded const allLogs = logs.getMockLines('all'); - expect(allLogs).toMatch('Fetching branch "master" from remote "origin"...'); + expect(allLogs).toMatch(`Fetching branch "master" from remote "origin" (${defaultRefSpec})...`); expect(allLogs).toMatch('Fetching branch "master" from remote "origin" completed successfully'); expect(filteredGitCalls([])).toEqual([ 'rev-parse --verify origin/master', - 'fetch origin master', + `fetch origin ${defaultRefSpec}`, 'merge-base origin/master HEAD', 'rev-parse --is-shallow-repository', ]); diff --git a/packages/beachball/src/__functional__/git/fetch.test.ts b/packages/beachball/src/__functional__/git/fetch.test.ts index 5e3d96680..b6a6787a0 100644 --- a/packages/beachball/src/__functional__/git/fetch.test.ts +++ b/packages/beachball/src/__functional__/git/fetch.test.ts @@ -27,13 +27,18 @@ describe('gitFetch', () => { /** To speed things up, some tests only check the arguments and skip the git operation */ const noOpSuccess = () => ({ success: true, stdout: '', stderr: '', status: 0 } as GitProcessOutput); - const realGit = jest.requireActual('workspace-tools').git; + const { git: realGit, gitFailFast: _realGitFailFast } = jest.requireActual('workspace-tools'); + const gitFailFast: typeof _realGitFailFast = (args, options) => + _realGitFailFast(args, { cwd: '', ...options, noExitCode: true }); + /** * Set this to override the git implementation for one test. * (Use this instead of `.mockImplementation()` to avoid interference with other mocks.) */ let gitOverride: typeof realGit | undefined; - const gitSpy = jest.spyOn(workspaceTools, 'git').mockImplementation((...args) => (gitOverride || realGit)(...args)); + const gitSpy = (workspaceTools.git as jest.MockedFunction).mockImplementation((...args) => + (gitOverride || realGit)(...args) + ); beforeAll(() => { repositoryFactory = new RepositoryFactory('single'); @@ -58,18 +63,19 @@ describe('gitFetch', () => { it('throws if mutually exclusive options are specified', () => { const err = '"depth", "deepen", and "unshallow" are mutually exclusive'; + const common = { cwd: '', remote: '', branch: defaultBranchName }; // use 0 for all of the depth/deepen values to verify it's not using falsy checks - expect(() => gitFetch({ cwd: '', depth: 0, deepen: 0 })).toThrow(err); - expect(() => gitFetch({ cwd: '', depth: 0, unshallow: true })).toThrow(err); - expect(() => gitFetch({ cwd: '', deepen: 0, unshallow: true })).toThrow(err); - expect(() => gitFetch({ cwd: '', depth: 0, deepen: 0, unshallow: true })).toThrow(err); + expect(() => gitFetch({ ...common, depth: 0, deepen: 0 })).toThrow(err); + expect(() => gitFetch({ ...common, depth: 0, unshallow: true })).toThrow(err); + expect(() => gitFetch({ ...common, deepen: 0, unshallow: true })).toThrow(err); + expect(() => gitFetch({ ...common, depth: 0, deepen: 0, unshallow: true })).toThrow(err); expect(gitSpy).not.toHaveBeenCalled(); }); it('fetches and does not log by default', () => { - const res = gitFetch({ cwd: repo.rootPath }); + const res = gitFetch({ cwd: repo.rootPath, remote: '', branch: defaultBranchName }); expect(gitSpy).toHaveBeenCalledWith(['fetch'], { cwd: repo.rootPath, stdio: 'pipe' }); - expect(res.success).toBe(true); + expect(res).toMatchObject({ success: true }); expect(logs.mocks.log).not.toHaveBeenCalled(); }); @@ -77,16 +83,14 @@ describe('gitFetch', () => { // This test uses controlled non-localized fake stdio so we can test the whole output gitOverride = () => ({ success: false, stdout: 'some logs', stderr: 'oh no', status: 1 } as GitProcessOutput); - const res = gitFetch({ cwd: repo.rootPath }); - expect(res).toEqual( - expect.objectContaining({ - success: false, - errorMessage: ['Fetching all remotes failed (code 1)', 'stdout:', 'some logs', 'stderr:', 'oh no'].join('\n'), - status: 1, - stderr: 'oh no', - stdout: 'some logs', - }) - ); + const res = gitFetch({ cwd: repo.rootPath, remote: '', branch: defaultBranchName }); + expect(res).toMatchObject({ + success: false, + errorMessage: ['Fetching all remotes failed (code 1)', 'stdout:', 'some logs', 'stderr:', 'oh no'].join('\n'), + status: 1, + stderr: 'oh no', + stdout: 'some logs', + }); expect(logs.mocks.log).not.toHaveBeenCalled(); }); @@ -94,11 +98,13 @@ describe('gitFetch', () => { repo.git(['remote', 'set-url', defaultRemoteName, 'invalid-url']); modifiedRemote = true; - const res = gitFetch({ cwd: repo.rootPath }); - expect(res.success).toBe(false); - expect(res.errorMessage).toContain('Fetching all remotes failed (code 128)'); - // The URL is the only part of the error message that isn't localized - expect(res.stderr).toContain('invalid-url'); + const res = gitFetch({ cwd: repo.rootPath, branch: defaultBranchName, remote: '' }); + expect(res).toMatchObject({ + success: false, + errorMessage: expect.stringContaining('Fetching all remotes failed (code 128)'), + // The URL is the only part of the error message that isn't localized + stderr: expect.stringContaining('invalid-url'), + }); expect(res.errorMessage).toContain('invalid-url'); }); @@ -106,11 +112,11 @@ describe('gitFetch', () => { // use predictable output gitOverride = () => ({ ...noOpSuccess(), stdout: 'some logs', stderr: 'some debug' }); - const res = gitFetch({ cwd: repo.rootPath, verbose: true }); + const res = gitFetch({ cwd: repo.rootPath, verbose: true, remote: '', branch: defaultBranchName }); // normally this would be called with stdio: inherit, but it's not done that way in tests // because process.stdout/stderr can't be mocked, so the test output would be too spammy expect(gitSpy).toHaveBeenCalledWith(['fetch'], expect.anything()); - expect(res.success).toBe(true); + expect(res).toMatchObject({ success: true }); expect(res.errorMessage).toBeUndefined(); expect(logs.mocks.log).toHaveBeenCalledWith('Fetching all remotes...'); expect(logs.mocks.log).toHaveBeenCalledWith('some logs'); @@ -121,74 +127,109 @@ describe('gitFetch', () => { it('logs git output with failed fetch if verbose is true', () => { gitOverride = () => ({ success: false, stdout: 'some logs', stderr: 'oh no', status: 1 } as GitProcessOutput); - const res = gitFetch({ cwd: repo.rootPath, verbose: true }); + const res = gitFetch({ cwd: repo.rootPath, verbose: true, remote: '', branch: defaultBranchName }); expect(gitSpy).toHaveBeenCalledWith(['fetch'], expect.anything()); - expect(res).toEqual( - expect.objectContaining({ - success: false, - errorMessage: 'Fetching all remotes failed (code 1) - see above for details', - status: 1, - }) - ); + expect(res).toMatchObject({ + success: false, + errorMessage: 'Fetching all remotes failed (code 1) - see above for details', + status: 1, + }); expect(logs.mocks.log).toHaveBeenCalledWith('Fetching all remotes...'); expect(logs.mocks.log).toHaveBeenCalledWith('some logs'); expect(logs.mocks.warn).toHaveBeenCalledWith('oh no'); expect(logs.mocks.warn).toHaveBeenCalledWith('Fetching all remotes failed (code 1)'); }); - it('fetches remote if specified', () => { - gitOverride = noOpSuccess; - const res = gitFetch({ cwd: repo.rootPath, remote: defaultRemoteName, verbose: true }); - - expect(gitSpy).toHaveBeenCalledWith(['fetch', defaultRemoteName], expect.anything()); - expect(res.success).toBe(true); - expect(logs.mocks.log).toHaveBeenCalledWith(`Fetching remote "${defaultRemoteName}"...`); - }); - it('fetches remote and branch if specified', () => { gitOverride = noOpSuccess; const res = gitFetch({ cwd: repo.rootPath, remote: defaultRemoteName, branch: defaultBranchName, verbose: true }); - expect(gitSpy).toHaveBeenCalledWith(['fetch', defaultRemoteName, defaultBranchName], expect.anything()); - expect(res.success).toBe(true); + // refs/heads/ on the source side is unambiguous: bare branch names can be silently + // misresolved, causing git to treat the ref as absent and delete the local tracking ref. + const refspec = `+refs/heads/${defaultBranchName}:refs/remotes/${defaultRemoteName}/${defaultBranchName}`; + expect(gitSpy).toHaveBeenCalledWith(['fetch', defaultRemoteName, refspec], expect.anything()); + expect(res).toMatchObject({ success: true }); expect(logs.mocks.log).toHaveBeenCalledWith( - `Fetching branch "${defaultBranchName}" from remote "${defaultRemoteName}"...` + `Fetching branch "${defaultBranchName}" from remote "${defaultRemoteName}" (${refspec})...` ); }); - it('ignores branch if remote is not specified', () => { - gitOverride = noOpSuccess; - const res = gitFetch({ cwd: repo.rootPath, branch: defaultBranchName, verbose: true }); + it('preserves the tracking ref after a real fetch', () => { + // With a bare branch name like 'master' as the refspec source, git can fail to resolve it + // on the remote and treat it as absent, pruning refs/remotes/origin/master (exit code 0). + // Using refs/heads/ avoids this. This test runs a real fetch to catch any regression. + const res = gitFetch({ cwd: repo.rootPath, remote: defaultRemoteName, branch: defaultBranchName }); + expect(res).toMatchObject({ success: true }); - expect(gitSpy).toHaveBeenCalledWith(['fetch'], expect.anything()); - expect(res.success).toBe(true); - expect(logs.mocks.log).toHaveBeenCalledWith(`Fetching all remotes...`); + const trackingRef = `refs/remotes/${defaultRemoteName}/${defaultBranchName}`; + gitFailFast(['rev-parse', '--verify', trackingRef], { cwd: repo.rootPath }); + gitFailFast(['merge-base', `${defaultRemoteName}/${defaultBranchName}`, 'HEAD'], { cwd: repo.rootPath }); + }); + + it('only updates the fetched remote tracking ref in a fork-like scenario', () => { + // Fork setup: 'origin' is the fork, 'upstream' is the original repo. + // Local master may track upstream, not origin. Fetching from origin must only update + // refs/remotes/origin/*, and must not touch refs/remotes/upstream/* or refs/heads/*. + const forkRepo = repositoryFactory.cloneRepository(); + forkRepo.git(['remote', 'add', 'upstream', realRemoteUrl]); + forkRepo.git(['fetch', 'upstream']); + + const upstreamTrackingRef = `refs/remotes/upstream/${defaultBranchName}`; + const upstreamShaBefore = realGit(['rev-parse', upstreamTrackingRef], { cwd: forkRepo.rootPath }).stdout.trim(); + const localBranchShaBefore = realGit(['rev-parse', `refs/heads/${defaultBranchName}`], { + cwd: forkRepo.rootPath, + }).stdout.trim(); + expect(upstreamShaBefore).toBeTruthy(); + + gitSpy.mockClear(); + const res = gitFetch({ cwd: forkRepo.rootPath, remote: defaultRemoteName, branch: defaultBranchName }); + expect(res).toMatchObject({ success: true }); + + // The fetch command must target only origin with the correct refspec + const expectedRefspec = `+refs/heads/${defaultBranchName}:refs/remotes/${defaultRemoteName}/${defaultBranchName}`; + expect(gitSpy).toHaveBeenCalledWith(['fetch', defaultRemoteName, expectedRefspec], expect.anything()); + + // origin/master must exist and be reachable + gitFailFast(['rev-parse', '--verify', `refs/remotes/${defaultRemoteName}/${defaultBranchName}`], { + cwd: forkRepo.rootPath, + }); + gitFailFast(['merge-base', `${defaultRemoteName}/${defaultBranchName}`, 'HEAD'], { cwd: forkRepo.rootPath }); + + // upstream/master must be completely unaffected + const upstreamShaAfter = realGit(['rev-parse', upstreamTrackingRef], { cwd: forkRepo.rootPath }).stdout.trim(); + expect(upstreamShaAfter).toBe(upstreamShaBefore); + + // refs/heads/master (local branch) must be untouched + const localBranchShaAfter = realGit(['rev-parse', `refs/heads/${defaultBranchName}`], { + cwd: forkRepo.rootPath, + }).stdout.trim(); + expect(localBranchShaAfter).toBe(localBranchShaBefore); }); it('respects depth option', () => { gitOverride = noOpSuccess; - const res = gitFetch({ cwd: repo.rootPath, depth: 1, verbose: true }); + const res = gitFetch({ cwd: repo.rootPath, depth: 1, verbose: true, remote: '', branch: defaultBranchName }); expect(gitSpy).toHaveBeenCalledWith(['fetch', '--depth=1'], expect.anything()); - expect(res.success).toBe(true); + expect(res).toMatchObject({ success: true }); expect(logs.mocks.log).toHaveBeenCalledWith(`Fetching all remotes (with --depth=1)...`); }); it('respects deepen option', () => { gitOverride = noOpSuccess; - const res = gitFetch({ cwd: repo.rootPath, deepen: 1, verbose: true }); + const res = gitFetch({ cwd: repo.rootPath, deepen: 1, verbose: true, remote: '', branch: defaultBranchName }); expect(gitSpy).toHaveBeenCalledWith(['fetch', '--deepen=1'], expect.anything()); - expect(res.success).toBe(true); + expect(res).toMatchObject({ success: true }); expect(logs.mocks.log).toHaveBeenCalledWith(`Fetching all remotes (with --deepen=1)...`); }); it('respects unshallow option', () => { gitOverride = noOpSuccess; - const res = gitFetch({ cwd: repo.rootPath, unshallow: true, verbose: true }); + const res = gitFetch({ cwd: repo.rootPath, unshallow: true, verbose: true, remote: '', branch: defaultBranchName }); expect(gitSpy).toHaveBeenCalledWith(['fetch', '--unshallow'], expect.anything()); - expect(res.success).toBe(true); + expect(res).toMatchObject({ success: true }); expect(logs.mocks.log).toHaveBeenCalledWith(`Fetching all remotes (with --unshallow)...`); }); }); diff --git a/packages/beachball/src/__tests__/publish/__snapshots__/bumpAndPush.test.ts.snap b/packages/beachball/src/__tests__/publish/__snapshots__/bumpAndPush.test.ts.snap index e1302b1a9..7cfd8de44 100644 --- a/packages/beachball/src/__tests__/publish/__snapshots__/bumpAndPush.test.ts.snap +++ b/packages/beachball/src/__tests__/publish/__snapshots__/bumpAndPush.test.ts.snap @@ -5,7 +5,7 @@ exports[`bumpAndPush retries on fetch failure then succeeds 2`] = ` [log] Bumping versions and pushing to git (attempt 1/5) [log] Reverting [log] -[log] Fetching branch "master" from remote "origin"... +[log] Fetching branch "master" from remote "origin" (+refs/heads/master:refs/remotes/origin/master)... [warn] fetch error [warn] Fetching branch "master" from remote "origin" failed (code 1) [warn] [WARN 1/5]: Fetching from origin/master has failed! (see above for details) @@ -13,7 +13,7 @@ exports[`bumpAndPush retries on fetch failure then succeeds 2`] = ` [log] Bumping versions and pushing to git (attempt 2/5) [log] Reverting [log] -[log] Fetching branch "master" from remote "origin"... +[log] Fetching branch "master" from remote "origin" (+refs/heads/master:refs/remotes/origin/master)... [log] Fetching branch "master" from remote "origin" completed successfully [log] Merging with origin/master... @@ -50,7 +50,7 @@ exports[`bumpAndPush succeeds on first attempt 1`] = ` [log] Bumping versions and pushing to git (attempt 1/5) [log] Reverting [log] -[log] Fetching branch "master" from remote "origin"... +[log] Fetching branch "master" from remote "origin" (+refs/heads/master:refs/remotes/origin/master)... [log] Fetching branch "master" from remote "origin" completed successfully [log] Merging with origin/master... @@ -87,7 +87,7 @@ exports[`bumpAndPush throws and calls displayManualRecovery after all retries ex [log] Bumping versions and pushing to git (attempt 1/3) [log] Reverting [log] -[log] Fetching branch "master" from remote "origin"... +[log] Fetching branch "master" from remote "origin" (+refs/heads/master:refs/remotes/origin/master)... [warn] network error [warn] Fetching branch "master" from remote "origin" failed (code 1) [warn] [WARN 1/3]: Fetching from origin/master has failed! (see above for details) @@ -95,7 +95,7 @@ exports[`bumpAndPush throws and calls displayManualRecovery after all retries ex [log] Bumping versions and pushing to git (attempt 2/3) [log] Reverting [log] -[log] Fetching branch "master" from remote "origin"... +[log] Fetching branch "master" from remote "origin" (+refs/heads/master:refs/remotes/origin/master)... [warn] network error [warn] Fetching branch "master" from remote "origin" failed (code 1) [warn] [WARN 2/3]: Fetching from origin/master has failed! (see above for details) @@ -103,7 +103,7 @@ exports[`bumpAndPush throws and calls displayManualRecovery after all retries ex [log] Bumping versions and pushing to git (attempt 3/3) [log] Reverting [log] -[log] Fetching branch "master" from remote "origin"... +[log] Fetching branch "master" from remote "origin" (+refs/heads/master:refs/remotes/origin/master)... [warn] network error [warn] Fetching branch "master" from remote "origin" failed (code 1) [warn] [WARN 3/3]: Fetching from origin/master has failed! (see above for details) diff --git a/packages/beachball/src/__tests__/publish/bumpAndPush.test.ts b/packages/beachball/src/__tests__/publish/bumpAndPush.test.ts index cccf92d50..cf1bd44bb 100644 --- a/packages/beachball/src/__tests__/publish/bumpAndPush.test.ts +++ b/packages/beachball/src/__tests__/publish/bumpAndPush.test.ts @@ -106,7 +106,7 @@ describe('bumpAndPush', () => { expect(mockPerformBump).toHaveBeenCalledTimes(1); expect(mockTagPackages).toHaveBeenCalledTimes(1); expect(wsToolsMocks.revertLocalChanges).toHaveBeenCalledTimes(1); - expect(getWsToolsGitCalls().join('\n')).toEqual('git fetch origin master'); + expect(getWsToolsGitCalls().join('\n')).toEqual('git fetch origin +refs/heads/master:refs/remotes/origin/master'); expect(getExecaCalls()).toEqual([ 'git merge -X theirs origin/master', 'git add .', diff --git a/packages/beachball/src/git/ensureSharedHistory.ts b/packages/beachball/src/git/ensureSharedHistory.ts index ce1e2514a..f1460b461 100644 --- a/packages/beachball/src/git/ensureSharedHistory.ts +++ b/packages/beachball/src/git/ensureSharedHistory.ts @@ -6,8 +6,14 @@ import { BeachballError } from '../types/BeachballError'; /** * Ensure that adequate history is available to check for changes between HEAD and `options.branch`. - * Otherwise attempting to get changes will fail with an error "no merge base". - * (This is mostly an issue with CI builds that use shallow clones.) + * Otherwise attempting to get changes will fail with an error "no merge base". (This is mostly an + * issue with CI builds that use shallow clones and may not have a tracking branch configured for + * the remote target branch.) + * + * Since repo may be very large, it's important to avoid fetching the entire history or extra refs. + * If the repo is shallow and no common commit is found after a normal fetch, this function will + * iteratively deepen the history (100 commits at a time by default) and check for a common commit + * each round. If still not found, it will unshallow the clone. * * Throws an error if history is inadequate and cannot be fixed. */ @@ -33,26 +39,13 @@ export function ensureSharedHistory( } if (!remote) { - // If the remote isn't specified, even if fetching is allowed it will be unclear what to - // compare against + // If the remote isn't specified, even if fetching is allowed, it will be unclear what to + // compare against, so throw an error. throw new BeachballError( `Target branch "${branch}" doesn't exist locally, and a remote name wasn't specified and couldn't be inferred. ` + 'Please set "repository" in your root package.json or include a remote in the beachball "branch" setting.' ); } - - // If fetching the requested branch isn't already (probably) configured, add it to the list so - // it can be fetched in the next step. Otherwise the ref / won't exist locally. - const fetchConfig = git(['config', '--get-all', `remote.${remote}.fetch`], { cwd }).stdout.trim(); - if (!fetchConfig.includes(`${remote}/*`) && !fetchConfig.includes(branch)) { - console.log(`Adding branch "${remoteBranch}" to fetch config for remote "${remote}"`); - const result = git(['remote', 'set-branches', '--add', remote, remoteBranch], { cwd }); - if (!result.success) { - throw new BeachballError( - `Failed to add branch "${remoteBranch}" to fetch config for remote "${remote}":\n${result.stderr}` - ); - } - } } if (fetch) { @@ -89,6 +82,8 @@ export function ensureSharedHistory( // Try fetching more history isConnected = deepenHistory({ remote, remoteBranch, branch, depth, cwd, verbose }); + } else { + // Repo isn't shallow, so potentially we just need to add a ref to be connected } if (!isConnected) { diff --git a/packages/beachball/src/git/fetch.ts b/packages/beachball/src/git/fetch.ts index 3950e183e..c0ad6cc68 100644 --- a/packages/beachball/src/git/fetch.ts +++ b/packages/beachball/src/git/fetch.ts @@ -3,10 +3,16 @@ import { getGitEnv } from './gitAsync'; type GitFetchParams = { cwd: string; - /** Remote to fetch from. If not specified, fetches all remotes. */ - remote?: string; - /** Branch to fetch. This will be ignored if `remote` is not also specified. */ - branch?: string; + /** + * Remote to fetch from. This should almost always be set but might be an empty string + * if the repo somehow has no remotes configured. + */ + remote: string; + /** + * Branch to fetch. It will be converted to a full refspec for fetching: + * e.g. `branch: 'main', remote: 'origin'` will be converted to `+main:refs/remotes/origin/main`. + */ + branch: string; /** Set depth to this number of commits (mutually exclusive with `deepen` and `unshallow`) */ depth?: number; /** Deepen a shallow clone by this number of commits (mutually exclusive with `depth` and `unshallow`) */ @@ -20,6 +26,9 @@ type GitFetchParams = { * Wrapper for `git fetch`. If `verbose` is true, log the command before starting, and display output * on stdout (except in tests). In tests with `verbose`, the output will be logged all together to * `console.log` when the command finishes (for easier mocking/capturing). + * + * This converts `remote` and `branch` into a fully qualified refspec, so it doesn't matter if + * the remote branch is tracked or not in the local repository. */ export function gitFetch(params: GitFetchParams): GitProcessOutput & { errorMessage?: string } { const { remote, branch, depth, deepen, unshallow, cwd, verbose } = params; @@ -31,14 +40,21 @@ export function gitFetch(params: GitFetchParams): GitProcessOutput & { errorMess const extraArgs = depth ? [`--depth=${depth}`] : deepen ? [`--deepen=${deepen}`] : unshallow ? ['--unshallow'] : []; - let description = remote - ? `Fetching ${branch ? `branch "${branch}" from ` : ''}remote "${remote}"` - : 'Fetching all remotes'; + // Be specific with the ref being fetched, so we don't have to worry about tracking configs. + // In git fetch +:... + // - The + means allow non-fast-forward updates (in case the remote was force pushed). + // - refs/heads/${branch} is resolved against the remote's advertised refs. The fully + // qualified ref is unambiguous, whereas bare branch names can be silently misresolved, + // causing git to treat the ref as absent and delete the local tracking ref. + // - refs/remotes/${remote}/${branch} is resolved locally and only moves the tracking ref + // for the remote branch, not the local refs/heads/${branch} or its tracking config. + const resolvedBranch = remote ? `+refs/heads/${branch}:refs/remotes/${remote}/${branch}` : undefined; - if (extraArgs.length) { - description += ` (with ${extraArgs.join(' ')})`; - } + const shortDescription = `Fetching ${resolvedBranch ? `branch "${branch}" from remote "${remote}"` : 'all remotes'}`; + let description = shortDescription; + resolvedBranch && (description += ` (${resolvedBranch})`); + extraArgs.length && (description += ` (with ${extraArgs.join(' ')})`); shouldLog && console.log(description + '...'); const result: ReturnType = git( @@ -46,7 +62,7 @@ export function gitFetch(params: GitFetchParams): GitProcessOutput & { errorMess 'fetch', ...extraArgs, // If the remote is unknown, don't specify the branch (fetching a branch without a remote is invalid) - ...(remote && branch ? [remote, branch] : remote ? [remote] : []), + ...(resolvedBranch ? [remote, resolvedBranch] : []), ], { cwd, stdio: shouldLog === 'live' ? 'inherit' : 'pipe' } ); @@ -59,7 +75,7 @@ export function gitFetch(params: GitFetchParams): GitProcessOutput & { errorMess result.stderr && log(result.stderr); } - let message = `${description} ${result.success ? 'completed successfully' : `failed (code ${result.status})`}`; + let message = `${shortDescription} ${result.success ? 'completed successfully' : `failed (code ${result.status})`}`; if (shouldLog) { log(message); message += ' - see above for details';