Rename detection for Advanced filter #9413

gerhardol · 2021-07-25T13:38:28Z

See long term goals in #7598

Proposed changes

Refactor FileChanges -> RevisionGrid (the normal name for the control in other forms)
Do not try to --follow for folders. Something like Bin/ is OK, but GitUI will give too many files and too long command lines. (This could be solved in other ways but do not bother.)
Note that 3.5 does not follow folders (or Submodules) so not limiting functionality. History for folders is useless in 3.5 as well.
Note: There is no enforcing of using the ending "/" in the Advanced filter, manual input.
Move detection of renamed files from FileHistory to RevisionGrid, to use the handling also for Advanced filters (with slightly more general rules).

Test methodology

Add an advanced filter like "**/Rev*Diff*" to see that GitUI\CommandsDialogs\RevisionDiffControl.cs was renamed

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

GitUI/UserControls/RevisionGrid/RevisionGridControl.cs

RussKie

Let's get it in before I get to the history dialog in my clean up.

RussKie · 2021-07-28T12:03:08Z

GitUI/UserControls/RevisionGrid/RevisionGridControl.cs

            }
            catch
            {
                SetPage(new ErrorControl());
                throw;
            }

-            return;
+            string BuildPathFilter(string? path)


❗ We really need to test it.

Can we take this later?
There are several PRs that depends on this PR.

The quoting handling is making some guessing, but as the handling ultimately uses Git to ignore bad paths, it should not be critical.
The quoting and --follow handling can be separated.

...and I have raised my opinions about the usefulness about parsing internal methods like this before...

Can we take this later?

Sure

..and I have raised my opinions about the usefulness about parsing internal methods like this before...

If we don't have tests, there's a high change we'll either keep churning here trying to fix unintended regressions, or regress while updating the functionality.

gerhardol · 2021-07-28T15:29:56Z

Suggest squash merging in a day or so.

mstv · 2021-07-28T22:44:08Z

GitUI/UserControls/RevisionGrid/FormRevisionFilter.cs

