Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

upstream and lookup_branch throw if not found #20127

Merged
merged 3 commits into from
Jan 28, 2017
Merged

upstream and lookup_branch throw if not found #20127

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
56f9b3a
upstream and lookup_branch throw if not found
kshyatt Jan 19, 2017
b46c391
Add docs
kshyatt Jan 26, 2017
d0d2306
Fix double-nullable
kshyatt Jan 26, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 18 additions & 18 deletions base/libgit2/libgit2.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -219,22 +219,22 @@ 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)
if branch_ref === nothing
branch_rmt_ref = isempty(track) ? nothing : lookup_branch(repo, "$track/$branch_name", true)
branch_ref = force ? Nullable{GitReference}() : lookup_branch(repo, branch_name)
Copy link
Contributor

Choose a reason for hiding this comment

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

should the force argument still be used?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh yeah probably

Copy link
Contributor

Choose a reason for hiding this comment

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

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

if isnull(branch_ref)
branch_rmt_ref = isempty(track) ? Nullable{GitReference}() : lookup_branch(repo, "$track/$branch_name", true)
# if commit is empty get head commit oid
commit_id = if isempty(commit)
if branch_rmt_ref === nothing
if isnull(branch_rmt_ref)
with(head(repo)) do head_ref
with(peel(GitCommit, head_ref)) do hrc
GitHash(hrc)
end
end
else
tmpcmt = with(peel(GitCommit, branch_rmt_ref)) do hrc
tmpcmt = with(peel(GitCommit, Base.get(branch_rmt_ref))) do hrc
GitHash(hrc)
end
close(branch_rmt_ref)
close(Base.get(branch_rmt_ref))
tmpcmt
end
else
Expand All @@ -244,10 +244,10 @@ function branch!(repo::GitRepo, branch_name::AbstractString,
cmt = get(GitCommit, repo, commit_id)
new_branch_ref = nothing
try
new_branch_ref = create_branch(repo, branch_name, cmt, force=force)
new_branch_ref = Nullable(create_branch(repo, branch_name, cmt, force=force))
finally
close(cmt)
new_branch_ref === nothing && throw(GitError(Error.Object, Error.ERROR, "cannot create branch `$branch_name` with `$commit_id`"))
isnull(new_branch_ref) && throw(GitError(Error.Object, Error.ERROR, "cannot create branch `$branch_name` with `$commit_id`"))
branch_ref = new_branch_ref
end
end
Expand All @@ -257,7 +257,7 @@ function branch!(repo::GitRepo, branch_name::AbstractString,
try
with(GitConfig, repo) do cfg
set!(cfg, "branch.$branch_name.remote", Consts.REMOTE_ORIGIN)
set!(cfg, "branch.$branch_name.merge", name(branch_ref))
set!(cfg, "branch.$branch_name.merge", name(Base.get(branch_ref)))
end
catch
warn("Please provide remote tracking for branch '$branch_name' in '$(path(repo))'")
Expand All @@ -266,15 +266,15 @@ function branch!(repo::GitRepo, branch_name::AbstractString,

if set_head
# checkout selected branch
with(peel(GitTree, branch_ref)) do btree
with(peel(GitTree, Base.get(branch_ref))) do btree
checkout_tree(repo, btree)
end

# switch head to the branch
head!(repo, branch_ref)
head!(repo, Base.get(branch_ref))
end
finally
close(branch_ref)
close(Base.get(branch_ref))
end
return
end
Expand Down Expand Up @@ -452,14 +452,14 @@ function merge!(repo::GitRepo;
else
with(head(repo)) do head_ref
tr_brn_ref = upstream(head_ref)
if tr_brn_ref === nothing
if isnull(tr_brn_ref)
throw(GitError(Error.Merge, Error.ERROR,
"There is no tracking information for the current branch."))
end
try
[GitAnnotated(repo, tr_brn_ref)]
[GitAnnotated(repo, Base.get(tr_brn_ref))]
finally
close(tr_brn_ref)
close(Base.get(tr_brn_ref))
end
end
end
Expand Down Expand Up @@ -494,14 +494,14 @@ a `GitError`. This is roughly equivalent to the following command line statement
function rebase!(repo::GitRepo, upstream::AbstractString="", newbase::AbstractString="")
with(head(repo)) do head_ref
head_ann = GitAnnotated(repo, head_ref)
upst_ann = if isempty(upstream)
upst_ann = if isempty(upstream)
brn_ref = LibGit2.upstream(head_ref)
if brn_ref === nothing
if isnull(brn_ref)
throw(GitError(Error.Rebase, Error.ERROR,
"There is no tracking information for the current branch."))
end
try
GitAnnotated(repo, brn_ref)
GitAnnotated(repo, Base.get(brn_ref))
finally
close(brn_ref)
end
Expand Down
38 changes: 30 additions & 8 deletions base/libgit2/reference.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,6 +161,17 @@ function head!(repo::GitRepo, ref::GitReference)
return ref
end

"""
lookup_branch(repo::GitRepo, branch_name::AbstractString, remote::Bool=false) -> Nullable{GitReference}

Determine if the branch specified by `branch_name` exists in the repository `repo`.
If `remote` is `true`, `repo` is assumed to be a remote git repository. Otherwise, it
is part of the local filesystem.

`lookup_branch` returns a `Nullable`, which will be null if the requested branch does
not exist yet. If the branch does exist, the `Nullable` contains a `GitReference` to
the branch.
"""
function lookup_branch(repo::GitRepo,
branch_name::AbstractString,
remote::Bool=false)
Expand All @@ -169,31 +180,42 @@ function lookup_branch(repo::GitRepo,
err = ccall((:git_branch_lookup, :libgit2), Cint,
Copy link
Contributor

Choose a reason for hiding this comment

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

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(...)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@kshyatt kshyatt Jan 19, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I will fix this in the other PR btw #20128

(Ptr{Ptr{Void}}, Ptr{Void}, Ptr{UInt8}, Cint),
ref_ptr_ptr, repo.ptr, branch_name, branch_type)
if err == Int(Error.ENOTFOUND)
return nothing
elseif err != Int(Error.GIT_OK)
if err != Int(Error.GIT_OK)
if err == Int(Error.ENOTFOUND)
return Nullable{GitReference}()
end
if ref_ptr_ptr[] != C_NULL
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

"""
upstream(ref::GitReference) -> Nullable{GitReference}

Determine if the branch specified by `ref` has a specified upstream branch.
Copy link
Contributor

Choose a reason for hiding this comment

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

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


`upstream` returns a `Nullable`, which will be null if the requested branch does
not have an upstream counterpart. If the upstream branch does exist, the `Nullable`
contains a `GitReference` to the upstream branch.
"""
function upstream(ref::GitReference)
Copy link
Contributor

Choose a reason for hiding this comment

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

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

isempty(ref) && return nothing
ref_ptr_ptr = Ref{Ptr{Void}}(C_NULL)
err = ccall((:git_branch_upstream, :libgit2), Cint,
Copy link
Contributor

Choose a reason for hiding this comment

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

and same here as well

(Ref{Ptr{Void}}, Ptr{Void},), ref_ptr_ptr, ref.ptr)
if err == Int(Error.ENOTFOUND)
return nothing
elseif err != Int(Error.GIT_OK)
if err != Int(Error.GIT_OK)
if err == Int(Error.ENOTFOUND)
return Nullable{GitReference}()
end
if ref_ptr_ptr[] != C_NULL
close(GitReference(ref.repo, ref_ptr_ptr[]))
end
throw(Error.GitError(err))
end
return GitReference(ref.repo, ref_ptr_ptr[])
return Nullable{GitReference}(GitReference(ref.repo, ref_ptr_ptr[]))
end

repository(ref::GitReference) = ref.repo
Expand Down
10 changes: 5 additions & 5 deletions base/pkg/entry.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -312,25 +312,25 @@ function pin(pkg::AbstractString, head::AbstractString)
end
ref = LibGit2.lookup_branch(repo, branch)
try
if ref !== nothing
if !isnull(ref)
if LibGit2.revparseid(repo, branch) != id
throw(PkgError("Package $pkg: existing branch $branch has " *
"been edited and doesn't correspond to its original commit"))
end
info("Package $pkg: checking out existing branch $branch")
else
info("Creating $pkg branch $branch")
ref = LibGit2.create_branch(repo, branch, commit)
ref = Nullable(LibGit2.create_branch(repo, branch, commit))
end

# checkout selected branch
with(LibGit2.peel(LibGit2.GitTree, ref)) do btree
with(LibGit2.peel(LibGit2.GitTree, get(ref))) do btree
Copy link
Contributor

Choose a reason for hiding this comment

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

Base.get

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

LibGit2.checkout_tree(repo, btree)
end
# switch head to the branch
LibGit2.head!(repo, ref)
LibGit2.head!(repo, get(ref))
Copy link
Contributor

Choose a reason for hiding this comment

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

Base.get

finally
close(ref)
close(get(ref))
Copy link
Contributor

Choose a reason for hiding this comment

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

Base.get

end
finally
close(commit)
Expand Down
8 changes: 4 additions & 4 deletions test/libgit2.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -324,17 +324,17 @@ mktempdir() do dir
@test LibGit2.name(brref) == "refs/heads/master"
@test LibGit2.shortname(brref) == master_branch
@test LibGit2.ishead(brref)
@test LibGit2.upstream(brref) === nothing
@test isnull(LibGit2.upstream(brref))
@test repo.ptr == LibGit2.repository(brref).ptr
@test brnch == master_branch
@test LibGit2.headname(repo) == master_branch
LibGit2.branch!(repo, test_branch, string(commit_oid1), set_head=false)

@test LibGit2.lookup_branch(repo, test_branch, true) === nothing
tbref = LibGit2.lookup_branch(repo, test_branch, false)
@test isnull(LibGit2.lookup_branch(repo, test_branch, true))
tbref = Base.get(LibGit2.lookup_branch(repo, test_branch, false))
try
@test LibGit2.shortname(tbref) == test_branch
@test LibGit2.upstream(tbref) === nothing
@test isnull(LibGit2.upstream(tbref))
finally
close(tbref)
end
Expand Down