Skip to content

unrelated changes in merge conflict commit#6826

Merged
outofambit merged 4 commits intodevelopmentfrom
bugfix/unrelated-changes-in-merge-conflict-commit
Feb 12, 2019
Merged

unrelated changes in merge conflict commit#6826
outofambit merged 4 commits intodevelopmentfrom
bugfix/unrelated-changes-in-merge-conflict-commit

Conversation

@outofambit
Copy link
Contributor

@outofambit outofambit commented Feb 12, 2019

Overview

closes #6349

Description

  • createMergeCommit and doesn't unstage anything prior to committing
  • _finishConflictedMerge only manually stages files that are conflicted

Taken together, this means that we only commit files that git deems part of a conflicted merge (git automatically stages unconflicted files during a conflicted merge) as well as files that the user resolves (or stages manually). This means the user can wreak a little havoc on their merge commit for a conflicted merge, but only via the terminal, and only in very direct ways.

NB — this diff is even smaller if you hide whitespace changes!

Release notes

Notes: [Improved] Merge conflict resolution committing only commits changes relevant to the merge

…esolution

don't unstage anything either!

sometimes it is just that easy!?
@outofambit outofambit added the ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Feb 12, 2019
@outofambit outofambit added this to the 1.6.2 milestone Feb 12, 2019
@billygriffin billygriffin requested a review from niik February 12, 2019 00:47
@shiftkey
Copy link
Member

👍 this is the same approach that I ended up going with for rebase flows

niik
niik previously approved these changes Feb 12, 2019
Copy link
Member

@niik niik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TL;DR: Me and @billygriffin paired on looking through this and testing a number of scenarios and I'm happy to bring this in. I'm heading home in a few minutes though so seeing as it's time sensitive I'll let anyone merge this when the time is right for it.

A small note about this change to share some context. I am hesitant about changes to how we position the app with regards to the index because from an application design perspective we've made the conscious decision to not care about the index. Here's a snippet I wrote in another PR related to how we treat the index

I have concerns about this change from a philosophical standpoint of Desktop caring about the index in the first place. Our philosophy (for better or worse) has been to not care about the index before and now we're opening the door slightly to that (conflicts is another area where we already do this however).

As stated above I think conflicts (merge and rebase) is an area where we by necessity need to be involved with the index but I'm hesitant nonetheless because we've had a lot of churn over how we deal with the index both in this application and in the classic apps with a lot of pain associated with that churn.

I'm especially happy that you put in that warning comment @outofambit, this isn't a change we should be doing lightly. Had this not been time sensitive the only thing I would have remarked on would be a wish to include a more detailed comment in the App Store function that filters out only conflicted files. I think it would be great if you could put some of the situational context that you've got right now into writing there because it's a very complex problem and I think having it outlined there will makes us think twice about churning it again.

@outofambit
Copy link
Contributor Author

Thanks @niik for your review! I totally agree we need to be careful about changes to how we ignore (or don't) the index. I went ahead and added the comment you mentioned as i agree that context will be important for us to preserve fo the future!

@outofambit
Copy link
Contributor Author

merging this now in preparation for today's beta

@outofambit outofambit merged commit 3ad733f into development Feb 12, 2019
@outofambit outofambit deleted the bugfix/unrelated-changes-in-merge-conflict-commit branch February 12, 2019 18:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready-for-review Pull Requests that are ready to be reviewed by the maintainers

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Working directory changes committed unintentionally during merge conflict resolution

5 participants