Skip to content

Commit 647f533

Browse files
stefanhallerstefanhaller
authored
With stacked branches, create fixup commit at the end of the branch it belongs to (#3892)
- **PR Description** When working with stacked branches, and creating a fixup commit for a commit in one of the lower branches of the stack, the fixup was created at the top of the stack and the user needed to move it down to the right branch manually. This is unnecessary extra work; create it at the end of the right branch automatically.
2 parents a793f70 + b22149d commit 647f533

File tree

7 files changed

+172
-23
lines changed

7 files changed

+172
-23
lines changed

‎pkg/app/daemon/daemon.go‎

Lines changed: 9 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -246,16 +246,18 @@ func (self *ChangeTodoActionsInstruction) run(common *common.Common) error {
246246

247247
// Takes the hash of some commit, and the hash of a fixup commit that was created
248248
// at the end of the branch, then moves the fixup commit down to right after the
249-
// original commit, changing its type to "fixup"
249+
// original commit, changing its type to "fixup" (only if ChangeToFixup is true)
250250
type MoveFixupCommitDownInstruction struct {
251-
OriginalHash string
252-
FixupHash string
251+
OriginalHash string
252+
FixupHash string
253+
ChangeToFixup bool
253254
}
254255

255-
func NewMoveFixupCommitDownInstruction(originalHash string, fixupHash string) Instruction {
256+
func NewMoveFixupCommitDownInstruction(originalHash string, fixupHash string, changeToFixup bool) Instruction {
256257
return &MoveFixupCommitDownInstruction{
257-
OriginalHash: originalHash,
258-
FixupHash: fixupHash,
258+
OriginalHash: originalHash,
259+
FixupHash: fixupHash,
260+
ChangeToFixup: changeToFixup,
259261
}
260262
}
261263

@@ -269,7 +271,7 @@ func (self *MoveFixupCommitDownInstruction) SerializedInstructions() string {
269271

270272
func (self *MoveFixupCommitDownInstruction) run(common *common.Common) error {
271273
return handleInteractiveRebase(common, func(path string) error {
272-
return utils.MoveFixupCommitDown(path, self.OriginalHash, self.FixupHash, getCommentChar())
274+
return utils.MoveFixupCommitDown(path, self.OriginalHash, self.FixupHash, self.ChangeToFixup, getCommentChar())
273275
})
274276
}
275277

‎pkg/commands/git_commands/rebase.go‎

Lines changed: 20 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -284,6 +284,11 @@ func (self *RebaseCommands) GitRebaseEditTodo(todosFileContent []byte) error {
284284
return cmdObj.Run()
285285
}
286286

287+
func (self *RebaseCommands) getHashOfLastCommitMade() (string, error) {
288+
cmdArgs := NewGitCmd("rev-parse").Arg("--verify", "HEAD").ToArgv()
289+
return self.cmd.New(cmdArgs).RunWithOutput()
290+
}
291+
287292
// AmendTo amends the given commit with whatever files are staged
288293
func (self *RebaseCommands) AmendTo(commits []*models.Commit, commitIndex int) error {
289294
commit := commits[commitIndex]
@@ -292,17 +297,28 @@ func (self *RebaseCommands) AmendTo(commits []*models.Commit, commitIndex int) e
292297
return err
293298
}
294299

295-
// Get the hash of the commit we just created
296-
cmdArgs := NewGitCmd("rev-parse").Arg("--verify", "HEAD").ToArgv()
297-
fixupHash, err := self.cmd.New(cmdArgs).RunWithOutput()
300+
fixupHash, err := self.getHashOfLastCommitMade()
298301
if err != nil {
299302
return err
300303
}
301304

302305
return self.PrepareInteractiveRebaseCommand(PrepareInteractiveRebaseCommandOpts{
303306
baseHashOrRoot: getBaseHashOrRoot(commits, commitIndex+1),
304307
overrideEditor: true,
305-
instruction: daemon.NewMoveFixupCommitDownInstruction(commit.Hash, fixupHash),
308+
instruction: daemon.NewMoveFixupCommitDownInstruction(commit.Hash, fixupHash, true),
309+
}).Run()
310+
}
311+
312+
func (self *RebaseCommands) MoveFixupCommitDown(commits []*models.Commit, targetCommitIndex int) error {
313+
fixupHash, err := self.getHashOfLastCommitMade()
314+
if err != nil {
315+
return err
316+
}
317+
318+
return self.PrepareInteractiveRebaseCommand(PrepareInteractiveRebaseCommandOpts{
319+
baseHashOrRoot: getBaseHashOrRoot(commits, targetCommitIndex+1),
320+
overrideEditor: true,
321+
instruction: daemon.NewMoveFixupCommitDownInstruction(commits[targetCommitIndex].Hash, fixupHash, false),
306322
}).Run()
307323
}
308324

‎pkg/gui/controllers/local_commits_controller.go‎

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -895,6 +895,10 @@ func (self *LocalCommitsController) createFixupCommit(commit *models.Commit) err
895895
return err
896896
}
897897

898+
if err := self.moveFixupCommitToOwnerStackedBranch(commit); err != nil {
899+
return err
900+
}
901+
898902
self.context().MoveSelectedLine(1)
899903
return self.c.Refresh(types.RefreshOptions{Mode: types.SYNC})
900904
})
@@ -924,6 +928,50 @@ func (self *LocalCommitsController) createFixupCommit(commit *models.Commit) err
924928
})
925929
}
926930

