From baa0b2804b03d811e498eb290bc47993e410d6a8 Mon Sep 17 00:00:00 2001 From: Valentin Kiselev Date: Tue, 14 Nov 2023 10:57:25 +0300 Subject: [PATCH] fix: don't check checksum file when explicitly calling lefthook install (#572) --- cmd/install.go | 19 ++- cmd/root.go | 14 ++- internal/lefthook/install.go | 17 +-- internal/lefthook/install_test.go | 190 +++++++++++++++++++++--------- internal/lefthook/lefthook.go | 5 +- internal/lefthook/run.go | 2 +- 6 files changed, 167 insertions(+), 80 deletions(-) diff --git a/cmd/install.go b/cmd/install.go index 6863e415..8fca459b 100644 --- a/cmd/install.go +++ b/cmd/install.go @@ -4,27 +4,34 @@ import ( "github.com/spf13/cobra" "github.com/evilmartians/lefthook/internal/lefthook" + "github.com/evilmartians/lefthook/internal/log" ) func newInstallCmd(opts *lefthook.Options) *cobra.Command { - args := lefthook.InstallArgs{} + var a, force bool installCmd := cobra.Command{ Use: "install", Short: "Write basic configuration file in your project repository. Or initialize existed config", RunE: func(cmd *cobra.Command, _args []string) error { - return lefthook.Install(opts, &args) + return lefthook.Install(opts, force) }, } + // To be dropped in next releases. installCmd.Flags().BoolVarP( - &args.Force, "force", "f", false, - "reinstall hooks without checking config version", + &force, "force", "f", false, + "overwrite .old hooks", ) installCmd.Flags().BoolVarP( - &args.Aggressive, "aggressive", "a", false, - "remove all hooks from .git/hooks dir and install lefthook hooks", + &a, "aggressive", "a", false, + "use --force flag instead", ) + err := installCmd.Flags().MarkDeprecated("aggressive", "use --force flag instead") + if err != nil { + log.Warn("Unexpected error:", err) + } + return &installCmd } diff --git a/cmd/root.go b/cmd/root.go index 12949eb1..c3ecae63 100644 --- a/cmd/root.go +++ b/cmd/root.go @@ -6,6 +6,7 @@ import ( "github.com/spf13/cobra" "github.com/evilmartians/lefthook/internal/lefthook" + "github.com/evilmartians/lefthook/internal/log" ) var commands = [...]func(*lefthook.Options) *cobra.Command{ @@ -41,14 +42,23 @@ func newRootCmd() *cobra.Command { &options.NoColors, "no-colors", false, "disable colored output", ) + // To be dropped in next releases. rootCmd.Flags().BoolVarP( &options.Force, "force", "f", false, - "DEPRECATED: reinstall hooks without checking config version", + "use command-specific --force option", ) rootCmd.Flags().BoolVarP( &options.Aggressive, "aggressive", "a", false, - "DEPRECATED: remove all hooks from .git/hooks dir and install lefthook hooks", + "use --force flag instead", ) + err := rootCmd.Flags().MarkDeprecated("aggressive", "use command-specific --force option") + if err != nil { + log.Warn("Unexpected error:", err) + } + err = rootCmd.Flags().MarkDeprecated("force", "use command-specific --force option") + if err != nil { + log.Warn("Unexpected error:", err) + } for _, subcommand := range commands { rootCmd.AddCommand(subcommand(&options)) diff --git a/internal/lefthook/install.go b/internal/lefthook/install.go index 3e77a78e..05627c39 100644 --- a/internal/lefthook/install.go +++ b/internal/lefthook/install.go @@ -34,21 +34,17 @@ var ( errNoConfig = fmt.Errorf("no lefthook config found") ) -type InstallArgs struct { - Force, Aggressive bool -} - // Install installs the hooks from config file to the .git/hooks. -func Install(opts *Options, args *InstallArgs) error { +func Install(opts *Options, force bool) error { lefthook, err := initialize(opts) if err != nil { return err } - return lefthook.Install(args) + return lefthook.Install(force) } -func (l *Lefthook) Install(args *InstallArgs) error { +func (l *Lefthook) Install(force bool) error { cfg, err := l.readOrCreateConfig() if err != nil { return err @@ -66,8 +62,7 @@ func (l *Lefthook) Install(args *InstallArgs) error { } } - return l.createHooksIfNeeded(cfg, - args.Force || args.Aggressive || l.Options.Force || l.Options.Aggressive) + return l.createHooksIfNeeded(cfg, false, force) } func (l *Lefthook) readOrCreateConfig() (*config.Config, error) { @@ -111,8 +106,8 @@ func (l *Lefthook) createConfig(path string) error { return nil } -func (l *Lefthook) createHooksIfNeeded(cfg *config.Config, force bool) error { - if !force && l.hooksSynchronized() { +func (l *Lefthook) createHooksIfNeeded(cfg *config.Config, checkHashSum, force bool) error { + if checkHashSum && l.hooksSynchronized() { return nil } diff --git a/internal/lefthook/install_test.go b/internal/lefthook/install_test.go index 6fafcba6..6772f8b3 100644 --- a/internal/lefthook/install_test.go +++ b/internal/lefthook/install_test.go @@ -36,7 +36,7 @@ func TestLefthookInstall(t *testing.T) { for n, tt := range [...]struct { name, config, checksum string - args InstallArgs + force bool existingHooks map[string]string wantExist, wantNotExist []string wantError bool @@ -119,7 +119,7 @@ post-commit: }, }, { - name: "with synchronized hooks", + name: "with stale timestamp and checksum", config: ` pre-commit: commands: @@ -131,20 +131,17 @@ post-commit: notify: run: echo 'Done!' `, - checksum: "8b2c9fc6b3391b3cf020b97ab7037c61 1655894410\n", + checksum: "8b2c9fc6b3391b3cf020b97ab7037c62 1555894310\n", wantExist: []string{ configPath, - infoPath(config.ChecksumFileName), - }, - wantNotExist: []string{ hookPath("pre-commit"), hookPath("post-commit"), hookPath(config.GhostHookName), + infoPath(config.ChecksumFileName), }, }, { - name: "with synchronized hooks forced", - args: InstallArgs{Force: true}, + name: "with existing hook and .old file", config: ` pre-commit: commands: @@ -156,18 +153,23 @@ post-commit: notify: run: echo 'Done!' `, - checksum: "8b2c9fc6b3391b3cf020b97ab7037c61 1655894410\n", + existingHooks: map[string]string{ + "pre-commit": "", + "pre-commit.old": "", + }, + wantError: true, wantExist: []string{ configPath, hookPath("pre-commit"), - hookPath("post-commit"), - hookPath(config.GhostHookName), + hookPath("pre-commit.old"), + }, + wantNotExist: []string{ infoPath(config.ChecksumFileName), }, }, { - name: "with stale timestamp but synchronized", - args: InstallArgs{}, + name: "with existing hook and .old file, but forced", + force: true, config: ` pre-commit: commands: @@ -179,20 +181,116 @@ post-commit: notify: run: echo 'Done!' `, - checksum: "8b2c9fc6b3391b3cf020b97ab7037c61 1555894310\n", + existingHooks: map[string]string{ + "pre-commit": "", + "pre-commit.old": "", + }, wantExist: []string{ configPath, - infoPath(config.ChecksumFileName), - }, - wantNotExist: []string{ hookPath("pre-commit"), + hookPath("pre-commit.old"), hookPath("post-commit"), - hookPath(config.GhostHookName), + infoPath(config.ChecksumFileName), }, }, + } { + fs := afero.NewMemMapFs() + lefthook := &Lefthook{ + Options: &Options{Fs: fs}, + repo: repo, + } + + t.Run(fmt.Sprintf("%d: %s", n, tt.name), func(t *testing.T) { + // Create configuration file + if len(tt.config) > 0 { + if err := afero.WriteFile(fs, configPath, []byte(tt.config), 0o644); err != nil { + t.Errorf("unexpected error: %s", err) + } + timestamp := time.Date(2022, time.June, 22, 10, 40, 10, 1, time.UTC) + if err := fs.Chtimes(configPath, timestamp, timestamp); err != nil { + t.Errorf("unexpected error: %s", err) + } + } + + if len(tt.checksum) > 0 { + if err := afero.WriteFile(fs, lefthook.checksumFilePath(), []byte(tt.checksum), 0o644); err != nil { + t.Errorf("unexpected error: %s", err) + } + } + + // Create files that should exist + for hook, content := range tt.existingHooks { + path := hookPath(hook) + if err := fs.MkdirAll(filepath.Dir(path), 0o755); err != nil { + t.Errorf("unexpected error: %s", err) + } + if err := afero.WriteFile(fs, path, []byte(content), 0o755); err != nil { + t.Errorf("unexpected error: %s", err) + } + } + + // Do install + err := lefthook.Install(tt.force) + if tt.wantError && err == nil { + t.Errorf("expected an error") + } else if !tt.wantError && err != nil { + t.Errorf("unexpected error: %s", err) + } + + // Test files that should exist + for _, file := range tt.wantExist { + ok, err := afero.Exists(fs, file) + if err != nil { + t.Errorf("unexpected error: %s", err) + } + if !ok { + t.Errorf("expected %s to exist", file) + } + } + + // Test files that should not exist + for _, file := range tt.wantNotExist { + ok, err := afero.Exists(fs, file) + if err != nil { + t.Errorf("unexpected error: %s", err) + } + if ok { + t.Errorf("expected %s to not exist", file) + } + } + }) + } +} + +func TestCreateHooksIfNeeded(t *testing.T) { + root, err := filepath.Abs("src") + if err != nil { + t.Errorf("unexpected error: %s", err) + } + + configPath := filepath.Join(root, "lefthook.yml") + + hookPath := func(hook string) string { + return filepath.Join(root, ".git", "hooks", hook) + } + + infoPath := func(file string) string { + return filepath.Join(root, ".git", "info", file) + } + + repo := &git.Repository{ + HooksPath: filepath.Join(root, ".git", "hooks"), + RootPath: root, + InfoPath: filepath.Join(root, ".git", "info"), + } + for n, tt := range [...]struct { + name, config, checksum string + existingHooks map[string]string + wantExist, wantNotExist []string + wantError bool + }{ { - name: "with stale timestamp and checksum", - args: InstallArgs{}, + name: "with synchronized hooks", config: ` pre-commit: commands: @@ -204,17 +302,19 @@ post-commit: notify: run: echo 'Done!' `, - checksum: "8b2c9fc6b3391b3cf020b97ab7037c62 1555894310\n", + checksum: "8b2c9fc6b3391b3cf020b97ab7037c61 1655894410\n", wantExist: []string{ configPath, + infoPath(config.ChecksumFileName), + }, + wantNotExist: []string{ hookPath("pre-commit"), hookPath("post-commit"), hookPath(config.GhostHookName), - infoPath(config.ChecksumFileName), }, }, { - name: "with existing hook and .old file", + name: "with stale timestamp but synchronized", config: ` pre-commit: commands: @@ -226,44 +326,15 @@ post-commit: notify: run: echo 'Done!' `, - existingHooks: map[string]string{ - "pre-commit": "", - "pre-commit.old": "", - }, - wantError: true, + checksum: "8b2c9fc6b3391b3cf020b97ab7037c61 1555894310\n", wantExist: []string{ configPath, - hookPath("pre-commit"), - hookPath("pre-commit.old"), - }, - wantNotExist: []string{ infoPath(config.ChecksumFileName), }, - }, - { - name: "with existing hook and .old file, but forced", - args: InstallArgs{Force: true}, - config: ` -pre-commit: - commands: - tests: - run: yarn test - -post-commit: - commands: - notify: - run: echo 'Done!' -`, - existingHooks: map[string]string{ - "pre-commit": "", - "pre-commit.old": "", - }, - wantExist: []string{ - configPath, + wantNotExist: []string{ hookPath("pre-commit"), - hookPath("pre-commit.old"), hookPath("post-commit"), - infoPath(config.ChecksumFileName), + hookPath(config.GhostHookName), }, }, } { @@ -302,8 +373,13 @@ post-commit: } } - // Do install - err := lefthook.Install(&tt.args) + cfg, err := config.Load(lefthook.Fs, repo) + if err != nil { + t.Errorf("unexpected error: %s", err) + } + + // Create hooks + err = lefthook.createHooksIfNeeded(cfg, true, true) if tt.wantError && err == nil { t.Errorf("expected an error") } else if !tt.wantError && err != nil { diff --git a/internal/lefthook/lefthook.go b/internal/lefthook/lefthook.go index a4bbd9a8..29de2f51 100644 --- a/internal/lefthook/lefthook.go +++ b/internal/lefthook/lefthook.go @@ -101,10 +101,9 @@ func (l *Lefthook) cleanHook(hook string, force bool) error { } if exists { if force { - log.Infof("File %s.old already exists, overwriting\n", hook) + log.Infof("\nFile %s.old already exists, overwriting\n", hook) } else { - log.Errorf("Can't rename %s to %s.old - file already exists\n", hook, hook) - return fmt.Errorf("file %s.old already exists", hook) + return fmt.Errorf("Can't rename %s to %s.old - file already exists", hook, hook) } } diff --git a/internal/lefthook/run.go b/internal/lefthook/run.go index a51ae4b4..b0724182 100644 --- a/internal/lefthook/run.go +++ b/internal/lefthook/run.go @@ -90,7 +90,7 @@ func (l *Lefthook) Run(hookName string, args RunArgs, gitArgs []string) error { } // This line controls updating the git hook if config has changed - if err = l.createHooksIfNeeded(cfg, false); err != nil { + if err = l.createHooksIfNeeded(cfg, true, false); err != nil { log.Warn( `⚠️ There was a problem with synchronizing git hooks. Run 'lefthook install' manually.`,