Skip to content

Commit 6aa0608

Browse files
mattseddonmattseddon
authored
Block users from attempting to run concurrent SCM commands (#2128)
* block users from accessing commands whilst executor is already running * do not block experiment commands * reference scm in variable names * self review
1 parent 552b25d commit 6aa0608

File tree

6 files changed

+136
-31
lines changed

6 files changed

+136
-31
lines changed

‎extension/package.json‎

Lines changed: 16 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -561,7 +561,7 @@
561561
},
562562
{
563563
"command": "dvc.checkout",
564-
"when": "dvc.commands.available && dvc.project.available"
564+
"when": "dvc.commands.available && dvc.project.available && !dvc.scm.command.running"
565565
},
566566
{
567567
"command": "dvc.checkCLICompatible",
@@ -573,7 +573,7 @@
573573
},
574574
{
575575
"command": "dvc.commit",
576-
"when": "dvc.commands.available && dvc.project.available"
576+
"when": "dvc.commands.available && dvc.project.available && !dvc.scm.command.running"
577577
},
578578
{
579579
"command": "dvc.commitTarget",
@@ -613,7 +613,7 @@
613613
},
614614
{
615615
"command": "dvc.init",
616-
"when": "dvc.commands.available && !dvc.cli.incompatible && !dvc.project.available"
616+
"when": "dvc.commands.available && !dvc.cli.incompatible && !dvc.project.available && !dvc.scm.command.running"
617617
},
618618
{
619619
"command": "dvc.moveTargets",
@@ -625,15 +625,15 @@
625625
},
626626
{
627627
"command": "dvc.pull",
628-
"when": "dvc.commands.available && dvc.project.available"
628+
"when": "dvc.commands.available && dvc.project.available && !dvc.scm.command.running"
629629
},
630630
{
631631
"command": "dvc.pullTarget",
632632
"when": "false"
633633
},
634634
{
635635
"command": "dvc.push",
636-
"when": "dvc.commands.available && dvc.project.available"
636+
"when": "dvc.commands.available && dvc.project.available && !dvc.scm.command.running"
637637
},
638638
{
639639
"command": "dvc.pushTarget",
@@ -689,7 +689,7 @@
689689
},
690690
{
691691
"command": "dvc.discardWorkspaceChanges",
692-
"when": "dvc.commands.available && dvc.project.available"
692+
"when": "dvc.commands.available && dvc.project.available && !dvc.scm.command.running"
693693
},
694694
{
695695
"command": "dvc.runExperiment",
@@ -836,34 +836,34 @@
836836
{
837837
"command": "dvc.addTarget",
838838
"group": "inline",
839-
"when": "scmProvider == dvc && scmResourceState == untracked && dvc.commands.available"
839+
"when": "scmProvider == dvc && scmResourceState == untracked && dvc.commands.available && !dvc.scm.command.running"
840840
},
841841
{
842842
"command": "dvc.checkoutTarget",
843843
"group": "inline",
844-
"when": "scmProvider == dvc && scmResourceGroup == changes && scmResourceState != untracked && scmResourceState != notInCache && dvc.commands.available"
844+
"when": "scmProvider == dvc && scmResourceGroup == changes && scmResourceState != untracked && scmResourceState != notInCache && dvc.commands.available && !dvc.scm.command.running"
845845
},
846846
{
847847
"command": "dvc.commitTarget",
848848
"group": "inline",
849-
"when": "scmProvider == dvc && scmResourceGroup == changes && scmResourceState != untracked && scmResourceState != deleted && dvc.commands.available"
849+
"when": "scmProvider == dvc && scmResourceGroup == changes && scmResourceState != untracked && scmResourceState != deleted && dvc.commands.available && !dvc.scm.command.running"
850850
},
851851
{
852852
"command": "dvc.pullTarget",
853853
"group": "inline",
854-
"when": "scmProvider == dvc && scmResourceGroup == notInCache && dvc.commands.available"
854+
"when": "scmProvider == dvc && scmResourceGroup == notInCache && dvc.commands.available && !dvc.scm.command.running"
855855
}
856856
],
857857
"scm/resourceGroup/context": [
858858
{
859859
"command": "dvc.checkout",
860860
"group": "inline",
861-
"when": "scmProvider == dvc && scmResourceGroup == changes"
861+
"when": "scmProvider == dvc && scmResourceGroup == changes && !dvc.scm.command.running"
862862
},
863863
{
864864
"command": "dvc.commit",
865865
"group": "inline",
866-
"when": "scmProvider == dvc && scmResourceGroup == changes"
866+
"when": "scmProvider == dvc && scmResourceGroup == changes && !dvc.scm.command.running"
867867
},
868868
{
869869
"command": "dvc.gitStageAll",
@@ -942,17 +942,17 @@
942942
{
943943
"command": "dvc.moveTargets",
944944
"group": "1_DVC@1",
945-
"when": "view == dvc.views.trackedExplorerTree && viewItem =~ /^dirData$/"
945+
"when": "view == dvc.views.trackedExplorerTree && viewItem =~ /^dirData$/ && !dvc.scm.command.running"
946946
},
947947
{
948948
"command": "dvc.pullTarget",
949949
"group": "1_DVC@2",
950-
"when": "view == dvc.views.trackedExplorerTree"
950+
"when": "view == dvc.views.trackedExplorerTree && !dvc.scm.command.running"
951951
},
952952
{
953953
"command": "dvc.pushTarget",
954954
"group": "1_DVC@3",
955-
"when": "view == dvc.views.trackedExplorerTree && viewItem != virtual"
955+
"when": "view == dvc.views.trackedExplorerTree && viewItem != virtual && !dvc.scm.command.running"
956956
},
957957
{
958958
"command": "dvc.compareSelected",
@@ -992,7 +992,7 @@
992992
{
993993
"command": "dvc.removeTarget",
994994
"group": "7_modification@2",
995-
"when": "view == dvc.views.trackedExplorerTree && viewItem =~ /^.*Data$/"
995+
"when": "view == dvc.views.trackedExplorerTree && viewItem =~ /^.*Data$/ && !dvc.scm.command.running"
996996
},
997997
{
998998
"command": "dvc.addExperimentsTableSort",

‎extension/src/cli/executor.test.ts‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,11 +8,13 @@ import { createProcess } from '../processExecution'
88
import { getMockedProcess } from '../test/util/jest'
99
import { getProcessEnv } from '../env'
1010
import { Config } from '../config'
11+
import { setContextValue } from '../vscode/context'
1112

1213
jest.mock('vscode')
1314
jest.mock('@hediet/std/disposable')
1415
jest.mock('../processExecution')
1516
jest.mock('../env')
17+
jest.mock('../vscode/context')
1618

1719
const mockedDisposable = jest.mocked(Disposable)
1820

@@ -24,6 +26,8 @@ const mockedEnv = {
2426
PATH: '/some/special/path'
2527
}
2628

29+
const mockedSetContextValue = jest.mocked(setContextValue)
30+
2731
beforeEach(() => {
2832
jest.resetAllMocks()
2933
mockedGetProcessEnv.mockReturnValueOnce(mockedEnv)
@@ -582,6 +586,16 @@ describe('CliExecutor', () => {
582586
env: mockedEnv,
583587
executable: 'dvc'
584588
})
589+
590+
expect(mockedSetContextValue).toBeCalledTimes(2)
591+
expect(mockedSetContextValue).toBeCalledWith(
592+
'dvc.scm.command.running',
593+
true
594+
)
595+
expect(mockedSetContextValue).toHaveBeenLastCalledWith(
596+
'dvc.scm.command.running',
597+
false
598+
)
585599
})
586600
})
587601
})

