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

feat(reword): Add --fixup flag to git branchless reword #538

Merged
merged 2 commits into from
Sep 13, 2022

Conversation

claytonrcarter
Copy link
Collaborator

This adds a --fixup option to git reword, providing a convenient way to turn existing commits into fixup! commit. With this, instead of doing something like git reword <commits to reword> -m 'fixup! <paste title of commit to fixup>', you can now do git reword <commits to reword> --fixup <commit to fixup> and reword will handle the rest, including some error checking.

I've been using this for a few weeks and it's been working well. My only remaining concern about this is that the CLI option docs and error messages feel a little clunky because it's hard to talk to about "the commit being fixed up" vs "the commits that will become fixup! commits". As usual, suggestions are welcome!

Comment on lines 104 to 108
if commits.iter().any(|commit| {
!dag.query()
.is_ancestor(commit_to_fixup.get_oid().into(), commit.get_oid().into())
.expect("Failed checking ancestry of commit.")
}) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I suppose that instead of iter().any(...) I could instead just use a single dag query; perhaps like

commits.intersection(
    dag.query.descendents(
        commit_to_fixup.get_oid().into()
    )
) == commits

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Nevermind this; I thought that NameSet allowed for ==/!= but it doesn't. I'll leave it as is unless you have a suggestion for a more intuitive way to check this.

Copy link
Owner

Choose a reason for hiding this comment

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

I think you could write this as dag.query().common_ancestors(CommitSet::from(commits))?.contains(commit_to_fixup)? or something like that. I don't know why there isn't a .equals() method or similar.

git-branchless/src/opts.rs Show resolved Hide resolved
Copy link
Owner

@arxanas arxanas left a comment

Choose a reason for hiding this comment

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

I'm okay with merging this as-is for now, but maybe we should invent a more straightforward method to fixup commits? For example, it would be more direct if you could write git amend <target> --from <revs> or something like that. Now that we can move exact commits, it should be possible to move all of the <revs> right on top of <target>, and then create a RebaseCommand::Fixup to handle the fixing up. The implementation for in-memory rebases doesn't seem too bad: you would basically do a pick but call amend_commit instead of cherry_pick_fast.

Comment on lines 104 to 108
if commits.iter().any(|commit| {
!dag.query()
.is_ancestor(commit_to_fixup.get_oid().into(), commit.get_oid().into())
.expect("Failed checking ancestry of commit.")
}) {
Copy link
Owner

Choose a reason for hiding this comment

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

I think you could write this as dag.query().common_ancestors(CommitSet::from(commits))?.contains(commit_to_fixup)? or something like that. I don't know why there isn't a .equals() method or similar.

git-branchless/src/commands/reword.rs Outdated Show resolved Hide resolved
git-branchless/src/opts.rs Outdated Show resolved Hide resolved
@arxanas
Copy link
Owner

arxanas commented Sep 13, 2022

I guess a theoretical fixup command should be part of git move; git amend should probably be for working copy changes only.

@claytonrcarter claytonrcarter enabled auto-merge (rebase) September 13, 2022 15:32
@claytonrcarter claytonrcarter merged commit c86771a into arxanas:master Sep 13, 2022
@claytonrcarter claytonrcarter deleted the reword-fixup branch September 13, 2022 16:01
@claytonrcarter
Copy link
Collaborator Author

Thanks for the review, @arxanas!

maybe we should invent a more straightforward method to fixup commits?

Oh, I'm all over that idea. This just seemed like an easier place to start. 😄 I had been thinking about a separate git combine command, but seeing you mention in it the context of git move makes a lot of sense as all of the "move exact or subtree" plumbing is already in place there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants