Migrate git.log.showGraph and git.log.order to app state

docs/Config.md

@@ -98,13 +98,6 @@ git:
     # extra args passed to `git merge`, e.g. --no-ff
     args: ''
   log:
-    # one of date-order, author-date-order, topo-order or default.
-    # topo-order makes it easier to read the git log graph, but commits may not
-    # appear chronologically. See https://git-scm.com/docs/git-log#_commit_ordering
-    order: 'topo-order'
-    # one of always, never, when-maximised
-    # this determines whether the git graph is rendered in the commits panel
-    showGraph: 'when-maximised'
     # displays the whole git graph by default in the commits panel (equivalent to passing the `--all` argument to `git log`)
     showWholeGraph: false
   skipHookPrefix: WIP

pkg/commands/git_commands/commit_loader.go

@@ -650,16 +650,16 @@ func (self *CommitLoader) getFirstPushedCommit(refName string) (string, error) {
 
 // getLog gets the git log.
 func (self *CommitLoader) getLogCmd(opts GetCommitsOptions) oscommands.ICmdObj {
-	config := self.UserConfig.Git.Log
-
 	refSpec := opts.RefName
 	if opts.RefToShowDivergenceFrom != "" {
 		refSpec += "..." + opts.RefToShowDivergenceFrom
 	}
 
+	logOrder := self.AppState.GitLogOrder
+
 	cmdArgs := NewGitCmd("log").
 		Arg(refSpec).
-		ArgIf(config.Order != "default", "--"+config.Order).
+		ArgIf(logOrder != "default", "--"+logOrder).
 		ArgIf(opts.All, "--all").
 		Arg("--oneline").
 		Arg(prettyFormat).

pkg/commands/git_commands/commit_loader_test.go

@@ -10,6 +10,7 @@ import (
 	"github.com/jesseduffield/lazygit/pkg/commands/models"
 	"github.com/jesseduffield/lazygit/pkg/commands/oscommands"
 	"github.com/jesseduffield/lazygit/pkg/commands/types/enums"
+	"github.com/jesseduffield/lazygit/pkg/config"
 	"github.com/jesseduffield/lazygit/pkg/utils"
 	"github.com/stretchr/testify/assert"
 )
@@ -305,7 +306,8 @@ func TestGetCommits(t *testing.T) {
 		scenario := scenario
 		t.Run(scenario.testName, func(t *testing.T) {
 			common := utils.NewDummyCommon()
-			common.UserConfig.Git.Log.Order = scenario.logOrder
+			common.AppState = config.GetDefaultAppState()
+			common.AppState.GitLogOrder = scenario.logOrder
 
 			builder := &CommitLoader{
 				Common:        common,

pkg/config/app_config.go

@@ -70,12 +70,7 @@ func NewAppConfig(
 		userConfigPaths = []string{filepath.Join(configDir, ConfigFilename)}
 	}
 
-	userConfig, err := loadUserConfigWithDefaults(userConfigPaths)
-	if err != nil {
-		return nil, err
-	}
-
-	appState, err := loadAppState()
+	userConfig, appState, err := loadUserConfig(userConfigPaths)
 	if err != nil {
 		return nil, err
 	}
@@ -94,6 +89,11 @@ func NewAppConfig(
 		IsNewRepo:       false,
 	}
 
+	// app state could have been affected by migrations and needs to be saved
+	if err := appConfig.SaveAppState(); err != nil {
+		return nil, err
+	}
+
 	return appConfig, nil
 }
 
@@ -124,21 +124,40 @@ func findOrCreateConfigDir() (string, error) {
 	return folder, os.MkdirAll(folder, 0o755)
 }
 
-func loadUserConfigWithDefaults(configFiles []string) (*UserConfig, error) {
-	return loadUserConfig(configFiles, GetDefaultConfig())
-}
+func loadUserConfig(configFiles []string) (*UserConfig, *AppState, error) {
+	userConfig := GetDefaultConfig()
+	appState := GetDefaultAppState()
+
+	// load recorded AppState from file
+	filepath, err := configFilePath("state.yml")
+	if err != nil {
+		if os.IsPermission(err) {
+			// todo: apparently when people have read-only permissions they prefer us to fail silently
+			return userConfig, appState, nil
+		}
+		return nil, nil, err
+	}
+	appStateBytes, err := os.ReadFile(filepath)
+	if err != nil && !os.IsNotExist(err) {
+		return nil, nil, err
+	}
+	if len(appStateBytes) != 0 {
+		err = yaml.Unmarshal(appStateBytes, appState)
+		if err != nil {
+			return nil, nil, err
+		}
+	}
 
-func loadUserConfig(configFiles []string, base *UserConfig) (*UserConfig, error) {
 	for _, path := range configFiles {
 		if _, err := os.Stat(path); err != nil {
 			if !os.IsNotExist(err) {
-				return nil, err
+				return nil, nil, err
 			}
 
 			// if use has supplied their own custom config file path(s), we assume
 			// the files have already been created, so we won't go and create them here.
 			if isCustomConfigFile(path) {
-				return nil, err
+				return nil, nil, err
 			}
 
 			file, err := os.Create(path)
@@ -147,46 +166,68 @@ func loadUserConfig(configFiles []string, base *UserConfig) (*UserConfig, error)
 					// apparently when people have read-only permissions they prefer us to fail silently
 					continue
 				}
-				return nil, err
+				return nil, nil, err
 			}
 			file.Close()
 		}
 
 		content, err := os.ReadFile(path)
 		if err != nil {
-			return nil, err
+			return nil, nil, err
 		}
 
-		content, err = migrateUserConfig(path, content)
+		content, err = migrateUserConfig(path, content, appState)
 		if err != nil {
-			return nil, err
+			return nil, nil, err
 		}
 
-		if err := yaml.Unmarshal(content, base); err != nil {
-			return nil, fmt.Errorf("The config at `%s` couldn't be parsed, please inspect it before opening up an issue.\n%w", path, err)
+		if err := yaml.Unmarshal(content, userConfig); err != nil {
+			return nil, nil, fmt.Errorf("the config at `%s` couldn't be parsed, please inspect it before opening up an issue.\n%w", path, err)
 		}
 	}
 
-	return base, nil
+	return userConfig, appState, nil
 }
 
 // Do any backward-compatibility migrations of things that have changed in the
 // config over time; examples are renaming a key to a better name, moving a key
-// from one container to another, or changing the type of a key (e.g. from bool
-// to an enum).
-func migrateUserConfig(path string, content []byte) ([]byte, error) {
+// from one container to another, changing the type of a key (e.g. from bool
+// to an enum) or moving a key from UserConfig to AppState.
+func migrateUserConfig(path string, content []byte, appState *AppState) ([]byte, error) {
 	changedContent, err := yaml_utils.RenameYamlKey(content, []string{"gui", "skipUnstageLineWarning"},
 		"skipDiscardChangeWarning")
 	if err != nil {
-		return nil, fmt.Errorf("Couldn't migrate config file at `%s`: %s", path, err)
+		return nil, fmt.Errorf("couldn't migrate config file at `%s`: %s", path, err)
+	}
+
+	changedContent, orderNode, err := yaml_utils.RemoveYamlNode(changedContent, []string{"git", "log", "order"})
+	if err != nil {
+		return nil, fmt.Errorf("couldn't migrate config file at `%s`: %s", path, err)
+	}
+	if orderNode != nil {
+		err := orderNode.Decode(&appState.GitLogOrder)
+		if err != nil {
+			return nil, fmt.Errorf("failed to migrate git.log.order at `%s`: %s", path, err)
+		}
+	}
+
+	changedContent, showGraphNode, err := yaml_utils.RemoveYamlNode(changedContent, []string{"git", "log", "showGraph"})
+	if err != nil {
+		return nil, fmt.Errorf("couldn't migrate config file at `%s`: %s", path, err)
+	}
+	if showGraphNode != nil {
+		err := showGraphNode.Decode(&appState.GitLogShowGraph)
+		if err != nil {
+			return nil, fmt.Errorf("failed to migrate git.log.showGraph at `%s`: %s", path, err)
+		}
 	}
 
 	// Add more migrations here...
 
 	// Write config back if changed
 	if string(changedContent) != string(content) {
 		if err := os.WriteFile(path, changedContent, 0o644); err != nil {
-			return nil, fmt.Errorf("Couldn't write migrated config back to `%s`: %s", path, err)
+			return nil, fmt.Errorf("couldn't write migrated config back to `%s`: %s", path, err)
 		}
 		return changedContent, nil
 	}
@@ -231,12 +272,13 @@ func (c *AppConfig) GetUserConfigDir() string {
 }
 
 func (c *AppConfig) ReloadUserConfig() error {
-	userConfig, err := loadUserConfigWithDefaults(c.UserConfigPaths)
+	userConfig, appState, err := loadUserConfig(c.UserConfigPaths)
 	if err != nil {
 		return err
 	}
 
 	c.UserConfig = userConfig
+	c.AppState = appState
 	return nil
 }
 
@@ -281,63 +323,6 @@ func (c *AppConfig) SaveAppState() error {
 	return err
 }
 
-// loadAppState loads recorded AppState from file
-func loadAppState() (*AppState, error) {
-	appState := getDefaultAppState()
-
-	filepath, err := configFilePath("state.yml")
-	if err != nil {
-		if os.IsPermission(err) {
-			// apparently when people have read-only permissions they prefer us to fail silently
-			return appState, nil
-		}
-		return nil, err
-	}
-
-	appStateBytes, err := os.ReadFile(filepath)
-	if err != nil && !os.IsNotExist(err) {
-		return nil, err
-	}
-
-	if len(appStateBytes) == 0 {
-		return appState, nil
-	}
-
-	err = yaml.Unmarshal(appStateBytes, appState)
-	if err != nil {
-		return nil, err
-	}
-
-	return appState, nil
-}
-
-// AppState stores data between runs of the app like when the last update check
-// was performed and which other repos have been checked out
-type AppState struct {
-	LastUpdateCheck     int64
-	RecentRepos         []string
-	StartupPopupVersion int
-
-	// these are for custom commands typed in directly, not for custom commands in the lazygit config
-	CustomCommandsHistory      []string
-	HideCommandLog             bool
-	IgnoreWhitespaceInDiffView bool
-	DiffContextSize            int
-	LocalBranchSortOrder       string
-	RemoteBranchSortOrder      string
-}
-
-func getDefaultAppState() *AppState {
-	return &AppState{
-		LastUpdateCheck:       0,
-		RecentRepos:           []string{},
-		StartupPopupVersion:   0,
-		DiffContextSize:       3,
-		LocalBranchSortOrder:  "recency",
-		RemoteBranchSortOrder: "alphabetical",
-	}
-}
-
 func LogPath() (string, error) {
 	if os.Getenv("LAZYGIT_LOG_PATH") != "" {
 		return os.Getenv("LAZYGIT_LOG_PATH"), nil

pkg/config/app_state.go

@@ -0,0 +1,39 @@
+package config
+
+// AppState stores data between runs of the app like when the last update check
+// was performed and which other repos have been checked out
+type AppState struct {
+	LastUpdateCheck     int64
+	RecentRepos         []string
+	StartupPopupVersion int
+
+	// these are for custom commands typed in directly, not for custom commands in the lazygit config
+	CustomCommandsHistory      []string
+	HideCommandLog             bool
+	IgnoreWhitespaceInDiffView bool
+	DiffContextSize            int
+	LocalBranchSortOrder       string
+	RemoteBranchSortOrder      string
+
+	// One of: 'date-order' | 'author-date-order' | 'topo-order | default'
+	// 'topo-order' makes it easier to read the git log graph, but commits may not
+	// appear chronologically. See https://git-scm.com/docs/
+	GitLogOrder string
+
+	// This determines whether the git graph is rendered in the commits panel
+	// One of 'always' | 'never' | 'when-maximised'
+	GitLogShowGraph string
+}
+
+func GetDefaultAppState() *AppState {
+	return &AppState{
+		LastUpdateCheck:       0,
+		RecentRepos:           []string{},
+		StartupPopupVersion:   0,
+		DiffContextSize:       3,
+		LocalBranchSortOrder:  "recency",
+		RemoteBranchSortOrder: "alphabetical",
+		GitLogOrder:           "topo-order",
+		GitLogShowGraph:       "when-maximised",
+	}
+}

pkg/config/user_config.go

@@ -243,13 +243,6 @@ type MergingConfig struct {
 }
 
 type LogConfig struct {
-	// One of: 'date-order' | 'author-date-order' | 'topo-order | default'
-	// 'topo-order' makes it easier to read the git log graph, but commits may not
-	// appear chronologically. See https://git-scm.com/docs/
-	Order string `yaml:"order" jsonschema:"enum=date-order,enum=author-date-order,enum=topo-order,enum=default"`
-	// This determines whether the git graph is rendered in the commits panel
-	// One of 'always' | 'never' | 'when-maximised'
-	ShowGraph string `yaml:"showGraph" jsonschema:"enum=always,enum=never,enum=when-maximised"`
 	// displays the whole git graph by default in the commits view (equivalent to passing the `--all` argument to `git log`)
 	ShowWholeGraph bool `yaml:"showWholeGraph"`
 }
@@ -653,8 +646,6 @@ func GetDefaultConfig() *UserConfig {
 				Args:         "",
 			},
 			Log: LogConfig{
-				Order:          "topo-order",
-				ShowGraph:      "when-maximised",
 				ShowWholeGraph: false,
 			},
 			SkipHookPrefix:      "WIP",

pkg/gui/context/local_commits_context.go

@@ -165,7 +165,7 @@ func shouldShowGraph(c *ContextCommon) bool {
 		return false
 	}
 
-	value := c.UserConfig.Git.Log.ShowGraph
+	value := c.GetAppState().GitLogShowGraph
 	switch value {
 	case "always":
 		return true

pkg/gui/controllers/local_commits_controller.go

@@ -883,7 +883,8 @@ func (self *LocalCommitsController) handleOpenLogMenu() error {
 				OnPress: func() error {
 					onPress := func(value string) func() error {
 						return func() error {
-							self.c.UserConfig.Git.Log.ShowGraph = value
+							self.c.GetAppState().GitLogShowGraph = value
+							self.c.SaveAppStateAndLogError()
 							return nil
 						}
 					}
@@ -912,7 +913,8 @@ func (self *LocalCommitsController) handleOpenLogMenu() error {
 				OnPress: func() error {
 					onPress := func(value string) func() error {
 						return func() error {
-							self.c.UserConfig.Git.Log.Order = value
+							self.c.GetAppState().GitLogOrder = value
+							self.c.SaveAppStateAndLogError()
 							return self.c.WithWaitingStatus(self.c.Tr.LoadingCommits, func(gocui.Task) error {
 								return self.c.Refresh(
 									types.RefreshOptions{

pkg/integration/tests/commit/highlight.go

@@ -10,7 +10,7 @@ var Highlight = NewIntegrationTest(NewIntegrationTestArgs{
 	ExtraCmdArgs: []string{},
 	Skip:         false,
 	SetupConfig: func(config *config.AppConfig) {
-		config.GetUserConfig().Git.Log.ShowGraph = "always"
+		config.GetAppState().GitLogShowGraph = "always"
 		config.GetUserConfig().Gui.AuthorColors = map[string]string{
 			"CI": "red",
 		}