Closed Gelio closed 2 years ago
Thanks for writing this all up!
git-branchless's logic for calling hooks is at https://github.com/arxanas/git-branchless/blob/496ad073d28382e967fc4e27eeeab26d6dc4c06f/src/git/run.rs#L349
an example of a hook call: https://github.com/arxanas/git-branchless/blob/496ad073d28382e967fc4e27eeeab26d6dc4c06f/src/core/rewrite/execute.rs#L115
Some other notes about hooks:
post-rewrite
:
git split
operation.drop
ped it as part of an interactive rebase. (I looked into fixing it upstream, but it's non-trivial.) git-branchless manually rewrites some commits to the zero OID to indicate that they've been dropped: https://github.com/arxanas/git-branchless/blob/496ad073d28382e967fc4e27eeeab26d6dc4c06f/src/core/rewrite/rewrite_hooks.rs#L468-L469. Although it's not explicitly specced in for Git hooks (well, not much is), I encourage git-stack
to explicitly indicate dropped commits in this way, if it creates any.reference-transaction
hook, you're supposed to first call it with prepared
and then abort the transaction if the hook returns non-zero (and call it again with aborted
). When calling hooks itself, git-branchless currently doesn't do that because it's a lot of work. Instead, it just calls the reference-transaction
hook with committed
. It's probably not a big deal if you were to defer implementing the prepared
/aborted
calls, since I'm not aware of any user-facing tooling that actually relies on this.sh
/bash
. If I recall correctly, this is to support Windows, which wouldn't normally be able to execute a shell script. If you don't support Windows, then you could skip that bit.git-stack
and which creates a dummy hook that just echoes its input. Then you can assert that the output includes the commit hashes you expect.Thanks for the details
I'm trying to focus on my current project (merging cargo-add into cargo) but will focus on this next.
In pulling out the hook calling code, I think I finally have a "good enough" (but not great) name for a shared org / repo for us to share logic like this, gitext-rs/git2ext
. If people have better ideas, I'm willing to take them.
I've gotten git2ext
out with some basic hooks support: https://docs.rs/git2ext/latest/git2ext/hooks/struct.Hooks.html
@arxanas I've tweaked some things from git-branchless to be closer to what git does. However, I did not implement some of the extra control you've implemented (distinct copy of environment, forwarding to custom streams). I wanted to get it implemented and see how it works out before trying to figure out how to allow the API to support those cases.
As a test run, I've implemented hook support for git branch-stash pop
. Hook support for git stack
will take a bit more work, so focusing on #194 first.
v0.7.2 is released with hook support
Awesome, thanks for all the effort put in delivering this functionality. I've just done git stack --pull
and git sl
seems to be only showing the rebased commits, meaning the hook seems to have been correctly invoked :tada:
I'm creating this issue as a follow-up to https://github.com/epage/git-stack/discussions/93#discussioncomment-2298830 to gather the information required to invoke git hooks after invoking
git stack
commands. This should improve the interoperability betweengit-stack
and git-branchless.Requirements
According to @arxanas, the following hooks should be called for
git-branchless
to be aware of the changes:post-commit
post-rewrite
post-merge
reference-transaction
source from
git-branchless
Hooks support in
libgit2
Unfortunately, calling git hooks is not supported yet in
libgit2
. There is https://github.com/libgit2/libgit2/pull/4620 that is supposed to add support for it. Considering the fact it has been open for almost 4 years and the last update was 2 years ago, I doubt it will get merged soon.Calling hooks by
git-stack
I believe
git-stack
could call the hooks manually. Let's investigate hooks to be called: which, when, and how. I will use the git hooks documentation as a base.post-commit
andpost-merge
I am not aware of
git-stack
creating any commits itself. I have only been using it to rebase branches, which does not create new commits.Thus, I believe the
post-commit
andpost-merge
hooks do not have to be called bygit-stack
.post-rewrite
The
post-rewrite
hook seems like the most important hook to be called bygit-stack
. It is called bygit rebase
, which is the command thatgit stack --pull
andgit stack --rebase
enhance.It requires passing the information about the rewrite to the script's standard input (i.e. hashes of old and new commits). This includes information about squashes/fixups.
extra-info
is optional for that hook. Looking atgit-branchless
' implementation of that hook handler, onlyold-object-name
andnew-object-name
are relevant.reference-transaction
My understanding of the
reference-transaction
hook is that it is called before and after updating the refs. Forgit-stack
this means the hook should be called after rebasing branches, since then the refs change (local branches, as well asHEAD
, is updated). This is becausegit-branchless
only cares aboutcommitted
transactions.Git hooks summary
If what I gathered above is true,
git-stack
should only callpost-rewrite
andreference-transaction
. They should be invoked after the in-memory changes are done and committed to the filesystem.Proposal how to call hooks
My limited understanding of the source code of
git-stack
leads me to believe that we would need to modify 2 places:When executing
crate::git::Command
s, remember the changedCherryPick
andFixup
commands)CreateBranch
andDeleteBranch
commands)I believe the
Executor
is the right place to store that information since that's the struct that executes commands.After all the scripts are executed, invoke
post-rewrite
hook with the list of changed commit IDsreference-transaction
hook with the list of updated referencesHow to test calling hooks
I haven't thought about it yet.
I could try to contribute some parts of this functionality after we agree on the general direction. Keep in mind I haven't verified that calling those 2 hooks will be sufficient for
git-branchless
to pick up the changes, but it looks probable.