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/" 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)); diff --git a/lib/git-shell-out-strategy.js b/lib/git-shell-out-strategy.js index 53c88b8919..b56cdcd641 100644 --- a/lib/git-shell-out-strategy.js +++ b/lib/git-shell-out-strategy.js @@ -1007,12 +1007,12 @@ export default class GitShellOutStrategy { let output; try { let args = ['config']; - if (local || atom.inSpecMode()) { args.push('--local'); } + if (local) { args.push('--local'); } 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; @@ -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) { args.push('--global'); } args = args.concat(option, value); return this.exec(args, {writeOperation: true}); } 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..0c0a9d7a09 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'; @@ -102,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) { @@ -519,10 +523,11 @@ 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), + {globally: options.global}, ); } @@ -930,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); }, ); @@ -1048,168 +1053,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, - ], -}; diff --git a/lib/models/repository-states/state.js b/lib/models/repository-states/state.js index 9152b98e55..c84ecad56a 100644 --- a/lib/models/repository-states/state.js +++ b/lib/models/repository-states/state.js @@ -1,8 +1,11 @@ +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'; +import {Keys} from './cache/keys'; /** * Map of registered subclasses to allow states to transition to one another without circular dependencies. @@ -210,8 +213,12 @@ export default class State { // Configuration - setConfig(option, value, {replaceAll} = {}) { - return unsupportedOperationPromise(this, 'setConfig'); + 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) { @@ -364,8 +371,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 @@ -412,6 +419,10 @@ export default class State { return null; } + acceptInvalidation() { + return null; + } + // Internal ////////////////////////////////////////////////////////////////////////////////////////////////////////// // Non-delegated methods that provide subclasses with convenient access to Repository properties. @@ -467,9 +478,21 @@ 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. + 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/lib/models/repository.js b/lib/models/repository.js index e46a7d238c..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); } @@ -372,6 +376,7 @@ const delegates = [ 'getCommitMessage', 'fetchCommitMessageTemplate', 'getCache', + 'acceptInvalidation', ]; for (let i = 0; i < delegates.length; i++) { 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])}); } diff --git a/test/models/repository.test.js b/test/models/repository.test.js index 3aede68ea6..510c9774be 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,11 @@ describe('Repository', function() { /fatal: Not a valid object name abcd/, ); }); + + it('works anyway', async function() { + await repository.setConfig('atomGithub.test', 'yes', {global: true}); + assert.strictEqual(await repository.getConfig('atomGithub.test'), 'yes'); + }); }); it('accesses an OperationStates model', async function() { @@ -1321,14 +1326,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 +1347,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() { diff --git a/test/models/workdir-context-pool.test.js b/test/models/workdir-context-pool.test.js index b5b12a22e5..d29c824af4 100644 --- a/test/models/workdir-context-pool.test.js +++ b/test/models/workdir-context-pool.test.js @@ -368,4 +368,44 @@ 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 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(); + 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], value0); + + 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, 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); + }); + }); });