| Age | Commit message (Collapse) | Author |
|---|
|
Disallow use of `os.Setenv()` and `os.Unsetenv()` in tests. Callers
should instead use `testhelper.ModifyEnvironment()`. Adjust existing
callers to do so. Note: the log tests cannot use the testhelper package
due to a cyclic import and thus use `t.Setenv()` directly.
|
|
Remove an unused golangci-lint linting exemption.
|
|
There's several locations where we don't verify that the `Kill()`
syscall succeeded. Move the linting exceptions we have in our
golangci-lint configuration inline to clearly indicate that this is
something we may want to fix in the future.
|
|
Both the local and SQL electors don't check errors returned by
`checkNodes()`. While we could convert all sites to properly do this, it
doesn't really feel worth it: both electors only exist due to legacy
reasons and should not be used in production systems at all.
Ignore most of those errors inline so that we can remove the excemptions
from golangci-lint's config file.
|
|
We currently ignore any errors when waiting for the supervised command
fails. Log the error so that we at least have a chance to see that
something may have gone wrong.
|
|
Our testhelper context has recently gained the ability to verify that
feature flags are exercised as expected. This is an important safety
guard which can help us to avoid releasing untested and thus potentially
buggy code.
Disallow usage of "normal" contexts which don't have this ability such
that we do not introduce their usage by accident.
|
|
Much of the flakiness we're seeing every day is caused by timeouts which
were too tight. This is only natural: our own developer machines are
much beefier than the CI runners we use, and furthermore they typically
don't suffer from noisy neighbours. So any timeout that works for our
machine is likely to not work for CI workers. It is questionable why
we'd want to have timeouts in the first place: most tests really
shouldn't verify how fast a certain operation is, but instead they
should verify that it does what we expect it to do. Otherwise, if one
cares about speed, one should write a benchmark instead.
One valid reason is to detect tests which are stuck completely, but the
Go runtime handles this for us already with the default 10 minute
timeout for tests. Other usecases which want to test for example
cancellation should really try hard to avoid using timeouts for this,
but instead the system under test should be designed in a way that makes
it possible to easily test it, for example by using manual tickers which
can be injected from a test.
Forbid usage of a subset of functions which we historically used for
creating contexts with timeouts. Most of the tests which used this have
been refactored to not do so anymore, with the exception of some tests
which explicitly want to exercise deadlines themselves.
This is only a first step: we also want to discourage the use of
`time.After()` and `time.Sleep()`. These are left for a different commit
series though.
|
|
While gofumpt will by default check for possible code simplifications
when run directly, its corresponding linter for golangci-lint doesn't
seem to do so. As a result, linting may not complain about misformatted
code while running the formatter would still perform reformatting.
Fix this by using both `gofmt` and `gofumpt` linters: `gofmt` by default
has simplifications enabled, and `gofumpt` will verify the extra rules
which are on top of `gofmt`.
|
|
By default, stylecheck checks for missing package comments. Given that
we don't really use these, this of course generates a lot of linting
violations for us. Because of this, we have added an exclude rule to
just skip over those violations completely. Excludes aren't the correct
way to go about this, though: the stylecheck linter can be configured to
skip specific checks, which is what we should be doing instead.
Convert the exclude rule to stylecheck configuration.
|
|
Linters and their settings are torn apart by our exclude rules. Let's
move them together to make the settings easier to discover.
|
|
Most of the linting exception we have are about missing documentation of
public variables or comments. Let's inline all of these -- it's hard to
keep track of exceptions and keep the list up-to-date. By having a
`//nolint` directive at the site where the document is missing it
becomes a breeze to directly clean it up. Furthermore, it also acts as a
reminder that one might want to add a comment when one stumbles over any
of these comments.
|
|
Clean up various exceptions for the errcheck linter, either by removing
exceptions which don't apply anymore, by handling the errors or by
ignoring errors.
|
|
The noctx linter labels code which should use a context but doesn't.
We've got a single location in our tests which violate this linter,
which is why we have an exception for this linter in place.
Fix this callsite and remove the exception.
|
|
Remove the unused exception for the "maligned" linter.
|
|
The "old_path" and "new_path" fields of the GetRawChanges responses have
been deprecated ever since we have moved the protobuf definitions into
our repo in b9c9aec5c (Start preparation for migrating .proto files,
2019-07-05). They had originally been deprecated given that they were
using strings as types, and invalid Unicode characters in strings get
dropped when serializing and deserializing Protobuf messages. They had
thus been replaced with two fields "old_path_bytes" and "new_path_bytes"
and downstream callers have long been converted to use these.
Let's drop the old and deprecated fields -- they're not used by
anything, and neither should they ever be used.
|
|
We've added a special exclude rule for the govet linter such that it
doesn't bug us about use of unkeyed fields in composite literals. We
only have two callsites which get this wrong though, and using field
names is preferable such that we don't by mistake assign a value to the
wrong field.
Reenable this linter and fix the two violations.
|
|
With golangci-lint v1.43, a new bidichk linter has been added. This
linter checks sources for bidirectional control characters which can be
abused to conceal adversary code behind seemingly benign code. This
issue (CVE-2021-42694, Trojan Source [1]), makes it easy for an
adversary to get changes through code review.
Enable the bidichk linter to detect any use of such characters to
protect us against such supply chain attacks.
[1]: https://trojansource.codes/
|
|
The "io/ioutil" package has been deprecated in Go 1.16 and thus
shouldn't be used in our codebase anymore. Disallow its usage by adding
the "depguard" linter with a single entry for this specific package.
Note that we still use the package in some places where we used to rely
on `ioutil.ReadDir()`'s old behaviour of stat'ting all directory
entries. Conversion of these is not part of this patch series given that
it would be a change in behaviour compared to all the other changes
which aren't. Those sites are thus annotated with `nolint` directives.
|
|
Add the gofumpt linting rule to golangci. This allows us to replace our
"check-formatting" Makefile target with a run of golangci-lint.
|
|
The golint linter has been deprecated by upstream and is thus also
marked as deprecated by golangci-lint. Replace it with the newer revive
linter, which has a superset of linters of golint.
|
|
The change removes out.Flush from lint rules exclusion
and fixes all the places where it causes the issue.
It affects only gitaly-debug binary, so it has no impact
on the production code.
The change adds a couple of helper functions that now used
in places where error were not checked, but should be.
|
|
The change removes os.Remove from lint rules exclusion
and fixes all the places where it causes the issue.
If removal ends up with an error we try to log it if the
logger is accessible. Otherwise the error is just omitted.
We don't return it back to the caller because we don't want
functional changes here.
|
|
The change removes os.RemoveAll from lint rules exclusion
and fixes all the places where it causes the issue.
If removal ends up with an error we try to log it if the
logger is accessible. Otherwise the error is just omitted.
We don't return it back to the caller because we don't want
functional changes here.
|
|
The change removes io.Copy from lint rules exclusion
and fixes all the places where it causes the issue.
|
|
The dependency github.com/golang/protobuf is deprecated and should
be replaced with google.golang.org/protobuf.
This change replaces usage of the ptypes package. The replacement
packages has a different structure and it requires code changes
to be done to migrate.
The exclusion rules removed from the golanci-lint tool configuration
file to prevent future use of the deprecated package.
|
|
The dependency github.com/golang/protobuf is deprecated and should
be replaced with google.golang.org/protobuf.
This change replaces usage of the proto package. The packages have
incompatible API that is why the code changes were done to migrate.
The exclusion rule removed from the golanci-lint tool configuration
file to prevent future use of the deprecated package.
|
|
The dependency github.com/golang/protobuf is deprecated and should
be replaced with google.golang.org/protobuf.
This change replaces usage of the jsonpb package with encoding/protojson.
The packages have incompatible API that is why the code changes
were done to migrate.
The exclusion rule removed from the golanci-lint tool configuration file
to prevent future use of the deprecated package.
|
|
Except for a single use in Praefec's tests, the "storage" module is only
used by Gitaly. Despite that, it's still located at `internal/storage`
and thus indicates that it may be a generic component for reuse.
Move it into `internal/gitaly` to clarify that it is indeed an
implementation detail of Gitaly, only.
|
|
We're about to extend dependencies required to run gitaly-git2go
subcommands, which will make calling them tedious. This commit thus
refactors the revert subcommand to be implemented on top of the
`Executor` structure, which encapsulates all required dependencies and
thus avoids the additional complexity.
|
|
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.
|
