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

Add new merge conflicts menu and move the previous external merge tool there #3477

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

recht
Copy link

@recht recht commented Apr 5, 2024

Also add options for git checkout --theirs and git checkout --ours

Fixes #2026

  • Please check if the PR fulfills these requirements
  • Cheatsheets are up-to-date (run go generate ./...)
  • Code has been formatted (see here)
  • Tests have been added/updated (see here for the integration test guide)
  • Text is internationalised (see here)
  • Docs (specifically docs/Config.md) have been updated if necessary
  • You've read through your own file changes for silly mistakes etc

…l there

Also add options for `git checkout --theirs` and `git checkout --ours`

Fixes jesseduffield#2026
Copy link
Collaborator

@stefanhaller stefanhaller left a comment

Choose a reason for hiding this comment

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

This is a good start, but it needs some more work.

  1. Most importantly, it should support range selection. It should be possible to press shift-down-arrow to select multiple files, and then checkout theirs or ours for all of them. This raises the question of what happens if you select both a folder, and some of the files inside it; it seems that you should be able to call normalisedSelectedNodes which was introduced for a similar situation.
  2. I find the terms theirs and ours problematic; they are intuitive for a merge conflict, but they appear swapped for a conflict in a rebase. I know that we usually want to stay close to the terms that git CLI uses, but in this case even git CLI admits that the terms can be confusing (there's a long section in the documentation about it). VS Code's conflict browser uses HEAD (Current Change) for ours and Incoming Change for theirs, which I find a little better. At the very least we should have good tooltips that explain this well.
  3. It would be really good to have some integration tests for this. (You ticked the "Tests have been added/updated" in the PR description, that was cheating. 😄)

{
Label: self.c.Tr.CheckoutOurs,
OnPress: func() error {
return self.c.Git().WorkingTree.CheckoutOurs(self.context().GetSelected().Name())
Copy link
Collaborator

Choose a reason for hiding this comment

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

This only works for top-level files. You want GetPath() instead of Name() so that it also works for files in folders.

Copy link
Collaborator

Choose a reason for hiding this comment

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

And you should call self.c.Helpers().MergeAndRebase.CheckMergeOrRebase(err) after this; this will cause any files that no longer have conflicts to be staged, and if no conflicting files are left, it will prompt to continue the rebase.

@recht
Copy link
Author

recht commented Apr 7, 2024

This is a good start, but it needs some more work.

  1. Most importantly, it should support range selection. It should be possible to press shift-down-arrow to select multiple files, and then checkout theirs or ours for all of them. This raises the question of what happens if you select both a folder, and some of the files inside it; it seems that you should be able to call normalisedSelectedNodes which was introduced for a similar situation.
  2. I find the terms theirs and ours problematic; they are intuitive for a merge conflict, but they appear swapped for a conflict in a rebase. I know that we usually want to stay close to the terms that git CLI uses, but in this case even git CLI admits that the terms can be confusing (there's a long section in the documentation about it). VS Code's conflict browser uses HEAD (Current Change) for ours and Incoming Change for theirs, which I find a little better. At the very least we should have good tooltips that explain this well.
  3. It would be really good to have some integration tests for this. (You ticked the "Tests have been added/updated" in the PR description, that was cheating. 😄)

I'll look into that - and I didn't quite cheat, I just didn't see that my git push failed 🤦🏼

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

conflict resolver, pick all from branch/stash
2 participants