Skip to content
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

use checkout to resolve unmerged binary file conflicts #8060

Closed

Conversation

outofambit
Copy link
Contributor

@outofambit outofambit commented Aug 1, 2019

Overview

Closes #8059

Description

it looks like our prior implementation of manual conflict resolution never worked for files that were changed in both branches. this is because it would simply stage whatever state the file was in in the index (probably a weird conflicted state, or whatever was their before the merge/rebase was started).

this PR adds the step of explicitly git checkout-ing the desired version of the file before staging it, ensuring we choose the single version of the file the user requested in the UI.

we need to double check that this works for merging and rebasing due to this potential complication. (it might check out the opposite version in a rebasing flow, but that should be easy to address if that is the case.)

Release notes

Notes: [Fixed] Manual conflict resolution always added the same file version

Co-Authored-By: Shenghua Chen <xiaozhuang1314@gmail.com>
@outofambit outofambit added the ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Aug 1, 2019
@outofambit outofambit added this to Needs to be Prioritized in PR Priority via automation Aug 1, 2019
@outofambit outofambit moved this from Needs to be Prioritized to ⭐️ PR Reviewable Priority ⭐️ in PR Priority Aug 1, 2019
['add', file.path],
repository.path,
'addConflictedFile'
)).exitCode
Copy link
Member

@shiftkey shiftkey Aug 1, 2019

Choose a reason for hiding this comment

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

🎨 not a blocker, but seeing how we poke at the exitCode within a switch statement a few times I wonder if there's a better way to organize this logic.

  • introduce a simple wrapper to de-duplicate the same work that each Git operation does in this function:
/**
 * Run a Git command and return whether the exit code indicated success
 *
 * This defers to the default error handling infrastructure inside if an error
 * is encountered.
 */
async function runGitCommand(
  args: string[],
  path: string,
  name: string
): Promise<boolean> {
  const { exitCode } = await git(args, path, name)
  return exitCode === 0
}
  • tidy up the simple case to make it a bit more readable:
    case GitStatusEntry.Deleted: {
      return await runGitCommand(
        ['rm', file.path],
        repository.path,
        'removeConflictedFile'
      )
    }
  • For this new case, we also get a bit more readability:
const choiceFlag =
  manualResolution === ManualConflictResolutionKind.theirs
    ? 'theirs'
    : 'ours'
const checkoutCompleted = await runGitCommand(
  ['checkout', `--${choiceFlag}`, '--', file.path],
  repository.path,
  'checkoutConflictedFile'
)
if (checkoutCompleted) {
  return await runGitCommand(
    ['add', file.path],
    repository.path,
    'addConflictedFile'
  )
}
return false

This should make the variable exitCode unnecessary, and you can return assertNever below to make the compiler happy.

@shiftkey
Copy link
Member

shiftkey commented Aug 1, 2019

(it might check out the opposite version in a rebasing flow, but that should be easy to address if that is the case.)

If you're not keen to tackle this case while you're in here, 👍 to tracking this in a fresh issue.

Any interest in capturing a test that verifies this behaviour for merge conflicts?

@tierninho
Copy link
Contributor

Testing results are as follows:

Rebasing

  • Rebase conflict, select option 1 = unable to successfully verify outcome as no commit was shown with the proper selected file
  • Rebase conflict, select option 2 = unable to verify outcome as no commit was shown with the proper selected file

Screen Shot 2019-08-01 at 8 41 02 AM

Merge conflict

  • Merge conflict, select option 1 = unable to successfully verify outcome as no commit was shown with the proper selected file
  • Merge conflict, select option 2 = unable to successfully verify outcome as no commit was shown with the proper selected file

Screen Shot 2019-08-01 at 8 59 55 AM

Perhaps I was looking for the wrong outcome. Please advise if so.

99dcd8b, mac

@outofambit outofambit removed the ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Aug 5, 2019
@outofambit outofambit moved this from ⭐️ PR Reviewable Priority ⭐️ to PR Draft in PR Priority Aug 9, 2019
@kuychaco kuychaco added this to In Progress in Desktop 2.1.2 release via automation Aug 12, 2019
@kuychaco kuychaco self-assigned this Aug 12, 2019
@kuychaco
Copy link
Contributor

I'll be picking this work up and carrying it forward while @outofambit is out this week. Decided to open a new PR for concision rather than add more noise to this one, and keep the PR body of this one preserved for posterity. So I'm closing this as it's superseded by #8097

@kuychaco kuychaco closed this Aug 13, 2019
PR Priority automation moved this from PR Draft to Done Aug 13, 2019
@kuychaco kuychaco removed this from In Progress in Desktop 2.1.2 release Aug 13, 2019
@rafeca rafeca deleted the bugfix/manual-conflict-resolution-checkout branch July 22, 2020 08:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

Binary conflict resolve doesn't work
4 participants