whiteinge / diffconflicts

A better Vimdiff Git mergetool
BSD 3-Clause "New" or "Revised" License
396 stars 20 forks source link

Feedback :) #26

Open bew opened 2 years ago

bew commented 2 years ago

This plugin is golden :heart: It's kept simple and does the job very well :+1:

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 :man_shrugging:

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

3ter commented 2 years ago

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 commented 2 years ago

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 :grinning: .

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 commented 2 years ago

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 commented 2 years ago

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. :+1:

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 commented 1 year ago

@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 commented 1 year ago

@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: