Hook support - #4620
Conversation
|
Editor's note : I should really have those .h files reversed. |
| } | ||
|
|
||
| if (!inmemory && upstream) { | ||
| if ((error = git_hook_call_pre_rebase(repo, upstream, (head_ref == NULL ? branch : NULL))) < 0) |
There was a problem hiding this comment.
Do we really want to check for < 0 here? Usually, return codes from exec'd programs are always positive in the range from 0 to 255, where non-zero positive values denote an error. So in case the callback simply exec's the hook and returns its value, we will not detect that error.
There was a problem hiding this comment.
I'm defaulting to the libgit2 convention. Right now the expectation is that the executor will bubble a GIT_* error, not the underlying exec return value.
|
There's context about what can be matched to our API/workflow/implementation-level here, which I've moved in this PR's summary. Also, someone recently asked about merge drivers in OCGit libgit2/objective-git#656, and this made me wonder about that exec support… |
7bfe136 to
4632523
Compare
|
Review comments handled. Note that I've moved the WIP commit hook support in, but it's really WIP. If someone has a better idea for wrapping prepare-commit-msg (and it's variable 5 arguments + command line flags + some repo state) that does not entails having 5 different functions, I'm 👂… About this :
It all depends on how I specify the valid return codes from the executor. My preference goes to keeping the libgit2 convention, which would make the executor responsible of not breaking us by returning positive error codes. |
|
Note that I've swapped functions around, so the low-level execution is in
|
b62ad25 to
2d6b7d6
Compare
|
Rebased, and updated. I've closed all review comments that seemed to have been resolved, and thus I'm dropping the WIP from it, as I feel it's a sufficient first step to merit inclusion, on the off-chance that it's acceptable to not have all current hook "helpers" wired yet. |
|
Can we have this now? |
2d6b7d6 to
4563cf8
Compare
|
Is there any status update on this pull request? |
|
On Wed, Feb 05, 2020 at 02:43:24AM -0800, Nicolas Loots wrote:
Is there any status update on this pull request?
We are currently in code freeze until v1.0 is released, probably around
early March. Until then we're holding off on all new features, but will
consider merging them as soon as v1.0 is out.
|
|
Now that 1.0 shipped, can we consider this PR? That would be amazing 🤞 |
|
Also wondering what keeps us from considering this now post-1.0 |
4563cf8 to
508ef04
Compare
508ef04 to
36c2150
Compare
36c2150 to
a466d92
Compare
|
I am also very interested in being able to support precommit hooks in my application using libgit2 (Rust version to be specific). What is the state of work on them? |
|
From 2018 to 2021, I can't wait. ... |
|
2024 and we're still here |
|
@Pyenb What do you want hook support to look like? Are these the APIs that you want? |
This is a heavily redacted version of #3824, since it's been 2 years in the making and much has changed (like nice commit headers). Most of the design comes from the discussion there, so you might want to take a look.
The plan is to have a low-level API for hook execution (because as a library, we don't want to go anywhere near
fork/exec/the Windows way), so we can instead ping "userspace" that we expect them to execute something with carefully prepared arguments. On top of that comes a somewhat higher-level API so that users should just need to call a single function with some of their current working objects and we'll translate those to one of those low-level "pings".Note that's there a feature/hooks-wip branch, which is even more WIP than this one, where I'm adding easy-to-use helpers for the various hooks.
EDIT Tentative support plan :
applypatch-msg, pre-applypatch, post-applypatch: no git-am.pre-commit, prepare-commit-msg, commit-msg, post-commit: our "commit workflow" doesn't match git.post-checkout: the "normal checkout" looks like a "no matching workflow" too, as our checkout doesn't work on references, but directly on HEAD/index/tree. The "clone checkout" looks ok though.post-merge: no git-pull.pre-receive, update, post-receive, post-update, push-to-checkout: no git-receive-pack.pre-auto-gc: no git-gc.