Skip to content

Refactored Network.Fetch() with FetchOptions #584

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Dec 12, 2013
Merged

Refactored Network.Fetch() with FetchOptions #584

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 13 additions & 7 deletions LibGit2Sharp.Tests/FetchFixture.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ public void CanFetchIntoAnEmptyRepository(string url)
}

// Perform the actual fetch
repo.Network.Fetch(remote, onUpdateTips: expectedFetchState.RemoteUpdateTipsHandler);
repo.Network.Fetch(remote, new FetchOptions { OnUpdateTips = expectedFetchState.RemoteUpdateTipsHandler });

// Verify the expected
expectedFetchState.CheckUpdatedReferences(repo);
Expand All @@ -62,11 +62,14 @@ public void CanFetchIntoAnEmptyRepositoryWithCredentials()
Remote remote = repo.Network.Remotes.Add(remoteName, Constants.PrivateRepoUrl);

// Perform the actual fetch
repo.Network.Fetch(remote, credentials: new Credentials
{
Username = Constants.PrivateRepoUsername,
Password = Constants.PrivateRepoPassword
});
repo.Network.Fetch(remote, new FetchOptions
{
Credentials = new Credentials
{
Username = Constants.PrivateRepoUsername,
Password = Constants.PrivateRepoPassword
}
});
}
}

Expand Down Expand Up @@ -94,7 +97,10 @@ public void CanFetchAllTagsIntoAnEmptyRepository(string url)
}

// Perform the actual fetch
repo.Network.Fetch(remote, TagFetchMode.All, onUpdateTips: expectedFetchState.RemoteUpdateTipsHandler);
repo.Network.Fetch(remote, new FetchOptions {
TagFetchMode = TagFetchMode.All,
OnUpdateTips = expectedFetchState.RemoteUpdateTipsHandler
});

// Verify the expected
expectedFetchState.CheckUpdatedReferences(repo);
Expand Down
4 changes: 2 additions & 2 deletions LibGit2Sharp.Tests/RepositoryFixture.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -186,13 +186,13 @@ public void CanFetchFromRemoteByName()
}

// Perform the actual fetch
repo.Fetch(remote.Name, onUpdateTips: expectedFetchState.RemoteUpdateTipsHandler);
repo.Fetch(remote.Name, new FetchOptions { OnUpdateTips = expectedFetchState.RemoteUpdateTipsHandler });

// Verify the expected state
expectedFetchState.CheckUpdatedReferences(repo);

// Now fetch the rest of the tags
repo.Fetch(remote.Name, tagFetchMode: TagFetchMode.All);
repo.Fetch(remote.Name, new FetchOptions { TagFetchMode = TagFetchMode.All });

// Verify that the "nearly-dangling" tag is now in the repo.
Tag nearlyDanglingTag = repo.Tags["nearly-dangling"];
Expand Down
46 changes: 46 additions & 0 deletions LibGit2Sharp/FetchOptions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
using LibGit2Sharp.Handlers;

