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

introduce --merge to bring up merge editor (for #5770) #155039

Merged
merged 19 commits into from
Jul 18, 2022
Merged

Conversation

bpasero
Copy link
Member

@bpasero bpasero commented Jul 13, 2022

This PR is for #5770

@bpasero bpasero self-assigned this Jul 13, 2022
@bpasero bpasero added this to the July 2022 milestone Jul 13, 2022
@bpasero bpasero marked this pull request as ready for review July 17, 2022 04:45
@bpasero bpasero requested review from jrieken and lramos15 July 17, 2022 04:45
@bpasero
Copy link
Member Author

bpasero commented Jul 17, 2022

@lramos15 @jrieken this is ready for a first look and should already work, despite not being the full solution yet, since the editor resolver is not yet working as we discussed, but I still think we could merge this to get feedback from users and work on the editor resolver independently.

To try it out:

  • scripts/code-cli.sh --help will explain the new --merge option
  • you have to pass in 4 paths and add --merge to bring up a merge editor

For Jo: mainly MergeEditorResolverContribution and friends where the merge editor factory gets registered. This is not yet the final look, once the resolver works as discussed I would only expect a single method being registered only for the merge editor.

For Logan: mainly editor resolver changes: there is currently a hack that I explicitly set the override: 'mergeEditor.Input' when opening the new untyped merge editor input. As discussed, it would be great if the resolver could find the editor by probing on the factory methods.

PS: at first I used names such as local, remote for the sides of the merge editor but then went back again to input1 and input2 like the merge editor internally uses. This makes the solution less opinionated around Git and more generic. The CLI also reflects this and pretty much aligns with this solution. The downside is that the labels for both sides are even more generic ("First Version", "Second Version").

//cc @hediet

@hediet
Copy link
Member

hediet commented Jul 18, 2022

The downside is that the labels for both sides are even more generic ("First Version", "Second Version").

Can we pass the labels for the sides as command line argument?

code -m $REMOTE $LOCAL $BASE $OUT --labelInput1 "Yours" --labelInput2 "Theirs"

jrieken
jrieken previously approved these changes Jul 18, 2022
@bpasero
Copy link
Member Author

bpasero commented Jul 18, 2022

Can we pass the labels for the sides as command line argument?

Yeah, maybe in another PR, will keep a note. Maybe there is also some cool ways in git config to let Git fill in a nice label via some script tricks.

lramos15
lramos15 previously approved these changes Jul 18, 2022
Copy link
Member

@lramos15 lramos15 left a comment

Choose a reason for hiding this comment

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

👍 , I will start on the resolver work

@bpasero bpasero enabled auto-merge (squash) July 18, 2022 15:36
lramos15
lramos15 previously approved these changes Jul 18, 2022
@bpasero bpasero merged commit a567b59 into main Jul 18, 2022
@bpasero bpasero deleted the ben/distinct-manatee branch July 18, 2022 16:44
@numandev1
Copy link

@bpasero @hediet thanks for working on this. I would say, 3 way merge UX is not soo much good. it should improve. as far as I know, IntelliJ merge UX is soo much mature

@github-actions github-actions bot locked and limited conversation to collaborators Sep 1, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants