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.

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

Refactor config-merging #147

Merged
merged 3 commits into from
Feb 7, 2025
Merged

Refactor config-merging #147

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
36a2bb4
Fix formatting
blairconrad Feb 5, 2025
c1a6efa
Unify configuration settings with config::unify
blairconrad Feb 6, 2025
fc20309
Deduplify boolean git config value fetching
blairconrad Feb 6, 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
73 changes: 43 additions & 30 deletions src/config.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,6 @@
use crate::Config;
use git2::Repository;

pub const MAX_STACK_CONFIG_NAME: &str = "absorb.maxStack";
pub const MAX_STACK: usize = 10;

Expand All @@ -13,6 +16,30 @@ pub const AUTO_STAGE_IF_NOTHING_STAGED_DEFAULT: bool = false;
pub const FIXUP_TARGET_ALWAYS_SHA_CONFIG_NAME: &str = "absorb.fixupTargetAlwaysSHA";
pub const FIXUP_TARGET_ALWAYS_SHA_DEFAULT: bool = false;

pub fn unify<'config>(config: &'config Config, repo: &Repository) -> Config<'config> {
Config {
// here, we default to the git config value,
// if the flag was not provided in the CLI.
//
// in the future, we'd likely want to differentiate between
// a "non-provided" option, vs an explicit --no-<option>
// that disables a behavior, much like git does.
// e.g. user may want to overwrite a config value with
// --no-one-fixup-per-commit -- then, defaulting to the config value
// like we do here is no longer sufficient. but until then, this is fine.
one_fixup_per_commit: config.one_fixup_per_commit
|| bool_value(
&repo,
ONE_FIXUP_PER_COMMIT_CONFIG_NAME,
ONE_FIXUP_PER_COMMIT_DEFAULT,
),
force_author: config.force_author
|| config.force
|| bool_value(&repo, FORCE_AUTHOR_CONFIG_NAME, FORCE_AUTHOR_DEFAULT),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I took the liberty of inlining the above two calls to get the configuration data from git. The long constant names introduce wrapping, which maybe spoils the effect. If you prefer the methods back, happy to re-add.

..*config
}
}

pub fn max_stack(repo: &git2::Repository) -> usize {
match repo
.config()
Expand All @@ -23,42 +50,28 @@ pub fn max_stack(repo: &git2::Repository) -> usize {
}
}

pub fn force_author(repo: &git2::Repository) -> bool {
match repo
.config()
.and_then(|config| config.get_bool(FORCE_AUTHOR_CONFIG_NAME))
{
Ok(force_author) => force_author,
_ => FORCE_AUTHOR_DEFAULT,
}
}

pub fn one_fixup_per_commit(repo: &git2::Repository) -> bool {
match repo
.config()
.and_then(|config| config.get_bool(ONE_FIXUP_PER_COMMIT_CONFIG_NAME))
{
Ok(one_commit_per_fixup) => one_commit_per_fixup,
_ => ONE_FIXUP_PER_COMMIT_DEFAULT,
}
}

pub fn auto_stage_if_nothing_staged(repo: &git2::Repository) -> bool {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wasn't sure if you'd want the values from these methods added to the Config object so we don't have to call them from throughout the code. Easy enough to do, if you like it, but introduces questions:

  1. do we want to expand Config with the extra attributes?
  2. if not, how do you feel about a new struct that holds the unified settings? It wouldn't bloat Config, and will eventually allow dropping of the force attribute when it's only used to enable the other force_ attributes
  3. if you did like the idea of a second struct, naming becomes trickier; me, I'd avoid Config for either the new or existing struct (already taken by git's config) and probably look for name that indicate how the one struct comes from command-line arguments and the new one represents the unified settings. I'm just thinking out loud here.

Copy link
Owner

Choose a reason for hiding this comment

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

let's leave em as standalone functions for now. Config was originally intended to carry the cli arguments. i'm not tied to that interpretation of it, but no need to move everything in there atm

match repo
.config()
.and_then(|config| config.get_bool(AUTO_STAGE_IF_NOTHING_STAGED_CONFIG_NAME))
{
Ok(val) => val,
_ => AUTO_STAGE_IF_NOTHING_STAGED_DEFAULT,
}
bool_value(
&repo,
AUTO_STAGE_IF_NOTHING_STAGED_CONFIG_NAME,
AUTO_STAGE_IF_NOTHING_STAGED_DEFAULT,
)
}

pub fn fixup_target_always_sha(repo: &git2::Repository) -> bool {
bool_value(
&repo,
FIXUP_TARGET_ALWAYS_SHA_CONFIG_NAME,
FIXUP_TARGET_ALWAYS_SHA_DEFAULT,
)
}

