Run git-hooks more correctly #2483
Conversation
Joshix-1
force-pushed
the
git-hooks-shell-fix
branch
from
859aa5a to
2d90eb5
Compare
|
Is there any way to write a test for this? |
|
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. |
we do exactly that in the hooks unittests |
|
running the tests with |
|
Why is git only adds |
|
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. |
There was a problem hiding this comment.
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.
git2-hooks/src/hookspath.rs
Outdated
| let command = { | ||
| let mut os_str = std::ffi::OsString::new(); | ||
| os_str.push("'"); | ||
| os_str.push(hook.as_os_str()); // TODO: this doesn't work if `hook` contains single-quotes |
There was a problem hiding this comment.
This is much better than what we did before.
Do you think we could rid ourselves of this last TODO within this PR? Or would you prefer to leave it?
If you want a test for it, I've added a test test_hooks_commit_msg_reject_in_hooks_folder_githooks_moved_absolute that is sensitive in #2571 if patched to use repo_init_with_prefix.
There was a problem hiding this comment.
How would you fix that todo? Can we just replace the single quotes?
There was a problem hiding this comment.
The command variable is sh syntax, right? So we just need to replace all ' with '\''.
https://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_02_02
There was a problem hiding this comment.
I realised that we probably do not need to use -c without it we should be able to put the arguments just after
There was a problem hiding this comment.
Quoting from the link you sent:
Enclosing characters in single-quotes ( '' ) shall preserve the literal value of each character within the single-quotes. A single-quote cannot occur within single-quotes.
This will be fixed in gitui-org#2483
Joshix-1
force-pushed
the
git-hooks-shell-fix
branch
from
69224e3 to
38a3484
Compare
This will be fixed in gitui-org#2483
This will be fixed in gitui-org#2483
This will be fixed in gitui-org#2483
This will be fixed in gitui-org#2483
This will be fixed in gitui-org#2483
This will be fixed in gitui-org#2483
git2-hooks/src/hookspath.rs
Outdated
| ); | ||
| os_str.push(hook.replace('\'', REPLACEMENT)); | ||
| } else { | ||
| os_str.push(hook.as_os_str()); // TODO: this doesn't work if `hook` contains single-quotes |
There was a problem hiding this comment.
We should probably fail here if there is a single-quote in the string. Trying to execute something where we know it will fail sounds like a bad idea.
Escaping stuff in non-utf8 is hard.
This Pull Request fixes #2482.
It changes the following:
I followed the checklist:
make checkwithout errorsDidn't test this on Windows