LibGit2: added tree related functions & tests #16537

wildart · 2016-05-23T18:57:27Z

These functions are required for reading repository blobs.

tkelman · 2016-05-24T22:54:11Z

base/libgit2/tree.jl

+You don't have to free it, but you must not use it after the `GitTree` is released.
+"""
+function lookup(tree::GitTree, fname::AbstractString)
+    te_ptr_ptr = Ref{Ptr{Void}}(C_NULL)


tkelman · 2016-06-04T13:54:27Z

base/libgit2/tree.jl

-"Get the filename of a tree entry."
+"""Returns a file name, as a `String`, of a tree entry.
+
+In case of error, `nothing` is returned.


no longer accurate?

darn, sorry

returning the empty string is also not that satisfying, what might cause this to error?

tkelman · 2016-06-14T16:18:43Z

base/libgit2/tree.jl

 function filename(te::GitTreeEntry)
    str = ccall((:git_tree_entry_name, :libgit2), Cstring, (Ptr{Void},), te.ptr)
-    str != C_NULL && return unsafe_string(str)
-    return nothing
+    str == C_NULL && return ""


why is this not an error? are there entries that have no name?

StefanKarpinski · 2016-06-14T19:05:46Z

I get the overall impression that our libgit2 bindings should be considerably more conservative with error handling. In general, any kind of failure should be propagated upwards or the system very rapidly becomes completely unreliable.

wildart · 2016-06-15T14:11:27Z

I like an idea of using nullable types despite unfortunately long type names.

tkelman · 2016-06-15T15:23:47Z

base/libgit2/tree.jl

-    str == C_NULL && return ""
+    str == C_NULL && throw(Error.GitError(Error.Tree,
+                                          LibGit2.Error.ENOTFOUND,
+                                          "Entry file name is not found"))


just say "not found," present tense is a bit off for an error message

I meant " not found", just drop the "is" - it's still useful to differentiate what wasn't found

tkelman · 2016-06-15T21:48:09Z

test/libgit2.jl

+                        end
+                        @test startswith(tfcontent, commit_msg1)
+                    finally
+                        finalize(tfte2)


is it necessary to have 3 nested finally blocks or could they be combined?

nope, that what you do when you need to be sure that resource is going to be freed. I could replace it with LibGit2.with call to make it more eye pleasing, but it is the same try block.

If only julia would have something similar to go-lang defer statement ...

okay so some of the immediately following tests require these to have been cleaned up already?

Tree entries need to be freed before the tree object is freed. But because entries created in try-block, a new try-block is created for finalizing them. All of it can be rearranged so finalization can be postponed as much as possible. I usually free the object as early as possible - that is what LibGit2.with does.

wildart · 2016-06-16T16:19:04Z

Something really strange with the OSX build, it gives a different error every time I run the test job. Today, it's memory handling error, yesterday one test failed.

tkelman · 2016-06-22T15:48:02Z

.travis.yml

@@ -68,7 +68,7 @@ before_install:
        gcc --version;
        BUILDOPTS="-j3 VERBOSE=1 FORCE_ASSERTIONS=1 LLVM_ASSERTIONS=1";
        echo "override ARCH=$ARCH" >> Make.user;
-        TESTSTORUN="all";
+        TESTSTORUN="libgit2";


NOT TO BE MERGED

wildart · 2016-06-23T20:02:16Z

I look that OSX build uses libgit2 v0.23.4 which might be the source of troubles for tree tests.
@tkelman Is there a way to update homebrew libgit2 version to v0.24.1?

tkelman · 2016-06-23T20:12:57Z

See Homebrew/homebrew-core#492, annoyingly needs to wait for homebrew to update all of gnome because there's a libgit2-glib package that depends on it. Have I ranted lately about how homebrew's versioning practices (or lack thereof) annoy me? If needed we could maybe host our own copy of the libgit2 formula and update that faster in the homebrew-juliadeps tap.

edit: looks like this was finished up in homebrew not long after I posted this

tkelman · 2016-06-24T13:50:29Z

base/libgit2/tree.jl

-    str != C_NULL && return unsafe_string(str)
-    return nothing
+    str == C_NULL && throw(Error.GitError(Error.Tree,
+                                          LibGit2.Error.ENOTFOUND, "not found"))


should still say what was not found if this same message is used in multiple places

added `finalize` for nullable type

tkelman · 2016-07-13T06:38:51Z

base/libgit2/tree.jl

+    res = ccall((:git_tree_entry_byname, :libgit2), Ptr{Void},
+                (Ref{Void}, Cstring), tree.ptr, fname)
+    res == C_NULL && return Nullable{GitTreeEntry}()
+    return Nullable(GitTreeEntry(res))


