add manual conflict resolution options in merge conflicts modal!!#6777
add manual conflict resolution options in merge conflicts modal!!#6777iAmWillShepherd merged 19 commits intodevelopmentfrom
Conversation
shiftkey
left a comment
There was a problem hiding this comment.
I know this isn't quite ready for review but I wanted to share these small suggestions while I was reading over the code to get across what's changed...
| // get manual resolutions in case there are manual conflicts | ||
| const repositoryState = this.repositoryStateManager.get(repository) | ||
| const { conflictState } = repositoryState.changesState | ||
| if (conflictState === null) { |
There was a problem hiding this comment.
i have mixed feelings about this check being in Dispatcher. do folks think this would be better as a wrapper function in AppStore?
There was a problem hiding this comment.
Did you mean like this?
diff --git a/app/src/lib/stores/app-store.ts b/app/src/lib/stores/app-store.ts
index ff72a3fee..71821ed17 100644
--- a/app/src/lib/stores/app-store.ts
+++ b/app/src/lib/stores/app-store.ts
@@ -3200,16 +3200,30 @@ export class AppStore extends TypedBaseStore<IAppState> {
/** This shouldn't be called directly. See `Dispatcher`. */
public async _finishConflictedMerge(
repository: Repository,
- workingDirectory: WorkingDirectoryStatus,
- manualResolutions: Map<string, ManualConflictResolutionKind>
+ workingDirectory: WorkingDirectoryStatus
): Promise<string | undefined> {
+ // get manual resolutions in case there are manual conflicts
+ const repositoryState = this.repositoryStateCache.get(repository)
+ const { conflictState } = repositoryState.changesState
+ if (conflictState === null) {
+ // if this doesn't exist, something is very wrong and we shouldn't proceed 😢
+ log.error(
+ 'Conflict state missing during finishConflictedMerge. No merge will be committed.'
+ )
+ return
+ }
+
// filter out untracked files so we don't commit them
const trackedFiles = workingDirectory.files.filter(f => {
return f.status.kind !== AppFileStatusKind.Untracked
})
const gitStore = this.gitStoreCache.get(repository)
return await gitStore.performFailableOperation(() =>
- createMergeCommit(repository, trackedFiles, manualResolutions)
+ createMergeCommit(
+ repository,
+ trackedFiles,
+ conflictState.manualResolutions
+ )
)
}
diff --git a/app/src/ui/dispatcher/dispatcher.ts b/app/src/ui/dispatcher/dispatcher.ts
index b2cb4f2ad..106a4b7c4 100644
--- a/app/src/ui/dispatcher/dispatcher.ts
+++ b/app/src/ui/dispatcher/dispatcher.ts
@@ -647,20 +647,9 @@ export class Dispatcher {
workingDirectory: WorkingDirectoryStatus,
successfulMergeBannerState: SuccessfulMergeBannerState
) {
- // get manual resolutions in case there are manual conflicts
- const repositoryState = this.repositoryStateManager.get(repository)
- const { conflictState } = repositoryState.changesState
- if (conflictState === null) {
- // if this doesn't exist, something is very wrong and we shouldn't proceed 😢
- log.error(
- 'Conflict state missing during finishConflictedMerge. No merge will be committed.'
- )
- return
- }
const result = await this.appStore._finishConflictedMerge(
repository,
- workingDirectory,
- conflictState.manualResolutions
+ workingDirectory
)
if (result !== undefined) {
this.appStore._setSuccessfulMergeBannerState(successfulMergeBannerState)There was a problem hiding this comment.
No, i tried that and i'd like to avoid it because it adds some side effects to _finishConflictedMerge that make it harder to test (it would then depend on state existing somewhere else in the store). I was thinking another method called something like "withConflictState" which would ensure conflict state is not null and then call _finishConflictedMerge. That way the side-effect-i-ness would be contained to a single clear function and _finishConflictedMerge could remain more functional. 🤔
| export function hasUnresolvedConflicts(status: ConflictedFileStatus) { | ||
| export function hasUnresolvedConflicts( | ||
| status: ConflictedFileStatus, | ||
| manualResolution?: ManualConflictResolution |
There was a problem hiding this comment.
we have to take into account a (possible) manualResolution if its a manual conflict. i think this points to a larger issue with manual conflict resolution state existing as a part of conflict state and not associated with the file status. 😕 and that we need a none type in the ManualConflictMethodKind enum.
i'd like to dive into this more, but i think this gets us where we need to be for now. ⏳
There was a problem hiding this comment.
i don't love this being optional ?, but again having an explicit type for "no manual resolution" would help with that... if this was ManualConflictResolution | null then when callers go to pull the value out of the Map<string, ManualConflictResolution> of resolutions in conflictState then they have to check for undefined fist and pass in null instead.
|
I'm going to avoid reviewing this PR because I've been involved a lot with Merge Conflicts reviews, and I'd love some fresh eyes in this area to avoid siloing of things. |
|
@niik or @iAmWillShepherd do you have time to take a looksee? 🙏 |
|
@outofambit I had a question about this screenshot, and the "Using our version" message we display here:
It's not clear from the current flow what "ours" or "theirs" represents here - the file could be modified or deleted (or maybe even added). Would if it'd be helpful to show that detail while we're in here (either in the context menu, after selecting or both)? For reference, this is what we had in an iteration of the design:
And this is what I had working in an prototype of the flow (disregard the branch names, I'm pretty sure they were hard-coded then):
|
|
@shiftkey correct, that is not in this PR. i'm including that in
to be followed up on in a subsequent PR. i'll open an issue for that now and update the description to be more clear. sorry about that. |
|
@outofambit cool, as long as we've got it tracked somewhere. Thanks for the context. |
There was a problem hiding this comment.
Left some comments about the names of 2 labels. @outofambit.
| } | ||
|
|
||
| return { type: SelectionType.CloningRepository, repository, progress } | ||
| return { |
There was a problem hiding this comment.
It's unclear why we are returning an object of this shape. A bit of documentation describing PossibleSelections and it's variants would go a long way in helping me understand it.
There was a problem hiding this comment.
this was simply a formatting change by prettier. @iAmWillShepherd do you want to file a new issue for some docs around this?
iAmWillShepherd
left a comment
There was a problem hiding this comment.
Just a few minor changes.
|
@iAmWillShepherd changes made! 🍎 |
Overview
Closes #6062
Apologies for the slightly large and winding nature of this PR. 😞 Please reach out with questions or if anything is unclear!
Description
merge-conflicts-dialog.tsxinto their own file of stateless functionstslintwon't check to make sure you use all of your props, but whenever that is fixed in the tslint react plugin, we'll get start getting the warnings without having to remove "disabling tslint" commentsNotes
Release notes
Notes: [Added] Use manual merge conflict resolution strategies from within UI