Generate one fixup per commit, not per-hunk?

[nickolay]

Currently running git-absorb with non-trivial changes creates LOTS of fixup commits with the same message, because a separate fixup commit is created for each hunk. Is there a reason to do this or could we squash those into a few commits?

[tummychow]

as you've observed, the user gets no benefit from having one fixup commit per hunk if they're both absorbing into the same commit. however, it makes the absorption loop a LOT easier to write. i would probably accept a PR for this if it could be implemented in a legible way, but instinctively i feel like this would be a scary refactor to actually do

[stof]

the advantage of limiting the number of fixup commits appears when a human inspects them. For instance, in our company, we regularly push our fixup commits into our feature branches on github, so that the reviewer of the PR can easily see what has changed since last time, and we squash them only at the end before merging the PR.

[sammko]

Chiming in with the same workflow as @stof, pushing fixup!s and squashing them only at the time of merging the PR makes it a bit easier for reviewers, compared to force pushing every time. I haven't looked at the code, but maybe doing it as a post-processing step could be more viable, instead of rewriting the loop to group hunks? As in

1. figure out all the commits to make (same as now)
2. merge those with same fixup target together

or if commits are not collected and committed at the end but committed immediately

1. commit the fixups
2. go back and run a rebase which squashes the fixups among themselves, but not with the original target.

[tummychow]

1. figure out all the commits to make (same as now)
2. merge those with same fixup target together

yes, i think this is the most likely implementation that would work. you have to wait to make commits until after the entire absorption loop is done.

1. commit the fixups
2. go back and run a rebase which squashes the fixups among themselves, but not with the original target.

where as this approach sounds quite fiddly to me - because you're performing two interactive rebases in a row (one to combine fixups together, and then another to absorb the fixups into the actual commits) and you have to keep the commit messages organized.

[craftey]

I have tested the new option --one-fixup-per-commit. Within my first test it seemed not to work as expected:

I have a feature with one commit that contains two hunks in a single file. Now I change some bits in both hunks, stage the changes and run git absorb --one-fixup-per-commit. This creates only one fixup commit for the second (!) hunk. The other change from the first hunk is kept in git stage. Running git absorb --one-fixup-per-commit again will create another fixup commit for the change that was left. I would have expected one fixup commit changing both hunks within the first run.

(Now running a soft reset to origin to start over) If I run just git absorb I get two fixup commits with same title, ie one per hunk.

[tummychow]

@craftey ugh, yes i can repro. it's some off-by-one line offset issue. i'm not going to revert the pr but the feature will stay out of a release until i (or someone else) have time to track it down. good thing is there's no regression in the old behavior, it still has the right line offsets there.

[craftey]

Maybe @rbartlensky can have a look.

[rbartlensky]

@craftey @tummychow I can have a look this week -- thanks for the repro case!

[rbartlensky]

Opened #93 to fix the issue

[nickolay]

The feature is available in the 0.6.11 version on an opt-in basis:

-F, --one-fixup-per-commit    Only generate one fixup per commit

I've been happily using it since August by adding it to my sublime merge configuration

$ cat /Users/nickolay/Library/Application\ Support/Sublime\ Merge/Packages/User/Default.sublime-commands
[
    {
        "caption": "Git absorb",
        "command": "git",
        "args": {"argv": ["absorb", "-F"]}
    }
]

As far as I am concerned the only reason to keep this issue open is to maybe make it the default, on which Stephen commented elsewhere:

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

Huge thanks again to everyone who made this happen — @rbartlensky, @tummychow, @craftey!

[kiprasmel]

fyi you can now enable -F to always be true via the new config option since #101:

[absorb]
    oneFixupPerCommit = true