fn bool_value(repo: &Repository, setting_name: &str, default_value: bool) -> bool {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This seemed to me to be a reasonable start at deduping. Requires a different function per type of config data (and there's only one usize, so I didn't bother extracting). Happy to hear what you think could be improved.

Copy link
Owner

Choose a reason for hiding this comment

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

quite reasonable

match repo
.config()
.and_then(|config| config.get_bool(FIXUP_TARGET_ALWAYS_SHA_CONFIG_NAME))
.and_then(|config| config.get_bool(setting_name))
{
Ok(val) => val,
_ => FIXUP_TARGET_ALWAYS_SHA_DEFAULT,
Ok(value) => value,
_ => default_value,
}
}
64 changes: 26 additions & 38 deletions src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,29 +20,16 @@ pub struct Config<'a> {
pub logger: &'a slog::Logger,
}

pub fn run(config: &mut Config) -> Result<()> {
pub fn run(config: &Config) -> Result<()> {
let repo = git2::Repository::open_from_env()?;
debug!(config.logger, "repository found"; "path" => repo.path().to_str());

run_with_repo(config, &repo)
run_with_repo(&config, &repo)
}

fn run_with_repo(config: &mut Config, repo: &git2::Repository) -> Result<()> {
fn run_with_repo(config: &Config, repo: &git2::Repository) -> Result<()> {
let config = config::unify(&config, repo);
// have force flag enable all force* flags
config.force_author |= config.force;

// here, we default to the git config value,
// if the flag was not provided in the CLI.
//
// in the future, we'd likely want to differentiate between
// a "non-provided" option, vs an explicit --no-<option>
// that disables a behavior, much like git does.
// e.g. user may want to overwrite a config value with
// --no-one-fixup-per-commit -- then, defaulting to the config value
// like we do here is no longer sufficient. but until then, this is fine.
//
config.one_fixup_per_commit |= config::one_fixup_per_commit(&repo);
config.force_author |= config::force_author(&repo);

let stack = stack::working_stack(
repo,
Expand Down Expand Up @@ -595,7 +582,7 @@ lines
// run 'git-absorb'
let drain = slog::Discard;
let logger = slog::Logger::root(drain, o!());
let mut config = Config {
let config = Config {
dry_run: false,
force_author: false,
force: false,
Expand All @@ -605,7 +592,7 @@ lines
one_fixup_per_commit: false,
logger: &logger,
};
run_with_repo(&mut config, &ctx.repo).unwrap();
run_with_repo(&config, &ctx.repo).unwrap();

let mut revwalk = ctx.repo.revwalk().unwrap();
revwalk.push_head().unwrap();
Expand All @@ -621,7 +608,7 @@ lines
// run 'git-absorb'
let drain = slog::Discard;
let logger = slog::Logger::root(drain, o!());
let mut config = Config {
let config = Config {
dry_run: false,
force_author: false,
force: false,
Expand All @@ -631,7 +618,7 @@ lines
one_fixup_per_commit: true,
logger: &logger,
};
run_with_repo(&mut config, &ctx.repo).unwrap();
run_with_repo(&config, &ctx.repo).unwrap();

let mut revwalk = ctx.repo.revwalk().unwrap();
revwalk.push_head().unwrap();
Expand All @@ -649,7 +636,7 @@ lines
// run 'git-absorb'
let drain = slog::Discard;
let logger = slog::Logger::root(drain, o!());
let mut config = Config {
let config = Config {
dry_run: false,
force_author: false,
force: false,
Expand All @@ -659,7 +646,7 @@ lines
one_fixup_per_commit: true,
logger: &logger,
};
run_with_repo(&mut config, &ctx.repo).unwrap();
run_with_repo(&config, &ctx.repo).unwrap();

let mut revwalk = ctx.repo.revwalk().unwrap();
revwalk.push_head().unwrap();
Expand All @@ -677,7 +664,7 @@ lines
// run 'git-absorb'
let drain = slog::Discard;
let logger = slog::Logger::root(drain, o!());
let mut config = Config {
let config = Config {
dry_run: false,
force_author: true,
force: false,
Expand All @@ -687,7 +674,7 @@ lines
one_fixup_per_commit: true,
logger: &logger,
};
run_with_repo(&mut config, &ctx.repo).unwrap();
run_with_repo(&config, &ctx.repo).unwrap();

let mut revwalk = ctx.repo.revwalk().unwrap();
revwalk.push_head().unwrap();
Expand All @@ -705,7 +692,7 @@ lines
// run 'git-absorb'
let drain = slog::Discard;
let logger = slog::Logger::root(drain, o!());
let mut config = Config {
let config = Config {
dry_run: false,
force_author: false,
force: true,
Expand All @@ -715,7 +702,7 @@ lines
one_fixup_per_commit: true,
logger: &logger,
};
run_with_repo(&mut config, &ctx.repo).unwrap();
run_with_repo(&config, &ctx.repo).unwrap();

let mut revwalk = ctx.repo.revwalk().unwrap();
revwalk.push_head().unwrap();
Expand All @@ -730,15 +717,16 @@ lines

become_new_author(&ctx);

ctx.repo.config()
ctx.repo
.config()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

As you can see, from a bonus commit that fixed bad formatting I previously introduced. You seem pragmatic, but I'm not sure how you feel about introducing such a bonus commit into this PR.

(As an aside, if keeping a good format is important to you (and I suspect it is, or the whole repo wouldn't've been formatted), is there interest in having a job that would check formatting when PRs come in? It could either prevent the merge or serve as a warning.)

Copy link
Owner

Choose a reason for hiding this comment

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

yeah, formatting in ci sgtm, and having this commit here is also fine with me

.unwrap()
.set_str("absorb.forceAuthor", "true")
.unwrap();

// run 'git-absorb'
let drain = slog::Discard;
let logger = slog::Logger::root(drain, o!());
let mut config = Config {
let config = Config {
dry_run: false,
force_author: false,
force: false,
Expand All @@ -748,7 +736,7 @@ lines
one_fixup_per_commit: true,
logger: &logger,
};
run_with_repo(&mut config, &ctx.repo).unwrap();
run_with_repo(&config, &ctx.repo).unwrap();

let mut revwalk = ctx.repo.revwalk().unwrap();
revwalk.push_head().unwrap();
Expand Down Expand Up @@ -787,7 +775,7 @@ lines
// run 'git-absorb'
let drain = slog::Discard;
let logger = slog::Logger::root(drain, o!());
let mut config = Config {
let config = Config {
dry_run: false,
force_author: false,
force: false,
Expand All @@ -797,7 +785,7 @@ lines
one_fixup_per_commit: false,
logger: &logger,
};
run_with_repo(&mut config, &ctx.repo).unwrap();
run_with_repo(&config, &ctx.repo).unwrap();

let mut revwalk = ctx.repo.revwalk().unwrap();
revwalk.push_head().unwrap();
Expand All @@ -824,7 +812,7 @@ lines
// run 'git-absorb'
let drain = slog::Discard;
let logger = slog::Logger::root(drain, o!());
let mut config = Config {
let config = Config {
dry_run: false,
force_author: false,
force: false,
Expand All @@ -834,7 +822,7 @@ lines
one_fixup_per_commit: false,
logger: &logger,
};
run_with_repo(&mut config, &ctx.repo).unwrap();
run_with_repo(&config, &ctx.repo).unwrap();

let mut revwalk = ctx.repo.revwalk().unwrap();
revwalk.push_head().unwrap();
Expand All @@ -859,7 +847,7 @@ lines
// run 'git-absorb'
let drain = slog::Discard;
let logger = slog::Logger::root(drain, o!());
let mut config = Config {
let config = Config {
dry_run: false,
force_author: false,
force: false,
Expand All @@ -869,7 +857,7 @@ lines
one_fixup_per_commit: false,
logger: &logger,
};
run_with_repo(&mut config, &ctx.repo).unwrap();
run_with_repo(&config, &ctx.repo).unwrap();

let mut revwalk = ctx.repo.revwalk().unwrap();
revwalk.push_head().unwrap();
Expand All @@ -891,7 +879,7 @@ lines
// run 'git-absorb'
let drain = slog::Discard;
let logger = slog::Logger::root(drain, o!());
let mut config = Config {
let config = Config {
dry_run: false,
force_author: false,
force: false,
Expand All @@ -901,7 +889,7 @@ lines
one_fixup_per_commit: true,
logger: &logger,
};
run_with_repo(&mut config, &ctx.repo).unwrap();
run_with_repo(&config, &ctx.repo).unwrap();
assert!(nothing_left_in_index(&ctx.repo).unwrap());

let mut revwalk = ctx.repo.revwalk().unwrap();
Expand Down
2 changes: 1 addition & 1 deletion src/main.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,7 @@ fn main() {
));
}

if let Err(e) = git_absorb::run(&mut git_absorb::Config {
if let Err(e) = git_absorb::run(&git_absorb::Config {
dry_run,
force_author,
force,
Expand Down