Run git-hooks more correctly

[Joshix-1]

This Pull Request fixes #2482.

It changes the following:

• Improve how git-hooks get run

I followed the checklist:

• I added unittests
• I ran make check without errors
• I tested the overall application
• I added an appropriate item to the changelog

Didn't test this on Windows

[extrawurst]

Is there any way to write a test for this?

[Joshix-1]

In GitoxideLabs/gitoxide@51bbb86#diff-b1f4fc8d8a8a51923a922a937745a1f5665443821b5c4df58efa44c7c4515216 there is a check if the file returned by path() exists. But that isn't really a useful test.

Maybe there could be some sort of integration test that creates a git-repo with hooks that then runs run_hook. But that wouldn't really help as that would only test one setup and this is about making it work on as many setups as possible.

[extrawurst]

Maybe there could be some sort of integration test that creates a git-repo with hooks that then runs run_hook

we do exactly that in the hooks unittests

[Joshix-1]

running the tests with SHELL=false or something like that, would check that the SHELL variable is ignored, but I'm not sure if that's helpful

[Joshix-1]

Why is -l added to the shell args?

git only adds -c https://github.com/git/git/blob/757161efcca150a9a96b312d9e780a071e601a03/run-command.c#L294-L306 and if i see that correctly it doesn't use string concatenation to provide arguments to the script.

[Joshix-1]

I think on non-Windows platforms it's best to just execute the hooks directly. I don't see a reason to use a shell.

[naseschwarz]

Thank you for your effort on this, @Joshix-1.
Generally, all of this is very reasonable.

Not executing bash -l on linux shaves off 300 ms on my system, too, which is a particularly pleasant aspect of this change! :)

git2-hooks is released separately from gitui, too, so we need to consider this regarding changes in git2-hooks behavior.

Two of the remarks below are just me writing down my thoughts, so that they're mentioned somewhat close to your change, both so why I'd argue that they're correct and so we could potentially refer to them.

[naseschwarz]

@Joshix-1: Thanks for your work on this. Is this ready for me to have another look?

[Joshix-1]

I think so

[extrawurst]

@Byron this one affects git2-hooks can you please have a look

[naseschwarz]

Thanks for your work. I realize that there's a TODO, but this correctly returns an error and it's still an improvement over what we had before. Thanks in particular for also taking care about making this work on a platform you're not using.

I also realize that you added broken UTF-8 to the tests. Naively I'd have wagered that this doesn't even work. It obviously does. TIL.

[naseschwarz]

Ah, one more thing: Would you please rebase or resolve conflicts?

[Byron]

Thanks for calling me in, @extrawurst , and thanks @Joshix-1 for tackling this!

I am very impressed by the attention to detail that went into this PR, it's beyond what I know about git-hooks. For all I know, there isn't much special about how Git executes hooks and gix-command would be able to capture all subtleties already.

In any case, it's an improvement, and if using gix-path or gix-command, that could definitely be addressed in a follow-up PR.

[extrawurst]

@Joshix-1 happy to move the two followups mentioned by @Byron into a later PR's - your call? if so please make an issue, link it in the conversation and close the convo so I can hit merge :)