feat(move): Add --fixup to squash moved commits into the destination

[claytonrcarter]

Summary
As the title says, this adds a move --fixup flag to squash commits into the destination, in memory or on disk. With this, both of these use cases:

• rebase -i + reordering & marking as fixup!
• reword --fixup <dest> <revset> ; rebase -i --autosquash

Can be replaced with just

• move -d <dest> -x <revset> --fixup

Add'l examples:

• Oops, used commit instead of amend, lets squash HEAD into it's parent: move -d @~ -x @ --fixup
• Squash a branch onto a single commit: move -d head(stack()) -s children(head(stack())) --fixup
  • Useful perhaps for cases like Dealing with `squash` and `sync` #965 (reply in thread)

The original idea came from #538 (review) and it's taken me most of a year of on and (mostly) off work to get it working.

Status

• In memory squashing seems to be working flawlessly. In the current state, I've yet to encounter any issues with this.
• On disk squashing seems to be working well with one caveat:
  • For some reason, the intermediate squashed commits aren't reliably cleaned up after the rebase, so the smartlog ends up with lots of # This is a combination of ... commits leftover.
  • This does not seem to be an issue for non-branchless triggered rebases. In other words, if I do add --updated ; absorb ; rebase -i --autosquash, it seems to work fine w/o leftover commits.
  • I've not been able to repro, but my current suspicion is that it's somehow related to merge conflicts and the usage of --fixup -m to kick things out to disk; and thus probably the rebase plan that I'm creating for on disk rebases or some of the changes related to that.
  • In all cases, the leftover commits can be safely discarded with some thing like hide 'stack() - main..branches()'

Review request

• This is a big PR (13 files changed, 2479 insertions(+), 204 deletions(-)), but most of these (1929 insertions) are new tests.
• The summary of how this works is:
  • sort the commits (inc the destination) topologically
  • apply all commits to the 'rootiest' of these, one after the other in topological order
  • remove all moved commits from the tree
  • replace the dest commit w/ the final squashed commit
  • For in-memory, this is all done in Rust/git2, creating a new in-mem commit for each squash but only writing the final one to disk. I think that this is "right", but I don't know if it's the fastest approach.
  • For on-disk, this is all done via git commands fed to rebase -i. This involved lots of manual commit rewriting and hiding to get the order and final tree right. This works (mostly, see above) but feels really clunky and heavy handed.
• There are 4 FIXME comments that I left in place w/ questions and/or concerns about my implementation.
  • I think that most of these are along the lines of "well this is working, but is it the right way to be doing it"
• I would love feedback on the names in the UI (--fixup) and API (structs and such).

As always, I welcome any feedback, guidance or suggestions. This has been tricky to get my head around, but I've found it very satisfying to use. I'm pretty happy w/ how it works, and I'd love to get it polished up and merged. Thank you!

Original description

@arxanas I could use some help with this.

This seems to be working for simple cases (ie in memory sticks) but I think I could use some guidance to get unblocked on where to go next. Specifically, I'm having a hard time getting my head around how to handle on-disk fixup rebases and having some issues with fixing up complicated commit graphs. I feel like I'm playing whack-a-mole with errors and conditions: if I get one test working, another breaks; when I get that working, the other one breaks; etc, and I'm left wondering if my approach is heading in the wrong direction and/or is too rigid or something.

• This is still pretty "draft"; please don't be too alarmed by the ugly bits that I need to go back through and clean up! 😄

• I added some code that uses git2::Tree::iter() directly

• I started to work on this, but it felt very copy-pasty and boilerplate-y and seemed to add a lot more code than I was expecting compared to just using tree.inner.iter(), plus other parts of repo.rs already use tree.inner.iter(), so I gave up and moved on to more fun stuff. 😆 Does it seem OK to leave that as is, or would you prefer that it be wrapped?

• In-memory fix ups seem to work for stick stacks, but fixup rebases that involve trees are being left in a "needs restack" state.