I was trying to use lookup_branch the other day, that would be another good one to make return nullable (ref #17364 where something is pretty broken)

tkelman · 2016-07-13T09:11:19Z

still wip? this looks pretty good now, assuming it passes on osx / linux

wildart · 2016-07-13T13:43:55Z

It's ready to merge.

tkelman · 2016-07-13T20:40:51Z

osx travis is having all sorts of issues. hopefully this passes there when that comes back.

tkelman · 2016-07-14T03:20:04Z

I think I spoke too soon, looks like this has been causing segfaults on osx in the libgit2 test since it was merged. I think we should revert for now if you don't have a fix. Can anyone reproduce locally?

kshyatt added the test This change adds or pertains to unit tests label May 23, 2016

tkelman reviewed May 24, 2016
View reviewed changes

wildart force-pushed the tree-funcs branch from 09259fd to 8176e8c Compare June 4, 2016 13:53

tkelman reviewed Jun 4, 2016
View reviewed changes

wildart force-pushed the tree-funcs branch 2 times, most recently from b1079b9 to 3378600 Compare June 4, 2016 14:03

kshyatt added the libgit2 The libgit2 library or the LibGit2 stdlib module label Jun 6, 2016

wildart force-pushed the tree-funcs branch 3 times, most recently from 8dfa0a7 to a5b21ea Compare June 14, 2016 00:30

tkelman reviewed Jun 14, 2016
View reviewed changes

wildart force-pushed the tree-funcs branch from a5b21ea to a38cfcb Compare June 15, 2016 14:38

tkelman reviewed Jun 15, 2016
View reviewed changes

wildart force-pushed the tree-funcs branch from a38cfcb to badd1ba Compare June 15, 2016 19:04

tkelman reviewed Jun 15, 2016
View reviewed changes

wildart force-pushed the tree-funcs branch from 49571ec to 18cfd73 Compare June 20, 2016 21:22

wildart force-pushed the tree-funcs branch 2 times, most recently from 4c963b8 to b107272 Compare June 22, 2016 14:10

tkelman reviewed Jun 22, 2016
View reviewed changes

wildart force-pushed the tree-funcs branch from b107272 to a43635c Compare June 23, 2016 18:42

tkelman reviewed Jun 24, 2016
View reviewed changes

wildart changed the title ~~LibGit2: added tree related functions & tests~~ WIP: LibGit2: added tree related functions & tests Jun 29, 2016

wildart added 5 commits July 12, 2016 23:49

LibGit2: added tree related functions & tests

a03c2b3

text fix & removed unused variable

fa0103f

docs update

84a6cc7

rebased & fixed incorrect string conversion

aebd5ce

use nullable types for values returned from tree search

9dfdb9a

added `finalize` for nullable type

wildart force-pushed the tree-funcs branch from a43635c to e1dbddd Compare July 13, 2016 03:51

fix tree tests & error msgs

99af6f2

wildart force-pushed the tree-funcs branch from e1dbddd to 99af6f2 Compare July 13, 2016 03:58

tkelman reviewed Jul 13, 2016
View reviewed changes

wildart changed the title ~~WIP: LibGit2: added tree related functions & tests~~ LibGit2: added tree related functions & tests Jul 13, 2016

tkelman merged commit 2ea885c into JuliaLang:master Jul 13, 2016

This was referenced Jul 14, 2016

RFC: Remove command-line Git from Mac and Windows binaries #17294

Merged

Revert "LibGit2: added tree related functions & tests" #17409

Merged

pabloferz mentioned this pull request Jul 14, 2016

Make promote_op rely on Core.Inference.return_type #17389

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

LibGit2: added tree related functions & tests #16537

LibGit2: added tree related functions & tests #16537

wildart commented May 23, 2016

tkelman May 24, 2016

wildart May 25, 2016

tkelman Jun 4, 2016

wildart Jun 4, 2016

tkelman Jun 4, 2016

tkelman Jun 14, 2016

StefanKarpinski commented Jun 14, 2016

wildart commented Jun 15, 2016

tkelman Jun 15, 2016

tkelman Jun 15, 2016 •

edited

Loading

tkelman Jun 15, 2016

wildart Jun 15, 2016

tkelman Jun 15, 2016

wildart Jun 15, 2016 •

edited

Loading

wildart commented Jun 16, 2016 •

edited

Loading

tkelman Jun 22, 2016

wildart commented Jun 23, 2016

tkelman commented Jun 23, 2016 •

edited

Loading

tkelman Jun 24, 2016

tkelman Jul 13, 2016

tkelman commented Jul 13, 2016

wildart commented Jul 13, 2016

tkelman commented Jul 13, 2016

tkelman commented Jul 14, 2016 •

edited

Loading

LibGit2: added tree related functions & tests #16537

LibGit2: added tree related functions & tests #16537

Conversation

wildart commented May 23, 2016

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

StefanKarpinski commented Jun 14, 2016

wildart commented Jun 15, 2016

Choose a reason for hiding this comment

tkelman Jun 15, 2016 • 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

wildart Jun 15, 2016 • edited Loading

Choose a reason for hiding this comment

wildart commented Jun 16, 2016 • edited Loading

Choose a reason for hiding this comment

wildart commented Jun 23, 2016

tkelman commented Jun 23, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tkelman commented Jul 13, 2016

wildart commented Jul 13, 2016

tkelman commented Jul 13, 2016

tkelman commented Jul 14, 2016 • edited Loading

tkelman Jun 15, 2016 •

edited

Loading

wildart Jun 15, 2016 •

edited

Loading

wildart commented Jun 16, 2016 •

edited

Loading

tkelman commented Jun 23, 2016 •

edited

Loading

tkelman commented Jul 14, 2016 •

edited

Loading