Adds a Url property to the RemoteUpdater, which allows the remote url to...

[sitereactor]

... be updated.

This is a PR for issue #788

Includes a single test case for verifying that the remote url was indeed changed.
Let me know if anything needs to be changed or corrected for this PR.

[Therzok]

Why not remote.Url?

[Therzok]

Actually, scrap that, don't even save the url locally. You have the remote saved so you can just do remote.Url everywhere.

[sitereactor]

Doh!! Yes, of course. Thanks 👍

[Therzok]

/cc @nulltoken Check my reviews. Is the RemoteSafeHandle immutable? Are changes that are done in native land nicely reflected over managed land? That's something I never thought to test.

[nulltoken]

Thinking about it, can one think of a use case where we'd need a getter for the Url in the RemoteUpdater? We don't have any for TagFetchMode and it doesn't seem to be missing.

[nulltoken]

Check my reviews.

You rock 💟

Is the RemoteSafeHandle immutable? Are changes that are done in native land nicely reflected over managed land? That's something I never thought to test.

The changes should indeed be reflected in unmanaged memory. Rule of thumb: we try to not keep hold of handles (except for major ones: repo, index, ...) in order to keep control of the memory allocation/release lifecycle.

However, in this case, caching the RemoteSafeHandle may not require complex code. As the user cannot create a RemoteUpdater by him/herself and we control the lifecycle of the type, we may indeed do this. I've created #804 to tackle the handle topic.

Let's keep this PR focused on adding the missing Url updation mechanism.

[dahlbyk]

Thinking about it, can one think of a use case where we'd need a getter for the Url in the RemoteUpdater? We don't have any for TagFetchMode and it doesn't seem to be missing.

I'd drop it.

While we're talking about RemoteUpdater changes, fetchRefSpecs and pushRefSpecs could be lazy.

[nulltoken]

fetchRefSpecs and pushRefSpecs could be lazy.

@dahlbyk Aren't they already?

[dahlbyk]

So they are. Carry on. 😇

[sitereactor]

Thanks for the feedback!
I've made the adjustments as per your suggestions and cleaned up the code a bit.

I'll grab #804 when this is merged 😉

[dahlbyk]

I see this remark exists on the RefSpecs properties, but not on TagFetchMode. I really don't know what it means... any existing Remote instances will not change, but the one returned from Remotes.Update(…) will. Thoughts, @nulltoken?

[nulltoken]

Good catch! I overlooked this in #567.

@sitereactor Could you please drop those <remarks/> where they appear?

[nulltoken]

@sitereactor Very cool PR! Once the last nitpick is tackled, could you please squash all those commits into one so we can get this merged?

Optionally, if you feel like it, you can drop the existing <remarks/> in a separate commit. 😉

[sitereactor]

@nulltoken I removed the <remarks/> and squashed the commits, but I still see the previous commits in the PR. Do I need to open up a new one or? ... sorry, first time squashing commits on a PR 😄

[nulltoken]

@sitereactor Hmmm. That didn't go as expected. Let's try to fix this.

First, make sure to fetch the latest tips from libgit2/libgit2sharp. This should update your local vNext the latest changes.

Then, given that your local checked out branch is update_remote_url_788.

The following should do the trick

# reset the HEAD to v0.19 without modifying your index nor your working directory
$ git reset --soft v0.19 

# Create one single commit with all your changes
$ git commit -m "Teach RemoteUpdater to set the Url"
[update_remote_url_788 61b712a] Teach RemoteUpdater to set the Url
 4 files changed, 50 insertions(+), 2 deletions(-)

# Rebase this commit onto the latest vNext
$ git rebase vNext update_remote_url_788
First, rewinding head to replay your work on top of it...
Applying: Teach RemoteUpdater to set the Url

$

Then a force push should dynamically update the PR.

[sitereactor]

Thanks @nulltoken that seemed to do the trick ⭐
Much appreciated 😄

[nulltoken]

@sitereactor ✨ ‼️