Update libgit2 to 3f8d005

[ethomson]

Updating libgit2 to 0a62918. The current version in HEAD (0383fa) had an unfortunate bug updating the tree cache (libgit2/libgit2#2629) that can cause some issues.

[Therzok]

Also change the build scripts to not set DTHREADSAFE, since it's ON by default now.

[ethomson]

Do we want Remotes.Rename to silently ignore when the source does not exist? This is the current behavior, which seems odd to me.

If we want to keep this behavior, I'll have to make some changes to libgit2.

/cc @nulltoken

[jamill]

I would expect an error if we tried to rename a non-existing remote. Maybe there is a specific use case this test is covering?

[nulltoken]

I would expect an error if we tried to rename a non-existing remote. Maybe there is a specific use case this test is covering?

👍

[ethomson]

Whether we want to throw a reasonable error message or swallow that, we need a GIT_ENOTFOUND out of libgit2. See libgit2/libgit2#2686

[ethomson]

Okay, with the inclusion of libgit2/libgit2#2686, we can now throw a NotFoundException when trying to rename a nonexistent remote. I think this is ready for review.

[nulltoken]

How about string.Format("Remote '{0}' hasn't been renamed as it doesn't exist.", name)?

[ethomson]

Sure - I changed the text ever so slightly (to lead with the nonexistent
ref), but let me know if you prefer your text.

On Mon, Nov 3, 2014 at 6:35 PM, nulltoken notifications@github.com wrote:

In LibGit2Sharp/Core/Proxy.cs:

                 {

•                    int res = NativeMethods.git_remote_rename(
  

•                        ref array.Array,
  

•                        remote,
  

•                        new_name);
  

•                    throw new NotFoundException("The given remote was not found");
  

How about string.Format("Remote '{0}' hasn't been renamed as it doesn't
exist.", name)?

—
Reply to this email directly or view it on GitHub
https://github.com/libgit2/libgit2sharp/pull/857/files#r19775923.

[nulltoken]

Well, the message proposal can surely be improved. But having the name of the missing remote in it would help 😉

[nulltoken]

My previous comment was referring to my proposal. Yours is just perfect!

[dahlbyk]

Shouldn't this go through an [Obsolete] cycle?

[ethomson]

It's gone in libgit2, so we'd have to fake this in libgit2sharp to do that, or revert the change in libgit2 to keep it obsolete for a while.

[nulltoken]

Unfortunately, it's no longer exposed by libgit2.

[nulltoken]

cf. libgit2/libgit2#2644

[dahlbyk]

[Obsolete] with a trivial implementation (e.g. url != null && url.IndexOf(':') > 0)?

[nulltoken]

This simple temporary implementation wouldn't be enough to cover the test cases

        [Theory]
        [InlineData("git@github.com:org/repo.git", false)]
        [InlineData("git://site.com:80/org/repo.git", true)]
        [InlineData("ssh://user@host.com:80/org/repo.git", false)]
        [InlineData("http://site.com:80/org/repo.git", true)]
        [InlineData("https://github.com:80/org/repo.git", true)]
        [InlineData("ftp://site.com:80/org/repo.git", false)]
        [InlineData("ftps://site.com:80/org/repo.git", false)]
        [InlineData("file:///path/repo.git", true)]
        [InlineData("protocol://blah.meh/whatever.git", false)]

...but I think it'd a waste of time to make it more complex for the sake of maintaining an obsolete for this function.

I'm 👍 to kill it

[dahlbyk]

If it was deleted from lg2 in the first place because it was too restrictive, those test cases are wrong anyway. ¯(°_o)/¯