Use `IGitModule` in signatures #11269

RussKie · 2023-10-12T22:39:18Z

Proposed changes

This PR removes the direct dependency on the concrete implementation of GitModule and replaces all callsites with the IGitModule interface. The change is somewhat crude to made the code compile. IGitModule interface has been updated with the required signature (almost) without any consideration on the quality of the API it provides.

This PR is about setting the foundation for using the abstractions, and the follow up work will concentrate on individual aspects such as

cleaning (or "beautifying") the interface,
adding xml-docs,
moving the abstractions to GitExtensions.Extensibility project and introducing the dependency on the said project, etc.

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 · 2023-10-14T14:33:19Z

GitCommands/Config/ConfigFile.cs

@@ -4,7 +4,7 @@

 namespace GitCommands.Config
 {
-    public class ConfigFile
+    public class ConfigFile : ISubmodulesConfigFile


❓ I read this as "Every config file is a submodules config file". This feels wrong.
The interface ISubmodulesConfigFile does not contain anything submodule-specific at a first glance.

It is the data in .gitmodules
The interface needs some comments...

I have added IConfigFile: ISubmodulesConfigFile and synchronized the method name GetSubmodulesConfigFile with the interface name in c3418e3 (after resolving the MC).

RussKie · 2023-10-15T02:16:58Z

This PR is about removing direct use of GitModule implementation and using IGitModule interface instead. So this PR will create some churn.

I found (and keep finding) implementations in GitModule that either have no real relation to git module or have a single callsite.
This PR isn't meant to make everything perfect, this is a mere first step to getting our head around what GitModule contains. It will also enable us to have a discussion what should be on IGitModule interface and what should be moved to other interfaces (e.g., submodule related implementations can probably be all grouped together under an interface).

Plugins/GitUIPluginInterfaces/ISubmodulesConfigFile.cs

RussKie · 2023-11-18T06:50:47Z

GitUI/CommandsDialogs/FormClone.cs

+            CancellationToken cancellationToken = _branchLoaderSequence.Next();
+            ThreadHelper.JoinableTaskFactory.RunAsync(async () =>
+            {
+                await TaskScheduler.Default;
+
+                RemoteActionResult<IReadOnlyList<IGitRef>> branchList;
+
+                IReadOnlyList<IGitRef> refs = Module.GetRemoteServerRefs(from, false, true, out string? errorOutput, cancellationToken);
+
+                if (string.IsNullOrEmpty(errorOutput))
+                {
+                    branchList = new(result: refs, authenticationFail: false, hostKeyFail: false);
+                }
+                else if (errorOutput.Contains("FATAL ERROR") && errorOutput.Contains("authentication"))
+                {
+                    // If the authentication failed because of a missing key, ask the user to supply one.
+                    branchList = new(result: null, authenticationFail: true, hostKeyFail: false);
+                }
+                else if (errorOutput.Contains("the server's host key is not cached in the registry", StringComparison.InvariantCultureIgnoreCase))
+                {
+                    branchList = new(result: null, authenticationFail: false, hostKeyFail: true);
+                }
+                else
+                {
+                    throw new NotImplementedException();
+                }
+
+                await this.SwitchToMainThreadAsync(cancellationToken);
+                if (!cancellationToken.IsCancellationRequested)
+                {
+                    UpdateBranches(branchList);
+                }
+            }).FileAndForget();


Moved the implementation from GitModule.

RussKie · 2023-11-18T06:57:47Z

I believe this is largely ready for review and for being taken in.

RussKie · 2023-11-22T06:38:35Z

Friendly ping @gerhardol @mstv @pmiossec

mstv

I need to continue the review of the middle commit "Compile" yet.
No comments on the first and third commits.

BugReporter/Info/GeneralInfo.cs

mstv · 2023-11-22T23:58:26Z

GitCommands/Config/ConfigFile.cs

@@ -4,7 +4,7 @@

 namespace GitCommands.Config
 {
-    public class ConfigFile
+    public class ConfigFile : ISubmodulesConfigFile


I have added IConfigFile: ISubmodulesConfigFile and synchronized the method name GetSubmodulesConfigFile with the interface name in c3418e3 (after resolving the MC).

that return the checked out branch or the commit hash if detached HEAD. + a little less misleading help + Invert `GetSelectedBranch()` boolean param logic from `setDefaultIfEmpty` to `emptyIfDetached`

mstv

I force pushed by mistake because I forgot to update the tracking reference.

GitCommands/Git/SubmoduleHelpers.cs

GitUI/CommandsDialogs/FormClone.cs

GitUI/CommitInfo/CommitInfo.cs

GitUI/GitUICommands.cs

RussKie · 2023-11-23T22:36:24Z

I force pushed by mistake because I forgot to update the tracking reference.

All good

RussKie · 2023-11-23T23:04:28Z

@mstv I believe I address the feedback. Thank you for your reviews.

GitUI/CommandsDialogs/FormClone.cs

GitCommands/Git/SubmoduleHelpers.cs

gerhardol

+1 only brief review

GitCommands/Git/SubmoduleHelpers.cs

GitCommands/Git/GitModule.cs

mstv · 2023-11-26T10:50:22Z

RussKie force-pushed

still MC

RussKie · 2023-11-26T11:00:02Z

I won't be able to look at it until tomorrow

RussKie · 2023-11-26T23:05:22Z

Thank you @mstv for rebasing this and getting the build green again.

Is there anything else outstanding that I missed? Or can we get this merged?

gerhardol · 2023-11-26T23:08:05Z

Or can we get this merged?

fine for me

ghost assigned RussKie Oct 12, 2023

RussKie changed the title ~~Use IGitModule `in signatures~~ Use IGitModule in signatures Oct 12, 2023

mstv reviewed Oct 14, 2023

View reviewed changes

This comment was marked as resolved.

Sign in to view

mstv reviewed Oct 15, 2023

View reviewed changes

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

RussKie mentioned this pull request Nov 2, 2023

Provide IServiceProvider to FormStatus #11320

Merged

RussKie force-pushed the use_IGitModule_in_signatures branch 2 times, most recently from 4354a2d to 90ed824 Compare November 18, 2023 05:58

RussKie commented Nov 18, 2023

View reviewed changes

RussKie requested a review from pmiossec November 18, 2023 06:57

RussKie marked this pull request as ready for review November 18, 2023 06:57

RussKie added status: ready 👓 status: needs review labels Nov 22, 2023

This comment was marked as resolved.

Sign in to view

mstv reviewed Nov 23, 2023

View reviewed changes

RussKie mentioned this pull request Nov 23, 2023

Update for GE 4.2 compatibility gitextensions/GitExtensions.GerritPlugin#82

Merged

10 tasks

RussKie referenced this pull request Nov 23, 2023

Scripts: add a new {HEAD} option

3d66936

that return the checked out branch or the commit hash if detached HEAD. + a little less misleading help + Invert `GetSelectedBranch()` boolean param logic from `setDefaultIfEmpty` to `emptyIfDetached`

mstv force-pushed the use_IGitModule_in_signatures branch from 7eef38d to cf5b521 Compare November 23, 2023 21:04

mstv reviewed Nov 23, 2023

View reviewed changes

GitCommands/Git/SubmoduleHelpers.cs Outdated Show resolved Hide resolved

GitUI/CommandsDialogs/FormClone.cs Outdated Show resolved Hide resolved

GitUI/CommandsDialogs/FormClone.cs Show resolved Hide resolved

GitUI/CommitInfo/CommitInfo.cs Show resolved Hide resolved

mstv reviewed Nov 23, 2023

View reviewed changes

GitUI/GitUICommands.cs Show resolved Hide resolved

mstv mentioned this pull request Nov 23, 2023

Remove redundant GitUICommands.GitModule #11386

Closed

RussKie force-pushed the use_IGitModule_in_signatures branch from cf5b521 to e5e5e38 Compare November 23, 2023 23:03

RussKie force-pushed the use_IGitModule_in_signatures branch from e5e5e38 to 2b1ad24 Compare November 24, 2023 01:13

RussKie mentioned this pull request Nov 24, 2023

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

Merged

mstv reviewed Nov 24, 2023

View reviewed changes

GitUI/CommandsDialogs/FormClone.cs Outdated Show resolved Hide resolved

GitUI/CommandsDialogs/FormClone.cs Outdated Show resolved Hide resolved

GitCommands/Git/SubmoduleHelpers.cs Outdated Show resolved Hide resolved

gerhardol reviewed Nov 25, 2023

View reviewed changes

GitCommands/Git/SubmoduleHelpers.cs Outdated Show resolved Hide resolved

GitCommands/Git/GitModule.cs Show resolved Hide resolved

GitCommands/Git/GitModule.cs Show resolved Hide resolved

RussKie force-pushed the use_IGitModule_in_signatures branch 2 times, most recently from 4d754ac to 7773123 Compare November 26, 2023 08:43

RussKie added 3 commits November 26, 2023 12:39

Update signatures

5edd1c1

Compile

0897e76

Update namespaces

be47ed9

mstv force-pushed the use_IGitModule_in_signatures branch from 7773123 to be47ed9 Compare November 26, 2023 11:45

mstv approved these changes Nov 27, 2023

View reviewed changes

RussKie merged commit 4d609aa into gitextensions:master Nov 27, 2023

RussKie deleted the use_IGitModule_in_signatures branch November 27, 2023 00:45

ghost added this to the vNext milestone Nov 27, 2023

RussKie added release-notes-worthy and removed 👓 status: needs review status: ready labels Nov 27, 2023

RussKie mentioned this pull request Dec 6, 2023

Follow file names in merge commits and all refs #11275

Merged

RussKie mentioned this pull request Mar 23, 2024

Restore splitter positions early in the layout process for FormCommit #11646

Merged

RussKie mentioned this pull request May 25, 2024

Use IGitUICommands in signatures #11752

Merged

Uh oh!

Use IGitModule in signatures #11269

Use IGitModule in signatures #11269

Uh oh!

Conversation

RussKie commented Oct 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed changes

Merge strategy

Uh oh!

mstv Oct 14, 2023

Choose a reason for hiding this comment

Uh oh!

gerhardol Oct 14, 2023

Choose a reason for hiding this comment

Uh oh!

mstv Nov 22, 2023

Choose a reason for hiding this comment

Uh oh!

This comment was marked as resolved.

RussKie commented Oct 15, 2023

Uh oh!

Uh oh!

RussKie Nov 18, 2023

Choose a reason for hiding this comment

Uh oh!

RussKie commented Nov 18, 2023

Uh oh!

RussKie commented Nov 22, 2023

Uh oh!

This comment was marked as resolved.

mstv left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mstv Nov 22, 2023

Choose a reason for hiding this comment

Uh oh!

mstv left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

RussKie commented Nov 23, 2023

Uh oh!

RussKie commented Nov 23, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gerhardol left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mstv commented Nov 26, 2023

Uh oh!

RussKie commented Nov 26, 2023 via email • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RussKie commented Nov 26, 2023

Uh oh!

gerhardol commented Nov 26, 2023

Uh oh!

Uh oh!

Use `IGitModule` in signatures #11269

Use `IGitModule` in signatures #11269

RussKie commented Oct 12, 2023 •

edited

Loading

RussKie commented Nov 26, 2023 via email •

edited by ghost

Loading