Merge pull request #1822 from atom/vy/merge-conflict-pane

Fix: git tab keeps popping open when there's a merge conflict

Author: vanessayuenn

lib/controllers/root-controller.js

@@ -93,7 +93,7 @@ export default class RootController extends React.Component {
     });
 
     this.subscription = new CompositeDisposable(
-      this.props.repository.onMergeError(this.gitTabTracker.ensureVisible),
+      this.props.repository.onPullError(this.gitTabTracker.ensureVisible),
     );
 
     this.props.commandRegistry.onDidDispatch(event => {
@@ -175,7 +175,6 @@ export default class RootController extends React.Component {
           confirm={this.props.confirm}
           toggleGitTab={this.gitTabTracker.toggle}
           toggleGithubTab={this.githubTabTracker.toggle}
-          ensureGitTabVisible={this.gitTabTracker.ensureVisible}
         />
       </StatusBar>
     );
@@ -453,7 +452,7 @@ export default class RootController extends React.Component {
   componentDidUpdate() {
     this.subscription.dispose();
     this.subscription = new CompositeDisposable(
-      this.props.repository.onMergeError(() => this.gitTabTracker.ensureVisible()),
+      this.props.repository.onPullError(() => this.gitTabTracker.ensureVisible()),
     );
   }
 

lib/controllers/status-bar-tile-controller.js

@@ -22,7 +22,6 @@ export default class StatusBarTileController extends React.Component {
     repository: PropTypes.object.isRequired,
     toggleGitTab: PropTypes.func,
     toggleGithubTab: PropTypes.func,
-    ensureGitTabVisible: PropTypes.func,
   }
 
   constructor(props) {
@@ -75,9 +74,6 @@ export default class StatusBarTileController extends React.Component {
       mergeConflictsPresent = Object.keys(data.statusesForChangedFiles.mergeConflictFiles).length > 0;
     }
 
-    if (mergeConflictsPresent) {
-      this.props.ensureGitTabVisible();
-    }
     const repoProps = {
       repository: this.props.repository,
       currentBranch: data.currentBranch,

lib/get-repo-pipeline-manager.js

@@ -72,6 +72,7 @@ export default function({confirm, notificationManager, workspace}) {
       return result;
     } catch (error) {
       if (error instanceof GitError) {
+        repository.didPullError();
         if (/error: Your local changes to the following files would be overwritten by merge/.test(error.stdErr)) {
           const lines = error.stdErr.split('\n');
           const files = lines.slice(3, lines.length - 3).map(l => `\`${l.trim()}\``).join('\n');
@@ -82,7 +83,6 @@ export default function({confirm, notificationManager, workspace}) {
             dismissable: true,
           });
         } else if (/Automatic merge failed; fix conflicts and then commit the result./.test(error.stdOut)) {
-          repository.didMergeError();
           notificationManager.addWarning('Merge conflicts', {
             description: `Your local changes conflicted with changes made on the remote branch. Resolve the conflicts
               with the Git panel and commit to continue.`,

lib/models/repository.js

@@ -108,12 +108,12 @@ export default class Repository {
     return this.emitter.on('did-update', callback);
   }
 
-  onMergeError(callback) {
-    return this.emitter.on('merge-error', callback);
+  onPullError(callback) {
+    return this.emitter.on('pull-error', callback);
   }
 
-  didMergeError() {
-    return this.emitter.emit('merge-error');
+  didPullError() {
+    return this.emitter.emit('pull-error');
   }
 
   // State-independent actions /////////////////////////////////////////////////////////////////////////////////////////

test/controllers/status-bar-tile-controller.test.js

@@ -42,7 +42,6 @@ describe('StatusBarTileController', function() {
         confirm={confirm}
         toggleGitTab={() => {}}
         toggleGithubTab={() => {}}
-        ensureGitTabVisible={() => {}}
         {...props}
       />
     );
@@ -622,14 +621,13 @@ describe('StatusBarTileController', function() {
         await assert.async.isFalse(repository.getOperationStates().isPushInProgress());
       });
 
-
       it('displays a warning notification when pull results in merge conflicts', async function() {
         const {localRepoPath} = await setUpLocalAndRemoteRepositories('multiple-commits', {remoteAhead: true});
         fs.writeFileSync(path.join(localRepoPath, 'file.txt'), 'apple');
         const repository = await buildRepositoryWithPipeline(localRepoPath, {confirm, notificationManager, workspace});
         await repository.git.exec(['commit', '-am', 'Add conflicting change']);
 
-        const wrapper = await mountAndLoad(buildApp({repository, ensureGitTabVisible: sinon.stub()}));
+        const wrapper = await mountAndLoad(buildApp({repository}));
 
         sinon.stub(notificationManager, 'addWarning');
 
@@ -640,8 +638,6 @@ describe('StatusBarTileController', function() {
         }
         repository.refresh();
 
-        await assert.async.isTrue(wrapper.instance().props.ensureGitTabVisible.called);
-
         await assert.async.isTrue(notificationManager.addWarning.called);
         const notificationArgs = notificationManager.addWarning.args[0];
         assert.equal(notificationArgs[0], 'Merge conflicts');

test/get-repo-pipeline-manager.test.js

@@ -0,0 +1,245 @@
+import {cloneRepository, buildRepositoryWithPipeline} from './helpers';
+import {GitError} from '../lib/git-shell-out-strategy';
+
+
+describe('getRepoPipelineManager()', function() {
+
+  let atomEnv, workspace, notificationManager, repo, pipelineManager, confirm;
+
+  const getPipeline = (pm, actionName) => {
+    const actionKey = pm.actionKeys[actionName];
+    return pm.getPipeline(actionKey);
+  };
+
+  const buildRepo = (workdir, override = {}) => {
+    const option = {
+      confirm,
+      notificationManager,
+      workspace,
+      ...override,
+    };
+    return buildRepositoryWithPipeline(workdir, option);
+  };
+
+  const gitErrorStub = (stdErr = '', stdOut = '') => {
+    return sinon.stub().throws(() => {
+      const err = new GitError();
+      err.stdErr = stdErr;
+      err.stdOut = stdOut;
+      return err;
+    });
+  };
+
+  beforeEach(async function() {
+    atomEnv = global.buildAtomEnvironment();
+    workspace = atomEnv.workspace;
+    notificationManager = atomEnv.notifications;
+    confirm = sinon.stub(atomEnv, 'confirm');
+
+    const workdir = await cloneRepository('multiple-commits');
+    repo = await buildRepo(workdir);
+    pipelineManager = repo.pipelineManager;
+  });
+
+  afterEach(function() {
+    atomEnv.destroy();
+  });
+
+  it('has all the action pipelines', function() {
+    const expectedActions = ['PUSH', 'PULL', 'FETCH', 'COMMIT', 'CHECKOUT', 'ADDREMOTE'];
+    for (const actionName of expectedActions) {
+      assert.ok(getPipeline(pipelineManager, actionName));
+    }
+  });
+
+  describe('PUSH pipeline', function() {
+
+    it('confirm-force-push', function() {
+      it('before confirming', function() {
+        const pushPipeline = getPipeline(pipelineManager, 'PUSH');
+        const pushStub = sinon.stub();
+        sinon.spy(notificationManager, 'addError');
+        pushPipeline.run(pushStub, repo, '', {force: true});
+        assert.isTrue(confirm.calledWith({
+          message: 'Are you sure you want to force push?',
+          detailedMessage: 'This operation could result in losing data on the remote.',
+          buttons: ['Force Push', 'Cancel'],
+        }));
+        assert.isTrue(pushStub.called());
+      });
+
+      it('after confirming', async function() {
+        const nWorkdir = await cloneRepository('multiple-commits');
+        const confirmStub = sinon.stub(atomEnv, 'confirm').return(0);
+        const nRepo = buildRepo(nWorkdir, {confirm: confirmStub});
+        const pushPipeline = getPipeline(nRepo.pipelineManager, 'PUSH');
+        const pushStub = sinon.stub();
+        sinon.spy(notificationManager, 'addError');
+
+        pushPipeline.run(pushStub, repo, '', {force: true});
+        assert.isFalse(confirm.called);
+        assert.isFalse(pushStub.called);
+      });
+    });
+
+    it('set-push-in-progress', async function() {
+      const pushPipeline = getPipeline(pipelineManager, 'PUSH');
+      const pushStub = sinon.stub().callsFake(() => {
+        assert.isTrue(repo.getOperationStates().isPushInProgress());
+        return Promise.resolve();
+      });
+      pushPipeline.run(pushStub, repo, '', {});
+      assert.isTrue(pushStub.called);
+      await assert.async.isFalse(repo.getOperationStates().isPushInProgress());
+    });
+
+    it('failed-to-push-error', function() {
+      const pushPipeline = getPipeline(pipelineManager, 'PUSH');
+      sinon.spy(notificationManager, 'addError');
+
+
+      pushPipeline.run(gitErrorStub('rejected failed to push'), repo, '', {});
+      assert.isTrue(notificationManager.addError.calledWithMatch('Push rejected', {dismissable: true}));
+
+      pushPipeline.run(gitErrorStub('something else'), repo, '', {});
+      assert.isTrue(notificationManager.addError.calledWithMatch('Unable to push', {dismissable: true}));
+    });
+  });
+
+  describe('PULL pipeline', function() {
+    it('set-pull-in-progress', async function() {
+      const pull = getPipeline(pipelineManager, 'PULL');
+      const pullStub = sinon.stub().callsFake(() => {
+        assert.isTrue(repo.getOperationStates().isPullInProgress());
+        return Promise.resolve();
+      });
+      pull.run(pullStub, repo, '', {});
+      assert.isTrue(pullStub.called);
+      await assert.async.isFalse(repo.getOperationStates().isPullInProgress());
+    });
+
+    it('failed-to-pull-error', function() {
+      const pullPipeline = getPipeline(pipelineManager, 'PULL');
+      sinon.spy(notificationManager, 'addError');
+      sinon.spy(notificationManager, 'addWarning');
+
+      pullPipeline.run(gitErrorStub('error: Your local changes to the following files would be overwritten by merge:\n\ta.txt\n\tb.txt'), repo, '', {});
+      assert.isTrue(notificationManager.addError.calledWithMatch('Pull aborted', {dismissable: true}));
+
+      pullPipeline.run(gitErrorStub('', 'Automatic merge failed; fix conflicts and then commit the result.'), repo, '', {});
+      assert.isTrue(notificationManager.addWarning.calledWithMatch('Merge conflicts', {dismissable: true}));
+
+      pullPipeline.run(gitErrorStub('fatal: Not possible to fast-forward, aborting.'), repo, '', {});
+      assert.isTrue(notificationManager.addWarning.calledWithMatch('Unmerged changes', {dismissable: true}));
+
+      pullPipeline.run(gitErrorStub('something else'), repo, '', {});
+      assert.isTrue(notificationManager.addError.calledWithMatch('Unable to pull', {dismissable: true}));
+    });
+  });
+
+  describe('FETCH pipeline', function() {
+    let fetchPipeline;
+
+    beforeEach(function() {
+      fetchPipeline = getPipeline(pipelineManager, 'FETCH');
+    });
+
+    it('set-fetch-in-progress', async function() {
+      const fetchStub = sinon.stub().callsFake(() => {
+        assert.isTrue(repo.getOperationStates().isFetchInProgress());
+        return Promise.resolve();
+      });
+      fetchPipeline.run(fetchStub, repo, '', {});
+      assert.isTrue(fetchStub.called);
+      await assert.async.isFalse(repo.getOperationStates().isFetchInProgress());
+    });
+
+    it('failed-to-fetch-error', function() {
+      sinon.spy(notificationManager, 'addError');
+
+      fetchPipeline.run(gitErrorStub('this is a nice error msg'), repo, '', {});
+      assert.isTrue(notificationManager.addError.calledWithMatch('Unable to fetch', {
+        detail: 'this is a nice error msg',
+        dismissable: true,
+      }));
+    });
+  });
+
+  describe('CHECKOUT pipeline', function() {
+    let checkoutPipeline;
+
+    beforeEach(function() {
+      checkoutPipeline = getPipeline(pipelineManager, 'CHECKOUT');
+    });
+
+    it('set-checkout-in-progress', async function() {
+      const checkoutStub = sinon.stub().callsFake(() => {
+        assert.isTrue(repo.getOperationStates().isCheckoutInProgress());
+        return Promise.resolve();
+      });
+      checkoutPipeline.run(checkoutStub, repo, '', {});
+      assert.isTrue(checkoutStub.called);
+      await assert.async.isFalse(repo.getOperationStates().isCheckoutInProgress());
+    });
+
+    it('failed-to-checkout-error', function() {
+      sinon.spy(notificationManager, 'addError');
+
+      checkoutPipeline.run(gitErrorStub('local changes would be overwritten: \n\ta.txt\n\tb.txt'), repo, '', {createNew: false});
+      assert.isTrue(notificationManager.addError.calledWithMatch('Checkout aborted', {dismissable: true}));
+
+      checkoutPipeline.run(gitErrorStub('branch x already exists'), repo, '', {createNew: false});
+      assert.isTrue(notificationManager.addError.calledWithMatch('Checkout aborted', {dismissable: true}));
+
+      checkoutPipeline.run(gitErrorStub('error: you need to resolve your current index first'), repo, '', {createNew: false});
+      assert.isTrue(notificationManager.addError.calledWithMatch('Checkout aborted', {dismissable: true}));
+
+      checkoutPipeline.run(gitErrorStub('something else'), repo, '', {createNew: true});
+      assert.isTrue(notificationManager.addError.calledWithMatch('Cannot create branch', {detail: 'something else', dismissable: true}));
+    });
+  });
+
+  describe('COMMIT pipeline', function() {
+    let commitPipeline;
+
+    beforeEach(function() {
+      commitPipeline = getPipeline(pipelineManager, 'COMMIT');
+    });
+
+    it('set-commit-in-progress', async function() {
+      const commitStub = sinon.stub().callsFake(() => {
+        assert.isTrue(repo.getOperationStates().isCommitInProgress());
+        return Promise.resolve();
+      });
+      commitPipeline.run(commitStub, repo, '', {});
+      assert.isTrue(commitStub.called);
+      await assert.async.isFalse(repo.getOperationStates().isCommitInProgress());
+    });
+
+    it('failed-to-commit-error', function() {
+      sinon.spy(notificationManager, 'addError');
+
+      commitPipeline.run(gitErrorStub('a nice msg'), repo, '', {});
+      assert.isTrue(notificationManager.addError.calledWithMatch('Unable to commit', {detail: 'a nice msg', dismissable: true}));
+    });
+  });
+
+  describe('ADDREMOTE pipeline', function() {
+    it('failed-to-add-remote', function() {
+      const addRemotePipeline = getPipeline(pipelineManager, 'ADDREMOTE');
+      sinon.spy(notificationManager, 'addError');
+
+      addRemotePipeline.run(gitErrorStub('fatal: remote x already exists.'), repo, 'existential-crisis');
+      assert.isTrue(notificationManager.addError.calledWithMatch('Cannot create remote', {
+        detail: 'The repository already contains a remote named existential-crisis.',
+        dismissable: true,
+      }));
+
+      addRemotePipeline.run(gitErrorStub('something else'), repo, 'remotename');
+      assert.isTrue(notificationManager.addError.calledWithMatch('Cannot create remote', {
+        detail: 'something else',
+        dismissable: true,
+      }));
+    });
+  });
+});