Fix resetting selected lines for renamed files in index #10252

mdonatas · 2022-10-13T20:15:15Z

Fixes

Fixes an issue where if you reset selected lines in a staged and renamed file then rename is undone too.

Proposed changes

Adjust the patch header to remove "reverse rename" part.

Screenshots

Before

After

Test methodology

Manual
Integration test

Test environment(s)

Git Extensions 33.33.33
Build 023b2d8 (Dirty)
Git 2.37.3.windows.1
Microsoft Windows NT 10.0.22000.0
.NET 6.0.10
DPI 192dpi (200% scaling)

Merge strategy

I agree that the maintainer squash merge this PR (if the commit message is clear).

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

mdonatas · 2022-10-13T20:16:57Z

Related discussion that I've opened for integration tests part #10251

mstv · 2022-10-14T20:21:20Z

GitUI/CommandsDialogs/FormCommit.cs

-                RescanChanges();
+                RescanChangesAsync();


The returned Task must be run, mustn't it?

It shouldn't be awaited as it's fire-and-forget kind of task. I reverted this commit since I was returning this task because I didn't know about ThreadHelper.JoinPendingOperations(); and that it does exactly what I need - waits for background tasks to complete (at least that's my assumption).

mstv · 2022-10-14T20:21:30Z

GitUI/CommandsDialogs/FormCommit.cs

-                Initialize();
+                InitializeAsync();


mstv · 2022-10-14T20:23:04Z

GitUI/CommandsDialogs/FormCommit.cs

+                return ThreadHelper.JoinableTaskFactory.RunAsync(() =>
                {
                    return _unstagedLoader.LoadAsync(GetAllChangedFilesWithSubmodulesStatus, onComputed);
-                });
+                }).Task;


❔ just

await _unstagedLoader.LoadAsync(GetAllChangedFilesWithSubmodulesStatus, onComputed);

Reverted this as it should only be awaited in tests and there is ThreadHelper.JoinPendingOperations(); for that.

gerhardol · 2022-10-14T22:34:55Z

GitCommands/Patches/PatchManager.cs

+                string line = headerLines[i];
+                if (line.StartsWith("--- "))
+                {
+                    line = $"--- a/{newName}";


This is a risky check. I believe that a/, b/ is forced right now in GE, but Git could change the output format any time.

I suppose that would be a serious breaking change (and not just for GE). Also git documentation mentions these prefixes explicitly https://git-scm.com/docs/diff-format

This can be changed with diff.mnemonicprefix=true. It was forced to false some time ago so a/b is likely stable.
But number of spaces etc are not porcelain and can be changed without notice.
It could be different in existing Git versions too.

Would you approve of using regex here? Something like:
^---\s+(?<a>.+)/(?<aName>.+)$ and
^\+\+\+\s+(?<b>.+)/(?<bName>.+)$

to capture a prefix used and a file name as well as make it more resilient to spacing.

gerhardol · 2022-10-14T22:35:47Z

GitCommands/Patches/PatchManager.cs

+                {
+                    line = line.Replace($" {oldNameFull} ", $" a/{newName} ");
+                }
+                else if (line.StartsWith("rename from ") || line.StartsWith("rename to ") || line.StartsWith("similarity index "))


Are any of these strings ever translated in Git?

Since these are part of a diff format used by git and are called extended headers I think it's safe to assume that that wouldn't happen. At least documentation doesn't mention anything of that sort.
https://git-scm.com/docs/diff-format

It is described as a format, it is not even a 'requested' format, certainly not a porcelain interface.
Adding translations in Git will break too.
It could be OK anyway, if the consequences are low (like current behavior in master).
There need to be at least comments about this.

I'd like to push back if I may. Translation argument sounds almost like saying that translating browsers would mean translating HTTP Headers they send (custom ones, not covered by the standard).
These are headers added by the tools for the tools (at least I would assume so). They are meant to be parsable. Translating them would break older versions thus I can't imaging that ever happening. Although if you still insist, I'll add a comment.

gerhardol · 2022-10-14T22:42:17Z

GitUI/Editor/FileViewer.cs

@@ -73,6 +73,7 @@ private enum ViewMode
        private Func<Task>? _deferShowFunc;
        private readonly ContinuousScrollEventManager _continuousScrollEventManager;
        private FileStatusItem? _viewItem;
+        private bool _confirmResetLines = true;


This should be marked as test specific, a proposal below
Even better would be to hide this in MessageBox.Show() instead with a test accessor
(assuming that there is no short term way to avoid this)

Suggested change

private bool _confirmResetLines = true;

// This variable is used by tests to avoid popups

private bool _unitTestConfirmResetLines = true;

MessageBox.Show() is not under our control and making a wrapper for it seems a daunting task for me at least :)
I like your suggested rename, thanks!

