From babca743d226101b7d2017cecf93b79275b37328 Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Mon, 16 Nov 2020 14:55:11 -0500 Subject: [PATCH 01/17] Support global setConfig and getConfig in all repository states --- lib/models/repository-states/state.js | 18 ++++++++++++++---- test/models/repository.test.js | 19 +++++++++++++++---- 2 files changed, 29 insertions(+), 8 deletions(-) diff --git a/lib/models/repository-states/state.js b/lib/models/repository-states/state.js index 9152b98e55..1e228fcf33 100644 --- a/lib/models/repository-states/state.js +++ b/lib/models/repository-states/state.js @@ -1,8 +1,10 @@ +import path from 'path'; import {nullCommit} from '../commit'; import BranchSet from '../branch-set'; import RemoteSet from '../remote-set'; import {nullOperationStates} from '../operation-states'; import MultiFilePatch from '../patch/multi-file-patch'; +import CompositeGitStrategy from '../../composite-git-strategy'; /** * Map of registered subclasses to allow states to transition to one another without circular dependencies. @@ -210,8 +212,8 @@ export default class State { // Configuration - setConfig(option, value, {replaceAll} = {}) { - return unsupportedOperationPromise(this, 'setConfig'); + setConfig(optionName, value, options) { + return this.workdirlessGit().setConfig(optionName, value, options); } unsetConfig(option) { @@ -364,8 +366,8 @@ export default class State { return Promise.resolve(null); } - getConfig(option, {local} = {}) { - return Promise.resolve(null); + getConfig(optionName, options) { + return this.workdirlessGit().getConfig(optionName, options); } // Direct blob access @@ -470,6 +472,14 @@ export default class State { // Direct git access // Non-delegated git operations for internal use within states. + workdirlessGit() { + // We want to report config values from the global or system level, but never local ones (unless we're in the + // present state, which overrides this). + // The filesystem root is the most likely and convenient place for this to be true. + const {root} = path.parse(process.cwd()); + return CompositeGitStrategy.create(root); + } + /* istanbul ignore next */ directResolveDotGitDir() { return Promise.resolve(null); diff --git a/test/models/repository.test.js b/test/models/repository.test.js index 3aede68ea6..97f9f16adb 100644 --- a/test/models/repository.test.js +++ b/test/models/repository.test.js @@ -74,7 +74,7 @@ describe('Repository', function() { // Methods that resolve to null for (const method of [ - 'getAheadCount', 'getBehindCount', 'getConfig', 'getLastHistorySnapshots', 'getCache', + 'getAheadCount', 'getBehindCount', 'getLastHistorySnapshots', 'getCache', ]) { assert.isNull(await repository[method]()); } @@ -121,7 +121,7 @@ describe('Repository', function() { 'init', 'clone', 'stageFiles', 'unstageFiles', 'stageFilesFromParentCommit', 'applyPatchToIndex', 'applyPatchToWorkdir', 'commit', 'merge', 'abortMerge', 'checkoutSide', 'mergeFile', 'writeMergeConflictToIndex', 'checkout', 'checkoutPathsAtRevision', 'undoLastCommit', 'fetch', 'pull', - 'push', 'setConfig', 'unsetConfig', 'createBlob', 'expandBlobToFile', 'createDiscardHistoryBlob', + 'push', 'unsetConfig', 'createBlob', 'expandBlobToFile', 'createDiscardHistoryBlob', 'updateDiscardHistory', 'storeBeforeAndAfterBlobs', 'restoreLastDiscardInTempFiles', 'popDiscardHistory', 'clearDiscardHistory', 'discardWorkDirChangesForPaths', 'addRemote', 'setCommitMessage', 'fetchCommitMessageTemplate', @@ -138,6 +138,12 @@ describe('Repository', function() { /fatal: Not a valid object name abcd/, ); }); + + it('works anyway', async function() { + sinon.stub(atom, 'inSpecMode').returns(false); + await repository.setConfig('atomGithub.test', 'yes', {global: true}); + assert.strictEqual(await repository.getConfig('atomGithub.test'), 'yes'); + }); }); it('accesses an OperationStates model', async function() { @@ -193,6 +199,8 @@ describe('Repository', function() { }); it('returns null remote before repository has loaded', async function() { + sinon.stub(atom, 'inSpecMode').returns(false); + const loadingRepository = new Repository(workdir); const remote = await loadingRepository.getCurrentGitHubRemote(); assert.isFalse(remote.isPresent()); @@ -1321,14 +1329,17 @@ describe('Repository', function() { }); it('does nothing on a destroyed repository', async function() { + sinon.spy(repository, 'setConfig'); repository.destroy(); await repository.saveDiscardHistory(); - assert.isNull(await repository.getConfig('atomGithub.historySha')); + assert.isFalse(repository.setConfig.called); }); it('does nothing if the repository is destroyed after the blob is created', async function() { + sinon.spy(repository, 'setConfig'); + let resolveCreateHistoryBlob = () => {}; sinon.stub(repository, 'createDiscardHistoryBlob').callsFake(() => new Promise(resolve => { resolveCreateHistoryBlob = resolve; @@ -1339,7 +1350,7 @@ describe('Repository', function() { resolveCreateHistoryBlob('nope'); await promise; - assert.isNull(await repository.getConfig('atomGithub.historySha')); + assert.isFalse(repository.setConfig.called); }); it('creates a blob and saves it in the git config', async function() { From 660302dc1b32cf1f816b36b05e7266da5c6cede0 Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Mon, 16 Nov 2020 15:03:09 -0500 Subject: [PATCH 02/17] Fire a repository update after workdirless setConfig --- lib/models/repository-states/state.js | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/lib/models/repository-states/state.js b/lib/models/repository-states/state.js index 1e228fcf33..e284b9b28e 100644 --- a/lib/models/repository-states/state.js +++ b/lib/models/repository-states/state.js @@ -212,8 +212,9 @@ export default class State { // Configuration - setConfig(optionName, value, options) { - return this.workdirlessGit().setConfig(optionName, value, options); + async setConfig(optionName, value, options) { + await this.workdirlessGit().setConfig(optionName, value, options); + this.didUpdate(); } unsetConfig(option) { From e944bb3f7c2749a62dbbc96386ff2f7d9f84212b Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Wed, 18 Nov 2020 14:32:18 -0500 Subject: [PATCH 03/17] Return null from getConfig on "--local" error --- lib/git-shell-out-strategy.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/git-shell-out-strategy.js b/lib/git-shell-out-strategy.js index 53c88b8919..f39b08a161 100644 --- a/lib/git-shell-out-strategy.js +++ b/lib/git-shell-out-strategy.js @@ -1011,8 +1011,8 @@ export default class GitShellOutStrategy { args = args.concat(option); output = await this.exec(args); } catch (err) { - if (err.code === 1) { - // No matching config found + if (err.code === 1 || err.code === 128) { + // No matching config found OR --local can only be used inside a git repository return null; } else { throw err; From 3957d6cc64f35a8916096a6c8c7e0bbd6dbfe74f Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Wed, 18 Nov 2020 15:20:23 -0500 Subject: [PATCH 04/17] Selectively override the automatic --local in specs --- lib/git-shell-out-strategy.js | 4 ++-- lib/models/repository-states/state.js | 2 +- test/models/repository.test.js | 2 -- 3 files changed, 3 insertions(+), 5 deletions(-) diff --git a/lib/git-shell-out-strategy.js b/lib/git-shell-out-strategy.js index f39b08a161..d61217b7a7 100644 --- a/lib/git-shell-out-strategy.js +++ b/lib/git-shell-out-strategy.js @@ -1003,11 +1003,11 @@ export default class GitShellOutStrategy { return (await this.exec(['describe', '--contains', '--all', '--always', 'HEAD'])).trim(); } - async getConfig(option, {local} = {}) { + async getConfig(option, {local, direct} = {}) { let output; try { let args = ['config']; - if (local || atom.inSpecMode()) { args.push('--local'); } + if (local || (!direct && atom.inSpecMode())) { args.push('--local'); } args = args.concat(option); output = await this.exec(args); } catch (err) { diff --git a/lib/models/repository-states/state.js b/lib/models/repository-states/state.js index e284b9b28e..5a03e17850 100644 --- a/lib/models/repository-states/state.js +++ b/lib/models/repository-states/state.js @@ -368,7 +368,7 @@ export default class State { } getConfig(optionName, options) { - return this.workdirlessGit().getConfig(optionName, options); + return this.workdirlessGit().getConfig(optionName, {direct: true, ...options}); } // Direct blob access diff --git a/test/models/repository.test.js b/test/models/repository.test.js index 97f9f16adb..17807756a9 100644 --- a/test/models/repository.test.js +++ b/test/models/repository.test.js @@ -199,8 +199,6 @@ describe('Repository', function() { }); it('returns null remote before repository has loaded', async function() { - sinon.stub(atom, 'inSpecMode').returns(false); - const loadingRepository = new Repository(workdir); const remote = await loadingRepository.getCurrentGitHubRemote(); assert.isFalse(remote.isPresent()); From 4c07417c9202a00dda0e6ee297389da9603976ce Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Wed, 18 Nov 2020 15:36:17 -0500 Subject: [PATCH 05/17] Set global git config in CI --- .github/workflows/ci.yml | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index a2f81f7e43..b063d7c50f 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -39,6 +39,11 @@ jobs: - name: install dependencies shell: bash run: ${APM} ci + - name: configure git + shell: bash + run: | + git config --global user.name Hubot + git config --global user.email hubot@github.com - name: run tests shell: bash run: ${ATOM} --test test/ @@ -79,6 +84,11 @@ jobs: - name: install dependencies shell: bash run: sh -c "${APM} ci" + - name: configure git + shell: bash + run: | + git config --global user.name Hubot + git config --global user.email hubot@github.com - name: run tests shell: bash run: sh -c "${ATOM} --test test/" From 29e175ca0f4fd86f79b48917119030236c619fce Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Fri, 20 Nov 2020 10:23:02 -0500 Subject: [PATCH 06/17] Allowlist atomGithub.test setting for global reads and writes --- lib/git-shell-out-strategy.js | 6 +++--- lib/models/repository-states/state.js | 2 +- test/models/repository.test.js | 1 - 3 files changed, 4 insertions(+), 5 deletions(-) diff --git a/lib/git-shell-out-strategy.js b/lib/git-shell-out-strategy.js index d61217b7a7..4d1b7827ce 100644 --- a/lib/git-shell-out-strategy.js +++ b/lib/git-shell-out-strategy.js @@ -1003,11 +1003,11 @@ export default class GitShellOutStrategy { return (await this.exec(['describe', '--contains', '--all', '--always', 'HEAD'])).trim(); } - async getConfig(option, {local, direct} = {}) { + async getConfig(option, {local} = {}) { let output; try { let args = ['config']; - if (local || (!direct && atom.inSpecMode())) { args.push('--local'); } + if (local || (atom.inSpecMode() && option !== 'atomGithub.test')) { args.push('--local'); } args = args.concat(option); output = await this.exec(args); } catch (err) { @@ -1025,7 +1025,7 @@ export default class GitShellOutStrategy { setConfig(option, value, {replaceAll, global} = {}) { let args = ['config']; if (replaceAll) { args.push('--replace-all'); } - if (global && !atom.inSpecMode()) { args.push('--global'); } + if (global && (!atom.inSpecMode() || option === 'atomGithub.test')) { args.push('--global'); } args = args.concat(option, value); return this.exec(args, {writeOperation: true}); } diff --git a/lib/models/repository-states/state.js b/lib/models/repository-states/state.js index 5a03e17850..e284b9b28e 100644 --- a/lib/models/repository-states/state.js +++ b/lib/models/repository-states/state.js @@ -368,7 +368,7 @@ export default class State { } getConfig(optionName, options) { - return this.workdirlessGit().getConfig(optionName, {direct: true, ...options}); + return this.workdirlessGit().getConfig(optionName, options); } // Direct blob access diff --git a/test/models/repository.test.js b/test/models/repository.test.js index 17807756a9..510c9774be 100644 --- a/test/models/repository.test.js +++ b/test/models/repository.test.js @@ -140,7 +140,6 @@ describe('Repository', function() { }); it('works anyway', async function() { - sinon.stub(atom, 'inSpecMode').returns(false); await repository.setConfig('atomGithub.test', 'yes', {global: true}); assert.strictEqual(await repository.getConfig('atomGithub.test'), 'yes'); }); From 9adff71c70545b71b108bb4c820aab1ead8ae6a0 Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Fri, 20 Nov 2020 11:20:35 -0500 Subject: [PATCH 07/17] Refactor cache keys into its own module --- lib/models/repository-states/cache/keys.js | 164 ++++++++++++++++++++ lib/models/repository-states/present.js | 166 +-------------------- 2 files changed, 165 insertions(+), 165 deletions(-) create mode 100644 lib/models/repository-states/cache/keys.js diff --git a/lib/models/repository-states/cache/keys.js b/lib/models/repository-states/cache/keys.js new file mode 100644 index 0000000000..1a58bfbdd0 --- /dev/null +++ b/lib/models/repository-states/cache/keys.js @@ -0,0 +1,164 @@ +class CacheKey { + constructor(primary, groups = []) { + this.primary = primary; + this.groups = groups; + } + + getPrimary() { + return this.primary; + } + + getGroups() { + return this.groups; + } + + removeFromCache(cache, withoutGroup = null) { + cache.removePrimary(this.getPrimary()); + + const groups = this.getGroups(); + for (let i = 0; i < groups.length; i++) { + const group = groups[i]; + if (group === withoutGroup) { + continue; + } + + cache.removeFromGroup(group, this); + } + } + + /* istanbul ignore next */ + toString() { + return `CacheKey(${this.primary})`; + } +} + +class GroupKey { + constructor(group) { + this.group = group; + } + + removeFromCache(cache) { + for (const matchingKey of cache.keysInGroup(this.group)) { + matchingKey.removeFromCache(cache, this.group); + } + } + + /* istanbul ignore next */ + toString() { + return `GroupKey(${this.group})`; + } +} + +export const Keys = { + statusBundle: new CacheKey('status-bundle'), + + stagedChanges: new CacheKey('staged-changes'), + + filePatch: { + _optKey: ({staged}) => (staged ? 's' : 'u'), + + oneWith: (fileName, options) => { // <-- Keys.filePatch + const optKey = Keys.filePatch._optKey(options); + const baseCommit = options.baseCommit || 'head'; + + const extraGroups = []; + if (options.baseCommit) { + extraGroups.push(`file-patch:base-nonhead:path-${fileName}`); + extraGroups.push('file-patch:base-nonhead'); + } else { + extraGroups.push('file-patch:base-head'); + } + + return new CacheKey(`file-patch:${optKey}:${baseCommit}:${fileName}`, [ + 'file-patch', + `file-patch:opt-${optKey}`, + `file-patch:opt-${optKey}:path-${fileName}`, + ...extraGroups, + ]); + }, + + eachWithFileOpts: (fileNames, opts) => { + const keys = []; + for (let i = 0; i < fileNames.length; i++) { + for (let j = 0; j < opts.length; j++) { + keys.push(new GroupKey(`file-patch:opt-${Keys.filePatch._optKey(opts[j])}:path-${fileNames[i]}`)); + } + } + return keys; + }, + + eachNonHeadWithFiles: fileNames => { + return fileNames.map(fileName => new GroupKey(`file-patch:base-nonhead:path-${fileName}`)); + }, + + allAgainstNonHead: new GroupKey('file-patch:base-nonhead'), + + eachWithOpts: (...opts) => opts.map(opt => new GroupKey(`file-patch:opt-${Keys.filePatch._optKey(opt)}`)), + + all: new GroupKey('file-patch'), + }, + + index: { + oneWith: fileName => new CacheKey(`index:${fileName}`, ['index']), + + all: new GroupKey('index'), + }, + + lastCommit: new CacheKey('last-commit'), + + recentCommits: new CacheKey('recent-commits'), + + authors: new CacheKey('authors'), + + branches: new CacheKey('branches'), + + headDescription: new CacheKey('head-description'), + + remotes: new CacheKey('remotes'), + + config: { + _optKey: options => (options.local ? 'l' : ''), + + oneWith: (setting, options) => { + const optKey = Keys.config._optKey(options); + return new CacheKey(`config:${optKey}:${setting}`, ['config', `config:${optKey}`]); + }, + + eachWithSetting: setting => [ + Keys.config.oneWith(setting, {local: true}), + Keys.config.oneWith(setting, {local: false}), + ], + + all: new GroupKey('config'), + }, + + blob: { + oneWith: sha => new CacheKey(`blob:${sha}`, ['blob']), + }, + + // Common collections of keys and patterns for use with invalidate(). + + workdirOperationKeys: fileNames => [ + Keys.statusBundle, + ...Keys.filePatch.eachWithFileOpts(fileNames, [{staged: false}]), + ], + + cacheOperationKeys: fileNames => [ + ...Keys.workdirOperationKeys(fileNames), + ...Keys.filePatch.eachWithFileOpts(fileNames, [{staged: true}]), + ...fileNames.map(Keys.index.oneWith), + Keys.stagedChanges, + ], + + headOperationKeys: () => [ + Keys.headDescription, + Keys.branches, + ...Keys.filePatch.eachWithOpts({staged: true}), + Keys.filePatch.allAgainstNonHead, + Keys.stagedChanges, + Keys.lastCommit, + Keys.recentCommits, + Keys.authors, + Keys.statusBundle, + ], +}; diff --git a/lib/models/repository-states/present.js b/lib/models/repository-states/present.js index 4f8414ee6c..a8faf87f79 100644 --- a/lib/models/repository-states/present.js +++ b/lib/models/repository-states/present.js @@ -3,6 +3,7 @@ import {Emitter} from 'event-kit'; import fs from 'fs-extra'; import State from './state'; +import {Keys} from './cache/keys'; import {LargeRepoError} from '../../git-shell-out-strategy'; import {FOCUS} from '../workspace-change-observer'; @@ -1048,168 +1049,3 @@ class Cache { this.emitter.dispose(); } } - -class CacheKey { - constructor(primary, groups = []) { - this.primary = primary; - this.groups = groups; - } - - getPrimary() { - return this.primary; - } - - getGroups() { - return this.groups; - } - - removeFromCache(cache, withoutGroup = null) { - cache.removePrimary(this.getPrimary()); - - const groups = this.getGroups(); - for (let i = 0; i < groups.length; i++) { - const group = groups[i]; - if (group === withoutGroup) { - continue; - } - - cache.removeFromGroup(group, this); - } - } - - /* istanbul ignore next */ - toString() { - return `CacheKey(${this.primary})`; - } -} - -class GroupKey { - constructor(group) { - this.group = group; - } - - removeFromCache(cache) { - for (const matchingKey of cache.keysInGroup(this.group)) { - matchingKey.removeFromCache(cache, this.group); - } - } - - /* istanbul ignore next */ - toString() { - return `GroupKey(${this.group})`; - } -} - -const Keys = { - statusBundle: new CacheKey('status-bundle'), - - stagedChanges: new CacheKey('staged-changes'), - - filePatch: { - _optKey: ({staged}) => (staged ? 's' : 'u'), - - oneWith: (fileName, options) => { // <-- Keys.filePatch - const optKey = Keys.filePatch._optKey(options); - const baseCommit = options.baseCommit || 'head'; - - const extraGroups = []; - if (options.baseCommit) { - extraGroups.push(`file-patch:base-nonhead:path-${fileName}`); - extraGroups.push('file-patch:base-nonhead'); - } else { - extraGroups.push('file-patch:base-head'); - } - - return new CacheKey(`file-patch:${optKey}:${baseCommit}:${fileName}`, [ - 'file-patch', - `file-patch:opt-${optKey}`, - `file-patch:opt-${optKey}:path-${fileName}`, - ...extraGroups, - ]); - }, - - eachWithFileOpts: (fileNames, opts) => { - const keys = []; - for (let i = 0; i < fileNames.length; i++) { - for (let j = 0; j < opts.length; j++) { - keys.push(new GroupKey(`file-patch:opt-${Keys.filePatch._optKey(opts[j])}:path-${fileNames[i]}`)); - } - } - return keys; - }, - - eachNonHeadWithFiles: fileNames => { - return fileNames.map(fileName => new GroupKey(`file-patch:base-nonhead:path-${fileName}`)); - }, - - allAgainstNonHead: new GroupKey('file-patch:base-nonhead'), - - eachWithOpts: (...opts) => opts.map(opt => new GroupKey(`file-patch:opt-${Keys.filePatch._optKey(opt)}`)), - - all: new GroupKey('file-patch'), - }, - - index: { - oneWith: fileName => new CacheKey(`index:${fileName}`, ['index']), - - all: new GroupKey('index'), - }, - - lastCommit: new CacheKey('last-commit'), - - recentCommits: new CacheKey('recent-commits'), - - authors: new CacheKey('authors'), - - branches: new CacheKey('branches'), - - headDescription: new CacheKey('head-description'), - - remotes: new CacheKey('remotes'), - - config: { - _optKey: options => (options.local ? 'l' : ''), - - oneWith: (setting, options) => { - const optKey = Keys.config._optKey(options); - return new CacheKey(`config:${optKey}:${setting}`, ['config', `config:${optKey}`]); - }, - - eachWithSetting: setting => [ - Keys.config.oneWith(setting, {local: true}), - Keys.config.oneWith(setting, {local: false}), - ], - - all: new GroupKey('config'), - }, - - blob: { - oneWith: sha => new CacheKey(`blob:${sha}`, ['blob']), - }, - - // Common collections of keys and patterns for use with invalidate(). - - workdirOperationKeys: fileNames => [ - Keys.statusBundle, - ...Keys.filePatch.eachWithFileOpts(fileNames, [{staged: false}]), - ], - - cacheOperationKeys: fileNames => [ - ...Keys.workdirOperationKeys(fileNames), - ...Keys.filePatch.eachWithFileOpts(fileNames, [{staged: true}]), - ...fileNames.map(Keys.index.oneWith), - Keys.stagedChanges, - ], - - headOperationKeys: () => [ - Keys.headDescription, - Keys.branches, - ...Keys.filePatch.eachWithOpts({staged: true}), - Keys.filePatch.allAgainstNonHead, - Keys.stagedChanges, - Keys.lastCommit, - Keys.recentCommits, - Keys.authors, - Keys.statusBundle, - ], -}; From 2ae9d8ed90b133e4bbcf45bfb73fc5d6e0e2a77d Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Fri, 20 Nov 2020 13:12:23 -0500 Subject: [PATCH 08/17] Test for cross-instance cache invalidation --- test/models/workdir-context-pool.test.js | 28 ++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/test/models/workdir-context-pool.test.js b/test/models/workdir-context-pool.test.js index b5b12a22e5..6f6b988c92 100644 --- a/test/models/workdir-context-pool.test.js +++ b/test/models/workdir-context-pool.test.js @@ -368,4 +368,32 @@ describe('WorkdirContextPool', function() { }); }); }); + + describe('cross-instance cache invalidation', function() { + it('invalidates a config cache key in different instances when a global setting is changed', async function() { + const value = String(Date.now()); + + const repo0 = pool.add(await cloneRepository('three-files')).getRepository(); + const repo1 = pool.add(await cloneRepository('three-files')).getRepository(); + const repo2 = pool.add(temp.mkdirSync()).getRepository(); + + await Promise.all([repo0, repo1, repo2].map(repo => repo.getLoadPromise())); + + const [before0, before1, before2] = await Promise.all( + [repo0, repo1, repo2].map(repo => repo.getConfig('atomGithub.test')), + ); + + assert.notInclude([before0, before1, before2], value); + + await repo2.setConfig('atomGithub.test', value, {global: true}); + + const [after0, after1, after2] = await Promise.all( + [repo0, repo1, repo2].map(repo => repo.getConfig('atomGithub.test')), + ); + + assert.strictEqual(after0, value); + assert.strictEqual(after1, value); + assert.strictEqual(after2, value); + }); + }); }); From 5370d5674df6a592c1ca5bb1c75b87023d63db91 Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Fri, 20 Nov 2020 13:12:59 -0500 Subject: [PATCH 09/17] Expose acceptInvalidation from a repository's state --- lib/models/repository-states/state.js | 4 ++++ lib/models/repository.js | 1 + 2 files changed, 5 insertions(+) diff --git a/lib/models/repository-states/state.js b/lib/models/repository-states/state.js index e284b9b28e..a9a687db9a 100644 --- a/lib/models/repository-states/state.js +++ b/lib/models/repository-states/state.js @@ -415,6 +415,10 @@ export default class State { return null; } + acceptInvalidation() { + return null; + } + // Internal ////////////////////////////////////////////////////////////////////////////////////////////////////////// // Non-delegated methods that provide subclasses with convenient access to Repository properties. diff --git a/lib/models/repository.js b/lib/models/repository.js index e46a7d238c..c2e22a22e9 100644 --- a/lib/models/repository.js +++ b/lib/models/repository.js @@ -372,6 +372,7 @@ const delegates = [ 'getCommitMessage', 'fetchCommitMessageTemplate', 'getCache', + 'acceptInvalidation', ]; for (let i = 0; i < delegates.length; i++) { From 7a035659a9b2764268dae7e192b6031161f79d88 Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Fri, 20 Nov 2020 13:13:27 -0500 Subject: [PATCH 10/17] Emit a Repository event for global invalidations --- lib/models/repository-states/state.js | 4 ++++ lib/models/repository.js | 4 ++++ 2 files changed, 8 insertions(+) diff --git a/lib/models/repository-states/state.js b/lib/models/repository-states/state.js index a9a687db9a..64e9ab4370 100644 --- a/lib/models/repository-states/state.js +++ b/lib/models/repository-states/state.js @@ -474,6 +474,10 @@ export default class State { return this.repository.emitter.emit('did-update'); } + didGloballyInvalidate(spec) { + return this.repository.emitter.emit('did-globally-invalidate', spec); + } + // Direct git access // Non-delegated git operations for internal use within states. diff --git a/lib/models/repository.js b/lib/models/repository.js index c2e22a22e9..a34ab6a7c3 100644 --- a/lib/models/repository.js +++ b/lib/models/repository.js @@ -108,6 +108,10 @@ export default class Repository { return this.emitter.on('did-update', callback); } + onDidGloballyInvalidate(callback) { + return this.emitter.on('did-globally-invalidate', callback); + } + onPullError(callback) { return this.emitter.on('pull-error', callback); } From 6426c2b4e29d865fe3650224558d69113907d2d5 Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Fri, 20 Nov 2020 13:14:06 -0500 Subject: [PATCH 11/17] Broadcast global invalidations on setConfig for global values --- lib/models/repository-states/present.js | 12 ++++++++---- lib/models/repository-states/state.js | 4 ++++ 2 files changed, 12 insertions(+), 4 deletions(-) diff --git a/lib/models/repository-states/present.js b/lib/models/repository-states/present.js index a8faf87f79..dc7efc9f79 100644 --- a/lib/models/repository-states/present.js +++ b/lib/models/repository-states/present.js @@ -103,9 +103,12 @@ export default class Present extends State { return true; } - acceptInvalidation(spec) { + acceptInvalidation(spec, {globally} = {}) { this.cache.invalidate(spec()); this.didUpdate(); + if (globally) { + this.didGloballyInvalidate(spec); + } } invalidateCacheAfterFilesystemChange(events) { @@ -524,6 +527,7 @@ export default class Present extends State { return this.invalidate( () => Keys.config.eachWithSetting(setting), () => this.git().setConfig(setting, value, options), + {globally: options.global}, ); } @@ -931,14 +935,14 @@ export default class Present extends State { return this.cache; } - invalidate(spec, body) { + invalidate(spec, body, options = {}) { return body().then( result => { - this.acceptInvalidation(spec); + this.acceptInvalidation(spec, options); return result; }, err => { - this.acceptInvalidation(spec); + this.acceptInvalidation(spec, options); return Promise.reject(err); }, ); diff --git a/lib/models/repository-states/state.js b/lib/models/repository-states/state.js index 64e9ab4370..89fc855b89 100644 --- a/lib/models/repository-states/state.js +++ b/lib/models/repository-states/state.js @@ -5,6 +5,7 @@ import RemoteSet from '../remote-set'; import {nullOperationStates} from '../operation-states'; import MultiFilePatch from '../patch/multi-file-patch'; import CompositeGitStrategy from '../../composite-git-strategy'; +import {Keys} from './cache/keys'; /** * Map of registered subclasses to allow states to transition to one another without circular dependencies. @@ -215,6 +216,9 @@ export default class State { async setConfig(optionName, value, options) { await this.workdirlessGit().setConfig(optionName, value, options); this.didUpdate(); + if (options.global) { + this.didGloballyInvalidate(() => Keys.config.eachWithSetting(optionName)); + } } unsetConfig(option) { From 2f979bd2a89b2414f2ba1a0bc039e584b9fbec66 Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Fri, 20 Nov 2020 13:18:46 -0500 Subject: [PATCH 12/17] Wire cache invalidations among pool members --- lib/models/workdir-context-pool.js | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/lib/models/workdir-context-pool.js b/lib/models/workdir-context-pool.js index 9bb2f1d891..0ee6e88e4e 100644 --- a/lib/models/workdir-context-pool.js +++ b/lib/models/workdir-context-pool.js @@ -67,6 +67,15 @@ export default class WorkdirContextPool { forwardEvent('onDidUpdateRepository', 'did-update-repository'); forwardEvent('onDidDestroyRepository', 'did-destroy-repository'); + // Propagate global cache invalidations across all resident contexts + disposable.add(context.getRepository().onDidGloballyInvalidate(spec => { + this.withResidentContexts((_workdir, eachContext) => { + if (eachContext !== context) { + eachContext.getRepository().acceptInvalidation(spec); + } + }); + })); + if (!silenceEmitter) { this.emitter.emit('did-change-contexts', {added: new Set([directory])}); } From cfc7340a6646bc5c46222b2358f8ed4ff8157fd7 Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Fri, 20 Nov 2020 13:23:41 -0500 Subject: [PATCH 13/17] Exercise the Present repository codepath --- test/models/workdir-context-pool.test.js | 24 ++++++++++++++++++------ 1 file changed, 18 insertions(+), 6 deletions(-) diff --git a/test/models/workdir-context-pool.test.js b/test/models/workdir-context-pool.test.js index 6f6b988c92..d29c824af4 100644 --- a/test/models/workdir-context-pool.test.js +++ b/test/models/workdir-context-pool.test.js @@ -371,7 +371,9 @@ describe('WorkdirContextPool', function() { describe('cross-instance cache invalidation', function() { it('invalidates a config cache key in different instances when a global setting is changed', async function() { - const value = String(Date.now()); + const base = String(Date.now()); + const value0 = `${base}-0`; + const value1 = `${base}-1`; const repo0 = pool.add(await cloneRepository('three-files')).getRepository(); const repo1 = pool.add(await cloneRepository('three-files')).getRepository(); @@ -383,17 +385,27 @@ describe('WorkdirContextPool', function() { [repo0, repo1, repo2].map(repo => repo.getConfig('atomGithub.test')), ); - assert.notInclude([before0, before1, before2], value); + assert.notInclude([before0, before1, before2], value0); - await repo2.setConfig('atomGithub.test', value, {global: true}); + await repo2.setConfig('atomGithub.test', value0, {global: true}); const [after0, after1, after2] = await Promise.all( [repo0, repo1, repo2].map(repo => repo.getConfig('atomGithub.test')), ); - assert.strictEqual(after0, value); - assert.strictEqual(after1, value); - assert.strictEqual(after2, value); + assert.strictEqual(after0, value0); + assert.strictEqual(after1, value0); + assert.strictEqual(after2, value0); + + await repo0.setConfig('atomGithub.test', value1, {global: true}); + + const [final0, final1, final2] = await Promise.all( + [repo0, repo1, repo2].map(repo => repo.getConfig('atomGithub.test')), + ); + + assert.strictEqual(final0, value1); + assert.strictEqual(final1, value1); + assert.strictEqual(final2, value1); }); }); }); From 9253fc7e898fd0e763295b4e9f4ef098df1768dc Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Fri, 20 Nov 2020 13:37:46 -0500 Subject: [PATCH 14/17] Provide a default for options --- lib/models/repository-states/present.js | 2 +- lib/models/repository-states/state.js | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/models/repository-states/present.js b/lib/models/repository-states/present.js index dc7efc9f79..0c0a9d7a09 100644 --- a/lib/models/repository-states/present.js +++ b/lib/models/repository-states/present.js @@ -523,7 +523,7 @@ export default class Present extends State { // Configuration - setConfig(setting, value, options) { + setConfig(setting, value, options = {}) { return this.invalidate( () => Keys.config.eachWithSetting(setting), () => this.git().setConfig(setting, value, options), diff --git a/lib/models/repository-states/state.js b/lib/models/repository-states/state.js index 89fc855b89..c84ecad56a 100644 --- a/lib/models/repository-states/state.js +++ b/lib/models/repository-states/state.js @@ -213,7 +213,7 @@ export default class State { // Configuration - async setConfig(optionName, value, options) { + async setConfig(optionName, value, options = {}) { await this.workdirlessGit().setConfig(optionName, value, options); this.didUpdate(); if (options.global) { From d7ce6f568a75d95ad9bfc8aa08be3f6ee172d2ab Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Fri, 20 Nov 2020 14:24:53 -0500 Subject: [PATCH 15/17] Stub inSpecMode for integration tests --- test/integration/helpers.js | 1 + 1 file changed, 1 insertion(+) diff --git a/test/integration/helpers.js b/test/integration/helpers.js index 4bbce8fa8b..251961a4cb 100644 --- a/test/integration/helpers.js +++ b/test/integration/helpers.js @@ -42,6 +42,7 @@ export async function setup(options = {}) { }; const atomEnv = global.buildAtomEnvironment(); + sinon.stub(atom, 'inSpecMode').returns(false); let suiteRoot = document.getElementById('github-IntegrationSuite'); if (!suiteRoot) { From ba5d25818714ed9d00348d30f900aec7d8c289e4 Mon Sep 17 00:00:00 2001 From: Someone Date: Fri, 20 Nov 2020 14:40:43 -0500 Subject: [PATCH 16/17] Take out the special config behavior in spec mode --- lib/git-shell-out-strategy.js | 4 ++-- test/integration/helpers.js | 1 - 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/lib/git-shell-out-strategy.js b/lib/git-shell-out-strategy.js index 4d1b7827ce..b56cdcd641 100644 --- a/lib/git-shell-out-strategy.js +++ b/lib/git-shell-out-strategy.js @@ -1007,7 +1007,7 @@ export default class GitShellOutStrategy { let output; try { let args = ['config']; - if (local || (atom.inSpecMode() && option !== 'atomGithub.test')) { args.push('--local'); } + if (local) { args.push('--local'); } args = args.concat(option); output = await this.exec(args); } catch (err) { @@ -1025,7 +1025,7 @@ export default class GitShellOutStrategy { setConfig(option, value, {replaceAll, global} = {}) { let args = ['config']; if (replaceAll) { args.push('--replace-all'); } - if (global && (!atom.inSpecMode() || option === 'atomGithub.test')) { args.push('--global'); } + if (global) { args.push('--global'); } args = args.concat(option, value); return this.exec(args, {writeOperation: true}); } diff --git a/test/integration/helpers.js b/test/integration/helpers.js index 251961a4cb..4bbce8fa8b 100644 --- a/test/integration/helpers.js +++ b/test/integration/helpers.js @@ -42,7 +42,6 @@ export async function setup(options = {}) { }; const atomEnv = global.buildAtomEnvironment(); - sinon.stub(atom, 'inSpecMode').returns(false); let suiteRoot = document.getElementById('github-IntegrationSuite'); if (!suiteRoot) { From f466251fbed28e023fd5a01a2b50c26fc61d46b7 Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Fri, 20 Nov 2020 15:23:07 -0500 Subject: [PATCH 17/17] Don't set username or email when unchanged --- lib/controllers/git-tab-controller.js | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/lib/controllers/git-tab-controller.js b/lib/controllers/git-tab-controller.js index 60cf360cd3..8a2b36262b 100644 --- a/lib/controllers/git-tab-controller.js +++ b/lib/controllers/git-tab-controller.js @@ -371,9 +371,19 @@ export default class GitTabController extends React.Component { closeIdentityEditor = () => this.setState({editingIdentity: false}) - setUsername = () => this.props.repository.setConfig('user.name', this.usernameBuffer.getText(), {global: true}) + setUsername = () => { + const newUsername = this.usernameBuffer.getText(); + if (newUsername !== this.props.username) { + this.props.repository.setConfig('user.name', newUsername, {global: true}); + } + } - setEmail = () => this.props.repository.setConfig('user.email', this.emailBuffer.getText(), {global: true}) + setEmail = () => { + const newEmail = this.emailBuffer.getText(); + if (newEmail !== this.props.email) { + this.props.repository.setConfig('user.email', newEmail, {global: true}); + } + } restoreFocus() { this.refView.map(view => view.setFocus(this.lastFocus));