RevDiff: compare any two files #8193

gerhardol · 2020-06-06T22:01:43Z

Proposed changes

If two items in the RevDiff list are selected, add menu item to compare them
If one item is selected, menu item to remember the file
If one item is selected and one item remembered, menu item to compare them

This allows to compare refactored files, between commits, currently you have to save temporary files and compare them
The primary use is refactoring, when files are renamed and changed without Git detecting the change or when the change is done over several commits. It also allows seeing the difference for images (like in p4merge) when files are renamed.

Viewing files requires that the file exist in the commit, the first item must not be work tree.
To simplify usage, the compared file is automatically selecting the first revision if the second revision does cannot be used for the diff.

See #8193 (comment) for some use cases.

The first and second revisions are not named, just First and Second as the revisions are explained in the top of the menu.

Refactoring:

DiffTool should always detect rename and copy
Hard to track that "-M -C" is always used, the parameters are passed around in some situations, in the Git command for others
RevDiff: Allow difftool menu for untracked
Working in some difftools, comparing to NUL
[New] Use Second instead of Selected in the Difftool submenu (should have been changed when the naming for the groups were changed).
[New] Keep Second above First in the difftool submenu (already this order in Reset submenu)

Possible enhancement:
Add Remember file and Diff with in FileTree

Screenshots

Before

After

(Change from Selected to Second, same as in Diff submenu)

Test methodology

Added tests

✒️ I contribute this code under The Developer Certificate of Origin.

codecov · 2020-06-06T23:11:52Z

Codecov Report

Merging #8193 into master will increase coverage by 0.05%.
The diff coverage is 64.68%.

@@            Coverage Diff             @@
##           master    #8193      +/-   ##
==========================================
+ Coverage   52.75%   52.81%   +0.05%     
==========================================
  Files         855      857       +2     
  Lines       62092    62305     +213     
  Branches    11154    11190      +36     
==========================================
+ Hits        32758    32905     +147     
- Misses      26737    26799      +62     
- Partials     2597     2601       +4

Flag	Coverage Δ
#production	`40.78% <18.96%> (-0.04%)`	⬇️
#tests	`94.46% <99.34%> (+0.05%)`	⬆️

mstv · 2020-06-09T19:49:26Z

👍 Works well. I'll review the code later (edit: has already been rebased).

I'd like to add a separator above the new menu items.
Save selected file to later should be rephrased to e.g. Save selected file for later comparison.
The filename should be quoted in Compare selected file to "file".
Perhaps this menu item should be suppressed if the identical file and revision is selected. Git does not open the difftool in this case.

I think it would be useful to have these context menu items in the file tree, too. E.g. if an unchanged file was cloned to a new one.

Slightly off-topic for this PR: I still quarrel over the new position of the submenu Open with difftool.
It even does not have a fixed position because
Cherry pick changes is hidden for multi-selections. It should rather be disabled if it is too difficult to implement it.

gerhardol · 2020-06-09T21:51:56Z

Works well. I'll review the code later (edit: has already been rebased).

I will rebase at least when there are conflicts, more often if I want to sync to other PRs.

I'd like to add a separator above the new menu items.
Save selected file to later should be rephrased to e.g. Save selected file for later comparison.
The filename should be quoted in Compare selected file to "file".
Perhaps this menu item should be suppressed if the identical file and revision is selected. Git does not open the difftool in this case.

Agree. I have to leave something for you too

I think it would be useful to have these context menu items in the file tree, too. E.g. if an unchanged file was cloned to a new one.

Can this be separate or do we need a separate resolver that is common?

I probably want to have an alternative for Save parent to selected for later comparison (nice and short? Should have the revision too). With a divider it may be OK to see these separate

Slightly off-topic for this PR: I still quarrel over the new position of the submenu Open with difftool.
It even does not have a fixed position because
Cherry pick changes is hidden for multi-selections. It should rather be disabled if it is too difficult to implement it.

(I do not like the position either, but found it better than an implementation that was inconsistent.)
Implementation right now is to get the diff from the diff viewer. It would be better to get the diff (patch) separately anyway and apply that (whitespaces etc).

