| Age | Commit message (Collapse) | Author |
|---|
|
We're about to extend dependencies required to run gitaly-git2go
subcommands, which will make calling them tedious. This commit thus
refactors the conflicts subcommand to be implemented on top of the
`Executor` structure, which encapsulates all required dependencies and
thus avoids the additional complexity.
|
|
The OutgoingCtxWithRubyFeatureFlags function is missing documentation.
Add it and remove the corresponding linter exemption.
|
|
Implement receiver function `IsEnabled()` and `IsDisabled()` on the
FeatureFlag structure. These new functions will replace the old
interface of `featureflag.IsEnabled(ctx, featureflag.MyFeatureFlag)`,
which stutters.
|
|
|
|
Pick up https://github.com/grpc/grpc-go/pull/4205
Changelog: fixed
|
|
The "analyzehttp.go" file's most prominent member is the `Clone`
structure. Let's rename the file to "clone.go" to better match its
contents. This requires us to fix one last missing comment on the
`Clone` structure.
|
|
The `Post` structure hosts statistics about a POST request against
git-upload-pack(1). Without knowing details about how git-upload-pack(1)
works though, one wouldn't be able to tell what it actually represents:
everything related to fetching the packfile.
Let's rename the structure to `HTTPFetchPack`, which more clearly labels
what it actually is about. While at it, this also refactors the way it's
constructed: instead of being constructed via the `Clone` structure,
it's now a standalone structure and can be constructed via
`performFetchPack()`. This disentangles concerns and makes it
easier to reason about the code.
|
|
The `Get` structure hosts statistics about a GET request against
git-upload-pack(1). Without knowing details about how git-upload-pack(1)
works though, one wouldn't be able to tell what it actually represents:
everything related to the reference discovery.
Let's rename the structure to `HTTPReferenceDiscovery`, which more
clearly labels what it actually is about. While at it, this also
refactors the way it's constructed: instead of being constructed via the
`Clone` structure, it's now a standalone structure and can be
constructed via `performReferenceDiscovery()`. This disentangles
concerns and makes it easier to reason about the code.
|
|
When doing a POST to git-upload-pack(1), then we send along a set of
"want"s which tell the remote side which references we want to receive.
So while this depends on us knowing which references exist in the first
place, it still is inarguably a property of the POST and not of the GET
which discovers available refs.
Let's make the relation more explicit by 1. moving wants into the `Post`
structure and 2. accepting announced references as parameter when doing
the post.
|
|
The gitaly-debug executable can be used to clone from a remote
repository and print several statistics about the clone itself, like for
example how long several stages took. Because of this, the stats package
supports an "interactive" flag which causes it to print various events
to stdout: only if it's set to true will `printInteractive()` actually
print anything.
One code smell we have right now is that `printInteractive()` returns an
error, but it's never actually checked. And we probably don't even want
to: it's only used for gitaly-debug to print to stdout, so it's harmless
enough if it would fail.
Get rid of the error code and just ignore when `fmt.Printf()` fails and
remove the golang-ci exemption.
|
|
The blackbox code is lacking some documentation on public symbols. Add
it as required and remove the golang-ci exemptions.
|
|
|
|
Add a missing comment for `PraefectServer`'s `Address()` receiver and
drop the corresponding golangci-lint rule.
|
|
Many of our components use both the `Collect()` and `Describe()`
functions of Prometheus metrics in order to encapsulate metrics part of
a different struct and expose them via a simple combined interface. Our
MockHistogramVec type cannot be used in such contexts though because it
does not implement these functions.
Extend both our `HistogramVec` interface and the `MockHistogramVec`
implementation to fix this. While at it, document the exposed types and
functions.
|
|
commit: Handle real errors when skipping commits failed
See merge request gitlab-org/gitaly!3392
|
|
When using loop variables, then one must take care to not store pointers
to these loop variables without rescoping them. The exportloopref linter
tries to find such invalid cases where the rescoping wasn't done.
Enable the linter.
|
|
It's a common pitfall to create a slice with a prespecified length and
then append to it. If the initial length hasn't been set to zero, then
this pattern causes the first `n` entries to be the zero value of its
type. The makezero linter finds such patterns.
Enable the linter and fix the single violation it surfaces.
|
|
The wastedassign linter will check whether a newly assigned variable is
used in any code path after its assignment. This seems like a useful
check to have, e.g. to not forget checking a reassigned error value.
Enable the linter and fix the single violation it surfaces.
|
|
Upgrade golangci-lint to v1.39.0 and fix all newly reported errors. This
upgrade disables the maligned linter, which has been deprecated upstream
in favor of govet.
|
|
When the user sets the `Offset` field of a FindCommits request, then we
effectively paginate the answer and only start returning commits after
this offset. But skipping commits can fail because of two reasons:
either because the user-provided offset is too large and there aren't
so many commits for the given file, or because a real error happened.
But right now, we're not checking any errors at all here.
Ignoring the first case where we do not have enough commits to satisfy
the request is an expected case, and we handle it just fine right now by
returning EOF. But the case where reading commits fails is a real error,
and we should report it to the caller when it has failed instead of just
returning EOF.
Fix the issue by having `GetCommits.Offset()` either return the error of
its scanner or an EOF if there are no more commits. Like this, we can
now do proper error checking in `findCommits()` and discern both cases.
|
|
While `remoterepo.Repository` is the remote equivalent to
`localrepo.Repo`, both types have diverging naming. Rename the former to
`remoterepo.Repo` to align them.
|
|
|
|
After tests, we're running `glsql.Clean()` to remove any data which
we've written to the database during tests. We never verify that the
call actually succeeds though, which would thus hide any errors in case
this ever started to fail.
Fix the issue by logging an error in case the function fails and drop
the corresponding linter ignore rules.
|
|
We're currently missing error checks for various invocations of
`Write()` functions. Fix this issue by properly checking returned errors
and drop the corresponding linter ignore rule.
|
|
We're currently explicitly killing the spawned test process in
`TestStolenPid()`. Convert this to instead use `exec.CommandContext()`,
which will automatically kill the child process in case the context gets
canceled. This allows us to get rid of one more linter ignore rule.
|
|
Our tests are missing various error checks when writing pktline
formatted data. Fix this by adding a set of wrappers which do the error
checking for us and drop the corresponding linter ignore rules.
|
|
Our bootstrap tests are missing error checks for multiple function
invocations. Add them and drop the corresponding linter ignore rules.
|
|
The gitaly-lfs-smudge tests do not verify that `initLogging()` succeeds.
Fix the issue and remove the corresponding linter ignore rule.
|
|
The proxy tests are missing error checking for various GRPC functions,
which may easily hide unexpected errors. Fix the issue and remove
corresponding linter ignore rules.
|
|
Our tests do not chekc the error value of `pool.Remove()` when executed
as part of test cleanup. This may easily hide any unexpected side
effects if e.g. the pool didn't exist or was removed because of whatever
bug.
Fix the issue by checking the returned error and drop the corresponding
linter ignore rule.
|
|
If `GetInfoAttributes()` doesn't have to do anything because there is no
gitattributes file, then we simply bail out and send a successful
response. We do not verify that sending the response succeeds though,
which could lead to us silently swallowing the error.
Fix the issue by returning the error of `stream.Send()` and drop the
corresponding linter ignore rule.
|
|
In `testhelper.BuildCommit()`, we call `Insert()` on the treebuilder
without checking the returned error. Fix this and drop the corresponding
ignore rule for our linter.
|
|
The golang-ci linter configuration currently has a rule to ignore error
return values of `CloseWithError()`. This rule kind of make sense given
that the only location where we call it is on an `io.PipeWriter()`, and
the documentation of that function explicitly states that it "always
returns nil.".
Let's explicitly ignore the error at the callsite and document why we do
so. This allows us to get rid of the rule.
|
|
With the preceding commits, a subset of linting warnings have been
squelched which are currently excluded in our golangci configuration.
Let's drop these exclusions.
|
|
With the new RunGitalyServer function that starts full list
of gitaly services we don't need RunInternalGitalyServer helper
anymore. We remove it and all code related to it by replacing its
usage with the new func.
Part of: https://gitlab.com/gitlab-org/gitaly/-/issues/2699
|
|
Our test naming convention requires underscores in test names.
Golint reports this as a linting violation as camel case is generally
used in Go for names. This commit excludes the rule from being applied
to test files.
|
|
Fix a few lint issues added to the exclusions list in 6e7eea01e (Lint:
turn "errcheck" on by default, 2021-01-19).
|
|
We can turn this on by default by being a bit more permissive about
common cases in non-*_test.go files that should be OK not to check the
return value of most of the time (e.g. os.Remove()), and then having a
giant exclusions list for the rest.
|
|
Since we've got quite a common warning about things needing comments,
add a gigantic list of those exclusions. Generated with:
make lint |grep golint | sort
| perl -pe 's/^(.*?):\d+:\d\s*golint\s*(.*)/ - linters: \n - golint\n path: "$1"\n text: "$2"/g'
Except I then had to escape "(" in the text to "\\(" because they're
regexes. I suggested simply turning this specific warning off, but
others in today's team meeting wanted to keep it. So here it is as a
giant list.
|
|
Take a different approach than 20ef26439 (golangci: Enable error and
documentation linters, 2020-12-17) did and get rid of "new-from-rev"
entirely.
Instead we have config for "make lint-strict" which isn't passing yet,
and the main config for "make lint" which'll cause a CI failure.
After looking a bit into how "new-from-rev" works I think using it is
just a fundamentally bad idea in our case. All we want is to phase-in
new lints, which we can just do better like this with a new config.
To figure out what lines to complain about since "b7d42677f" it has to
run:
git diff --relative b7d42677f
Which currently takes ~200ms for me, but that relatively short time is
because it's ~300 commits ago.
If we take something ~3000 commits ago (early 2020, so not that long)
like "1e3d3131f" it's going to take ~3 seconds, and worse we're
starting to run into the default diff.renameLimit there, so the diff
will suddenly get much worse. So the whole thing is a ticking timebomb
in behavior & gradual slowdown waiting to happen.
|
|
In 0b414007c (golangci: Sort linters alphabetically, 2020-12-16) we
sorted the linters, but then right afterwards 0b14423fb (golangci:
Enable missing default linters, 2020-12-16) added "unused" out of the
sort order. Let's fix that.
|
|
Widen the regexp in my 224e7ecb9 (golangci: whitelist common test
patterns, 2020-12-24) to whitelist a few more things, as suggested in
https://gitlab.com/gitlab-org/gitaly/-/merge_requests/2962#note_475118299
I'm not just whitelisting any *.Close() as before since that's likely
to catch some false positives. Instead I'm focusing on the common
"defer" patterns.
Before this we'd emit 617 warnings, now we'll emit 603, of those
the *_test.go files went from 425 to 411, so this change only targets
the test suite.
|
|
Follow-up 149d56118 (golangci: fix linting issues on "master",
2020-12-24) and exempt common patterns in our tests that newly added
tests are very likely to run afoul of. See [1] for such a failure.
The common pattern is seen in the diff to tags_test.go here. The
standard boilerplate is to call newOperationClient()'s teardown like
that, and the context lint is warning about a widely used function
added in e87b76dac (featureset: Simplify execution with a new `Run()`
function, 2020-11-03).
Maybe we'd like to clean those up, but let's do that all in one MR,
not by prodding every author of a new test copying established pattern
to fix this for their new test.
Before this if the revision limitation in 20ef26439 (golangci: Enable
error and documentation linters, 2020-12-17) is removed we'd emit 1369
issues under "make lint", now it would be 831. Of those 831 we've got
431 emitted from *_test.go files.
Perhaps we'd like to be more permissive still, but by my skimming most
of the new ones make sense, e.g. warning about exec.Command[...].Run()
return values not being checked.
1. https://gitlab.com/gitlab-org/gitaly/-/pipelines/234007367/failures
|
|
gochecknoglobals linter errors out on our feature flag definitions.
This commit disables the linter until a solution is put to place or
the feature flags are annotated as nolint.
|
|
This commit enables a set of linters which the project currently doesn't
abide to in a lot of places:
- errcheck verifies that error returns are checked correctly.
- errorlint verifies that Go 1.13-style errors are handled correctly
by using the new `%w` formatter to wrap errors and using
`errors.Is()` to check them.
- golint verifies various things, but the most important one is that
it checks that each exported symbol has proper documentation.
Fixing all errors pointed out by those linters at once is infeasible, as
that'd require hundreds of changes altogether. But the reported problems
are useful regardless of that and may point out real errors and style
violations.
So let's enable those linters for now, but only for new changes. While
this in fact influences all linters, there are currently no problems
reported previous to this commit. So effectively, it doesn't make a
difference for those other linters.
|
|
The golangci-lint project has a set of default excludes for linters
which are applied by default. This means that we cannot choose ourselves
which linting messages we want to include, as some of them cannot be
controlled by us.
One noteworthy linting check which is excluded right now checks whether
comments for types and functions are prefixed with their respective
name. This is something which we try to enforce in Gitaly, too, but
naturally there's cases where it slips through review.
So let's disable the default excludes such that we can enable this
linting check. As it turns out, there's quite a lot of cases where we
failed to adhere to this style, which this commit also fixes. One thing
which we don't do though is to have per-package comments, so let's
actively ignore it for now.
|
|
The nolintlint linter asserts that the `nolint` instruction uses correct
syntax to really disable a warning. Let's enable it and fix one location
where we had a spurious `//nolint` comment.
|
|
The gci linter asserts that all imports are always sorted in the same
way. Let's enable it and fix the few locations where we don't abide with
this new policy.
|
|
The golangci-lint project has a list of linters [1] which are enabled by
default, which indicates that they're deemed useful enough to be always
enabled. As we're default-disabling all linters, we do not have those
enabled for Gitaly.
Given that they don't add any warnings, let's enable all defaul linters.
The only exception is the errcheck linter as we have quite some
locations where we don't perform error checking at all.
[1]: https://golangci-lint.run/usage/linters/
|
|
It's currently quite hard to compare the list of enabled linters with
the list of available linters as our list isn't sorted. This commit thus
sorts it alphabetically to make it easier to extend.
|
