Skip to content

Commit cb52f7b

Browse files
DaRacciDaRacci
committed
feat(asyncgit): add timeouts for hooks
1 parent 687d429 commit cb52f7b

File tree

2 files changed

+192
-31
lines changed

2 files changed

+192
-31
lines changed

‎asyncgit/src/sync/hooks.rs‎

Lines changed: 177 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,10 @@ use super::{repository::repo, RepoPath};
22
use crate::error::Result;
33
pub use git2_hooks::PrepareCommitMsgSource;
44
use scopetime::scope_time;
5+
use std::{
6+
sync::mpsc::{channel, RecvTimeoutError},
7+
time::Duration,
8+
};
59

610
///
711
#[derive(Debug, PartialEq, Eq)]
@@ -26,50 +30,148 @@ impl From<git2_hooks::HookResult> for HookResult {
2630
}
2731
}
2832

33+
fn run_with_timeout<F>(
34+
f: F,
35+
timeout: Duration,
36+
) -> Result<(HookResult, Option<String>)>
37+
where
38+
F: FnOnce() -> Result<(HookResult, Option<String>)>
39+
+ Send
40+
+ Sync
41+
+ 'static,
42+
{
43+
if timeout.is_zero() {
44+
return f(); // Don't bother with threads if we don't have a timeout
45+
}
46+
47+
let (tx, rx) = channel();
48+
let _ = std::thread::spawn(move || {
49+
let result = f();
50+
tx.send(result)
51+
});
52+
53+
match rx.recv_timeout(timeout) {
54+
Ok(result) => result,
55+
Err(RecvTimeoutError::Timeout) => Ok((
56+
HookResult::NotOk("hook timed out".to_string()),
57+
None,
58+
)),
59+
Err(RecvTimeoutError::Disconnected) => {
60+
unreachable!()
61+
}
62+
}
63+
}
64+
2965
/// see `git2_hooks::hooks_commit_msg`
3066
pub fn hooks_commit_msg(
3167
repo_path: &RepoPath,
3268
msg: &mut String,
69+
timeout: Duration,
3370
) -> Result<HookResult> {
3471
scope_time!("hooks_commit_msg");
3572

36-
let repo = repo(repo_path)?;
73+
let repo_path = repo_path.clone();
74+
let mut msg_clone = msg.clone();
75+
let (result, msg_opt) = run_with_timeout(
76+
move || {
77+
let repo = repo(&repo_path)?;
78+
Ok((
79+
git2_hooks::hooks_commit_msg(
80+
&repo,
81+
None,
82+
&mut msg_clone,
83+
)?
84+
.into(),
85+
Some(msg_clone),
86+
))
87+
},
88+
timeout,
89+
)?;
3790

38-
Ok(git2_hooks::hooks_commit_msg(&repo, None, msg)?.into())
91+
if let Some(updated_msg) = msg_opt {
92+
msg.clear();
93+
msg.push_str(&updated_msg);
94+
}
95+
96+
Ok(result)
3997
}
4098