931+
func (self *LocalCommitsController) moveFixupCommitToOwnerStackedBranch(targetCommit *models.Commit) error {
932+
if self.c.Git().Version.IsOlderThan(2, 38, 0) {
933+
// Git 2.38.0 introduced the `rebase.updateRefs` config option. Don't
934+
// move the commit down with older versions, as it would break the stack.
935+
return nil
936+
}
937+
938+
if self.c.Git().Status.WorkingTreeState() != enums.REBASE_MODE_NONE {
939+
// Can't move commits while rebasing
940+
return nil
941+
}
942+
943+
if targetCommit.Status == models.StatusMerged {
944+
// Target commit is already on main. It's a bit questionable that we
945+
// allow creating a fixup commit for it in the first place, but we
946+
// always did, so why restrict that now; however, it doesn't make sense
947+
// to move the created fixup commit down in that case.
948+
return nil
949+
}
950+
951+
if !self.c.Git().Config.GetRebaseUpdateRefs() {
952+
// If the user has disabled rebase.updateRefs, we don't move the fixup
953+
// because this would break the stack of branches (presumably they like
954+
// to manage it themselves manually, or something).
955+
return nil
956+
}
957+
958+
headOfOwnerBranchIdx := -1
959+
for i := self.context().GetSelectedLineIdx(); i > 0; i-- {
960+
if lo.SomeBy(self.c.Model().Branches, func(b *models.Branch) bool {
961+
return b.CommitHash == self.c.Model().Commits[i].Hash
962+
}) {
963+
headOfOwnerBranchIdx = i
964+
break
965+
}
966+
}
967+
968+
if headOfOwnerBranchIdx == -1 {
969+
return nil
970+
}
971+
972+
return self.c.Git().Rebase.MoveFixupCommitDown(self.c.Model().Commits, headOfOwnerBranchIdx)
973+
}
974+
927975
func (self *LocalCommitsController) createAmendCommit(commit *models.Commit, includeFileChanges bool) error {
928976
commitMessage, err := self.c.Git().Commit.GetCommitMessage(commit.Hash)
929977
if err != nil {
@@ -947,6 +995,10 @@ func (self *LocalCommitsController) createAmendCommit(commit *models.Commit, inc
947995
return err
948996
}
949997

998+
if err := self.moveFixupCommitToOwnerStackedBranch(commit); err != nil {
999+
return err
1000+
}
1001+
9501002
self.context().MoveSelectedLine(1)
9511003
return self.c.Refresh(types.RefreshOptions{Mode: types.SYNC})
9521004
})

‎pkg/integration/tests/commit/create_fixup_commit_in_branch_stack.go‎