namespace LibGit2Sharp
{
/// <summary>
/// Collection of parameters controlling Fetch behavior.
/// </summary>
public sealed class FetchOptions
{
/// <summary>
/// Specifies the tag-following behavior of the fetch operation.
/// <para>
/// If not set, the fetch operation will follow the default behavior for the <see cref="Remote"/>
/// based on the remote's <see cref="Remote.TagFetchMode"/> configuration.
/// </para>
/// <para>If neither this property nor the remote `tagopt` configuration is set,
/// this will default to <see cref="TagFetchMode.Auto"/> (i.e. tags that point to objects
/// retrieved during this fetch will be retrieved as well).</para>
/// </summary>
public TagFetchMode? TagFetchMode { get; set; }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Relates to my earlier comment about default value.

I'd rather make this non nullable and set a (documented) default value in the FetchOptions ctor.

/cc @carlosmn @jamill

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you're going to have a default value, it needs to be a new value that says "don't touch it". Anything else would override the user's configuration of remote.foo.autotag.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think @carlosmn is correct. We need a way to differentiate between "Not Set" and one of the values supported by LibGit2 (and a nullable value is one way to do that). The version in RepositoryExtensions looks like it would override the configured value - this should be corrected.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's keep the nullable enum value here. At least for now. Let's move the potential addition to TagFetchMode in a separate issue.
However, I think we should add some xml doc to this property explaining what null means regarding the Fetch process.

@carlosmn Any idea regarding how to formulate that?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd go for something like

If non-null, override the remote's tag-following behaviour to the given value for this fetch.

though there's probably a better way of saying "to the given value"

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some other possibilities:

Controls the tag-following behavior of the fetch operation. If not set, the fetch operation will follow the default behavior for the remote.

Or:

Specifies the tag-following behavior of the fetch operation. If not set, the fetch operation will follow the default behavior for the remote based on the remote's configuration.`

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, tagopt. Just like the function name :)

If nothing is set, then it's auto-follow.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I quite like @jamill's

<summary>
Specifies the tag-following behavior of the fetch operation.
<para>
If not set, the fetch operation will follow the default behavior for the remote
based on the remote's configuration.
</para>
</summary>

Would adding to this something like the following make sense?

If neither this property nor the remote `tagopt` configuration is set,
this will default to <see cref="TagFetchMode.Auto"/> (ie. tags that point to objects
retrieved during this fetch will be retrieved as well).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I quite like @jamill's

👍 Taken it.

Would adding to this something like the following make sense?

I think this would better fit in Remote.TagFetchMode, as it describes the default value for that property.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this would better fit in Remote.TagFetchMode, as it describes the default value for that property.

Remote.TagFetchMode is not nullable, so the returned value is always well known.

In this case, however, I think it may be worth describing how the fetch operation will behave when FetchOptions.TagFetchMode is unspecified and no remote.*.tagopt entry exists in the config.


/// <summary>
/// Delegate that progress updates of the network transfer portion of fetch
/// will be reported through.
/// </summary>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jamill IIRC you made Fetch() happen. Could you please help us make the documentation for OnProgress and OnUpdateTips a bit less dry?

public ProgressHandler OnProgress { get; set; }

/// <summary>
/// Delegate that updates of remote tracking branches will be reported through.
/// </summary>
public UpdateTipsHandler OnUpdateTips { get; set; }

/// <summary>
/// Callback method that transfer progress will be reported through.
/// <para>
/// Reports the client's state regarding the received and processed (bytes, objects) from the server.
/// </para>
/// </summary>
public TransferProgressHandler OnTransferProgress { get; set; }

/// <summary>
/// Credentials to use for username/password authentication.
/// </summary>
public Credentials Credentials { get; set; }
}
}
1 change: 1 addition & 0 deletions LibGit2Sharp/LibGit2Sharp.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,7 @@
<Compile Include="CommitFilter.cs" />
<Compile Include="CommitSortStrategies.cs" />
<Compile Include="CompareOptions.cs" />
<Compile Include="FetchOptions.cs" />
<Compile Include="RefSpec.cs" />
<Compile Include="RefSpecCollection.cs" />
<Compile Include="Core\EncodingMarshaler.cs" />
Expand Down
66 changes: 45 additions & 21 deletions LibGit2Sharp/Network.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,13 +96,21 @@ public virtual IEnumerable<DirectReference> ListReferences(string url)
}
}

