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

Feedback :) #26

Open
bew opened this issue Apr 29, 2022 · 6 comments
Open

Feedback :) #26

bew opened this issue Apr 29, 2022 · 6 comments

Comments

@bew
Copy link

bew commented Apr 29, 2022

This plugin is golden ❤️
It's kept simple and does the job very well 👍

Then I was reading git config options, I saw mergetool.hideResolved (added somewhere in 2021):

During a merge Git will automatically resolve as many conflicts as possible and write the MERGED file containing conflict markers around any conflicts that it cannot
resolve; LOCAL and REMOTE normally represent the versions of the file from before Git's conflict resolution. This flag causes LOCAL and REMOTE to be overwriten so that only
the unresolved conflicts are presented to the merge tool. Can be configured per-tool via the mergetool..hideResolved configuration variable. Defaults to false.

Which basically implements what you did with this plugin. Thought you'd want to know 🤷‍♂️

The plugin still helps with the UI setup, where we can easily access local, base & remote with the ...WithHistory variant

@3ter
Copy link

3ter commented Apr 29, 2022

Funny that you right now provide feedback as I just found out about this plugin (way too late 😅) and I do agree, it's 🏅 and I'm thankful for the contributors' efforts!

@whiteinge
Copy link
Owner

Hi, @bew (and @3ter )! I'm the one who implemented hideResolved (with a good deal of guidance from the awesome folk on the Git mailing list). But I do appreciate the heads-up all the same -- I'm happy to hear that addition got noticed 😀 .

I'm yet uncertain what that means for this project but I think on it from time to time. When implementing hideResolved I unfortunately allowed myself to get distracted by an unrelated discussion and I didn't push hard enough to include the withHistory feature. With hindsight I think that's a critical omission that all mergetools would benefit from. Perhaps this project should adapt to use hideResolved and include the history views, or perhaps the history views should be included upstream and this project retired entirely. The former is (much) easier but the latter feels more "correct".

@bew
Copy link
Author

bew commented May 1, 2022

Oh that was you! Nice 👍✨

I think it's a good idea to keep existing behavior so the plugin can still be used on systems with old version of git (looking at you CentOS..), but mentioning the new option and how to use it to skip processing in the plugin would be good as well, allows to reduce what the plugin does on top of git!

perhaps the history views should be included upstream and this project retired entirely.

I don't understand what you mean by that, how would that work?
Git already provides base/remote/local version of the files, is the history view different from that?

@whiteinge
Copy link
Owner

whiteinge commented May 2, 2022

so the plugin can still be used on systems with old version of git

Yeah, good call. That's a good path forward for this project. 👍

Git already provides base/remote/local version of the files, is the history view different from that?

hideResolved was implemented slightly differently than in this Vim plugin. That flag causes Git to overwrite LOCAL and REMOTE, whereas this plugin leaves them intact and only operates on MERGED. (Full explanation here.)

Pros: Many merge tools bindly diff LOCAL and REMOTE, so by overwriting those with each "side" of MERGED all those tools immediately benefit from hideResolved without any modification.

Cons: Overwriting LOCAL and REMOTE causes a loss of that useful historical data. hideResolved does not (currently) provide a way to deliver all versions of the conflicted file (LCONFL, RCONFL, LOCAL, REMOTE, BASE) to the merge tool like this plugin does and I think that should be an option.

@danielcarr
Copy link

@whiteinge For users who don't usually diff with history, and using recent versions of git, would you recommend continuing to use this plugin, or should we rather set git's mergetool to vimdiff with mergetool.vimdiff.hideResolved set to true?

@whiteinge
Copy link
Owner

@danielcarr I'm personally still using this plugin and not currently using hideResolved. Although I don't often reference LOCAL and REMOTE when resolving conflicts, I do like the option to open those versions with a Vim command rather than having to abort the merge and restart it using a different mergetool.

To try and make a recommendation:

  • If you never look at LOCAL and REMOTE then you might as well just use hideResolved. Same end result and doing so will spare you one Vim plugin and ~100 SLOC of Vimscript.
  • If you do look at those (or want to retain the option, like me) then stick with this plugin.

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

No branches or pull requests

4 participants