Show Stash in rev grid #10138

gerhardol · 2022-08-07T22:28:32Z

Proposed changes

Show stash commits in the revision grid, with menu options to pop and drop stashes.

A stash is implemented in Git with 3 internal commit.
Some more explanations in GitUI\UserControls\RevisionGrid\RevisionGridControl.cs:988
The implementations I have seen do not expose the third commit with untracked files.
The PR includes this "untracked" commit (empty "untracked" are filtered) for the first 10 stashes (see Performance below for details).
This setting is read from the settings but no setter is exposed, mostly because there is no natural place to show it (maybe in View is the best, but other similar settings are in General, that is considered full).

The implementation by default only show the stashes where the parents are shown in the grid.
So for instance if you stash and then rebase then the stash is not shown (but see below).
Unless reflogs are shown, only stashes that are related to commits visible in the grid is shown.
(Another difference with reflog is that stashes are presented in Git order, the "inserted" stashes are always shown related to the main commit).

The label for stash commits is partly for this reason set to the reflog selector like "stash@{0}" (required to pop/drop stashes) (another reason is to not get lost too easy when looking at commits, especially when many stashes are created for the same commit).
(For instance GitKraken shows these commits.)
FormStash can still be used to see these, they could also be added to the side panel (similar to SourceTree) (but the contents would not be shown). See #5460.
(Sublime merge shows both in grid and sidepanel)

GE currently always show the latest stash, this was expected by mstv in the initial version.
The latest stash is therefore always shown if stashes are shown.
This commit will always include the index and untracked commits.
See #10138 (comment) for a discussion about how that could be changed.

There is no new menu operation to create a stash, use stash button or Ctrl-Alt-Up to create a stash.
Create a stash operate on both Index and WorkTree commits, so there is no natural way to connect this to a revision (but partial commit should be possible to create in RevDiff).
FormStash was improved in #9303 too.

Performance

Loading revisions in 4.0 is heavily optimized, some performance measurements to ensure there is no regression.
(I have tried to make sure the addition has minimal impact on performance).

My GE repo has 19400 commits and 542 stashes, testing on Win10 with Zen2600, 16GB, SSD.
I consider 542 stashes to be an upper limit to what is reasonable to test with.

If stashes are not displayed, basically one null variable is, adding less than 1 ms to process all revisions. So usecases not showing stashes are not affected.
Enabled but no existing stashes has a few more checks, affects loading with about 9 ms.

Times are approximate, some variation (+/- 30 ms), but relative consistent differences.
Time will be affected by number of cores etc
Refreshing refs is done in three partly parallel steps:

Getting refs and checkout
Total step 175 ms (git-for-each-ref 125 ms)
Getting Stash commit (10 stash checked for untracked)
Total step 175 ms (git-stash-list 95 ms, git-log 70 ms)
If this time is higher than step 1 the load time is increasing.
Note that the second Git command for untracked is not needed if reflog is shown.
Getting revisions
Total git-log time 600 ms but first revision is delivered and could be processed for display after 140 ms (this is when the spinner is replaced with "Loading" in upper right corner) (as refs are slower, revgrid will be updated 35 ms after this)
(if sorting in the grid is enabled, revisions will be shown with at least a 100 ms delay)
Processing time to find if stashes should be added is about 9 ms.
The GUI update is done in about the time that git-log runs.
The "Loading" label is then removed, but some additional CPU may be needed for selecting a new revision (if filtering changed).

Stashes slow down showing revisions in the grid with about 9 ms plus possible additional time due to parallel processing overhead (but I have not seen any faster ref handling with stash disabled).
This is assuming that the 10 "untracked" takes the same time as getting refs, which in my case is 10, which I also believe is an OK value of untracked to show.
0 untracked limits step 2 with 60 ms (but revisions are not shown earlier), 50 untracked adds 30 ms and all 542 adds 400 ms.

Brief tests with Linux repo in wsl limited to 100000 commits shown with one local branch and 10 stashes.
With so few refs, refs are handled in 150 ms and stash-list in 170 and listing untracked will add 100 ms and 50 ms (processing adding to grid) out of total 4.1 s. So hardly noticeable.

Screenshots

With reflogs shown, the change is that labels and menu operations are enabled for all commits.

Before

Latest stash shown

After

If parent commit is not shown, stash is not shown (first is shown though, not updated example)

Other commits are shown, "untracked" commit only if it is not empty

Menu

Test methodology

A few tests added, some modified.

Test environment(s)

Git 2.37.1
No other version tested, nothing seen in the Git doc about any compatibility.

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.

mstv · 2022-08-24T08:09:07Z

