Jitx 8574/libgit2 integration #83
Conversation
Can you make tickets for the fallbacks that need to be implemented if 1) they're not planned for this diff and 2) if they don't have tickets already? Nice work! |
…ng of annotated tags
|
Looking now. |
| run-git-command-in-dir(path(d), ["checkout", "--quiet", "--force", tag]) | ||
|
|
||
| ; Use git if available | ||
| if has-git?() : |
There was a problem hiding this comment.
You do this pattern a lot, ie - If we have the git binary - use it. Other wise - use libgit2.
I posit that:
- This puts a lot of the nuts and bolts of the git clone/fetch/sync/whatever in the application code of
slm - The interface to
gitis quite well defined for our purposes. - We can structure this slightly differently to make the abstraction easier to use.
For example - we could make a GitInterface base type with the following types of methods:
deftype GitInterface
defmulti fetch(tag:String) -> False
defmulti sync(tag:String) -> False
defmulti rev-parse(...) -> False
...
Then we can implement two concrete types for the git binary version and the libgit2 version.
Thoughts ? Any reason not to do this?
There was a problem hiding this comment.
That is what I initially tried to do, and would certainly look nicer. However, doing so would require repeatedly re-initializing libgit and re-opening the repository for each individual operation. I could try to create a "LibGitSession" object of sorts to avoid that, but passing that object around would require additional plumbing which the git binary version does not have. If we don't mind the performance overhead of re-initializing and re-opening for each operation I can change it.
There was a problem hiding this comment.
Can you please provide a more concrete example of where this is failing?
From my perspective - implementing as an interface like this allows us to hide all of those details and make the application layer code for SLM more maintainable.
| libgit2_init() | ||
|
|
||
| ; Clone | ||
| val repo = match(libgit2_clone(url, path, 1)): |
There was a problem hiding this comment.
I think libgit2_* wrapper functions should handle the error conversion to exception. You needing to handling each and every function call is not scalable.
It also prevents you from being able to do:
try:
... libgit2 calls here
catch():
... handling
finally:
libgit2_repository_free(repo)
libgit2_shutdown()
which would be a much cleaner pattern.
Lets move this conversion to exception code into the libgit2 library and clean up our interface code.
There was a problem hiding this comment.
In general I definitely prefer having cleaner interfaces, but in this case I think it would just add more hassle. I handle each libgit function's errors separately because they each require different reactions. For example, the libgit2_repository_free function should not be called if libgit2_repository_open or libgit2_clone themselves fail at the start. Some of the return codes are also "desirable" return values which I switch off of. For example, in my other branch, I use the return code of libgit-revparse to determine which style (v0.1.0 or 0.1.0) of tag exists in the local repository. I would rather not have to try-catch the result of that call to determine which tags exist. I also include information about the dependency and the requested user operation when throwing errors on the SLM side, information that would not be present on the libgit2 library side unless I explicitly plumbed it over.
There was a problem hiding this comment.
I would rather not have to try-catch the result of that call to determine which tags exist.
Can you expound on this ? What is the drawback here ?
There was a problem hiding this comment.
libgit-revparse returns the error code GIT_ENOTFOUND if a tag (or any reference-ish string in general) can not be resolved in the local repository. In my "non-v-prepended tags" branch (JITX-8660/non-v-tag), I check if the returned code is this in order to determine which tag style (v0.1.0 or 0.1.0) exists for a dependency. I would prefer to do this check with a switch or if else statement instead of a try catch.
There was a problem hiding this comment.
Actually the function is named libgit2_revparse not libgit-revparse, sorry for any confusion
There was a problem hiding this comment.
Right - but you haven't addressed my question:
I would prefer to do this check with a switch or if else statement instead of a try catch.
Why ?
There was a problem hiding this comment.
It technically works, but I think it's better to reserve exceptions for when something has actually gone wrong. I think it is also more awkward to read a try-catch versus a switch. Also, I would still need a switch in the try-catch anyways to distinguish between a GIT_ENOTFOUND exception versus other libgit exceptions.
|
Can we please add documentation to the help string in Can we also please add the |
|
Have we tested this in the |
Will do |
I agree that this branch needs a lot more testing. So far I've tried some simple testing where I run a bunch of |
I think the manual way would be to copy the Otherwise, we would need to make a slm pre-release |
…mplete pack header' error on Windows
Adds libgit fallbacks for clone, fetch, checkout, lsremote, and rev-parse. Also changes the default SLM_PROTOCOL to HTTPS and adds an error if SSH is requested without Git installed. Does not add fallbacks for git push (for slm publish) and git init (for slm init) yet.