Wrap the call to MessageBox.Show() instead or rather switch to TaskDialog

Have a look at Use TaskDialog instead of MessageBox.Show for reset selected lines confirmation. Thoughts?

IntegrationTests/UI.IntegrationTests/CommandsDialogs/FormCommitTests.cs

RussKie

👍 modulo other comments

RussKie · 2022-10-16T04:06:21Z

GitUI/Editor/FileViewer.cs

@@ -1476,7 +1481,7 @@ public void ResetNoncommittedSelectedLines()
                return;
            }

-            if (MessageBox.Show(this, TranslatedStrings.ResetSelectedLinesConfirmation, TranslatedStrings.ResetChangesCaption,
+            if (_unitTestConfirmResetLines && MessageBox.Show(this, TranslatedStrings.ResetSelectedLinesConfirmation, TranslatedStrings.ResetChangesCaption,


We should change MessageBox.Show to TaskDialog and then we won't have test-only code outside the TestAccessor.

Please have a look at Use TaskDialog instead of MessageBox.Show for reset selected lines confirmation commit. I've tried to look for examples of manipulating TaskDialogPage in the tests but couldn't find any. I am not sure if this is a correct approach.

RussKie · 2022-10-16T11:12:58Z

I had a very cursory look from a phone, and I think you're on the right track. You can see examples in https://github.com/dotnet/winforms/blob/main/src/System.Windows.Forms/tests/IntegrationTests/WinformsControlsTest/TaskDialogSamples.cs and https://github.com/dotnet/samples/tree/main/windowsforms/TaskDialogDemo/cs. A task dialog page can be autoclosed, IIRC, though it's been a while since I looked at the API.

mdonatas · 2022-10-16T12:56:10Z

A task dialog page can be autoclosed, IIRC

Thanks for the pointers!

https://github.com/dotnet/winforms/blob/main/src/System.Windows.Forms/tests/IntegrationTests/WinformsControlsTest/TaskDialogSamples.cs#L217-L233 uses a timer to call PerformClick on a desired button to auto-close while I do it immediately upon creation of the dialog. Having looked at examples I think what I've got is correct.

mdonatas · 2022-10-16T17:29:47Z

Update: The fix was to call Application.EnableVisualStyles(); in a GlobalSetupOnce.cs where this call would be made once per assembly

AppVeyor is failing because of TaskDialogPage and I've been hitting my head against the problem for quite some time now... @RussKie it seems you are somewhat familiar with the root cause dotnet/wpf#6810 any ideas how to solve this here?

p.s. Works on my machine™
p.s.s. Although locally I needed to add

public FormCommitTests()
{
    Application.EnableVisualStyles();
}

mstv

LGTM, have not run

UnitTests/CommonTestUtils/ReferenceRepository.cs

GitCommands/Patches/PatchManager.cs

UnitTests/CommonTestUtils/ReferenceRepository.cs

….Command.ResetLines

This reverts commit 665eb27.

…inish

…nfirmation

RussKie · 2022-10-19T06:05:36Z

GitUI/Editor/FileViewer.cs

+                Caption = TranslatedStrings.ResetChangesCaption,
+                Icon = TaskDialogIcon.Warning,
+                Buttons = { TaskDialogButton.Yes, TaskDialogButton.No },
+                DefaultButton = TaskDialogButton.Yes,


Is this a good idea? I think the default button for destructive operations should be the one that doesn't lead to data loss.
Which button is selected by default now (in the message box)?

Yes is currently the default in the MessageBox and because I use it a lot in my daily work (select text, r, enter, repeat) I didn't want to change the behavior even though I agree with what you said.

RussKie · 2022-10-19T06:08:31Z

IntegrationTests/UI.IntegrationTests/GlobalSetupOnce.cs

+using NUnit.Framework;
+
+[SetUpFixture]
+public class GlobalSetupOnce


How does this class get used?

From the NUnit docs

This is the attribute that marks a class that contains the one-time setup or teardown methods for all the test fixtures in a given namespace.

Since GlobalSetupOnce is in no namespace it's executed once per assembly. I've followed Answer 2 from this article.

RussKie

👍

mstv

Thank you

(cherry picked from commit 69a8570)

ghost assigned mdonatas Oct 13, 2022

mstv reviewed Oct 14, 2022

View reviewed changes

gerhardol reviewed Oct 14, 2022

View reviewed changes

mdonatas requested review from gerhardol and mstv and removed request for gerhardol and mstv October 15, 2022 01:16

RussKie reviewed Oct 16, 2022

View reviewed changes

RussKie added the 📭 needs: author feedback More info/confirmation awaited from OP; issues typically get closed after 30 days of inactivity label Oct 16, 2022

ghost removed the 📭 needs: author feedback More info/confirmation awaited from OP; issues typically get closed after 30 days of inactivity label Oct 16, 2022

mdonatas force-pushed the reset-renamed-from-index branch from d779757 to 95f5b50 Compare October 16, 2022 10:09

mdonatas force-pushed the reset-renamed-from-index branch from 95f5b50 to ad88677 Compare October 16, 2022 12:35

mdonatas force-pushed the reset-renamed-from-index branch 2 times, most recently from 0873181 to 562f4e7 Compare October 16, 2022 16:53

mstv reviewed Oct 16, 2022

View reviewed changes

UnitTests/CommonTestUtils/ReferenceRepository.cs Outdated Show resolved Hide resolved

GitCommands/Patches/PatchManager.cs Outdated Show resolved Hide resolved

mstv reviewed Oct 16, 2022

View reviewed changes

UnitTests/CommonTestUtils/ReferenceRepository.cs Outdated Show resolved Hide resolved

mdonatas force-pushed the reset-renamed-from-index branch from 562f4e7 to ee74e97 Compare October 18, 2022 17:28

mdonatas added 8 commits October 18, 2022 20:54

Fix resetting selected lines for renamed files in index

be2d92f

Enable tests to await rescanning of FormCommit

0a9f850

Add DeleteRepoFile to ReferenceRepository

e571947

Add an overload to CreateCommit for single file

663d435

Add a workaround for tests to skip confirmation dialog for FileViewer…

96a780a

….Command.ResetLines

Add a test that FileViewer.Command.ResetLines doesn't revert file rename

02f2318

Revert "Enable tests to await rescanning of FormCommit"

2aa3d7a

This reverts commit 665eb27.

Use ThreadHelper.JoinPendingOperations(); to wait for operations to f…

8219a9a

…inish

mdonatas added 5 commits October 18, 2022 20:54

Rename _confirmResetLines to _unitTestConfirmResetLines

98d762b

Use TaskDialog instead of MessageBox.Show for reset selected lines co…

9313a33

…nfirmation

Call Application.EnableVisualStyles(); once per tests assembly

c1a9d49

Add suggested ReferenceRepository style improvements

928ce1f

Add comments to explain CorrectHeaderForRenamedFile innards

36eb407

mdonatas force-pushed the reset-renamed-from-index branch from ee74e97 to 36eb407 Compare October 18, 2022 18:01

mdonatas requested review from mstv and removed request for gerhardol October 18, 2022 18:20

RussKie reviewed Oct 19, 2022

View reviewed changes

RussKie added the 📭 needs: author feedback More info/confirmation awaited from OP; issues typically get closed after 30 days of inactivity label Oct 19, 2022

ghost removed the 📭 needs: author feedback More info/confirmation awaited from OP; issues typically get closed after 30 days of inactivity label Oct 19, 2022

mstv approved these changes Oct 19, 2022

View reviewed changes

RussKie merged commit 69a8570 into gitextensions:master Oct 19, 2022

ghost added this to the vNext milestone Oct 19, 2022

mdonatas deleted the reset-renamed-from-index branch October 20, 2022 09:20

RussKie pushed a commit that referenced this pull request Oct 22, 2022

Fix resetting selected lines for renamed files in index (#10252)

714d98a

(cherry picked from commit 69a8570)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix resetting selected lines for renamed files in index #10252

Fix resetting selected lines for renamed files in index #10252

mdonatas commented Oct 13, 2022 •

edited

mdonatas commented Oct 13, 2022

mstv Oct 14, 2022

mdonatas Oct 15, 2022

mstv Oct 14, 2022

mstv Oct 14, 2022

mdonatas Oct 15, 2022

gerhardol Oct 14, 2022

mdonatas Oct 15, 2022 •

edited

gerhardol Oct 15, 2022

mdonatas Oct 16, 2022

gerhardol Oct 14, 2022

mdonatas Oct 15, 2022

gerhardol Oct 15, 2022

mdonatas Oct 16, 2022

gerhardol Oct 14, 2022

mdonatas Oct 15, 2022

gerhardol Oct 15, 2022

mdonatas Oct 16, 2022

RussKie left a comment •

edited

RussKie Oct 16, 2022

mdonatas Oct 16, 2022 •

edited

RussKie commented Oct 16, 2022 via email •

edited by ghost

mdonatas commented Oct 16, 2022

mdonatas commented Oct 16, 2022 •

edited

mstv left a comment

RussKie Oct 19, 2022 •

edited

mdonatas Oct 19, 2022

RussKie Oct 19, 2022

mdonatas Oct 19, 2022 •

edited

RussKie Oct 19, 2022

RussKie left a comment

mstv left a comment

-        private bool _confirmResetLines = true;
+        // This variable is used by tests to avoid popups
+        private bool _unitTestConfirmResetLines = true;

Fix resetting selected lines for renamed files in index #10252

Fix resetting selected lines for renamed files in index #10252

Conversation

mdonatas commented Oct 13, 2022 • edited

Fixes

Proposed changes

Screenshots

Before

After

Test methodology

Test environment(s)

Merge strategy

mdonatas commented Oct 13, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mdonatas Oct 15, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RussKie left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mdonatas Oct 16, 2022 • edited

Choose a reason for hiding this comment

RussKie commented Oct 16, 2022 via email • edited by ghost

mdonatas commented Oct 16, 2022

mdonatas commented Oct 16, 2022 • edited

mstv left a comment

Choose a reason for hiding this comment

RussKie Oct 19, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mdonatas Oct 19, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RussKie left a comment

Choose a reason for hiding this comment

mstv left a comment

Choose a reason for hiding this comment

mdonatas commented Oct 13, 2022 •

edited

mdonatas Oct 15, 2022 •

edited

RussKie left a comment •

edited

mdonatas Oct 16, 2022 •

edited

RussKie commented Oct 16, 2022 via email •

edited by ghost

mdonatas commented Oct 16, 2022 •

edited

RussKie Oct 19, 2022 •

edited

mdonatas Oct 19, 2022 •

edited