Lines changed: 53 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,53 @@
1+
package commit
2+
3+
import (
4+
"github.com/jesseduffield/lazygit/pkg/config"
5+
. "github.com/jesseduffield/lazygit/pkg/integration/components"
6+
)
7+
8+
var CreateFixupCommitInBranchStack = NewIntegrationTest(NewIntegrationTestArgs{
9+
Description: "Create a fixup commit in a stack of branches, verify that it is created at the end of the branch it belongs to",
10+
ExtraCmdArgs: []string{},
11+
Skip: false,
12+
GitVersion: AtLeast("2.38.0"),
13+
SetupConfig: func(config *config.AppConfig) {},
14+
SetupRepo: func(shell *Shell) {
15+
shell.NewBranch("branch1")
16+
shell.EmptyCommit("branch1 commit 1")
17+
shell.EmptyCommit("branch1 commit 2")
18+
shell.EmptyCommit("branch1 commit 3")
19+
shell.NewBranch("branch2")
20+
shell.EmptyCommit("branch2 commit 1")
21+
shell.EmptyCommit("branch2 commit 2")
22+
shell.CreateFileAndAdd("fixup-file", "fixup content")
23+
24+
shell.SetConfig("rebase.updateRefs", "true")
25+
},
26+
Run: func(t *TestDriver, keys config.KeybindingConfig) {
27+
t.Views().Commits().
28+
Focus().
29+
Lines(
30+
Contains("CI ◯ branch2 commit 2"),
31+
Contains("CI ◯ branch2 commit 1"),
32+
Contains("CI ◯ * branch1 commit 3"),
33+
Contains("CI ◯ branch1 commit 2"),
34+
Contains("CI ◯ branch1 commit 1"),
35+
).
36+
NavigateToLine(Contains("branch1 commit 2")).
37+
Press(keys.Commits.CreateFixupCommit).
38+
Tap(func() {
39+
t.ExpectPopup().Menu().
40+
Title(Equals("Create fixup commit")).
41+
Select(Contains("fixup! commit")).
42+
Confirm()
43+
}).
44+
Lines(
45+
Contains("CI ◯ branch2 commit 2"),
46+
Contains("CI ◯ branch2 commit 1"),
47+
Contains("CI ◯ * fixup! branch1 commit 2"),
48+
Contains("CI ◯ branch1 commit 3"),
49+
Contains("CI ◯ branch1 commit 2"),
50+
Contains("CI ◯ branch1 commit 1"),
51+
)
52+
},
53+
})

