Skip to content

Commit b4f9f77

Browse files
mhutchiemhutchie
committed
mhutchie#507 Display a warning before pushing a tag when it's on a commit that isn't on any known branch on the remote(s) the tag is being pushed to.
1 parent 5681ac7 commit b4f9f77

8 files changed

Lines changed: 320 additions & 72 deletions

File tree

‎src/dataSource.ts‎

Lines changed: 38 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@ import * as vscode from 'vscode';
66
import { AskpassEnvironment, AskpassManager } from './askpass/askpassManager';
77
import { getConfig } from './config';
88
import { Logger } from './logger';
9-
import { CommitOrdering, DateType, DeepWriteable, ErrorInfo, GitCommit, GitCommitDetails, GitCommitStash, GitConfigLocation, GitFileChange, GitFileStatus, GitPushBranchMode, GitRepoConfig, GitRepoConfigBranches, GitResetMode, GitSignature, GitSignatureStatus, GitStash, GitTagDetails, MergeActionOn, RebaseActionOn, SquashMessageFormat, TagType, Writeable } from './types';
9+
import { CommitOrdering, DateType, DeepWriteable, ErrorInfo, ErrorInfoExtensionPrefix, GitCommit, GitCommitDetails, GitCommitStash, GitConfigLocation, GitFileChange, GitFileStatus, GitPushBranchMode, GitRepoConfig, GitRepoConfigBranches, GitResetMode, GitSignature, GitSignatureStatus, GitStash, GitTagDetails, MergeActionOn, RebaseActionOn, SquashMessageFormat, TagType, Writeable } from './types';
1010
import { GitExecutable, GitVersionRequirement, UNABLE_TO_FIND_GIT_MSG, UNCOMMITTED, abbrevCommit, constructIncompatibleGitVersionMessage, doesVersionMeetRequirement, getPathFromStr, getPathFromUri, openGitTerminal, pathWithTrailingSlash, realpath, resolveSpawnOutput, showErrorMessage } from './utils';
1111
import { Disposable } from './utils/disposable';
1212
import { Event } from './utils/event';
@@ -779,17 +779,6 @@ export class DataSource extends Disposable {
779779
return this.runGitCommand(args, repo);
780780
}
781781

782-
/**
783-
* Push a tag to a remote.
784-
* @param repo The path of the repository.
785-
* @param tagName The name of the tag to push.
786-
* @param remote The remote to push the tag to.
787-
* @returns The ErrorInfo from the executed command.
788-
*/
789-
public pushTag(repo: string, tagName: string, remote: string) {
790-
return this.runGitCommand(['push', remote, tagName], repo);
791-
}
792-
793782
/**
794783
* Push a branch to multiple remotes.
795784
* @param repo The path of the repository.
@@ -814,20 +803,30 @@ export class DataSource extends Disposable {
814803
}
815804

816805
/**
817-
* Push a tag to multiple remotes.
806+
* Push a tag to remote(s).
818807
* @param repo The path of the repository.
819808
* @param tagName The name of the tag to push.
820-
* @param remote The remotes to push the tag to.
809+
* @param remotes The remote(s) to push the tag to.
810+
* @param commitHash The commit hash the tag is on.
811+
* @param skipRemoteCheck Skip checking that the tag is on each of the `remotes`.
821812
* @returns The ErrorInfo's from the executed commands.
822813
*/
823-
public async pushTagToMultipleRemotes(repo: string, tagName: string, remotes: string[]): Promise<ErrorInfo[]> {
814+
public async pushTag(repo: string, tagName: string, remotes: string[], commitHash: string, skipRemoteCheck: boolean): Promise<ErrorInfo[]> {
824815
if (remotes.length === 0) {
825816
return ['No remote(s) were specified to push the tag ' + tagName + ' to.'];
826817
}
827818

819+
if (!skipRemoteCheck) {
820+
const remotesContainingCommit = await this.getRemotesContainingCommit(repo, commitHash, remotes).catch(() => remotes);
821+
const remotesNotContainingCommit = remotes.filter((remote) => !remotesContainingCommit.includes(remote));
822+
if (remotesNotContainingCommit.length > 0) {
823+
return [ErrorInfoExtensionPrefix.PushTagCommitNotOnRemote + JSON.stringify(remotesNotContainingCommit)];
824+
}
825+
}
826+
828827
const results: ErrorInfo[] = [];
829828
for (let i = 0; i < remotes.length; i++) {
830-
const result = await this.pushTag(repo, tagName, remotes[i]);
829+
const result = await this.runGitCommand(['push', remotes[i], tagName], repo);
831830
results.push(result);
832831
if (result !== null) break;
833832
}
@@ -1573,6 +1572,29 @@ export class DataSource extends Disposable {
15731572
});
15741573
}
15751574

