-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
RFC: Rename LibGit2.Oid to LibGit2.GitHash #19878
Conversation
Fantastic thanks. However there is one other thing that has since occurred to me that we may want to change. Command line git accepts both full and shortened hashes whenever they're unique. LibGit2 implements this by providing:
Currently this is exposed in a somewhat cumbersome manner. e.g. here you need to manually provide the length of the hash. My current thinking is to define 2 types:
immutable GitShortHash <: AbstractGitHash
hash::GitHash
len::Csize_t
end Additionally:
|
I've implemented bits and pieces of what you outlined, Simon. It's turning into a somewhat bigger job than I had intended this PR to be, however, so is there any chance I could do that as a separate PR? I think that alone will produce a sizable diff, and for those changes I'd want to get more focused feedback on design aspects, etc. Does that seem reasonable? If not I'll just keep chipping away at it here. |
No worries, sorry for lumping that on you. Add whatever you've done, and
I'll have a look over the weekend to see if I can sort it out.
|
I've pushed my interpretation of the plan you outlined. Will add some tests as well for the new functionality. |
Alright, well those changes broke Pkg, Documenter, or both... |
Seems like it failed on |
tree_id::Oid = Oid(), | ||
parent_ids::Vector{Oid}=Oid[]) | ||
tree_id::AbstractGitHash = GitHash(), | ||
parent_ids::Vector{AbstractGitHash}=AbstractGitHash[]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does this usually get passed as a vector of one or the other concretely-typed hash objects?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Before this change, it's always been a vector of concretely-typed hash objects because there was only one hash type. I didn't think letting this be either would cause a problem, since the hashes are passed to get
individually, and the separate get
methods return the same type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
well now it might be a Vector{GitHash}
or Vector{GitShortHash}
which are not subtypes of Vector{AbstractGitHash}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I see what you mean. Hm.
Good to know, thanks @MichaelHatherly. |
For now I'm just going to remove the commits that extend functionality and return this PR to its initial scope of simply renaming |
92c5881
to
7d9edd3
Compare
Fascinating. My initial renaming commit had originally passed its tests but now they're failing. Hmm. |
are there new occurrences now on master from anything that got merged in the interim? |
b50967b
to
2803d76
Compare
You're a wizard, Tony. That seems to have been the problem. |
FYI: I've pushed the stuff with |
@@ -112,7 +112,7 @@ function get{T <: GitObject}(::Type{T}, repo::GitRepo, oid::Oid, oid_size::Int=O | |||
end | |||
|
|||
function get{T <: GitObject}(::Type{T}, repo::GitRepo, oid::AbstractString) | |||
return get(T, repo, Oid(oid), length(oid)) | |||
return get(T, r, GitHash(oid), length(oid)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should be repo now
Hey, thanks so much for the extra commits, Simon! It must be that "allow edits from maintainers" feature that allows you to do that. Much appreciated! 👍 Is this good to go now? |
Thanks! |
NEWS.md incorrectly says also, it would've been nice to add this to |
This addresses one of the points in #19839: renaming
Oid
toGitHash
in LibGit2. I only did this one rename in this PR, since the diff is fairly large as it is.cc @simonbyrne