From d1202340d980ea8e48422df09d64326fe021d5d7 Mon Sep 17 00:00:00 2001 From: Alex March Date: Thu, 28 Dec 2023 19:41:37 +0900 Subject: [PATCH] Migrate git.log.showGraph and git.log.order to app state --- docs/Config.md | 7 - pkg/commands/git_commands/commit_loader.go | 6 +- .../git_commands/commit_loader_test.go | 4 +- pkg/config/app_config.go | 149 ++++++++---------- pkg/config/app_state.go | 39 +++++ pkg/config/user_config.go | 9 -- pkg/gui/context/local_commits_context.go | 2 +- .../controllers/local_commits_controller.go | 6 +- pkg/integration/tests/commit/highlight.go | 2 +- pkg/utils/yaml_utils/yaml_utils.go | 66 +++++++- pkg/utils/yaml_utils/yaml_utils_test.go | 62 ++++++++ schema/config.json | 21 --- 12 files changed, 240 insertions(+), 133 deletions(-) create mode 100644 pkg/config/app_state.go diff --git a/docs/Config.md b/docs/Config.md index 2f1f57ecc233..16e2d85f4450 100644 --- a/docs/Config.md +++ b/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 diff --git a/pkg/commands/git_commands/commit_loader.go b/pkg/commands/git_commands/commit_loader.go index 4b0a23692733..10da7558b77d 100644 --- a/pkg/commands/git_commands/commit_loader.go +++ b/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). diff --git a/pkg/commands/git_commands/commit_loader_test.go b/pkg/commands/git_commands/commit_loader_test.go index 3f8fbd58e6c0..12f10d375da7 100644 --- a/pkg/commands/git_commands/commit_loader_test.go +++ b/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, diff --git a/pkg/config/app_config.go b/pkg/config/app_config.go index b6366cd2d7e6..7db577564e0b 100644 --- a/pkg/config/app_config.go +++ b/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,38 +166,60 @@ 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... @@ -186,7 +227,7 @@ func migrateUserConfig(path string, content []byte) ([]byte, error) { // 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 diff --git a/pkg/config/app_state.go b/pkg/config/app_state.go new file mode 100644 index 000000000000..958daf7af7aa --- /dev/null +++ b/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", + } +} diff --git a/pkg/config/user_config.go b/pkg/config/user_config.go index 749b41822404..7297d07aae71 100644 --- a/pkg/config/user_config.go +++ b/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", diff --git a/pkg/gui/context/local_commits_context.go b/pkg/gui/context/local_commits_context.go index e0172638daae..6e57e6801680 100644 --- a/pkg/gui/context/local_commits_context.go +++ b/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 diff --git a/pkg/gui/controllers/local_commits_controller.go b/pkg/gui/controllers/local_commits_controller.go index efb06fe71c6e..5f581b85e809 100644 --- a/pkg/gui/controllers/local_commits_controller.go +++ b/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{ diff --git a/pkg/integration/tests/commit/highlight.go b/pkg/integration/tests/commit/highlight.go index eaa77ccf16bb..077a42b9de94 100644 --- a/pkg/integration/tests/commit/highlight.go +++ b/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", } diff --git a/pkg/utils/yaml_utils/yaml_utils.go b/pkg/utils/yaml_utils/yaml_utils.go index 9d96fa7a7530..a9df97422dbb 100644 --- a/pkg/utils/yaml_utils/yaml_utils.go +++ b/pkg/utils/yaml_utils/yaml_utils.go @@ -60,7 +60,7 @@ func updateYamlNode(node *yaml.Node, path []string, value string) (bool, error) } key := path[0] - if _, valueNode := lookupKey(node, key); valueNode != nil { + if _, valueNode, _ := lookupKey(node, key); valueNode != nil { return updateYamlNode(valueNode, path[1:], value) } @@ -89,14 +89,14 @@ func updateYamlNode(node *yaml.Node, path []string, value string) (bool, error) return updateYamlNode(newNode, path[1:], value) } -func lookupKey(node *yaml.Node, key string) (*yaml.Node, *yaml.Node) { +func lookupKey(node *yaml.Node, key string) (*yaml.Node, *yaml.Node, int) { for i := 0; i < len(node.Content)-1; i += 2 { if node.Content[i].Value == key { - return node.Content[i], node.Content[i+1] + return node.Content[i], node.Content[i+1], i } } - return nil, nil + return nil, nil, -1 } // takes a yaml document in bytes, a path to a key, and a new name for the key. @@ -135,7 +135,7 @@ func renameYamlKey(node *yaml.Node, path []string, newKey string) (bool, error) return false, errors.New("yaml node in path is not a dictionary") } - keyNode, valueNode := lookupKey(node, path[0]) + keyNode, valueNode, _ := lookupKey(node, path[0]) if keyNode == nil { return false, nil } @@ -143,7 +143,7 @@ func renameYamlKey(node *yaml.Node, path []string, newKey string) (bool, error) // end of path reached: rename key if len(path) == 1 { // Check that new key doesn't exist yet - if newKeyNode, _ := lookupKey(node, newKey); newKeyNode != nil { + if newKeyNode, _, _ := lookupKey(node, newKey); newKeyNode != nil { return false, fmt.Errorf("new key `%s' already exists", newKey) } @@ -153,3 +153,57 @@ func renameYamlKey(node *yaml.Node, path []string, newKey string) (bool, error) return renameYamlKey(valueNode, path[1:], newKey) } + +func RemoveYamlNode(yamlBytes []byte, path []string) ([]byte, *yaml.Node, error) { + var node yaml.Node + err := yaml.Unmarshal(yamlBytes, &node) + if err != nil { + return nil, nil, fmt.Errorf("failed to parse YAML: %w", err) + } + + if len(node.Content) == 0 { + return yamlBytes, nil, nil + } + + body := node.Content[0] + + if body.Kind != yaml.MappingNode { + return yamlBytes, nil, errors.New("yaml document is not a dictionary") + } + + didChange, removedNode, err := removeYamlNode(body, path) + if err != nil || !didChange { + return yamlBytes, removedNode, err + } + + // Convert the updated YAML node back to YAML bytes. + updatedYAMLBytes, err := yaml.Marshal(body) + if err != nil { + return nil, nil, fmt.Errorf("failed to convert YAML node to bytes: %w", err) + } + + return updatedYAMLBytes, removedNode, nil +} + +func removeYamlNode(node *yaml.Node, path []string) (bool, *yaml.Node, error) { + if len(path) == 0 { + return false, nil, nil + } + + keyNode, valueNode, idx := lookupKey(node, path[0]) + if keyNode == nil { + return false, nil, nil + } + + if len(path) == 1 { + removedNode := &yaml.Node{ + Kind: yaml.ScalarNode, + Value: valueNode.Value, + } + // remove key and value nodes from parent contents + node.Content = append(node.Content[:idx], node.Content[idx+2:]...) + return true, removedNode, nil + } + + return removeYamlNode(valueNode, path[1:]) +} diff --git a/pkg/utils/yaml_utils/yaml_utils_test.go b/pkg/utils/yaml_utils/yaml_utils_test.go index 7f9dc20f7126..6373eeaea727 100644 --- a/pkg/utils/yaml_utils/yaml_utils_test.go +++ b/pkg/utils/yaml_utils/yaml_utils_test.go @@ -4,6 +4,7 @@ import ( "testing" "github.com/stretchr/testify/assert" + "gopkg.in/yaml.v3" ) func TestUpdateYamlValue(t *testing.T) { @@ -199,3 +200,64 @@ func TestRenameYamlKey(t *testing.T) { }) } } + +func TestRemoveYamlNode(t *testing.T) { + tests := []struct { + name string + in string + path []string + expectedNode *yaml.Node + expectedOut string + expectedErr string + }{ + { + name: "remove a scalar node", + in: "foo: bar\n", + path: []string{"foo"}, + expectedNode: &yaml.Node{Kind: yaml.ScalarNode, Value: "bar"}, + expectedOut: "{}\n", + expectedErr: "", + }, + { + name: "remove a nested node", + in: "foo:\n bar: baz\n qux: val\n", + path: []string{"foo", "bar"}, + expectedNode: &yaml.Node{Kind: yaml.ScalarNode, Value: "baz"}, + expectedOut: "foo:\n qux: val\n", + expectedErr: "", + }, + { + name: "remove last nested node", + in: "foo:\n bar: baz\n", + path: []string{"foo", "bar"}, + expectedNode: &yaml.Node{Kind: yaml.ScalarNode, Value: "baz"}, + expectedOut: "foo: {}\n", + expectedErr: "", + }, + + // Error cases + { + name: "existing document is not a dictionary", + in: "42\n", + path: []string{"foo"}, + expectedNode: nil, + expectedOut: "42\n", + expectedErr: "yaml document is not a dictionary", + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + out, yamlNode, actualErr := RemoveYamlNode([]byte(test.in), test.path) + + if test.expectedErr == "" { + assert.NoError(t, actualErr) + } else { + assert.EqualError(t, actualErr, test.expectedErr) + } + + assert.Equal(t, test.expectedNode, yamlNode) + assert.Equal(t, test.expectedOut, string(out)) + }) + } +} diff --git a/schema/config.json b/schema/config.json index 54f6d69612b2..0621c48a599a 100644 --- a/schema/config.json +++ b/schema/config.json @@ -513,27 +513,6 @@ }, "log": { "properties": { - "order": { - "type": "string", - "enum": [ - "date-order", - "author-date-order", - "topo-order", - "default" - ], - "description": "One of: 'date-order' | 'author-date-order' | 'topo-order | default'\n'topo-order' makes it easier to read the git log graph, but commits may not\nappear chronologically. See https://git-scm.com/docs/", - "default": "topo-order" - }, - "showGraph": { - "type": "string", - "enum": [ - "always", - "never", - "when-maximised" - ], - "description": "This determines whether the git graph is rendered in the commits panel\nOne of 'always' | 'never' | 'when-maximised'", - "default": "when-maximised" - }, "showWholeGraph": { "type": "boolean", "description": "displays the whole git graph by default in the commits view (equivalent to passing the `--all` argument to `git log`)"