A bigger problem is to implement the fish-needs-bicycle tests for the controller.

Edit: I have pushed an update for the comments to a private branch. Will update the PR later

GitUI/CommandsDialogs/FileStatusListContextMenuController.cs

RussKie · 2020-06-11T12:17:15Z

What does "Save selected file to later" mean?

gerhardol · 2020-06-11T21:26:20Z

What does "Save selected file to later" mean?

Save the file+commit so it can be compared later. Really similar to comparing the two commits, then compare, but it may be easier to find the files/commits to compare.
Similar to the explorer context menus for difftools like BeyondCompare, DiffMerge and TortoiseGit.

RussKie · 2020-06-12T07:11:19Z

Similar to the explorer context menus for difftools like BeyondCompare, DiffMerge and TortoiseGit.

Do they have the same wording?
May be it should be for - "Save selected file for later"?

...and even then it looks/sounds awkward.

RussKie · 2020-06-12T07:27:22Z

GitUI/CommandsDialogs/RevisionDiffControl.cs

+            compareToExistingDifftoolToolStripMenuItem.Visible = diffFiles.Count == 1 && _savedCompareFileItem != null;
+            compareToExistingDifftoolToolStripMenuItem.Enabled = diffFiles.Count == 1 && _revisionDiffContextMenuController.ShouldEnableSecondSpecialCompare(diffFiles?[0]);
+            compareToExistingDifftoolToolStripMenuItem.Text = _savedCompareFileItem != null ? string.Format(_compareSelectedFile.Text, _savedCompareFileItem.Item.Name) : string.Empty;
+            saveForLaterDifftoolToolStripMenuItem.Visible = diffFiles.Count == 1;
+            saveForLaterDifftoolToolStripMenuItem.Enabled = diffFiles.Count == 1 && _revisionDiffContextMenuController.ShouldEnableFirstSpecialCompare(diffFiles?[0]);


I like the idea of being able to compare arbitrary files across revisions, but I'm finding the visual presentation of the feature confusing. I had to look at the code to understand the screenshots showed...

Few Qs that immediately popped up:

how do I know which files from which revisions I marked?

how do I unmark selected/all files?

how do I change the order of files?

how do I know which files from which revisions I marked?

The file name is shown in the menu item Compare selected file to "file".
Its revision is something for your short-term memory :)

how do I change the order of files?

Click
Or replace the selection using Save selected file for later comparison and then choose Compare selected file to "file" after selecting the first one again.

how do I unmark selected/all files?

It's all about comparing exactly two files.
No need to unmark, just start over with Save selected file for later comparison.

I want to have the previous selectable too
Diff the selected files (when two selected)
Diff with "file" (one selected, revision not included in text)
Save b/12345678 for later (one selected, filename not included)
Save a/12345678 for later

Screenshots for other apps. No consistent description. The proposals are considerably longer...

No selection

Select the same file again

Another selected

Two selected

more than two selected

DiffMerge can save several files, as well as clear them

Tortoise supports both git-diff and file system from the context menu

Thank you, it's starting to make sense.
Now to pick and agree on the wording :) I think if I saw BC and TG menus I'd conceivably guess their intent.
Would "Remember file for compare" work? And then "Compare with <the remembered file>"?
Is "first" and "second" placement material?

