proper pre-push hook implementation #2811
Conversation
|
I can confirm the issue appears to be gone when building from this branch. I am not that familiar with Rust development, so let me share the steps I followed to ensure I am not missing anything:
|
The way you did it works, if you wanna use this custom build in a few more places to test, simply run |
Excellent, that will get me through the last work day of the year. Thank you for solving the issue so quickly. Have a great end of the year. |
Branch case now uses the same destination ref logic as the push path: it honors `push.default=upstream` (when not deleting) via `branch_push_destination_ref`, so the `<remote-ref>` field matches the refspec Git would use.
ce17490 to
6998e42
Compare
|
@extrawurst I’ll try and have a look later this week! (I want to set up a proper repository with |
|
I started reviewing this PR, but since it’s a lot of new code and I’m not deeply familiar with hooks, it might take a while. :-) I can confirm, though, that the error message mentioned in the bug report is gone. Instead, |
cruessler
left a comment
There was a problem hiding this comment.
As far as I can tell, things are looking good! My understanding is that this PR adds proper passing of arguments and stdin to pre-push hook calls, to be more in line with what git does.
The changes in hooks.rs look good, but I haven’t been able to review them as detailed as the ones in lib.rs as this is an area I’m less familiar with.
Some of my comments are repetitive, e. g. ones to the effect of “Can this comment be removed”. I hope that’s more helpful than just a general “Some of the comments could potentially be removed”.
|
@cruessler ok thanks for the intensive review, the result I think is much better, no bash based asserting anymore and a better api on HookResult. Last sanity check please |
closes #2809