Refactor `LibGit2.set_remote_url` #22062

omus · 2017-05-25T20:45:38Z

Added new functions set_remote_fetch_url and set_remote_push_url which allow you to specify the fetch/push remote URLs independently. Additionally, I switched set_remote_url from using a the keyword remote to using an optional argument.

Fixes #21773

omus · 2017-05-25T20:46:41Z

base/libgit2/libgit2.jl

-            if push != url
-                set!(cfg, "remote.$remote.pushurl", push)
-            end
-        end


Caused #21773

StefanKarpinski · 2017-05-25T21:22:55Z

Feature or bugfix?

tkelman · 2017-05-25T22:21:06Z

looks like a little of both, plus a new deprecation

tkelman · 2017-05-26T02:13:17Z

base/libgit2/remote.jl

+repo_path = joinpath("test_directory", "Example")
+repo = LibGit2.init(repo_path)
+url1 = "https://github.com/JuliaLang/Example.jl"
+LibGit2.set_remote_url(repo, url1, remote="upstream")


bit misleading to name the argument at the call site if it's positional

I missed this when I moved away from a keyword.

tkelman · 2017-05-26T02:15:27Z

base/deprecated.jl

+    if remote_kwarg === nothing
+        return remote_arg
+    else
+        msg = "$f($sig; remote=\"$remote_kwarg\") is deprecated, use $f($sig, \"$remote_kwarg\")"


these usually end with "instead"

Ok. I modeled this off of the deprecate_negate function. Does the rest of this seem reasonable?

I wrote that one, and went through a couple refactors of it. it does say instead in it.

the argument order differs from the libgit2 c functions', but I guess that's okay. positionals always run the risk of "what order is it supposed to be" when writing or reading code that uses the method, so I'm neutral on this part of the change

I modeled this off of the deprecate_negate function. Does the rest of this seem reasonable?

I should have been more specific. I was wondering if the deprecate_remote_keyword function was reasonable.

the argument order differs from the libgit2 c functions'

Re-ordering the functions to be func(repo::GitRepo, remote::AbstractString, url::AbstractString) would more closely match both LibGit2 and the git command line. Personally I am fine with always having to specify the remote name.

tkelman · 2017-05-26T02:17:37Z

test/libgit2.jl

@@ -37,6 +37,15 @@ function with_libgit2_temp_home(f)
    end
 end

+function is_defined_remote(repo::LibGit2.GitRepo, remote::AbstractString)
+    try
+        LibGit2.get(LibGit2.GitRemote, repo, name)


can this be done without try-catching?

I'll look into this again tomorrow. I didn't find a LibGit2 method which seemed to cover this.

Made lookup_remote to address this

tkelman · 2017-05-26T02:28:16Z

test/libgit2.jl

+            @test !is_defined_remote(repo, name)
+
+            # Set just the fetch URL
+            LibGit2.set_remote_fetch_url(repo, url, name)


this is testing the repo::GitRepo versions pretty well, and the existing block was testing some of the path::AbstractString versions, but not the ones you're newly adding

I'll extend the tests to cover those.

omus · 2017-05-26T04:01:43Z

Feature or bugfix?

Definitely both. When addressing the bug I noticed that the GitConfig was being used to modify the remote rather than using the available LibGit2 functions. Using the config method is bad as it doesn't verify that things like the remote name is valid. After getting my hands into the code I noticed that the remote keyword was unnecessary which is how I ended up here.

omus · 2017-05-26T04:02:19Z

I'll add some docstrings to the new functions in addition to the review change requests.

StefanKarpinski · 2017-05-26T16:32:05Z

I guess what I was getting at was is this backportable to 0.6 or does it need to stay on master?

omus · 2017-05-26T17:16:57Z

I guess what I was getting at was is this backportable to 0.6 or does it need to stay on master?

I can separate the bugfix so it can be backported to 0.6.

omus · 2017-05-26T19:10:39Z

A note for the future @tkelman: Only backport commit d4a43e4

omus · 2017-06-05T03:18:54Z

Anything left to do here? Maybe a NEWS entry?

tkelman · 2017-06-05T17:33:01Z

base/libgit2/remote.jl

