Skip to content

Rename Exponential backoff to Quadratic #1815

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Jan 27, 2025
Merged

Rename Exponential backoff to Quadratic #1815

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d737c4a
fix!: Rename `Exponential` backoff to `Quadratic`
EliahKagan Jan 26, 2025
bf4fa1b
Adapt to changes in `gix-utils`
EliahKagan Jan 27, 2025
6990f76
Comment forthcoming dependency changes for gix-testtools
EliahKagan Jan 26, 2025
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
View file
Open in desktop
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.
/// 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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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 •
edited
Loading

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 •
edited
Loading

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.

Copy link
Member Author

@EliahKagan EliahKagan Feb 15, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

As described in the "Note" in #1840 (reply in thread), there's a separate issue that affects shebangs like #!/bin/sh as gix-command handles them on Windows: paths that start with / in a shebang line, at least if they start with only one /, are very unlikely to be meant in the way Windows interprets them--as being relative to the current drive--but gix-command uses them with that interpretation. (Git does not do this. For example, a hook or custom git-* command can have a #!/bin/sh shebang and git runs it with its sh.) Is there an existing issue for that?

Copy link
Member

Choose a reason for hiding this comment

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

I don't think so, and would definitely appreciate an issue or even a quick PR with a fix. Running absolute paths accidentally in such a way that it will pick up files relative to some directory sounds like a way to execute adversary-controlled binaries.

Copy link
Member Author

@EliahKagan EliahKagan Feb 15, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, that's true. I'll try to do something for this sometime soon. Since some of the regression tests that I wrote for #1840 and expected already to pass failed due to this problem, they might be good tests for a fix, or might be possible to adapt for that purpose. So I'll look into doing at least a partial fix for this based on that, either before a fix for #1840 or at the same time. Then I can circle back later to polish things further, as well as to examine this and related shebang handling behavior in greater depth.

Copy link
Member Author

@EliahKagan EliahKagan Feb 24, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Just as a quick and incomplete update: I have not forgotten about this, but it looks like fixing this is not a quick PR, unless the fix is to disable gix-command shebang processing completely on Windows. A fix for this that one can be reasonably confident does not make things worse overall is one of the goals alluded to in #1862 (comment) and that I hope that #1862 may help with, but which I can possibly manage to do even if that PR does not work out.

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