Skip to content

Commit 08eaefe

Browse files
DaRacciDaRacci
committed
feat(asyncgit): add timeouts for hooks
1 parent 44d4a8d commit 08eaefe

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+
)?;
90+
91+
if let Some(updated_msg) = msg_opt {
92+
msg.clear();
93+
msg.push_str(&updated_msg);
94+
}
3795

38-
Ok(git2_hooks::hooks_commit_msg(&repo, None, msg)?.into())
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)]
@@ -99,7 +201,7 @@ mod tests {
99201
let (_td, repo) = repo_init().unwrap();
100202
let root = repo.workdir().unwrap();
101203

102-
let hook = b"#!/bin/sh
204+
let hook = b"#!/usr/bin/env sh
103205
echo 'rejected'
104206
exit 1
105207
";
@@ -113,9 +215,11 @@ mod tests {
113215
let subfolder = root.join("foo/");
114216
std::fs::create_dir_all(&subfolder).unwrap();
115217

116-
let res =
117-
hooks_post_commit(&subfolder.to_str().unwrap().into())
118-
.unwrap();
218+
let res = hooks_post_commit(
219+
&subfolder.to_str().unwrap().into(),
220+
Duration::ZERO,
221+
)
222+
.unwrap();
119223

120224
assert_eq!(
121225
res,
@@ -136,7 +240,7 @@ mod tests {
136240
let workdir =
137241
crate::sync::utils::repo_work_dir(repo_path).unwrap();
138242

139-
let hook = b"#!/bin/sh
243+
let hook = b"#!/usr/bin/env sh
140244
echo $(pwd)
141245
exit 1
142246
";
@@ -146,7 +250,8 @@ mod tests {
146250
git2_hooks::HOOK_PRE_COMMIT,
147251
hook,
148252
);
149-
let res = hooks_pre_commit(repo_path).unwrap();
253+
let res =
254+
hooks_pre_commit(repo_path, Duration::ZERO).unwrap();
150255
if let HookResult::NotOk(res) = res {
151256
assert_eq!(
152257
std::path::Path::new(res.trim_end()),
@@ -162,7 +267,7 @@ mod tests {
162267
let (_td, repo) = repo_init().unwrap();
163268
let root = repo.workdir().unwrap();
164269

165-
let hook = b"#!/bin/sh
270+
let hook = b"#!/usr/bin/env sh
166271
echo 'msg' > $1
167272
echo 'rejected'
168273
exit 1
@@ -181,6 +286,7 @@ mod tests {
181286
let res = hooks_commit_msg(
182287
&subfolder.to_str().unwrap().into(),
183288
&mut msg,
289+
Duration::ZERO,
184290
)
185291
.unwrap();
186292

@@ -224,4 +330,53 @@ mod tests {
224330

225331
assert_eq!(msg, String::from("msg\n"));
226332
}
333+
334+
#[test]
335+
fn test_hooks_respect_timeout() {
336+
let (_td, repo) = repo_init().unwrap();
337+
let root = repo.path().parent().unwrap();
338+
339+
let hook = b"#!/usr/bin/env sh
340+
sleep 1
341+
";
342+
343+
git2_hooks::create_hook(
344+
&repo,
345+
git2_hooks::HOOK_COMMIT_MSG,
346+
hook,
347+
);
348+
349+
let res = hooks_pre_commit(
350+
&root.to_str().unwrap().into(),
351+
Duration::ZERO,
352+
)
353+
.unwrap();
354+
355+
assert_eq!(res, HookResult::Ok);
356+
}
357+
358+
#[test]
359+
fn test_hooks_timeout_zero() {
360+
let (_td, repo) = repo_init().unwrap();
361+
let root = repo.path().parent().unwrap();
362+
363+
let hook = b"#!/usr/bin/env sh
364+
sleep 1
365+
";
366+
367+
git2_hooks::create_hook(
368+
&repo,
369+
git2_hooks::HOOK_COMMIT_MSG,
370+
hook,
371+
);
372+
373+
let res = hooks_commit_msg(
374+
&root.to_str().unwrap().into(),
375+
&mut String::new(),
376+
Duration::ZERO,
377+
)
378+
.unwrap();
379+
380+
assert_eq!(res, HookResult::Ok);
381+
}
227382
}

‎src/popups/commit.rs

Lines changed: 15 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,7 @@ use ratatui::{
2828
Frame,
2929
};
3030

31+
use std::time::Duration;
3132
use std::{
3233
fmt::Write as _,
3334
fs::{read_to_string, File},
@@ -237,9 +238,10 @@ impl CommitPopup {
237238

238239
if verify {
239240
// run pre commit hook - can reject commit
240-
if let HookResult::NotOk(e) =
241-
sync::hooks_pre_commit(&self.repo.borrow())?
242-
{
241+
if let HookResult::NotOk(e) = sync::hooks_pre_commit(
242+
&self.repo.borrow(),
243+
Duration::ZERO,
244+
)? {
243245
log::error!("pre-commit hook error: {}", e);
244246
self.queue.push(InternalEvent::ShowErrorMsg(
245247
format!("pre-commit hook error:\n{e}"),
@@ -253,9 +255,11 @@ impl CommitPopup {
253255

254256
if verify {
255257
// run commit message check hook - can reject commit
256-
if let HookResult::NotOk(e) =
257-
sync::hooks_commit_msg(&self.repo.borrow(), &mut msg)?
258-
{
258+
if let HookResult::NotOk(e) = sync::hooks_commit_msg(
259+
&self.repo.borrow(),
260+
&mut msg,
261+
Duration::ZERO,
262+
)? {
259263
log::error!("commit-msg hook error: {}", e);
260264
self.queue.push(InternalEvent::ShowErrorMsg(
261265
format!("commit-msg hook error:\n{e}"),
@@ -265,9 +269,10 @@ impl CommitPopup {
265269
}
266270
self.do_commit(&msg)?;
267271

268-
if let HookResult::NotOk(e) =
269-
sync::hooks_post_commit(&self.repo.borrow())?
270-
{
272+
if let HookResult::NotOk(e) = sync::hooks_post_commit(
273+
&self.repo.borrow(),
274+
Duration::ZERO,
275+
)? {
271276
log::error!("post-commit hook error: {}", e);
272277
self.queue.push(InternalEvent::ShowErrorMsg(format!(
273278
"post-commit hook error:\n{e}"
@@ -445,6 +450,7 @@ impl CommitPopup {
445450
&self.repo.borrow(),
446451
msg_source,
447452
&mut msg,
453+
Duration::ZERO,
448454
)? {
449455
log::error!("prepare-commit-msg hook rejection: {e}",);
450456
}

0 commit comments

Comments
 (0)