New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 21 commits into from Dec 11, 2018

Conversation

4 participants
@vanessayuenn
Copy link
Contributor

vanessayuenn commented Dec 4, 2018

Description of the Change

This PR fixes the bug where the git tab keeps opening itself when a merge conflict is detected and remains unfixed. Instead of doing that, I moved the error signaling up one level, so now all git errors resulted from a pull action will trigger an error pop up, and git tab will open itself (one time only).

  • I think this might be an opportunity to increase test coverage for get-repo-pipeline-manager.js

Alternate Designs

N/A

Benefits

Users won't get annoyed at git tab opening itself anymore. People don't like it when robots act smarter than them, y'know.

Applicable Issues

Resolves #1816

Metrics

N/A

Tests

See above

@vanessayuenn vanessayuenn changed the title Fix: git tab keeps popping open when there's a merge conflict [wip] Fix: git tab keeps popping open when there's a merge conflict Dec 4, 2018

@coveralls

This comment has been minimized.

Copy link

coveralls commented Dec 4, 2018

Coverage Status

Coverage increased (+0.4%) to 85.952% when pulling ec8dbe7 on vy/merge-conflict-pane into 0bf8106 on master.

@vanessayuenn vanessayuenn changed the title [wip] Fix: git tab keeps popping open when there's a merge conflict Fix: git tab keeps popping open when there's a merge conflict Dec 5, 2018

Stability Sprint : 20 November 2018 - 8 January 2019 : v0.24.0 automation moved this from In Progress 🔧 to QA Review 🔬 Dec 6, 2018

@smashwilson
Copy link
Member

smashwilson left a comment

Nice I had one question but no other suggestions 😄

@@ -72,6 +72,7 @@ export default function({confirm, notificationManager, workspace}) {
return result;
} catch (error) {
if (error instanceof GitError) {
repository.didPullError();

This comment has been minimized.

@smashwilson

smashwilson Dec 6, 2018

Member

What's the reason for generalizing this from specifically merge errors to any pull errors?

import {GitError} from '../lib/git-shell-out-strategy';


describe('getRepoPipelineManager()', function() {

This comment has been minimized.

@smashwilson

smashwilson Dec 6, 2018

Member

Thank you for doing this 🙏

Go go gadget test coverage 📈

@@ -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()),

This comment has been minimized.

@smashwilson
@smashwilson

This comment has been minimized.

Copy link
Member

smashwilson commented Dec 9, 2018

:shipit: :shipit: :shipit:

@vanessayuenn

This comment has been minimized.

Copy link
Contributor

vanessayuenn commented Dec 11, 2018

@smashwilson when I was testing it, I have found the merge error check we have does not capture all cases where merge conflicts arise (e.g. in my case it's because I have set my pull to use rebase automatically). And looking through the other types of pull errors, it makes sense to have the git tab pulled open to address those errors. If this gets too much, I am totally down to revert the behavior back to only be triggered by merge errors.

@vanessayuenn vanessayuenn merged commit b1f5c03 into master Dec 11, 2018

2 checks passed

codecov/patch 100% of diff hit (target 89.52%)
Details
codecov/project 89.79% (+0.26%) compared to 2246691
Details

Stability Sprint : 20 November 2018 - 8 January 2019 : v0.24.0 automation moved this from QA Review 🔬 to Merged ☑️ Dec 11, 2018

@vanessayuenn vanessayuenn deleted the vy/merge-conflict-pane branch Dec 11, 2018

@smashwilson

This comment has been minimized.

Copy link
Member

smashwilson commented Dec 11, 2018

Ohh autorebase trips it up, huh. Good call 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment