Update libgit2 to 3f8d005 #857
Conversation
|
Also change the build scripts to not set DTHREADSAFE, since it's ON by default now. |
|
Do we want If we want to keep this behavior, I'll have to make some changes to libgit2. /cc @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
force-pushed
the
ed/libgit2_update
branch
from
eca5b53 to
fe473b1
Compare
👍 |
|
Whether we want to throw a reasonable error message or swallow that, we need a |
ethomson
force-pushed
the
ed/libgit2_update
branch
2 times, most recently
from
5538ab6 to
1c4e13e
Compare
|
Okay, with the inclusion of libgit2/libgit2#2686, we can now throw a |
LibGit2Sharp/Core/Proxy.cs
Outdated
| ref array.Array, | ||
| remote, | ||
| new_name); | ||
| throw new NotFoundException("The given remote was not found"); |
There was a problem hiding this comment.
How about string.Format("Remote '{0}' hasn't been renamed as it doesn't exist.", name)?
There was a problem hiding this comment.
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 [email protected] 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.
There was a problem hiding this comment.
Well, the message proposal can surely be improved. But having the name of the missing remote in it would help 😉
There was a problem hiding this comment.
My previous comment was referring to my proposal. Yours is just perfect!
| /// </summary> | ||
| /// <param name="url">The URL to be checked.</param> | ||
| /// <returns>true if the url is supported; false otherwise.</returns> | ||
| public static bool IsSupportedUrl(string url) |
There was a problem hiding this comment.
Shouldn't this go through an [Obsolete] cycle?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Unfortunately, it's no longer exposed by libgit2.
There was a problem hiding this comment.
[Obsolete] with a trivial implementation (e.g. url != null && url.IndexOf(':') > 0)?
There was a problem hiding this comment.
This simple temporary implementation wouldn't be enough to cover the test cases
[Theory]
[InlineData("[email protected]:org/repo.git", false)]
[InlineData("git://site.com:80/org/repo.git", true)]
[InlineData("ssh://[email protected]: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
There was a problem hiding this comment.
If it was deleted from lg2 in the first place because it was too restrictive, those test cases are wrong anyway. ¯(°_o)/¯
ethomson
force-pushed
the
ed/libgit2_update
branch
2 times, most recently
from
2597ae8 to
4d76d49
Compare
ethomson
force-pushed
the
ed/libgit2_update
branch
from
4d76d49 to
ed519c5
Compare
5 participants
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.