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

Add support for Git Hooks #1186

Closed
6 tasks
crutchcorn opened this issue Jul 8, 2020 · 10 comments · Fixed by #1797
Closed
6 tasks

Add support for Git Hooks #1186

crutchcorn opened this issue Jul 8, 2020 · 10 comments · Fixed by #1797

Comments

@crutchcorn
Copy link
Contributor

crutchcorn commented Jul 8, 2020

I'd love to see support for the git hooks. Some of these hooks are:

  • pre-commit
  • prepare-commit-msg
  • commit-msg
  • post-commit
  • post-merge
  • pre-push

And others. They can be found via this resource and this resource in a more "plain-english" way.

I'm willing to implement this functionality, however, I'm wondering what the API should look like. Do we want to look for the related files and run them with spawn? Do we want to make pre-X and post-X actions and utilize a plugin-like system and provide this logic as an additional package? What do we think?

@jcubic
Copy link
Contributor

jcubic commented Jul 9, 2020

If you want to spawn new process you need to think about the browsers. I think that, if implemented,it should be compatible with canonical git but in browsers you don't have processes.

@crutchcorn
Copy link
Contributor Author

Definately @jcubic. My primary usage for isomorphic-git is in React Native, which has similar limitations. Needless to say, I have selfish reasons for keeping in browser limitations for moving forward.

I'm thinking that, if we move forward with integrating this logic into the package itself (as opposed to making a new one via the plugin system), that we allow passing a spawn prop that follows the Node API (similar to how fs is passed) that conditionally applies the hooks

@jcubic
Copy link
Contributor

jcubic commented Jun 30, 2021

Maybe the solution is to use some kind of events in the library, the old version if the library had an emitter option. But it got removed I think this will require thinking how those events will be added.

@CoalZombik
Copy link
Contributor

I would like to implement this issue. My idea is use similar strategy like in fs or http. Each function that use any of available hooks have an optional argument - a object with hook functions:

hooks: {
  preCommit?: ({fs, http, dir, gitdir}) => Promise<{fs, http, dir, gitdir}>,
  prepareCommitMsg?: ({fs, http, dir, gitdir, message, source, oid}) => Promise<{fs, http, dir, gitdir, message, source, oid}>,
  prePush?: ({fs, http, dir, gitdir, remote, url, refs}) => Promise<{fs, http, dir, gitdir, remote, url, refs}>,
  ...
}

For each hook exists one function (just as exists only one script in .git/hooks), but multiple hooks can be added via chained promises. Functions get arguments that is equal with original hooks and in addition base arguments from git function, because many of hooks probably need read/write files or use network.

Supporting hooks from .git/hooks can be implemented as object with wrappers to spawn function.

Still there is some details to think about:

  • Hooks manipulating with commit message received path of file with the commit message. I can't decide, if JS get filepath too or just message stored in memory as string.
  • Maybe add possibility to have function as commit message argument. This function will substitute text editor for editing message. Without it, prepare-commit-msg doesn't have any result, because will be overwritten.
  • Some git functions don't get http argument and then cannot be send to hook. Is not too limiting to include http only for some of hooks? Force to add http to every git function with hooks? Or do not send http (and probably fs too) to hook callbacks at all and rely on that hooks get these objects from elsewhere?
  • Is need to support every existing hooks? For example git am have not any alternative in isomorphic-git, then hooks applypath-mgs, pre-applypatch, post-applypatch are not needed.

@jcubic
Copy link
Contributor

jcubic commented Aug 27, 2022

@CoalZombik what is your use case with Git hooks? I now thinking about this and I'm wondering what is the point of having hooks. If you have code that runs git.commit, why do you need pre-commit if you can create an Adapter that will do your code and then invoke git.commit? Why making the library more complex? It's your code that executes the git function.

With canonical git it's different because it's single command and if you want to invoke some code automatically you need to write git hook. I don't think that you can have the same functionality with aliases git hooks are more convenient for this purpose. But if you have API and programming language you have full control over git functions, so you don't need hooks. Unless there are hooks into the internals of git.

Also if you want to use .git/hooks you will need to support shell scripts and it will be dependent on the Unix system. It will not work on Windows and in Browser. So I don't think it's a good idea after all.

@CoalZombik
Copy link
Contributor

I am thinking about some support of git lfs that need pre-push hook to send tracked files to remote server. Difference between adapter and hook is existence of additional information about process, hook contains list of changed refs and adapter must get this information alone (probably with same code as in commit function).

(Now I release I had talk about this in lfs issue first and don't want to implement only part of whole, sorry about that.)

@jcubic
Copy link
Contributor

jcubic commented Aug 27, 2022

I'm ok for hooks if they get internals of git like refs. But we need to come up with an API that will match the current architecture of passing stuff to each function.

@CoalZombik
Copy link
Contributor

I looked more deeply to existing git hooks and I realized only two hooks is worth it to implement: post-checkout and pre-push.

Hook Arguments Related command To implement
applypatch-msg messageFile: string None
pre-applypatch None None
post-applypatch None None
pre-commit None commit
pre-merge-commit None merge
prepare-commit-msg messageFile: string
source: 'message' | 'merge' (template and commit not supported)
 commitOid?: string (not supported)
commit
merge
commit-msg messageFile: string commit
merge
post-commit None commit
pre-rebase upstream: string
branch?: string
None (#189)
post-checkout previousHead: string
newHead: string
branchCheckout: boolean or type: 'branch' | 'file'
checkout
clone
post-merge squashed: boolean (not supported) merge
pre-push remote: string
remoteUrl: string
localRef: string
localOid: string
remoteRef: string
remoteOid: string
push
pre-auto-gc None None
post-rewrite source: string
oldOid: string
newOid: string
None (#189)

About API, I am thinking about callback that is passed to related function and called in valid time, same as for example onAuthSuccess, onProgress or onSign callbacks.

@jcubic
Copy link
Contributor

jcubic commented Aug 28, 2022

Sounds like a plan

@CoalZombik
Copy link
Contributor

Sorry for very long response, but I finally created a pull request with implementation of these two hooks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants