Add flag and configuration to generate fixup commits for commits written by another author#143
Conversation
blairconrad
left a comment
blairconrad
left a comment
There was a problem hiding this comment.
Thanks for considering the PR. As I mentioned before, this is my first time seeing rust. I've tried to conform to the existing styles and conventions where possible, but may have still done something unrustic or just silly.
I'm happy to make any changes at all, to the code, docs, commit comments, you name it.
| run_with_repo(config, &repo) | ||
| } | ||
|
|
||
| fn run_with_repo(config: &mut Config, repo: &git2::Repository) -> Result<()> { |
There was a problem hiding this comment.
Possibly controversial change?
It was much easier to write tests to assess the effects of the flag and new configuration option after pulling the config-altering code into this method.
There was a problem hiding this comment.
i have a weak dislike for it, but that's not enough to block a functionally complete pr. refactors are a matter of taste after all. if i was going to spend time cleaning it up, i would probably make a function in config.rs that takes a Config and returns a new Config with flags unified and repo defaults applied (then run_with_repo can go back to taking an immutable Config). maybe dedup the config fetching code too, but that might require a macro.
| repo: &'repo git2::Repository, | ||
| user_provided_base: Option<&str>, | ||
| force_author: bool, | ||
| force: bool, |
There was a problem hiding this comment.
I think force looks funny here. I'd had a commit that would rename this to force_branch, but
- wasn't sure if it was overstepping the bounds of this PR, and
- hadn't approval for the name (even though it would only be internal)
Longer term, I'd consider adding a --force-branch (or similar) flag to the CLI (and possibly a configuration option if desired). If this PR is eventually acceptable, and there's interest, I would be open to doing that work as well.
There was a problem hiding this comment.
yes, we should have a separate --force-detach for absorbing on a detached head, then --force would apply both --force-author and --force-detach, and the two flags can be separated internally. feel free to submit that pr if you like
|
... and I see there's a test failure. 🤦🏻 This could be exciting, as the tests pass on my (windows) machine. Starting to investigate, but I'm not set up for *nix development, so it may be slow. |
|
... and now I have cargo set up in ubuntu on WSL and all the tests pass |
The --force flag continues to enable this behavior
… for foreign authors
blairconrad
force-pushed
the
force-author
branch
from
fe28ea9 to
8dc4599
Compare
|
this is good enough for me. there's more stuff that needs to be cleaned up here but it shouldn't block this pr. |
| Ok(nothing) | ||
| } | ||
|
|
||
| fn something_left_in_index(repo: &git2::Repository) -> Result<bool> { |
There was a problem hiding this comment.
if you're in the mood to do more refactoring: i didn't notice we had so many test-only helpers in this module. we should wrap them in a module behind a cfg(test) so the helpers can't accidentally be used in library code
|
Thanks for the review and merge, @tummychow. I am happy to do more work. How about I make issues out of your comments and I (or others!) can see if they can handle them. I wouldn't mind working on any or all, but it'd be greedy to grab them all at once! |
2 participants
Fixes #140.