Fix the Commits panel's column widths when scrolling during interactive rebase or bisect (#6022)

The commit list only renders the visible rows for performance reasons
(see #3687), which means it also uses only the visible rows for
determining the widths of the list's columns. In an interactive rebase,
when you scroll down so that no rebase todos are visible any more, the
author and subject columns would snap to the left, which looks ugly and
distracting. A similar thing happened in half-screen mode when only
commits from today are visible: today's commits use a shorter date
format, so as you scroll down to make an older commit visible, the
columns to the right of the date would move to the right to make room
for the longer date.

Fix this by taking the width requirements of _all_ commits into account,
not just the visible ones, and pad the column texts to those widths. For
the date we take a shortcut: measuring the widths of all commits would
be too expensive, so we only format the oldest commit's date, on the
assumption that this one is the least likely to be from today. This only
works when the long time format is actually longer than the short one,
and when the long format always results in the same width for all dates;
both of these assumptions might be false when users reconfigure their
`gui.timeFormat` or `gui.shortTimeFormat` configs (e.g. to include a
week day), but for the default values of these it works.

pkg/gui/context/reflog_commits_context.go

@@ -27,13 +27,10 @@ func NewReflogCommitsContext(c *ContextCommon) *ReflogCommitsContext {
 	)
 
 	getDisplayStrings := func(startIdx int, endIdx int) [][]string {
-		commits := viewModel.GetItems()
-		if startIdx >= len(commits) {
-			return nil
-		}
-
 		return presentation.GetReflogCommitListDisplayStrings(
-			commits[startIdx:endIdx],
+			viewModel.GetItems(),
+			startIdx,
+			endIdx,
 			c.State().GetRepoState().GetScreenMode() != types.SCREEN_NORMAL,
 			c.Modes().CherryPicking.SelectedHashSet(),
 			c.Modes().Diffing.Ref,

pkg/gui/presentation/commits.go

@@ -177,6 +177,10 @@ func GetCommitListDisplayStrings(
 					(hasRebaseUpdateRefsConfig || b.CommitHash != commits[0].Hash())
 		}))
 
+	reservedWidths := getReservedColumnWidths(
+		commits, common.UserConfig().Gui.CommitHashLength, fullDescription,
+		timeFormat, shortTimeFormat, now, bisectInfo, bisectBounds)
+
 	lines := make([][]string, 0, len(filteredCommits))
 	var bisectStatus BisectStatus
 	willBeRebased := markedBaseCommit == ""
@@ -204,11 +208,64 @@ func GetCommitListDisplayStrings(
 			fullDescription,
 			bisectStatus,
 			bisectInfo,
+			reservedWidths,
 		))
 	}
 	return lines
 }
 