I have not had a look at the details yet.
Though the current approach does not display (the most recent) stash if it is based on an invisible commit, e.g. "disappears" after a rebase.
The stash index is shown also only if the reflog is shown.
Edit: You have mentioned these in the PR description. But the most recent stash should always be visible. The dependency on the reflog feels too random to me. At the moment I think rather to display all or no other stashes.

gerhardol · 2022-08-24T13:48:43Z

Latest stash can always be included (it is explicitly excluded now), but then the latest stash will always look like the following, unless there are some funky code to remove the index and empty untracked commits.

But maybe a fixed number of stashes can always be added ("stash@{0} stash@{1} stash@{2} ... stash@{20}") by default.
Then there would be a smaller delay then if stashes have to be queried first (so maybe 20 ms extra instead of 200 ms in my GE clone).

The full info should be seen in the side panel, similar to filtered branches (not implemented).

gerhardol · 2022-08-24T23:05:51Z

Some measurements:
git-log <stash@{} for 542 stashes> adds about 550 ms to the listing of the first commit. Limiting to 20 adds about 20 ms. (So about 1 ms/commit. So if "force include" is used, the list should be limited. This will require a hard to explain configuration. I canparse about 2000 stashes, then the command line get too long.

To avoid adding the "index" commit and empty untracked, processing time added is about 10 ms, similar handling as for inserting untracked commits

mstv · 2022-08-25T21:40:18Z

Latest stash can always be included (it is explicitly excluded now), but then the latest stash will always look like the following, unless there are some funky code to remove the index and empty untracked commits.

That's how it used to be. Some of this info is rarely really needed indeed. Though stashes can be not just one commit. So, alright for me.

But maybe a fixed number of stashes can always be added ("stash@{0} stash@{1} stash@{2} ... stash@{20}") by default.

Will Git complain if there are fewer stashes?

Then there would be a smaller delay then if stashes have to be queried first (so maybe 20 ms extra instead of 200 ms in my GE clone).

👍
I think 10 would suffice.

gerhardol · 2022-08-25T21:52:31Z

But maybe a fixed number of stashes can always be added ("stash@{0} stash@{1} stash@{2} ... stash@{20}") by default.

Will Git complain if there are fewer stashes?

No, that is fine.

Then there would be a smaller delay then if stashes have to be queried first (so maybe 20 ms extra instead of 200 ms in my GE clone).

👍 I think 10 would suffice.

I have a problem explaining this setting, so not keen on it.
As rev-parse stash@{0} etc can take long time, the complete stash listing should probably be done before listing in the grid, always delaying commit presentation.
(Except for reflog when the stashes are added anyway)

mstv · 2022-08-25T22:29:02Z

I have a problem explaining this setting, so not keen on it.

OK, so explicitly request just the most recent stash.

RussKie · 2022-08-28T13:31:45Z

Perhaps worth considering bumping the stash'related menus to the top:

gerhardol · 2022-09-03T21:24:12Z

I have a problem explaining this setting, so not keen on it.

OK, so explicitly request just the most recent stash.

Changed to show the latest stash if ShowStashes

If (currently hidden) and MinimumStashesAlwaysInGrid are added in settings, the descriptions could be something like below.
Likely a follow up. Similar with stash in sidepanel.

ShowStashes
(Checkbox as in View menu)

MinimumStashesAlwaysInGrid (default 1)
Stashes in the grid are by default shown in the grid if the parent commit is visible in the revision grid.
(If reflogs are shown, all commits including all stashes are shown.)
This setting will ensure that the stashes are added to the grid.
Note that the index and untracked commits are always included.
Note: Setting this value to a high value may delay the display of commits.

MaxStashesWithUntrackedFiles (default 10)
By default only MinimumStashesAlwaysInGrid stashes will include the stash commit with untracked files.
This setting will include the untracked commit (unless it is empty) for additional stashes.
Note: Setting this value to a high value may delay the display of commits.

gerhardol · 2022-09-04T07:54:59Z

squashed, rebased (mstv had pulled the latest updates so they could be reviewed...) and updated the description, removed draft.

RussKie

A quick review, will need more time for a more in depth review

RussKie · 2022-09-04T10:56:34Z

GitCommands/RevisionReader.cs

+        public List<GitRevision> GetStashes(CancellationToken token)
+        {
+            Debug.Assert(_hasReflogSelector, "_hasReflogSelector must be set to get the reflog selectors (to identify stashes)");
+            GitArgumentBuilder arguments = new("stash")
+            {
+                "list",
+                "-z",
+                $"--pretty=format:\"{LogFormat}\""
+            };
+
+            List<GitRevision> stashes = new();
+            using (IProcess process = _module.GitCommandRunner.RunDetached(arguments, redirectOutput: true, outputEncoding: GitModule.LosslessEncoding))
+            {
+                byte[] buffer = new byte[4096];
+
+                foreach (ArraySegment<byte> chunk in process.StandardOutput.BaseStream.ReadNullTerminatedChunks(ref buffer))
+                {
+                    token.ThrowIfCancellationRequested();
+
+                    if (TryParseRevision(chunk, out GitRevision? revision))
+                    {
+                        stashes.Add(revision);
+                    }
+                }
+            }
+
+            return stashes;
+        }


I don't believe this functionality belong in this class. Probably this logically belongs to GitModule (explosed via IGitModule).

This is leading revisions, just a different Git command than the git-log currently in the class.
So an instance of RevisionReader is needed regardless.

GitCommands/RevisionReader.cs

RussKie · 2022-09-04T11:06:22Z

GitCommands/RevisionReader.cs

+        /// <param name="untracked">commit list</param>
+        /// <param name="token">Cancellation token</param>
+        /// <returns>List with GitRevisions</returns>
+        public List<GitRevision> GetRevisionsFromList(IList<ObjectId> untracked, CancellationToken token)


I don't believe we need to materialise the list, and we can use the sequence just as well with a little tweak:

public List<GitRevision> GetRevisionsFromList(IEnumerable<ObjectId> untracked, CancellationToken token) { List<GitRevision> stashes = new(); string untrackedIds = string.Join(" ", untracked); if (string.IsNullOrWhiteSpace(untrackedIds)) { return stashes; } GitArgumentBuilder arguments = new("log") { "-z", $"--pretty=format:\"{LogFormat}\"", "--dirstat=files", untrackedIds, ".", };

What is the benefit with this?
I find it easier to read checking the size (and slightly more efficient even if that is not a focus here).

We constantly reading revisions, and this code is on the hot path. So we want to avoid memory churn as much as possible.

I did not see any improvement for allocation in your proposal, just a slower check.

For performance and allocation when loading, the important path is the reading of revisions (in RevisionReader that is optimized with my best effort) and in RevGrid (can likely be a few tuning done). The general improvement is to avoid calling Git when not needed.
I am adding an if and a null allocation in this PR to the rev reading loop, which will add ms.
(Replacing String.Empty with "" and removing precompiled regex is a more obvious optimization...)

I made a simple refactoring and changed this code slightly.

GitCommands/Settings/AppSettings.cs

GitUI/UserControls/RevisionGrid/RevisionGridControl.Designer.cs

GitUI/UserControls/RevisionGrid/RevisionGridControl.cs

mstv · 2022-09-05T18:54:27Z

After recovering the lost object, the commit is filtered out somehow as long as RefLog references are shown.

The restored commit "LOST_FOUND_1" (and the index commit) is not returned by
git -c log.showSignature=false log -z --pretty=format:"%H%T%P%n%at%n%ct%n%aN%n%aE%n%cN%n%cE%n%B"
neither with nor without --reflog.
How is it found and added if not showing RefLog references?
The base commit c9b8c5d is only returned with --reflog. Weird!

mstv

👍 Thank you
Fixes #9873

GitUI/UserControls/RevisionGrid/RevisionGridControl.cs

gerhardol · 2022-09-11T20:03:29Z

Plan to squash and rebase tomorrow
@RussKie @pmiossec Any objections to merging and including in 4.0?

RussKie

Looked through on a phone in a browser. I don't have any major objections. Supportive of taking it in to the release.

GitUI/UserControls/RevisionGrid/RevisionGridControl.Designer.cs

Show stash commits in the revision grid, with menu options to apply, pop and drop stashes. Stashes are listed in parallel to refs and the first part reading logs, to not delay the revision display in the grid. The first stash is always shown similar to current implementation, other stashes are shown only if the parent commit is shown (so all stashes are chown with rev grid). (Except for the first grid and for reflog, the index commit is not shown. The internal commit with untracked files is hidden if it is empty for the first stashes. (Limited to not delay the revision display.) Further performance evaluation in the pull request.

gerhardol · 2022-09-14T20:11:17Z

Plan to merge this tomorrow

Show stash commits in the revision grid, with menu options to apply, pop and drop stashes. Stashes are listed in parallel to refs and the first part reading logs, to not delay the revision display in the grid. The first stash is always shown similar to current implementation (with index/untracked commits), other stashes are shown only if the parent commit is shown (so all stashes are chown with rev grid). (Except for the first grid and for reflog, the index commit is not shown. The internal commit with untracked files is hidden if it is empty for the first stashes. (Limited to 10 to not delay the revision display.) Further performance evaluation in the pull request. (cherry picked from commit ad6671d)

ghost assigned gerhardol Aug 7, 2022

gerhardol force-pushed the feature/i5371-stash-in-grid branch 2 times, most recently from 40e3fa5 to 7d4f50d Compare August 8, 2022 20:05

gerhardol mentioned this pull request Aug 20, 2022

FormStash: Display index of stashes #10161

Merged

gerhardol mentioned this pull request Aug 28, 2022

Cleanup git-log filter option handling #10129

Merged

gerhardol force-pushed the feature/i5371-stash-in-grid branch 3 times, most recently from d71a6a1 to 2b610b2 Compare September 3, 2022 19:52

gerhardol force-pushed the feature/i5371-stash-in-grid branch from 2b610b2 to ccb43d8 Compare September 4, 2022 07:52

gerhardol marked this pull request as ready for review September 4, 2022 07:53

RussKie reviewed Sep 4, 2022

View reviewed changes

mstv reviewed Sep 4, 2022

View reviewed changes

GitCommands/Settings/AppSettings.cs Outdated Show resolved Hide resolved

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

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

gerhardol force-pushed the feature/i5371-stash-in-grid branch from d8af884 to d9df4d6 Compare September 7, 2022 21:35

mstv approved these changes Sep 11, 2022

View reviewed changes

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

RussKie reviewed Sep 12, 2022

View reviewed changes

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

gerhardol force-pushed the feature/i5371-stash-in-grid branch from 6bcf9eb to bdda77b Compare September 12, 2022 20:31

gerhardol merged commit ad6671d into gitextensions:master Sep 15, 2022

gerhardol deleted the feature/i5371-stash-in-grid branch September 15, 2022 19:37

ghost added this to the vNext milestone Sep 15, 2022

gerhardol modified the milestones: vNext, 4.0.0 Sep 17, 2022

gerhardol mentioned this pull request Oct 14, 2022

SidePanel: Show Stashes #10258

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Show Stash in rev grid #10138

Show Stash in rev grid #10138

gerhardol commented Aug 7, 2022 •

edited

mstv commented Aug 24, 2022 •

edited

gerhardol commented Aug 24, 2022 •

edited

gerhardol commented Aug 24, 2022

mstv commented Aug 25, 2022

gerhardol commented Aug 25, 2022

mstv commented Aug 25, 2022

RussKie commented Aug 28, 2022

gerhardol commented Sep 3, 2022

gerhardol commented Sep 4, 2022

RussKie left a comment

RussKie Sep 4, 2022

gerhardol Sep 4, 2022

RussKie Sep 4, 2022

gerhardol Sep 4, 2022

RussKie Sep 6, 2022

gerhardol Sep 6, 2022

gerhardol Sep 10, 2022

mstv commented Sep 5, 2022

mstv left a comment

gerhardol commented Sep 11, 2022

RussKie left a comment

gerhardol commented Sep 14, 2022

Show Stash in rev grid #10138

Show Stash in rev grid #10138

Conversation

gerhardol commented Aug 7, 2022 • edited

Proposed changes

Performance

Screenshots

Before

After

Test methodology

Test environment(s)

Merge strategy

mstv commented Aug 24, 2022 • edited

gerhardol commented Aug 24, 2022 • edited

gerhardol commented Aug 24, 2022

mstv commented Aug 25, 2022

gerhardol commented Aug 25, 2022

mstv commented Aug 25, 2022

RussKie commented Aug 28, 2022

gerhardol commented Sep 3, 2022

gerhardol commented Sep 4, 2022

RussKie left a comment

Choose a reason for hiding this comment

RussKie Sep 4, 2022

Choose a reason for hiding this comment

gerhardol Sep 4, 2022

Choose a reason for hiding this comment

RussKie Sep 4, 2022

Choose a reason for hiding this comment

gerhardol Sep 4, 2022

Choose a reason for hiding this comment

RussKie Sep 6, 2022

Choose a reason for hiding this comment

gerhardol Sep 6, 2022

Choose a reason for hiding this comment

gerhardol Sep 10, 2022

Choose a reason for hiding this comment

mstv commented Sep 5, 2022

mstv left a comment

Choose a reason for hiding this comment

gerhardol commented Sep 11, 2022

RussKie left a comment

Choose a reason for hiding this comment

gerhardol commented Sep 14, 2022

gerhardol commented Aug 7, 2022 •

edited

mstv commented Aug 24, 2022 •

edited

gerhardol commented Aug 24, 2022 •

edited