Use standard Julia mechanisms for LibGit2 StrArrayStruct and Buffer t…

…ypes. (#19962)

* use standard Julia mechanisms for passing StrArrayStruct

* rejig Buffer free function

* update to use RefArray

* add julia annotation to code block

* typo

* fix diff error

* change unsafe_convert back to convert

* update test

base/libgit2/config.jl

@@ -52,12 +52,13 @@ function addfile(cfg::GitConfig, path::AbstractString,
 end
 
 function get{T<:AbstractString}(::Type{T}, c::GitConfig, name::AbstractString)
-    buf_ptr = Ref(Buffer())
+    buf_ref = Ref(Buffer())
     @check ccall((:git_config_get_string_buf, :libgit2), Cint,
-                (Ptr{Buffer}, Ptr{Void}, Cstring), buf_ptr, c.ptr, name)
-    return with(buf_ptr[]) do buf
-        unsafe_string(buf.ptr, buf.size)
-    end
+                 (Ptr{Buffer}, Ptr{Void}, Cstring), buf_ref, c.ptr, name)
+    buf = buf_ref[]
+    str = unsafe_string(buf.ptr, buf.size)
+    free(buf_ref)
+    str
 end
 
 function get(::Type{Bool}, c::GitConfig, name::AbstractString)

base/libgit2/diff.jl

@@ -1,24 +1,27 @@
 # This file is a part of Julia. License is MIT: http://julialang.org/license
 
+# TODO: make this a general purpose solution
+function Base.cconvert(::Type{Ptr{DiffOptionsStruct}}, pathspecs::AbstractString)
+    str_ref = Base.cconvert(Ref{Cstring}, [pathspecs])
+    sa = StrArrayStruct(Base.unsafe_convert(Ref{Cstring}, str_ref), 1)
+    do_ref = Ref(DiffOptions(pathspec = sa))
+    do_ref, str_ref
+end
+function Base.unsafe_convert(::Type{Ptr{DiffOptionsStruct}}, rr::Tuple{Ref{DiffOptionsStruct}, Ref{Cstring}})
+    Base.unsafe_convert(Ptr{DiffOptionStruct}, first(rr))
+end
+
+
 function diff_tree(repo::GitRepo, tree::GitTree, pathspecs::AbstractString=""; cached::Bool=false)
-    emptypathspec = isempty(pathspecs)
     diff_ptr_ptr = Ref{Ptr{Void}}(C_NULL)
-    if !emptypathspec
-        sa = StrArrayStruct(pathspecs)
-        diff_opts = DiffOptionsStruct(pathspec = sa)
-    end
-    try
-        if cached
-            @check ccall((:git_diff_tree_to_index, :libgit2), Cint,
-                          (Ptr{Ptr{Void}}, Ptr{Void}, Ptr{Void}, Ptr{Void}, Ptr{DiffOptionsStruct}),
-                           diff_ptr_ptr, repo.ptr, tree.ptr, C_NULL,  emptypathspec ? C_NULL : Ref(diff_opts))
-        else
-            @check ccall((:git_diff_tree_to_workdir_with_index, :libgit2), Cint,
-                          (Ptr{Ptr{Void}}, Ptr{Void}, Ptr{Void}, Ptr{DiffOptionsStruct}),
-                           diff_ptr_ptr, repo.ptr, tree.ptr, emptypathspec ? C_NULL : Ref(diff_opts))
-        end
-    finally
-        !emptypathspec && close(sa)
+    if cached
+        @check ccall((:git_diff_tree_to_index, :libgit2), Cint,
+                     (Ptr{Ptr{Void}}, Ptr{Void}, Ptr{Void}, Ptr{Void}, Ptr{DiffOptionsStruct}),
+                     diff_ptr_ptr, repo.ptr, tree.ptr, C_NULL, isempty(pathspecs) ? C_NULL : pathspecs)
+    else
+        @check ccall((:git_diff_tree_to_workdir_with_index, :libgit2), Cint,
+                     (Ptr{Ptr{Void}}, Ptr{Void}, Ptr{Void}, Ptr{DiffOptionsStruct}),
+                     diff_ptr_ptr, repo.ptr, tree.ptr, isempty(pathspecs) ? C_NULL : pathspecs)
     end
     return GitDiff(repo, diff_ptr_ptr[])
 end

base/libgit2/index.jl

@@ -42,36 +42,21 @@ end
 
 function add!{T<:AbstractString}(idx::GitIndex, files::T...;
              flags::Cuint = Consts.INDEX_ADD_DEFAULT)
-    sa = StrArrayStruct(files...)
-    try
-        @check ccall((:git_index_add_all, :libgit2), Cint,
-                     (Ptr{Void}, Ptr{StrArrayStruct}, Cuint, Ptr{Void}, Ptr{Void}),
-                      idx.ptr, Ref(sa), flags, C_NULL, C_NULL)
-    finally
-        close(sa)
-    end
+    @check ccall((:git_index_add_all, :libgit2), Cint,
+                 (Ptr{Void}, Ptr{StrArrayStruct}, Cuint, Ptr{Void}, Ptr{Void}),
+                 idx.ptr, collect(files), flags, C_NULL, C_NULL)
 end
 
 function update!{T<:AbstractString}(idx::GitIndex, files::T...)
-    sa = StrArrayStruct(files...)
-    try
-        @check ccall((:git_index_update_all, :libgit2), Cint,
-                     (Ptr{Void}, Ptr{StrArrayStruct}, Ptr{Void}, Ptr{Void}),
-                      idx.ptr, Ref(sa), C_NULL, C_NULL)
-    finally
-        close(sa)
-    end
+    @check ccall((:git_index_update_all, :libgit2), Cint,
+                 (Ptr{Void}, Ptr{StrArrayStruct}, Ptr{Void}, Ptr{Void}),
+                 idx.ptr, collect(files), C_NULL, C_NULL)
 end
 
 function remove!{T<:AbstractString}(idx::GitIndex, files::T...)
-    sa = StrArrayStruct(files...)
-    try
-        @check ccall((:git_index_remove_all, :libgit2), Cint,
-                     (Ptr{Void}, Ptr{StrArrayStruct}, Ptr{Void}, Ptr{Void}),
-                      idx.ptr, Ref(sa), C_NULL, C_NULL)
-    finally
-        close(sa)
-    end
+    @check ccall((:git_index_remove_all, :libgit2), Cint,
+                 (Ptr{Void}, Ptr{StrArrayStruct}, Ptr{Void}, Ptr{Void}),
+                 idx.ptr, collect(files), C_NULL, C_NULL)
 end
 
 function add!{T<:AbstractString}(repo::GitRepo, files::T...;

base/libgit2/reference.jl

@@ -131,12 +131,12 @@ function peel{T <: GitObject}(::Type{T}, ref::GitReference)
 end
 
 function ref_list(repo::GitRepo)
-    with(StrArrayStruct()) do sa
-        sa_ref = Ref(sa)
-        @check ccall((:git_reference_list, :libgit2), Cint,
+    sa_ref = Ref(StrArrayStruct())
+    @check ccall((:git_reference_list, :libgit2), Cint,
                       (Ptr{StrArrayStruct}, Ptr{Void}), sa_ref, repo.ptr)
-        convert(Vector{AbstractString}, sa_ref[])
-    end
+    res = convert(Vector{String}, sa_ref[])
+    free(sa_ref)
+    res
 end
 
 function create_branch(repo::GitRepo,

base/libgit2/remote.jl

@@ -45,54 +45,38 @@ function name(rmt::GitRemote)
 end
 
 function fetch_refspecs(rmt::GitRemote)
-    sa_ref = Ref{StrArrayStruct}()
-    try
-        @check ccall((:git_remote_get_fetch_refspecs, :libgit2), Cint,
-                      (Ptr{LibGit2.StrArrayStruct}, Ptr{Void}), sa_ref, rmt.ptr)
-        convert(Vector{AbstractString}, sa_ref[])
-    finally
-        close(sa_ref[])
-    end
+    sa_ref = Ref(StrArrayStruct())
+    @check ccall((:git_remote_get_fetch_refspecs, :libgit2), Cint,
+                 (Ptr{StrArrayStruct}, Ptr{Void}), sa_ref, rmt.ptr)
+    res = convert(Vector{String}, sa_ref[])
+    free(sa_ref)
+    res
 end
 
 function push_refspecs(rmt::GitRemote)
-    sa_ref = Ref{StrArrayStruct}()
-    try
-        @check ccall((:git_remote_get_push_refspecs, :libgit2), Cint,
-                      (Ptr{LibGit2.StrArrayStruct}, Ptr{Void}), sa_ref, rmt.ptr)
-        convert(Vector{AbstractString}, sa_ref[])
-    finally
-        close(sa_ref[])
-    end
+    sa_ref = Ref(StrArrayStruct())
+    @check ccall((:git_remote_get_push_refspecs, :libgit2), Cint,
+                 (Ptr{StrArrayStruct}, Ptr{Void}), sa_ref, rmt.ptr)
+    res = convert(Vector{String}, sa_ref[])
+    free(sa_ref)
+    res
 end
 
 function fetch{T<:AbstractString}(rmt::GitRemote, refspecs::Vector{T};
                options::FetchOptions = FetchOptions(),
                msg::AbstractString="")
     msg = "libgit2.fetch: $msg"
-    no_refs = isempty(refspecs)
-    !no_refs && (sa = StrArrayStruct(refspecs...))
-    try
-        @check ccall((:git_remote_fetch, :libgit2), Cint,
-            (Ptr{Void}, Ptr{StrArrayStruct}, Ptr{FetchOptions}, Cstring),
-            rmt.ptr, no_refs ? C_NULL : Ref(sa), Ref(options), msg)
-    finally
-        !no_refs && close(sa)
-    end
+    @check ccall((:git_remote_fetch, :libgit2), Cint,
+                 (Ptr{Void}, Ptr{StrArrayStruct}, Ptr{FetchOptions}, Cstring),
+                 rmt.ptr, isempty(refspecs) ? C_NULL : refspecs, Ref(options), msg)
 end
 
 function push{T<:AbstractString}(rmt::GitRemote, refspecs::Vector{T};
                                  force::Bool = false,
                                  options::PushOptions = PushOptions())
-    no_refs = isempty(refspecs)
-    !no_refs && (sa = StrArrayStruct(refspecs...))
-    try
-        @check ccall((:git_remote_push, :libgit2), Cint,
-                      (Ptr{Void}, Ptr{StrArrayStruct}, Ptr{PushOptions}),
-                       rmt.ptr, no_refs ? C_NULL : Ref(sa), Ref(options))
-    finally
-        !no_refs && close(sa)
-    end
+    @check ccall((:git_remote_push, :libgit2), Cint,
+                 (Ptr{Void}, Ptr{StrArrayStruct}, Ptr{PushOptions}),
+                 rmt.ptr, isempty(refspecs) ? C_NULL : refspecs, Ref(options))
 end
 
 Base.show(io::IO, rmt::GitRemote) = print(io, "GitRemote:\nRemote name: ", name(rmt), " url: ", url(rmt))

base/libgit2/repository.jl

@@ -223,13 +223,11 @@ end
 
 """Updates some entries, determined by the `pathspecs`, in the index from the target commit tree."""
 function reset!{T<:AbstractString, S<:GitObject}(repo::GitRepo, obj::Nullable{S}, pathspecs::T...)
-    with(StrArrayStruct(pathspecs...)) do sa
-        @check ccall((:git_reset_default, :libgit2), Cint,
-                (Ptr{Void}, Ptr{Void}, Ptr{StrArrayStruct}),
-                repo.ptr,
-                isnull(obj) ? C_NULL: Base.get(obj).ptr,
-                Ref(sa))
-    end
+    @check ccall((:git_reset_default, :libgit2), Cint,
+                 (Ptr{Void}, Ptr{Void}, Ptr{StrArrayStruct}),
+                 repo.ptr,
+                 isnull(obj) ? C_NULL: Base.get(obj).ptr,
+                 collect(pathspecs))
 end
 
 """Sets the current head to the specified commit oid and optionally resets the index and working tree to match."""
@@ -259,11 +257,18 @@ function fetchheads(repo::GitRepo)
     return fhr[]
 end
 
+"""
+    LibGit2.remotes(repo::GitRepo)
+
+Returns a vector of the names of the remotes of `repo`.
+"""
 function remotes(repo::GitRepo)
-    out = Ref(StrArrayStruct())
+    sa_ref = Ref(StrArrayStruct())
     @check ccall((:git_remote_list, :libgit2), Cint,
-                  (Ptr{Void}, Ptr{Void}), out, repo.ptr)
-    return convert(Vector{AbstractString}, out[])
+                  (Ptr{StrArrayStruct}, Ptr{Void}), sa_ref, repo.ptr)
+    res = convert(Vector{String}, sa_ref[])
+    free(sa_ref)
+    return res
 end
 
 function Base.show(io::IO, repo::GitRepo)

base/libgit2/strarray.jl

@@ -1,37 +1,15 @@
 # This file is a part of Julia. License is MIT: http://julialang.org/license
 
-function StrArrayStruct{T<:AbstractString}(strs::T...)
-    strcount = length(strs)
-    for s in strs
-        if Base.containsnul(s)
-            throw("embedded NULs are not allowed in C strings: $(repr(s))")
-        end
-    end
-    sa_strings = convert(Ptr{Cstring}, Libc.malloc(sizeof(Cstring) * strcount))
-    for i=1:strcount
-        len = length(strs[i])
-        in_ptr  = pointer(String(strs[i]))
-        out_ptr = convert(Ptr{UInt8}, Libc.malloc(sizeof(UInt8) * (len + 1)))
-        unsafe_copy!(out_ptr, in_ptr, len)
-        unsafe_store!(out_ptr, zero(UInt8), len + 1) # NULL byte
-        unsafe_store!(sa_strings, convert(Cstring, out_ptr), i)
-    end
-    return StrArrayStruct(sa_strings, strcount)
-end
-StrArrayStruct{T<:AbstractString}(strs::Vector{T}) = StrArrayStruct(strs...)
 
-function Base.convert(::Type{Vector{AbstractString}}, sa::StrArrayStruct)
-    arr = Array{AbstractString}(sa.count)
-    for i=1:sa.count
-        arr[i] = unsafe_string(unsafe_load(sa.strings, i))
-    end
-    return arr
+function Base.cconvert(::Type{Ptr{StrArrayStruct}}, x::Vector)
+    str_ref = Base.cconvert(Ref{Cstring}, x)
+    sa_ref = Ref(StrArrayStruct(Base.unsafe_convert(Ref{Cstring}, str_ref), length(x)))
+    sa_ref, str_ref
+end
+function Base.unsafe_convert(::Type{Ptr{StrArrayStruct}}, rr::Tuple{Ref{StrArrayStruct}, Ref{Cstring}})
+    Base.unsafe_convert(Ptr{StrArrayStruct}, first(rr))
 end
 
-function Base.copy(src::StrArrayStruct)
-    dst_ptr = Ref(StrArrayStruct())
-    @check ccall((:git_strarray_copy, :libgit2), Cint,
-                  (Ptr{StrArrayStruct}, Ptr{StrArrayStruct}),
-                   dst_ptr, Ref(src))
-    return dst_ptr[]
+function Base.convert(::Type{Vector{String}}, sa::StrArrayStruct)
+    [unsafe_string(unsafe_load(sa.strings, i)) for i = 1:sa.count]
 end

base/libgit2/tag.jl

@@ -1,12 +1,12 @@
 # This file is a part of Julia. License is MIT: http://julialang.org/license
 
 function tag_list(repo::GitRepo)
-    with(StrArrayStruct()) do sa
-        sa_ref = Ref(sa)
-        @check ccall((:git_tag_list, :libgit2), Cint,
-                      (Ptr{StrArrayStruct}, Ptr{Void}), sa_ref, repo.ptr)
-        convert(Vector{AbstractString}, sa_ref[])
-    end
+    sa_ref = Ref(StrArrayStruct())
+    @check ccall((:git_tag_list, :libgit2), Cint,
+                 (Ptr{StrArrayStruct}, Ptr{Void}), sa_ref, repo.ptr)
+    res = convert(Vector{String}, sa_ref[])
+    free(sa_ref)
+    res
 end
 
 function tag_delete(repo::GitRepo, tag::AbstractString)

base/libgit2/types.jl

@@ -38,34 +38,61 @@ end
 """
     LibGit2.StrArrayStruct
 
-Array of strings.
+A LibGit2 representation of an array of strings.
 Matches the [`git_strarray`](https://libgit2.github.com/libgit2/#HEAD/type/git_strarray) struct.
-"""
-@kwdef immutable StrArrayStruct
+
+When fetching data from LibGit2, a typical usage would look like:
+```julia
+sa_ref = Ref(StrArrayStruct())
+@check ccall(..., (Ptr{StrArrayStruct},), sa_ref)
+res = convert(Vector{String}, sa_ref[])
+free(sa_ref)
+```
+In particular, note that `LibGit2.free` should be called afterward on the `Ref` object.
+
+Conversely, when passing a vector of strings to LibGit2, it is generally simplest to rely
+on implicit conversion:
+```julia
+strs = String[...]
+@check ccall(..., (Ptr{StrArrayStruct},), strs)
+```
+Note that no call to `free` is required as the data is allocated by Julia.
+"""
+immutable StrArrayStruct
    strings::Ptr{Cstring}
    count::Csize_t
 end
-function Base.close(sa::StrArrayStruct)
-    sa_ptr = Ref(sa)
-    ccall((:git_strarray_free, :libgit2), Void, (Ptr{StrArrayStruct},), sa_ptr)
-    return sa_ptr[]
+StrArrayStruct() = StrArrayStruct(C_NULL, 0)
+
+function free(sa_ref::Base.Ref{StrArrayStruct})
+    ccall((:git_strarray_free, :libgit2), Void, (Ptr{StrArrayStruct},), sa_ref)
 end
 
 """
     LibGit2.Buffer
 
 A data buffer for exporting data from libgit2.
 Matches the [`git_buf`](https://libgit2.github.com/libgit2/#HEAD/type/git_buf) struct.
+
+When fetching data from LibGit2, a typical usage would look like:
+```julia
+buf_ref = Ref(Buffer())
+@check ccall(..., (Ptr{Buffer},), buf_ref)
+# operation on buf_ref
+free(buf_ref)
+```
+In particular, note that `LibGit2.free` should be called afterward on the `Ref` object.
+
 """
-@kwdef immutable Buffer
+immutable Buffer
     ptr::Ptr{Cchar}
     asize::Csize_t
     size::Csize_t
 end
-function Base.close(buf::Buffer)
-    buf_ptr = Ref(buf)
-    ccall((:git_buf_free, :libgit2), Void, (Ptr{Buffer},), buf_ptr)
-    return buf_ptr[]
+Buffer() = Buffer(C_NULL, 0, 0)
+
+function free(buf_ref::Base.Ref{Buffer})
+    ccall((:git_buf_free, :libgit2), Void, (Ptr{Buffer},), buf_ref)
 end
 
 "Abstract credentials payload"