add GitShortHash type, remove some get methods #20104

simonbyrne · 2017-01-18T18:06:45Z

Another part of #19839.

ararslan · 2017-01-18T18:10:27Z

base/libgit2/oid.jl

-        ccall((:git_oid_fromstrp, :libgit2), Cint,
-              (Ptr{GitHash}, Cstring), oid_ptr, bstr)
+    if len < OID_HEXSZ
+        error("Input string is too short, use `GitShortHash` for partial hashes")


Might make more sense as an ArgumentError?

ararslan · 2017-01-18T18:13:15Z

base/libgit2/types.jl

+
+
+
+


Four newlines seems like a lot of vertical whitespace. Perhaps for consistency it should be 1-2?

tkelman · 2017-01-19T13:32:11Z

base/deprecated.jl

+     @deprecate get{T<:GitObject}(::Type{T}, repo::GitRepo, oid::GitHash, oid_size::Int) T(repo, GitShortHash(oid, oid_size))
+     @deprecate revparse(repo::GitRepo, objname::AbstractString) GitObject(repo, objname)
+     @deprecate object(repo::GitRepo, te::GitTreeEntry) GitObject(repo, te)
+     @deprecate commit(ann::GitAnnotated) GitHash(ann)


were these previously exported from the LibGit2 module?

No. Should I not bother deprecating them then?

dunno who if anyone would be using these. the main concern is that @deprecate exports by default, if you want a non exported deprecation then you have to manually call depwarn

These are sometimes used externally (e.g. PkgDev calls LibGit2.get).
I've added an option to @deprecate on whether to export or not.

tkelman · 2017-01-21T20:50:43Z

base/libgit2/libgit2.jl

    else
-        return nothing
+        nothing


what does this correspond to?

If the resulting object is a commit, tag or tree. Arguably this is another function that could be removed altogether.

tkelman · 2017-01-21T20:52:24Z

base/libgit2/oid.jl

    end
