refactor: Management of remote repositories

Remote repositories are accessible on a several forms, however the experience is quite inconsistent.
The management of the remotes is quite poor too, UX is confusing and buggy.
In the original implementation the UI and the BL concerns intertwined which makes it difficult to maintain, extend and test.

* The new implementation moves the functionality concerning dealing with the remotes repositories into a
separate controller abstracting the data management from the UI.
* The controller enables testing
* The controller also simplifies the remotes management (FormRemotes). The "remote repositories" tab's layout has also been reworked and updated.
* All affected forms now need to instantiate a controller, load the remotes and bind them to the desired UI control (i.e. to a combo).

The following reported bugs are fixed:
- gitextensions#2550 - Remote Repositories > Separate Push URL not working (gitextensions#2550)
- gitextensions#1465 - Tab Stop Order in Remote Repositories dialog is very messed up (gitextensions#1465)

The following change was undone, as it introduced quite inconsistent and confusing UX:
- gitextensions#1208 - Rearrange buttons in "Manage Remote Repositories" dialog (gitextensions#1208)

Translations:
The following items added in FormRemotes:
- lblMgtPuttyPanelHeader.Text
- _gitMessage.Text
- _gbMgtPanelHeaderNew.Text
- _gbMgtPanelHeaderEdit.Text
- lblMgtDetailsPanelHeader.Text
The following items removed from FormRemotes:
- New.Text
- Delete.Text
- buttonClose.Text
- PuTTYSSH.Text
- _hintDelete.Text
- groupBox1.Text

GitCommands/Config/SettingKeyString.cs

@@ -5,26 +5,36 @@ namespace GitCommands.Config
     /// Defines the strings to access certain git config settings.
     /// Goal is to eliminate duplicate string constants in the code.
     /// </summary>
-    public class SettingKeyString
+    public static class SettingKeyString
     {
+        /// <summary>
+        /// "remote.{0}.push"
+        /// </summary>
+        public static string RemotePush = "remote.{0}.push";
+
         /// <summary>
         /// "remote.{0}.pushurl"
         /// </summary>
-        public const string RemotePushUrl = "remote.{0}.pushurl";
+        public static string RemotePushUrl = "remote.{0}.pushurl";
 
         /// <summary>
         /// "remote.{0}.url"
         /// </summary>
-        public const string RemoteUrl = "remote.{0}.url";
+        public static string RemoteUrl = "remote.{0}.url";
+
+        /// <summary>
+        /// "remote.{0}.puttykeyfile"
+        /// </summary>
+        public static string RemotePuttySshKey = "remote.{0}.puttykeyfile";
 
         /// <summary>
         /// user.name
         /// </summary>
-        public const string UserName = "user.name";
+        public static string UserName = "user.name";
 
         /// <summary>
         /// user.email
         /// </summary>
-        public const string UserEmail = "user.email";
+        public static string UserEmail = "user.email";
     }
 }

GitCommands/Git/GitRef.cs

@@ -1,7 +1,7 @@
 using System;
 using System.Collections.Generic;
-using GitCommands.Config;
 using GitCommands.Settings;
