Hook support #4620
Hook support #4620
Conversation
|
Editor's note : I should really have those .h files reversed. |
| @@ -718,6 +719,11 @@ int git_rebase_init( | |||
| branch = head_branch; | |||
| } | |||
|
|
|||
| 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… |
tiennou
force-pushed
the
feature/hooks
branch
2 times, most recently
from
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
|
tiennou
force-pushed
the
feature/hooks
branch
3 times, most recently
from
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? |
tiennou
force-pushed
the
feature/hooks
branch
from
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 |
tiennou
force-pushed
the
feature/hooks
branch
from
4563cf8
to
508ef04
Compare
tiennou
force-pushed
the
feature/hooks
branch
from
508ef04
to
36c2150
Compare
tiennou
force-pushed
the
feature/hooks
branch
from
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. ... |
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.