unrelated changes in merge conflict commit#6826
Conversation
…esolution don't unstage anything either! sometimes it is just that easy!?
|
👍 this is the same approach that I ended up going with for rebase flows |
niik
left a comment
There was a problem hiding this comment.
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.
|
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! |
|
merging this now in preparation for today's beta |
Overview
closes #6349
Description
createMergeCommitand doesn't unstage anything prior to committing_finishConflictedMergeonly manually stages files that are conflictedTaken 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