static void DoFetch(RemoteSafeHandle remoteHandle, GitRemoteCallbacks gitCallbacks, TagFetchMode? tagFetchMode)
static void DoFetch(RemoteSafeHandle remoteHandle, FetchOptions options)
{
if (tagFetchMode.HasValue)
if (options == null)
{
Proxy.git_remote_set_autotag(remoteHandle, tagFetchMode.Value);
options = new FetchOptions();
}

if (options.TagFetchMode.HasValue)
{
Proxy.git_remote_set_autotag(remoteHandle, options.TagFetchMode.Value);
}

var callbacks = new RemoteCallbacks(options);
GitRemoteCallbacks gitCallbacks = callbacks.GenerateCallbacks();

// It is OK to pass the reference to the GitCallbacks directly here because libgit2 makes a copy of
// the data in the git_remote_callbacks structure. If, in the future, libgit2 changes its implementation
// to store a reference to the git_remote_callbacks structure this would introduce a subtle bug
Expand Down Expand Up @@ -135,22 +143,48 @@ static void DoFetch(RemoteSafeHandle remoteHandle, GitRemoteCallbacks gitCallbac
/// <param name="onTransferProgress">Callback method that transfer progress will be reported through.
/// Reports the client's state regarding the received and processed (bytes, objects) from the server.</param>
/// <param name="credentials">Credentials to use for username/password authentication.</param>
[Obsolete("This overload will be removed in the next release. Please use Fetch(Remote, FetchOptions) instead.")]
public virtual void Fetch(
Remote remote,
TagFetchMode? tagFetchMode = null,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't there something fishy here? This defaults to null here and in FetchOptions, but defaults to TagFetchMode.Auto in RepositoryExtensions..

/cc @carlosmn @jamill

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like a bug in RepositoryExtensions.cs:251. As @carlosmn pointed out below, Auto is not necessarily the default tag fetch mode for a remote.

ProgressHandler onProgress = null,
UpdateTipsHandler onUpdateTips = null,
TransferProgressHandler onTransferProgress = null,
Credentials credentials = null)
{
Fetch(remote, new FetchOptions
{
TagFetchMode = tagFetchMode,
OnProgress = onProgress,
OnUpdateTips = onUpdateTips,
OnTransferProgress = onTransferProgress,
Credentials = credentials
});
}

/// <summary>
/// Fetch from the <see cref="Remote"/>.
/// </summary>
/// <param name="remote">The remote to fetch</param>
public virtual void Fetch(Remote remote)
{
// This overload is required as long as the obsolete overload exists.
// Otherwise, Fetch(Remote) is ambiguous.
Fetch(remote, (FetchOptions)null);
}

/// <summary>
/// Fetch from the <see cref="Remote"/>.
/// </summary>
/// <param name="remote">The remote to fetch</param>
/// <param name="options"><see cref="FetchOptions"/> controlling fetch behavior</param>
public virtual void Fetch(Remote remote, FetchOptions options = null)
{
Ensure.ArgumentNotNull(remote, "remote");

using (RemoteSafeHandle remoteHandle = Proxy.git_remote_load(repository.Handle, remote.Name, true))
{
var callbacks = new RemoteCallbacks(onProgress, onTransferProgress, onUpdateTips, credentials);
GitRemoteCallbacks gitCallbacks = callbacks.GenerateCallbacks();

DoFetch(remoteHandle, gitCallbacks, tagFetchMode);
DoFetch(remoteHandle, options);
}
}

Expand All @@ -159,30 +193,20 @@ public virtual void Fetch(
/// </summary>
/// <param name="url">The url to fetch from</param>
/// <param name="refspecs">The list of resfpecs to use</param>
/// <param name="tagFetchMode">Optional parameter indicating what tags to download.</param>
/// <param name="onProgress">Progress callback. Corresponds to libgit2 progress callback.</param>
/// <param name="onUpdateTips">UpdateTips callback. Corresponds to libgit2 update_tips callback.</param>
/// <param name="onTransferProgress">Callback method that transfer progress will be reported through.
/// Reports the client's state regarding the received and processed (bytes, objects) from the server.</param>
/// <param name="credentials">Credentials to use for username/password authentication.</param>
/// <param name="options"><see cref="FetchOptions"/> controlling fetch behavior</param>
public virtual void Fetch(
string url,
IEnumerable<string> refspecs,
TagFetchMode? tagFetchMode = null,
ProgressHandler onProgress = null,
UpdateTipsHandler onUpdateTips = null,
TransferProgressHandler onTransferProgress = null,
Credentials credentials = null)
FetchOptions options = null)
{
Ensure.ArgumentNotNull(url, "url");
Ensure.ArgumentNotNull(refspecs, "refspecs");

using (RemoteSafeHandle remoteHandle = Proxy.git_remote_create_inmemory(repository.Handle, null, url))
{
Proxy.git_remote_set_fetch_refspecs(remoteHandle, refspecs);
var callbacks = new RemoteCallbacks(onProgress, onTransferProgress, onUpdateTips, credentials);
GitRemoteCallbacks gitCallbacks = callbacks.GenerateCallbacks();

DoFetch(remoteHandle, gitCallbacks, tagFetchMode);
DoFetch(remoteHandle, options);
}
}

Expand Down
9 changes: 9 additions & 0 deletions LibGit2Sharp/RemoteCallbacks.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,15 @@ internal RemoteCallbacks(
Credentials = credentials;
}

internal RemoteCallbacks(FetchOptions fetchOptions)
{
Ensure.ArgumentNotNull(fetchOptions, "fetchOptions");
Progress = fetchOptions.OnProgress;
DownloadTransferProgress = fetchOptions.OnTransferProgress;
UpdateTips = fetchOptions.OnUpdateTips;
Credentials = fetchOptions.Credentials;
}

#region Delegates

/// <summary>
Expand Down
38 changes: 36 additions & 2 deletions LibGit2Sharp/RepositoryExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -246,6 +246,7 @@ public static Commit Commit(this IRepository repository, string message, Signatu
/// <param name="onTransferProgress">Callback method that transfer progress will be reported through.
/// Reports the client's state regarding the received and processed (bytes, objects) from the server.</param>
/// <param name="credentials">Credentials to use for username/password authentication.</param>
[Obsolete("This overload will be removed in the next release. Please use Fetch(Remote, FetchOptions) instead.")]
public static void Fetch(this IRepository repository, string remoteName,
TagFetchMode tagFetchMode = TagFetchMode.Auto,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'll stick with this cough debatable cough default value. Once the method is dropped, callers will have to align to the new behavior (whatever it'll be 😉).

ProgressHandler onProgress = null,
Expand All @@ -257,8 +258,41 @@ public static void Fetch(this IRepository repository, string remoteName,
Ensure.ArgumentNotNullOrEmptyString(remoteName, "remoteName");

Remote remote = repository.Network.Remotes.RemoteForName(remoteName, true);
repository.Network.Fetch(remote, tagFetchMode, onProgress, onUpdateTips,
onTransferProgress, credentials);
repository.Network.Fetch(remote, new FetchOptions
{
TagFetchMode = tagFetchMode,
OnProgress = onProgress,
OnUpdateTips = onUpdateTips,
OnTransferProgress = onTransferProgress,
Credentials = credentials
});
}

/// <summary>
/// Fetch from the specified remote.
/// </summary>
/// <param name="repository">The <see cref="Repository"/> being worked with.</param>
/// <param name="remoteName">The name of the <see cref="Remote"/> to fetch from.</param>
public static void Fetch(this IRepository repository, string remoteName)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about dropping this overload...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

...And replacing the call in FetchHeadFixture.cs with the following?

repo.Network.Fetch(repo.Network.Remotes["origin"]);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't you think that some production code is using Repository.Fetch(string)? We would break that without notice.

We would have to keep this hack in Network.Fetch(Remote) anyway, otherwise that statement would now work, either. In the end, when the obsolete overloads are removed, we can drop them both.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you update the "obsoleted" method to call the updated API (e.g. create the FetchOptions object from the input parameters)? That way you would not need a separate 2 parameter method.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That does not help. A call to Fetch(string) would still be ambiguous, because there is a Fetch(string, [TagFetchMode], ...) and a Fetch(string, [FetchOptions]) overload. The compiler does not know which one to pick, because both accept a single string argument, and neither of them gets precedence. Only a Fetch(string) overload is favored over these two, and eliminates the error.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Duh. Let's keep it that way, then.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, yes - that is correct!

{
// This overload is required as long as the obsolete overload exists.
// Otherwise, Fetch(string) is ambiguous.
Fetch(repository, remoteName, (FetchOptions)null);
}

/// <summary>
/// Fetch from the specified remote.
/// </summary>
/// <param name="repository">The <see cref="Repository"/> being worked with.</param>
/// <param name="remoteName">The name of the <see cref="Remote"/> to fetch from.</param>
/// <param name="options"><see cref="FetchOptions"/> controlling fetch behavior</param>
public static void Fetch(this IRepository repository, string remoteName, FetchOptions options = null)
{
Ensure.ArgumentNotNull(repository, "repository");
Ensure.ArgumentNotNullOrEmptyString(remoteName, "remoteName");

Remote remote = repository.Network.Remotes.RemoteForName(remoteName, true);
repository.Network.Fetch(remote, options);
}

/// <summary>
Expand Down