1575+
/**
1576+
* Get all of the remotes that contain the specified commit hash.
1577+
* @param repo The path of the repository.
1578+
* @param commitHash The commit hash to test.
1579+
* @param knownRemotes The list of known remotes to check for.
1580+
* @returns A promise resolving to a list of remote names.
1581+
*/
1582+
private getRemotesContainingCommit(repo: string, commitHash: string, knownRemotes: string[]) {
1583+
return this.spawnGit(['branch', '-r', '--no-color', '--contains=' + commitHash], repo, (stdout) => {
1584+
// Get the names of all known remote branches that contain commitHash
1585+
const branchNames = stdout.split(EOL_REGEX)
1586+
.filter((line) => line.length > 2)
1587+
.map((line) => line.substring(2).split(' -> ')[0])
1588+
.filter((branchName) => !INVALID_BRANCH_REGEXP.test(branchName));
1589+
1590+
// Get all the remotes that are the prefix of at least one remote branch name
1591+
return knownRemotes.filter((knownRemote) => {
1592+
const knownRemotePrefix = knownRemote + '/';
1593+
return branchNames.some((branchName) => branchName.startsWith(knownRemotePrefix));
1594+
});
1595+
});
1596+
}
1597+
15761598
/**
15771599
* Get the stashes in a repository.
15781600
* @param repo The path of the repository.

‎src/extensionState.ts‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,8 @@ export const DEFAULT_REPO_STATE: GitRepoState = {
4141

4242
const DEFAULT_GIT_GRAPH_VIEW_GLOBAL_STATE: GitGraphViewGlobalState = {
4343
alwaysAcceptCheckoutCommit: false,
44-
issueLinkingConfig: null
44+
issueLinkingConfig: null,
45+
pushTagSkipRemoteCheck: false
4546
};
4647

4748
const DEFAULT_GIT_GRAPH_VIEW_WORKSPACE_STATE: GitGraphViewWorkspaceState = {

‎src/gitGraphView.ts‎

Lines changed: 14 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -181,9 +181,16 @@ export class GitGraphView extends Disposable {
181181
case 'addTag':
182182
errorInfos = [await this.dataSource.addTag(msg.repo, msg.tagName, msg.commitHash, msg.type, msg.message, msg.force)];
183183
if (errorInfos[0] === null && msg.pushToRemote !== null) {
184-
errorInfos.push(await this.dataSource.pushTag(msg.repo, msg.tagName, msg.pushToRemote));
184+
errorInfos.push(...await this.dataSource.pushTag(msg.repo, msg.tagName, [msg.pushToRemote], msg.commitHash, msg.pushSkipRemoteCheck));
185185
}
186-
this.sendMessage({ command: 'addTag', errors: errorInfos });
186+
this.sendMessage({
187+
command: 'addTag',
188+
repo: msg.repo,
189+
tagName: msg.tagName,
190+
pushToRemote: msg.pushToRemote,
191+
commitHash: msg.commitHash,
192+
errors: errorInfos
193+
});
187194
break;
188195
case 'applyStash':
189196
this.sendMessage({
@@ -509,7 +516,11 @@ export class GitGraphView extends Disposable {
509516
case 'pushTag':
510517
this.sendMessage({
511518
command: 'pushTag',
512-
errors: await this.dataSource.pushTagToMultipleRemotes(msg.repo, msg.tagName, msg.remotes)
519+
repo: msg.repo,
520+
tagName: msg.tagName,
521+
remotes: msg.remotes,
522+
commitHash: msg.commitHash,
523+
errors: await this.dataSource.pushTag(msg.repo, msg.tagName, msg.remotes, msg.commitHash, msg.skipRemoteCheck)
513524
});
514525
break;
515526
case 'rebase':

‎src/types.ts‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -265,6 +265,7 @@ export interface GitGraphViewConfig {
265265
export interface GitGraphViewGlobalState {
266266
alwaysAcceptCheckoutCommit: boolean;
267267
issueLinkingConfig: IssueLinkingConfig | null;
268+
pushTagSkipRemoteCheck: boolean;
268269
}
269270

270271
export interface GitGraphViewWorkspaceState {
@@ -567,6 +568,9 @@ export interface ResponseWithMultiErrorInfo extends BaseMessage {
567568

568569
export type ErrorInfo = string | null; // null => no error, otherwise => error message
569570

571+
export const enum ErrorInfoExtensionPrefix {
572+
PushTagCommitNotOnRemote = 'VSCODE_GIT_GRAPH:PUSH_TAG:COMMIT_NOT_ON_REMOTE:'
573+
}
570574

571575
/* Request / Response Messages */
572576