• Is this inevitable, or is there is a convenient way to recognize and automatically restack in cases like these? (See test_move_fixup_complicated.

• On disk rebase is not working Update On disk fixup rebases are working.

  • As noted, the last commit in this stack is me hacking around to try to get on-disk fixup rebases to work. It's incomplete and breaks many/most of the in-memory fixup rebase tests.
  • I have tried doing SkipUpstreamAppliedCommit after every FixUp. This helps in some cases, but not all, and it makes in-memory fixups involving multiple fixup commits complicated b/c it's hard to keep track of the target/fixed-up commit as it's always being "rewritten to zero".
  • Perhaps I need to be sorting the RebaseCommands differently, so that SkipUpstreamAppliedCommit is only called at the end of the rebase?
  • Or could it be that I'm not giving the post-rewrite etc hooks the info they need to figure things out?

[claytonrcarter]

I just pushed a new stack that has in-mem and on-disk rebases working for (almost) all tests. The last test (in a separate commit) is still buh-roken and I'm just parking it for now so I can ruminate on it some more. I think it's just coming down to the order in which the rebase commands are executed, thus the order in which they're added to the queue. (That is what fixed my problem w/ on-disk rebases: they just weren't being executed in the correct order.)

[claytonrcarter]

Sorry for the ongoing monologue, but I'm making progress: all on-disk tests are passing, and I'm close to all in-mem tests, too. The last test revealed a misconception regarding my usage and implementation of amend_fast that I'm working on now.

[claytonrcarter]

And, we're good! All tests passing and the amend is working correctly (I think). I still have some proofreading and clean up to do, but I think it's at least working. I'll be playing with it for a few days to see how it behaves under real use.

[arxanas]

Sorry I wasn't much help with this! Hopefully I'll have some time to review this weekend or next week.

[pokey]

Just to check, this flag doesn't replace the original commits, right? Would be cool to combine with #553 so that you could replace the original commits. Then a proper squash would be

git move --exact "<revset>" --replace "<revset>" --fixup

[pokey]

I just looked through the tests—this is so cool! 😍

[claytonrcarter]

Update: this has been dealt with.

Just a comment to myself about something I will need to fix but can't at this moment: encountered a bug related to squashing older commits into newer commits, in particular when interim commits also changed a file. Example with commits A-B-C-D-E:

• A introduces file
• B&C each make "unrelated" changes to the file, resulting in different hunks (ie they're easily commutable w/o conflict)
• D&E introduce changes related to B but to different files (ie no conflicts)

Goal: squash B and E into D, to create A-C-D' where D'=B+D+E.

Ran: git move -Fx B+E -d D. This seems to have correctly applied E to D, but not B. Instead, the resulting commit included none of the changes from B but did include a revert of the change from C.

Have not looked into it, but I suspect that this has something to do w/ comparing commits in the wrong order and possibly to the wrong "parents". (eg git diff B C will show an addition of C, but git diff C B will show a deletion.) This also highlights the need for the tests to be updated to compare the contents of changed files, not just the presence of changed paths.

[claytonrcarter]

OK, now I think this is really working. My previous version was naively replacing trees w/o actually comparing their contents, but this version is actually doing what one would expect it to be doing, at least according to the tests. The implementation is still pretty rough and gross and I'll need to go back and deal with FIXMEs, fill in error messages, wrap some git2 function calls, improve the portability, etc before it's really ready. First, though, I'd like to play with it in real code for a while.

[claytonrcarter]

I'll leave this closed until I get further with it, but the branch is rebased and up to date at https://github.com/claytonrcarter/git-branchless/tree/move-fixup

[claytonrcarter]

Reopening this PR. This seems to be working as expected for nearly all day-to-day uses, and the code is as good as I can get w/o having add'l 👀 on it.

[claytonrcarter]

I'm assuming that the CI lint error is unrelated. I don't get any clippy warnings locally w/ Rust 1.70, but it looks like CI is using 1.71. Also the error is at git-branchless-lib/src/core/dag.rs:402:19, which this PR doesn't touch.

[claytonrcarter]

I'm assuming that the CI lint error is unrelated.

Yup. Fixed in #996

[pokey]

For some reason, the intermediate squashed commits aren't reliably cleaned up after the rebase, so the smartlog ends up with lots of # This is a combination of ... commits leftover.

Fwiw I've seen this behaviour with regular rebases as well. I don't think it always happens for me tho

[arxanas]

Thanks for implementing!

[claytonrcarter]

Parking this for the night. There are a couple more things to work through, but most feedback has been resolved. I want to give this another full read through, too, just to be fussy.

[claytonrcarter]

🎉 Thanks for your patience and help on this @arxanas. If you notice anything wonky, let me know and I can look into it.

[claytonrcarter]

For some reason, the intermediate squashed commits aren't reliably cleaned up after the rebase, so the smartlog ends up with lots of # This is a combination of ... commits leftover.

Fwiw I've seen this behaviour with regular rebases as well. I don't think it always happens for me tho

BTW I looked into this further and I was able to repro this on (pre-merge) master, so I don't think it's related to this PR. Thanks again @pokey