upstream and lookup_branch throw if not found #20127

kshyatt · 2017-01-19T15:54:12Z

No description provided.

simonbyrne · 2017-01-19T16:12:01Z

base/libgit2/reference.jl

@@ -169,10 +169,8 @@ function lookup_branch(repo::GitRepo,
    err = ccall((:git_branch_lookup, :libgit2), Cint,


I asked on the libgit2 slack, and they said that if an allocating function returns an error then you don't need to free the resulting pointer. So you can just replace this by an @check ccall(...)

oh awesome I can fix my close pr then too :D

I will fix this in the other PR btw #20128

simonbyrne · 2017-01-19T16:15:28Z

base/libgit2/reference.jl

@@ -185,10 +183,8 @@ function upstream(ref::GitReference)
    ref_ptr_ptr = Ref{Ptr{Void}}(C_NULL)
    err = ccall((:git_branch_upstream, :libgit2), Cint,


and same here as well

tkelman · 2017-01-21T00:43:20Z

base/libgit2/libgit2.jl

@@ -207,7 +207,14 @@ function branch!(repo::GitRepo, branch_name::AbstractString,
                 force::Bool=false,           # force branch creation
                 set_head::Bool=true)         # set as head reference on exit
    # try to lookup branch first
-    branch_ref = force ? nothing : lookup_branch(repo, branch_name)


should the force argument still be used?

Oh yeah probably

tkelman · 2017-01-21T03:09:53Z

I think changing this to return a nullable would be preferable to throwing and forcing try-catches everywhere, unless there's a better way of asking in advance whether these lookup operations are going to succeed

simonbyrne · 2017-01-21T10:27:59Z

I think errors are okay. Most calls to these functions end up checking if it was an error and throwing otherwise (or they don't handle it at all and cause more problems).

Also, these functions are unlikely to be in tight loops, so the performance advantages of Nullable here are not so relevant.

tkelman · 2017-01-21T10:57:04Z

A try-catch shouldn't be the only way of asking if something exists.

simonbyrne · 2017-01-21T13:29:27Z

Agreed, but then we should have separate functions, at least until we've figured out a standard Julia idiom for this.

tkelman · 2017-01-25T21:12:00Z

base/libgit2/libgit2.jl

+        try
+            branch_ref = lookup_branch(repo, branch_name)
+        catch err
+            if !isa(err, LibGit2.GitError) && err.code != LibGit2.ENOTFOUND


shouldn't need this now?

simonbyrne · 2017-01-26T10:12:12Z

base/libgit2/reference.jl

            close(GitReference(repo, ref_ptr_ptr[]))
        end
        throw(Error.GitError(err))
    end
-    return GitReference(repo, ref_ptr_ptr[])
+    return Nullable{GitReference}(GitReference(repo, ref_ptr_ptr[]))
 end

 function upstream(ref::GitReference)


Should document the return value, since Nullable is a bit unexpected. Or maybe call it tryupstream?

tkelman · 2017-01-26T18:20:34Z

base/libgit2/libgit2.jl

-    branch_ref = force ? nothing : lookup_branch(repo, branch_name)
-    if branch_ref === nothing
-        branch_rmt_ref = isempty(track) ? nothing : lookup_branch(repo, "$track/$branch_name", true)
+    branch_ref = Nullable{GitReference}(force ? nothing : lookup_branch(repo, branch_name))


what does lookup_branch return here?

if it returns a not-found Nullable, does this end up wrapping it in two layers of Nullable or one?

Oh nice, good catch

tkelman · 2017-01-26T18:23:26Z

base/libgit2/reference.jl

+    if err != Int(Error.GIT_OK)
+        if err == Int(Error.ENOTFOUND)
+            return Nullable{GitReference}()
+        elseif err != Int(Error.ENOTFOUND) && ref_ptr_ptr[] != C_NULL


it can't be ENOTFOUND since it would have just returned?

tkelman · 2017-01-26T18:24:11Z

base/libgit2/reference.jl

+    if err != Int(Error.GIT_OK)
+        if err == Int(Error.ENOTFOUND)
+            return Nullable{GitReference}()
+        elseif err != Int(Error.ENOTFOUND) && ref_ptr_ptr[] != C_NULL


same here, would have just returned if it was ENOTFOUND

tkelman · 2017-01-26T18:25:04Z

base/libgit2/libgit2.jl

                throw(GitError(Error.Rebase, Error.ERROR,
                               "There is no tracking information for the current branch."))
            end
            try
-                GitAnnotated(repo, brn_ref)
+                GitAnnotated(repo, get(brn_ref))


will this need to be Base.get? if so, was this not covered by tests?

Doesn't need to be Base.get inside base/libgit2, I don't think. It does need to be Base.get in test/libgit2

there's still a LibGit2.get that isn't nullable-related, right?

Soon to be removed. If that breaks this we can just keep importing Base.get?

There are a lot of other places where you've used Base.get for nullable unpacking here though - does the LibGit2 module currently import Base.get ? If so it's extending a different concept in a way it probably shouldn't be. If not, then this line is calling the wrong get

If so it's extending a different concept in a way it probably shouldn't be.

Yes, we all agree, which is why it's being got rid of, which @simonbyrne has an open PR to do.

Anyway I'll make this a Base.get for consistency. This code path is not tested, something we can deal with in a separate PR.

I don't think it's being imported:

$ git grep -n import base/libgit2 base/libgit2/libgit2.jl:5:import Base: merge!, cat, == base/libgit2/types.jl:2:import Base.@kwdef base/libgit2/types.jl:3:import .Consts: GIT_SUBMODULE_IGNORE, GIT_MERGE_FILE_FAVOR, GIT_MERGE_FILE base/libgit2/types.jl:590:import Base.securezero!

My point wasn't LibGit2.get is bad, that much is obvious. It was the "does the LibGit2 module currently import Base.get ?" question, because the yes/no answer to that determines whether or not this code is correct right now.

I did a grep and it appears not to. So the code was incorrect, and I've fixed it, and this code path is not tested currently.

tkelman · 2017-01-26T19:10:28Z

base/libgit2/libgit2.jl

@@ -219,9 +219,9 @@ function branch!(repo::GitRepo, branch_name::AbstractString,
                 force::Bool=false,           # force branch creation
                 set_head::Bool=true)         # set as head reference on exit
    # try to lookup branch first
-    branch_ref = Nullable{GitReference}(force ? nothing : lookup_branch(repo, branch_name))
+    branch_ref = force ? Nullable{GitReference}() : lookup_branch(repo, branch_name)


this looks better than relying on the conversion from nothing to a null anyway IMO

simonbyrne · 2017-01-26T19:51:45Z

base/pkg/entry.jl

                end

                # checkout selected branch
-                with(LibGit2.peel(LibGit2.GitTree, ref)) do btree
+                with(LibGit2.peel(LibGit2.GitTree, get(ref))) do btree


I think in pkg/ we don't need the Base. part because something is importing it. Look up earlier in the file at the get(ENV calls.

simonbyrne · 2017-01-26T19:52:05Z

base/pkg/entry.jl

                    LibGit2.checkout_tree(repo, btree)
                end
                # switch head to the branch
-                LibGit2.head!(repo, ref)
+                LibGit2.head!(repo, get(ref))


simonbyrne · 2017-01-26T19:52:13Z

base/pkg/entry.jl

            finally
-                close(ref)
+                close(get(ref))


kshyatt · 2017-01-27T16:59:48Z

CI passed and I don't think we need Base.get in base/pkg/entry because it's been imported already for ENV. Good to squash?

tkelman · 2017-01-27T17:11:06Z

wasn't explicitly imported there, but since Pkg is a submodule of Base and doesn't define its own locally-shadowing get then it resolves to the usual Base one, unlike inside LibGit2

simonbyrne · 2017-01-28T21:31:40Z

Ah right. In that case, LGTM.

tkelman · 2017-01-28T21:40:22Z

base/libgit2/reference.jl

+"""
+    upstream(ref::GitReference) -> Nullable{GitReference}
+
+Determine if the branch specified by `ref` has a specified upstream branch.


might read more nicely if two different words were used rather than saying specified twice in a row here

kshyatt added the libgit2 The libgit2 library or the LibGit2 stdlib module label Jan 19, 2017

kshyatt requested a review from simonbyrne January 19, 2017 15:54

simonbyrne reviewed Jan 19, 2017

View reviewed changes

kshyatt force-pushed the ksh/upstreamthrow branch from 024b003 to e80782a Compare January 21, 2017 00:16

tkelman reviewed Jan 21, 2017

View reviewed changes

kshyatt force-pushed the ksh/upstreamthrow branch from e80782a to 4865f02 Compare January 21, 2017 01:00

kshyatt force-pushed the ksh/upstreamthrow branch from 4865f02 to 1bfd093 Compare January 25, 2017 21:05

tkelman reviewed Jan 25, 2017

View reviewed changes

kshyatt force-pushed the ksh/upstreamthrow branch from 1bfd093 to 2d196e1 Compare January 25, 2017 21:39

simonbyrne reviewed Jan 26, 2017

View reviewed changes

kshyatt added 2 commits January 26, 2017 08:53

upstream and lookup_branch throw if not found

56f9b3a

Add docs

b46c391

kshyatt force-pushed the ksh/upstreamthrow branch from 2d196e1 to b46c391 Compare January 26, 2017 16:58

tkelman reviewed Jan 26, 2017

View reviewed changes

Fix double-nullable

d0d2306

kshyatt force-pushed the ksh/upstreamthrow branch from 0af7d4e to d0d2306 Compare January 26, 2017 19:29

simonbyrne reviewed Jan 26, 2017

View reviewed changes

base/pkg/entry.jl

finally

close(ref)

close(get(ref))

Copy link

Contributor

simonbyrne Jan 26, 2017

Choose a reason for hiding this comment

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

Base.get

simonbyrne merged commit 4530c58 into master Jan 28, 2017

simonbyrne deleted the ksh/upstreamthrow branch January 28, 2017 21:33

tkelman reviewed Jan 28, 2017

View reviewed changes

		@@ -169,10 +169,8 @@ function lookup_branch(repo::GitRepo,
		err = ccall((:git_branch_lookup, :libgit2), Cint,

		@@ -185,10 +183,8 @@ function upstream(ref::GitReference)
		ref_ptr_ptr = Ref{Ptr{Void}}(C_NULL)
		err = ccall((:git_branch_upstream, :libgit2), Cint,

upstream and lookup_branch throw if not found #20127

upstream and lookup_branch throw if not found #20127

Conversation

kshyatt commented Jan 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kshyatt Jan 19, 2017 • edited Loading

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

tkelman commented Jan 21, 2017

simonbyrne commented Jan 21, 2017 • edited Loading

tkelman commented Jan 21, 2017

simonbyrne commented Jan 21, 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

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

kshyatt commented Jan 27, 2017

tkelman commented Jan 27, 2017

simonbyrne commented Jan 28, 2017

Choose a reason for hiding this comment

kshyatt Jan 19, 2017 •

edited

Loading

simonbyrne commented Jan 21, 2017 •

edited

Loading