Form Rebase: Add rebase dependent branches `--update-refs` rebase option #11335

pmiossec · 2023-11-07T14:09:18Z

Fixes #11333

doc: https://github.blog/2022-10-03-highlights-from-git-2-38/#rebase-dependent-branches-with-update-refs

Linked to what have already been done with #10289

Screenshots

Before

(normal rebase form)

After

Test methodology

Manual

Test environment(s)

Git Extensions 33.33.33
Build a2d7046
Git 2.42.0.windows.2
Microsoft Windows NT 10.0.22621.0
.NET 6.0.21
DPI 96dpi (no scaling)
Portable: False
Microsoft.WindowsDesktop.App Versions

    Microsoft.WindowsDesktop.App 6.0.21 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]
    Microsoft.WindowsDesktop.App 7.0.10 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]

Merge strategy

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

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

pmiossec · 2023-11-07T14:14:52Z

FYI, this form would need some redesign to make some room to rebase options.
The options lines are flow layout panel and by default the new checkbox is hidden because it appears outside of the initial form size.
To be able to push the feature and not wait an undetermined time (as it already stayed one year on my personal branch), I have enabled the wrapping in the flow panel so that the option is always visible...(until the rework of the form is effective).

pmiossec · 2023-11-07T14:15:56Z

/cc @quinmars you could test it and give feedback.

mstv · 2023-11-07T23:11:16Z

GitCommands/Git/GitModule.cs

-            return EffectiveConfigFile.GetValue(setting);
-        }
+        public string GetEffectiveSetting(string setting) => EffectiveConfigFile.GetValue(setting);
+        public bool IsEffectiveSettingEnabled(string setting) => EffectiveConfigFile.IsEnabled(setting);


Suggested change

public bool IsEffectiveSettingEnabled(string setting) => EffectiveConfigFile.IsEnabled(setting);

public bool? GetEffectiveBoolSetting(string setting) => EffectiveConfigFile.GetBool(setting);

I'd like us to consider an opportunity to generalise and have something like:

public T GetEffectiveSetting<T>(string settingName) { ... } public T GetSetting<T>(string settingName) { ... }

GitCommands/Settings/ConfigFileSettings.cs

mstv · 2023-11-07T23:17:26Z

GitUI/Translation/English.xlf

@@ -5993,6 +5993,10 @@ Nothing to rebase.</source>
 </source>
        <target />
      </trans-unit>
+      <trans-unit id="checkBoxUpdateRefs.Text">
+        <source>Update dependent refs</source>


Add mnemonic?

mstv · 2023-11-07T23:18:28Z

GitUI/UserControls/RevisionGrid/RevisionGridControl.cs

@@ -2968,7 +2968,7 @@ private void LaunchRebase(string command)

            string rebaseCmd = Commands.Rebase(
                GetActualRevision(LatestSelectedRevision)?.FirstParentId?.ToString(), interactive: true, preserveMerges: false,
-                autosquash: false, autoStash: true, ignoreDate: false, committerDateIsAuthorDate: false, supportRebaseMerges: Module.GitVersion.SupportRebaseMerges);
+                autosquash: false, autoStash: true, ignoreDate: false, committerDateIsAuthorDate: false, supportRebaseMerges: Module.GitVersion.SupportRebaseMerges, updateRefs: false);


Make it the default argument value?

