Improve rebase onto commit selection #10879

pmiossec · 2023-04-13T15:30:39Z

Improvement over #9686

Proposed changes

Hide stashes in revision grid when selecting commit range (now that we display all the stashes in the revision grid)
Select first parent which is a better default selection. So if the user want to rebase only one commit (which is not so rare), he only has to validate the selection...

Screenshots

Before

We see stashes and the selection is on the tip of the branch:

After

Stashes are not displayed and the 2nd commit of the branch is selected

Test methodology

Manual

Test environment(s)

Git Extensions 33.33.33
Build 74ad450
Git 2.40.0.windows.1
Microsoft Windows NT 10.0.22621.0
.NET 6.0.15
DPI 96dpi (no scaling)

Merge strategy

Rebase merge.

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

mstv · 2023-04-13T20:25:24Z

GitUI/CommandsDialogs/FormRebase.cs


            try
            {
-                using FormChooseCommit chooseForm = new(UICommands, txtFrom.Text, showCurrentBranchOnly: true);
+                AppSettings.ShowStashes = false;
+                ObjectId firstParent = UICommands.GitModule.RevParse(Currentbranch.Text + "~");


Does this also work for detached head?

yes, failure to parse will give null (no error popup)

I have a little changed that to improve when in detached head and have the same behavior (even if I really don't see the case of rebasing in detached head)

mstv · 2023-04-13T20:25:44Z

GitUI/CommandsDialogs/FormRebase.cs

-                using FormChooseCommit chooseForm = new(UICommands, txtFrom.Text, showCurrentBranchOnly: true);
+                AppSettings.ShowStashes = false;
+                ObjectId firstParent = UICommands.GitModule.RevParse(Currentbranch.Text + "~");
+                var preSelectedCommit = string.IsNullOrEmpty(txtFrom.Text) ? firstParent?.ToString() ?? string.Empty : txtFrom.Text;


Suggested change

var preSelectedCommit = string.IsNullOrEmpty(txtFrom.Text) ? firstParent?.ToString() ?? string.Empty : txtFrom.Text;

string preSelectedCommit = string.IsNullOrEmpty(txtFrom.Text) ? firstParent?.ToString() ?? string.Empty : txtFrom.Text;

gerhardol

+1

gerhardol · 2023-04-13T21:27:57Z

GitUI/CommandsDialogs/FormRebase.cs


            try
            {
-                using FormChooseCommit chooseForm = new(UICommands, txtFrom.Text, showCurrentBranchOnly: true);
+                AppSettings.ShowStashes = false;
+                ObjectId firstParent = UICommands.GitModule.RevParse(Currentbranch.Text + "~");


Side note: This should be queried from RevGrid to avoid extra Git commands. It is Git access that slows down the app...

I disagree in this case.
In this control we don't have access to the revision grid and I had a look, it will add a lot of logic to be able to pass the needed information as the form browse is called in multiple case (rebase, interactive, rebase onto, ....). So more than 4 or 6 different cases to handle.

And for the performance, doing a revparse is very quick (around 40ms) which is much less than requesting the logs to display the revision grid and opening the form. And also the goal of this feature is to make the use more convenient when you want to rebase onto only one commit and in this case, you probably gain 2 or 3s. So the net gain should be good 😉

gerhardol · 2023-04-13T21:33:31Z

GitUI/CommandsDialogs/FormRebase.cs


            try
            {
-                using FormChooseCommit chooseForm = new(UICommands, txtFrom.Text, showCurrentBranchOnly: true);
+                AppSettings.ShowStashes = false;
+                ObjectId firstParent = UICommands.GitModule.RevParse(Currentbranch.Text + "~");


yes, failure to parse will give null (no error popup)

RussKie

Modulo the other comments

RussKie · 2023-04-14T07:40:36Z

GitUI/CommandsDialogs/FormRebase.cs

-                using FormChooseCommit chooseForm = new(UICommands, txtFrom.Text, showCurrentBranchOnly: true);
+                AppSettings.ShowStashes = false;
+                ObjectId firstParent = UICommands.GitModule.RevParse("HEAD~");
+                string preSelectedCommit = string.IsNullOrEmpty(txtFrom.Text) ? firstParent?.ToString() ?? string.Empty : txtFrom.Text;


Text value can't be null, but it can whitespace. So the check should be IsNullOrWhitespace.
Also suggest to invert the condition as it'd read easier.

due to how rebase onto is working (the selected revision is excluded from the rebase --as it is the "base") So if the user want to rebase only one commit (which is not so rare), he only has to validate the selection...

vbjay · 2023-04-14T11:55:01Z

If there isn't a toggle to take off the first parent then I don't think it's a good idea. That will upset a lot of people if they can't select a mid work commit. If a visible toggle is there to allow them to turn it off, then that would be ok.

pmiossec · 2023-04-14T16:28:14Z

If there isn't a toggle to take off the first parent then I don't think it's a good idea. That will upset a lot of people if they can't select a mid work commit. If a visible toggle is there to allow them to turn it off, then that would be ok.

@vbjay I absolutely don' t understand you point of view.
This feature prevent no one to select the commit he wants. That's just that a better commit is pre-selected (because it makes no sense for the user to select the 1st as base) and so in the best case the user has just to validate. Wort case, he just has to select the commit he wants as base commit.
The only thing that could happen to the user is that he economize 1 click and some seconds...

vbjay · 2023-04-14T22:03:12Z

If there isn't a toggle to take off the first parent then I don't think it's a good idea. That will upset a lot of people if they can't select a mid work commit. If a visible toggle is there to allow them to turn it off, then that would be ok.

@vbjay I absolutely don' t understand you point of view.
This feature prevent no one to select the commit he wants. That's just that a better commit is pre-selected (because it makes no sense for the user to select the 1st as base) and so in the best case the user has just to validate. Wort case, he just has to select the commit he wants as base commit.
The only thing that could happen to the user is that he economize 1 click and some seconds...

Select first parent along with commit grid after image sounded like he set show first parents.

mstv · 2023-04-14T22:09:45Z

Select first parent along with commit grid after image sounded like he set show first parents.

Yes, but only for short time. :)

vbjay · 2023-04-14T22:47:18Z

How do you know they didn't want to rebase in-between 2 first parents? Forcing them to only select between 1st parents without giving them the option to turn it off is my problem with it.

pmiossec · 2023-04-14T22:53:14Z

How do you know they didn't want to rebase in-between 2 first parents? Forcing them to only select between 1st parents without giving them the option to turn it off is my problem with it.

In 10 years of using GE, I never tried to do that so it should not happen too often.
So in this case, either they use #10885 or they copy/paste the hash from revision grid should be enough....

vbjay · 2023-04-14T22:58:21Z

You are not the only user of ge. All I'm saying is if you are going to only show first parents in the select commit dialog, then show the first parents togle button on dialog to make it clear and to allow the user to have control. I don't mind first parents. I do have a problem with it being forced.

pmiossec · 2023-04-14T23:10:56Z

All I'm saying is if you are going to only show first parents in the select commit dialog

If fact I don't know why you start a discussion on a false subject....
The PR is absolutely not about that (it's just hide stash and preselect another commit)
And looking at the code, what you could have easily done, we do not enable the "view first parent" feature.
So everything is ok....

vbjay · 2023-04-14T23:18:37Z

All I'm saying is if you are going to only show first parents in the select commit dialog

If fact I don't know why you start a discussion on a false subject....
The PR is absolutely not about that (it's just hide stash and preselect another commit)
And looking at the code, what you could have easily done, we do not enable the "view first parent" feature.
So everything is ok....

Awesome. It was the description and the image that made me think the select dialog had been altered to only show first parents. I could see a ton of people who use that dialog getting very upset over not being able to select a non 1st parent. Having it automatically go down one commit so we don't have to is another thing entirely. The image made me think it was the first thing. So I made sure.

Rebase: hide stashes in revision grid when selecting commit range

3634fb8

ghost assigned pmiossec Apr 13, 2023

mstv reviewed Apr 13, 2023

View reviewed changes

gerhardol reviewed Apr 13, 2023

View reviewed changes

RussKie approved these changes Apr 13, 2023

View reviewed changes

pmiossec force-pushed the improve_rebase_onto_commit_selection branch from 74ad450 to 4370d91 Compare April 14, 2023 07:13

RussKie reviewed Apr 14, 2023

View reviewed changes

RussKie approved these changes Apr 14, 2023

View reviewed changes

FormRebase: select first parent which is a better default selection

400d6e9

due to how rebase onto is working (the selected revision is excluded from the rebase --as it is the "base") So if the user want to rebase only one commit (which is not so rare), he only has to validate the selection...

pmiossec force-pushed the improve_rebase_onto_commit_selection branch from 4370d91 to 400d6e9 Compare April 14, 2023 10:03

mstv approved these changes Apr 14, 2023

View reviewed changes

pmiossec merged commit 9d97624 into gitextensions:release/4.1 Apr 15, 2023
3 checks passed

pmiossec deleted the improve_rebase_onto_commit_selection branch April 15, 2023 08:54

RussKie added this to the 4.1 milestone Apr 16, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve rebase onto commit selection #10879

Improve rebase onto commit selection #10879

pmiossec commented Apr 13, 2023

mstv Apr 13, 2023

gerhardol Apr 13, 2023

pmiossec Apr 14, 2023

mstv Apr 13, 2023

gerhardol left a comment

gerhardol Apr 13, 2023

pmiossec Apr 14, 2023

gerhardol Apr 13, 2023

RussKie left a comment

RussKie Apr 14, 2023

vbjay commented Apr 14, 2023 via email •

edited by ghost

pmiossec commented Apr 14, 2023

vbjay commented Apr 14, 2023 •

edited

mstv commented Apr 14, 2023

vbjay commented Apr 14, 2023

pmiossec commented Apr 14, 2023

vbjay commented Apr 14, 2023 •

edited

pmiossec commented Apr 14, 2023 •

edited

vbjay commented Apr 14, 2023

	var preSelectedCommit = string.IsNullOrEmpty(txtFrom.Text) ? firstParent?.ToString() ?? string.Empty : txtFrom.Text;
	string preSelectedCommit = string.IsNullOrEmpty(txtFrom.Text) ? firstParent?.ToString() ?? string.Empty : txtFrom.Text;

Improve rebase onto commit selection #10879

Improve rebase onto commit selection #10879

Conversation

pmiossec commented Apr 13, 2023

Proposed changes

Screenshots

Before

After

Test methodology

Test environment(s)

Merge strategy

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

gerhardol left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vbjay commented Apr 14, 2023 via email • edited by ghost

pmiossec commented Apr 14, 2023

vbjay commented Apr 14, 2023 • edited

mstv commented Apr 14, 2023

vbjay commented Apr 14, 2023

pmiossec commented Apr 14, 2023

vbjay commented Apr 14, 2023 • edited

pmiossec commented Apr 14, 2023 • edited

vbjay commented Apr 14, 2023

vbjay commented Apr 14, 2023 via email •

edited by ghost

vbjay commented Apr 14, 2023 •

edited

vbjay commented Apr 14, 2023 •

edited

pmiossec commented Apr 14, 2023 •

edited