+function set_remote_push_url end
+
+function set_remote_push_url(repo::GitRepo, remote_name::AbstractString, url::AbstractString)
+@check ccall((:git_remote_set_pushurl, :libgit2), Cint,


under-indented

When using `LibGit2.set_remote_url` to specify a GitHub HTTP URL only the fetch URL would be modified to use HTTP and the push URL would always be set to SSH.

Changes include: - Creating `set_remote_fetch_url`, `set_remote_push_url`, `remote_delete`, and `lookup_remote` - Updating `set_remote_url` to use LibGit2 calls - Deprecate `set_remote_url` which uses keyword and reorder arguments to match order of `GitRemote` constructor. - Update all existing calls to `set_remote_url`

omus · 2017-06-06T16:00:15Z

Rebased to resolve conflict in "base/deprecated.jl"

omus · 2017-06-06T22:04:25Z

Ready for merge

tkelman · 2017-06-07T00:22:44Z

didn't add a news entry, were you going to?

omus · 2017-06-07T05:15:05Z

I wasn't planning on adding one but I will now.

[ci skip]

omus · 2017-06-07T14:36:42Z

NEWS entry added.

When using `LibGit2.set_remote_url` to specify a GitHub HTTP URL only the fetch URL would be modified to use HTTP and the push URL would always be set to SSH. (cherry picked from commit d4a43e4) partial backport of #22062

omus added the libgit2 The libgit2 library or the LibGit2 stdlib module label May 25, 2017

omus commented May 25, 2017

View reviewed changes

base/libgit2/libgit2.jl

if push != url

set!(cfg, "remote.$remote.pushurl", push)

end

end

Copy link

Member Author

omus May 25, 2017

Choose a reason for hiding this comment

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

Caused #21773

omus requested a review from kshyatt May 25, 2017 20:52

tkelman reviewed May 26, 2017

View reviewed changes

omus force-pushed the cv/set-remote branch from 55a2880 to 1afa37a Compare May 26, 2017 19:03

omus added the backport pending 0.6 label May 26, 2017

omus force-pushed the cv/set-remote branch 3 times, most recently from 29fb185 to 729f891 Compare May 29, 2017 15:41

tkelman reviewed Jun 5, 2017

View reviewed changes

omus force-pushed the cv/set-remote branch from 9b60841 to bb7d26d Compare June 5, 2017 18:14

omus added 2 commits June 6, 2017 10:44

Fix issue #21773

d4a43e4

When using `LibGit2.set_remote_url` to specify a GitHub HTTP URL only the fetch URL would be modified to use HTTP and the push URL would always be set to SSH.

omus force-pushed the cv/set-remote branch from bb7d26d to c5006be Compare June 6, 2017 15:58

Add NEWS entry

006743d

[ci skip]

omus merged commit 13c1830 into master Jun 7, 2017

omus deleted the cv/set-remote branch June 7, 2017 20:22

tkelman removed the backport pending 0.6 label Jun 20, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor `LibGit2.set_remote_url` #22062

Refactor `LibGit2.set_remote_url` #22062

omus commented May 25, 2017

omus May 25, 2017

StefanKarpinski commented May 25, 2017

tkelman commented May 25, 2017

tkelman May 26, 2017

omus May 26, 2017

omus May 26, 2017

tkelman May 26, 2017

omus May 26, 2017

tkelman May 26, 2017

omus May 26, 2017

omus May 26, 2017

omus May 26, 2017

tkelman May 26, 2017

omus May 26, 2017

omus May 26, 2017

tkelman May 26, 2017

omus May 26, 2017

omus May 26, 2017

omus commented May 26, 2017

omus commented May 26, 2017

StefanKarpinski commented May 26, 2017

omus commented May 26, 2017

omus commented May 26, 2017 •

edited

Loading

omus commented Jun 5, 2017

tkelman Jun 5, 2017

omus Jun 5, 2017

omus commented Jun 6, 2017

omus commented Jun 6, 2017

tkelman commented Jun 7, 2017

omus commented Jun 7, 2017

omus commented Jun 7, 2017

Refactor LibGit2.set_remote_url #22062

Refactor LibGit2.set_remote_url #22062

Conversation

omus commented May 25, 2017

Choose a reason for hiding this comment

StefanKarpinski commented May 25, 2017

tkelman commented May 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

omus commented May 26, 2017

omus commented May 26, 2017

StefanKarpinski commented May 26, 2017

omus commented May 26, 2017

omus commented May 26, 2017 • edited Loading

omus commented Jun 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

omus commented Jun 6, 2017

omus commented Jun 6, 2017

tkelman commented Jun 7, 2017

omus commented Jun 7, 2017

omus commented Jun 7, 2017

Refactor `LibGit2.set_remote_url` #22062

Refactor `LibGit2.set_remote_url` #22062

omus commented May 26, 2017 •

edited

Loading