Bug: Global extend options will take precedence over local options

[ardelato]

🔧 Summary

We recently came across some unexpected behavior when trying to make use of the exclude_tags option in a lefthook-local.yml.

We had already declared a exclude_tags option in one of the extends files included through the global config, but we wanted to overwrite it through a lefthook-local.yml config.

We discovered that the extends files will take precedence over the lefthook-local.yml config because it gets re-applied at this line:

lefthook/internal/config/load.go

Lines 116 to 117 in 8959135

	if err := mergeOne([]string{"lefthook-local", ".lefthook-local"}, "", extends); err == nil {
	if err = extend(extends, repo.RootPath); err != nil {

Steps to reproduce

I added a test to load_test.go to reproduce the issue: master...ardelato:bug-repro--extends-takes-precendence-over-local

--- FAIL: TestLoad (0.06s)
    --- FAIL: TestLoad/13:_with_extends_and_local (0.01s)
        load_test.go:717: configs should be equal
        load_test.go:718: (-want +got):
              &config.Config{
                ... // 10 identical fields
                Remote:  nil,
                Remotes: nil,
                Hooks: map[string]*config.Hook{
                        "pre-commit": &{
                                Commands: map[string]*config.Command{
            +                           "extended-tests": &{Run: "bundle exec rspec", Tags: []string{"backend", "test"}},
            +                           "global-lint":    &{Run: "bundle exec rubocop", Tags: []string{"backend", "linter"}, Glob: "*.rb"},
                                        "global-other":   &{Run: "bundle exec rubocop", Tags: {"other"}},
                                },
                                Scripts:     nil,
                                Files:       "",
                                Parallel:    true,
                                Piped:       false,
                                Follow:      false,
            -                   ExcludeTags: []string{"backend"},
            +                   ExcludeTags: []string{"test"},
                                Skip:        nil,
                                Only:        nil,
                        },
                },
              }
FAIL

Expected results

I expected all options from the lefthook-local.yml config to take precedence over everything else. Even more so because of this comment:

lefthook/internal/config/load.go

Lines 97 to 102 in 8959135

	// mergeAll merges configs using the following order.
	// - lefthook/.lefthook
	// - files from `extends`
	// - files from `remotes`
	// - lefthook-local/.lefthook-local.
	func mergeAll(fs afero.Fs, repo *git.Repository) (*viper.Viper, error) {

Actual results

Options from the extends files will take precedence.

Possible Solution

A potential fix could involve modifying the merge process to ensure that extends files do not get reapplied. This could be achieved by filtering out previously applied extends files during the merging process to prevent them from being in the list when extends is called after merging each config source:

• Global Config Extend
• Remote Config Extend
• Local Config Extend

[mrexox]

Hey! Thank you for the issue. I have prepared the fix: #754. Now extends option will be merged for the local config only if it differs from global extends.

[mrexox]

Please, check out 1.6.17 for the fix

[ardelato]

@mrexox That fix worked out!

But oh man I didn't expect this turn around speed! Thank you very much @mrexox 🙇

[ardelato]

@mrexox I am sorry to say but the recent fix may have exposed a different bug.

The bug may just be our fault for how we implemented the lefthook configs.

For context, we currently have a lefthook-config package which neatly houses all our commands and scripts. We were partly inspired by Nozomiishii's configuration approach here - https://github.com/nozomiishii/configs/tree/592d241fdc5fb6750b03fbca9e95c8f15e31e9f5/packages/lefthook-config

The package's lefthook.yml file extends all the commands. We then extend this package's lefthook.yml in our global lefthook.yml. Overall, we have a set of nested extends at least 2 levels deep.

The issue is that the second level extends is no longer applied. Before the change, the code would still enter this block, even if there was no lefthook-local.yml file, and thus execute the extends function. As such, the second level extend commands would be applied.

lefthook/internal/config/load.go

Lines 116 to 118 in 8959135

	if err := mergeOne([]string{"lefthook-local", ".lefthook-local"}, "", extends); err == nil {
	if err = extend(extends, repo.RootPath); err != nil {
	return nil, err

---

Overall, was lefthook's extend option meant to be nested like this?

[mrexox]

@ardelato , thank you for providing a great example of the configuration setup! I never thought of such cases with multiple extends levels. Originally it was supposed to have only one level of extends. I think it might be useful to allow as many levels as possible but it may require some time to implement this feature.

If you used to share the configuration I can suggest you the lefthook's remotes feature. You can specify multiple configs for a single remote which might cover your use case.

I will try to implement recursive merging of extends but I think it may take some time. I'll let you know if I find a blocker.

[mrexox]

Hey @ardelato! It was easier than I thought. I will prepare a new version with this fix soon

[ardelato]

Hot damn! I would have been just fine if this just went on your radar but to address it so quickly makes me really appreciate this repo and the work you guys are doing 🙇.

---

That said, the new version does seem to have fixed both issues now 🙌. Thank you @mrexox.