Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rename Exponential backoff to Quadratic #1815

Merged
merged 3 commits into from
Jan 27, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions gix-lock/src/acquire.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ pub enum Fail {
/// Fail after the first unsuccessful attempt of obtaining a lock.
#[default]
Immediately,
/// Retry after failure with exponentially longer sleep times to block the current thread.
/// Retry after failure with quadratically longer sleep times to block the current thread.
Byron marked this conversation as resolved.
Show resolved Hide resolved
/// Fail once the given duration is exceeded, similar to [Fail::Immediately]
AfterDurationWithBackoff(Duration),
}
Expand Down Expand Up @@ -176,7 +176,7 @@ fn lock_with_mode<T>(
match mode {
Fail::Immediately => try_lock(&lock_path, directory, cleanup),
Fail::AfterDurationWithBackoff(time) => {
for wait in backoff::Exponential::default_with_random().until_no_remaining(time) {
for wait in backoff::Quadratic::default_with_random().until_no_remaining(time) {
attempts += 1;
match try_lock(&lock_path, directory, cleanup.clone()) {
Ok(v) => return Ok((lock_path, v)),
Expand Down
18 changes: 9 additions & 9 deletions gix-utils/src/backoff.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,17 +9,17 @@ fn randomize(backoff_ms: usize) -> usize {
}
}

/// A utility to calculate steps for exponential backoff similar to how it's done in `git`.
pub struct Exponential<Fn> {
/// A utility to calculate steps for quadratic backoff similar to how it's done in `git`.
pub struct Quadratic<Fn> {
multiplier: usize,
max_multiplier: usize,
exponent: usize,
transform: Fn,
}

impl Default for Exponential<fn(usize) -> usize> {
impl Default for Quadratic<fn(usize) -> usize> {
fn default() -> Self {
Exponential {
Quadratic {
multiplier: 1,
max_multiplier: 1000,
exponent: 1,
Expand All @@ -28,10 +28,10 @@ impl Default for Exponential<fn(usize) -> usize> {
}
}

impl Exponential<fn(usize) -> usize> {
/// Create a new exponential backoff iterator that backs off in randomized, ever increasing steps.
impl Quadratic<fn(usize) -> usize> {
/// Create a new quadratic backoff iterator that backs off in randomized, ever increasing steps.
pub fn default_with_random() -> Self {
Exponential {
Quadratic {
multiplier: 1,
max_multiplier: 1000,
exponent: 1,
Expand All @@ -40,7 +40,7 @@ impl Exponential<fn(usize) -> usize> {
}
}

impl<Transform> Exponential<Transform>
impl<Transform> Quadratic<Transform>
where
Transform: Fn(usize) -> usize,
{
Expand All @@ -62,7 +62,7 @@ where
}
}

impl<Transform> Iterator for Exponential<Transform>
impl<Transform> Iterator for Quadratic<Transform>
where
Transform: Fn(usize) -> usize,
{
Expand Down
12 changes: 6 additions & 6 deletions gix-utils/tests/backoff/mod.rs
Original file line number Diff line number Diff line change
@@ -1,16 +1,16 @@
use std::time::Duration;

use gix_utils::backoff::Exponential;
use gix_utils::backoff::Quadratic;

const EXPECTED_TILL_SECOND: &[usize] = &[
1usize, 4, 9, 16, 25, 36, 49, 64, 81, 100, 121, 144, 169, 196, 225, 256, 289, 324, 361, 400, 441, 484, 529, 576,
625, 676, 729, 784, 841, 900, 961, 1000, 1000,
];

#[test]
fn random_exponential_produces_values_in_the_correct_range() {
fn random_quadratic_produces_values_in_the_correct_range() {
let mut num_identities = 0;
for (actual, expected) in Exponential::default_with_random().zip(EXPECTED_TILL_SECOND) {
for (actual, expected) in Quadratic::default_with_random().zip(EXPECTED_TILL_SECOND) {
let actual: usize = actual.as_millis().try_into().unwrap();
if actual == *expected {
num_identities += 1;
Expand All @@ -33,9 +33,9 @@ fn random_exponential_produces_values_in_the_correct_range() {
#[test]
fn how_many_iterations_for_a_second_of_waittime() {
let max = Duration::from_millis(1000);
assert_eq!(Exponential::default().until_no_remaining(max).count(), 14);
assert_eq!(Quadratic::default().until_no_remaining(max).count(), 14);
assert_eq!(
Exponential::default()
Quadratic::default()
.until_no_remaining(max)
.reduce(|acc, n| acc + n)
.unwrap(),
Expand All @@ -47,7 +47,7 @@ fn how_many_iterations_for_a_second_of_waittime() {
#[test]
fn output_with_default_settings() {
assert_eq!(
Exponential::default().take(33).collect::<Vec<_>>(),
Quadratic::default().take(33).collect::<Vec<_>>(),
EXPECTED_TILL_SECOND
.iter()
.map(|n| Duration::from_millis(*n as u64))
Expand Down
3 changes: 2 additions & 1 deletion tests/tools/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -219,6 +219,7 @@ pub fn spawn_git_daemon(working_dir: impl AsRef<Path>) -> std::io::Result<GitDae
.spawn()?;

let server_addr = addr_at(free_port);
// TODO(deps): Upgrading dependencies will require changing `Exponential` to `Quadratic`.
for time in gix_lock::backoff::Exponential::default_with_random() {
std::thread::sleep(time);
if std::net::TcpStream::connect(server_addr).is_ok() {
Expand Down Expand Up @@ -652,8 +653,8 @@ fn configure_command<'a, I: IntoIterator<Item = S>, S: AsRef<OsStr>>(
}

fn bash_program() -> &'static Path {
// TODO: use `gix_path::env::login_shell()` when available.
if cfg!(windows) {
// TODO(deps): Once `gix_path::env::shell()` is available, maybe do `shell().parent()?.join("bash.exe")`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am looking forward to that!

Copy link
Member Author

@EliahKagan EliahKagan Jan 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I'm looking forward to it, actually. I'm not sure if shell() currently works properly in all the cases listed in #1761 where it reasonably should. But one case listed there that I expect it can and should be made to work--if it doesn't work already--is BusyBox-based MinGit. For that, there would not be a bash.exe in the same directory as the sh implementation shell() should find.

I look forward more to the possibility that gix-path--or some gix-* crate, since maybe this is actually in the ambit of gix-command--could report the locations of more executables.

sh should be preferred over bash for running shell scripts even when they are identical executables or one is a symlink to the other--because bash, and most other Bourne-style shells, behaves more POSIX-compatibly when invoked by the name sh. Yet finding and running bash is something that is often reasonable to do for specific reasons. It is a better choice in the gitoxide test suite, because the fixture scripts use bash-specific language features and they are not currently run in a way where their #! lines are automatically respected. There may even be some applications that need to use bash to run scripts that ideally should run in sh, just because they have been doing that for a long time already and users rely on it.

There are also various other executables that are often but not always available, and are relevant to Git, that are not found in the same directories as the Git for Windows sh.exe and bash.exe. An important case is perl. (MinGit does not have perl, but full Git for Windows installations do ship it, and I think it could be useful to find the one that comes with Git for Windows.)

More broadly, this could be part of a way of figuring out how to run interpreters specified in #! lines. Perhaps we should also have a function in gix-path, or somewhere. that takes a command name, to which "bash" could be passed.

Edit: Or maybe this is actually an alternative search behavior that should be achieved by opting into it through gix_command::Prepare.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, thanks for sharing! Finding binaries isn't straightforward and having read this, I'd be glad if providing such functionality in gix-* crates would be minimised, strictly limited to strong use-cases or demands.

More broadly, this could be part of a way of figuring out how to run interpreters specified in #! lines. Perhaps we should also have a function in gix-path, or somewhere. that takes a command name, to which "bash" could be passed.

If I remember correctly, on Windows this is already done via gix-command. But I don't recall if anything special is done to launch the command specified in the shebang. but think the implementation closely follows the one in Git.

Copy link
Member Author

@EliahKagan EliahKagan Jan 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

gix-command attempts to interpret shebangs, but I don't think it does so using the environment modifications Git for Windows puts in place. Traditionally Git for Windows did this in the git.exe shim but not the real git.exe executable that the shim points to. But since git-for-windows/git#2506 the real git.exe will detect when it looks like the customizations were not made and do them itself.

For PATH customizations especially, I think it might not be good to perform them or act by default as though they have been done. The kind of thing it does now, searching the preexisting PATH, seems better. Applications that use gix-* crates may run on Windows systems where Git for Windows is absent. Even when it is present, it is not clear that interpreters for hooks should in general be searched for first in Git for Windows related directories. There is also the subtlety that some executables found in directories Git for Windows adds to the PATH might malfunction in some uses if run without that environment set.

So while I think this may be useful functionality, it should probably continue not to be the default, and be some combination of opt-in and/or enabled automatically only for interpreters that are very likely to be wanted from Git for Windows. This might just be sh, but bash (which is alongside sh) and perl (which is not) are plausible others.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for sharing, I wasn't aware that git.exe auto-modifies its PATH, something which indeed isn't done in gix-command. I also agree that it shouldn't be done by default, even though it might be useful to have at some point for opting in.

For now I assume that sh.exe used by Git will automatically allow all sibling commands to be called as well, which should make shell scripts on Windows more usable overall.

static GIT_BASH: Lazy<Option<PathBuf>> = Lazy::new(|| {
GIT_CORE_DIR
.parent()?
Expand Down
Loading