@@ -588,10 +592,15 @@ export interface RequestAddTag extends RepoRequest {
588592
readonly type: TagType;
589593
readonly message: string;
590594
readonly pushToRemote: string | null; // string => name of the remote to push the tag to, null => don't push to a remote
595+
readonly pushSkipRemoteCheck: boolean;
591596
readonly force: boolean;
592597
}
593598
export interface ResponseAddTag extends ResponseWithMultiErrorInfo {
594599
readonly command: 'addTag';
600+
readonly repo: string;
601+
readonly tagName: string;
602+
readonly pushToRemote: string | null;
603+
readonly commitHash: string;
595604
}
596605

597606
export interface RequestApplyStash extends RepoRequest {
@@ -1054,9 +1063,15 @@ export interface RequestPushTag extends RepoRequest {
10541063
readonly command: 'pushTag';
10551064
readonly tagName: string;
10561065
readonly remotes: string[];
1066+
readonly commitHash: string;
1067+
readonly skipRemoteCheck: boolean;
10571068
}
10581069
export interface ResponsePushTag extends ResponseWithMultiErrorInfo {
10591070
readonly command: 'pushTag';
1071+
readonly repo: string;
1072+
readonly tagName: string;
1073+
readonly remotes: string[];
1074+
readonly commitHash: string;
10601075
}
10611076

10621077
export const enum RebaseActionOn {

‎tests/dataSource.test.ts‎

Lines changed: 124 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -4934,31 +4934,6 @@ describe('DataSource', () => {
49344934
});
49354935
});
49364936