(I missed this comment, waiting with a merge)
The menu and Git uses diff rather than compare, so I prefer diff.
"Compare selected..." should be "Diff with " (dropping "selected file".

But "Remember Second for later" or "Remember Second" (similar to First) is possible. The shorter, the better is my preference (I constantly fail though.)

@mstv ?

gerhardol · 2020-06-14T22:54:56Z

I have pushed changes to private branches in my repo only (will test some more)
9d89892 contains the differences, will squash that, see 18e14f654472655d3611992c4e9d7c1b28bc14ba
Description and screenshots will be updated

This cleans up the the handling of assuming first rev when second rev cannot be used, including addig "Save first revision" to the context menu.
(Prepared for mstv's suggestion to add to the context menu there, not done though, awaiting review first.)

Use case 1:
File is renamed but Git do not detect the change
5dae302
Use P4Merge or some tool that display images
Diff IconAbout.co and information.ico

Use case 2:
Compare similar files
dc89d30

Use case 3:
(My primary use, but also the most complicated)
Refactoring splitting a file (there are better examples that I do not find now)

3a
f056b71
Save First for ScriptInfo.cs then ScriptEvent.cs
(to see that the enum is unchanged)

3b
19129fb
Select FormBrowse.cs, First, then RevisionDiff.cs, Diff with.

mstv

LGTM

Found an edge case: No matter whether the first or second of a renamed file is saved for later, the new name appears in the menu:

And: Should it be First -> Second instead of First -> Selected?

Should Second b/... rather be the top item in order to match the order in the RevisionGrid and in the Reset file(s) to menu?

mstv · 2020-06-15T21:37:03Z

GitCommands/Git/GitModule.cs

+        /// <param name="firstGitCommit">commitish</param>
+        /// <param name="secondGitCommit">commitish</param>
+        /// <returns>empty string</returns>
+        public string OpenFilesWithDifftool(string firstGitCommit, string secondGitCommit)


Why not make it return void?

It follows the pattern for OpenWithDifftool()
See the TODO there
As an empty string is always returned anyway, few callers try to use the data
This should be handled the same

GitCommands/Git/GitModule.cs

GitUI/CommandsDialogs/RevisionDiffControl.cs

gerhardol · 2020-06-16T04:21:24Z

Found an edge case: No matter whether the first or second of a renamed file is saved for later, the new name appears in the menu:

I had ignored that, but since you are commenting on it I will change that
Sidenote: Save for later is intentionally not disabling the menuitem if already selected, intentional

And: Should it be First -> Second instead of First -> Selected?

Yes

Should Second b/... rather be the top item in order to match the order in the RevisionGrid and in the Reset file(s) to menu?

It should be the same as in Reset at least. That Selected was before First was natural, but First before Second may not be.
But if the natural selection should be first, the order should be Send, First also in Reset.

mstv · 2020-06-16T21:34:48Z

Sidenote: Save for later is intentionally not disabling the menuitem if already selected

👍 else I would have complained 😉

GitUI/CommandsDialogs/RevisionDiffControl.cs

gerhardol · 2020-06-20T20:46:08Z

Should Second b/... rather be the top item in order to match the order in the RevisionGrid and in the Reset file(s) to menu?

It should be the same as in Reset at least. That Selected was before First was natural, but First before Second may not be.
But if the natural selection should be first, the order should be Send, First also in Reset.

Added a commit to align diff submenu with reset submenu, updated description and screenshot, also removed WIP

mstv

👍

RussKie · 2020-06-25T10:29:19Z

I had a brief looked at the code, it looks good, I have no objections.

gerhardol · 2020-06-25T22:04:00Z

@msftbot merge in 24 hours

RussKie · 2020-06-26T14:00:29Z

For the ease of navigation, continuation of the discussion in #8193 (comment)

Something like this?

"Diff the selected files" (when two selected)
"Diff with <remembered file>" (visible, if there is a 'remembered' file)
separator
"Remember b/12345678 for diff` (one selected, filename not included)
"Remember a/12345678 for diff`
"Forget <remembered file>" (visible, if there is a 'remembered' file)

gerhardol · 2020-06-26T14:11:33Z

Adapted the proposal.

separator

(No need to have a separator further down "Diff the selected" is only shown with two selected, the other only with one

"Diff the selected files" (when two selected)
"Diff with " (when one file selected) (visible, if there is a 'remembered' file)
"Remember Second for diff (one selected, filename not included) "Remember First for diff

Second and First is explained in the top of the menu, no need to do it again

"Forget " (visible, if there is a 'remembered' file)

No need for this one, forgotten if swiching repos.
I consider this as junk in the menu actions, more confusing than helping

...And the references to "saveForLater" in the code should be changed to remember file...

RussKie · 2020-06-26T14:14:16Z

👍
So then, just "Save" -> "Remember"

Thank you

mstv · 2020-06-26T16:05:32Z

I agree.
Though I noticed today: Compared to the menu items above the separator, Compare selected file to "remembered file" could / should be simplified to Second -> "remembered file". If you find this too reduced, then I suggest to avoid the term "Compare to" and rather use "Diff with", which seems to be preferred by git and GE.

Is "first" and "second" placement material?

It has been made consistent with the other submenus, and matches the order in the RevisionGrid.
Sorry this comes really late: Unfortunately, it is opposite to the display of patches. Which should take precedence: DiffView or RevisionGrid? Which is nearer?

gerhardol · 2020-06-26T21:48:07Z

It has been made consistent with the other submenus, and matches the order in the RevisionGrid.
Sorry this comes really late: Unfortunately, it is opposite to the display of patches. Which should take precedence: DiffView or RevisionGrid? Which is nearer?

At this point, I say follow RevisionGrid "one selected" order
The order in the grid can vary though, but the Second is normally the Selected commit, so it is natural to keep it first in the list and First as second

Will squash latest commit in a day or so (no changes other than naming in the latest commit)
(The first four commits should remain)
If no further comments, I will set a merge a day after that

#8239 will follow, then likely #8254 before #7825 can be resumed

RussKie · 2020-06-26T23:11:39Z

Works for me. Thanks

Working in some difftools

* If two items in the RevDiff list are selected, add menu item to diff them * If one item is selected, menu item to remember the file * If one item is selected and one item remembered, menu item to diff them This allows to compare refactored files, between commits

RussKie · 2020-06-28T22:59:13Z

GitUI/CommandsDialogs/RememberFileContextMenuController.cs

+        /// </summary>
+        public FileStatusItem RememberedDiffFileItem { get; set; }
+
+        // Note that the methods in this class are without side effects (static)


There's [Pure] attribute for that.

ghost assigned gerhardol Jun 6, 2020

gerhardol changed the title ~~Feature/i8048 compare two files~~ RevDiff: compare any two files Jun 6, 2020

gerhardol force-pushed the feature/i8048-compare-two-files branch from b0f96ff to e8a9085 Compare June 6, 2020 22:46

gerhardol force-pushed the feature/i8048-compare-two-files branch from e8a9085 to 19a3d21 Compare June 8, 2020 04:53

RussKie reviewed Jun 11, 2020

View reviewed changes

GitUI/CommandsDialogs/FileStatusListContextMenuController.cs Outdated Show resolved Hide resolved

RussKie reviewed Jun 12, 2020

View reviewed changes

gerhardol force-pushed the feature/i8048-compare-two-files branch from 19a3d21 to 4068565 Compare June 15, 2020 20:21

gerhardol requested review from mstv and RussKie June 15, 2020 20:46

mstv reviewed Jun 15, 2020

View reviewed changes

mstv approved these changes Jun 17, 2020

View reviewed changes

GitUI/CommandsDialogs/RevisionDiffControl.cs Outdated Show resolved Hide resolved

gerhardol mentioned this pull request Jun 17, 2020

RevDiff: Add hotkeys to stage/unstage/reset #8239

Merged

gerhardol force-pushed the feature/i8048-compare-two-files branch from 5c9bf87 to a802050 Compare June 20, 2020 14:53

gerhardol mentioned this pull request Jun 20, 2020

RevDiff: diff second/first parent to working directory #8253

Closed

gerhardol force-pushed the feature/i8048-compare-two-files branch from a802050 to 0cd1ffc Compare June 20, 2020 20:44

mstv approved these changes Jun 20, 2020

View reviewed changes

gerhardol mentioned this pull request Jun 20, 2020

RevDiff: Remove parent diff #8254

Merged

gerhardol force-pushed the feature/i8048-compare-two-files branch from 0cd1ffc to fe5fbeb Compare June 25, 2020 21:57

ghost added the status: auto merge label Jun 25, 2020

gerhardol removed the status: auto merge label Jun 25, 2020

gitextensions deleted a comment Jun 26, 2020

gerhardol added 5 commits June 27, 2020 20:52

DiffTool should always detect rename and copy

d79c20b

RevDiff: Allow difftool menu for untracked

05c740f

Working in some difftools

RevDiff: diff menu Selected -> Second

0969afb

RevDiff: Diff submenu: Keep Second before First

435a729

gerhardol force-pushed the feature/i8048-compare-two-files branch 2 times, most recently from 477c86f to b8f68cb Compare June 27, 2020 21:40

gerhardol mentioned this pull request Jun 27, 2020

RevFile: Diff any two files #8275

Merged

RussKie approved these changes Jun 28, 2020

View reviewed changes

RussKie merged commit 7ab900b into gitextensions:master Jun 28, 2020

ghost added this to the 4.0 milestone Jun 28, 2020

gerhardol deleted the feature/i8048-compare-two-files branch November 14, 2021 13:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RevDiff: compare any two files #8193

RevDiff: compare any two files #8193

gerhardol commented Jun 6, 2020 •

edited

codecov bot commented Jun 6, 2020 •

edited

mstv commented Jun 9, 2020 •

edited

gerhardol commented Jun 9, 2020 •

edited

RussKie commented Jun 11, 2020

gerhardol commented Jun 11, 2020

RussKie commented Jun 12, 2020

RussKie Jun 12, 2020

mstv Jun 12, 2020 •

edited

gerhardol Jun 12, 2020

RussKie Jun 25, 2020 •

edited

gerhardol Jun 25, 2020

gerhardol commented Jun 14, 2020 •

edited

mstv left a comment •

edited

mstv Jun 15, 2020

gerhardol Jun 16, 2020

gerhardol commented Jun 16, 2020

mstv commented Jun 16, 2020

gerhardol commented Jun 20, 2020

mstv left a comment

RussKie commented Jun 25, 2020

gerhardol commented Jun 25, 2020

RussKie commented Jun 26, 2020

gerhardol commented Jun 26, 2020

RussKie commented Jun 26, 2020

mstv commented Jun 26, 2020

gerhardol commented Jun 26, 2020 •

edited

RussKie commented Jun 26, 2020 via email •

edited by ghost

RussKie Jun 28, 2020

RevDiff: compare any two files #8193

RevDiff: compare any two files #8193

Conversation

gerhardol commented Jun 6, 2020 • edited

Proposed changes

Screenshots

Before

After

Test methodology

codecov bot commented Jun 6, 2020 • edited

Codecov Report

mstv commented Jun 9, 2020 • edited

gerhardol commented Jun 9, 2020 • edited

RussKie commented Jun 11, 2020

gerhardol commented Jun 11, 2020

RussKie commented Jun 12, 2020

RussKie Jun 12, 2020

Choose a reason for hiding this comment

mstv Jun 12, 2020 • edited

Choose a reason for hiding this comment

gerhardol Jun 12, 2020

Choose a reason for hiding this comment

RussKie Jun 25, 2020 • edited

Choose a reason for hiding this comment

gerhardol Jun 25, 2020

Choose a reason for hiding this comment

gerhardol commented Jun 14, 2020 • edited

mstv left a comment • edited

Choose a reason for hiding this comment

mstv Jun 15, 2020

Choose a reason for hiding this comment

gerhardol Jun 16, 2020

Choose a reason for hiding this comment

gerhardol commented Jun 16, 2020

mstv commented Jun 16, 2020

gerhardol commented Jun 20, 2020

mstv left a comment

Choose a reason for hiding this comment

RussKie commented Jun 25, 2020

gerhardol commented Jun 25, 2020

RussKie commented Jun 26, 2020

gerhardol commented Jun 26, 2020

RussKie commented Jun 26, 2020

mstv commented Jun 26, 2020

gerhardol commented Jun 26, 2020 • edited

RussKie commented Jun 26, 2020 via email • edited by ghost

RussKie Jun 28, 2020

Choose a reason for hiding this comment

gerhardol commented Jun 6, 2020 •

edited

codecov bot commented Jun 6, 2020 •

edited

mstv commented Jun 9, 2020 •

edited

gerhardol commented Jun 9, 2020 •

edited

mstv Jun 12, 2020 •

edited

RussKie Jun 25, 2020 •

edited

gerhardol commented Jun 14, 2020 •

edited

mstv left a comment •

edited

gerhardol commented Jun 26, 2020 •

edited

RussKie commented Jun 26, 2020 via email •

edited by ghost