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

Jump to bottom

Add feature to generate one fixup per commit. #91

Merged
merged 3 commits into from
Jul 25, 2023
Merged

Add feature to generate one fixup per commit. #91

merged 3 commits into from
Jul 25, 2023

Conversation

rbartlensky
Copy link
Contributor

This is my attempt at implementing the feature request from #19.

I would suggest reviewing the changeset commit by commit, since I made sure to split the work into logical chunks: refactor, add feature, tests, and so on.

Closes issue #19.

tummychow
tummychow requested changes Jul 21, 2023
View reviewed changes
Copy link
Owner

@tummychow tummychow left a comment

Choose a reason for hiding this comment

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

sorry that my primary feedback is "i disagree with the structure of the refactor", i know that's the most annoying sort of feedback i could have given.

src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Show resolved Hide resolved
tummychow
tummychow requested changes Jul 23, 2023
View reviewed changes
Copy link
Owner

@tummychow tummychow left a comment

Choose a reason for hiding this comment

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

thanks, that's the arrangement i had in mind

src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
tummychow
tummychow requested changes Jul 25, 2023
View reviewed changes
Copy link
Owner

@tummychow tummychow left a comment

Choose a reason for hiding this comment

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

lgtm other than this last comment. let me know if you still want to try mempack on this pr, otherwise i'll merge after this one is addressed

src/lib.rs Outdated Show resolved Hide resolved
@tummychow
Copy link
Owner

oh and also, the new test is failing so you'll have to figure out what's going on there. looks like something trivial related to the new repo having no configured user

@rbartlensky
Add option to create one fixup per commit.
019cd8a 
This means that all hunks meant to be fixed up into the same commit
will be grouped into a single commit, rather than multiple ones.

In order to not break the workflow of others, we choose to make this
an option rather than the default. Moreover, folks can `alias
git-absorb="git-absorb --one-fixup-per-commit"` if they wish to always
use this option, and not have to type it out.
@rbartlensky
Add basic tests for one_fixup_per_commit feature.
88928cd
@rbartlensky
Add some cargo-fmt changes.
e16fcff
@rbartlensky
Copy link
Contributor Author

@tummychow I squashed away the commits to clean the history up a bit. I think I will follow-up with improving the tests since it's taking longer than I expected

@rbartlensky
Copy link
Contributor Author

Also thank you for the quick turnaround!

@tummychow
Copy link
Owner

tummychow commented Jul 25, 2023 •
edited
Loading

no problem, thanks for implementing all the requested changes. will probably cut a release for this later in the week

@tummychow tummychow merged commit bca5f29 into tummychow:master Jul 25, 2023
@rbartlensky rbartlensky deleted the one-fixup-per-commit branch July 25, 2023 21:58
@rbartlensky rbartlensky mentioned this pull request Jul 26, 2023
Merged
@nickolay
Copy link
Contributor

Thanks for implementing this feature! Any reason the new -F mode is not the default?

@rbartlensky
Copy link
Contributor Author

Any reason the new -F mode is not the default?

There's an explanation in the first commit message:

In order to not break the workflow of others, we choose to make this
an option rather than the default. Moreover, folks can alias git-absorb="git-absorb --one-fixup-per-commit" if they wish to always
use this option, and not have to type it out.

I will let @tummychow decide whether it should become the default in a later release or not.

@tummychow
Copy link
Owner

yeah i want to let it bake for a while, given the amount of code getting moved around. i would accept a pr to add a git config option for it though

@nickolay nickolay mentioned this pull request Dec 31, 2023
Open
@kiprasmel kiprasmel mentioned this pull request Feb 23, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tummychow tummychow tummychow requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@rbartlensky @tummychow @nickolay