4937-
describe('pushTag', () => {
4938-
it('Should push a tag to the remote', async () => {
4939-
// Setup
4940-
mockGitSuccessOnce();
4941-
4942-
// Run
4943-
const result = await dataSource.pushTag('/path/to/repo', 'tag-name', 'origin');
4944-
4945-
// Assert
4946-
expect(result).toBe(null);
4947-
expect(spyOnSpawn).toBeCalledWith('/path/to/git', ['push', 'origin', 'tag-name'], expect.objectContaining({ cwd: '/path/to/repo' }));
4948-
});
4949-
4950-
it('Should return an error message thrown by git', async () => {
4951-
// Setup
4952-
mockGitThrowingErrorOnce();
4953-
4954-
// Run
4955-
const result = await dataSource.pushTag('/path/to/repo', 'tag-name', 'origin');
4956-
4957-
// Assert
4958-
expect(result).toBe('error message');
4959-
});
4960-
});
4961-
49624937
describe('pushBranchToMultipleRemotes', () => {
49634938
it('Should push a branch to one remote', async () => {
49644939
// Setup
@@ -5009,13 +4984,13 @@ describe('DataSource', () => {
50094984
});
50104985
});
50114986

5012-
describe('pushTagToMultipleRemotes', () => {
4987+
describe('pushTag', () => {
50134988
it('Should push a tag to one remote', async () => {
50144989
// Setup
50154990
mockGitSuccessOnce();
50164991

50174992
// Run
5018-
const result = await dataSource.pushTagToMultipleRemotes('/path/to/repo', 'tag-name', ['origin']);
4993+
const result = await dataSource.pushTag('/path/to/repo', 'tag-name', ['origin'], '1a2b3c4d5e6f1a2b3c4d5e6f1a2b3c4d5e6f1a2b', true);
50194994

50204995
// Assert
50214996
expect(result).toStrictEqual([null]);
@@ -5028,21 +5003,140 @@ describe('DataSource', () => {
50285003
mockGitSuccessOnce();
50295004

50305005
// Run
5031-
const result = await dataSource.pushTagToMultipleRemotes('/path/to/repo', 'tag-name', ['origin', 'other-origin']);
5006+
const result = await dataSource.pushTag('/path/to/repo', 'tag-name', ['origin', 'other-origin'], '1a2b3c4d5e6f1a2b3c4d5e6f1a2b3c4d5e6f1a2b', true);
50325007

50335008
// Assert
50345009
expect(result).toStrictEqual([null, null]);
50355010
expect(spyOnSpawn).toBeCalledWith('/path/to/git', ['push', 'origin', 'tag-name'], expect.objectContaining({ cwd: '/path/to/repo' }));
50365011
expect(spyOnSpawn).toBeCalledWith('/path/to/git', ['push', 'other-origin', 'tag-name'], expect.objectContaining({ cwd: '/path/to/repo' }));
50375012
});
50385013

5014+
describe('Should check that the commit exists on each remote the tag is being pushed to', () => {
5015+
it('Commit exists on all remotes', async () => {
5016+
// Setup
5017+
mockGitSuccessOnce(
5018+
' origin/master\n' +
5019+
' other-origin/master\n'
5020+
);
5021+
mockGitSuccessOnce();
5022+
mockGitSuccessOnce();
5023+
5024+
// Run
5025+
const result = await dataSource.pushTag('/path/to/repo', 'tag-name', ['origin', 'other-origin'], '1a2b3c4d5e6f1a2b3c4d5e6f1a2b3c4d5e6f1a2b', false);
5026+
5027+
// Assert
5028+
expect(result).toStrictEqual([null, null]);
5029+
expect(spyOnSpawn).toBeCalledWith('/path/to/git', ['branch', '-r', '--no-color', '--contains=1a2b3c4d5e6f1a2b3c4d5e6f1a2b3c4d5e6f1a2b'], expect.objectContaining({ cwd: '/path/to/repo' }));
5030+
expect(spyOnSpawn).toBeCalledWith('/path/to/git', ['push', 'origin', 'tag-name'], expect.objectContaining({ cwd: '/path/to/repo' }));
5031+
expect(spyOnSpawn).toBeCalledWith('/path/to/git', ['push', 'other-origin', 'tag-name'], expect.objectContaining({ cwd: '/path/to/repo' }));
5032+
});
5033+
5034+
it('Commit exists on one remote', async () => {
5035+
// Setup
5036+
mockGitSuccessOnce(
5037+
' origin/master\n'
5038+
);
5039+
5040+
// Run
5041+
const result = await dataSource.pushTag('/path/to/repo', 'tag-name', ['origin', 'other-origin'], '1a2b3c4d5e6f1a2b3c4d5e6f1a2b3c4d5e6f1a2b', false);
5042+
5043+
// Assert
5044+
expect(result).toStrictEqual(['VSCODE_GIT_GRAPH:PUSH_TAG:COMMIT_NOT_ON_REMOTE:[\"other-origin\"]']);
5045+
expect(spyOnSpawn).toHaveBeenCalledTimes(1);
5046+
expect(spyOnSpawn).toBeCalledWith('/path/to/git', ['branch', '-r', '--no-color', '--contains=1a2b3c4d5e6f1a2b3c4d5e6f1a2b3c4d5e6f1a2b'], expect.objectContaining({ cwd: '/path/to/repo' }));
5047+
});
5048+
5049+
it('Commit doesn\'t exist on any remote', async () => {
5050+
// Setup
5051+
mockGitSuccessOnce('');
5052+
5053+
// Run
5054+
const result = await dataSource.pushTag('/path/to/repo', 'tag-name', ['origin', 'other-origin'], '1a2b3c4d5e6f1a2b3c4d5e6f1a2b3c4d5e6f1a2b', false);
5055+
5056+
// Assert
5057+
expect(result).toStrictEqual(['VSCODE_GIT_GRAPH:PUSH_TAG:COMMIT_NOT_ON_REMOTE:[\"origin\",\"other-origin\"]']);
5058+
expect(spyOnSpawn).toHaveBeenCalledTimes(1);
5059+
expect(spyOnSpawn).toBeCalledWith('/path/to/git', ['branch', '-r', '--no-color', '--contains=1a2b3c4d5e6f1a2b3c4d5e6f1a2b3c4d5e6f1a2b'], expect.objectContaining({ cwd: '/path/to/repo' }));
5060+
});
5061+
5062+
it('Handles remote branches with symbolic references', async () => {
5063+
// Setup
5064+
mockGitSuccessOnce(
5065+
' origin/HEAD -> origin/master\n' +
5066+
' other-origin/master\n'
5067+
);
5068+
mockGitSuccessOnce();
5069+
mockGitSuccessOnce();
5070+
5071+
// Run
5072+
const result = await dataSource.pushTag('/path/to/repo', 'tag-name', ['origin', 'other-origin'], '1a2b3c4d5e6f1a2b3c4d5e6f1a2b3c4d5e6f1a2b', false);
5073+
5074+
// Assert
5075+
expect(result).toStrictEqual([null, null]);
5076+
expect(spyOnSpawn).toBeCalledWith('/path/to/git', ['branch', '-r', '--no-color', '--contains=1a2b3c4d5e6f1a2b3c4d5e6f1a2b3c4d5e6f1a2b'], expect.objectContaining({ cwd: '/path/to/repo' }));
5077+
expect(spyOnSpawn).toBeCalledWith('/path/to/git', ['push', 'origin', 'tag-name'], expect.objectContaining({ cwd: '/path/to/repo' }));
5078+
expect(spyOnSpawn).toBeCalledWith('/path/to/git', ['push', 'other-origin', 'tag-name'], expect.objectContaining({ cwd: '/path/to/repo' }));
5079+
});
5080+
5081+
it('Handles remote names that contain slashes', async () => {
5082+
// Setup
5083+
mockGitSuccessOnce(
5084+
' origin/master\n' +
5085+
' other/origin/master\n'
5086+
);
5087+
mockGitSuccessOnce();
5088+
mockGitSuccessOnce();
5089+
5090+
// Run
5091+
const result = await dataSource.pushTag('/path/to/repo', 'tag-name', ['origin', 'other/origin'], '1a2b3c4d5e6f1a2b3c4d5e6f1a2b3c4d5e6f1a2b', false);
5092+
5093+
// Assert
5094+
expect(result).toStrictEqual([null, null]);
5095+
expect(spyOnSpawn).toBeCalledWith('/path/to/git', ['branch', '-r', '--no-color', '--contains=1a2b3c4d5e6f1a2b3c4d5e6f1a2b3c4d5e6f1a2b'], expect.objectContaining({ cwd: '/path/to/repo' }));
5096+
expect(spyOnSpawn).toBeCalledWith('/path/to/git', ['push', 'origin', 'tag-name'], expect.objectContaining({ cwd: '/path/to/repo' }));
5097+
expect(spyOnSpawn).toBeCalledWith('/path/to/git', ['push', 'other/origin', 'tag-name'], expect.objectContaining({ cwd: '/path/to/repo' }));
5098+
});
5099+
5100+
it('Ignores records that aren\'t branches in the git branch output', async () => {
5101+
// Setup
5102+
mockGitSuccessOnce(
5103+
' (invalid branch)\n' +
5104+
' other-origin/master\n'
5105+
);
5106+
5107+
// Run
5108+
const result = await dataSource.pushTag('/path/to/repo', 'tag-name', ['origin', 'other-origin'], '1a2b3c4d5e6f1a2b3c4d5e6f1a2b3c4d5e6f1a2b', false);
5109+
5110+
// Assert
5111+
expect(result).toStrictEqual(['VSCODE_GIT_GRAPH:PUSH_TAG:COMMIT_NOT_ON_REMOTE:[\"origin\"]']);
5112+
expect(spyOnSpawn).toHaveBeenCalledTimes(1);
5113+
expect(spyOnSpawn).toBeCalledWith('/path/to/git', ['branch', '-r', '--no-color', '--contains=1a2b3c4d5e6f1a2b3c4d5e6f1a2b3c4d5e6f1a2b'], expect.objectContaining({ cwd: '/path/to/repo' }));
5114+
});
5115+
5116+
it('Ignores when Git throws an exception', async () => {
5117+
// Setup
5118+
mockGitThrowingErrorOnce();
5119+
mockGitSuccessOnce();
5120+
mockGitSuccessOnce();
5121+
5122+
// Run
5123+
const result = await dataSource.pushTag('/path/to/repo', 'tag-name', ['origin', 'other-origin'], '1a2b3c4d5e6f1a2b3c4d5e6f1a2b3c4d5e6f1a2b', false);
5124+
5125+
// Assert
5126+
expect(result).toStrictEqual([null, null]);
5127+
expect(spyOnSpawn).toBeCalledWith('/path/to/git', ['branch', '-r', '--no-color', '--contains=1a2b3c4d5e6f1a2b3c4d5e6f1a2b3c4d5e6f1a2b'], expect.objectContaining({ cwd: '/path/to/repo' }));
5128+
expect(spyOnSpawn).toBeCalledWith('/path/to/git', ['push', 'origin', 'tag-name'], expect.objectContaining({ cwd: '/path/to/repo' }));
5129+
expect(spyOnSpawn).toBeCalledWith('/path/to/git', ['push', 'other-origin', 'tag-name'], expect.objectContaining({ cwd: '/path/to/repo' }));
5130+
});
5131+
});
5132+
50395133
it('Should push a tag to multiple remotes, stopping if an error occurs', async () => {
50405134
// Setup
50415135
mockGitSuccessOnce();
50425136
mockGitThrowingErrorOnce();
50435137

50445138
// Run
5045-
const result = await dataSource.pushTagToMultipleRemotes('/path/to/repo', 'tag-name', ['origin', 'other-origin', 'another-origin']);
5139+
const result = await dataSource.pushTag('/path/to/repo', 'tag-name', ['origin', 'other-origin', 'another-origin'], '1a2b3c4d5e6f1a2b3c4d5e6f1a2b3c4d5e6f1a2b', true);
50465140

50475141
// Assert
50485142
expect(result).toStrictEqual([null, 'error message']);
@@ -5052,7 +5146,7 @@ describe('DataSource', () => {
50525146

50535147
it('Should return an error when no remotes are specified', async () => {
50545148
// Run
5055-
const result = await dataSource.pushTagToMultipleRemotes('/path/to/repo', 'tag-name', []);
5149+
const result = await dataSource.pushTag('/path/to/repo', 'tag-name', [], '1a2b3c4d5e6f1a2b3c4d5e6f1a2b3c4d5e6f1a2b', true);
50565150

50575151
// Assert
50585152
expect(result).toStrictEqual(['No remote(s) were specified to push the tag tag-name to.']);

0 commit comments

Comments
 (0)