| Age | Commit message (Collapse) | Author |
|---|
|
Convert the tests for UserRebaseConfirmable to not use a worktree
anymore.
|
|
Remove the handcrafted version of `gittest.ResolveRevision()` in favor
of this new helper function. While at it, drop mentions of `SHA`: this
is an implementation detail, and for all it's worth we should just call
this an "object ID".
|
|
Adjust names of tests for the UserRebaseConfirmable RPC to match our
best practices.
|
|
Convert the test that demonstrates squashing with commits that contain a
rename and modification of the renamed file to not use a worktree.
|
|
Fix the test for squashing commits with renames to indeed squash the
correct commit range so that the rename is actually visible.
|
|
One of our tests tries to verify that we can squash a commit when there
was a rename on the target branch. This test is silently broken though
because of the way we set up the commits: while we do rename the changed
file, the commit containing that change is not used to compute the
squash.
Demonstrate this brokenness by asserting the tree's contents. As can be
seen, the resulting tree has the changed content but is still using the
original filename.
|
|
One of our `FindPage()` tests uses a worktree so that it can amend the
latest commit creating a Wiki page to have author information including
UTF8-encoded characters. Convert the test to not use a worktree by
amending the `createWikiPageOpts` to allow for specifying an author.
|
|
One of our tests for `ListRefs()` is setting up a worktree in order to
write a set of commits that can then be used as targets for references.
Convert this to instead use `gittest.WriteCommit()` to get rid of the
worktree.
|
|
There aren't any users left anymore which create repositories via our
gittest helpers and `WithWorktree` enabled, and furthermore it is
discouraged to do so because it creates on-disk state that wouldn't ever
exist in a production setup.
Remove the ability to create repositories with worktrees via the gittest
helpers.
|
|
Convert the `GetArchive()` test that verifies we cannot inject arguments
into git-archive(1) to not use a worktree anymore.
|
|
It seemingly used to be possible to inject arguments to `GetArchive()`
via the path parameter so that an adversary could overwrite arbitrary
locations on disk. The test we have that verifies that this cannot
happen is a tad complex and not really obvious with regards to what's
happening.
Refactor the test and add documentation to simplify it. This is done in
preparation for a change where we stop using a worktree.
|
|
In order to list compressed archive's contents we're currently first
writing the archive to disk and then pass it to a helper function that
executes a binary depending on what archive format is used. The first
step to write the contents to disk is handled by the caller, which is a
bit unwieldy.
Refactor the helper function to write the file to disk itself in order
to prepare for a second caller of it. Ideally, we'd not write the file
to disk at all but instead just use the `archive` Go package to
enumerate contents. But that refactoring is left for another day.
|
|
Adjust names for the `GetArchive()` RPC's tests to match our modern best
practices.
|
|
The use of worktrees in our tests is discouraged in favor of using test
helpers like `git.WriteCommit()` to set up required test data. Convert
one of our tests for `SearchFilesByContent()` to do so.
|
|
The use of worktrees in our tests is discouraged in favor of using test
helpers like `git.WriteCommit()` to set up required test data. One of
our tests for `RawPatch()` is using a worktree though to verify that the
generated diff can in fact be reapplied to a repository to round-trip to
the same tree again, and git-apply(1) requires a worktree to apply
patches.
Convert the test to not use a worktree anymore by instead using the
`gitaly-git2go apply` command to apply the patch, which doesn't require
a worktree.
|
|
The use of worktrees in our tests is discouraged in favor of using test
helpers like `git.WriteCommit()` to set up required test data. One of
our tests for `RawDiff()` is using a worktree though to verify that the
generated diff can in fact be reapplied to a repository to round-trip to
the same tree again, and git-apply(1) requires a worktree to apply
patches.
Convert the test by instead using `gitaly-git2go apply` to apply the
patch. Note that we're forced to use a different set of commits to do
this test though: the current difference contains changes to binary
files, and these cannot be handled by `gitaly-git2go`.
|
|
Improve names for `RawDiff()` and `RawPatch()` tests to match our
current best practices. While at it, improve parallelization of tests by
adding missing calls to `t.Parallel()`.
|
|
Add helper function to resolve revisions to an object ID, which is a
frequently used pattern in our tests.
|
|
Add a bunch of options to allow overriding the author and committer name
and date when writing commits via `gittest.WriteCommit()`.
|
|
go: Update Postgres dependencies
See merge request gitlab-org/gitaly!4647
|
|
ssh: Extend tests to emulate client-side failure conditions with sidechannel
See merge request gitlab-org/gitaly!4615
|
|
repository: Always use long-running LFS filter process in GetArchive
Closes #4282
See merge request gitlab-org/gitaly!4661
|
|
operations: Always enable structured errors in UserDeleteBranch
Closes #4286
See merge request gitlab-org/gitaly!4660
|
|
The command package is not only used to spawn Git commands, but may also
spawn any other arbitrary command. It's thus quite misleading that some
of the error messages are mentioninng "GitCommand"s as context.
Reword these errors to be command-agnostic. While at it, let's also use
the `%w` formatter to chain errors together.
|
|
The read monitor is implemented via a Unix pipe, which means that its
file descriptors need to always be closed by us or otherwise we have the
potential for a resource leak. This is done by spawning a Goroutine that
waits for the context to be cancelled. Unfortunately, when the context
is cancelled when spawning a command that is about to use these file
descriptors then we now run into a race: `exec.Cmd.Start()` may just be
about to duplicate those file descriptors to pass them to the child
process while we're in the process of closing them.
Fix this race by cleaning them up inside of the command's finalizer
instead. Like this we ensure that the descriptors are only closed after
we have already reaped the child process.
Changelog: fixed
|
|
We have cases where we need to clean up resources as soon as a command
has finished. Add the ability to set up a finalizer that runs exactly
once after the command has been reaped to make it easier to implement
this in a race-free manner.
|
|
When spawning commands, callers can afterwards add these commands to a
Cgroup and then call `SetCgroupPath()` to add the path to log messages.
But because we spawn a Goroutine that creates these logs as soon as the
context is done, this creates a race between setting the Cgroup path and
context cancellation.
Fix this race by handling addition to the Cgroups manager in the command
package itself. Like this, we can set up the Cgroup path before starting
the Goroutine and thus fix the race.
Changelog: fixed
|
|
When spawning commands, callers can afterwards call `SetMetricsCmd()`
and `SetMetricsSubCmd()` to have the command be properly represented in
Prometheus metrics via its command and subcommand. But because we spawn
a Goroutine that logs these metrics as soon as the context is done, this
creates a race between setting those names and context cancellation.
Fix this race by converting these functions into options that can be
passed to `New()` so that they're set up correctly at construction time.
Changelog: fixed
|
|
The `StdinSentinel` value can be passed to `WithStdin()` in order to
tell the command package to set up the command for `Write()`ing to it.
Replace this with a new option `WithSetupStdin()` so that we can avoid
the use of sentinel values.
|
|
Now that the command package accepts options, it becomes easier for us
to pass standard streams to `New()` by just assembling them into an
array of `command.Option`s, as well.
Refactor the code to do so.
|
|
We're about to extend construction of commands so that all members of
the resulting structure will be set at construction time in order to fix
racy access patterns. For this we will need to pass more parameters to
`New()` so that it has all information available at construction time.
Right now the function isn't all that extensive though given that it
only accepts a fixed set of parameters.
Refactor `New()` to instead accept a variable number of `Option`s. As a
first step, convert all parameters currently passed to it to instead use
these new options.
|
|
Modernize tests of the command package to follow our current code style.
|
|
Reorder the code so that all functions belonging to the `Command` type
are grouped together.
|
|
Add several tests that exercise how SSHUploadPackWithSidechannel behaves
in case the client-side fails in unexpected ways.
|
|
In order to be able to monitor what git-upload-pack(1) is communicating
with the user we create a pipe so that we can intercept the data. And
while make sure to close the reading-side of the pipe in all cases, we
in fact don't close the writing side when spawning git-upload-pack(1)
fails.
Fix this potential resource leak by immediately deferring the close of
the writing side.
|
|
localrepo: Speed up calculating size for repo with excluded alternates
See merge request gitlab-org/gitaly!4657
|
|
go: Update module github.com/google/uuid to v1.3.0
See merge request gitlab-org/gitaly!4648
|
|
go: Update module github.com/pelletier/go-toml to v1.9.5
See merge request gitlab-org/gitaly!4650
|
|
In 0603c758d (repository: Use long-running filter process for converting
LFS pointers, 2022-05-31), we have introduced support for the long-lived
process protocol so that we only require a single `gitaly-lfs-smudge`
to convert multiple LFS pointers instead of one per pointer. This has
been wired up for `GetArchive()` to speed up this RPC in large repos
with many pointers.
This change was rolled out on June 13th without any issues having been
observed so far. So let's remove the feature flag so that we always use
the long-running filter process protocol.
|
|
The `PreReceiveError` field in the `UserDeleteBranchResponse` is never
set anymore in favor of the new structured errors. Mark the field as
deprecated.
|
|
With 30d4fd427 (operations: Use structured errors in UserDeleteBranch,
2022-06-07), we have introduced structured errors in UserDeletBranch.
The intent is to both fix silent errors in case we failed to delete the
branch, and to fix replication in case no transactional votes were cast
in the error case.
The change has been rolled out on June 14th and is part of v15.1. So
let's remove the feature flag guarding it.
Changelog: fixed
|
|
Optimize ChompBytes() function
See merge request gitlab-org/gitaly!4659
|
|
The github.com/pelletier/go-toml package is updated to 1.9.5, and this
requires us to update NOTICE.
|
|
No need to allocate an intermediate string. Trim suffix first, then turn the result into a string.
|
|
When calculating a repository's size, we optionally allow the caller to
exclude the size of any object pools the repository is connected to.
This causes us to add `--not --alternate-refs` to the git-rev-list(1)
command, which will thus exclude all objects from disk usage calculation
that are reachable by the alternate.
As it turns out though, we're hitting a performance edge case: we ask
git-rev-list(1) to use bitmaps to calculate the size, but in the case of
a pooled repository only the object pool itself will have a bitmap. This
means that by definition, the bitmap can only contain objects that we
wish to exclude from the disk calculations anyway. All objects that are
not reachable by the pool are thus known to not be contained in any
bitmap. Because of this using bitmaps is extremely inefficient as shown
by the following benchmark, which is performed in `gitlab-org/gitlab`:
Benchmark 1: git rev-list --all --objects --disk-usage
Time (mean ± σ): 13.290 s ± 0.085 s [User: 13.023 s, System: 0.255 s]
Range (min … max): 13.160 s … 13.355 s 5 runs
Benchmark 2: git rev-list --all --objects --disk-usage --use-bitmap-index
Time (mean ± σ): 3.588 s ± 0.016 s [User: 3.326 s, System: 0.259 s]
Range (min … max): 3.576 s … 3.616 s 5 runs
Benchmark 3: git rev-list --not --alternate-refs --not --all --objects --disk-usage
Time (mean ± σ): 6.828 s ± 0.056 s [User: 6.601 s, System: 0.363 s]
Range (min … max): 6.761 s … 6.897 s 5 runs
Benchmark 4: git rev-list --not --alternate-refs --not --all --objects --disk-usage --use-bitmap-index
Time (mean ± σ): 68.105 s ± 0.383 s [User: 67.471 s, System: 0.744 s]
Range (min … max): 67.663 s … 68.509 s 5 runs
Summary
'git rev-list --all --objects --disk-usage --use-bitmap-index' ran
1.90 ± 0.02 times faster than 'git rev-list --not --alternate-refs --not --all --objects --disk-usage'
3.70 ± 0.03 times faster than 'git rev-list --all --objects --disk-usage'
18.98 ± 0.14 times faster than 'git rev-list --not --alternate-refs --not --all --objects --disk-usage --use-bitmap-index'
As you can see in benchmark #1 and #2, bitmaps speed up disk usage
calculations when not using alternate references. But the use of bitmaps
severely degrades performance by almost a factor of 10 as soon as we use
them in combination with `--alternate-refs` as shown in #4. On the other
hand, when we disable the use of bitmaps with alternate refs we are only
about twice as slow as compared to not iterating over alternate refs.
Interestingly, we never hit this issue in production until recently.
This is because of a configuration issue we have had in production: we
unconditionally set `core.alternateRefsCommand=exit 0 #`, which causes
us to skip over any alternate refs even when explicitly asking for them
via `--alternate-refs`. This is definitely unintentional as it causes us
to not honor the case where the client asks for shared objects to be
excluded from the size calculations. With a recent change though we
fixed this issue and started to correctly iterate over alterante refs
again, but that resulted in a 20-fold increase in latency for the
`RepositorySize()` RPC. So we're currently living in a world where
`RepostiorySize()` is either broken, or where it has significant issues
with performance.
Mitigate the performance hit by not using bitmaps when the client asks
tor alternate references to be excluded only in case the repository has
an object pool. As shown by the benchmark, this should result in a 10x
speedup compared to using bitmaps for repositories with many refs.
Changelog: fixed
|
|
Convert the tests for calculating the repository size for a repository
with alternates to be table-driven instead of using a standalone test.
This will our increase test coverage when we start verifying command
line parameters passed to git-rev-list(1).
|
|
gitaly-hooks: Update README.md
See merge request gitlab-org/gitaly!4616
|
|
Replace deprecated WithInsecure with WithTransportCredentials
See merge request gitlab-org/gitaly!4658
|
|
Clarify the story of how `gitaly-hooks` is invoked through being
symlinked from `post-receive`, `pre-receive`, `update`,
`reference-transaction`.
Add section on `reference-transaction` hook.
Update some outdated code references.
|
|
gRPC has deprecated the WithInsecure option in favor of the more
general WithTransportCredentials combined with insecure.NewCredentials.
This commit removes our usage of the deprecated option so we don't get
lint failures for using deprecated functionality when upgrading our
gRPC dependency.
|