‎pkg/integration/tests/test_list.go‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -87,6 +87,7 @@ var tests = []*components.IntegrationTest{
8787
commit.CommitWithNonMatchingBranchName,
8888
commit.CommitWithPrefix,
8989
commit.CreateAmendCommit,
90+
commit.CreateFixupCommitInBranchStack,
9091
commit.CreateTag,
9192
commit.DiscardOldFileChanges,
9293
commit.FindBaseCommitForFixup,

‎pkg/utils/rebase_todo.go‎

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -221,21 +221,21 @@ func moveTodosUp(todos []todo.Todo, todosToMove []Todo) ([]todo.Todo, error) {
221221
return todos, nil
222222
}
223223

224-
func MoveFixupCommitDown(fileName string, originalHash string, fixupHash string, commentChar byte) error {
224+
func MoveFixupCommitDown(fileName string, originalHash string, fixupHash string, changeToFixup bool, commentChar byte) error {
225225
todos, err := ReadRebaseTodoFile(fileName, commentChar)
226226
if err != nil {
227227
return err
228228
}
229229

230-
newTodos, err := moveFixupCommitDown(todos, originalHash, fixupHash)
230+
newTodos, err := moveFixupCommitDown(todos, originalHash, fixupHash, changeToFixup)
231231
if err != nil {
232232
return err
233233
}
234234

235235
return WriteRebaseTodoFile(fileName, newTodos, commentChar)
236236
}
237237

238-
func moveFixupCommitDown(todos []todo.Todo, originalHash string, fixupHash string) ([]todo.Todo, error) {
238+
func moveFixupCommitDown(todos []todo.Todo, originalHash string, fixupHash string, changeToFixup bool) ([]todo.Todo, error) {
239239
isOriginal := func(t todo.Todo) bool {
240240
return (t.Command == todo.Pick || t.Command == todo.Merge) && equalHash(t.Commit, originalHash)
241241
}
@@ -259,7 +259,9 @@ func moveFixupCommitDown(todos []todo.Todo, originalHash string, fixupHash strin
259259

260260
newTodos := MoveElement(todos, fixupIndex, originalIndex+1)
261261

262-
newTodos[originalIndex+1].Command = todo.Fixup
262+
if changeToFixup {
263+
newTodos[originalIndex+1].Command = todo.Fixup
264+
}
263265

264266
return newTodos, nil
265267
}

‎pkg/utils/rebase_todo_test.go‎

Lines changed: 31 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -266,32 +266,50 @@ func TestRebaseCommands_moveFixupCommitDown(t *testing.T) {
266266
todos []todo.Todo
267267
originalHash string
268268
fixupHash string
269+
changeToFixup bool
269270
expectedTodos []todo.Todo
270271
expectedErr error
271272
}{
272273
{
273-
name: "fixup commit is the last commit",
274+
name: "fixup commit is the last commit (change to fixup)",
274275
todos: []todo.Todo{
275276
{Command: todo.Pick, Commit: "original"},
276277
{Command: todo.Pick, Commit: "fixup"},
277278
},
278-
originalHash: "original",
279-
fixupHash: "fixup",
279+
originalHash: "original",
280+
fixupHash: "fixup",
281+
changeToFixup: true,
280282
expectedTodos: []todo.Todo{
281283
{Command: todo.Pick, Commit: "original"},
282284
{Command: todo.Fixup, Commit: "fixup"},
283285
},
284286
expectedErr: nil,
285287
},
288+
{
289+
name: "fixup commit is the last commit (don't change to fixup)",
290+
todos: []todo.Todo{
291+
{Command: todo.Pick, Commit: "original"},
292+
{Command: todo.Pick, Commit: "fixup"},
293+
},
294+
originalHash: "original",
295+
fixupHash: "fixup",
296+
changeToFixup: false,
297+
expectedTodos: []todo.Todo{
298+
{Command: todo.Pick, Commit: "original"},
299+
{Command: todo.Pick, Commit: "fixup"},
300+
},
301+
expectedErr: nil,
302+
},
286303
{
287304
name: "fixup commit is separated from original commit",
288305
todos: []todo.Todo{
289306
{Command: todo.Pick, Commit: "original"},
290307
{Command: todo.Pick, Commit: "other"},
291308
{Command: todo.Pick, Commit: "fixup"},
292309
},
293-
originalHash: "original",
294-
fixupHash: "fixup",
310+
originalHash: "original",
311+
fixupHash: "fixup",
312+
changeToFixup: true,
295313
expectedTodos: []todo.Todo{
296314
{Command: todo.Pick, Commit: "original"},
297315
{Command: todo.Fixup, Commit: "fixup"},
@@ -306,8 +324,9 @@ func TestRebaseCommands_moveFixupCommitDown(t *testing.T) {
306324
{Command: todo.Pick, Commit: "other"},
307325
{Command: todo.Pick, Commit: "fixup"},
308326
},
309-
originalHash: "original",
310-
fixupHash: "fixup",
327+
originalHash: "original",
328+
fixupHash: "fixup",
329+
changeToFixup: true,
311330
expectedTodos: []todo.Todo{
312331
{Command: todo.Merge, Commit: "original"},
313332
{Command: todo.Fixup, Commit: "fixup"},
@@ -324,6 +343,7 @@ func TestRebaseCommands_moveFixupCommitDown(t *testing.T) {
324343
},
325344
originalHash: "original",
326345
fixupHash: "fixup",
346+
changeToFixup: true,
327347
expectedTodos: nil,
328348
expectedErr: errors.New("Expected exactly one original hash, found 2"),
329349
},
@@ -336,6 +356,7 @@ func TestRebaseCommands_moveFixupCommitDown(t *testing.T) {
336356
},
337357
originalHash: "original",
338358
fixupHash: "fixup",
359+
changeToFixup: true,
339360
expectedTodos: nil,
340361
expectedErr: errors.New("Expected exactly one fixup hash, found 2"),
341362
},
@@ -346,6 +367,7 @@ func TestRebaseCommands_moveFixupCommitDown(t *testing.T) {
346367
},
347368
originalHash: "original",
348369
fixupHash: "fixup",
370+
changeToFixup: true,
349371
expectedTodos: nil,
350372
expectedErr: errors.New("Expected exactly one fixup hash, found 0"),
351373
},
@@ -356,14 +378,15 @@ func TestRebaseCommands_moveFixupCommitDown(t *testing.T) {
356378
},
357379
originalHash: "original",
358380
fixupHash: "fixup",
381+
changeToFixup: true,
359382
expectedTodos: nil,
360383
expectedErr: errors.New("Expected exactly one original hash, found 0"),
361384
},
362385
}
363386

364387
for _, scenario := range scenarios {
365388
t.Run(scenario.name, func(t *testing.T) {
366-
actualTodos, actualErr := moveFixupCommitDown(scenario.todos, scenario.originalHash, scenario.fixupHash)
389+
actualTodos, actualErr := moveFixupCommitDown(scenario.todos, scenario.originalHash, scenario.fixupHash, scenario.changeToFixup)
367390

368391
if scenario.expectedErr == nil {
369392
assert.NoError(t, actualErr)

0 commit comments

Comments
 (0)