feat: implement git reword command (#179)

[claytonrcarter]

Updated PR description:

This supports all of the use-cases that I mentioned on #179, except for the fancy bulk editing case. That is to say:

• Reword single or multiple commits within or across subtrees.
• Currently, the same message is applied to all commits.
• If multiple message flags are given on the CLI, they are combined into separate paragraphs.
• If no message flags are passed on the CLI, $EDITOR is opened w/ either the message of the commit being reworded (if just rewording a single commit) or a stub message when rewording multiple commits.
• Bails out if the edited message is identical to the input message, or if it's blank.

A few things to make special note of:

• I'm still very much learning my way through Rust, so please let me know where I can improve my usage of the language or be more in line w/ the community or project.
• I think I've taken a pretty good stab at error handling, but please keep a special eye out for possible places that I missed something or was over eager w/ my usage of ? or .unwrap(), etc

Finally, there a few things left to do:

• I haven't done anything to look at @epage request that it operate independent of branchless init. At this time, I don't even know where to start w/ that, so any pointers would be most welcome.
• The bulk editing feature is not implemented; I am voting ✋ for us to review this as is and add that later, though I'm also fine to push on to work on bulk editing now if you'd prefer.
• During my latest read through, I decided that we should probably a "footer" message to the $EDITOR akin to the default commit message from git. I'll work on this soon.
• I'd like to take another look through my error handling and borrow/clone/dereferencing/option/basically/everything usage again.

I think that covers most of it, but I'm sure I'm missing something! 😄

Original PR description below:

Still very draft, but it's working for all of the use-cases that I mentioned on #179, except for the fancy bulk editing case. That is to say:

• Reword single or multiple commits within or across subtrees.
• Currently, the same message is applied to all commits.
• If multiple message flags are given on the CLI, they are combined into separate paragraphs.
• If no message flags are passed on the CLI, $EDITOR is opened w/ either the message of the commit being reworded (if just rewording a single commit) or a stub message when rewording multiple commits.
• Prompt for confirmation when appling the same message to multiple commits. (--force flag skips confirmation.)
• Bails out if the edited message is identical to the input message, or if it's blank.

So, lots to love there already, but still lots to do:

• I'm still very much learning my way through Rust, so expect lots of 🤦‍♂️ moments if you're brave enough to read through this, especially when it comes to borrowing, scope and such.
• This is very "happy pathy" right now. I need to go back and flesh out the error handling.
• For expediency, I implemented this by altering r#move::r#move (rewording is kind of just a special case of moving, at least, it seems so to me) but I just did that so that I could focus on the fun parts and not get bogged down trying to figure out the plumbing. (It's just a 4 line change in r#move.) I intend to break this entanglement and move the actual rebase planning into reword, but it's still in place at this time. Because of this, though, it's doing a restack/smartlog for every subtree that's being reworded, instead of just doing a single restack and smartlog at the end. Edit: I've removed the coupling w/ r#move and rewritten the rebase planning and execution based on sync.
• Also note that I did not implement this via a new RebaseCommand,. eg RebaseCommand::Reword. Instead I just updatedRebaseCommand::Pick to support an optional message since a reword is just a pick but with a different message and the change was trivial. I'm inclined to leave this in place unless you feel otherwise.
• B/c reword is treated as just a special case of move, I wanted to reuse all of the normal MoveOptions, but I wanted to reserve the shorthand -m flag for --messages, not for merge as in MoveOptions ... so I just duplicated MoveOptions into RebaseOption except w/o the -m flag. This is still TBD, but I'd like a way to just reuse MoveOptions while overriding the binding of -m, but I haven't delved into how/if this can be done in clap.
• I haven't done anything to look at @epage request that it operate independent of branchless init. At this time, I don't even know where to start w/ that, so any pointers would be most welcome.
• Bulk editting? I wonder if bulk editing might be best considered a feature unto itself. If so, this existing command could be finished off and proper bulk editing support (ie different messages for different commits) could be handled via a separate PR.

I'm going to park this here for a bit while I go away and read more about borrowing, Boxes, memory mgmt, error handling etc in Rust. 😄 I hesitate to even ask for input yet, but I think it would be good to know if I'm heading in the right direction or not. (And of course even if I'm way off base and this is just totally not viable, it was still a fun learning exercise!)

Oh, and I totally used this to squash all of my ugly working commits. This is a slightly edited version:

❯ ./target/debug/git-branchless reword --message 'hacking on `reword`' <base commit on branch>
❯ ./target/debug/git-branchless reword --message 'fixup! hacking on `reword`' <lots of descendent commits>
❯ git rebase --autosquash -i master 
❯ ./target/debug/git-branchless reword --message 'feat: implement `reword` command'

Thanks for your consideration!

[claytonrcarter]

Sorry for the churn. I think I've fixed all of the formatting and clippy issues.

[arxanas]

This is really exciting!

One note: before we merge this, squash together the non-functional changes like formatting and linting into the appropriate original commits. You can use git rebase -i to do this, or something like git co <commit> && cargo fmt && cargo clippy --fix && git amend -m.

[claytonrcarter]

I just pushed a bunch of changes, but FYI they're not complete. I've taken care of all of the "easy" suggestions, now I'll start working on the juicier bits. I squashed all of the changes that you looked at yesterday into a single commit (noted as such) to maybe make it clearer what's been added when. Thanks again!

[claytonrcarter]

Do you think this should have a basic "status report" after the reword, similar to what git hide shows? I'm implemented this locally, but wanted to check before I push it. It looks something like this:

...
In-memory rebase succeeded.
Reworded commit 34538cdd more file-b again REWORDED as 9b6f8ced asdf
Reworded commit 9337b169 edit file-b as 79a75f06 asdf
Reworded commit 5605e4e6 more file-c as 3f05d582 asdf
Reworded 3 commits with same message. If this was unintentional, run: git undo

(This format is totally up for discussion, but currently it's "Reworded commit <old commit> as <new commit>" w/ friendly_describe(), and the "Reworded 3 commits..." line is only printed if the command run on >1 commit.)

I ask b/c the most obvious implementation is to propagate rewritten_oids from RebaseInMemoryResult::Succeeded (which is only used w/i execute.rs) into ExecuteRebasePlanResult::Succeeded so that the old commit <-> new commit mapping is available w/i the command. This is straightforward, but means updating 3 other commands that don't need this data.

On one hand, I think displaying a list of what's happened is desirable, but I want to confirm that w/ you before I solidify it and push it.

For reference, here's the output w/ this while doing a rewording of 3 commits that results in rewriting a tree of 5 commits:

❯ git sl
⋮
◇ 9337b169 55d edit file-b
┣━┓
┃ ◯ 5d86daa1 55d add file-c
┃ ┃
┃ ● 5605e4e6 1d (branch-c) more file-c
┃
◇ 079ca218 20d (main) more file-b
┃
◯ 34538cdd 1d more file-b again REWORDED

❯ git reword -m asdf 34538cdd 9337b169 5605e4e6
Attempting rebase in-memory...
[1/5] Committed as: 79a75f06 asdf
[2/5] Committed as: b4893191 more file-b
[3/5] Committed as: 9b6f8ced asdf
[4/5] Committed as: 4367170e add file-c
[5/5] Committed as: 3f05d582 asdf
branchless: processing 2 updates: branch branch-c, branch main
branchless: processing 5 rewritten commits
branchless: running command: git checkout 3f05d582ea23c7582e87a82f8c1f24f2a9940c86
Previous HEAD position was 5605e4e more file-c
branchless: processing 1 update: ref HEAD
HEAD is now at 3f05d58 asdf
branchless: processing checkout
⋮
◇ 79a75f06 0s asdf
┣━┓
┃ ◯ 4367170e 0s add file-c
┃ ┃
┃ ● 3f05d582 0s (branch-c) asdf
┃
◇ b4893191 0s (main) more file-b
┃
◯ 9b6f8ced 0s asdf
In-memory rebase succeeded.
Reworded commit 34538cdd more file-b again REWORDED as 9b6f8ced asdf
Reworded commit 9337b169 edit file-b as 79a75f06 asdf
Reworded commit 5605e4e6 more file-c as 3f05d582 asdf
Reworded 3 commits with same message. If this was unintentional, run: git undo

[arxanas]

Do you think this should have a basic "status report" after the reword, similar to what git hide shows?

Whatever you think is best is fine here 🙂. I think it's not necessary to show the smartlog, though, since it shouldn't have changed as part of the reword.

I ask b/c the most obvious implementation is to propagate rewritten_oids from RebaseInMemoryResult::Succeeded (which is only used w/i execute.rs) into ExecuteRebasePlanResult::Succeeded so that the old commit <-> new commit mapping is available w/i the command. This is straightforward, but means updating 3 other commands that don't need this data.

That's perfectly fine. We'll probably use this data for something else in the future too.

[arxanas]

Also, see this regarding the failing Windows CI runs: #306 (comment)

[claytonrcarter]

Whatever you think is best is fine here

OK, I'm leaving it in, and I've disabled the smartlog. The format that I settled on is Rewording commit <old oid> as <new oid> <new title>, for example:

❯ /Users/crcarter/src/Other/git-branchless/target/debug/git-branchless reword c91c645c 40983c8a -m qwerty
Attempting rebase in-memory...
[1/2] Committed as: 838fb6c0 qwerty
[2/2] Committed as: 28174d68 qwerty
branchless: processing 1 update: branch branch-c
branchless: processing 2 rewritten commits
branchless: running command: git checkout branch-c
Previous HEAD position was 40983c8 zxvc
Switched to branch 'branch-c'
branchless: processing checkout
In-memory rebase succeeded.
Reworded commit c91c645c as 28174d68 qwerty
Reworded commit 40983c8a as 838fb6c0 qwerty
Reworded 2 commits with same message. If this was unintentional, run: git undo

I think I've taken care of all of the outstanding review items with my latest push. I would still like to do another full read through before I call it ready, but that is mostly to look for odd or non-idiomatic constructs, things I could simplify, etc. I believe that all of the features are in place and working.

There are a few outstanding TODOs: 2 of them are related to the as-yet-unimplemented bulk edit feature, and 2 more I'll call out specifically in PR comments.

To confirm, are you OK if we leave the bulk edit feature out of this and add it later, after this has been merged? Or would you prefer to go for broke and add bulk editing now? On one hand, I think it would be nice to get this merged into master so folks could try it out as is; on the other hand, I don't think that bulk editing will be too difficult.

[arxanas]

I haven't done anything to look at @epage request that it #179 (comment). At this time, I don't even know where to start w/ that, so any pointers would be most welcome.

I think git reword should actually work as-is without git branchless init (although it will lazily construct the repository commit graph so that it knows how to rebase descendant commits). However, I think there's a bug and it'll only work if the main branch happens to be named master. If that's the case, it should be fixed by changing get_main_branch_name to delegate to get_default_branch_name, rather than the hardcoded master value.

To test this, try just running git branchless reword in a repository where you haven't run git branchless init and see what happens.

The bulk editing feature is not implemented; I am voting ✋ for us to review this as is and add that later, though I'm also fine to push on to work on bulk editing now if you'd prefer.

Certainly, let's defer that.

During my latest read through, I decided that we should probably a "footer" message to the $EDITOR akin to the default commit message from git. I'll work on this soon.

We can also ship what you have now and you can follow up in future PRs if you like. Besides a few minor points above, I think this is good to ship when you're ready.

[claytonrcarter]

To test this, try just running git branchless reword in a repository where you haven't run git branchless init and see what happens.

Yup, good call. I confirmed this and fixed it. I also confirmed that reword works as expected w/o init. 🚀

[claytonrcarter]

OK, I think I taken care of the latest round of review comments. Note that I have squashed all of the last round of reviewed work into 25346b9, and all new work is built on that. I gave everything a read through and think that this is good to go. I don't know what else to add or (more importantly) what else to remove. 😄

I'll leave a comment to draw your attention to one detail in particular, but I think we can call this done.

[arxanas]

🚀 HYPE 🚀