Can we please create a struct that contains all the parameters instead? Not only it'd be more maintainable; there's a threshold on a number of parameters (I don't know it by heart) after which every new parameter becomes an overhead.

@RussKie Not sure that I agree with this point of view (even if I understand that there are a lot of params due to the number of options of the git rebase command) because:

it's consistent with all the methods of the Commands class. I would say it's even the goal of this class to use this pattern where you have a param for each command option.

create a struct/record will have the same problem where you initialize a lot of values. You just add a level of indirection

it's acceptable because the command is used only 3 times and I don't it will increase in the future and that's not a call that we modify often. I think we can live with that.

@RussKie I don't really get what you means by overhead.
Is it a performance one? In this case it is not a problem because it is called once and waiting for human interaction.
Or cognitive one? If that's the case, we say that the limit is around 7 +/-2. But as it is a simple mapping, with a very simple logic and without interaction between the parameters, the code logic is very easy to understand and a much bigger number is acceptable.

GitCommands/Settings/ConfigFileSettings.cs

quinmars · 2023-11-08T08:34:30Z

@pmiossec, I have tested it an it works like a charme, thank you. I have a small suggestion. When I set updateRefs = true in the configuration, the checkbox is checked and disabled. That's not bad, but even better would be if the user could uncheck the checkbox to use --no-update-refs for this commit. I do not know, however, how difficult this would be to implement. Personally, I will not change the default setting, so for me, your solution is all I need!

pmiossec · 2023-11-08T11:01:41Z

That's not bad, but even better would be if the user could uncheck the checkbox to use --no-update-refs for this commit

@quinmars
I looked for that when I developed the feature and put a comment saying explicitly that there was no way to override the setting. I can't explained how I missed it (maybe the git pro documentation was not up to date at the time).
I will have to better handle it, you're right.

GitUI/CommandsDialogs/FormRebase.cs

pmiossec · 2023-11-09T13:37:50Z

Except the request on change on parameters number of Commands.Rebase(), I have made a new proposition for everything else.

Point of view?

RussKie

Looking better

GitCommands/Settings/ConfigFileSettings.cs

RussKie · 2023-11-13T08:39:03Z

GitCommands/Settings/ConfigFileSettings.cs

+            catch (Exception ex)
+            {
+                Debug.WriteLine($"Setting '{setting}': fail to convert value '{value}' into type '{targetType}'. Error: {ex}");
+                return null;


Suggested change

return null;

return default(T);

Why handle the exception at all?
If we try to get a setting using a wrong type, it is a bug or an invalid manipulation of the settings file

It was at least for when the setting was not set. But indeed it is better to handle it before. Done.

GitUI/CommandsDialogs/FormRebase.cs

GitUI/CommandsDialogs/SettingsDialog/Pages/GitConfigAdvancedSettingsPage.cs

GitUI/CommandsDialogs/FormRebase.cs

RussKie

Looking better

mstv · 2023-11-23T20:17:32Z

GitCommands/Settings/ConfigFileSettings.cs

+            catch (Exception ex)
+            {
+                Debug.WriteLine($"Setting '{setting}': fail to convert value '{value}' into type '{targetType}'. Error: {ex}");
+                return null;


Why handle the exception at all?
If we try to get a setting using a wrong type, it is a bug or an invalid manipulation of the settings file

GitCommands/Settings/ConfigFileSettings.cs

GitUI/CommandsDialogs/FormRebase.cs

RussKie · 2023-11-24T01:23:09Z

I'd like for this to go behind #11269.

GitCommands/Settings/ConfigFileSettings.cs

GitCommands/Git/GitModule.cs

gerhardol

I have been using n earlier version of this PR for some time. Very useful - you just have to remember to remove the update-ref when rebasing temp branches.
Implementation is fine for me. Compare the handling to #11369 (for Git settings).

pmiossec · 2023-12-03T14:26:31Z

Done.

mstv

just a few nits in addition to Igor's comments

GitCommands/Config/ConfigFile.cs

GitCommands/Git/Commands.Arguments.cs

GitUI/UserControls/RevisionGrid/RevisionGridControl.cs

GitCommands/Git/Commands.Arguments.cs

RussKie

There are few small outstanding items. Other than that 👍

RussKie · 2023-12-05T07:30:40Z

GitCommands/Settings/ConfigFileSettings.cs

+            catch (Exception ex)
+            {
+                Debug.WriteLine($"Setting '{setting}': fail to convert value '{value}' into type '{targetType}'. Error: {ex}");
+                throw new GitConfigFormatException($"Setting '{setting}': fail to convert value '{value}' into type '{targetType}'");


Gentle ping

RussKie · 2023-12-05T07:31:03Z

Plugins/GitUIPluginInterfaces/IGitModule.cs

+        /// <summary>
+        /// Get the config setting from git converted in an expected C# value type (bool, int, ...)
+        /// </summary>
+        /// <typeparam name="T">the expected type of the git setting.</typeparam>
+        /// <param name="setting">the git setting key</param>
+        /// <returns>
+        /// null if the settings is not set
+        /// the value converted in the <typeparamref name="T" /> type otherwise.
+        /// </returns>


Gentle ping

Plugins/GitUIPluginInterfaces/IGitModule.cs

Plugins/GitUIPluginInterfaces/ISettingsValueGetter.cs

GitCommands/Settings/ConfigFileSettings.cs

mstv · 2023-12-05T21:31:04Z

Plugins/GitUIPluginInterfaces/IGitModule.cs

@@ -156,8 +156,31 @@ public interface IGitModule
        Task<IReadOnlyList<Remote>> GetRemotesAsync();

        string GetSetting(string setting);
+
+#pragma warning disable CS1574 // XML comment has cref attribute that could not be resolved


Move GitConfigFormatException to Plugins/GitUIPluginInterfaces?

Good call - we can't have the upstream dependency refer to downstream implementations.

Plugins/GitUIPluginInterfaces/ISettingsValueGetter.cs

Plugins/GitUIPluginInterfaces/IGitModule.cs

GitCommands/Settings/ConfigFileSettings.cs

RussKie · 2023-12-06T01:33:21Z

Did I ever mention that building public API is extremely hard? 🤣

https://github.blog/2022-10-03-highlights-from-git-2-38/#rebase-dependent-branches-with-update-refs and depending on user "rebase.updateRefs" setting, let the user unselect it (that will happen "--no-update-refs" to the arguments) Fixes gitextensions#11333

RussKie · 2023-12-06T23:09:36Z

Thank you!

ghost assigned pmiossec Nov 7, 2023

mstv reviewed Nov 7, 2023

View reviewed changes

RussKie reviewed Nov 8, 2023

View reviewed changes

GitCommands/Settings/ConfigFileSettings.cs Show resolved Hide resolved

pmiossec force-pushed the update-refs_support branch from 1f0bd23 to 6703b46 Compare November 8, 2023 13:43

RussKie reviewed Nov 9, 2023

View reviewed changes

GitUI/CommandsDialogs/FormRebase.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 Nov 9, 2023

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

pmiossec force-pushed the update-refs_support branch from 6703b46 to 2303cb0 Compare November 9, 2023 13:33

RussKie reviewed Nov 13, 2023

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 Nov 13, 2023

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

pmiossec force-pushed the update-refs_support branch 2 times, most recently from 82769a8 to 15230e4 Compare November 23, 2023 12:02

mstv reviewed Nov 23, 2023

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 Nov 24, 2023

pmiossec force-pushed the update-refs_support branch from 15230e4 to 0653ca6 Compare November 24, 2023 08:27

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

pmiossec force-pushed the update-refs_support branch from 2365fc1 to 57b5e2f Compare November 24, 2023 09:40

mstv approved these changes Nov 24, 2023

View reviewed changes

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

RussKie reviewed Nov 25, 2023

View reviewed changes

GitCommands/Git/GitModule.cs 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 Nov 25, 2023

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

gerhardol approved these changes Nov 25, 2023

View reviewed changes

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

mstv reviewed Dec 3, 2023

View reviewed changes

pmiossec force-pushed the update-refs_support branch from 439018b to 25e2385 Compare December 4, 2023 09:00

RussKie reviewed Dec 5, 2023

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 Dec 5, 2023

pmiossec force-pushed the update-refs_support branch from 25e2385 to 15f4c1e Compare December 5, 2023 11:14

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

pmiossec force-pushed the update-refs_support branch 3 times, most recently from c565b3a to ee5ea8b Compare December 5, 2023 12:01

mstv reviewed Dec 5, 2023

View reviewed changes

RussKie reviewed Dec 6, 2023

View reviewed changes

Plugins/GitUIPluginInterfaces/ISettingsValueGetter.cs Outdated Show resolved Hide resolved

RussKie reviewed Dec 6, 2023

View reviewed changes

Plugins/GitUIPluginInterfaces/IGitModule.cs Outdated Show resolved Hide resolved

RussKie reviewed Dec 6, 2023

View reviewed changes

Plugins/GitUIPluginInterfaces/IGitModule.cs Outdated Show resolved Hide resolved

RussKie reviewed Dec 6, 2023

View reviewed changes

GitCommands/Settings/ConfigFileSettings.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 Dec 6, 2023

pmiossec and others added 3 commits December 6, 2023 09:01

Settings: refactor getting true/false settings

87f5771

Use RebaseOptions

99559d3

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

More user friendly error popup when setting conversion failed

4fe3ff9

pmiossec force-pushed the update-refs_support branch from 223c4bd to 4fe3ff9 Compare December 6, 2023 08:24

RussKie approved these changes Dec 6, 2023

View reviewed changes

RussKie merged commit 8ece7c4 into gitextensions:master Dec 6, 2023

ghost added this to the vNext milestone Dec 6, 2023

pmiossec deleted the update-refs_support branch December 7, 2023 07:47

mstv mentioned this pull request Feb 11, 2024

Option rebase.updateRefs from Rebase dialog #11577

Closed

	public bool IsEffectiveSettingEnabled(string setting) => EffectiveConfigFile.IsEnabled(setting);
	public bool? GetEffectiveBoolSetting(string setting) => EffectiveConfigFile.GetBool(setting);

Uh oh!

Form Rebase: Add rebase dependent branches --update-refs rebase option #11335

Form Rebase: Add rebase dependent branches --update-refs rebase option #11335

Uh oh!

Conversation

pmiossec commented Nov 7, 2023

Screenshots

Before

After

Test methodology

Test environment(s)

Merge strategy

Uh oh!

pmiossec commented Nov 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pmiossec commented Nov 7, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

quinmars commented Nov 8, 2023

Uh oh!

pmiossec commented Nov 8, 2023

Uh oh!

Uh oh!

pmiossec commented Nov 9, 2023

Uh oh!

RussKie left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mstv Nov 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

RussKie left a comment

Choose a reason for hiding this comment

Uh oh!

mstv Nov 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

RussKie commented Nov 24, 2023

Uh oh!

Uh oh!

Uh oh!

gerhardol left a comment

Choose a reason for hiding this comment

Uh oh!

pmiossec commented Dec 3, 2023

Uh oh!

mstv left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Form Rebase: Add rebase dependent branches `--update-refs` rebase option #11335

Form Rebase: Add rebase dependent branches `--update-refs` rebase option #11335

pmiossec commented Nov 7, 2023 •

edited

Loading

mstv Nov 23, 2023 •

edited

Loading

mstv Nov 23, 2023 •

edited

Loading