From 3b308f47b62e98f97e72df3c2bb1f46cd948be1d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Mart=C3=ADn=20Nieto?= Date: Mon, 21 Mar 2016 12:49:25 +0100 Subject: [PATCH] Take a name instead of an instance to update a remote All the Remote instance does in RemoteUpdater is carry the name, as we need it in order to perform the update. Reflect this in the class fields as well as in the preferred way of calling it. While we're dealing with the updater, make it perform its actions inside a configuration transaction so the configuration does not change while we're processing updates. --- LibGit2Sharp.Tests/BranchFixture.cs | 4 +- LibGit2Sharp.Tests/FetchFixture.cs | 2 +- LibGit2Sharp.Tests/RefSpecFixture.cs | 101 +++++++++++++++------------ LibGit2Sharp.Tests/RemoteFixture.cs | 34 +++++---- LibGit2Sharp/Configuration.cs | 24 +++++++ LibGit2Sharp/Core/NativeMethods.cs | 9 +++ LibGit2Sharp/Core/Proxy.cs | 23 ++++++ LibGit2Sharp/RemoteCollection.cs | 20 ++++++ LibGit2Sharp/RemoteUpdater.cs | 34 ++++++--- 9 files changed, 178 insertions(+), 73 deletions(-) diff --git a/LibGit2Sharp.Tests/BranchFixture.cs b/LibGit2Sharp.Tests/BranchFixture.cs index 1fd943c00..a5c3f02b5 100644 --- a/LibGit2Sharp.Tests/BranchFixture.cs +++ b/LibGit2Sharp.Tests/BranchFixture.cs @@ -457,7 +457,7 @@ public void QueryUnresolvableRemoteForRemoteBranch() Remote remote = repo.Network.Remotes["origin"]; Assert.NotNull(remote); - repo.Network.Remotes.Update(remote, r => r.FetchRefSpecs = fetchRefSpecs); + repo.Network.Remotes.Update("origin", r => r.FetchRefSpecs = fetchRefSpecs); Branch branch = repo.Branches["refs/remotes/origin/master"]; @@ -750,7 +750,7 @@ public void SetTrackedBranchForUnreasolvableRemoteThrows() // cannot be resolved. Remote remote = repo.Network.Remotes["origin"]; Assert.NotNull(remote); - repo.Network.Remotes.Update(remote, r => r.FetchRefSpecs = fetchRefSpecs); + repo.Network.Remotes.Update("origin", r => r.FetchRefSpecs = fetchRefSpecs); // Now attempt to update the tracked branch Branch branch = repo.CreateBranch(testBranchName); diff --git a/LibGit2Sharp.Tests/FetchFixture.cs b/LibGit2Sharp.Tests/FetchFixture.cs index 99ca5bd50..0689bc47e 100644 --- a/LibGit2Sharp.Tests/FetchFixture.cs +++ b/LibGit2Sharp.Tests/FetchFixture.cs @@ -177,7 +177,7 @@ public void FetchRespectsConfiguredAutoTagSetting(TagFetchMode tagFetchMode, int Assert.NotNull(remote); // Update the configured autotag setting. - repo.Network.Remotes.Update(remote, + repo.Network.Remotes.Update(remoteName, r => r.TagFetchMode = tagFetchMode); // Perform the actual fetch. diff --git a/LibGit2Sharp.Tests/RefSpecFixture.cs b/LibGit2Sharp.Tests/RefSpecFixture.cs index b9ac369e9..b8f2b6a09 100644 --- a/LibGit2Sharp.Tests/RefSpecFixture.cs +++ b/LibGit2Sharp.Tests/RefSpecFixture.cs @@ -1,4 +1,5 @@ -using System.Linq; +using System.Collections.Generic; +using System.Linq; using LibGit2Sharp.Tests.TestHelpers; using Xunit; using Xunit.Extensions; @@ -75,25 +76,30 @@ public void CanReplaceRefSpecs(string[] newFetchRefSpecs, string[] newPushRefSpe var path = SandboxStandardTestRepo(); using (var repo = new Repository(path)) { - var remote = repo.Network.Remotes["origin"]; - var oldRefSpecs = remote.RefSpecs.ToList(); - - var newRemote = repo.Network.Remotes.Update(remote, - r => r.FetchRefSpecs = newFetchRefSpecs, r => r.PushRefSpecs = newPushRefSpecs); - - Assert.Equal(oldRefSpecs, remote.RefSpecs.ToList()); + List oldRefSpecs; + using (var remote = repo.Network.Remotes["origin"]) + { + oldRefSpecs = remote.RefSpecs.ToList(); - var actualNewFetchRefSpecs = newRemote.RefSpecs - .Where(s => s.Direction == RefSpecDirection.Fetch) - .Select(r => r.Specification) - .ToArray(); - Assert.Equal(newFetchRefSpecs, actualNewFetchRefSpecs); + repo.Network.Remotes.Update("origin", + r => r.FetchRefSpecs = newFetchRefSpecs, r => r.PushRefSpecs = newPushRefSpecs); + Assert.Equal(oldRefSpecs, remote.RefSpecs.ToList()); + } - var actualNewPushRefSpecs = newRemote.RefSpecs - .Where(s => s.Direction == RefSpecDirection.Push) - .Select(r => r.Specification) - .ToArray(); - Assert.Equal(newPushRefSpecs, actualNewPushRefSpecs); + using (var newRemote = repo.Network.Remotes["origin"]) + { + var actualNewFetchRefSpecs = newRemote.RefSpecs + .Where(s => s.Direction == RefSpecDirection.Fetch) + .Select(r => r.Specification) + .ToArray(); + Assert.Equal(newFetchRefSpecs, actualNewFetchRefSpecs); + + var actualNewPushRefSpecs = newRemote.RefSpecs + .Where(s => s.Direction == RefSpecDirection.Push) + .Select(r => r.Specification) + .ToArray(); + Assert.Equal(newPushRefSpecs, actualNewPushRefSpecs); + } } } @@ -105,15 +111,12 @@ public void RemoteUpdaterSavesRefSpecsPermanently() using (var repo = new Repository(path)) { - using (var remote = repo.Network.Remotes["origin"]) - { - repo.Network.Remotes.Update(remote, r => r.FetchRefSpecs = fetchRefSpecs); - } + repo.Network.Remotes.Update("origin", r => r.FetchRefSpecs = fetchRefSpecs); } using (var repo = new Repository(path)) + using (var remote = repo.Network.Remotes["origin"]) { - var remote = repo.Network.Remotes["origin"]; var actualRefSpecs = remote.RefSpecs .Where(r => r.Direction == RefSpecDirection.Fetch) .Select(r => r.Specification) @@ -129,22 +132,25 @@ public void CanAddAndRemoveRefSpecs() var path = SandboxStandardTestRepo(); using (var repo = new Repository(path)) - using (var remote = repo.Network.Remotes["origin"]) { - using (var updatedRemote = repo.Network.Remotes.Update(remote, + repo.Network.Remotes.Update("origin", r => r.FetchRefSpecs.Add(newRefSpec), - r => r.PushRefSpecs.Add(newRefSpec))) + r => r.PushRefSpecs.Add(newRefSpec)); + + using (var remote = repo.Network.Remotes["origin"]) + { + Assert.Contains(newRefSpec, remote.FetchRefSpecs.Select(r => r.Specification)); + Assert.Contains(newRefSpec, remote.PushRefSpecs.Select(r => r.Specification)); + } + + repo.Network.Remotes.Update("origin", + r => r.FetchRefSpecs.Remove(newRefSpec), + r => r.PushRefSpecs.Remove(newRefSpec)); + + using (var remote = repo.Network.Remotes["origin"]) { - Assert.Contains(newRefSpec, updatedRemote.FetchRefSpecs.Select(r => r.Specification)); - Assert.Contains(newRefSpec, updatedRemote.PushRefSpecs.Select(r => r.Specification)); - - using (var updatedRemote2 = repo.Network.Remotes.Update(updatedRemote, - r => r.FetchRefSpecs.Remove(newRefSpec), - r => r.PushRefSpecs.Remove(newRefSpec))) - { - Assert.DoesNotContain(newRefSpec, updatedRemote2.FetchRefSpecs.Select(r => r.Specification)); - Assert.DoesNotContain(newRefSpec, updatedRemote2.PushRefSpecs.Select(r => r.Specification)); - } + Assert.DoesNotContain(newRefSpec, remote.FetchRefSpecs.Select(r => r.Specification)); + Assert.DoesNotContain(newRefSpec, remote.PushRefSpecs.Select(r => r.Specification)); } } } @@ -155,18 +161,20 @@ public void CanClearRefSpecs() var path = SandboxStandardTestRepo(); using (var repo = new Repository(path)) { - var remote = repo.Network.Remotes["origin"]; // Push refspec does not exist in cloned repository - remote = repo.Network.Remotes.Update(remote, r => r.PushRefSpecs.Add("+refs/test:refs/test")); + repo.Network.Remotes.Update("origin", r => r.PushRefSpecs.Add("+refs/test:refs/test")); - remote = repo.Network.Remotes.Update(remote, + repo.Network.Remotes.Update("origin", r => r.FetchRefSpecs.Clear(), r => r.PushRefSpecs.Clear()); - Assert.Empty(remote.FetchRefSpecs); - Assert.Empty(remote.PushRefSpecs); - Assert.Empty(remote.RefSpecs); + using (var remote = repo.Network.Remotes["origin"]) + { + Assert.Empty(remote.FetchRefSpecs); + Assert.Empty(remote.PushRefSpecs); + Assert.Empty(remote.RefSpecs); + } } } @@ -183,11 +191,14 @@ public void SettingInvalidRefSpecsThrows(string refSpec) var path = SandboxStandardTestRepo(); using (var repo = new Repository(path)) { - var remote = repo.Network.Remotes["origin"]; - var oldRefSpecs = remote.RefSpecs.Select(r => r.Specification).ToList(); + IEnumerable oldRefSpecs; + using (var remote = repo.Network.Remotes["origin"]) + { + oldRefSpecs = remote.RefSpecs.Select(r => r.Specification).ToList(); + } Assert.Throws(() => - repo.Network.Remotes.Update(remote, r => r.FetchRefSpecs.Add(refSpec))); + repo.Network.Remotes.Update("origin", r => r.FetchRefSpecs.Add(refSpec))); var newRemote = repo.Network.Remotes["origin"]; Assert.Equal(oldRefSpecs, newRemote.RefSpecs.Select(r => r.Specification).ToList()); diff --git a/LibGit2Sharp.Tests/RemoteFixture.cs b/LibGit2Sharp.Tests/RemoteFixture.cs index ddd080410..28049f0e0 100644 --- a/LibGit2Sharp.Tests/RemoteFixture.cs +++ b/LibGit2Sharp.Tests/RemoteFixture.cs @@ -66,10 +66,13 @@ public void CanSetTagFetchMode(TagFetchMode tagFetchMode) Remote remote = repo.Network.Remotes[name]; Assert.NotNull(remote); - Remote updatedremote = repo.Network.Remotes.Update(remote, + repo.Network.Remotes.Update(name, r => r.TagFetchMode = tagFetchMode); - Assert.Equal(tagFetchMode, updatedremote.TagFetchMode); + using (var updatedremote = repo.Network.Remotes[name]) + { + Assert.Equal(tagFetchMode, updatedremote.TagFetchMode); + } } } @@ -87,12 +90,14 @@ public void CanSetRemoteUrl() Remote remote = repo.Network.Remotes[name]; Assert.NotNull(remote); - Remote updatedremote = repo.Network.Remotes.Update(remote, - r => r.Url = newUrl); + repo.Network.Remotes.Update(name, r => r.Url = newUrl); - Assert.Equal(newUrl, updatedremote.Url); - // with no push url set, PushUrl defaults to the fetch url - Assert.Equal(newUrl, updatedremote.PushUrl); + using (var updatedremote = repo.Network.Remotes[name]) + { + Assert.Equal(newUrl, updatedremote.Url); + // with no push url set, PushUrl defaults to the fetch url + Assert.Equal(newUrl, updatedremote.PushUrl); + } } } @@ -114,12 +119,14 @@ public void CanSetRemotePushUrl() Assert.Equal(url, remote.Url); Assert.Equal(url, remote.PushUrl); - Remote updatedremote = repo.Network.Remotes.Update(remote, - r => r.PushUrl = pushurl); + repo.Network.Remotes.Update(name, r => r.PushUrl = pushurl); - // url should not change, push url should be set to new value - Assert.Equal(url, updatedremote.Url); - Assert.Equal(pushurl, updatedremote.PushUrl); + using (var updatedremote = repo.Network.Remotes[name]) + { + // url should not change, push url should be set to new value + Assert.Equal(url, updatedremote.Url); + Assert.Equal(pushurl, updatedremote.PushUrl); + } } } @@ -292,8 +299,7 @@ public void ReportsRemotesWithNonDefaultRefSpecs() { Assert.NotNull(repo.Network.Remotes["origin"]); - repo.Network.Remotes.Update( - repo.Network.Remotes["origin"], + repo.Network.Remotes.Update("origin", r => r.FetchRefSpecs = new[] { "+refs/heads/*:refs/remotes/upstream/*" }); repo.Network.Remotes.Rename("origin", "nondefault", problem => Assert.Equal("+refs/heads/*:refs/remotes/upstream/*", problem)); diff --git a/LibGit2Sharp/Configuration.cs b/LibGit2Sharp/Configuration.cs index 63065841a..5a0aa03f3 100644 --- a/LibGit2Sharp/Configuration.cs +++ b/LibGit2Sharp/Configuration.cs @@ -759,5 +759,29 @@ private ConfigurationHandle Snapshot() { return Proxy.git_config_snapshot(configHandle); } + + /// + /// Perform a series of actions within a transaction. + /// + /// The configuration will be locked during this function and the changes will be committed at the end. These + /// changes will not be visible in the configuration until the end of this method. + /// + /// If the action throws an exception, the changes will be rolled back. + /// + /// The code to run under the transaction + public virtual unsafe void WithinTransaction(Action action) + { + IntPtr txn = IntPtr.Zero; + try + { + txn = Proxy.git_config_lock(configHandle); + action(); + Proxy.git_transaction_commit(txn); + } + finally + { + Proxy.git_transaction_free(txn); + } + } } } diff --git a/LibGit2Sharp/Core/NativeMethods.cs b/LibGit2Sharp/Core/NativeMethods.cs index 3e2f8825c..267df1998 100644 --- a/LibGit2Sharp/Core/NativeMethods.cs +++ b/LibGit2Sharp/Core/NativeMethods.cs @@ -342,6 +342,9 @@ internal static extern unsafe int git_config_delete_entry( git_config* cfg, [MarshalAs(UnmanagedType.CustomMarshaler, MarshalCookie = UniqueId.UniqueIdentifier, MarshalTypeRef = typeof(StrictUtf8Marshaler))] string name); + [DllImport(libgit2)] + internal static extern unsafe int git_config_lock(out IntPtr txn, git_config* config); + [DllImport(libgit2)] internal static extern unsafe int git_config_delete_multivar( git_config* cfg, @@ -1825,6 +1828,12 @@ internal static extern unsafe int git_treebuilder_insert( [DllImport(libgit2)] internal static extern unsafe int git_cherrypick(git_repository* repo, git_object* commit, GitCherryPickOptions options); + + [DllImport(libgit2)] + internal static extern int git_transaction_commit(IntPtr txn); + + [DllImport(libgit2)] + internal static extern void git_transaction_free(IntPtr txn); } } // ReSharper restore InconsistentNaming diff --git a/LibGit2Sharp/Core/Proxy.cs b/LibGit2Sharp/Core/Proxy.cs index b89e7d5ae..b8d4e11da 100644 --- a/LibGit2Sharp/Core/Proxy.cs +++ b/LibGit2Sharp/Core/Proxy.cs @@ -635,6 +635,15 @@ public static unsafe ConfigurationHandle git_config_snapshot(ConfigurationHandle return new ConfigurationHandle(handle, true); } + public static unsafe IntPtr git_config_lock(git_config* config) + { + IntPtr txn; + int res = NativeMethods.git_config_lock(out txn, config); + Ensure.ZeroResult(res); + + return txn; + } + #endregion #region git_cred_ @@ -3197,6 +3206,20 @@ public static unsafe ObjectId git_treebuilder_write(TreeBuilderHandle bld) #endregion + #region git_transaction_ + + public static void git_transaction_commit(IntPtr txn) + { + NativeMethods.git_transaction_commit(txn); + } + + public static void git_transaction_free(IntPtr txn) + { + NativeMethods.git_transaction_free(txn); + } + + #endregion + #region git_libgit2_ /// diff --git a/LibGit2Sharp/RemoteCollection.cs b/LibGit2Sharp/RemoteCollection.cs index 2d33ffd0f..fe750c390 100644 --- a/LibGit2Sharp/RemoteCollection.cs +++ b/LibGit2Sharp/RemoteCollection.cs @@ -53,6 +53,7 @@ internal Remote RemoteForName(string name, bool shouldThrowIfNotFound = true) /// The remote to update. /// Delegate to perform updates on the remote. /// The updated remote. + [Obsolete("This method is deprecated. Use the overload with a remote name")] public virtual Remote Update(Remote remote, params Action[] actions) { var updater = new RemoteUpdater(repository, remote); @@ -65,6 +66,25 @@ public virtual Remote Update(Remote remote, params Action[] actio return this[remote.Name]; } + /// + /// Update properties of a remote. + /// + /// These updates will be performed as a bulk update at the end of the method. + /// + /// The name of the remote to update. + /// Delegate to perform updates on the remote. + public virtual void Update(string remote, params Action[] actions) + { + var updater = new RemoteUpdater(repository, remote); + + repository.Config.WithinTransaction(() => { + foreach (Action action in actions) + { + action(updater); + } + }); + } + /// /// Returns an enumerator that iterates through the collection. /// diff --git a/LibGit2Sharp/RemoteUpdater.cs b/LibGit2Sharp/RemoteUpdater.cs index 14d5c82df..ec8b08bcd 100644 --- a/LibGit2Sharp/RemoteUpdater.cs +++ b/LibGit2Sharp/RemoteUpdater.cs @@ -14,7 +14,7 @@ public class RemoteUpdater private readonly UpdatingCollection fetchRefSpecs; private readonly UpdatingCollection pushRefSpecs; private readonly Repository repo; - private readonly Remote remote; + private readonly string remoteName; /// /// Needed for mocking purposes. @@ -28,7 +28,19 @@ internal RemoteUpdater(Repository repo, Remote remote) Ensure.ArgumentNotNull(remote, "remote"); this.repo = repo; - this.remote = remote; + this.remoteName = remote.Name; + + fetchRefSpecs = new UpdatingCollection(GetFetchRefSpecs, SetFetchRefSpecs); + pushRefSpecs = new UpdatingCollection(GetPushRefSpecs, SetPushRefSpecs); + } + + internal RemoteUpdater(Repository repo, string remote) + { + Ensure.ArgumentNotNull(repo, "repo"); + Ensure.ArgumentNotNull(remote, "remote"); + + this.repo = repo; + this.remoteName = remote; fetchRefSpecs = new UpdatingCollection(GetFetchRefSpecs, SetFetchRefSpecs); pushRefSpecs = new UpdatingCollection(GetPushRefSpecs, SetPushRefSpecs); @@ -36,7 +48,7 @@ internal RemoteUpdater(Repository repo, Remote remote) private IEnumerable GetFetchRefSpecs() { - using (RemoteHandle remoteHandle = Proxy.git_remote_lookup(repo.Handle, remote.Name, true)) + using (RemoteHandle remoteHandle = Proxy.git_remote_lookup(repo.Handle, remoteName, true)) { return Proxy.git_remote_get_fetch_refspecs(remoteHandle); } @@ -44,17 +56,17 @@ private IEnumerable GetFetchRefSpecs() private void SetFetchRefSpecs(IEnumerable value) { - repo.Config.UnsetMultivar(string.Format("remote.{0}.fetch", remote.Name), ConfigurationLevel.Local); + repo.Config.UnsetMultivar(string.Format("remote.{0}.fetch", remoteName), ConfigurationLevel.Local); foreach (var url in value) { - Proxy.git_remote_add_fetch(repo.Handle, remote.Name, url); + Proxy.git_remote_add_fetch(repo.Handle, remoteName, url); } } private IEnumerable GetPushRefSpecs() { - using (RemoteHandle remoteHandle = Proxy.git_remote_lookup(repo.Handle, remote.Name, true)) + using (RemoteHandle remoteHandle = Proxy.git_remote_lookup(repo.Handle, remoteName, true)) { return Proxy.git_remote_get_push_refspecs(remoteHandle); } @@ -62,11 +74,11 @@ private IEnumerable GetPushRefSpecs() private void SetPushRefSpecs(IEnumerable value) { - repo.Config.UnsetMultivar(string.Format("remote.{0}.push", remote.Name), ConfigurationLevel.Local); + repo.Config.UnsetMultivar(string.Format("remote.{0}.push", remoteName), ConfigurationLevel.Local); foreach (var url in value) { - Proxy.git_remote_add_push(repo.Handle, remote.Name, url); + Proxy.git_remote_add_push(repo.Handle, remoteName, url); } } @@ -75,7 +87,7 @@ private void SetPushRefSpecs(IEnumerable value) /// public virtual TagFetchMode TagFetchMode { - set { Proxy.git_remote_set_autotag(repo.Handle, remote.Name, value); } + set { Proxy.git_remote_set_autotag(repo.Handle, remoteName, value); } } /// @@ -83,7 +95,7 @@ public virtual TagFetchMode TagFetchMode /// public virtual string Url { - set { Proxy.git_remote_set_url(repo.Handle, remote.Name, value); } + set { Proxy.git_remote_set_url(repo.Handle, remoteName, value); } } /// @@ -91,7 +103,7 @@ public virtual string Url /// public virtual string PushUrl { - set { Proxy.git_remote_set_pushurl(repo.Handle, remote.Name, value); } + set { Proxy.git_remote_set_pushurl(repo.Handle, remoteName, value); } } ///