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.

Sign up for GitHub

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

Jump to bottom

RFC: Rename LibGit2.Oid to LibGit2.GitHash #19878

Merged
merged 4 commits into from
Jan 9, 2017
Merged

RFC: Rename LibGit2.Oid to LibGit2.GitHash #19878

merged 4 commits into from
Jan 9, 2017

Conversation

ararslan
Copy link
Member

@ararslan ararslan commented Jan 5, 2017

This addresses one of the points in #19839: renaming Oid to GitHash in LibGit2. I only did this one rename in this PR, since the diff is fairly large as it is.

cc @simonbyrne

@simonbyrne
Copy link
Contributor

simonbyrne commented Jan 5, 2017 •
edited
Loading

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:

  • GitHash as defined in this PR
  • GitShortHash as
immutable GitShortHash <: AbstractGitHash
    hash::GitHash
    len::Csize_t
end

Additionally:

  • GitHash(::String) would throw an error if the string is too short.
  • Define a macro githash_str which would create the appropriate object depending on the input length.

@tkelman tkelman added the libgit2 The libgit2 library or the LibGit2 stdlib module label Jan 5, 2017
@simonbyrne simonbyrne mentioned this pull request Jan 5, 2017
Open
42 tasks
@ararslan
Copy link
Member Author

ararslan commented Jan 6, 2017

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.

@simonbyrne
Copy link
Contributor

simonbyrne commented Jan 6, 2017 via email

@ararslan
Copy link
Member Author

ararslan commented Jan 6, 2017

I've pushed my interpretation of the plan you outlined. Will add some tests as well for the new functionality.

@ararslan ararslan changed the title RFC: Rename LibGit2.Oid to LibGit2.GitHash RFC: Rename LibGit2.Oid to LibGit2.GitHash, add GitShortHash Jan 6, 2017
@ararslan
Copy link
Member Author

ararslan commented Jan 7, 2017

Alright, well those changes broke Pkg, Documenter, or both...

@MichaelHatherly
Copy link
Member

Seems like it failed on Pkg.update, not in Documenter itself. As far as I recall I've not been using anything from Base.LibGit2 aside from GITHUB_REGEX.

tkelman
tkelman reviewed Jan 7, 2017
View reviewed changes
base/libgit2/commit.jl
tree_id::Oid = Oid(),
parent_ids::Vector{Oid}=Oid[])
tree_id::AbstractGitHash = GitHash(),
parent_ids::Vector{AbstractGitHash}=AbstractGitHash[])
Copy link
Contributor

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?

Copy link
Member Author

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.

Copy link
Contributor

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}

Copy link
Member Author

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.

@ararslan
Copy link
Member Author

ararslan commented Jan 7, 2017

Good to know, thanks @MichaelHatherly.

@ararslan
Copy link
Member Author

ararslan commented Jan 8, 2017

For now I'm just going to remove the commits that extend functionality and return this PR to its initial scope of simply renaming Oid. I'll work on the other changes some more and post it as a separate PR once I've got things figured out.

@ararslan ararslan force-pushed the aa/libgit2-renames branch from 92c5881 to 7d9edd3 Compare January 8, 2017 00:24
@ararslan ararslan changed the title RFC: Rename LibGit2.Oid to LibGit2.GitHash, add GitShortHash RFC: Rename LibGit2.Oid to LibGit2.GitHash Jan 8, 2017
@ararslan
Copy link
Member Author

ararslan commented Jan 8, 2017

Fascinating. My initial renaming commit had originally passed its tests but now they're failing. Hmm.

@tkelman
Copy link
Contributor

tkelman commented Jan 8, 2017

are there new occurrences now on master from anything that got merged in the interim?

@ararslan ararslan force-pushed the aa/libgit2-renames branch from b50967b to 2803d76 Compare January 8, 2017 21:32
@ararslan
Copy link
Member Author

ararslan commented Jan 8, 2017

You're a wizard, Tony. That seems to have been the problem.

@kshyatt kshyatt added the deprecation This change introduces or involves a deprecation label Jan 8, 2017
@ararslan
Copy link
Member Author

ararslan commented Jan 8, 2017 •
edited
Loading

FYI: I've pushed the stuff with GitShortHash that I had been working on here to a separate branch in my fork, which I will continue to update: https://github.com/ararslan/julia/tree/aa/gitshorthash

tkelman
tkelman reviewed Jan 9, 2017
View reviewed changes
base/libgit2/repository.jl
@@ -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))
Copy link
Contributor

Choose a reason for hiding this comment

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

should be repo now

@ararslan
Copy link
Member Author

ararslan commented Jan 9, 2017

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?

@simonbyrne simonbyrne merged commit 0b8b6f1 into JuliaLang:master Jan 9, 2017
@simonbyrne
Copy link
Contributor

Thanks!

@ararslan ararslan deleted the aa/libgit2-renames branch January 9, 2017 21:08
@simonbyrne simonbyrne mentioned this pull request Jan 10, 2017
Merged
@Sacha0 Sacha0 added the needs news A NEWS entry is required for this change label May 13, 2017
@Sacha0 Sacha0 mentioned this pull request May 13, 2017
Closed
@bjarthur
Copy link
Contributor

NEWS.md incorrectly says GitOid (instead of Oid) was renamed to GitHash.

also, it would've been nice to add this to Compat, though at this late stage maybe not worth the effort.

@KristofferC KristofferC removed the needs news A NEWS entry is required for this change label Nov 13, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tkelman tkelman tkelman left review comments

Assignees
No one assigned
Labels
deprecation This change introduces or involves a deprecation libgit2 The libgit2 library or the LibGit2 stdlib module
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@ararslan @simonbyrne @MichaelHatherly @tkelman @bjarthur @kshyatt @KristofferC @Sacha0