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.

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

Fix return type of git_blob_is_binary() #20272

Merged
merged 1 commit into from
Jan 28, 2017
Merged

Fix return type of git_blob_is_binary() #20272

merged 1 commit into from
Jan 28, 2017

Conversation

nalimilan
Copy link
Member

This triggered a failure in the libgit2 blobs test on i686 when building
RPM nightlies. I don't understand why this error does not happen on CI.

This code was introduced in 78133de.

Cc: @kshyatt

This triggered a failure in the libgit2 blobs test on i686 when building
RPM nightlies. I don't understand why this error does not happen on CI.
@tkelman
Copy link
Contributor

tkelman commented Jan 27, 2017

What version of libgit2 was that building against? I don't know whether this API has changed at all, but what was the failure you were seeing?

@tkelman tkelman added bugfix This change fixes an existing bug libgit2 The libgit2 library or the LibGit2 stdlib module labels Jan 27, 2017
@nalimilan
Copy link
Member Author

I've checked the libgit2 code, and that function hasn't been touched for three years. But indeed I was using 0.24.6, so that might have revealed the bug for some obscure reason while it was hidden with 0.25.

@nalimilan
Copy link
Member Author

Error was:

Error in testset libgit2:
Test Failed
  Expression: LibGit2.isbinary(blob)
Error in testset libgit2:
Error During Test
  Got an exception of type MethodError outside of a @test
  MethodError: no method matching split(::Ptr{Void}, ::String)
  Closest candidates are:
    split{T<:SubString}(!Matched::T<:SubString, ::Any; limit, keep) at strings/util.jl:231
    split{T<:AbstractString}(!Matched::T<:AbstractString, ::Any; limit, keep) at strings/util.jl:254
  Stacktrace:
   [1] #wrapped_lines#175(::Int32, ::Int32, ::Function, ::String) at ./markdown/render/terminal/formatting.jl:68
   [2] #wrapped_lines#175(::Int32, ::Int32, ::Function, ::String) at ./markdown/render/terminal/formatting.jl:67
   [3] VersionNumber(::Int32, ::Int32, ::Int32, ::Tuple{}, ::Tuple{}) at ./version.jl:38 (repeats 2 times)
ERROR: LoadError: 

https://copr-be.cloud.fedoraproject.org/results/nalimilan/julia-nightlies/fedora-26-i386/00504417-julia/build.log.gz

@tkelman
Copy link
Contributor

tkelman commented Jan 27, 2017

Looks like you found 2 bugs then, and you're only fixing the easy one! split(content(blob), "\n") isn't going to work if content(blob) returns a Ptr{Void}. The show(::IO, ::GitBlob) was only being tested for binary GitBlobs (aside: funny naming on libgit2's part, I was under the impression that the first b in blob meant binary), you just happened to hit that code path in your tests by accident before the CI here happened to do so. I do wonder what's different about your environment than the CI here that would cause an incorrect ccall return type to behave differently, or maybe it's intermittent.

@nalimilan
Copy link
Member Author

Indeed. I'd appreciate if somebody familiar with libgit2 could fix the second bug, as I don't really now what are the different types of GitBlobs.

FWIW, the failure was pretty systematic on the Copr.

@simonbyrne
Copy link
Contributor

The content issue is fixed by #20104 (see https://github.com/JuliaLang/julia/pull/20104/files#diff-3384e375f713976e7e985a7dfe83a626R3).

@kshyatt
Copy link
Contributor

kshyatt commented Jan 27, 2017

So I don't have to do anything? Thanks for finding and fixing this!

@simonbyrne simonbyrne merged commit 34d0f15 into master Jan 28, 2017
@simonbyrne simonbyrne deleted the nl/blobs branch January 28, 2017 21:26
@tkelman
Copy link
Contributor

tkelman commented Jan 28, 2017

Would be good to dig into why only you were hitting this.

@nalimilan
Copy link
Member Author

Hard to tell, but I'm the only one to use 0.24 and it doesn't sound impossible that the 32 additional bytes happened to be all zeros on 0.25, which most people use. Bugs involving bools are less likely to be noticed than those involving types in which all 64-bits are actually significant.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix This change fixes an existing bug libgit2 The libgit2 library or the LibGit2 stdlib module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants