diff options
author | Pavlo Strokov <pstrokov@gitlab.com> | 2021-03-15 15:10:38 +0300 |
---|---|---|
committer | Pavlo Strokov <pstrokov@gitlab.com> | 2021-03-18 12:49:31 +0300 |
commit | 63c55949d54543fce205c0f750f8fc502ed9566a (patch) | |
tree | a26b220fde2aea0495789d584b35432bc6212751 /internal/gitaly/linguist | |
parent | 2093a05fa4214e69abc75eacea26e926266264b1 (diff) |
Linguist converted into an instantiation dependency
Implementation of the linguist package relies on the
package-private state: the use of the colorMap variable.
It is filled with a hook mechanism after validation of the
gitaly configuration. The problem is that this operation
invokes ruby code to get it done and this call is pretty
expensive. It also doesn't allow us to convert tests to
parallel as it causes race conditions in attempt to fill
the global variable.
The solution to this problem is creation of the struct
instance that encapsulates the state and can be instantiated
only when needed. The change also includes minification in
generating test configuration that does not require to make
a ruby call and uses a stub file for initialisation. The
behaviour could be returned back to the normal with usage of
the WithActualLinguist option for configuration creation.
The change also includes renames of the test functions for
the linguist package. The change speeds up execution of the
tests: before 0m43.907s and after 0m21.659s on my local.
Part of: https://gitlab.com/gitlab-org/gitaly/-/issues/2699
Diffstat (limited to 'internal/gitaly/linguist')
-rw-r--r-- | internal/gitaly/linguist/linguist.go | 47 | ||||
-rw-r--r-- | internal/gitaly/linguist/linguist_test.go | 30 |
2 files changed, 43 insertions, 34 deletions
diff --git a/internal/gitaly/linguist/linguist.go b/internal/gitaly/linguist/linguist.go index 9356a8af1..8cb187569 100644 --- a/internal/gitaly/linguist/linguist.go +++ b/internal/gitaly/linguist/linguist.go @@ -15,13 +15,8 @@ import ( "gitlab.com/gitlab-org/gitaly/internal/gitaly/config" ) -func init() { - config.RegisterHook(LoadColors) -} - var ( exportedEnvVars = []string{"HOME", "PATH", "GEM_HOME", "BUNDLE_PATH", "BUNDLE_APP_CONFIG"} - colorMap = make(map[string]Language) ) // Language is used to parse Linguist's language.json file. @@ -32,8 +27,31 @@ type Language struct { // ByteCountPerLanguage represents a counter value (bytes) per language. type ByteCountPerLanguage map[string]uint64 +// Instance is a holder of the defined in the system language settings. +type Instance struct { + colorMap map[string]Language +} + +// New loads the name->color map from the Linguist gem and returns initialised instance +// to use back to the caller or an error. +func New(cfg config.Cfg) (*Instance, error) { + jsonReader, err := openLanguagesJSON(cfg) + if err != nil { + return nil, err + } + defer jsonReader.Close() + + var inst Instance + + if err := json.NewDecoder(jsonReader).Decode(&inst.colorMap); err != nil { + return nil, err + } + + return &inst, nil +} + // Stats returns the repository's language stats as reported by 'git-linguist'. -func Stats(ctx context.Context, cfg config.Cfg, repoPath string, commitID string) (ByteCountPerLanguage, error) { +func (inst *Instance) Stats(ctx context.Context, cfg config.Cfg, repoPath string, commitID string) (ByteCountPerLanguage, error) { cmd, err := startGitLinguist(ctx, cfg, repoPath, commitID, "stats") if err != nil { return nil, err @@ -53,8 +71,8 @@ func Stats(ctx context.Context, cfg config.Cfg, repoPath string, commitID string } // Color returns the color Linguist has assigned to language. -func Color(language string) string { - if color := colorMap[language].Color; color != "" { +func (inst *Instance) Color(language string) string { + if color := inst.colorMap[language].Color; color != "" { return color } @@ -62,17 +80,6 @@ func Color(language string) string { return fmt.Sprintf("#%x", colorSha[0:3]) } -// LoadColors loads the name->color map from the Linguist gem. -func LoadColors(cfg *config.Cfg) error { - jsonReader, err := openLanguagesJSON(cfg) - if err != nil { - return err - } - defer jsonReader.Close() - - return json.NewDecoder(jsonReader).Decode(&colorMap) -} - func startGitLinguist(ctx context.Context, cfg config.Cfg, repoPath string, commitID string, linguistCommand string) (*command.Command, error) { bundle, err := exec.LookPath("bundle") if err != nil { @@ -115,7 +122,7 @@ func startGitLinguist(ctx context.Context, cfg config.Cfg, repoPath string, comm return internalCmd, nil } -func openLanguagesJSON(cfg *config.Cfg) (io.ReadCloser, error) { +func openLanguagesJSON(cfg config.Cfg) (io.ReadCloser, error) { if jsonPath := cfg.Ruby.LinguistLanguagesPath; jsonPath != "" { // This is a fallback for environments where dynamic discovery of the // linguist path via Bundler is not working for some reason, for example diff --git a/internal/gitaly/linguist/linguist_test.go b/internal/gitaly/linguist/linguist_test.go index 58f10e81a..6188204d0 100644 --- a/internal/gitaly/linguist/linguist_test.go +++ b/internal/gitaly/linguist/linguist_test.go @@ -7,6 +7,7 @@ import ( "testing" "github.com/stretchr/testify/require" + "gitlab.com/gitlab-org/gitaly/internal/gitaly/config" "gitlab.com/gitlab-org/gitaly/internal/testhelper" "gitlab.com/gitlab-org/gitaly/internal/testhelper/testcfg" ) @@ -22,40 +23,41 @@ func testMain(m *testing.M) int { return m.Run() } -func TestStatsUnmarshalJSONError(t *testing.T) { +func TestInstance_Stats_unmarshalJSONError(t *testing.T) { cfg := testcfg.Build(t) ctx, cancel := testhelper.Context() defer cancel() + ling, err := New(cfg) + require.NoError(t, err) + // When an error occurs, this used to trigger JSON marshelling of a plain string // the new behaviour shouldn't do that, and return an command error - _, err := Stats(ctx, cfg, "/var/empty", "deadbeef") + _, err = ling.Stats(ctx, cfg, "/var/empty", "deadbeef") require.Error(t, err) _, ok := err.(*json.SyntaxError) require.False(t, ok, "expected the error not be a json Syntax Error") } -func TestLoadLanguages(t *testing.T) { - cfg := testcfg.Build(t) +func TestNew(t *testing.T) { + cfg := testcfg.Build(t, testcfg.WithRealLinguist()) - colorMap = make(map[string]Language) - require.NoError(t, LoadColors(&cfg), "load colors") + ling, err := New(cfg) + require.NoError(t, err) - require.Equal(t, "#701516", Color("Ruby"), "color value for 'Ruby'") + require.Equal(t, "#701516", ling.Color("Ruby"), "color value for 'Ruby'") } -func TestLoadLanguagesCustomPath(t *testing.T) { - cfg := testcfg.Build(t) - +func TestNew_loadLanguagesCustomPath(t *testing.T) { jsonPath, err := filepath.Abs("testdata/fake-languages.json") require.NoError(t, err) - cfg.Ruby.LinguistLanguagesPath = jsonPath + cfg := testcfg.Build(t, testcfg.WithBase(config.Cfg{Ruby: config.Ruby{LinguistLanguagesPath: jsonPath}})) - colorMap = make(map[string]Language) - require.NoError(t, LoadColors(&cfg), "load colors") + ling, err := New(cfg) + require.NoError(t, err) - require.Equal(t, "foo color", Color("FooBar")) + require.Equal(t, "foo color", ling.Color("FooBar")) } |