+using GitUIPluginInterfaces;
 
 namespace GitCommands
 {
@@ -22,12 +22,12 @@ public class GitRef : IGitItem
         /// <summary>"^{}"</summary>
         public static readonly string TagDereferenceSuffix = "^{}";
 
-        public GitModule Module { get; private set; }
+        public IGitModule Module { get; private set; }
 
-        public GitRef(GitModule module, string guid, string completeName)
+        public GitRef(IGitModule module, string guid, string completeName)
             : this(module, guid, completeName, string.Empty) { }
 
-        public GitRef(GitModule module, string guid, string completeName, string remote)
+        public GitRef(IGitModule module, string guid, string completeName, string remote)
         {
             Module = module;
             Guid = guid;

GitExtensionsTest/GitExtensionsTest.csproj

@@ -100,6 +100,7 @@
     <Compile Include="GitUI\FollowParentRewriterFixture.cs" />
     <Compile Include="GitUI\FormUpdateFixture.cs" />
     <Compile Include="GitUI\HotkeySettingsManagerFixture.cs" />
+    <Compile Include="GitUI\Objects\GitRemoteControllerTests.cs" />
     <Compile Include="GitUI\SingleHtmlUserManualFixture.cs" />
     <Compile Include="GitUI\StandardHtmlUserManualFixture.cs" />
     <Compile Include="Plugins\ReleaseNotesGenerator\MainFixture.cs" />

GitExtensionsTest/GitUI/Objects/GitRemoteControllerTests.cs

@@ -0,0 +1,178 @@
+using System;
+using System.Collections.Generic;
+using System.Linq;
+using System.Text;
+using FluentAssertions;
+using GitCommands.Config;
+using GitUI.Objects;
+using GitUIPluginInterfaces;
+using NSubstitute;
+using NUnit.Framework;
+
+namespace GitExtensionsTest.GitUI.Objects
+{
+    [TestFixture]
+    class GitRemoteControllerTests
+    {
+        private IGitModule _module;
+        private IGitRemoteController _controller;
+
+
+        [SetUp]
+        public void Setup()
+        {
+            _module = Substitute.For<IGitModule>();
+
+            _controller = new GitRemoteController(_module);
+        }
+
+
+        [Test]
+        public void LoadRemotes_should_not_throw_if_module_is_null()
+        {
+            _module = null;
+
+            ((Action)(() => _controller.LoadRemotes())).ShouldNotThrow();
+        }
+
+        [Test]
+        public void LoadRemotes_should_not_populate_remotes_if_none()
+        {
+            _module.GetRemotes().Returns(x => Enumerable.Empty<string>());
+
+            _controller.LoadRemotes();
+
+            _controller.Remotes.Count.Should().Be(0);
+            _module.Received(1).GetRemotes();
+            _module.DidNotReceive().GetSetting(Arg.Any<string>());
+            _module.DidNotReceive().GetSettings(Arg.Any<string>());
+        }
+
+        [Test]
+        public void LoadRemotes_should_not_populate_remotes_if_those_are_null_or_whitespace()
+        {
+            _module.GetRemotes().Returns(x => new[] { null, "", " ", "    ", "\t" });
+
+            _controller.LoadRemotes();
+
+            _controller.Remotes.Count.Should().Be(0);
+            _module.Received(1).GetRemotes();
+            _module.DidNotReceive().GetSetting(Arg.Any<string>());
+            _module.DidNotReceive().GetSettings(Arg.Any<string>());
+        }
+
+        [Test]
+        public void LoadRemotes_should_populate_remotes_if_any()
+        {
+            const string remote = "a";
+            _module.GetRemotes().Returns(x => new[] { null, "", " ", "    ", remote, "\t" });
+
+            _controller.LoadRemotes();
+
+            _controller.Remotes.Count.Should().Be(1);
+            _module.Received(1).GetRemotes();
+            _module.Received(1).GetSetting(string.Format(SettingKeyString.RemoteUrl, remote));
+            _module.Received(1).GetSetting(string.Format(SettingKeyString.RemotePushUrl, remote));
+            _module.Received(1).GetSetting(string.Format(SettingKeyString.RemotePuttySshKey, remote));
+            _module.Received(1).GetSettings(string.Format(SettingKeyString.RemotePush, remote));
+        }
+
+        [Test]
+        public void RemoveRemote_should_throw_if_remote_is_null()
+        {
+            ((Action)(() => _controller.RemoveRemote(null))).ShouldThrow<ArgumentNullException>()
+                .WithMessage("Value cannot be null.\r\nParameter name: remote");
+        }
+
+        [Test]
+        public void RemoveRemote_success()
+        {
+            var remote = new GitRemote { Name = "bla" };
+
+            _controller.RemoveRemote(remote);
+
+            _module.Received(1).RemoveRemote(remote.Name);
+        }
+
+        [Test]
+        public void SaveRemote_should_throw_if_remoteName_is_null()
+        {
+            ((Action)(() => _controller.SaveRemote(null, null, "b", "c", "d"))).ShouldThrow<ArgumentNullException>()
+                .WithMessage("Value cannot be null.\r\nParameter name: remoteName");
+        }
+
+        [Test]
+        public void SaveRemote_null_remote_should_invoke_AddRemote_and_require_update()
+        {
+            const string remoteName = "a";
+            const string remoteUrl = "b";
+            const string output = "yes!";
+            _module.AddRemote(Arg.Any<string>(), Arg.Any<string>()).Returns(x => output);
+
+            var result = _controller.SaveRemote(null, remoteName, remoteUrl, null, null);
+
+            result.UserMessage.Should().Be(output);
+            result.ShouldUpdateRemote.Should().BeTrue();
+            _module.Received(1).AddRemote(remoteName, remoteUrl);
+        }
+
+        [Test]
+        public void SaveRemote_populated_remote_should_invoke_RenameRemote_if_remoteName_mismatch_no_update_required()
+        {
+            const string remoteName = "a";
+            const string remoteUrl = "b";
+            const string output = "yes!";
+            var gitRemote = new GitRemote { Name = "old", Url = remoteUrl };
+            _module.RenameRemote(Arg.Any<string>(), Arg.Any<string>()).Returns(x => output);
+
+            var result = _controller.SaveRemote(gitRemote, remoteName, remoteUrl, null, null);
+
+            result.UserMessage.Should().Be(output);
+            result.ShouldUpdateRemote.Should().BeFalse();
+            _module.Received(1).RenameRemote(gitRemote.Name, remoteName);
+        }
+
+        [Test]
+        public void SaveRemote_populated_remote_should_require_update_if_remoteUrl_mismatch()
+        {
+            const string remoteName = "a";
+            const string remoteUrl = "b";
+            const string output = "yes!";
+            var gitRemote = new GitRemote { Name = "old", Url = "old" };
+            _module.RenameRemote(Arg.Any<string>(), Arg.Any<string>()).Returns(x => output);
+
+            var result = _controller.SaveRemote(gitRemote, remoteName, remoteUrl, null, null);
+
+            result.UserMessage.Should().Be(output);
+            result.ShouldUpdateRemote.Should().BeTrue();
+            _module.Received(1).RenameRemote(gitRemote.Name, remoteName);
+        }
+
+        [TestCase(null, null, null)]
+        [TestCase("a", null, null)]
+        [TestCase("a", "b", null)]
+        [TestCase("a", "b", "c")]
+        public void SaveRemote_should_update_settings(string remoteUrl, string remotePushUrl, string remotePuttySshKey)
+        {
+            var remote = new GitRemote { Name = "bla", Url = remoteUrl };
+
+            _controller.SaveRemote(remote, remote.Name, remoteUrl, remotePushUrl, remotePuttySshKey);
+
+            Action<string, string> ensure = (setting, value) =>
+            {
+                setting = string.Format(setting, remote.Name);
+                if (!string.IsNullOrWhiteSpace(value))
+                {
+                    _module.Received(1).SetSetting(setting, value);
+                }
+                else
+                {
+                    _module.Received(1).UnsetSetting(setting);
+                }
+            };
+            ensure(SettingKeyString.RemoteUrl, remoteUrl);
+            ensure(SettingKeyString.RemotePushUrl, remotePushUrl);
+            ensure(SettingKeyString.RemotePuttySshKey, remotePuttySshKey);
+        }
+    }
+}

GitUI/CommandsDialogs/FormPull.cs

@@ -3,11 +3,13 @@
 using System.ComponentModel;
 using System.Diagnostics;
 using System.IO;
+using System.Linq;
 using System.Text.RegularExpressions;
 using System.Windows.Forms;
 using GitCommands;
 using GitCommands.Config;
 using GitCommands.Repository;
+using GitUI.Objects;
 using GitUI.Properties;
 using GitUI.Script;
 using GitUI.UserControls;
@@ -95,6 +97,8 @@ public partial class FormPull : GitModuleForm
         private IList<GitRef> _heads;
         private string _branch;
         private const string AllRemotes = "[ All ]";
+        private readonly IGitRemoteController _gitRemoteController;
+
 
         private FormPull()
             : this(null, null, null)
@@ -110,7 +114,10 @@ public FormPull(GitUICommands aCommands, string defaultRemoteBranch, string defa
             helpImageDisplayUserControl1.IsOnHoverShowImage2NoticeText = _hoverShowImageLabelText.Text;
 
             if (aCommands != null)
+            {
+                _gitRemoteController = new GitRemoteController(Module);
                 Init(defaultRemote);
+            }
 
             Merge.Checked = Settings.FormPullAction == Settings.PullAction.Merge;
             Rebase.Checked = Settings.FormPullAction == Settings.PullAction.Rebase;
@@ -135,40 +142,42 @@ public FormPull(GitUICommands aCommands, string defaultRemoteBranch, string defa
             }
         }
 
+
         private void Init(string defaultRemote)
         {
-            UpdateRemotesList();
-
             _branch = Module.GetSelectedBranch();
+            localBranch.Text = _branch;
+
+            BindRemotesDropDown(defaultRemote);
+        }
+
+        private void BindRemotesDropDown(string selectedRemoteName)
+        {
+            // refresh registered git remotes
+            _gitRemoteController.LoadRemotes();
+
+            _NO_TRANSLATE_Remotes.Sorted = false;
+            _NO_TRANSLATE_Remotes.DataSource = new[] { new GitRemote { Name = AllRemotes } }.Union(_gitRemoteController.Remotes).ToList();
+            _NO_TRANSLATE_Remotes.DisplayMember = "Name";
+            _NO_TRANSLATE_Remotes.SelectedIndex = -1;
+            ComboBoxHelper.ResizeComboBoxDropDownWidth(_NO_TRANSLATE_Remotes, AppSettings.BranchDropDownMinWidth, AppSettings.BranchDropDownMaxWidth);
 
-            string currentBranchRemote;
-            if (defaultRemote.IsNullOrEmpty())
+            var currentBranchRemote = _gitRemoteController.Remotes.FirstOrDefault(x => x.Name.Equals(selectedRemoteName, StringComparison.OrdinalIgnoreCase));
+            if (currentBranchRemote != null)
             {
-                currentBranchRemote = Module.GetSetting(string.Format("branch.{0}.remote", _branch));
+                _NO_TRANSLATE_Remotes.SelectedItem = currentBranchRemote;
             }
-            else
+            else if (_gitRemoteController.Remotes.Any())
             {
-                currentBranchRemote = defaultRemote;
+                // we couldn't find the default assigned remote for the selected branch
+                // it is usually gets mapped via FormRemotes -> "default pull behavior" tab
+                // so pick the default user remote
+                _NO_TRANSLATE_Remotes.SelectedIndex = 1;
             }
-
-            if (currentBranchRemote.IsNullOrEmpty() && _NO_TRANSLATE_Remotes.Items.Count >= 3)
+            else
             {
-                IList<string> remotes = (IList<string>)_NO_TRANSLATE_Remotes.DataSource;
-                int i = remotes.IndexOf("origin");
-                _NO_TRANSLATE_Remotes.SelectedIndex = i >= 0 ? i : 1;
+                _NO_TRANSLATE_Remotes.SelectedIndex = 0;
             }
-            else
-                _NO_TRANSLATE_Remotes.Text = currentBranchRemote;
-            localBranch.Text = _branch;
-        }
-
-        private void UpdateRemotesList()
-        {
-            IList<string> remotes = new List<string>(Module.GetRemotes());
-            remotes.Insert(0, AllRemotes);
-            _NO_TRANSLATE_Remotes.DataSource = remotes;
-
-            ComboBoxHelper.ResizeComboBoxDropDownWidth (_NO_TRANSLATE_Remotes, AppSettings.BranchDropDownMinWidth, AppSettings.BranchDropDownMaxWidth);
         }
 
         public DialogResult PullAndShowDialogWhenFailed(IWin32Window owner)
@@ -785,11 +794,7 @@ private void AddRemoteClick(object sender, EventArgs e)
 
             _bInternalUpdate = true;
             string origText = _NO_TRANSLATE_Remotes.Text;
-            UpdateRemotesList();
-            if (_NO_TRANSLATE_Remotes.Items.Contains(origText)) // else first item gets selected
-            {
-                _NO_TRANSLATE_Remotes.Text = origText;
-            }
+            BindRemotesDropDown(origText);
             _bInternalUpdate = false;
         }