Display warning notification when pull results in merge conflicts

[kuychaco]

Fixes #821

I started implementing a fix for #821 that involves using a warning notification rather than an error notification when there are merge conflicts as a result of a pull. Here's what it looks like:

I'm thinking that a notification might not be that helpful, and that it might be more of an annoyance. In my opinion it's pretty clear what's happening based on the fact that merge conflicts appear and the commit box is populated with a message:

Merge branch 'master' of https://github.com/owner/repo

# Conflicts:
#	file.txt

Another alternative I'm considering is simply removing the notification.

[simurai]

Another alternative I'm considering is simply removing the notification.

• I think that would be ok if the Git panel is already opened
• But if the Git panel is closed, show the notification. Otherwise it's hard to guess why that ⬇️ 1 doesn't turn into 0.
  • Or just open the Git panel?

Actually, maybe better to temporarily replace the ⬇️ 1 ⬆️ 1 with a ⚠️ Merge conflict. Clicking it would open the Git panel (if not already opened). Then people know what's going on and don't get annoyed by a notification or auto-opening the Git panel.

[winstliu]

👍 on switching the push/pull status to a merge conflict warning and not displaying a notification.

[kuychaco]

Actually, maybe better to temporarily replace the ⬇️ 1 ⬆️ 1 with a ⚠️ Merge conflict. Clicking it would open the Git panel (if not already opened).

Oooh interesting idea. After thinking a bit about how this would look I have a couple of concerns around tradeoffs - mainly inconsistency, loss of data and access to functionality.

Concerns:
I feel like the ⬇️ 1 ⬆️ 1 and ⚠️ Merge conflict are independent and convey different information to the user. They each have meaning outside of this particular scenario where the user pulls and there is a resulting merge conflict and they just happen to intersect.

If we add a ⚠️ Merge conflict then I think it would make sense to have this displayed whenever there is a merge conflict, otherwise it might seem inconsistent to only have it during some conflict situations. But with this I think about possibly cluttering up the status bar unnecessarily... is it really that helpful or necessary to have a status bar indication of merge conflicts?

And I hesitate to replace the ahead/behind count because that would again make for an inconsistent experience where sometimes it's there, sometimes it's not, and perhaps it's not immediately obvious to the user that it was replaced by the ⚠️ Merge conflict indicator. We also take away information about the ahead/behind count and make the fetch button inaccessible by doing this.

So if anything I'd advocate for having both shown independently, but I'm not confident that it's worth it to add the ⚠️ Merge conflict indicator. And even then I think that the notification conveys additional useful information. After testing things out a bit I personally don't feel the notification is too obtrusive.

Notification good enough for now?
In my opinion the notification is the clearest indicator of what's happening. It explicitly describes what happened (automatic merge after pull failed) and what you need to do next (fix conflicts and then commit results). For super new Git users this might be helpful and informative.

So I think I'm inclined to leave this as-is for now. Happy to revisit it if people feel strongly about it.

[simurai]

For super new Git users this might be helpful and informative.

Hmm.. not sure if super new Git users would know what to do. 😜 But yeah, since it's already an improvement, we can revisit in another PR.

Edit: Just saw that the file count increases. So another idea is to add an ⚠️ icon there. It means: There are files with conflicts.

[BinaryMuse]

I'm personally not sure a status-bar icon is enough for this scenario. If the Git panel is closed, and the user pulls and gets merge conflicts, and the only thing that happens is a small icon appears in the status bar, it's very likely they'll continue with their work never realizing there's a merge conflict.

[BinaryMuse]

@kuychaco Maybe the language in the notification could be improved? Perhaps something like:

Your pull resulted in merge conflicts
Your last pull resulted in merge conflicts between your local changes and the changes on the remote. You can resolve the conflicts with the Git panel and commit them to continue. If you need help, check out this article on github.atom.io.

(where we would need to create the associated help article — or maybe even embed it in the package as a pane item?)

Perhaps also we can toggle the Git panel open...?

[kuychaco]

All great suggestions. I think for now I'll do the following:

• add ⚠️ icon to status bar changed file count tile when there are merge conflicts
• use improved language in notification (but punt on the help article to tackle later)
• toggle Git panel open when pull results in conflicts

[smashwilson]

Two suggestions you may or may not want to follow 😄

[smashwilson]

👍

[smashwilson]

Care to make these Symbols instead of Strings? 🤔

Or: because the difference between the two arms of the if statements is the method called on this.props.notificationManager, why not pass the name of the method directly (and maybe default to addError)? Something like this:

const {notificationMethod = 'addError', message, description} = errorTransform(error);
this.props.notificationManager[notificationMethod](
  message || 'Cannot complete remote interaction',
  {description, dismissable: true},
);

[smashwilson]

To bikeshed this message a little more, how about:

{
  message: '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.`
}

Same basic message content, but (a) slightly shorter and (b) imperative tense rather than second-person.

I don't know, maybe that's too terse. What do you think?

[kuychaco]

I actually brevity makes more sense for a notification. Easier to scan, and not feel overwhelmed by text. Thanks for the great suggestions :)

[smashwilson]

👍 LGTM