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

LibGit2 refactor #19839

Open
21 of 42 tasks
simonbyrne opened this issue Jan 3, 2017 · 16 comments
Open
21 of 42 tasks

LibGit2 refactor #19839

simonbyrne opened this issue Jan 3, 2017 · 16 comments
Labels
help wanted Indicates that a maintainer wants help on an issue or pull request libgit2 The libgit2 library or the LibGit2 stdlib module

Comments

@simonbyrne
Copy link
Contributor

simonbyrne commented Jan 3, 2017

The LibGit2 module could do with some work. Some possible changes

  • functions should mirror CLI (command line interface) git functionality when possible
    • e.g. LibGit2.checkout instead of LibGit2.branch!
    • possible exceptions when CLI does multiple things in one go
  • consistent use of ! suffix
  • use Enum and FlagEnum (pending RFC: EnumSet type #19470) for constants whenever possible
  • don't swallow errors:
    • e.g. rebase! will silently abort if it fails
    • revparse as well
  • Normalize and check path inputs, ref libgit2 path issues #18724.
  • Clean up a few objects which allocate memory in a confusing way:
    • StrArrayStruct: manually calls Libc.malloc, but then frees via LibGit2 git_strarray_free
    • Buffer: is an immutable object, but needs a finalizer
    • SignatureStruct, GitSignature, Signature objects should be simplified
  • split out LibGit2.get into more usefully-named functions
    • Maybe via constructors? e.g. GitBlob(repo::GitRepo, hash::GitHash)
    • when returning a GitObject, automatically resolve its type via git_object_type function
    • LibGit2.get(T, repo, oid) should throw an error (TypeError?) if incorrect type T is used.
  • Make GitConfig objects act like an Associative{String,String} (i.e. overload getindex/setindex!)
  • rename GitAnyObject to GitUnknownObject Deprecate GitAnyObject -> GitUnknownObject #19935
  • rename Oid to GitHash RFC: Rename LibGit2.Oid to LibGit2.GitHash #19878
  • Different types for full GitHash and short hashes: see RFC: Rename LibGit2.Oid to LibGit2.GitHash #19878 (comment)
  • More sensible errors and return values
    • LibGit2.fetch should return nothing (since it will throw an error for any other value)
    • LibGit2.reset! and rebase should return current HEAD commit
    • LibGit2.upstream and LibGit2.lookup_branch should throw errors if not found, instead of returning nothing
    • Don't use "zero" GitHash objects as null values.
  • Support newbase option for rebase! (cf Throw error on aborted rebase #19651 (comment))
  • Use correct iteration protocol for Base.next(::LibGit2.Rebase) (or until new iteration protocol is in place)
  • Combine LibGit2.owner and LibGit2.repository
  • Get rid of a lot of the with and explicit finalize/close calls (pending Attach finalizers to LibGit2 methods #19660)
  • Sensible show methods for various objects
    • GitBlob
    • GitCommit
    • GitRemote
    • GitSignature
    • GitTag
    • GitTreeEntry
    • GitReference
    • GitIndex
    • DiffDelta
    • DiffFile
    • FetchHead
    • GitTree
    • GitDiff
    • GitAnnotated
    • GitRebase
  • Better documentation (Add more comprehensive docstrings to Base.LibGit2 #18810)
    • what the function/type does
    • arguments/output value
    • equivalent git CLI command where appropriate
  • Better tests:
    • More coverage
    • Can test behaviour against command line Git
  • Consistent handling of git_*_options structs
    • Correct handling of memory allocation of pointer fields
    • Callback interface
  • Rename isdiff to a more descriptive name (fix some LibGit2 errors #20155 (comment))
  • get rid of cat?
    • It's more or less a wrapper around content.
  • document status/GitStatus return types and what results mean MORE docs for libgit2 #20503 (comment)

Some of these are breaking, so we may want to get the deprecations in place before 0.6.

@kshyatt kshyatt added the libgit2 The libgit2 library or the LibGit2 stdlib module label Jan 3, 2017
@simonbyrne
Copy link
Contributor Author

I should say that I would love some help with this if anyone is keen.

@simonbyrne simonbyrne added the help wanted Indicates that a maintainer wants help on an issue or pull request label Jan 4, 2017
@pfitzseb
Copy link
Member

pfitzseb commented Jan 8, 2017

Maybe add a

item to that list.

@kshyatt
Copy link
Contributor

kshyatt commented Jan 9, 2017

Currently we have show for:

  • Oid (soon to be Hash)
  • GitError
  • IndexEntry
  • RebaseOperation

The IndexEntry and RebaseOperation ones are really barebones.

@simonbyrne
Copy link
Contributor Author

simonbyrne commented Feb 21, 2017

One issue that has arisen a couple of times now (e.g. JuliaLang/PkgDev.jl#101) is how to check if an object is in a repo, then do or not do something, e.g. in calls to GitBlob(repo, ...).

The old code would return nothing, which is probably not a good idea as (1) it's type unstable, and (2) it is returning a type of a different object than called by the constructor.

Two possible options, neither of which I'm particularly keen on:

  1. Define a Nullable{GitBlob}(repo, ...) method:
nobj = Nullable{GitBlob}(repo, hash)
if !isnull(nobj)
   obj = get(nobj)
   ...
end
  • could make this a function instead of a constructor (e.g. tryobject or something)?
  1. Define an in method:
if hash in repo
   obj = GitBlob(repo, hash)
   ...
end
  • how do we check if it's actually a GitBlob, and not say, a GitCommit?
  • should we also overload getindex(repo, ...)?

Any better ideas? Or preferences on which is the least worst?

@tkelman
Copy link
Contributor

tkelman commented Feb 21, 2017

always returning Nullable{result} for most of these (probably using a function name instead of the type constructor name as the exported API) is what I'd prefer

@simonbyrne
Copy link
Contributor Author

any suggestions for such a name?

@kshyatt
Copy link
Contributor

kshyatt commented Feb 22, 2017

query_blob?

@kshyatt
Copy link
Contributor

kshyatt commented Feb 22, 2017

are_you_there_blob_its_me_margaret?

@kshyatt
Copy link
Contributor

kshyatt commented Feb 22, 2017

i_cant_believe_its_not_blobcedonia

@simonbyrne
Copy link
Contributor Author

tryobject?

@omus
Copy link
Member

omus commented Feb 24, 2017

functions should mirror CLI (command line interface) git functionality when possible

  • e.g. LibGit2.checkout instead of LibGit2.branch!
  • possible exceptions when CLI does multiple things in one go

Does this mean that LibGit2.checkout won't be able to create a branch? I would be ok with that.

@simonbyrne
Copy link
Contributor Author

The other problem is that you often want to know the reason why the field is null, e.g. https://github.com/JuliaLang/julia/pull/20752/files#diff-82889f262e9bc3ab1b675224c253a830R163

What if we were to define a special GitNullable type?:

immutable GitNullable{T}
    code::Error.Code
    value::T
end

Base.isnull(x::GitNullable) = x.code != Error.GIT_OK

and we were to create lower-level "try" interfaces based on this?

@tkelman
Copy link
Contributor

tkelman commented Mar 5, 2017

Not really a libgit2-specific concept, but would certainly be useful here. Ref https://github.com/iamed2/ResultTypes.jl

@simonbyrne
Copy link
Contributor Author

simonbyrne commented Mar 6, 2017

Here's a confusing one that I came across in #20916: git checkout can do several different things:

  1. change the working tree and index
  2. switch branches, i.e. change HEAD reference to a commit or branch (if called without a file path or without the --patch option)
  3. create a new branch (if called with the -b/-B option)

On the other hand, the libgit2 git_checkout_* functions only do 1 (and available in Julia via, e.g. checkout_tree). At the moment, we provide functionality for 1 & 2 via checkout! and 1,2 & 3 via branch!.

Too late for 0.6, but it would be nice to have a better and more consistent way to handle this.

@StefanKarpinski
Copy link
Member

I think the trying to emulate the git CLI one-on-one is not the best idea – the ways that it's overloaded are confusing and hard to use, IMO. Probably better to have a cleaner API that's more like an easy-to-use version of the libgit2 API.

@simonbyrne
Copy link
Contributor Author

Another one: in a couple of places it is useful to immediately peel an object obtained from a GitHash or refspec (i.e. to get the GitTree underlying a GitCommit).

Two options:

  1. A keyword arg to the constructors, e.g.:
GitTree(repo, hash, peel=true)
  1. Allow a hash or refspec arg to peel, e.g.:
peel(GitTree, repo, hash)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Indicates that a maintainer wants help on an issue or pull request libgit2 The libgit2 library or the LibGit2 stdlib module
Projects
None yet
Development

No branches or pull requests

6 participants