+        public void SetPathFilter(string filter)
+        {
+            FileFilterCheck.Checked = !string.IsNullOrWhiteSpace(filter);
+            FileFilter.Text = filter?.Trim();


Have you considered filenames with spaces at start/end?

GE added filenames (for FileHistory and a later PR for a context menu for RevDiff etc) are quouted when adding.
User added paths in Advanced filter must be correctly quoted by the user.
(There is a helper for quoting paths without spaces and quotes, likely the most common case).

A separate commit with the renaming would ease the review a little.

This was separated in several commits until I squashed it preparing for merge.
But commits are still separate in branch gerhardol/feature/i7598-replace-formfilehistory

mstv · 2021-07-28T22:46:17Z

A separate commit with the renaming would ease the review a little.

mstv

Almost OK for me.

This was separated in several commits until I squashed it preparing for merge.

My 2ct: I would keep such plain refactoring commits separate.
Since my available time is getting less, I have started my review after the requested changes were done.
(Not requesting changes neither for the edge case with spaces nor for separate commits.)

But commits are still separate in branch gerhardol/feature/i7598-replace-formfilehistory

This branch does not match. I have split it myself for reviewing.

GitUI/UserControls/RevisionGrid/RevisionGridControl.cs

Filename rename detection requires a separate `git-log --follow` Move this handling from FileHistory to RivisionGridControl to use this handling also for path filters in Advanced filter. * FileHistory: No --follow for folders Normally not interesting, can give extremely long command line paths. * Always quote internal (i.e. FileHistory) paths Assume manual filter paths are quoted correctly. git-log --follow only accepts one path argument. * Skip --find-renames and --find-copies in git-log to get the full revision history as they have no effect. * Use Advanced filter also for FileHistory paths

gerhardol · 2021-07-30T22:47:51Z

My 2ct: I would keep such plain refactoring commits separate.

Separated to a refactor commit.
Git supports ignoring some changes in some commits in blame:
https://akrabat.com/ignoring-revisions-with-git-blame/
With no --ff-only in GH this requires a separate PR which makes this very cumbersome.
In this case I did not think it was necessary, so the commits was marked with squash! in the original commits.

Squashed the two minor changes and modified the HEAD commit message, so RussKie can rebase merge this PR to get going with his changes.

GitUI/UserControls/RevisionGrid/RevisionGridControl.cs

the common name of the control used in other forms.

Filename rename detection requires a separate `git-log --follow` Move this handling from FileHistory to RivisionGridControl to use this handling also for path filters in Advanced filter. * FileHistory: No --follow for folders Normally not interesting, can give extremely long command line paths. * Always quote internal (i.e. FileHistory) paths Assume manual filter paths are quoted correctly. git-log --follow only accepts one path argument. * Skip --find-renames and --find-copies in git-log to get the full revision history as they have no effect. * Use Advanced filter also for FileHistory paths

gerhardol · 2021-07-31T10:14:54Z

Rebased on master, squashing #9413 (review) and activate the comment discussed in #9413 (comment) due to #9056.

gerhardol · 2021-07-31T11:35:49Z

Rebase and merge in 10 hours (manual, no msftbot?)

RussKie · 2021-07-31T12:26:38Z

👍

Filename rename detection requires a separate `git-log --follow` Move this handling from FileHistory to RivisionGridControl to use this handling also for path filters in Advanced filter. * FileHistory: No --follow for folders Normally not interesting, can give extremely long command line paths. * Always quote internal (i.e. FileHistory) paths Assume manual filter paths are quoted correctly. git-log --follow only accepts one path argument. * Skip --find-renames and --find-copies in git-log to get the full revision history as they have no effect. * Use Advanced filter also for FileHistory paths

gerhardol · 2021-07-31T22:33:37Z

(switched order Text/Checked in FormFileHistory.cs as setting Checked triggers EnableFilters() - no difference right now though)

ghost assigned gerhardol Jul 25, 2021

gerhardol mentioned this pull request Jul 25, 2021

Redesign of FormFileHistory #7598

Closed

RussKie reviewed Jul 25, 2021

View reviewed changes

GitUI/UserControls/RevisionGrid/RevisionGridControl.cs Outdated Show resolved Hide resolved

GitUI/UserControls/RevisionGrid/RevisionGridControl.cs Outdated Show resolved Hide resolved

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

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

gerhardol force-pushed the feature/adv-filter-rename-detect branch from d69f4dc to 2f271f3 Compare July 26, 2021 19:21

gerhardol mentioned this pull request Jul 26, 2021

Throw on non-zero exit code and on stderror output #9056

Merged

RussKie marked this pull request as draft July 28, 2021 10:25

gerhardol marked this pull request as ready for review July 28, 2021 10:56

RussKie approved these changes Jul 28, 2021

View reviewed changes

RussKie reviewed Jul 28, 2021

View reviewed changes

gerhardol force-pushed the feature/adv-filter-rename-detect branch from 2f271f3 to d14448c Compare July 28, 2021 15:21

mstv reviewed Jul 28, 2021

View reviewed changes

mstv reviewed Jul 30, 2021

View reviewed changes

GitUI/UserControls/RevisionGrid/RevisionGridControl.cs Show resolved Hide resolved

GitUI/UserControls/RevisionGrid/RevisionGridControl.cs Outdated Show resolved Hide resolved

gerhardol force-pushed the feature/adv-filter-rename-detect branch from d14448c to 85f97ba Compare July 30, 2021 22:40

mstv approved these changes Jul 30, 2021

View reviewed changes

RussKie reviewed Jul 31, 2021

View reviewed changes

GitUI/UserControls/RevisionGrid/RevisionGridControl.cs Outdated Show resolved Hide resolved

RussKie approved these changes Jul 31, 2021

View reviewed changes

FileHistory: rename FileChanges to RevisionGrid

9b95341

the common name of the control used in other forms.

gerhardol force-pushed the feature/adv-filter-rename-detect branch from 85f97ba to ea8b1aa Compare July 31, 2021 10:09

gerhardol force-pushed the feature/adv-filter-rename-detect branch from ea8b1aa to f6bc83d Compare July 31, 2021 22:31

gerhardol merged commit 4a4674c into gitextensions:master Jul 31, 2021

ghost added this to the 3.6 milestone Jul 31, 2021

gerhardol deleted the feature/adv-filter-rename-detect branch July 31, 2021 22:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rename detection for Advanced filter #9413

Rename detection for Advanced filter #9413

gerhardol commented Jul 25, 2021 •

edited by mstv

RussKie left a comment

RussKie Jul 28, 2021

gerhardol Jul 28, 2021

RussKie Jul 31, 2021

gerhardol commented Jul 28, 2021

mstv Jul 28, 2021

gerhardol Jul 28, 2021

mstv commented Jul 28, 2021

mstv left a comment

gerhardol commented Jul 30, 2021

gerhardol commented Jul 31, 2021

gerhardol commented Jul 31, 2021

RussKie commented Jul 31, 2021 via email •

edited by ghost

gerhardol commented Jul 31, 2021

Rename detection for Advanced filter #9413

Rename detection for Advanced filter #9413

Conversation

gerhardol commented Jul 25, 2021 • edited by mstv

Proposed changes

Test methodology

RussKie left a comment

Choose a reason for hiding this comment

RussKie Jul 28, 2021

Choose a reason for hiding this comment

gerhardol Jul 28, 2021

Choose a reason for hiding this comment

RussKie Jul 31, 2021

Choose a reason for hiding this comment

gerhardol commented Jul 28, 2021

mstv Jul 28, 2021

Choose a reason for hiding this comment

gerhardol Jul 28, 2021

Choose a reason for hiding this comment

mstv commented Jul 28, 2021

mstv left a comment

Choose a reason for hiding this comment

gerhardol commented Jul 30, 2021

gerhardol commented Jul 31, 2021

gerhardol commented Jul 31, 2021

RussKie commented Jul 31, 2021 via email • edited by ghost

gerhardol commented Jul 31, 2021

gerhardol commented Jul 25, 2021 •

edited by mstv

RussKie commented Jul 31, 2021 via email •

edited by ghost