-    err != 0 && return GitHash()
+    oid_ptr = Ref{GitHash}()
+    @check ccall((:git_oid_fromstrn, :libgit2), Cint,


it had been calling fromstrp in this case, will they return the same thing if full length?

I believe so

Yes, fromstrp is just the null-terminated version of fromstrn. Since Julia strings store the length, we might as well use fromstrn.

tkelman · 2017-01-21T20:55:51Z

base/libgit2/oid.jl

 end

 Base.hex(id::GitHash) = join([hex(i,2) for i in id.val])
+Base.hex(id::GitShortHash) = hex(id.hash)[1:id.len]


it looks like there are two concepts of length for hashes, raw size and hex size, that differ by a factor of 2?

yes, exactly. id.len is hex size.

it looked like it was getting used as if it was raw size in a few places, though I may have misread

It's only used in a handful of functions: git_oid_ncmp and git_object_lookup_prefix

tkelman · 2017-01-21T21:01:16Z

base/libgit2/types.jl

    val::NTuple{OID_RAWSZ, UInt8}
    GitHash(val::NTuple{OID_RAWSZ, UInt8}) = new(val)
 end
 GitHash() = GitHash(ntuple(i->zero(UInt8), OID_RAWSZ))

+immutable GitShortHash <: AbstractGitHash
+    hash::GitHash
+    len::Csize_t


should be extra careful (and add lots of comments) that this gets used consistently as the right notion of length everywhere, hex or raw

Good point.

tkelman · 2017-01-21T21:25:58Z

base/libgit2/oid.jl

+    return c == 0 ? cmp(id1.len, id2.len) : c
+end
+Base.cmp(id1::GitHash, id2::GitShortHash) = cmp(GitShortHash(id1, OID_RAWSZ), id2)
+Base.cmp(id1::GitShortHash, id2::GitHash) = cmp(id1, GitShortHash(id2, OID_RAWSZ))


should these be using OID_HEXSZ then when converting a full GitHash to a GitShortHash?

tkelman · 2017-01-21T21:29:24Z

base/libgit2/types.jl

+This is a shortened form of `GitHash`, which can be used to identify a git object when it
+is unique.
+
+Internally it is stored as two fields: a full-size `GitHash` (`hash`) and a length (`len`). Only the initial `len` hex digits of `hash` are used.


line length

tkelman · 2017-01-21T21:30:11Z

base/libgit2/repository.jl

-    close(obj)
-    return oid
+Return the specified git object from `repo` specified by `hash` or `spec`.
+- `hash` is a full (`GitHash`) or partial (`GitShortHash`)


either leave out the parens or add the word hash at the end

tkelman · 2017-01-21T21:36:46Z

base/libgit2/oid.jl

-Base.cmp(id1::GitHash, id2::GitShortHash) = cmp(GitShortHash(id1, OID_RAWSZ), id2)
-Base.cmp(id1::GitShortHash, id2::GitHash) = cmp(id1, GitShortHash(id2, OID_RAWSZ))
+Base.cmp(id1::GitHash, id2::GitShortHash) = cmp(GitShortHash(id1, OID_HEXSZ), id2)
+Base.cmp(id1::GitShortHash, id2::GitHash) = cmp(id1, GitShortHash(id2, OID_HEXSZ))


guess a comparison that depended on the latter half of the full input would have given differing results here, but we didn't have any such cases?

I don't think this function was used (or tested for that matter).

via equality only?

tkelman · 2017-01-21T21:38:43Z

base/libgit2/oid.jl


-==(id1::GitHash, id2::GitHash) = cmp(id1, id2) == 0
-Base.isless(id1::GitHash, id2::GitHash)  = cmp(id1, id2) < 0
+==(id1::AbstractGitHash, id2::AbstractGitHash) = cmp(id1, id2) == 0


should this try to canonicalize short hashes and consider them equal to a full hash if they are unambiguous?

The problem is that they can only be canonicalized in the context of a single repository. But GitHashes exist independent of the repository.

Oh good point. So maybe we shouldn't define this for short hashes, and only use repo-context-aware equality testing with them?

I don't know...

Perhaps we could define isequal with a repo argument for any AbstractGitHash then as Tony said only have == for GitHash?

Meh, I'm not really convinced it's a problem that different AbstractGitHashes can refer identify the same object.

We should probably define == for GitObjects as well: could be "are they from the same repo, and are their GitHashes equal"?

two references to the same object comparing as not equal because they had different partial hash lengths strikes me as unintuitive and not a useful notion of equality

I guess it depends on whether you view a GitHash by itself as a reference to an object? Or is it only a reference in the context of a specific repo, in which case you have a GitObject?

I'm thinking that a GitHash type without a reference to an owning repository isn't all that meaningful on its own.

okay, should we just revert this to ==(id1::GitHash, id2::GitHash) = cmp(id1, id2) == 0?

kshyatt · 2017-01-24T20:35:46Z

base/libgit2/index.jl

+    @check ccall((:git_index_read_tree, :libgit2), Cint,
+                 (Ptr{Void}, Ptr{Void}), idx.ptr, tree.ptr)
+end
+function read_tree!(idx::GitIndex, hash::AbstractGitHash)


Can this be a one-line?

kshyatt · 2017-01-24T20:36:25Z

base/libgit2/libgit2.jl

-    end
-    return head_oid(repo)
+""" git reset [--soft | --mixed | --hard] <id> """
+function reset!(repo::GitRepo, id::GitHash, mode::Cint = Consts.RESET_MIXED)


Again can this be a one-line?

tkelman · 2017-01-25T12:53:33Z

base/libgit2/reference.jl

+    peel([T,] ref::GitReference)
+
+Recursively peel `ref` until an object of type `T` is obtained. If no `T` is provided,
+then it will be peeled until an object other than a `GitTag` is obtained.


maybe ref in place of "it" would be more clear

(same for L181 of repository.jl)

simonbyrne · 2017-01-27T09:43:31Z

I think this is ready to go.

tkelman · 2017-01-27T13:18:22Z

base/libgit2/repository.jl

+    peel([T,] obj::GitObject)
+
+Recursively peel `obj` until an object of type `T` is obtained. If no `T` is provided,
+then it will be peeled until the type changes.


can we put obj instead of "it" here, otherwise it's potentially ambiguous whether "it" refers to T or obj

done. I've also added docs for all the GitObject types.

tkelman · 2017-01-27T15:51:47Z

base/libgit2/blob.jl

        for i in 1:showlen
            print(io, conts[i],"\n")
        end
    else
-        print(io, "GitBlob:\nBlob id: ", GitHash(blob.ptr), "\nContents are binary.")
+        print(io, "GitBlob:\nBlob id: ", GitHash(blob), "\nContents are binary.")


should this be println? along with the other prints in this function that have been adding \n manually

ararslan · 2017-01-27T17:11:38Z

base/libgit2/repository.jl

@@ -73,15 +73,29 @@ function isattached(repo::GitRepo)
    ccall((:git_repository_head_detached, :libgit2), Cint, (Ptr{Void},), repo.ptr) != 1
 end

-"""
+@doc """


Why is the @doc necessary here?

tkelman · 2017-01-27T17:19:51Z

base/libgit2/blob.jl

@@ -1,7 +1,7 @@
 # This file is a part of Julia. License is MIT: http://julialang.org/license

 function content(blob::GitBlob)
-    return ccall((:git_blob_rawcontent, :libgit2), Ptr{Void}, (Ptr{Void},), blob.ptr)
+    unsafe_string(ccall((:git_blob_rawcontent, :libgit2), Ptr{UInt8}, (Ptr{Void},), blob.ptr))


if this is binary content, it could easily have embedded nuls or be invalid utf-8, right?

Yes. We can deal with the NUL issue by calling it with a length argument (which I presume is git_blob_rawsize). But I don't know how we could deal with non-UTF8 data.

have it return an array of bytes?

tkelman · 2017-01-29T17:10:04Z

base/libgit2/blob.jl

+end
+
+function content(blob::GitBlob)
+    s = String(rawcontent)


rawcontent(blob)

kshyatt · 2017-01-31T18:26:32Z

Can this be merged?

ararslan

Looks good to me!

ararslan reviewed Jan 18, 2017

View reviewed changes

base/libgit2/types.jl

Copy link

Member

ararslan Jan 18, 2017

Choose a reason for hiding this comment

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

Four newlines seems like a lot of vertical whitespace. Perhaps for consistency it should be 1-2?

ararslan added the libgit2 The libgit2 library or the LibGit2 stdlib module label Jan 18, 2017

tkelman reviewed Jan 19, 2017

View reviewed changes

simonbyrne force-pushed the sb/gitshorthash branch from 9a0af6a to 776e66d Compare January 19, 2017 17:55

This was referenced Jan 20, 2017

Deprecate LibGit2.owner in favour of repository #20135

Merged

At least WARN if revparse{id} fails #20136

Closed

simonbyrne force-pushed the sb/gitshorthash branch from 776e66d to 4a1e17a Compare January 20, 2017 13:58

simonbyrne mentioned this pull request Jan 20, 2017

fix some LibGit2 errors #20155

Merged

simonbyrne requested a review from kshyatt January 21, 2017 10:36

tkelman reviewed Jan 21, 2017

View reviewed changes

simonbyrne force-pushed the sb/gitshorthash branch from 4a1e17a to ed65d4d Compare January 21, 2017 21:17

tkelman reviewed Jan 21, 2017

View reviewed changes

simonbyrne force-pushed the sb/gitshorthash branch from ed65d4d to 74b585d Compare January 21, 2017 21:33

tkelman reviewed Jan 21, 2017

View reviewed changes

kshyatt reviewed Jan 24, 2017

View reviewed changes

simonbyrne added 2 commits January 25, 2017 11:46

add GitShortHash type, remove some get methods

fc6b868

changes suggested by @kshyatt

ec8e2f6

simonbyrne force-pushed the sb/gitshorthash branch from c62ef8f to ec8e2f6 Compare January 25, 2017 11:48

tkelman reviewed Jan 25, 2017

View reviewed changes

simonbyrne and others added 3 commits January 25, 2017 13:05

clarify doc

2587850

Merge branch 'master' into sb/gitshorthash

8a02da9

use quote instead of begin block, qualify macro

dcde790

Merge branch 'master' into sb/gitshorthash

0b1ad7f

tkelman reviewed Jan 27, 2017

View reviewed changes

improve docs

d5bbd02

simonbyrne mentioned this pull request Jan 27, 2017

Fix return type of git_blob_is_binary() #20272

Merged

tkelman reviewed Jan 27, 2017

View reviewed changes

newline

2a2e936

ararslan reviewed Jan 27, 2017

View reviewed changes

tkelman reviewed Jan 27, 2017

View reviewed changes

simonbyrne added 2 commits January 29, 2017 17:00

Merge branch 'master' into sb/gitshorthash

3c19117

add test for valid blob data

50261a3

tkelman reviewed Jan 29, 2017

View reviewed changes

base/libgit2/blob.jl

end

function content(blob::GitBlob)

s = String(rawcontent)

Copy link

Contributor

tkelman Jan 29, 2017

Choose a reason for hiding this comment

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

rawcontent(blob)

fix function call

d9d15a8

ararslan approved these changes Jan 31, 2017

View reviewed changes

kshyatt merged commit c99e12c into master Jan 31, 2017

kshyatt deleted the sb/gitshorthash branch January 31, 2017 18:53

add GitShortHash type, remove some get methods #20104

add GitShortHash type, remove some get methods #20104

Conversation

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

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

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

tkelman Jan 25, 2017 • edited Loading

Choose a reason for hiding this comment

simonbyrne commented Jan 27, 2017

tkelman Jan 27, 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

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 31, 2017

ararslan left a comment

Choose a reason for hiding this comment

tkelman Jan 21, 2017 •

edited

Loading

simonbyrne Jan 21, 2017 •

edited

Loading

tkelman Jan 25, 2017 •

edited

Loading

tkelman Jan 27, 2017 •

edited

Loading