4199
/// see `git2_hooks::hooks_pre_commit`
42-
pub fn hooks_pre_commit(repo_path: &RepoPath) -> Result<HookResult> {
100+
pub fn hooks_pre_commit(
101+
repo_path: &RepoPath,
102+
timeout: Duration,
103+
) -> Result<HookResult> {
43104
scope_time!("hooks_pre_commit");
44105

45-
let repo = repo(repo_path)?;
46-
47-
Ok(git2_hooks::hooks_pre_commit(&repo, None)?.into())
106+
let repo_path = repo_path.clone();
107+
run_with_timeout(
108+
move || {
109+
let repo = repo(&repo_path)?;
110+
Ok((
111+
git2_hooks::hooks_pre_commit(&repo, None)?.into(),
112+
None,
113+
))
114+
},
115+
timeout,
116+
)
117+
.map(|res| res.0)
48118
}
49119

50120
/// see `git2_hooks::hooks_post_commit`
51-
pub fn hooks_post_commit(repo_path: &RepoPath) -> Result<HookResult> {
121+
pub fn hooks_post_commit(
122+
repo_path: &RepoPath,
123+
timeout: Duration,
124+
) -> Result<HookResult> {
52125
scope_time!("hooks_post_commit");
53126

54-
let repo = repo(repo_path)?;
55-
56-
Ok(git2_hooks::hooks_post_commit(&repo, None)?.into())
127+
let repo_path = repo_path.clone();
128+
run_with_timeout(
129+
move || {
130+
let repo = repo(&repo_path)?;
131+
Ok((
132+
git2_hooks::hooks_post_commit(&repo, None)?.into(),
133+
None,
134+
))
135+
},
136+
timeout,
137+
)
138+
.map(|res| res.0)
57139
}
58140

59141
/// see `git2_hooks::hooks_prepare_commit_msg`
60142
pub fn hooks_prepare_commit_msg(
61143
repo_path: &RepoPath,
62144
source: PrepareCommitMsgSource,
63145
msg: &mut String,
146+
timeout: Duration,
64147
) -> Result<HookResult> {
65148
scope_time!("hooks_prepare_commit_msg");
66149

67-
let repo = repo(repo_path)?;
150+
let repo_path = repo_path.clone();
151+
let mut msg_cloned = msg.clone();
152+
let (result, msg_opt) = run_with_timeout(
153+
move || {
154+
let repo = repo(&repo_path)?;
155+
Ok((
156+
git2_hooks::hooks_prepare_commit_msg(
157+
&repo,
158+
None,
159+
source,
160+
&mut msg_cloned,
161+
)?
162+
.into(),
163+
Some(msg_cloned),
164+
))
165+
},
166+
timeout,
167+
)?;
168+
169+
if let Some(updated_msg) = msg_opt {
170+
msg.clear();
171+
msg.push_str(&updated_msg);
172+
}
68173

69-
Ok(git2_hooks::hooks_prepare_commit_msg(
70-
&repo, None, source, msg,
71-
)?
72-
.into())
174+
Ok(result)
73175
}
74176

75177
#[cfg(test)]
@@ -82,7 +184,7 @@ mod tests {
82184
let (_td, repo) = repo_init().unwrap();
83185
let root = repo.path().parent().unwrap();
84186

85-
let hook = b"#!/bin/sh
187+
let hook = b"#!/usr/bin/env sh
86188
echo 'rejected'
87189
exit 1
88190
";
@@ -96,9 +198,11 @@ mod tests {
96198
let subfolder = root.join("foo/");
97199
std::fs::create_dir_all(&subfolder).unwrap();
98200

99-
let res =
100-
hooks_post_commit(&subfolder.to_str().unwrap().into())
101-
.unwrap();
201+
let res = hooks_post_commit(
202+
&subfolder.to_str().unwrap().into(),
203+
Duration::ZERO,
204+
)
205+
.unwrap();
102206

103207
assert_eq!(
104208
res,
@@ -119,7 +223,7 @@ mod tests {
119223
let workdir =
120224
crate::sync::utils::repo_work_dir(repo_path).unwrap();
121225

122-
let hook = b"#!/bin/sh
226+
let hook = b"#!/usr/bin/env sh
123227
echo $(pwd)
124228
exit 1
125229
";
@@ -129,7 +233,8 @@ mod tests {
129233
git2_hooks::HOOK_PRE_COMMIT,
130234
hook,
131235
);
132-
let res = hooks_pre_commit(repo_path).unwrap();
236+
let res =
237+
hooks_pre_commit(repo_path, Duration::ZERO).unwrap();
133238
if let HookResult::NotOk(res) = res {
134239
assert_eq!(
135240
std::path::Path::new(res.trim_end()),
@@ -145,7 +250,7 @@ mod tests {
145250
let (_td, repo) = repo_init().unwrap();
146251
let root = repo.path().parent().unwrap();
147252

148-
let hook = b"#!/bin/sh
253+
let hook = b"#!/usr/bin/env sh
149254
echo 'msg' > $1
150255
echo 'rejected'
151256
exit 1
@@ -164,6 +269,7 @@ mod tests {
164269
let res = hooks_commit_msg(
165270
&subfolder.to_str().unwrap().into(),
166271
&mut msg,
272+
Duration::ZERO,
167273
)
168274
.unwrap();
169275

@@ -174,4 +280,53 @@ mod tests {
174280

175281
assert_eq!(msg, String::from("msg\n"));
176282
}
283+
284+
#[test]
285+
fn test_hooks_respect_timeout() {
286+
let (_td, repo) = repo_init().unwrap();
287+
let root = repo.path().parent().unwrap();
288+
289+
let hook = b"#!/usr/bin/env sh
290+
sleep 1
291+
";
292+
293+
git2_hooks::create_hook(
294+
&repo,
295+
git2_hooks::HOOK_COMMIT_MSG,
296+
hook,
297+
);
298+
299+
let res = hooks_pre_commit(
300+
&root.to_str().unwrap().into(),
301+
Duration::ZERO,
302+
)
303+
.unwrap();
304+
305+
assert_eq!(res, HookResult::Ok);
306+
}
307+
308+
#[test]
309+
fn test_hooks_timeout_zero() {
310+
let (_td, repo) = repo_init().unwrap();
311+
let root = repo.path().parent().unwrap();
312+
313+
let hook = b"#!/usr/bin/env sh
314+
sleep 1
315+
";
316+
317+
git2_hooks::create_hook(
318+
&repo,
319+
git2_hooks::HOOK_COMMIT_MSG,
320+
hook,
321+
);
322+
323+
let res = hooks_commit_msg(
324+
&root.to_str().unwrap().into(),
325+
&mut String::new(),
326+
Duration::ZERO,
327+
)
328+
.unwrap();
329+
330+
assert_eq!(res, HookResult::Ok);
331+
}
177332
}

‎src/popups/commit.rs‎

Lines changed: 15 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@ use ratatui::{
2727
widgets::Paragraph,
2828
Frame,
2929
};
30+
use std::time::Duration;
3031
use std::{
3132
fs::{read_to_string, File},
3233
io::{Read, Write},
@@ -235,9 +236,10 @@ impl CommitPopup {
235236

236237
if verify {
237238
// run pre commit hook - can reject commit
238-
if let HookResult::NotOk(e) =
239-
sync::hooks_pre_commit(&self.repo.borrow())?
240-
{
239+
if let HookResult::NotOk(e) = sync::hooks_pre_commit(
240+
&self.repo.borrow(),
241+
Duration::ZERO,
242+
)? {
241243
log::error!("pre-commit hook error: {}", e);
242244
self.queue.push(InternalEvent::ShowErrorMsg(
243245
format!("pre-commit hook error:\n{e}"),
@@ -251,9 +253,11 @@ impl CommitPopup {
251253

252254
if verify {
253255
// run commit message check hook - can reject commit
254-
if let HookResult::NotOk(e) =
255-
sync::hooks_commit_msg(&self.repo.borrow(), &mut msg)?
256-
{
256+
if let HookResult::NotOk(e) = sync::hooks_commit_msg(
257+
&self.repo.borrow(),
258+
&mut msg,
259+
Duration::ZERO,
260+
)? {
257261
log::error!("commit-msg hook error: {}", e);
258262
self.queue.push(InternalEvent::ShowErrorMsg(
259263
format!("commit-msg hook error:\n{e}"),
@@ -263,9 +267,10 @@ impl CommitPopup {
263267
}
264268
self.do_commit(&msg)?;
265269

266-
if let HookResult::NotOk(e) =
267-
sync::hooks_post_commit(&self.repo.borrow())?
268-
{
270+
if let HookResult::NotOk(e) = sync::hooks_post_commit(
271+
&self.repo.borrow(),
272+
Duration::ZERO,
273+
)? {
269274
log::error!("post-commit hook error: {}", e);
270275
self.queue.push(InternalEvent::ShowErrorMsg(format!(
271276
"post-commit hook error:\n{e}"
@@ -443,6 +448,7 @@ impl CommitPopup {
443448
&self.repo.borrow(),
444449
msg_source,
445450
&mut msg,
451+
Duration::ZERO,
446452
)? {
447453
log::error!("prepare-commit-msg hook rejection: {e}",);
448454
}

0 commit comments

Comments
 (0)