+// The width of a column is the width of the widest string in it, and a column
+// whose strings are all empty is left out entirely. The panels that show a
+// commit list hand over only the lines that are on screen, so several columns
+// would change their width, or come and go, as the user scrolls. These are the
+// widths those columns need for all the commits in the list.
+type reservedColumnWidths struct {
+	hash        int
+	bisect      int
+	description int
+	action      int
+}
+
+// precondition: commits is not empty
+func getReservedColumnWidths(
+	commits []*models.Commit,
+	hashLength int,
+	fullDescription bool,
+	timeFormat string,
+	shortTimeFormat string,
+	now time.Time,
+	bisectInfo *git_commands.BisectInfo,
+	bisectBounds *bisectBounds,
+) reservedColumnWidths {
+	result := reservedColumnWidths{}
+
+	for i, commit := range commits {
+		result.hash = max(result.hash, utils.StringWidth(getHashText(commit, hashLength)))
+		if commit.IsTODO() {
+			result.action = max(result.action, utils.StringWidth(getActionText(commit)))
+		}
+		bisectStatus := getBisectStatus(i, commit.Hash(), bisectInfo, bisectBounds)
+		result.bisect = max(result.bisect,
+			utils.StringWidth(getBisectStatusText(bisectStatus, bisectInfo)))
+	}
+
+	if fullDescription {
+		// Formatting the date of every commit on every render would be too
+		// expensive, so measure the oldest one only. It is the one least likely
+		// to be from today, and so the one most likely to be shown in the long
+		// time format; with a conventional time format that one is both wider
+		// than the short format and the same width for every date, which makes
+		// it the width the whole column needs. An unconventional format can
+		// break either of those assumptions, and then some of the column's
+		// width still comes and goes; it can never reserve more width than one
+		// of the commits asks for, though.
+		result.description = utils.StringWidth(utils.UnixToDateSmart(
+			now, commits[len(commits)-1].UnixTimestamp, timeFormat, shortTimeFormat))
+	}
+
+	return result
+}
+
 func getbisectBounds(commits []*models.Commit, bisectInfo *git_commands.BisectInfo) *bisectBounds {
 	if !bisectInfo.Bisecting() {
 		return nil
@@ -313,24 +370,18 @@ func getBisectStatus(index int, commitHash string, bisectInfo *git_commands.Bise
 }
 
 func getBisectStatusText(bisectStatus BisectStatus, bisectInfo *git_commands.BisectInfo) string {
-	if bisectStatus == BisectStatusNone {
-		return ""
-	}
-
-	style := getBisectStatusColor(bisectStatus)
-
 	switch bisectStatus {
 	case BisectStatusNew:
-		return style.Sprintf("<-- " + bisectInfo.NewTerm())
+		return "<-- " + bisectInfo.NewTerm()
 	case BisectStatusOld:
-		return style.Sprintf("<-- " + bisectInfo.OldTerm())
+		return "<-- " + bisectInfo.OldTerm()
 	case BisectStatusCurrent:
 		// TODO: i18n
-		return style.Sprintf("<-- current")
+		return "<-- current"
 	case BisectStatusSkipped:
-		return style.Sprintf("<-- skipped")
+		return "<-- skipped"
 	case BisectStatusCandidate:
-		return style.Sprintf("?")
+		return "?"
 	case BisectStatusNone:
 		return ""
 	}
@@ -338,6 +389,33 @@ func getBisectStatusText(bisectStatus BisectStatus, bisectInfo *git_commands.Bis
 	return ""
 }
 
+func getHashText(commit *models.Commit, hashLength int) string {
+	hash := commit.Hash()
+	if hashLength >= len(hash) {
+		return hash
+	}
+	if hashLength > 0 {
+		return hash[:hashLength]
+	}
+	if !icons.IsIconEnabled() { // hashLength <= 0
+		return "*"
+	}
+	return ""
+}
+
+func getActionText(commit *models.Commit) string {
+	if commit.Action == models.ActionNone {
+		return ""
+	}
+
+	text := commit.Action.String()
+	// Only show the flag for fixup commands (where -C changes the meaning)
+	if commit.ActionFlag != "" && commit.Action == todo.Fixup {
+		text += " " + commit.ActionFlag
+	}
+	return text
+}
+
 func displayCommit(
 	common *common.Common,
 	commit *models.Commit,
@@ -355,18 +433,17 @@ func displayCommit(
 	fullDescription bool,
 	bisectStatus BisectStatus,
 	bisectInfo *git_commands.BisectInfo,
+	reservedWidths reservedColumnWidths,
 ) []string {
-	bisectString := getBisectStatusText(bisectStatus, bisectInfo)
+	bisectString := ""
+	if bisectText := getBisectStatusText(bisectStatus, bisectInfo); bisectText != "" {
+		bisectString = getBisectStatusColor(bisectStatus).Sprint(bisectText)
+	}
 
-	hashString := ""
 	hashColor := getHashColor(commit, diffName, cherryPickedCommitHashSet, bisectStatus, bisectInfo)
-	hashLength := common.UserConfig().Gui.CommitHashLength
-	if hashLength >= len(commit.Hash()) {
-		hashString = hashColor.Sprint(commit.Hash())
-	} else if hashLength > 0 {
-		hashString = hashColor.Sprint(commit.Hash()[:hashLength])
-	} else if !icons.IsIconEnabled() { // hashLength <= 0
-		hashString = hashColor.Sprint("*")
+	hashString := ""
+	if hashText := getHashText(commit, common.UserConfig().Gui.CommitHashLength); hashText != "" {
+		hashString = hashColor.Sprint(hashText)
 	}
 
 	divergenceString := ""
@@ -384,13 +461,8 @@ func displayCommit(
 	}
 
 	actionString := ""
-	if commit.Action != models.ActionNone {
-		actionStr := commit.Action.String()
-		// Only show the flag for fixup commands (where -C changes the meaning)
-		if commit.ActionFlag != "" && commit.Action == todo.Fixup {
-			actionStr += " " + commit.ActionFlag
-		}
-		actionString = actionColorMap(commit.Action, commit.Status).Sprint(actionStr)
+	if actionText := getActionText(commit); actionText != "" {
+		actionString = actionColorMap(commit.Action, commit.Status).Sprint(actionText)
 	}
 
 	tagString := ""
@@ -443,10 +515,10 @@ func displayCommit(
 	cols = append(
 		cols,
 		divergenceString,
-		hashString,
-		bisectString,
-		descriptionString,
-		actionString,
+		utils.WithPadding(hashString, reservedWidths.hash, utils.AlignLeft),
+		utils.WithPadding(bisectString, reservedWidths.bisect, utils.AlignLeft),
+		utils.WithPadding(descriptionString, reservedWidths.description, utils.AlignLeft),
+		utils.WithPadding(actionString, reservedWidths.action, utils.AlignLeft),
 		author,
 		graphLine+mark+tagString+theme.DefaultTextColor.Sprint(name),
 	)

pkg/gui/presentation/commits_test.go

@@ -17,8 +17,13 @@ import (
 	"github.com/xo/terminfo"
 )
 
+// Scenarios write their expected output as a raw string literal, indented with
+// tabs so that it lines up with the surrounding code. Strip that indentation,
+// along with the newlines after the opening backtick and before the closing
+// one. Spaces are left alone, so that a scenario can expect a line that starts
+// with an empty column.
 func formatExpected(expected string) string {
-	return strings.TrimSpace(strings.ReplaceAll(expected, "\t", ""))
+	return strings.Trim(strings.ReplaceAll(expected, "\t", ""), "\n")
 }
 
 func TestGetCommitListDisplayStrings(t *testing.T) {
@@ -270,8 +275,8 @@ func TestGetCommitListDisplayStrings(t *testing.T) {
 			cherryPickedCommitHashSet: set.New[string](),
 			now:                       time.Date(2020, 1, 1, 0, 0, 0, 0, time.UTC),
 			expected: formatExpected(`
-		hash4 ○ commit4
-		hash5 ○ commit5
+		hash4      ○ commit4
+		hash5      ○ commit5
 				`),
 		},
 		{
@@ -333,6 +338,25 @@ func TestGetCommitListDisplayStrings(t *testing.T) {
 			hash2 pick commit2
 				`),
 		},
+		{
+			testName: "only showing TODO commits that have no hash",
+			commitOpts: []models.NewCommitOpts{
+				{Name: "refs/heads/branch1", Action: todo.UpdateRef},
+				{Name: "refs/heads/branch2", Action: todo.UpdateRef},
+				{Name: "commit1", Hash: "hash1", Parents: []string{"hash2"}, Action: todo.Pick},
+				{Name: "commit2", Hash: "hash2", Parents: []string{"hash3"}},
+			},
+			startIdx:                  0,
+			endIdx:                    2,
+			showGraph:                 false,
+			bisectInfo:                git_commands.NewNullBisectInfo(),
+			cherryPickedCommitHashSet: set.New[string](),
+			now:                       time.Date(2020, 1, 1, 0, 0, 0, 0, time.UTC),
+			expected: formatExpected(`
+		      update-ref branch1
+		      update-ref branch2
+				`),
+		},
 		{
 			testName: "graph in divergence view - all commits visible",
 			commitOpts: []models.NewCommitOpts{
@@ -525,6 +549,25 @@ func TestGetCommitListDisplayStrings(t *testing.T) {
 		hash2 2019-12-20 Jesse Duffield    commit2
 						`),
 		},
+		{
+			testName: "only showing commits from today",
+			commitOpts: []models.NewCommitOpts{
+				{Name: "commit1", Hash: "hash1", UnixTimestamp: 1577844184, AuthorName: "Jesse Duffield"},
+				{Name: "commit2", Hash: "hash2", UnixTimestamp: 1576844184, AuthorName: "Jesse Duffield"},
+			},
+			fullDescription:           true,
+			timeFormat:                "2006-01-02",
+			shortTimeFormat:           "3:04PM",
+			startIdx:                  0,
+			endIdx:                    1,
+			showGraph:                 false,
+			bisectInfo:                git_commands.NewNullBisectInfo(),
+			cherryPickedCommitHashSet: set.New[string](),
+			now:                       time.Date(2020, 1, 1, 5, 3, 4, 0, time.UTC),
+			expected: formatExpected(`
+		hash1 2:03AM     Jesse Duffield    commit1
+						`),
+		},
 	}
 
 	oldColorLevel := color.ForceSetColorLevel(terminfo.ColorLevelNone)

pkg/gui/presentation/reflog_commits.go

@@ -12,25 +12,35 @@ import (
 	"github.com/samber/lo"
 )
 
-func GetReflogCommitListDisplayStrings(commits []*models.Commit, fullDescription bool, cherryPickedCommitHashSet *set.Set[string], diffName string, now time.Time, timeFormat string, shortTimeFormat string, parseEmoji bool) [][]string {
+func GetReflogCommitListDisplayStrings(commits []*models.Commit, startIdx int, endIdx int, fullDescription bool, cherryPickedCommitHashSet *set.Set[string], diffName string, now time.Time, timeFormat string, shortTimeFormat string, parseEmoji bool) [][]string {
+	if startIdx >= len(commits) {
+		return nil
+	}
+
 	var displayFunc func(*models.Commit, reflogCommitDisplayAttributes) []string
+	reservedDateWidth := 0
 	if fullDescription {
 		displayFunc = getFullDescriptionDisplayStringsForReflogCommit
+		// See getReservedColumnWidths for why the oldest entry alone decides
+		// how much width the date column needs.
+		reservedDateWidth = utils.StringWidth(utils.UnixToDateSmart(
+			now, commits[len(commits)-1].UnixTimestamp, timeFormat, shortTimeFormat))
 	} else {
 		displayFunc = getDisplayStringsForReflogCommit
 	}
 
-	return lo.Map(commits, func(commit *models.Commit, _ int) []string {
+	return lo.Map(commits[startIdx:endIdx], func(commit *models.Commit, _ int) []string {
 		diffed := commit.Hash() == diffName
 		cherryPicked := cherryPickedCommitHashSet.Includes(commit.Hash())
 		return displayFunc(commit,
 			reflogCommitDisplayAttributes{
-				cherryPicked:    cherryPicked,
-				diffed:          diffed,
-				parseEmoji:      parseEmoji,
-				timeFormat:      timeFormat,
-				shortTimeFormat: shortTimeFormat,
-				now:             now,
+				cherryPicked:      cherryPicked,
+				diffed:            diffed,
+				parseEmoji:        parseEmoji,
+				timeFormat:        timeFormat,
+				shortTimeFormat:   shortTimeFormat,
+				now:               now,
+				reservedDateWidth: reservedDateWidth,
 			})
 	})
 }
@@ -55,6 +65,9 @@ type reflogCommitDisplayAttributes struct {
 	timeFormat      string
 	shortTimeFormat string
 	now             time.Time
+	// The width the date column needs for the whole reflog, not just for the
+	// lines that are on screen
+	reservedDateWidth int
 }
 
 func getFullDescriptionDisplayStringsForReflogCommit(c *models.Commit, attrs reflogCommitDisplayAttributes) []string {
@@ -63,9 +76,12 @@ func getFullDescriptionDisplayStringsForReflogCommit(c *models.Commit, attrs ref
 		name = emoji.Sprint(name)
 	}
 
+	date := style.FgMagenta.Sprint(
+		utils.UnixToDateSmart(attrs.now, c.UnixTimestamp, attrs.timeFormat, attrs.shortTimeFormat))
+
 	return []string{
 		reflogHashColor(attrs.cherryPicked, attrs.diffed).Sprint(c.ShortHash()),
-		style.FgMagenta.Sprint(utils.UnixToDateSmart(attrs.now, c.UnixTimestamp, attrs.timeFormat, attrs.shortTimeFormat)),
+		utils.WithPadding(date, attrs.reservedDateWidth, utils.AlignLeft),
 		theme.DefaultTextColor.Sprint(name),
 	}
 }

pkg/gui/presentation/reflog_commits_test.go

@@ -0,0 +1,116 @@
+package presentation
+
+import (
+	"strings"
+	"testing"
+	"time"
+
+	"github.com/gookit/color"
+	"github.com/jesseduffield/generics/set"
+	"github.com/jesseduffield/lazygit/pkg/commands/models"
+	"github.com/jesseduffield/lazygit/pkg/utils"
+	"github.com/samber/lo"
+	"github.com/stretchr/testify/assert"
+	"github.com/xo/terminfo"
+)
+
+func TestGetReflogCommitListDisplayStrings(t *testing.T) {
+	scenarios := []struct {
+		testName        string
+		commitOpts      []models.NewCommitOpts
+		fullDescription bool
+		timeFormat      string
+		shortTimeFormat string
+		now             time.Time
+		startIdx        int
+		endIdx          int
+		expected        string
+	}{
+		{
+			testName:   "no commits",
+			commitOpts: []models.NewCommitOpts{},
+			startIdx:   0,
+			endIdx:     1,
+			now:        time.Date(2020, 1, 1, 5, 3, 4, 0, time.UTC),
+			expected:   "",
+		},
+		{
+			testName: "some commits",
+			commitOpts: []models.NewCommitOpts{
+				{Name: "checkout: moving from master to mybranch", Hash: "hash1"},
+				{Name: "commit: make a change", Hash: "hash2"},
+			},
+			startIdx: 0,
+			endIdx:   2,
+			now:      time.Date(2020, 1, 1, 5, 3, 4, 0, time.UTC),
+			expected: formatExpected(`
+		hash1 checkout: moving from master to mybranch
+		hash2 commit: make a change
+				`),
+		},
+		{
+			testName: "full description",
+			commitOpts: []models.NewCommitOpts{
+				{Name: "commit: today", Hash: "hash1", UnixTimestamp: 1577844184},
+				{Name: "commit: a while ago", Hash: "hash2", UnixTimestamp: 1576844184},
+			},
+			fullDescription: true,
+			timeFormat:      "2006-01-02",
+			shortTimeFormat: "3:04PM",
+			startIdx:        0,
+			endIdx:          2,
+			now:             time.Date(2020, 1, 1, 5, 3, 4, 0, time.UTC),
+			expected: formatExpected(`
+		hash1 2:03AM     commit: today
+		hash2 2019-12-20 commit: a while ago
+				`),
+		},
+		{
+			testName: "only showing commits from today",
+			commitOpts: []models.NewCommitOpts{
+				{Name: "commit: today", Hash: "hash1", UnixTimestamp: 1577844184},
+				{Name: "commit: a while ago", Hash: "hash2", UnixTimestamp: 1576844184},
+			},
+			fullDescription: true,
+			timeFormat:      "2006-01-02",
+			shortTimeFormat: "3:04PM",
+			startIdx:        0,
+			endIdx:          1,
+			now:             time.Date(2020, 1, 1, 5, 3, 4, 0, time.UTC),
+			expected: formatExpected(`
+		hash1 2:03AM     commit: today
+				`),
+		},
+	}
+
+	oldColorLevel := color.ForceSetColorLevel(terminfo.ColorLevelNone)
+	defer color.ForceSetColorLevel(oldColorLevel)
+
+	for _, s := range scenarios {
+		t.Run(s.testName, func(t *testing.T) {
+			hashPool := &utils.StringPool{}
+
+			commits := lo.Map(s.commitOpts,
+				func(opts models.NewCommitOpts, _ int) *models.Commit { return models.NewCommit(hashPool, opts) })
+
+			result := GetReflogCommitListDisplayStrings(
+				commits,
+				s.startIdx,
+				s.endIdx,
+				s.fullDescription,
+				set.New[string](),
+				"",
+				s.now,
+				s.timeFormat,
+				s.shortTimeFormat,
+				false,
+			)
+
+			renderedLines, _ := utils.RenderDisplayStrings(result, nil)
+			renderedResult := strings.Join(renderedLines, "\n")
+			t.Logf("\n%s", renderedResult)
+
+			assert.EqualValues(t, s.expected, renderedResult)
+		})
+	}
+}

pkg/utils/color.go

@@ -12,21 +12,22 @@ import (
 var (
 	decoloriseCache = make(map[string]string)
 	decoloriseMutex sync.RWMutex
+
+	colorCodeRe = regexp.MustCompile(`\x1B\[([0-9]{1,3}(;[0-9]{1,3})*)?[mGK]`)
+	linkRe      = regexp.MustCompile(`\x1B]8;[^;]*;(.*?)(\x1B.|\x07)`)
 )
 
 // Decolorise strips a string of color
 func Decolorise(str string) string {
 	decoloriseMutex.RLock()
-	val := decoloriseCache[str]
+	val, ok := decoloriseCache[str]
 	decoloriseMutex.RUnlock()
 
-	if val != "" {
+	if ok {
 		return val
 	}
 
-	re := regexp.MustCompile(`\x1B\[([0-9]{1,3}(;[0-9]{1,3})*)?[mGK]`)
-	linkRe := regexp.MustCompile(`\x1B]8;[^;]*;(.*?)(\x1B.|\x07)`)
-	ret := re.ReplaceAllString(str, "")
+	ret := colorCodeRe.ReplaceAllString(str, "")
 	ret = linkRe.ReplaceAllString(ret, "")
 
 	decoloriseMutex.Lock()

pkg/utils/formatting.go

@@ -36,6 +36,12 @@ func StringWidth(s string) int {
 
 // WithPadding pads a string as much as you want
 func WithPadding(str string, padding int, alignment Alignment) string {
+	if padding <= 0 {
+		// Nothing to pad to, and measuring the string isn't free: Decolorise
+		// compiles a regex whenever it is called with a string it hasn't cached.
+		return str
+	}
+
 	uncoloredStr := Decolorise(str)
 	width := StringWidth(uncoloredStr)
 	if padding < width {