‎extension/src/cli/executor.ts‎

Lines changed: 32 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ import {
77
Flag,
88
GcPreserveFlag
99
} from './constants'
10+
import { setContextValue } from '../vscode/context'
1011

1112
export const autoRegisteredCommands = {
1213
ADD: 'add',
@@ -19,6 +20,7 @@ export const autoRegisteredCommands = {
1920
EXPERIMENT_REMOVE: 'experimentRemove',
2021
EXPERIMENT_REMOVE_QUEUE: 'experimentRemoveQueue',
2122
INIT: 'init',
23+
IS_SCM_COMMAND_RUNNING: 'isScmCommandRunning',
2224
MOVE: 'move',
2325
PULL: 'pull',
2426
PUSH: 'push',
@@ -31,16 +33,18 @@ export class CliExecutor extends Cli {
3133
this
3234
)
3335

36+
private scmCommandRunning = false
37+
3438
public add(cwd: string, target: string) {
35-
return this.executeProcess(cwd, Command.ADD, target)
39+
return this.blockAndExecuteProcess(cwd, Command.ADD, target)
3640
}
3741

3842
public checkout(cwd: string, ...args: Args) {
39-
return this.executeProcess(cwd, Command.CHECKOUT, ...args)
43+
return this.blockAndExecuteProcess(cwd, Command.CHECKOUT, ...args)
4044
}
4145

4246
public commit(cwd: string, ...args: Args) {
43-
return this.executeProcess(cwd, Command.COMMIT, ...args)
47+
return this.blockAndExecuteProcess(cwd, Command.COMMIT, ...args)
4448
}
4549

4650
public experimentApply(cwd: string, experimentName: string) {
@@ -97,27 +101,47 @@ export class CliExecutor extends Cli {
97101
)
98102
}
99103

104+
public isScmCommandRunning() {
105+
return this.scmCommandRunning
106+
}
107+
100108
public init(cwd: string) {
101-
return this.executeProcess(cwd, Command.INITIALIZE, Flag.SUBDIRECTORY)
109+
return this.blockAndExecuteProcess(
110+
cwd,
111+
Command.INITIALIZE,
112+
Flag.SUBDIRECTORY
113+
)
102114
}
103115

104116
public move(cwd: string, target: string, destination: string) {
105-
return this.executeProcess(cwd, Command.MOVE, target, destination)
117+
return this.blockAndExecuteProcess(cwd, Command.MOVE, target, destination)
106118
}
107119

108120
public pull(cwd: string, ...args: Args) {
109-
return this.executeProcess(cwd, Command.PULL, ...args)
121+
return this.blockAndExecuteProcess(cwd, Command.PULL, ...args)
110122
}
111123

112124
public push(cwd: string, ...args: Args) {
113-
return this.executeProcess(cwd, Command.PUSH, ...args)
125+
return this.blockAndExecuteProcess(cwd, Command.PUSH, ...args)
114126
}
115127

116128
public remove(cwd: string, ...args: Args) {
117-
return this.executeProcess(cwd, Command.REMOVE, ...args)
129+
return this.blockAndExecuteProcess(cwd, Command.REMOVE, ...args)
118130
}
119131

120132
private executeExperimentProcess(cwd: string, ...args: Args) {
121133
return this.executeProcess(cwd, Command.EXPERIMENT, ...args)
122134
}
135+
136+
private async blockAndExecuteProcess(cwd: string, ...args: Args) {
137+
this.setRunning(true)
138+
const output = await this.executeProcess(cwd, ...args)
139+
this.setRunning(false)
140+
return output
141+
}
142+
143+
private setRunning(running: boolean) {
144+
this.scmCommandRunning = running
145+
setContextValue('dvc.scm.command.running', running)
146+
}
123147
}

‎extension/src/repository/commands/index.test.ts‎

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,13 @@ mockedInternalCommands.registerCommand(mockedCommandId, (...args) =>
2525
mockedFunc(...args)
2626
)
2727

28+
const mockedIsScmCommandRunning = jest.fn()
29+
30+
mockedInternalCommands.registerCommand(
31+
'isScmCommandRunning',
32+
mockedIsScmCommandRunning
33+
)
34+
2835
const TRY_FORCE = 'Use `-f|--force` to force.'
2936

3037
jest.mock('vscode')
@@ -182,6 +189,7 @@ describe('getSimpleResourceCommand', () => {
182189
describe('getRootCommand', () => {
183190
it('should return a function that returns early if a cwd is not provided', async () => {
184191
mockedGetCwd.mockResolvedValueOnce(undefined)
192+
mockedIsScmCommandRunning.mockResolvedValueOnce(false)
185193
const stdout = 'I cannot run without a cwd'
186194
mockedFunc.mockResolvedValueOnce(stdout)
187195

@@ -199,6 +207,7 @@ describe('getRootCommand', () => {
199207

200208
it('should return a function that only calls the first function if it succeeds', async () => {
201209
mockedGetCwd.mockImplementationOnce(uri => uri?.fsPath)
210+
mockedIsScmCommandRunning.mockResolvedValueOnce(false)
202211
const stdout = 'all went well, congrats'
203212
mockedFunc.mockResolvedValueOnce(stdout)
204213

@@ -219,6 +228,7 @@ describe('getRootCommand', () => {
219228

220229
it('should return a function that throws an error if the underlying function fails without a force prompt', async () => {
221230
mockedGetCwd.mockImplementationOnce(uri => uri?.fsPath)
231+
mockedIsScmCommandRunning.mockResolvedValueOnce(false)
222232
const stderr = 'I deed'
223233
mockedFunc.mockRejectedValueOnce({ stderr })
224234

@@ -239,6 +249,7 @@ describe('getRootCommand', () => {
239249

240250
it('should return a function that calls warnOfConsequences if the first function fails with a force prompt', async () => {
241251
mockedGetCwd.mockImplementationOnce(uri => uri?.fsPath)
252+
mockedIsScmCommandRunning.mockResolvedValueOnce(false)
242253
const stderr = `I deed, but ${TRY_FORCE}`
243254
const userCancelled = undefined
244255
mockedFunc.mockRejectedValueOnce({ stderr })
@@ -260,6 +271,7 @@ describe('getRootCommand', () => {
260271

261272
it('should return a function that does not call the force func if the user selects cancel', async () => {
262273
mockedGetCwd.mockImplementationOnce(uri => uri?.fsPath)
274+
mockedIsScmCommandRunning.mockResolvedValueOnce(false)
263275
const stderr = `You don't have to do this, but ${TRY_FORCE}`
264276
const userCancelled = 'Cancel'
265277
mockedFunc.mockRejectedValueOnce({ stderr })
@@ -281,6 +293,7 @@ describe('getRootCommand', () => {
281293

282294
it('should return a function that does not call the force func if no stderr is returned in the underlying error', async () => {
283295
mockedGetCwd.mockImplementationOnce(uri => uri?.fsPath)
296+
mockedIsScmCommandRunning.mockResolvedValueOnce(false)
284297
const userCancelled = 'Cancel'
285298
mockedFunc.mockRejectedValueOnce({})
286299
mockedGetWarningResponse.mockResolvedValueOnce(userCancelled)
@@ -302,6 +315,7 @@ describe('getRootCommand', () => {
302315

303316
it('should return a function that calls the force function if the first function fails with a force prompt and the user responds with force', async () => {
304317
mockedGetCwd.mockImplementationOnce(uri => uri?.fsPath)
318+
mockedIsScmCommandRunning.mockResolvedValueOnce(false)
305319
const stderr = `I can fix this... maybe, but ${TRY_FORCE}`
306320
const forcedStdout = 'ok, nw I forced it'
307321
const userApproves = 'Force'
@@ -325,4 +339,22 @@ describe('getRootCommand', () => {
325339
expect(mockedFunc).toHaveBeenCalledWith(mockedDvcRoot, '-f')
326340
expect(mockedFunc).toHaveBeenCalledTimes(2)
327341
})
342+
343+
it('should return a function that returns early if another user initiated command is running', async () => {
344+
mockedGetCwd.mockImplementationOnce(uri => uri?.fsPath)
345+
mockedIsScmCommandRunning.mockResolvedValueOnce(true)
346+
347+
const commandToRegister = getRootCommand(
348+
mockedRepositories,
349+
mockedInternalCommands,
350+
mockedCommandId
351+
)
352+
353+
const output = await commandToRegister({
354+
rootUri: { fsPath: mockedDvcRoot } as Uri
355+
})
356+
357+
expect(output).toBeUndefined()
358+
expect(mockedFunc).not.toHaveBeenCalled()
359+
})
328360
})

0 commit comments

Comments
 (0)