diff options
87 files changed, 562 insertions, 1716 deletions
diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index b1ca532e7..98380b298 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -344,9 +344,6 @@ test:macos: variables: <<: *test_variables CACHE_PREFIX: !reference [.versions, macos] - # This is required to fix inconsistent deployment targets across CGo and - # libgit2. - MACOSX_DEPLOYMENT_TARGET: "12.0" PGDATA: /Volumes/RAMDisk/postgres PGHOST: localhost PGUSER: gitlab diff --git a/CHANGELOG.md b/CHANGELOG.md index 34f1d481d..03fc9a970 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,9 @@ # Gitaly changelog +## 16.3.2 (2023-09-05) + +No changes. + ## 16.3.1 (2023-08-31) No changes. @@ -67,7 +67,6 @@ GITALY_PACKAGE := $(shell go list -m 2>/dev/null || echo unknown) GITALY_VERSION := $(shell ${GIT} describe --match v* 2>/dev/null | sed 's/^v//' || cat ${SOURCE_DIR}/VERSION 2>/dev/null || echo unknown) GO_LDFLAGS := -X ${GITALY_PACKAGE}/internal/version.version=${GITALY_VERSION} SERVER_BUILD_TAGS := tracer_static,tracer_static_jaeger,tracer_static_stackdriver,continuous_profiler_stackdriver -GIT2GO_BUILD_TAGS := static,system_libgit2 # Temporary GNU build ID used as a placeholder value so that we can replace it # with our own one after binaries have been built. This is the ASCII encoding @@ -80,7 +79,6 @@ FIPS_MODE ?= ifdef FIPS_MODE SERVER_BUILD_TAGS := ${SERVER_BUILD_TAGS},fips - GIT2GO_BUILD_TAGS := ${GIT2GO_BUILD_TAGS},fips # Build Git with the OpenSSL backend for SHA256 in case FIPS-mode is # requested. Note that we explicitly don't do the same for SHA1: we @@ -175,41 +173,6 @@ ifdef GIT_FIPS_BUILD_OPTIONS GIT_BUILD_OPTIONS += ${GIT_FIPS_BUILD_OPTIONS} endif -# libgit2 target -# Git2Go and libgit2 may need to be updated in sync. Please refer to -# https://github.com/libgit2/git2go/#which-go-version-to-use for a -# compatibility matrix. -GIT2GO_VERSION ?= v34 -LIBGIT2_VERSION ?= v1.5.1 -LIBGIT2_REPO_URL ?= https://gitlab.com/libgit2/libgit2.git -LIBGIT2_SOURCE_DIR ?= ${DEPENDENCY_DIR}/libgit2/source -LIBGIT2_BUILD_DIR ?= ${DEPENDENCY_DIR}/libgit2/build -LIBGIT2_INSTALL_DIR ?= ${DEPENDENCY_DIR}/libgit2/install - -ifeq ($(origin LIBGIT2_BUILD_OPTIONS),undefined) - ## Build options for libgit2. - LIBGIT2_BUILD_OPTIONS ?= - LIBGIT2_BUILD_OPTIONS += -DBUILD_CLI=OFF - LIBGIT2_BUILD_OPTIONS += -DBUILD_TESTS=OFF - LIBGIT2_BUILD_OPTIONS += -DBUILD_SHARED_LIBS=OFF - LIBGIT2_BUILD_OPTIONS += -DCMAKE_C_FLAGS=-fPIC - LIBGIT2_BUILD_OPTIONS += -DCMAKE_BUILD_TYPE=Release - LIBGIT2_BUILD_OPTIONS += -DCMAKE_INSTALL_PREFIX=${LIBGIT2_INSTALL_DIR} - LIBGIT2_BUILD_OPTIONS += -DCMAKE_INSTALL_LIBDIR=lib - LIBGIT2_BUILD_OPTIONS += -DUSE_SSH=OFF - LIBGIT2_BUILD_OPTIONS += -DUSE_HTTPS=OFF - LIBGIT2_BUILD_OPTIONS += -DUSE_ICONV=OFF - LIBGIT2_BUILD_OPTIONS += -DUSE_NTLMCLIENT=OFF - LIBGIT2_BUILD_OPTIONS += -DUSE_BUNDLED_ZLIB=ON - LIBGIT2_BUILD_OPTIONS += -DUSE_HTTP_PARSER=builtin - LIBGIT2_BUILD_OPTIONS += -DUSE_THREADS=ON - LIBGIT2_BUILD_OPTIONS += -DREGEX_BACKEND=builtin -endif - -ifdef LIBGIT2_APPEND_BUILD_OPTIONS - LIBGIT2_BUILD_OPTIONS += ${LIBGIT2_APPEND_BUILD_OPTIONS} -endif - # These variables control test options and artifacts ## List of Go packages which shall be tested. ## Go packages to test when using the test-go target. @@ -236,7 +199,7 @@ BUILD_GEM_OPTIONS ?= # All executables provided by Gitaly. GITALY_EXECUTABLES = $(addprefix ${BUILD_DIR}/bin/,$(notdir $(shell find ${SOURCE_DIR}/cmd -mindepth 1 -maxdepth 1 -type d -print))) # All executables packed inside the Gitaly binary. -GITALY_PACKED_EXECUTABLES = $(filter %gitaly-hooks %gitaly-gpg %gitaly-git2go %gitaly-ssh %gitaly-lfs-smudge, ${GITALY_EXECUTABLES}) +GITALY_PACKED_EXECUTABLES = $(filter %gitaly-hooks %gitaly-gpg %gitaly-ssh %gitaly-lfs-smudge, ${GITALY_EXECUTABLES}) # All executables that should be installed. GITALY_INSTALLED_EXECUTABLES = $(filter-out ${GITALY_PACKED_EXECUTABLES}, ${GITALY_EXECUTABLES}) # Find all Go source files. @@ -251,7 +214,7 @@ find_go_sources = $(shell find ${SOURCE_DIR} -type d \( -path "${SO run_go_tests = PATH='${SOURCE_DIR}/internal/testhelper/testdata/home/bin:${PATH}' \ TEST_TMP_DIR='${TEST_TMP_DIR}' \ TEST_LOG_DIR='${TEST_LOG_DIR}' \ - ${GOTESTSUM} --format ${TEST_FORMAT} --junitfile '${TEST_JUNIT_REPORT}' --jsonfile '${TEST_JSON_REPORT}' -- -ldflags '${GO_LDFLAGS}' -tags '${SERVER_BUILD_TAGS},${GIT2GO_BUILD_TAGS}' ${TEST_OPTIONS} ${TEST_PACKAGES} + ${GOTESTSUM} --format ${TEST_FORMAT} --junitfile '${TEST_JUNIT_REPORT}' --jsonfile '${TEST_JSON_REPORT}' -- -ldflags '${GO_LDFLAGS}' -tags '${SERVER_BUILD_TAGS}' ${TEST_OPTIONS} ${TEST_PACKAGES} ## Test options passed to `dlv test`. DEBUG_OPTIONS ?= $(patsubst -%,-test.%,${TEST_OPTIONS}) @@ -262,7 +225,7 @@ DEBUG_OPTIONS ?= $(patsubst -%,-test.%,${TEST_OPTIONS}) # DEBUG_OPTIONS: any additional options, will default to TEST_OPTIONS if not set. debug_go_tests = PATH='${SOURCE_DIR}/internal/testhelper/testdata/home/bin:${PATH}' \ TEST_TMP_DIR='${TEST_TMP_DIR}' \ - ${DELVE} test --build-flags="-ldflags '${GO_LDFLAGS}' -tags '${SERVER_BUILD_TAGS},${GIT2GO_BUILD_TAGS}'" ${TEST_PACKAGES} -- ${DEBUG_OPTIONS} + ${DELVE} test --build-flags="-ldflags '${GO_LDFLAGS}' -tags '${SERVER_BUILD_TAGS}'" ${TEST_PACKAGES} -- ${DEBUG_OPTIONS} unexport GOROOT ## GOCACHE_MAX_SIZE_KB is the maximum size of Go's build cache in kilobytes before it is cleaned up. @@ -270,9 +233,6 @@ GOCACHE_MAX_SIZE_KB ?= 5000000 export GOCACHE ?= ${BUILD_DIR}/cache export GOPROXY ?= https://proxy.golang.org|https://proxy.golang.org export PATH := ${BUILD_DIR}/bin:${PATH} -export PKG_CONFIG_PATH := ${LIBGIT2_INSTALL_DIR}/lib/pkgconfig:${PKG_CONFIG_PATH} -# Allow the linker flag -D_THREAD_SAFE as libgit2 is compiled with it on FreeBSD -export CGO_LDFLAGS_ALLOW = -D_THREAD_SAFE # By default, intermediate targets get deleted automatically after a successful # build. We do not want that though: there's some precious intermediate targets @@ -371,7 +331,7 @@ run_go_tests += \ endif .PHONY: prepare-tests -prepare-tests: libgit2 ${GOTESTSUM} ${GITALY_PACKED_EXECUTABLES} +prepare-tests: ${GOTESTSUM} ${GITALY_PACKED_EXECUTABLES} ${Q}mkdir -p "$(dir ${TEST_JUNIT_REPORT})" .PHONY: prepare-debug @@ -444,13 +404,13 @@ ${TOOLS_DIR}/gitaly-linters.so: ${SOURCE_DIR}/tools/golangci-lint/go.sum $(wildc .PHONY: lint ## Run Go linter. -lint: ${GOLANGCI_LINT} libgit2 ${GITALY_PACKED_EXECUTABLES} ${TOOLS_DIR}/gitaly-linters.so lint-gitaly-linters - ${Q}${GOLANGCI_LINT} run --build-tags "${SERVER_BUILD_TAGS},${GIT2GO_BUILD_TAGS}" --out-format tab --config ${GOLANGCI_LINT_CONFIG} ${GOLANGCI_LINT_OPTIONS} +lint: ${GOLANGCI_LINT} ${GITALY_PACKED_EXECUTABLES} ${TOOLS_DIR}/gitaly-linters.so lint-gitaly-linters + ${Q}${GOLANGCI_LINT} run --build-tags "${SERVER_BUILD_TAGS}" --out-format tab --config ${GOLANGCI_LINT_CONFIG} ${GOLANGCI_LINT_OPTIONS} .PHONY: lint-fix ## Run Go linter and write back fixes to the files (not supported by all linters). -lint-fix: ${GOLANGCI_LINT} libgit2 ${GITALY_PACKED_EXECUTABLES} ${TOOLS_DIR}/gitaly-linters.so - ${Q}${GOLANGCI_LINT} run --fix --build-tags "${SERVER_BUILD_TAGS},${GIT2GO_BUILD_TAGS}" --out-format tab --config ${GOLANGCI_LINT_CONFIG} ${GOLANGCI_LINT_OPTIONS} +lint-fix: ${GOLANGCI_LINT} ${GITALY_PACKED_EXECUTABLES} ${TOOLS_DIR}/gitaly-linters.so + ${Q}${GOLANGCI_LINT} run --fix --build-tags "${SERVER_BUILD_TAGS}" --out-format tab --config ${GOLANGCI_LINT_CONFIG} ${GOLANGCI_LINT_OPTIONS} .PHONY: lint-docs ## Run markdownlint-cli2-config to lint the documentation. @@ -552,16 +512,12 @@ build-git: ${DEPENDENCY_DIR}/git-distribution/git install-git: build-git ${Q}env -u PROFILE -u MAKEFLAGS -u GIT_VERSION ${MAKE} -C "${DEPENDENCY_DIR}/git-distribution" -j$(shell nproc) prefix=${GIT_PREFIX} ${GIT_BUILD_OPTIONS} install -.PHONY: libgit2 -## Build libgit2. -libgit2: ${LIBGIT2_INSTALL_DIR}/lib/libgit2.a - ${SOURCE_DIR}/NOTICE: ${BUILD_DIR}/NOTICE ${Q}mv $< $@ ${BUILD_DIR}/NOTICE: ${GO_LICENSES} ${GITALY_PACKED_EXECUTABLES} ${Q}rm -rf ${BUILD_DIR}/licenses - ${Q}GOOS=linux GOFLAGS="-tags=${SERVER_BUILD_TAGS},${GIT2GO_BUILD_TAGS}" ${GO_LICENSES} save ${SOURCE_DIR}/... --save_path=${BUILD_DIR}/licenses + ${Q}GOOS=linux GOFLAGS="-tags=${SERVER_BUILD_TAGS}" ${GO_LICENSES} save ${SOURCE_DIR}/... --save_path=${BUILD_DIR}/licenses ${Q}go run ${SOURCE_DIR}/tools/noticegen/noticegen.go -source ${BUILD_DIR}/licenses -template ${SOURCE_DIR}/tools/noticegen/notice.template > ${BUILD_DIR}/NOTICE ${BUILD_DIR}: @@ -612,8 +568,6 @@ clear-go-build-cache-if-needed: ${BUILD_DIR}/intermediate/gitaly: GO_BUILD_TAGS = ${SERVER_BUILD_TAGS} ${BUILD_DIR}/intermediate/gitaly: ${GITALY_PACKED_EXECUTABLES} ${BUILD_DIR}/intermediate/praefect: GO_BUILD_TAGS = ${SERVER_BUILD_TAGS} -${BUILD_DIR}/intermediate/gitaly-git2go: GO_BUILD_TAGS = ${GIT2GO_BUILD_TAGS} -${BUILD_DIR}/intermediate/gitaly-git2go: libgit2 ${BUILD_DIR}/intermediate/%: clear-go-build-cache-if-needed .FORCE @ # We're building intermediate binaries first which contain a fixed build ID @ # of "TEMP_GITALY_BUILD_ID". In the final binary we replace this build ID with @@ -627,25 +581,11 @@ ${BUILD_DIR}/intermediate/%: clear-go-build-cache-if-needed .FOR # change. The dependency on the phony target is required to always rebuild # these targets. .PHONY: dependency-version -${DEPENDENCY_DIR}/libgit2.version: dependency-version | ${DEPENDENCY_DIR} - ${Q}[ x"$$(cat "$@" 2>/dev/null)" = x"${LIBGIT2_VERSION} ${LIBGIT2_BUILD_OPTIONS}" ] || >$@ echo -n "${LIBGIT2_VERSION} ${LIBGIT2_BUILD_OPTIONS}" ${DEPENDENCY_DIR}/git-%.version: dependency-version | ${DEPENDENCY_DIR} ${Q}[ x"$$(cat "$@" 2>/dev/null)" = x"${GIT_VERSION} ${GIT_BUILD_OPTIONS}" ] || >$@ echo -n "${GIT_VERSION} ${GIT_BUILD_OPTIONS}" ${DEPENDENCY_DIR}/protoc.version: dependency-version | ${DEPENDENCY_DIR} ${Q}[ x"$$(cat "$@" 2>/dev/null)" = x"${PROTOC_VERSION} ${PROTOC_BUILD_OPTIONS}" ] || >$@ echo -n "${PROTOC_VERSION} ${PROTOC_BUILD_OPTIONS}" -${LIBGIT2_INSTALL_DIR}/lib/libgit2.a: ${DEPENDENCY_DIR}/libgit2.version - ${Q}${GIT} -c init.defaultBranch=master init ${GIT_QUIET} ${LIBGIT2_SOURCE_DIR} - ${Q}${GIT} -C "${LIBGIT2_SOURCE_DIR}" config remote.origin.url ${LIBGIT2_REPO_URL} - ${Q}${GIT} -C "${LIBGIT2_SOURCE_DIR}" config remote.origin.tagOpt --no-tags - ${Q}${GIT} -C "${LIBGIT2_SOURCE_DIR}" fetch --depth 1 ${GIT_QUIET} origin ${LIBGIT2_VERSION} - ${Q}${GIT} -C "${LIBGIT2_SOURCE_DIR}" checkout ${GIT_QUIET} --detach FETCH_HEAD - ${Q}rm -rf ${LIBGIT2_BUILD_DIR} - ${Q}mkdir -p ${LIBGIT2_BUILD_DIR} - ${Q}cd ${LIBGIT2_BUILD_DIR} && cmake ${LIBGIT2_SOURCE_DIR} ${LIBGIT2_BUILD_OPTIONS} - ${Q}CMAKE_BUILD_PARALLEL_LEVEL=$(shell nproc) cmake --build ${LIBGIT2_BUILD_DIR} --target install - go install -a github.com/libgit2/git2go/${GIT2GO_VERSION} - # This target is responsible for checking out Git sources. In theory, we'd only # need to depend on the source directory. But given that the source directory # always changes when anything inside of it changes, like when we for example @@ -21181,30 +21181,6 @@ OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ -LICENSE - github.com/libgit2/git2go/v34 -The MIT License - -Copyright (c) 2013 The git2go contributors - -Permission is hereby granted, free of charge, to any person obtaining a copy -of this software and associated documentation files (the "Software"), to deal -in the Software without restriction, including without limitation the rights -to use, copy, modify, merge, publish, distribute, sublicense, and/or sell -copies of the Software, and to permit persons to whom the Software is -furnished to do so, subject to the following conditions: - -The above copyright notice and this permission notice shall be included in -all copies or substantial portions of the Software. - -THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR -IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, -FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE -AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER -LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, -OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN -THE SOFTWARE. - -~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ LICENSE - github.com/mattn/go-isatty Copyright (c) Yasuhiro MATSUMOTO <mattn.jp@gmail.com> @@ -22177,7 +22153,8 @@ LICENSE - github.com/opentracing/opentracing-go LICENSE - github.com/pelletier/go-toml/v2 The MIT License (MIT) -Copyright (c) 2013 - 2022 Thomas Pelletier, Eric Anderton +go-toml v2 +Copyright (c) 2021 - 2023 Thomas Pelletier Permission is hereby granted, free of charge, to any person obtaining a copy of this software and associated documentation files (the "Software"), to deal @@ -55,7 +55,6 @@ graph LR subgraph "Gitaly Service" Gitaly == git ==> Filesystem - Gitaly -- "libgit2" --> Filesystem[(Filesystem)] end subgraph "Clients" @@ -73,9 +72,8 @@ In [High Availability](#high-availability) mode, the current implementation look ```mermaid graph LR - subgraph "Gitaly Nodes" + subgraph "Gitaly Nodes" Gitaly == git ==> Filesystem - Gitaly -- "libgit2" --> Filesystem[(Filesystem)] end subgraph "Praefects" @@ -89,10 +87,10 @@ graph LR subgraph "Clients" Rails[gitlab-rails] - Workhorse + Workhorse Shell[gitlab-shell] end - + Clients --> Praefects --> Gitaly ``` diff --git a/cmd/gitaly-git2go/featureflags.go b/cmd/gitaly-git2go/featureflags.go deleted file mode 100644 index 055f18be9..000000000 --- a/cmd/gitaly-git2go/featureflags.go +++ /dev/null @@ -1,41 +0,0 @@ -//go:build static && system_libgit2 && gitaly_test - -package main - -import ( - "context" - "encoding/gob" - "flag" - - "gitlab.com/gitlab-org/gitaly/v16/internal/featureflag" - "gitlab.com/gitlab-org/gitaly/v16/internal/git2go" -) - -// This subcommand is only called in tests, so we don't want to register it like -// the other subcommands but instead will do it in an init block. The gitaly_test build -// flag will guarantee that this is not built and registered in the -// gitaly-git2go binary -func init() { - subcommands["feature-flags"] = &featureFlagsSubcommand{} -} - -type featureFlagsSubcommand struct{} - -func (featureFlagsSubcommand) Flags() *flag.FlagSet { - return flag.NewFlagSet("feature-flags", flag.ExitOnError) -} - -func (featureFlagsSubcommand) Run(ctx context.Context, decoder *gob.Decoder, encoder *gob.Encoder) error { - var flags []git2go.FeatureFlag - for flag, value := range featureflag.FromContext(ctx) { - flags = append(flags, git2go.FeatureFlag{ - Name: flag.Name, - MetadataKey: flag.MetadataKey(), - Value: value, - }) - } - - return encoder.Encode(git2go.FeatureFlags{ - Flags: flags, - }) -} diff --git a/cmd/gitaly-git2go/git2goutil/commit.go b/cmd/gitaly-git2go/git2goutil/commit.go deleted file mode 100644 index 804715884..000000000 --- a/cmd/gitaly-git2go/git2goutil/commit.go +++ /dev/null @@ -1,47 +0,0 @@ -package git2goutil - -import ( - "fmt" - - git "github.com/libgit2/git2go/v34" -) - -// CommitSubmitter is the helper struct to make signed Commits conveniently. -type CommitSubmitter struct { - Repo *git.Repository - SigningKeyPath string -} - -// NewCommitSubmitter creates a new CommitSubmitter. -func NewCommitSubmitter(repo *git.Repository, signingKeyPath string) *CommitSubmitter { - return &CommitSubmitter{ - Repo: repo, - SigningKeyPath: signingKeyPath, - } -} - -// Commit commits a commit with or without signature depends on SigningKeyPath value. -func (cs *CommitSubmitter) Commit( - author, committer *git.Signature, - messageEncoding git.MessageEncoding, - message string, - tree *git.Tree, - parents ...*git.Commit, -) (*git.Oid, error) { - commitBytes, err := cs.Repo.CreateCommitBuffer(author, committer, messageEncoding, message, tree, parents...) - if err != nil { - return nil, err - } - - signature, err := CreateCommitSignature(cs.SigningKeyPath, commitBytes) - if err != nil { - return nil, fmt.Errorf("create commit signature: %w", err) - } - - commitID, err := cs.Repo.CreateCommitWithSignature(string(commitBytes), string(signature), "") - if err != nil { - return nil, err - } - - return commitID, nil -} diff --git a/cmd/gitaly-git2go/git2goutil/repo.go b/cmd/gitaly-git2go/git2goutil/repo.go deleted file mode 100644 index 1a1c4ede2..000000000 --- a/cmd/gitaly-git2go/git2goutil/repo.go +++ /dev/null @@ -1,10 +0,0 @@ -package git2goutil - -import ( - git "github.com/libgit2/git2go/v34" -) - -// OpenRepository opens the repository located at path as a Git2Go repository. -func OpenRepository(path string) (*git.Repository, error) { - return git.OpenRepositoryExtended(path, git.RepositoryOpenFromEnv, "") -} diff --git a/cmd/gitaly-git2go/git2goutil/sign.go b/cmd/gitaly-git2go/git2goutil/sign.go deleted file mode 100644 index efa8d1b1d..000000000 --- a/cmd/gitaly-git2go/git2goutil/sign.go +++ /dev/null @@ -1,22 +0,0 @@ -package git2goutil - -import ( - "fmt" - - "gitlab.com/gitlab-org/gitaly/v16/internal/signature" -) - -// CreateCommitSignature reads the given signing key and produces PKCS#7 detached signature. -// When the path to the signing key is not present, an empty signature is returned. -func CreateCommitSignature(signingKeyPath string, contentToSign []byte) ([]byte, error) { - if signingKeyPath == "" { - return nil, nil - } - - signingKeys, err := signature.ParseSigningKeys(signingKeyPath) - if err != nil { - return nil, fmt.Errorf("failed to parse signing key: %w", err) - } - - return signingKeys.CreateSignature(contentToSign) -} diff --git a/cmd/gitaly-git2go/main.go b/cmd/gitaly-git2go/main.go deleted file mode 100644 index 9c5c4009e..000000000 --- a/cmd/gitaly-git2go/main.go +++ /dev/null @@ -1,165 +0,0 @@ -//go:build static && system_libgit2 - -package main - -import ( - "context" - "encoding/gob" - "flag" - "fmt" - "os" - "strings" - - "github.com/grpc-ecosystem/go-grpc-middleware/logging/logrus/ctxlogrus" - git "github.com/libgit2/git2go/v34" - "github.com/sirupsen/logrus" - "gitlab.com/gitlab-org/gitaly/v16/internal/featureflag" - "gitlab.com/gitlab-org/gitaly/v16/internal/git2go" - glog "gitlab.com/gitlab-org/gitaly/v16/internal/log" - "gitlab.com/gitlab-org/labkit/correlation" -) - -type subcmd interface { - Flags() *flag.FlagSet - Run(ctx context.Context, decoder *gob.Decoder, encoder *gob.Encoder) error -} - -var subcommands = map[string]subcmd{ - "rebase": &rebaseSubcommand{}, -} - -func fatalf(logger logrus.FieldLogger, encoder *gob.Encoder, format string, args ...interface{}) { - err := encoder.Encode(git2go.Result{ - Err: git2go.SerializableError(fmt.Errorf(format, args...)), - }) - if err != nil { - logger.WithError(err).Error("encode to gob failed") - } - // An exit code of 1 would indicate an error over stderr. Since our errors - // are encoded over gob, we need to exit cleanly - os.Exit(0) -} - -func main() { - decoder := gob.NewDecoder(os.Stdin) - encoder := gob.NewEncoder(os.Stdout) - - var logFormat, logLevel, correlationID string - var enabledFeatureFlags, disabledFeatureFlags featureFlagArg - - flags := flag.NewFlagSet(git2go.BinaryName, flag.PanicOnError) - flags.StringVar(&logFormat, "log-format", "", "logging format") - flags.StringVar(&logLevel, "log-level", "", "logging level") - flags.StringVar(&correlationID, "correlation-id", "", "correlation ID used for request tracing") - flags.Var( - &enabledFeatureFlags, - "enabled-feature-flags", - "comma separated list of explicitly enabled feature flags", - ) - flags.Var( - &disabledFeatureFlags, - "disabled-feature-flags", - "comma separated list of explicitly disabled feature flags", - ) - _ = flags.Parse(os.Args[1:]) - - if correlationID == "" { - correlationID = correlation.SafeRandomID() - } - - ctx := correlation.ContextWithCorrelation(context.Background(), correlationID) - - logger, err := glog.Configure(os.Stderr, logFormat, logLevel) - if err != nil { - fmt.Printf("configuring logger failed: %v", err) - os.Exit(1) - } - - logger = logger.WithFields(logrus.Fields{ - "command.name": git2go.BinaryName, - "correlation_id": correlationID, - "enabled_feature_flags": enabledFeatureFlags, - "disabled_feature_flags": disabledFeatureFlags, - }) - - if flags.NArg() < 1 { - fatalf(logger, encoder, "missing subcommand") - } - - subcmd, ok := subcommands[flags.Arg(0)] - if !ok { - fatalf(logger, encoder, "unknown subcommand: %q", flags.Arg(0)) - } - - subcmdFlags := subcmd.Flags() - if err := subcmdFlags.Parse(flags.Args()[1:]); err != nil { - fatalf(logger, encoder, "parsing flags of %q: %s", subcmdFlags.Name(), err) - } - - if subcmdFlags.NArg() != 0 { - fatalf(logger, encoder, "%s: trailing arguments", subcmdFlags.Name()) - } - - if err := git.EnableFsyncGitDir(true); err != nil { - fatalf(logger, encoder, "enable fsync: %s", err) - } - - for _, configLevel := range []git.ConfigLevel{ - git.ConfigLevelSystem, - git.ConfigLevelXDG, - git.ConfigLevelGlobal, - } { - if err := git.SetSearchPath(configLevel, "/dev/null"); err != nil { - fatalf(logger, encoder, "setting search path: %s", err) - } - } - - subcmdLogger := logger.WithField("command.subcommand", subcmdFlags.Name()) - subcmdLogger.Infof("starting %s command", subcmdFlags.Name()) - - ctx = ctxlogrus.ToContext(ctx, subcmdLogger) - ctx = enabledFeatureFlags.ToContext(ctx, true) - ctx = disabledFeatureFlags.ToContext(ctx, false) - - if err := subcmd.Run(ctx, decoder, encoder); err != nil { - subcmdLogger.WithError(err).Errorf("%s command failed", subcmdFlags.Name()) - fatalf(logger, encoder, "%s: %s", subcmdFlags.Name(), err) - } - - subcmdLogger.Infof("%s command finished", subcmdFlags.Name()) -} - -type featureFlagArg []featureflag.FeatureFlag - -func (v *featureFlagArg) String() string { - metadataKeys := make([]string, 0, len(*v)) - for _, flag := range *v { - metadataKeys = append(metadataKeys, flag.MetadataKey()) - } - return strings.Join(metadataKeys, ",") -} - -func (v *featureFlagArg) Set(s string) error { - if s == "" { - return nil - } - - for _, metadataKey := range strings.Split(s, ",") { - flag, err := featureflag.FromMetadataKey(metadataKey) - if err != nil { - return err - } - - *v = append(*v, flag) - } - - return nil -} - -func (v featureFlagArg) ToContext(ctx context.Context, enabled bool) context.Context { - for _, flag := range v { - ctx = featureflag.IncomingCtxWithFeatureFlag(ctx, flag, enabled) - } - - return ctx -} diff --git a/cmd/gitaly-git2go/merge.go b/cmd/gitaly-git2go/merge.go deleted file mode 100644 index 92f0f53fb..000000000 --- a/cmd/gitaly-git2go/merge.go +++ /dev/null @@ -1,57 +0,0 @@ -//go:build static && system_libgit2 - -package main - -import ( - "errors" - "fmt" - - git "github.com/libgit2/git2go/v34" -) - -func getConflictingFiles(index *git.Index) ([]string, error) { - conflicts, err := getConflicts(index) - if err != nil { - return nil, fmt.Errorf("getting conflicts: %w", err) - } - - conflictingFiles := make([]string, 0, len(conflicts)) - for _, conflict := range conflicts { - switch { - case conflict.Our != nil: - conflictingFiles = append(conflictingFiles, conflict.Our.Path) - case conflict.Ancestor != nil: - conflictingFiles = append(conflictingFiles, conflict.Ancestor.Path) - case conflict.Their != nil: - conflictingFiles = append(conflictingFiles, conflict.Their.Path) - default: - return nil, errors.New("invalid conflict") - } - } - - return conflictingFiles, nil -} - -func getConflicts(index *git.Index) ([]git.IndexConflict, error) { - var conflicts []git.IndexConflict - - iterator, err := index.ConflictIterator() - if err != nil { - return nil, err - } - defer iterator.Free() - - for { - conflict, err := iterator.Next() - if err != nil { - if git.IsErrorCode(err, git.ErrorCodeIterOver) { - break - } - return nil, err - } - - conflicts = append(conflicts, conflict) - } - - return conflicts, nil -} diff --git a/cmd/gitaly-git2go/rebase.go b/cmd/gitaly-git2go/rebase.go deleted file mode 100644 index cf221007f..000000000 --- a/cmd/gitaly-git2go/rebase.go +++ /dev/null @@ -1,192 +0,0 @@ -//go:build static && system_libgit2 - -package main - -import ( - "context" - "encoding/gob" - "errors" - "flag" - "fmt" - - git "github.com/libgit2/git2go/v34" - "gitlab.com/gitlab-org/gitaly/v16/cmd/gitaly-git2go/git2goutil" - "gitlab.com/gitlab-org/gitaly/v16/internal/git2go" -) - -type rebaseSubcommand struct{} - -func (cmd *rebaseSubcommand) Flags() *flag.FlagSet { - return flag.NewFlagSet("rebase", flag.ExitOnError) -} - -func (cmd *rebaseSubcommand) Run(ctx context.Context, decoder *gob.Decoder, encoder *gob.Encoder) error { - var request git2go.RebaseCommand - if err := decoder.Decode(&request); err != nil { - return err - } - - commitID, err := cmd.rebase(ctx, &request) - return encoder.Encode(git2go.Result{ - CommitID: commitID, - Err: git2go.SerializableError(err), - }) -} - -func (cmd *rebaseSubcommand) verify(ctx context.Context, r *git2go.RebaseCommand) error { - if r.Repository == "" { - return errors.New("missing repository") - } - if r.Committer.Name == "" { - return errors.New("missing committer name") - } - if r.Committer.Email == "" { - return errors.New("missing committer email") - } - if r.BranchName == "" && r.CommitID == "" { - return errors.New("missing branch name") - } - if r.BranchName != "" && r.CommitID != "" { - return errors.New("both branch name and commit ID") - } - if r.UpstreamRevision == "" && r.UpstreamCommitID == "" { - return errors.New("missing upstream revision") - } - if r.UpstreamRevision != "" && r.UpstreamCommitID != "" { - return errors.New("both upstream revision and upstream commit ID") - } - return nil -} - -func (cmd *rebaseSubcommand) rebase(ctx context.Context, request *git2go.RebaseCommand) (string, error) { - if err := cmd.verify(ctx, request); err != nil { - return "", err - } - - repo, err := git2goutil.OpenRepository(request.Repository) - if err != nil { - return "", fmt.Errorf("open repository: %w", err) - } - - opts, err := git.DefaultRebaseOptions() - if err != nil { - return "", fmt.Errorf("get rebase options: %w", err) - } - opts.InMemory = 1 - opts.CommitCreateCallback = git2goutil.NewCommitSubmitter(repo, request.SigningKey).Commit - - var commit *git.AnnotatedCommit - if request.BranchName != "" { - commit, err = repo.AnnotatedCommitFromRevspec(fmt.Sprintf("refs/heads/%s", request.BranchName)) - if err != nil { - return "", fmt.Errorf("look up branch %q: %w", request.BranchName, err) - } - } else { - commitOid, err := git.NewOid(request.CommitID.String()) - if err != nil { - return "", fmt.Errorf("parse commit %q: %w", request.CommitID, err) - } - - commit, err = repo.LookupAnnotatedCommit(commitOid) - if err != nil { - return "", fmt.Errorf("look up commit %q: %w", request.CommitID, err) - } - } - - upstreamCommitParam := request.UpstreamRevision - if upstreamCommitParam == "" { - upstreamCommitParam = request.UpstreamCommitID.String() - } - - upstreamCommitOID, err := git.NewOid(upstreamCommitParam) - if err != nil { - return "", fmt.Errorf("parse upstream revision %q: %w", upstreamCommitParam, err) - } - - upstreamCommit, err := repo.LookupAnnotatedCommit(upstreamCommitOID) - if err != nil { - return "", fmt.Errorf("look up upstream revision %q: %w", upstreamCommitParam, err) - } - - mergeBase, err := repo.MergeBase(upstreamCommit.Id(), commit.Id()) - if err != nil { - return "", fmt.Errorf("find merge base: %w", err) - } - - if mergeBase.Equal(upstreamCommit.Id()) { - // Branch is zero commits behind, so do not rebase - return commit.Id().String(), nil - } - - if mergeBase.Equal(commit.Id()) { - // Branch is merged, so fast-forward to upstream - return upstreamCommit.Id().String(), nil - } - - mergeCommit, err := repo.LookupAnnotatedCommit(mergeBase) - if err != nil { - return "", fmt.Errorf("look up merge base: %w", err) - } - - rebase, err := repo.InitRebase(commit, mergeCommit, upstreamCommit, &opts) - if err != nil { - return "", fmt.Errorf("initiate rebase: %w", err) - } - - committer := git.Signature(request.Committer) - var oid *git.Oid - for { - op, err := rebase.Next() - if git.IsErrorCode(err, git.ErrorCodeIterOver) { - break - } else if err != nil { - return "", fmt.Errorf("rebase iterate: %w", err) - } - - commit, err := repo.LookupCommit(op.Id) - if err != nil { - return "", fmt.Errorf("lookup commit: %w", err) - } - - if err := rebase.Commit(op.Id, nil, &committer, commit.Message()); err != nil { - if git.IsErrorCode(err, git.ErrorCodeUnmerged) { - index, err := rebase.InmemoryIndex() - if err != nil { - return "", fmt.Errorf("getting conflicting index: %w", err) - } - - conflictingFiles, err := getConflictingFiles(index) - if err != nil { - return "", fmt.Errorf("getting conflicting files: %w", err) - } - - return "", fmt.Errorf("commit %q: %w", op.Id.String(), git2go.ConflictingFilesError{ - ConflictingFiles: conflictingFiles, - }) - } - - // If the commit has already been applied on the target branch then we can - // skip it if we were told to. - if request.SkipEmptyCommits && git.IsErrorCode(err, git.ErrorCodeApplied) { - continue - } - - return "", fmt.Errorf("commit %q: %w", op.Id.String(), err) - } - - oid = op.Id.Copy() - } - - // When the OID is unset here, then we didn't have to rebase any commits at all. We can - // thus return the upstream commit directly: rebasing nothing onto the upstream commit is - // the same as the upstream commit itself. - if oid == nil { - return upstreamCommit.Id().String(), nil - } - - if err = rebase.Finish(); err != nil { - return "", fmt.Errorf("finish rebase: %w", err) - } - - return oid.String(), nil -} diff --git a/cmd/gitaly-git2go/rebase_test.go b/cmd/gitaly-git2go/rebase_test.go deleted file mode 100644 index 6de901e32..000000000 --- a/cmd/gitaly-git2go/rebase_test.go +++ /dev/null @@ -1,423 +0,0 @@ -//go:build static && system_libgit2 - -package main - -import ( - "fmt" - "testing" - "time" - - "github.com/stretchr/testify/require" - "gitlab.com/gitlab-org/gitaly/v16/cmd/gitaly-git2go/git2goutil" - "gitlab.com/gitlab-org/gitaly/v16/internal/git" - "gitlab.com/gitlab-org/gitaly/v16/internal/git/gittest" - "gitlab.com/gitlab-org/gitaly/v16/internal/git2go" - "gitlab.com/gitlab-org/gitaly/v16/internal/testhelper" - "gitlab.com/gitlab-org/gitaly/v16/internal/testhelper/testcfg" -) - -var masterRevision = "1e292f8fedd741b75372e19097c76d327140c312" - -func TestRebase_validation(t *testing.T) { - gittest.SkipWithSHA256(t) - - ctx := testhelper.Context(t) - cfg := testcfg.Build(t) - - testcfg.BuildGitalyGit2Go(t, cfg) - - repo, repoPath := gittest.CreateRepository(t, ctx, cfg, gittest.CreateRepositoryConfig{ - SkipCreationViaService: true, - }) - - committer := git2go.NewSignature("Foo", "foo@example.com", time.Now()) - executor := buildExecutor(t, cfg) - - testcases := []struct { - desc string - request git2go.RebaseCommand - expectedErr string - }{ - { - desc: "no arguments", - expectedErr: "rebase: missing repository", - }, - { - desc: "missing repository", - request: git2go.RebaseCommand{Committer: committer, BranchName: "feature", UpstreamRevision: masterRevision}, - expectedErr: "rebase: missing repository", - }, - { - desc: "missing committer name", - request: git2go.RebaseCommand{Repository: repoPath, Committer: git2go.Signature{Email: "foo@example.com"}, BranchName: "feature", UpstreamRevision: masterRevision}, - expectedErr: "rebase: missing committer name", - }, - { - desc: "missing committer email", - request: git2go.RebaseCommand{Repository: repoPath, Committer: git2go.Signature{Name: "Foo"}, BranchName: "feature", UpstreamRevision: masterRevision}, - expectedErr: "rebase: missing committer email", - }, - { - desc: "missing branch name", - request: git2go.RebaseCommand{Repository: repoPath, Committer: committer, UpstreamRevision: masterRevision}, - expectedErr: "rebase: missing branch name", - }, - { - desc: "missing upstream branch", - request: git2go.RebaseCommand{Repository: repoPath, Committer: committer, BranchName: "feature"}, - expectedErr: "rebase: missing upstream revision", - }, - { - desc: "both branch name and commit ID", - request: git2go.RebaseCommand{Repository: repoPath, Committer: committer, BranchName: "feature", CommitID: "a"}, - expectedErr: "rebase: both branch name and commit ID", - }, - { - desc: "both upstream revision and upstream commit ID", - request: git2go.RebaseCommand{Repository: repoPath, Committer: committer, BranchName: "feature", UpstreamRevision: "a", UpstreamCommitID: "a"}, - expectedErr: "rebase: both upstream revision and upstream commit ID", - }, - } - for _, tc := range testcases { - t.Run(tc.desc, func(t *testing.T) { - _, err := executor.Rebase(ctx, repo, tc.request) - require.EqualError(t, err, tc.expectedErr) - }) - } -} - -func TestRebase_rebase(t *testing.T) { - gittest.SkipWithSHA256(t) - - t.Parallel() - - ctx := testhelper.Context(t) - cfg := testcfg.Build(t) - testcfg.BuildGitalyGit2Go(t, cfg) - executor := buildExecutor(t, cfg) - - committer := git2go.NewSignature( - string(gittest.TestUser.Name), - string(gittest.TestUser.Email), - time.Date(2021, 3, 1, 13, 45, 50, 0, time.FixedZone("", +2*60*60)), - ) - - type setup struct { - base, upstream, downstream git.ObjectID - expecetedCommitsAhead int - expectedObjectID git.ObjectID - expectedErr string - } - - testcases := []struct { - desc string - setup func(testing.TB, string) setup - }{ - { - desc: "Single commit rebase", - setup: func(tb testing.TB, repoPath string) setup { - base := gittest.WriteCommit(t, cfg, repoPath, gittest.WithTreeEntries( - gittest.TreeEntry{Path: "path", Mode: "100644", Content: "a\nb\nc\nd\ne\nf\ng\n"}, - )) - upstream := gittest.WriteCommit(t, cfg, repoPath, gittest.WithMessage("upstream"), gittest.WithParents(base), gittest.WithTreeEntries( - gittest.TreeEntry{Path: "path", Mode: "100644", Content: "upstream\nb\nc\nd\ne\nf\ng\n"}, - )) - downstream := gittest.WriteCommit(t, cfg, repoPath, gittest.WithMessage("downstream"), gittest.WithParents(base), gittest.WithTreeEntries( - gittest.TreeEntry{Path: "path", Mode: "100644", Content: "a\nb\nc\nd\ne\nf\ndownstream\n"}, - )) - - return setup{ - base: base, - upstream: upstream, - downstream: downstream, - expectedObjectID: "ef018adb419cd97453a0624c28271fafe622b83e", - expecetedCommitsAhead: 1, - } - }, - }, - { - desc: "Multiple commits", - setup: func(tb testing.TB, repoPath string) setup { - base := gittest.WriteCommit(t, cfg, repoPath, gittest.WithTreeEntries( - gittest.TreeEntry{Path: "path", Mode: "100644", Content: "a\nb\nc\nd\ne\nf\ng\n"}, - )) - upstream := gittest.WriteCommit(t, cfg, repoPath, gittest.WithMessage("upstream"), gittest.WithParents(base), gittest.WithTreeEntries( - gittest.TreeEntry{Path: "path", Mode: "100644", Content: "upstream\nb\nc\nd\ne\nf\ng\n"}, - )) - downstream1 := gittest.WriteCommit(t, cfg, repoPath, gittest.WithMessage("downstream-1"), gittest.WithParents(base), gittest.WithTreeEntries( - gittest.TreeEntry{Path: "path", Mode: "100644", Content: "a\nb\nc\nd\ne\nf\ndownstream-1\n"}, - )) - downstream2 := gittest.WriteCommit(t, cfg, repoPath, gittest.WithMessage("downstream-2"), gittest.WithParents(downstream1), gittest.WithTreeEntries( - gittest.TreeEntry{Path: "path", Mode: "100644", Content: "a\nb\nc\nd\ne\nf\ndownstream-2\n"}, - )) - downstream3 := gittest.WriteCommit(t, cfg, repoPath, gittest.WithMessage("downstream-3"), gittest.WithParents(downstream2), gittest.WithTreeEntries( - gittest.TreeEntry{Path: "path", Mode: "100644", Content: "a\nb\nc\nd\ne\nf\ndownstream-3\n"}, - )) - - return setup{ - base: base, - upstream: upstream, - downstream: downstream3, - expectedObjectID: "d3c737fdb3a0c4da3a371fc01de6df4cbb5bc3e4", - expecetedCommitsAhead: 3, - } - }, - }, - { - desc: "Branch zero commits behind", - setup: func(tb testing.TB, repoPath string) setup { - base := gittest.WriteCommit(t, cfg, repoPath, gittest.WithTreeEntries( - gittest.TreeEntry{Path: "path", Mode: "100644", Content: "a\nb\nc\nd\ne\nf\ng\n"}, - )) - downstream := gittest.WriteCommit(t, cfg, repoPath, gittest.WithMessage("downstream"), gittest.WithParents(base), gittest.WithTreeEntries( - gittest.TreeEntry{Path: "path", Mode: "100644", Content: "a\nb\nc\nd\ne\nf\ndownstream\n"}, - )) - - return setup{ - base: base, - upstream: base, - downstream: downstream, - expectedObjectID: downstream, - expecetedCommitsAhead: 1, - } - }, - }, - { - desc: "Merged branch", - setup: func(tb testing.TB, repoPath string) setup { - base := gittest.WriteCommit(t, cfg, repoPath, gittest.WithTreeEntries( - gittest.TreeEntry{Path: "path", Mode: "100644", Content: "a\nb\nc\nd\ne\nf\ng\n"}, - )) - downstream := gittest.WriteCommit(t, cfg, repoPath, gittest.WithParents(base), gittest.WithTreeEntries( - gittest.TreeEntry{Path: "path", Mode: "100644", Content: "a\nb\nc\nd\ne\nf\ndownstream\n"}, - )) - merge := gittest.WriteCommit(t, cfg, repoPath, gittest.WithParents(base, downstream), gittest.WithTreeEntries( - gittest.TreeEntry{Path: "path", Mode: "100644", Content: "a\nb\nc\nd\ne\nf\ndownstream\n"}, - )) - - return setup{ - base: base, upstream: merge, downstream: downstream, - expectedObjectID: merge, - } - }, - }, - { - desc: "Partially merged branch", - setup: func(tb testing.TB, repoPath string) setup { - base := gittest.WriteCommit(t, cfg, repoPath, gittest.WithTreeEntries( - gittest.TreeEntry{Path: "path", Mode: "100644", Content: "a\nb\nc\nd\ne\nf\ng\n"}, - )) - downstream1 := gittest.WriteCommit(t, cfg, repoPath, gittest.WithParents(base), gittest.WithTreeEntries( - gittest.TreeEntry{Path: "path", Mode: "100644", Content: "a\nb\nc\nd\ne\nf\ndownstream-1\n"}, - )) - downstream2 := gittest.WriteCommit(t, cfg, repoPath, gittest.WithParents(downstream1), gittest.WithTreeEntries( - gittest.TreeEntry{Path: "path", Mode: "100644", Content: "a\nb\nc\nd\ne\nf\ndownstream-2\n"}, - )) - merge := gittest.WriteCommit(t, cfg, repoPath, gittest.WithParents(base, downstream1), gittest.WithTreeEntries( - gittest.TreeEntry{Path: "path", Mode: "100644", Content: "a\nb\nc\nd\ne\nf\ndownstream-1\n"}, - )) - - return setup{ - base: base, - upstream: merge, - downstream: downstream2, - expectedObjectID: "721e8bd36a394a7cc243b8c3960b44c5520c6246", - expecetedCommitsAhead: 1, - } - }, - }, - { - desc: "With upstream merged into", - setup: func(tb testing.TB, repoPath string) setup { - base := gittest.WriteCommit(t, cfg, repoPath, gittest.WithTreeEntries( - gittest.TreeEntry{Path: "path", Mode: "100644", Content: "a\nb\nc\nd\ne\nf\ng\n"}, - )) - upstream := gittest.WriteCommit(t, cfg, repoPath, gittest.WithMessage("upstream"), gittest.WithParents(base), gittest.WithTreeEntries( - gittest.TreeEntry{Path: "path", Mode: "100644", Content: "upstream\nb\nc\nd\ne\nf\ng\n"}, - )) - downstream := gittest.WriteCommit(t, cfg, repoPath, gittest.WithMessage("downstream"), gittest.WithParents(base), gittest.WithTreeEntries( - gittest.TreeEntry{Path: "path", Mode: "100644", Content: "a\nb\nc\nd\ne\nf\ndownstream\n"}, - )) - downstreamMerge := gittest.WriteCommit(t, cfg, repoPath, gittest.WithParents(downstream, upstream), gittest.WithTreeEntries( - gittest.TreeEntry{Path: "path", Mode: "100644", Content: "upstream\nb\nc\nd\ne\nf\ndownstream\n"}, - )) - - return setup{ - base: base, - upstream: upstream, - downstream: downstreamMerge, - expectedObjectID: "aa375bc059fa8830d9489d89af1278632722407d", - expecetedCommitsAhead: 2, - } - }, - }, - { - desc: "Rebase with conflict", - setup: func(tb testing.TB, repoPath string) setup { - base := gittest.WriteCommit(t, cfg, repoPath, gittest.WithTreeEntries( - gittest.TreeEntry{Path: "path", Mode: "100644", Content: "base\n"}, - )) - upstream := gittest.WriteCommit(t, cfg, repoPath, gittest.WithParents(base), gittest.WithTreeEntries( - gittest.TreeEntry{Path: "path", Mode: "100644", Content: "upstream\n"}, - )) - downstream := gittest.WriteCommit(t, cfg, repoPath, gittest.WithParents(base), gittest.WithTreeEntries( - gittest.TreeEntry{Path: "path", Mode: "100644", Content: "downstream\n"}, - )) - - return setup{ - upstream: upstream, - downstream: downstream, - expectedErr: fmt.Sprintf("rebase: commit %q: there are conflicting files", downstream), - } - }, - }, - { - desc: "Orphaned branch", - setup: func(tb testing.TB, repoPath string) setup { - upstream := gittest.WriteCommit(t, cfg, repoPath, gittest.WithTreeEntries( - gittest.TreeEntry{Path: "path", Mode: "100644", Content: "upstream\n"}, - )) - downstream := gittest.WriteCommit(t, cfg, repoPath, gittest.WithTreeEntries( - gittest.TreeEntry{Path: "path", Mode: "100644", Content: "downstream\n"}, - )) - - return setup{ - upstream: upstream, - downstream: downstream, - expectedErr: "rebase: find merge base: no merge base found", - } - }, - }, - } - - for _, tc := range testcases { - tc := tc - - t.Run(tc.desc, func(t *testing.T) { - t.Parallel() - - repoProto, repoPath := gittest.CreateRepository(t, ctx, cfg, gittest.CreateRepositoryConfig{ - SkipCreationViaService: true, - }) - setup := tc.setup(t, repoPath) - - gittest.WriteRef(t, cfg, repoPath, "refs/heads/upstream", setup.upstream) - gittest.WriteRef(t, cfg, repoPath, "refs/heads/downstream", setup.downstream) - - repo, err := git2goutil.OpenRepository(repoPath) - require.NoError(t, err) - - for desc, request := range map[string]git2go.RebaseCommand{ - "with branch and upstream": { - Repository: repoPath, - Committer: committer, - BranchName: "downstream", - UpstreamRevision: setup.upstream.String(), - }, - "with branch and upstream commit ID": { - Repository: repoPath, - Committer: committer, - BranchName: "downstream", - UpstreamCommitID: setup.upstream, - }, - "with commit ID and upstream": { - Repository: repoPath, - Committer: committer, - CommitID: setup.downstream, - UpstreamRevision: setup.upstream.String(), - }, - "with commit ID and upstream commit ID": { - Repository: repoPath, - Committer: committer, - CommitID: setup.downstream, - UpstreamCommitID: setup.upstream, - }, - } { - t.Run(desc, func(t *testing.T) { - response, err := executor.Rebase(ctx, repoProto, request) - if setup.expectedErr != "" { - require.EqualError(t, err, setup.expectedErr) - } else { - require.NoError(t, err) - require.Equal(t, setup.expectedObjectID, response) - - commit, err := lookupCommit(repo, response.String()) - require.NoError(t, err) - - for i := setup.expecetedCommitsAhead; i > 0; i-- { - commit = commit.Parent(0) - } - baseCommit, err := lookupCommit(repo, setup.base.String()) - require.NoError(t, err) - require.Equal(t, baseCommit, commit) - } - }) - } - }) - } -} - -func TestRebase_skipEmptyCommit(t *testing.T) { - gittest.SkipWithSHA256(t) - - ctx := testhelper.Context(t) - cfg := testcfg.Build(t) - - testcfg.BuildGitalyGit2Go(t, cfg) - - repoProto, repoPath := gittest.CreateRepository(t, ctx, cfg, gittest.CreateRepositoryConfig{ - SkipCreationViaService: true, - }) - - // Set up history with two diverging lines of branches, where both sides have implemented - // the same changes. During rebase, the diff will thus become empty. - base := gittest.WriteCommit(t, cfg, repoPath, - gittest.WithTreeEntries(gittest.TreeEntry{ - Path: "a", Content: "base", Mode: "100644", - }), - ) - theirs := gittest.WriteCommit(t, cfg, repoPath, gittest.WithMessage("theirs"), - gittest.WithParents(base), gittest.WithTreeEntries(gittest.TreeEntry{ - Path: "a", Content: "changed", Mode: "100644", - }), - ) - ours := gittest.WriteCommit(t, cfg, repoPath, gittest.WithMessage("ours"), - gittest.WithParents(base), gittest.WithTreeEntries(gittest.TreeEntry{ - Path: "a", Content: "changed", Mode: "100644", - }), - ) - - for _, tc := range []struct { - desc string - skipEmptyCommits bool - expectedErr string - expectedResponse git.ObjectID - }{ - { - desc: "do not skip empty commit", - skipEmptyCommits: false, - expectedErr: fmt.Sprintf("rebase: commit %q: this patch has already been applied", ours), - }, - { - desc: "skip empty commit", - skipEmptyCommits: true, - expectedResponse: theirs, - }, - } { - t.Run(tc.desc, func(t *testing.T) { - response, err := buildExecutor(t, cfg).Rebase(ctx, repoProto, git2go.RebaseCommand{ - Repository: repoPath, - Committer: git2go.NewSignature("Foo", "foo@example.com", time.Now()), - CommitID: ours, - UpstreamCommitID: theirs, - SkipEmptyCommits: tc.skipEmptyCommits, - }) - if tc.expectedErr == "" { - require.NoError(t, err) - } else { - require.EqualError(t, err, tc.expectedErr) - } - require.Equal(t, tc.expectedResponse, response) - }) - } -} diff --git a/cmd/gitaly-git2go/testhelper_test.go b/cmd/gitaly-git2go/testhelper_test.go deleted file mode 100644 index 0d72bec57..000000000 --- a/cmd/gitaly-git2go/testhelper_test.go +++ /dev/null @@ -1,44 +0,0 @@ -//go:build static && system_libgit2 - -package main - -import ( - "fmt" - "testing" - - git "github.com/libgit2/git2go/v34" - "gitlab.com/gitlab-org/gitaly/v16/internal/git/gittest" - "gitlab.com/gitlab-org/gitaly/v16/internal/git2go" - "gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/config" - "gitlab.com/gitlab-org/gitaly/v16/internal/testhelper" -) - -// DefaultAuthor is the author used by BuildCommit -var DefaultAuthor = git.Signature{ - Name: gittest.DefaultCommitterName, - Email: gittest.DefaultCommitterMail, - When: gittest.DefaultCommitTime, -} - -func TestMain(m *testing.M) { - testhelper.Run(m, testhelper.WithSetup(func() error { - // We use Git2go to access repositories in our tests, so we must tell it to ignore - // any configuration files that happen to exist. We do the same in `main()`, so - // this is not only specific to tests. - for _, configLevel := range []git.ConfigLevel{ - git.ConfigLevelSystem, - git.ConfigLevelXDG, - git.ConfigLevelGlobal, - } { - if err := git.SetSearchPath(configLevel, "/dev/null"); err != nil { - return fmt.Errorf("setting Git2go search path: %w", err) - } - } - - return nil - })) -} - -func buildExecutor(tb testing.TB, cfg config.Cfg) *git2go.Executor { - return git2go.NewExecutor(cfg, gittest.NewCommandFactory(tb, cfg), config.NewLocator(cfg), testhelper.SharedLogger(tb)) -} diff --git a/cmd/gitaly-git2go/util.go b/cmd/gitaly-git2go/util.go deleted file mode 100644 index c76b9370c..000000000 --- a/cmd/gitaly-git2go/util.go +++ /dev/null @@ -1,32 +0,0 @@ -//go:build static && system_libgit2 - -package main - -import ( - "fmt" - - git "github.com/libgit2/git2go/v34" - "gitlab.com/gitlab-org/gitaly/v16/internal/git2go" -) - -func lookupCommit(repo *git.Repository, ref string) (*git.Commit, error) { - object, err := repo.RevparseSingle(ref) - switch { - case git.IsErrorCode(err, git.ErrorCodeNotFound): - return nil, git2go.CommitNotFoundError{Revision: ref} - case err != nil: - return nil, fmt.Errorf("lookup commit %q: %w", ref, err) - } - - peeled, err := object.Peel(git.ObjectCommit) - if err != nil { - return nil, fmt.Errorf("lookup commit %q: peel: %w", ref, err) - } - - commit, err := peeled.AsCommit() - if err != nil { - return nil, fmt.Errorf("lookup commit %q: as commit: %w", ref, err) - } - - return commit, nil -} diff --git a/cmd/gitaly-hooks/hooks_test.go b/cmd/gitaly-hooks/hooks_test.go index 934f1a3ce..25cd7937f 100644 --- a/cmd/gitaly-hooks/hooks_test.go +++ b/cmd/gitaly-hooks/hooks_test.go @@ -826,9 +826,10 @@ remote: error executing git hook var stderr, stdout bytes.Buffer gittest.ExecOpts(t, cfg, gittest.ExecConfig{ - Env: envForHooks(t, ctx, cfg, repo, glHookValues{GitalyLogDir: logDir}, proxyValues{}), - Stdout: &stdout, - Stderr: &stderr, + Env: envForHooks(t, ctx, cfg, repo, glHookValues{GitalyLogDir: logDir}, proxyValues{}), + Stdout: &stdout, + Stderr: &stderr, + ExpectedExitCode: 128, }, args...) require.Equal(t, "", stdout.String()) diff --git a/doc/beginners_guide.md b/doc/beginners_guide.md index 0fbed62ca..4de508911 100644 --- a/doc/beginners_guide.md +++ b/doc/beginners_guide.md @@ -75,7 +75,6 @@ To enable linting in your code editor: 1. Run `make lint` at least once. That builds a version of `golangci-lint` for you. 1. Point your code editor or code editor's plugin to the binary at `_build/tools/golangci-lint`. -1. If necessary, add `_build/deps/libgit2/install/lib/pkgconfig` to your `PKG_CONFIG_PATH` environment variable. #### Experimenting with editing code diff --git a/doc/serverside_git_usage.md b/doc/serverside_git_usage.md index 889dc0035..823191bc6 100644 --- a/doc/serverside_git_usage.md +++ b/doc/serverside_git_usage.md @@ -1,9 +1,8 @@ # Server side Git usage -Gitaly uses three implementations to read and write to Git repositories: +Gitaly uses two implementations to read and write to Git repositories: 1. `git(1)` - The same Git used by clients all over the world -1. [LibGit2](https://github.com/libgit2/libgit2) - a linkable library used through Rugged and Git2Go 1. On ad-hoc basis, part of Git is implemented in this repository if the implementation is easy and stable. For example the [pktline](../internal/git/pktline) package. @@ -39,10 +38,3 @@ interfaces. These make sure Gitaly is protected against command injection, the correct `git` is used, and correct setup for observable command invocations are used. When working with `git(1)` in Ruby, please be sure to read the [Ruby shell scripting guide](https://docs.gitlab.com/ee/development/shell_commands.html). - -## Using LibGit2 - -Gitaly uses [Git2Go](https://github.com/libgit2/git2go) for Golang, and -[Rugged](https://github.com/libgit2/rugged) which both are thin adapters to call -the C functions of LibGit2. Git2Go is always invoked through `cmd/gitaly-git2go` -to mitigate issues with context cancellation and the potential for memory leaks. @@ -24,12 +24,11 @@ require ( github.com/hashicorp/yamux v0.1.2-0.20220728231024-8f49b6f63f18 github.com/jackc/pgx/v5 v5.4.3 github.com/kelseyhightower/envconfig v1.4.0 - github.com/libgit2/git2go/v34 v34.0.0 github.com/miekg/dns v1.1.55 github.com/olekukonko/tablewriter v0.0.5 github.com/opencontainers/runtime-spec v1.1.0 github.com/opentracing/opentracing-go v1.2.0 - github.com/pelletier/go-toml/v2 v2.0.9 + github.com/pelletier/go-toml/v2 v2.1.0 github.com/prometheus/client_golang v1.16.0 github.com/rubenv/sql-migrate v1.5.2 github.com/sirupsen/logrus v1.9.3 @@ -38,11 +37,11 @@ require ( github.com/urfave/cli/v2 v2.25.7 gitlab.com/gitlab-org/labkit v1.20.0 go.uber.org/goleak v1.2.1 - gocloud.dev v0.33.0 + gocloud.dev v0.34.0 golang.org/x/crypto v0.12.0 golang.org/x/exp v0.0.0-20230817173708-d852ddb80c63 golang.org/x/sync v0.3.0 - golang.org/x/sys v0.11.0 + golang.org/x/sys v0.12.0 golang.org/x/text v0.13.0 golang.org/x/time v0.3.0 google.golang.org/genproto/googleapis/rpc v0.0.0-20230822172742-b8732ec3820d @@ -365,8 +365,6 @@ github.com/google/pprof v0.0.0-20230705174524-200ffdc848b8/go.mod h1:Jh3hGz2jkYa github.com/google/renameio v0.1.0/go.mod h1:KWCgfxg9yswjAJkECMjeO8J8rahYeXnNhOm40UhjYkI= github.com/google/s2a-go v0.1.4 h1:1kZ/sQM3srePvKs3tXAvQzo66XfcReoqFpIpIccE7Oc= github.com/google/s2a-go v0.1.4/go.mod h1:Ej+mSEMGRnqRzjc7VtF+jdBwYG5fuJfiZ8ELkjEwM0A= -github.com/google/shlex v0.0.0-20191202100458-e7afc7fbc510 h1:El6M4kTTCOh6aBiKaUGG7oYTSPP8MxqL4YI3kZKwcP4= -github.com/google/shlex v0.0.0-20191202100458-e7afc7fbc510/go.mod h1:pupxD2MaaD3pAXIBCelhxNneeOaAeabZDe5s4K6zSpQ= github.com/google/subcommands v1.0.1/go.mod h1:ZjhPrFU+Olkh9WazFPsl27BQ4UPiG37m3yTrtFlrHVk= github.com/google/uuid v1.1.2/go.mod h1:TIyPZe4MgqvfeYDBFedMoGGpEw/LqOeaOT+nhxU+yHo= github.com/google/uuid v1.3.1 h1:KjJaJ9iWZ3jOFZIf1Lqf4laDRCasjl0BCmnEGxkdLb4= @@ -458,8 +456,6 @@ github.com/kylelemons/godebug v1.1.0/go.mod h1:9/0rRGxNHcop5bhtWyNeEfOS8JIWk580+ github.com/leonelquinteros/gotext v1.5.0 h1:ODY7LzLpZWWSJdAHnzhreOr6cwLXTAmc914FOauSkBM= github.com/leonelquinteros/gotext v1.5.0/go.mod h1:OCiUVHuhP9LGFBQ1oAmdtNCHJCiHiQA8lf4nAifHkr0= github.com/lib/pq v1.10.9 h1:YXG7RB+JIjhP29X+OtkiDnYaXQwpS4JEWq7dtCCRUEw= -github.com/libgit2/git2go/v34 v34.0.0 h1:UKoUaKLmiCRbOCD3PtUi2hD6hESSXzME/9OUZrGcgu8= -github.com/libgit2/git2go/v34 v34.0.0/go.mod h1:blVco2jDAw6YTXkErMMqzHLcAjKkwF0aWIRHBqiJkZ0= github.com/lightstep/lightstep-tracer-common/golang/gogo v0.0.0-20210210170715-a8dfcb80d3a7 h1:YjW+hUb8Fh2S58z4av4t/0cBMK/Q0aP48RocCFsC8yI= github.com/lightstep/lightstep-tracer-common/golang/gogo v0.0.0-20210210170715-a8dfcb80d3a7/go.mod h1:Spd59icnvRxSKuyijbbwe5AemzvcyXAUBgApa7VybMw= github.com/lightstep/lightstep-tracer-go v0.25.0 h1:sGVnz8h3jTQuHKMbUe2949nXm3Sg09N1UcR3VoQNN5E= @@ -506,8 +502,8 @@ github.com/opentracing/opentracing-go v1.1.0/go.mod h1:UkNAQd3GIcIGf0SeVgPpRdFSt github.com/opentracing/opentracing-go v1.2.0 h1:uEJPy/1a5RIPAJ0Ov+OIO8OxWu77jEv+1B0VhjKrZUs= github.com/opentracing/opentracing-go v1.2.0/go.mod h1:GxEUsuufX4nBwe+T+Wl9TAgYrxe9dPLANfrWvHYVTgc= github.com/pborman/getopt v0.0.0-20170112200414-7148bc3a4c30/go.mod h1:85jBQOZwpVEaDAr341tbn15RS4fCAsIst0qp7i8ex1o= -github.com/pelletier/go-toml/v2 v2.0.9 h1:uH2qQXheeefCCkuBBSLi7jCiSmj3VRh2+Goq2N7Xxu0= -github.com/pelletier/go-toml/v2 v2.0.9/go.mod h1:tJU2Z3ZkXwnxa4DPO899bsyIoywizdUvyaeZurnPPDc= +github.com/pelletier/go-toml/v2 v2.1.0 h1:FnwAJ4oYMvbT/34k9zzHuZNrhlz48GB3/s6at6/MHO4= +github.com/pelletier/go-toml/v2 v2.1.0/go.mod h1:tJU2Z3ZkXwnxa4DPO899bsyIoywizdUvyaeZurnPPDc= github.com/philhofer/fwd v1.1.1 h1:GdGcTjf5RNAxwS4QLsiMzJYj5KEvPJD3Abr261yRQXQ= github.com/philhofer/fwd v1.1.1/go.mod h1:gk3iGcWd9+svBvR0sR+KPcfE+RNWozjowpeBVG3ZVNU= github.com/pingcap/errors v0.11.4 h1:lFuQV/oaUMGcD2tqt+01ROSmJs75VG1ToEOkZIZ4nE4= @@ -626,8 +622,8 @@ go.uber.org/goleak v1.2.1 h1:NBol2c7O1ZokfZ0LEU9K6Whx/KnwvepVetCUhtKja4A= go.uber.org/goleak v1.2.1/go.mod h1:qlT2yGI9QafXHhZZLxlSuNsMw3FFLxBr+tBRlmO1xH4= go.uber.org/multierr v1.6.0/go.mod h1:cdWPpRnG4AhwMwsgIHip0KRBQjJy5kYEpYjJxpXp9iU= go.uber.org/zap v1.18.1/go.mod h1:xg/QME4nWcxGxrpdeYfq7UvYrLh66cuVKdrbD1XF/NI= -gocloud.dev v0.33.0 h1:ET5z49jm1+eUhY5BkuGk2d7czfgGeXKd4vtg1Jcg9OQ= -gocloud.dev v0.33.0/go.mod h1:z6W8qorjrfM09H8t1MDk8KLPj3Xi26aFBzDKAHWIgLU= +gocloud.dev v0.34.0 h1:LzlQY+4l2cMtuNfwT2ht4+fiXwWf/NmPTnXUlLmGif4= +gocloud.dev v0.34.0/go.mod h1:psKOachbnvY3DAOPbsFVmLIErwsbWPUG2H5i65D38vE= golang.org/x/crypto v0.0.0-20190219172222-a4c6cb3142f2/go.mod h1:6SG95UA2DQfeDnfUPMdvaQW0Q7yPrPDi9nlGo2tz2b4= golang.org/x/crypto v0.0.0-20190308221718-c2843e01d9a2/go.mod h1:djNgcEr1/C05ACkg1iLfiJU5Ep61QUkGW8qpdssI0+w= golang.org/x/crypto v0.0.0-20190510104115-cbcb75029529/go.mod h1:yigFU9vqHzYiE8UmvKecakEJjdnWj3jj499lnFckfCI= @@ -635,7 +631,6 @@ golang.org/x/crypto v0.0.0-20190605123033-f99c8df09eb5/go.mod h1:yigFU9vqHzYiE8U golang.org/x/crypto v0.0.0-20191011191535-87dc89f01550/go.mod h1:yigFU9vqHzYiE8UmvKecakEJjdnWj3jj499lnFckfCI= golang.org/x/crypto v0.0.0-20200622213623-75b288015ac9/go.mod h1:LzIPMQfyMNhhGPhUkYOs5KpL4U8rLKemX1yGLhDgUto= golang.org/x/crypto v0.0.0-20201112155050-0c6587e931a9/go.mod h1:LzIPMQfyMNhhGPhUkYOs5KpL4U8rLKemX1yGLhDgUto= -golang.org/x/crypto v0.0.0-20201203163018-be400aefbc4c/go.mod h1:jdWPYTVW3xRLrWPugEBEK3UY2ZEsg3UU495nc5E+M+I= golang.org/x/crypto v0.0.0-20210322153248-0c34fe9e7dc2/go.mod h1:T9bdIzuCu7OtxOm1hfPfRQxPLYneinmdGuTeoZ9dtd4= golang.org/x/crypto v0.0.0-20210421170649-83a5a9bb288b/go.mod h1:T9bdIzuCu7OtxOm1hfPfRQxPLYneinmdGuTeoZ9dtd4= golang.org/x/crypto v0.0.0-20210921155107-089bfa567519/go.mod h1:GvvjBRRGRdwPK5ydBHafDWAxML/pGHZbMvKqRZ5+Abc= @@ -836,9 +831,8 @@ golang.org/x/sys v0.6.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= golang.org/x/sys v0.8.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= golang.org/x/sys v0.9.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= golang.org/x/sys v0.10.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= -golang.org/x/sys v0.11.0 h1:eG7RXZHdqOJ1i+0lgLgCpSXAp6M3LYlAo6osgSi0xOM= -golang.org/x/sys v0.11.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= -golang.org/x/term v0.0.0-20201117132131-f5c789dd3221/go.mod h1:Nr5EML6q2oocZ2LXRh80K7BxOlk5/8JxuGnuhpl+muw= +golang.org/x/sys v0.12.0 h1:CM0HF96J0hcLAwsHPJZjfdNzs0gftsLfgKt57wWHJ0o= +golang.org/x/sys v0.12.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= golang.org/x/term v0.0.0-20201126162022-7de9c90e9dd1/go.mod h1:bj7SfCRtBDWHUb9snDiAeCFNEtKQo2Wmx5Cou7ajbmo= golang.org/x/term v0.0.0-20210927222741-03fcf44c2211/go.mod h1:jbD1KX2456YbFQfuXm/mYQcufACuNUgVhRMnK/tPxf8= golang.org/x/term v0.1.0/go.mod h1:jbD1KX2456YbFQfuXm/mYQcufACuNUgVhRMnK/tPxf8= diff --git a/internal/archive/archive.go b/internal/archive/archive.go index 20735edfe..4667f9ae0 100644 --- a/internal/archive/archive.go +++ b/internal/archive/archive.go @@ -1,14 +1,43 @@ package archive import ( + "archive/tar" "context" + "errors" "fmt" "io" + "io/fs" + "os" + "path/filepath" "runtime" + "strings" "gitlab.com/gitlab-org/gitaly/v16/internal/command" + "golang.org/x/exp/slices" ) +const ( + // readDirEntriesPageSize is an amount of fs.DirEntry(s) to read + // from the opened file descriptor of the directory. + readDirEntriesPageSize = 32 + + // Below is a set of flags used to decide what to do with the file/directory + // found on the disk. + // decisionWrite means that the file/directory needs to be added to the archive + decisionWrite = iota + // decisionSkip means that the file/directory shouldn't be added to the archive + decisionSkip + // decisionStop means that nothing else left to be added to the archive + decisionStop + + // tarFormat is a format to use for the archived files/directories/etc. + // The decision is made to use it as it is the latest version of the format. + // It allows sparse files, long file names, etc. that older formats doesn't support. + tarFormat = tar.FormatPAX +) + +type decider func(string) int + // WriteTarball writes a tarball to an `io.Writer` for the provided path // containing the specified archive members. Members should be specified // relative to `path`. @@ -32,3 +61,212 @@ func WriteTarball(ctx context.Context, writer io.Writer, path string, members .. return nil } + +// writeTarball creates a tar archive by flushing data into writer. +// Files and folders to be included into archive needs to be provided as members. +// If entry from members slice points to a folder all content inside it will be archived. +// The empty folders will be archived as well. +// It doesn't support sparse files efficiently, see https://github.com/golang/go/issues/22735 +func writeTarball(ctx context.Context, writer io.Writer, path string, members ...string) error { + // The function decides what to do with the entry: write, skip or finish archive + // creation if all members are added. + decide := func(candidate string) int { + if len(members) == 0 { + return decisionStop + } + + idx := slices.Index(members, candidate) + if idx < 0 { + return decisionSkip + } + + members = slices.Delete(members, idx, idx+1) + return decisionWrite + } + + archWriter := tar.NewWriter(writer) + if err := walkDir(ctx, archWriter, path, "", decide); err != nil { + return fmt.Errorf("walk dir: %w", err) + } + + if err := archWriter.Close(); err != nil { + return fmt.Errorf("closing archive writer: %w", err) + } + + return nil +} + +func walkDir(ctx context.Context, archWriter *tar.Writer, path, currentPrefix string, decide decider) error { + if cancelled(ctx) { + return ctx.Err() + } + + dir, err := os.Open(path) + if err != nil { + return fmt.Errorf("open dir: %w", err) + } + defer func() { _ = dir.Close() }() + + dirStat, err := dir.Stat() + if err != nil { + return fmt.Errorf("dir description: %w", err) + } + + if dirStat.Mode().Type() != fs.ModeDir { + return errors.New("is not a dir") + } + + for { + // Own implementation of the directory walker is used for optimization. + // fs.WalkDir() reads all the entries into memory at once and does the sorting. + // If there are a lot of files in the nested directories all that info will remain + // in the memory while the leaf is reached. In other words if there is 1000 files + // in the root and 1000 file in the nested directory of the root in one moment + // 2000 entries will remain in the memory for no reason. + dirEntries, err := dir.ReadDir(readDirEntriesPageSize) + if err != nil { + if errors.Is(err, io.EOF) { + return nil + } + + return fmt.Errorf("chunked dir read: %w", err) + } + + for _, dirEntry := range dirEntries { + if cancelled(ctx) { + return ctx.Err() + } + + entryName := dirEntry.Name() + entryRelativePath := filepath.Join(currentPrefix, entryName) + + decision := decide(entryRelativePath) + switch decision { + case decisionStop: + // Nothing needs to be added to the archive. + return nil + case decisionSkip: + // Current entry doesn't need to be added, but we need travers it if it is a directory. + if dirEntry.IsDir() { + if err := walkDir(ctx, archWriter, filepath.Join(path, entryName), entryRelativePath, decide); err != nil { + return fmt.Errorf("walk dir: %w", err) + } + } + case decisionWrite: + if dirEntry.IsDir() { + // As this is a dir we add all its content to the archive. + if err := tarDir(ctx, archWriter, filepath.Join(path, entryName), entryRelativePath, func(string) int { return decisionWrite }); err != nil { + return fmt.Errorf("write dir: %w", err) + } + continue + } + + if err := tarFile(archWriter, path, entryRelativePath); err != nil { + return fmt.Errorf("write file: %w", err) + } + default: + return fmt.Errorf("unhandled decision: %d", decision) + } + } + } +} + +func tarDir(ctx context.Context, archWriter *tar.Writer, path, currentPrefix string, decide decider) error { + // The current entry is a directory that needs to be added to the archive. + stat, err := os.Lstat(path) + if err != nil { + return err + } + + link := "" + if stat.Mode()&fs.ModeSymlink != 0 { + link, err = os.Readlink(path) + if err != nil { + return fmt.Errorf("read link destination: %w", err) + } + } + + header, err := tar.FileInfoHeader(stat, link) + if err != nil { + return fmt.Errorf("file info header: %w", err) + } + header.Name = strings.TrimSuffix(currentPrefix, "/") + "/" + header.Format = tarFormat + + if err := archWriter.WriteHeader(header); err != nil { + return fmt.Errorf("write file info header: %w", err) + } + + if err := walkDir(ctx, archWriter, path, currentPrefix, decide); err != nil { + return fmt.Errorf("walk dir: %w", err) + } + + return nil +} + +func tarFile(archWriter *tar.Writer, path, entryRelativePath string) error { + entryPath := filepath.Join(path, filepath.Base(entryRelativePath)) + + entryStat, err := os.Lstat(entryPath) + if err != nil { + return fmt.Errorf("file/link description: %w", err) + } + + if entryStat.Mode()&fs.ModeSymlink != 0 { + targetPath, err := os.Readlink(entryPath) + if err != nil { + return fmt.Errorf("read link destination: %w", err) + } + + header := &tar.Header{ + Typeflag: tar.TypeSymlink, + Name: entryRelativePath, + Linkname: targetPath, + Format: tarFormat, + Mode: int64(entryStat.Mode().Perm()), + } + + if err := archWriter.WriteHeader(header); err != nil { + return fmt.Errorf("write symlink file info header: %w", err) + } + + return nil + } + + curFile, err := os.Open(entryPath) + if err != nil { + return fmt.Errorf("open file: %w", err) + } + defer func() { _ = curFile.Close() }() + + curFileStat, err := curFile.Stat() + if err != nil { + return fmt.Errorf("file description: %w", err) + } + + header, err := tar.FileInfoHeader(curFileStat, curFileStat.Name()) + if err != nil { + return fmt.Errorf("file info header: %w", err) + } + header.Name = entryRelativePath + header.Format = tarFormat + + if err := archWriter.WriteHeader(header); err != nil { + return fmt.Errorf("write file info header: %w", err) + } + + if _, err := io.Copy(archWriter, curFile); err != nil { + return fmt.Errorf("copy file: %w", err) + } + + return nil +} + +func cancelled(ctx context.Context) bool { + select { + case <-ctx.Done(): + return true + default: + return false + } +} diff --git a/internal/archive/archive_test.go b/internal/archive/archive_test.go new file mode 100644 index 000000000..39c2abbed --- /dev/null +++ b/internal/archive/archive_test.go @@ -0,0 +1,93 @@ +package archive + +import ( + "fmt" + "os" + "os/exec" + "path/filepath" + "strings" + "testing" + + "github.com/stretchr/testify/require" + "gitlab.com/gitlab-org/gitaly/v16/internal/git/gittest" + "gitlab.com/gitlab-org/gitaly/v16/internal/helper/perm" + "gitlab.com/gitlab-org/gitaly/v16/internal/testhelper" + "gitlab.com/gitlab-org/gitaly/v16/internal/testhelper/testcfg" +) + +func TestWriteTarball(t *testing.T) { + t.Parallel() + + ctx := testhelper.Context(t) + tempDir := testhelper.TempDir(t) + srcDir := filepath.Join(tempDir, "src") + + // regular file + writeFile(t, filepath.Join(srcDir, "a.txt"), []byte("a")) + // empty dir + require.NoError(t, os.Mkdir(filepath.Join(srcDir, "empty_dir"), perm.PublicDir)) + // file with long name + writeFile(t, filepath.Join(srcDir, strings.Repeat("b", 150)+".txt"), []byte("b")) + // regular file that is not expected to be part of the archive (not in the members list) + writeFile(t, filepath.Join(srcDir, "excluded.txt"), []byte("excluded")) + // folder with multiple files all expected to be archived + nestedPath := filepath.Join(srcDir, "nested1") + for i := 0; i < readDirEntriesPageSize+1; i++ { + writeFile(t, filepath.Join(nestedPath, fmt.Sprintf("%d.txt", i)), []byte{byte(i)}) + } + // nested file that is not expected to be part of the archive + writeFile(t, filepath.Join(srcDir, "nested2/nested/nested/c.txt"), []byte("c")) + // deeply nested file + writeFile(t, filepath.Join(srcDir, "nested2/nested/nested/nested/nested/d.txt"), []byte("d")) + // file that is used to create a symbolic link, is not expected to be part of the archive + writeFile(t, filepath.Join(srcDir, "nested3/target.txt"), []byte("target")) + // link to the file above + require.NoError(t, os.Symlink(filepath.Join(srcDir, "nested3/target.txt"), filepath.Join(srcDir, "link.to.target.txt"))) + // directory that is a target of the symlink should not be archived + writeFile(t, filepath.Join(srcDir, "nested4/stub.txt"), []byte("symlinked")) + // link to the folder above + require.NoError(t, os.Symlink(filepath.Join(srcDir, "nested4"), filepath.Join(srcDir, "link.to.nested4"))) + + tarPath := filepath.Join(tempDir, "out.tar") + archFile, err := os.Create(tarPath) + require.NoError(t, err) + defer func() { _ = archFile.Close() }() + + err = writeTarball( + ctx, + archFile, + srcDir, + "a.txt", + strings.Repeat("b", 150)+".txt", + "nested1", + "nested2/nested/nested/nested/nested/d.txt", + "link.to.target.txt", + "link.to.nested4", + ) + require.NoError(t, err) + require.NoError(t, archFile.Close()) + + cfg := testcfg.Build(t) + + dstDir := filepath.Join(tempDir, "dst") + require.NoError(t, os.Mkdir(dstDir, perm.PublicDir)) + output, err := exec.Command("tar", "-xf", tarPath, "-C", dstDir).CombinedOutput() + require.NoErrorf(t, err, "%s", output) + diff := gittest.ExecOpts(t, cfg, gittest.ExecConfig{ExpectedExitCode: 1}, "diff", "--no-index", "--name-only", "--exit-code", dstDir, srcDir) + expected := strings.Join( + []string{ + filepath.Join(srcDir, "excluded.txt"), + filepath.Join(srcDir, "nested2/nested/nested/c.txt"), + filepath.Join(srcDir, "nested3/target.txt"), + filepath.Join(srcDir, "nested4/stub.txt\n"), + }, + "\n", + ) + require.Equal(t, expected, string(diff)) +} + +func writeFile(tb testing.TB, path string, data []byte) { + tb.Helper() + require.NoError(tb, os.MkdirAll(filepath.Dir(path), perm.PrivateDir)) + require.NoError(tb, os.WriteFile(path, data, perm.PrivateFile)) +} diff --git a/internal/git2go/testhelper_test.go b/internal/archive/testhelper_test.go index f4298b718..b8f9015cb 100644 --- a/internal/git2go/testhelper_test.go +++ b/internal/archive/testhelper_test.go @@ -1,4 +1,4 @@ -package git2go +package archive import ( "testing" diff --git a/internal/cli/gitaly/serve.go b/internal/cli/gitaly/serve.go index 825bb19d8..989c175c3 100644 --- a/internal/cli/gitaly/serve.go +++ b/internal/cli/gitaly/serve.go @@ -23,7 +23,6 @@ import ( "gitlab.com/gitlab-org/gitaly/v16/internal/git/catfile" "gitlab.com/gitlab-org/gitaly/v16/internal/git/housekeeping" "gitlab.com/gitlab-org/gitaly/v16/internal/git/localrepo" - "gitlab.com/gitlab-org/gitaly/v16/internal/git2go" "gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/config" "gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/config/sentry" "gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/hook" @@ -320,8 +319,6 @@ func run(cfg config.Cfg, logger logrus.FieldLogger) error { ) defer gitalyServerFactory.Stop() - git2goExecutor := git2go.NewExecutor(cfg, gitCmdFactory, locator, logger) - updaterWithHooks := updateref.NewUpdaterWithHooks(cfg, locator, hookManager, gitCmdFactory, catfileCache) streamCache := streamcache.New(cfg.PackObjectsCache, logger) @@ -375,7 +372,6 @@ func run(cfg config.Cfg, logger logrus.FieldLogger) error { PackObjectsCache: streamCache, PackObjectsLimiter: packObjectsLimiter, RepositoryCounter: repoCounter, - Git2goExecutor: git2goExecutor, UpdaterWithHooks: updaterWithHooks, HousekeepingManager: housekeepingManager, BackupSink: backupSink, diff --git a/internal/git/gittest/command.go b/internal/git/gittest/command.go index 7fd44151b..862c7822b 100644 --- a/internal/git/gittest/command.go +++ b/internal/git/gittest/command.go @@ -26,6 +26,9 @@ type ExecConfig struct { // Env contains environment variables that should be appended to the spawned command's // environment. Env []string + // ExpectedExitCode is used to check the resulting exit code of the command. This can be used in case a command + // is expected to return an error code. + ExpectedExitCode int } // Exec runs a git command and returns the standard output, or fails. @@ -59,13 +62,18 @@ func ExecOpts(tb testing.TB, cfg config.Cfg, execCfg ExecConfig, args ...string) } func handleExecErr(tb testing.TB, cfg config.Cfg, execCfg ExecConfig, args []string, err error) { - if execCfg.Stderr == nil { - tb.Log(cfg.Git.BinPath, args) - if ee, ok := err.(*exec.ExitError); ok { - tb.Logf("%s\n", ee.Stderr) + var stderr []byte + if ee, ok := err.(*exec.ExitError); ok { + if execCfg.ExpectedExitCode == ee.ExitCode() { + return } - tb.Fatal(err) + stderr = ee.Stderr } + tb.Log(cfg.Git.BinPath, args) + if len(stderr) > 0 { + tb.Logf("%s\n", stderr) + } + tb.Fatal(err) } // NewCommand creates a new Git command ready for execution. diff --git a/internal/git/localrepo/remote_extra_test.go b/internal/git/localrepo/remote_extra_test.go index 0dcd2188e..4e4f693ce 100644 --- a/internal/git/localrepo/remote_extra_test.go +++ b/internal/git/localrepo/remote_extra_test.go @@ -48,7 +48,6 @@ func TestRepo_FetchInternal(t *testing.T) { deps.GetGitCmdFactory(), deps.GetCatfileCache(), deps.GetConnsPool(), - deps.GetGit2goExecutor(), deps.GetHousekeepingManager(), deps.GetBackupSink(), deps.GetBackupLocator(), diff --git a/internal/git/packfile/packfile_test.go b/internal/git/packfile/packfile_test.go index 7605db79d..1c87407fa 100644 --- a/internal/git/packfile/packfile_test.go +++ b/internal/git/packfile/packfile_test.go @@ -11,10 +11,6 @@ import ( "gitlab.com/gitlab-org/gitaly/v16/internal/testhelper/testcfg" ) -func TestMain(m *testing.M) { - testhelper.Run(m) -} - func TestList(t *testing.T) { t.Parallel() diff --git a/internal/git/packfile/testhelper_test.go b/internal/git/packfile/testhelper_test.go new file mode 100644 index 000000000..5c11ab605 --- /dev/null +++ b/internal/git/packfile/testhelper_test.go @@ -0,0 +1,11 @@ +package packfile_test + +import ( + "testing" + + "gitlab.com/gitlab-org/gitaly/v16/internal/testhelper" +) + +func TestMain(m *testing.M) { + testhelper.Run(m) +} diff --git a/internal/git/remoterepo/testhelper_test.go b/internal/git/remoterepo/testhelper_test.go index ebac4880c..2a9b0ce9b 100644 --- a/internal/git/remoterepo/testhelper_test.go +++ b/internal/git/remoterepo/testhelper_test.go @@ -30,7 +30,6 @@ func setupGitalyServer(t *testing.T) config.Cfg { deps.GetGitCmdFactory(), deps.GetCatfileCache(), deps.GetConnsPool(), - deps.GetGit2goExecutor(), deps.GetHousekeepingManager(), deps.GetBackupSink(), deps.GetBackupLocator(), diff --git a/internal/git2go/executor.go b/internal/git2go/executor.go deleted file mode 100644 index f5cca8252..000000000 --- a/internal/git2go/executor.go +++ /dev/null @@ -1,142 +0,0 @@ -package git2go - -import ( - "bytes" - "context" - "encoding/gob" - "errors" - "fmt" - "io" - "strings" - - "github.com/sirupsen/logrus" - "gitlab.com/gitlab-org/gitaly/v16/internal/command" - "gitlab.com/gitlab-org/gitaly/v16/internal/featureflag" - "gitlab.com/gitlab-org/gitaly/v16/internal/git" - "gitlab.com/gitlab-org/gitaly/v16/internal/git/alternates" - "gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/config" - "gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/storage" - "gitlab.com/gitlab-org/labkit/correlation" -) - -var ( - // ErrInvalidArgument is returned in case the merge arguments are invalid. - ErrInvalidArgument = errors.New("invalid parameters") - - // BinaryName is the name of the gitaly-git2go binary. - BinaryName = "gitaly-git2go" -) - -// Executor executes gitaly-git2go. -type Executor struct { - binaryPath string - signingKey string - gitCmdFactory git.CommandFactory - locator storage.Locator - logger logrus.FieldLogger - logFormat, logLevel string -} - -// NewExecutor returns a new gitaly-git2go executor using binaries as configured in the given -// configuration. -func NewExecutor(cfg config.Cfg, gitCmdFactory git.CommandFactory, locator storage.Locator, logger logrus.FieldLogger) *Executor { - return &Executor{ - binaryPath: cfg.BinaryPath(BinaryName), - signingKey: cfg.Git.SigningKey, - gitCmdFactory: gitCmdFactory, - locator: locator, - logger: logger, - logFormat: cfg.Logging.Format, - logLevel: cfg.Logging.Level, - } -} - -func (b *Executor) run(ctx context.Context, repo storage.Repository, stdin io.Reader, subcmd string, args ...string) (*bytes.Buffer, error) { - repoPath, err := b.locator.GetRepoPath(repo) - if err != nil { - return nil, fmt.Errorf("gitaly-git2go: %w", err) - } - - var enabledFeatureFlags, disabledFeatureFlags []string - - for flag, value := range featureflag.FromContext(ctx) { - switch value { - case true: - enabledFeatureFlags = append(enabledFeatureFlags, flag.MetadataKey()) - case false: - disabledFeatureFlags = append(disabledFeatureFlags, flag.MetadataKey()) - } - } - - env := alternates.Env(repoPath, repo.GetGitObjectDirectory(), repo.GetGitAlternateObjectDirectories()) - env = append(env, b.gitCmdFactory.GetExecutionEnvironment(ctx).EnvironmentVariables...) - - // Construct a log writer that we pass to gitaly-git2go. Note that this is not exactly pretty: the output - // generated by the logger is not going to be parsed, but will be written to the log without any change as log - // message. - // - // Previously, we would have written these messages to `logrus.Logger.Out` directly. But we have refactored our - // code to get rid of the global logger instance, and thus we cannot access that field anymore. Given that the - // git2go executor is going away next release anyway, let's just live with this ugly workaround here. We still - // have visibility into what the process is doing, but with a bit less comfort. That seems like an acceptable - // tradeoff in order to get rid of the global logger altogether. - logWriter := b.logger.WithField("component", "git2go."+subcmd).Writer() - defer logWriter.Close() - - args = append([]string{ - "-log-format", b.logFormat, - "-log-level", b.logLevel, - "-correlation-id", correlation.ExtractFromContext(ctx), - "-enabled-feature-flags", strings.Join(enabledFeatureFlags, ","), - "-disabled-feature-flags", strings.Join(disabledFeatureFlags, ","), - subcmd, - }, args...) - - var stdout bytes.Buffer - cmd, err := command.New(ctx, append([]string{b.binaryPath}, args...), - command.WithStdin(stdin), - command.WithStdout(&stdout), - command.WithStderr(logWriter), - command.WithEnvironment(env), - command.WithCommandName("gitaly-git2go", subcmd), - ) - if err != nil { - return nil, err - } - - if err := cmd.Wait(); err != nil { - return nil, err - } - - return &stdout, nil -} - -// runWithGob runs the specified gitaly-git2go cmd with the request gob-encoded -// as input and returns the commit ID as string or an error. -func (b *Executor) runWithGob(ctx context.Context, repo storage.Repository, cmd string, request interface{}) (git.ObjectID, error) { - input := &bytes.Buffer{} - if err := gob.NewEncoder(input).Encode(request); err != nil { - return "", fmt.Errorf("%s: %w", cmd, err) - } - - output, err := b.run(ctx, repo, input, cmd) - if err != nil { - return "", fmt.Errorf("%s: %w", cmd, err) - } - - var result Result - if err := gob.NewDecoder(output).Decode(&result); err != nil { - return "", fmt.Errorf("%s: %w", cmd, err) - } - - if result.Err != nil { - return "", fmt.Errorf("%s: %w", cmd, result.Err) - } - - commitID, err := git.ObjectHashSHA1.FromHex(result.CommitID) - if err != nil { - return "", fmt.Errorf("could not parse commit ID: %w", err) - } - - return commitID, nil -} diff --git a/internal/git2go/featureflags.go b/internal/git2go/featureflags.go deleted file mode 100644 index bfde6867b..000000000 --- a/internal/git2go/featureflags.go +++ /dev/null @@ -1,22 +0,0 @@ -package git2go - -// FeatureFlag is a feature flag state as seen by the `gitaly-git2go featureflag` test subcommand. -type FeatureFlag struct { - // MetadataKey is the metadata key of the feature flag. - MetadataKey string - // Name is the name of the feature flag. - Name string - // Value is the value of the feature flag. - Value bool -} - -// FeatureFlags is a struct only used by tests to confirm that feature flags are -// being properly propagated from the git2go Executor to the gitaly-git2go -// binary -type FeatureFlags struct { - // Flags is the set of feature flags observed by the command. - Flags []FeatureFlag - // Err is set if an error occurred. Err must exist on all gob serialized - // results so that any error can be returned. - Err error -} diff --git a/internal/git2go/featureflags_test.go b/internal/git2go/featureflags_test.go deleted file mode 100644 index db2a35992..000000000 --- a/internal/git2go/featureflags_test.go +++ /dev/null @@ -1,72 +0,0 @@ -package git2go - -import ( - "context" - "encoding/gob" - "testing" - - "github.com/stretchr/testify/require" - "gitlab.com/gitlab-org/gitaly/v16/internal/featureflag" - "gitlab.com/gitlab-org/gitaly/v16/internal/git/gittest" - "gitlab.com/gitlab-org/gitaly/v16/internal/git/localrepo" - "gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/config" - "gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/storage" - "gitlab.com/gitlab-org/gitaly/v16/internal/testhelper" - "gitlab.com/gitlab-org/gitaly/v16/internal/testhelper/testcfg" -) - -func (b *Executor) FeatureFlags(ctx context.Context, repo storage.Repository) ([]FeatureFlag, error) { - output, err := b.run(ctx, repo, nil, "feature-flags") - if err != nil { - return nil, err - } - - var result FeatureFlags - if err := gob.NewDecoder(output).Decode(&result); err != nil { - return nil, err - } - - if result.Err != nil { - return nil, result.Err - } - - return result.Flags, err -} - -var ( - featureA = featureflag.NewFeatureFlag("feature_a", "", "", false) - featureB = featureflag.NewFeatureFlag("feature_b", "", "", true) -) - -func TestExecutor_explicitFeatureFlags(t *testing.T) { - testhelper.NewFeatureSets(featureA, featureB).Run(t, testExecutorFeatureFlags) -} - -func testExecutorFeatureFlags(t *testing.T, ctx context.Context) { - cfg := testcfg.Build(t) - testcfg.BuildGitalyGit2Go(t, cfg) - - repoProto, _ := gittest.CreateRepository(t, ctx, cfg, gittest.CreateRepositoryConfig{ - SkipCreationViaService: true, - }) - - repo := localrepo.NewTestRepo(t, cfg, repoProto) - - executor := NewExecutor(cfg, gittest.NewCommandFactory(t, cfg), config.NewLocator(cfg), testhelper.SharedLogger(t)) - - flags, err := executor.FeatureFlags(ctx, repo) - require.NoError(t, err) - - require.Subset(t, flags, []FeatureFlag{ - { - Name: "feature_a", - MetadataKey: "gitaly-feature-feature-a", - Value: featureA.IsEnabled(ctx), - }, - { - Name: "feature_b", - MetadataKey: "gitaly-feature-feature-b", - Value: featureB.IsEnabled(ctx), - }, - }, flags) -} diff --git a/internal/git2go/rebase.go b/internal/git2go/rebase.go deleted file mode 100644 index 3bdc0e9e6..000000000 --- a/internal/git2go/rebase.go +++ /dev/null @@ -1,42 +0,0 @@ -package git2go - -import ( - "context" - - "gitlab.com/gitlab-org/gitaly/v16/internal/git" - "gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/storage" -) - -// RebaseCommand contains parameters to rebase a branch. -type RebaseCommand struct { - // Repository is the path to execute rebase in. - Repository string - // Committer contains the the committer signature. - Committer Signature - // BranchName is the branch that is rebased. Deprecated, can be removed in the next release. - BranchName string - // UpstreamRevision is the revision where the branch is rebased onto. Deprecated, can be - // removed in the next release. - UpstreamRevision string - // CommitID is the object ID of the commit that shall be rebased. Deprecates BranchName. - CommitID git.ObjectID - // UpstreamCommitID is the object ID of the commit which is considered to be the - // upstream branch. This parameter determines both the commit onto which we're - // about to rebase, which is the merge base of the upstream commit and rebased - // commit, and which commits should be rebased, which is the commit range - // upstream..commit. Deprecates the UpstreamRevision. - UpstreamCommitID git.ObjectID - // SkipEmptyCommits will cause commits which have already been applied on the target branch - // and which are thus empty to be skipped. If unset, empty commits will cause the rebase to - // fail. - SkipEmptyCommits bool - // SigningKey is a path to the key to sign commit using OpenPGP - SigningKey string -} - -// Rebase performs the rebase via gitaly-git2go -func (b *Executor) Rebase(ctx context.Context, repo storage.Repository, r RebaseCommand) (git.ObjectID, error) { - r.SigningKey = b.signingKey - - return b.runWithGob(ctx, repo, "rebase", r) -} diff --git a/internal/git2go/serialization.go b/internal/git2go/serialization.go deleted file mode 100644 index a1454e916..000000000 --- a/internal/git2go/serialization.go +++ /dev/null @@ -1,102 +0,0 @@ -package git2go - -import ( - "encoding/gob" - "errors" - "fmt" - "reflect" -) - -func init() { - for typeToRegister := range registeredTypes { - gob.Register(reflect.Zero(typeToRegister).Interface()) - } -} - -var registeredTypes = map[reflect.Type]struct{}{ - reflect.TypeOf(wrapError{}): {}, - reflect.TypeOf(HasConflictsError{}): {}, - reflect.TypeOf(ConflictingFilesError{}): {}, - reflect.TypeOf(EmptyError{}): {}, - reflect.TypeOf(CommitNotFoundError{}): {}, -} - -// Result is the serialized result. -type Result struct { - // CommitID is the result of the call. - CommitID string - // Err is set if an error occurred. Err must exist on all gob serialized - // results so that all errors can be returned. - Err error -} - -// wrapError is used to serialize wrapped errors as fmt.wrapError type only has -// private fields and can't be serialized via gob. It's also used to serialize unregistered -// error types by serializing only their error message. -type wrapError struct { - Message string - Err error -} - -func (err wrapError) Error() string { return err.Message } - -func (err wrapError) Unwrap() error { return err.Err } - -// HasConflictsError is used when a change, for example a revert, could not be -// applied due to a conflict. -type HasConflictsError struct{} - -func (err HasConflictsError) Error() string { - return "could not apply due to conflicts" -} - -// ConflictingFilesError is an error raised when there are conflicting files. -type ConflictingFilesError struct { - // ConflictingFiles is the set of files which have conflicts. - ConflictingFiles []string -} - -func (err ConflictingFilesError) Error() string { - return "there are conflicting files" -} - -// EmptyError indicates the command, for example cherry-pick, did result in no -// changes, so the result is empty. -type EmptyError struct{} - -func (err EmptyError) Error() string { - return "could not apply because the result was empty" -} - -// CommitNotFoundError indicates that the given commit rev could not be found. -type CommitNotFoundError struct { - // Revision used to lookup the commit - Revision string -} - -func (err CommitNotFoundError) Error() string { - return fmt.Sprintf("commit not found: %q", err.Revision) -} - -// SerializableError returns an error that is Gob serializable. -// Registered types are serialized directly. Unregistered types -// are transformed in to an opaque error using their error message. -// Wrapped errors remain unwrappable. -func SerializableError(err error) error { - if err == nil { - return nil - } - - if unwrappedErr := errors.Unwrap(err); unwrappedErr != nil { - return wrapError{ - Message: err.Error(), - Err: SerializableError(unwrappedErr), - } - } - - if _, ok := registeredTypes[reflect.TypeOf(err)]; !ok { - return wrapError{Message: err.Error()} - } - - return err -} diff --git a/internal/git2go/signature.go b/internal/git2go/signature.go deleted file mode 100644 index 1d66ed5e0..000000000 --- a/internal/git2go/signature.go +++ /dev/null @@ -1,9 +0,0 @@ -package git2go - -import "gitlab.com/gitlab-org/gitaly/v16/internal/git" - -// Signature represents a commits signature, synced from internal/git. -type Signature = git.Signature - -// NewSignature creates a new sanitized signature, syned from internal/git. -var NewSignature = git.NewSignature diff --git a/internal/gitaly/config/config.go b/internal/gitaly/config/config.go index 1b4b7bbe0..a938b7c0c 100644 --- a/internal/gitaly/config/config.go +++ b/internal/gitaly/config/config.go @@ -759,7 +759,6 @@ func validateIsDirectory(path, name string) error { var packedBinaries = map[string]struct{}{ "gitaly-hooks": {}, "gitaly-ssh": {}, - "gitaly-git2go": {}, "gitaly-lfs-smudge": {}, "gitaly-gpg": {}, } diff --git a/internal/gitaly/service/blob/testhelper_test.go b/internal/gitaly/service/blob/testhelper_test.go index bf76a55cc..4ca1a85ae 100644 --- a/internal/gitaly/service/blob/testhelper_test.go +++ b/internal/gitaly/service/blob/testhelper_test.go @@ -36,7 +36,6 @@ func setup(tb testing.TB, ctx context.Context) (config.Cfg, gitalypb.BlobService deps.GetGitCmdFactory(), deps.GetCatfileCache(), deps.GetConnsPool(), - deps.GetGit2goExecutor(), deps.GetHousekeepingManager(), deps.GetBackupSink(), deps.GetBackupLocator(), diff --git a/internal/gitaly/service/cleanup/testhelper_test.go b/internal/gitaly/service/cleanup/testhelper_test.go index 074a9052a..00f7a107a 100644 --- a/internal/gitaly/service/cleanup/testhelper_test.go +++ b/internal/gitaly/service/cleanup/testhelper_test.go @@ -54,7 +54,6 @@ func runCleanupServiceServer(t *testing.T, cfg config.Cfg) string { deps.GetGitCmdFactory(), deps.GetCatfileCache(), deps.GetConnsPool(), - deps.GetGit2goExecutor(), deps.GetHousekeepingManager(), deps.GetBackupSink(), deps.GetBackupLocator(), diff --git a/internal/gitaly/service/commit/testhelper_test.go b/internal/gitaly/service/commit/testhelper_test.go index d0b457d0e..44356057a 100644 --- a/internal/gitaly/service/commit/testhelper_test.go +++ b/internal/gitaly/service/commit/testhelper_test.go @@ -53,7 +53,6 @@ func startTestServices(tb testing.TB, cfg config.Cfg, opts ...testserver.GitalyS deps.GetGitCmdFactory(), deps.GetCatfileCache(), deps.GetConnsPool(), - deps.GetGit2goExecutor(), deps.GetHousekeepingManager(), deps.GetBackupSink(), deps.GetBackupLocator(), diff --git a/internal/gitaly/service/conflicts/testhelper_test.go b/internal/gitaly/service/conflicts/testhelper_test.go index 813826e52..1d1b2f6e8 100644 --- a/internal/gitaly/service/conflicts/testhelper_test.go +++ b/internal/gitaly/service/conflicts/testhelper_test.go @@ -51,7 +51,6 @@ func runConflictsServer(tb testing.TB, cfg config.Cfg, hookManager hook.Manager) deps.GetGitCmdFactory(), deps.GetCatfileCache(), deps.GetConnsPool(), - deps.GetGit2goExecutor(), deps.GetHousekeepingManager(), deps.GetBackupSink(), deps.GetBackupLocator(), diff --git a/internal/gitaly/service/dependencies.go b/internal/gitaly/service/dependencies.go index 664aeca96..d0f568af3 100644 --- a/internal/gitaly/service/dependencies.go +++ b/internal/gitaly/service/dependencies.go @@ -7,7 +7,6 @@ import ( "gitlab.com/gitlab-org/gitaly/v16/internal/git" "gitlab.com/gitlab-org/gitaly/v16/internal/git/catfile" "gitlab.com/gitlab-org/gitaly/v16/internal/git/housekeeping" - "gitlab.com/gitlab-org/gitaly/v16/internal/git2go" "gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/config" gitalyhook "gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/hook" "gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/hook/updateref" @@ -38,7 +37,6 @@ type Dependencies struct { PackObjectsLimiter limiter.Limiter LimitHandler *limithandler.LimiterMiddleware RepositoryCounter *counter.RepositoryCounter - Git2goExecutor *git2go.Executor UpdaterWithHooks *updateref.UpdaterWithHooks HousekeepingManager housekeeping.Manager PartitionManager *storagemgr.PartitionManager @@ -111,11 +109,6 @@ func (dc *Dependencies) GetRepositoryCounter() *counter.RepositoryCounter { return dc.RepositoryCounter } -// GetGit2goExecutor returns the git2go executor. -func (dc *Dependencies) GetGit2goExecutor() *git2go.Executor { - return dc.Git2goExecutor -} - // GetUpdaterWithHooks returns the updater with hooks executor. func (dc *Dependencies) GetUpdaterWithHooks() *updateref.UpdaterWithHooks { return dc.UpdaterWithHooks diff --git a/internal/gitaly/service/diff/raw_test.go b/internal/gitaly/service/diff/raw_test.go index e88d2210f..43ef42b73 100644 --- a/internal/gitaly/service/diff/raw_test.go +++ b/internal/gitaly/service/diff/raw_test.go @@ -10,7 +10,6 @@ import ( "gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/storage" "gitlab.com/gitlab-org/gitaly/v16/internal/structerr" "gitlab.com/gitlab-org/gitaly/v16/internal/testhelper" - "gitlab.com/gitlab-org/gitaly/v16/internal/testhelper/testcfg" "gitlab.com/gitlab-org/gitaly/v16/proto/go/gitalypb" "gitlab.com/gitlab-org/gitaly/v16/streamio" ) @@ -20,7 +19,6 @@ func TestRawDiff_successful(t *testing.T) { ctx := testhelper.Context(t) cfg, client := setupDiffService(t) - testcfg.BuildGitalyGit2Go(t, cfg) repoProto, repoPath := gittest.CreateRepository(t, ctx, cfg) oldBlob := gittest.WriteBlob(t, cfg, repoPath, []byte("old\n")) diff --git a/internal/gitaly/service/diff/testhelper_test.go b/internal/gitaly/service/diff/testhelper_test.go index e19294090..4bde0b629 100644 --- a/internal/gitaly/service/diff/testhelper_test.go +++ b/internal/gitaly/service/diff/testhelper_test.go @@ -35,7 +35,6 @@ func setupDiffService(tb testing.TB, opt ...testserver.GitalyServerOpt) (config. deps.GetGitCmdFactory(), deps.GetCatfileCache(), deps.GetConnsPool(), - deps.GetGit2goExecutor(), deps.GetHousekeepingManager(), deps.GetBackupSink(), deps.GetBackupLocator(), diff --git a/internal/gitaly/service/hook/testhelper_test.go b/internal/gitaly/service/hook/testhelper_test.go index 51404398e..95c2de53e 100644 --- a/internal/gitaly/service/hook/testhelper_test.go +++ b/internal/gitaly/service/hook/testhelper_test.go @@ -73,7 +73,6 @@ func runHooksServer(tb testing.TB, cfg config.Cfg, opts []serverOption, serverOp deps.GetGitCmdFactory(), deps.GetCatfileCache(), deps.GetConnsPool(), - deps.GetGit2goExecutor(), deps.GetHousekeepingManager(), deps.GetBackupSink(), deps.GetBackupLocator(), diff --git a/internal/gitaly/service/objectpool/testhelper_test.go b/internal/gitaly/service/objectpool/testhelper_test.go index 48a5e2967..a20c28ed1 100644 --- a/internal/gitaly/service/objectpool/testhelper_test.go +++ b/internal/gitaly/service/objectpool/testhelper_test.go @@ -85,7 +85,6 @@ func runObjectPoolServer(t *testing.T, cfg config.Cfg, locator storage.Locator, deps.GetGitCmdFactory(), deps.GetCatfileCache(), deps.GetConnsPool(), - deps.GetGit2goExecutor(), deps.GetHousekeepingManager(), deps.GetBackupSink(), deps.GetBackupLocator(), diff --git a/internal/gitaly/service/operations/apply_patch.go b/internal/gitaly/service/operations/apply_patch.go index 644bcec5f..aa38e378d 100644 --- a/internal/gitaly/service/operations/apply_patch.go +++ b/internal/gitaly/service/operations/apply_patch.go @@ -93,12 +93,9 @@ func (s *Server) userApplyPatch(ctx context.Context, header *gitalypb.UserApplyP branchCreated = true } - committerTime := time.Now() - if header.Timestamp != nil { - committerTime, err = dateFromProto(header) - if err != nil { - return structerr.NewInvalidArgument("%w", err) - } + committerDate, err := dateFromProto(header) + if err != nil { + return structerr.NewInvalidArgument("%w", err) } worktreePath := newWorktreePath(path, "am-") @@ -128,7 +125,7 @@ func (s *Server) userApplyPatch(ctx context.Context, header *gitalypb.UserApplyP git.WithEnv( "GIT_COMMITTER_NAME="+string(header.GetUser().Name), "GIT_COMMITTER_EMAIL="+string(header.GetUser().Email), - "GIT_COMMITTER_DATE="+git.FormatTime(committerTime), + "GIT_COMMITTER_DATE="+git.FormatTime(committerDate), ), git.WithStdin(streamio.NewReader(func() ([]byte, error) { req, err := stream.Recv() diff --git a/internal/gitaly/service/operations/cherry_pick.go b/internal/gitaly/service/operations/cherry_pick.go index beca52822..53fc217ba 100644 --- a/internal/gitaly/service/operations/cherry_pick.go +++ b/internal/gitaly/service/operations/cherry_pick.go @@ -36,9 +36,9 @@ func (s *Server) UserCherryPick(ctx context.Context, req *gitalypb.UserCherryPic return nil, structerr.NewInternal("has branches: %w", err) } - committerDate := time.Now() - if req.Timestamp != nil { - committerDate = req.Timestamp.AsTime() + committerDate, err := dateFromProto(req) + if err != nil { + return nil, structerr.NewInvalidArgument("%w", err) } cherryCommit, err := quarantineRepo.ReadCommit(ctx, git.Revision(req.Commit.Id)) diff --git a/internal/gitaly/service/operations/cherry_pick_test.go b/internal/gitaly/service/operations/cherry_pick_test.go index f962cbe87..dd6cfa92a 100644 --- a/internal/gitaly/service/operations/cherry_pick_test.go +++ b/internal/gitaly/service/operations/cherry_pick_test.go @@ -575,8 +575,8 @@ func testServerUserCherryPickStableID(t *testing.T, ctx context.Context) { require.NoError(t, err) expectedCommitID := gittest.ObjectHashDependent(t, map[string]string{ - "sha1": "92452444836a56b6fb1b2f0e4e62384d7d6f49db", - "sha256": "92cb8205718f443de173cff9997b3ea49e3ef5864b700a64403cae221a38338e", + "sha1": "e4e27d5484b1862f887391fe3417fecb95ee4fef", + "sha256": "e0cc4d237718edca19b01bfe90d8742a940b82aa7596124d46f6ac02818bf232", }) require.Equal(t, expectedCommitID, response.BranchUpdate.CommitId) @@ -605,7 +605,7 @@ func testServerUserCherryPickStableID(t *testing.T, ctx context.Context) { Date: ×tamppb.Timestamp{ Seconds: 12345, }, - Timezone: []byte("+0000"), + Timezone: []byte(gittest.TimezoneOffset), }, }, pickCommit) } diff --git a/internal/gitaly/service/operations/commit_files.go b/internal/gitaly/service/operations/commit_files.go index 46b6008b2..401ee8d29 100644 --- a/internal/gitaly/service/operations/commit_files.go +++ b/internal/gitaly/service/operations/commit_files.go @@ -413,7 +413,7 @@ func (s *Server) userCommitFilesGit( repoPath string, actions []commitAction, ) (git.ObjectID, error) { - now, err := dateFromProto(header) + authorDate, err := dateFromProto(header) if err != nil { return "", structerr.NewInvalidArgument("getting date from proto: %w", err) } @@ -476,10 +476,10 @@ func (s *Server) userCommitFilesGit( } cfg := localrepo.WriteCommitConfig{ - AuthorDate: now, + AuthorDate: authorDate, AuthorName: strings.TrimSpace(string(header.CommitAuthorName)), AuthorEmail: strings.TrimSpace(string(header.CommitAuthorEmail)), - CommitterDate: now, + CommitterDate: authorDate, CommitterName: strings.TrimSpace(string(header.User.Name)), CommitterEmail: strings.TrimSpace(string(header.User.Email)), Message: string(header.CommitMessage), diff --git a/internal/gitaly/service/operations/rebase_confirmable.go b/internal/gitaly/service/operations/rebase_confirmable.go index 291ba539e..f8a5bcd1c 100644 --- a/internal/gitaly/service/operations/rebase_confirmable.go +++ b/internal/gitaly/service/operations/rebase_confirmable.go @@ -4,7 +4,6 @@ import ( "errors" "fmt" "strings" - "time" "gitlab.com/gitlab-org/gitaly/v16/internal/git" "gitlab.com/gitlab-org/gitaly/v16/internal/git/localrepo" @@ -54,10 +53,11 @@ func (s *Server) UserRebaseConfirmable(stream gitalypb.OperationService_UserReba return structerr.NewInternal("%w", err) } - committer := git.NewSignature(string(header.User.Name), string(header.User.Email), time.Now()) - if header.Timestamp != nil { - committer.When = header.Timestamp.AsTime() + committerDate, err := dateFromProto(header) + if err != nil { + return structerr.NewInvalidArgument("%w", err) } + committer := git.NewSignature(string(header.User.Name), string(header.User.Email), committerDate) newrev, err := quarantineRepo.Rebase( ctx, diff --git a/internal/gitaly/service/operations/rebase_confirmable_test.go b/internal/gitaly/service/operations/rebase_confirmable_test.go index 378cf0f36..71f94db26 100644 --- a/internal/gitaly/service/operations/rebase_confirmable_test.go +++ b/internal/gitaly/service/operations/rebase_confirmable_test.go @@ -181,8 +181,8 @@ func testUserRebaseConfirmableSkipEmptyCommits(t *testing.T, ctx context.Context Body: []byte("ours with additional changes"), BodySize: 28, Id: gittest.ObjectHashDependent(t, map[string]string{ - "sha1": "ef7f98be1f753f1a9fa895d999a855611d691629", - "sha256": "29c9b79bd0e742d7bb51ac0be5283f65fb806a94c19cb591b3621e58703164fa", + "sha1": "346ded91d8ed7a2a49ba50a6a76d3d0f3ffd23f2", + "sha256": "eda1cf87f683b102b7cb65b289c984e66192ac785cac0af4aed63a28b33471eb", }), ParentIds: []string{theirs.String()}, TreeId: gittest.ObjectHashDependent(t, map[string]string{ @@ -194,7 +194,7 @@ func testUserRebaseConfirmableSkipEmptyCommits(t *testing.T, ctx context.Context Name: gittest.TestUser.Name, Email: gittest.TestUser.Email, Date: ×tamppb.Timestamp{Seconds: 123456}, - Timezone: []byte("+0000"), + Timezone: []byte(gittest.TimezoneOffset), }, }, rebaseCommit) } @@ -328,8 +328,8 @@ func testUserRebaseConfirmableStableCommitIDs(t *testing.T, ctx context.Context) }), "send header") expectedCommitID := gittest.ObjectHashDependent(t, map[string]string{ - "sha1": "85b0186925c57efa608939afea01b627a2f4d4cf", - "sha256": "a14d9fb56edf718b4aaeaabd2de8cd2403820396ee905f9c87337c5bea8598cf", + "sha1": "96ce8a9f0bc1706adbc5deb513c196607a85241a", + "sha256": "78ac6c2d059c6149231ad9b648d67194852bc0af3ac162a5fcfaf7b2f52270fd", }) response, err := rebaseStream.Recv() @@ -364,7 +364,7 @@ func testUserRebaseConfirmableStableCommitIDs(t *testing.T, ctx context.Context) Email: gittest.TestUser.Email, // Nanoseconds get ignored because commit timestamps aren't that granular. Date: committerDate, - Timezone: []byte("+0000"), + Timezone: []byte(gittest.TimezoneOffset), }, }, commit) } diff --git a/internal/gitaly/service/operations/rebase_to_ref.go b/internal/gitaly/service/operations/rebase_to_ref.go index 3e601b9cf..b643a1481 100644 --- a/internal/gitaly/service/operations/rebase_to_ref.go +++ b/internal/gitaly/service/operations/rebase_to_ref.go @@ -4,7 +4,6 @@ import ( "context" "errors" "fmt" - "time" "gitlab.com/gitlab-org/gitaly/v16/internal/git" "gitlab.com/gitlab-org/gitaly/v16/internal/git/localrepo" @@ -74,10 +73,11 @@ func (s *Server) UserRebaseToRef(ctx context.Context, request *gitalypb.UserReba return nil, structerr.NewInternal("could not read target reference: %w", err) } - committer := git.NewSignature(string(request.User.Name), string(request.User.Email), time.Now()) - if request.Timestamp != nil { - committer.When = request.Timestamp.AsTime() + committerDate, err := dateFromProto(request) + if err != nil { + return nil, structerr.NewInvalidArgument("%w", err) } + committer := git.NewSignature(string(request.User.Name), string(request.User.Email), committerDate) rebasedOID, err := quarantineRepo.Rebase( ctx, diff --git a/internal/gitaly/service/operations/server.go b/internal/gitaly/service/operations/server.go index 06386828a..b96e1ab18 100644 --- a/internal/gitaly/service/operations/server.go +++ b/internal/gitaly/service/operations/server.go @@ -8,7 +8,6 @@ import ( "gitlab.com/gitlab-org/gitaly/v16/internal/git/catfile" "gitlab.com/gitlab-org/gitaly/v16/internal/git/localrepo" "gitlab.com/gitlab-org/gitaly/v16/internal/git/quarantine" - "gitlab.com/gitlab-org/gitaly/v16/internal/git2go" "gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/hook" "gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/hook/updateref" "gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/storage" @@ -20,15 +19,14 @@ import ( //nolint:revive // This is unintentionally missing documentation. type Server struct { gitalypb.UnimplementedOperationServiceServer - hookManager hook.Manager - txManager transaction.Manager - locator storage.Locator - conns *client.Pool - git2goExecutor *git2go.Executor - gitCmdFactory git.CommandFactory - catfileCache catfile.Cache - updater *updateref.UpdaterWithHooks - signingKey string + hookManager hook.Manager + txManager transaction.Manager + locator storage.Locator + conns *client.Pool + gitCmdFactory git.CommandFactory + catfileCache catfile.Cache + updater *updateref.UpdaterWithHooks + signingKey string } // NewServer creates a new instance of a grpc OperationServiceServer @@ -37,22 +35,20 @@ func NewServer( txManager transaction.Manager, locator storage.Locator, conns *client.Pool, - git2goExecutor *git2go.Executor, gitCmdFactory git.CommandFactory, catfileCache catfile.Cache, updater *updateref.UpdaterWithHooks, signingKey string, ) *Server { return &Server{ - hookManager: hookManager, - txManager: txManager, - locator: locator, - conns: conns, - git2goExecutor: git2goExecutor, - gitCmdFactory: gitCmdFactory, - catfileCache: catfileCache, - updater: updater, - signingKey: signingKey, + hookManager: hookManager, + txManager: txManager, + locator: locator, + conns: conns, + gitCmdFactory: gitCmdFactory, + catfileCache: catfileCache, + updater: updater, + signingKey: signingKey, } } diff --git a/internal/gitaly/service/operations/squash.go b/internal/gitaly/service/operations/squash.go index 9f712b3c9..9e8b8cf3f 100644 --- a/internal/gitaly/service/operations/squash.go +++ b/internal/gitaly/service/operations/squash.go @@ -114,7 +114,7 @@ func (s *Server) userSquash(ctx context.Context, req *gitalypb.UserSquashRequest ) } - commitDate, err := dateFromProto(req) + committerDate, err := dateFromProto(req) if err != nil { return "", structerr.NewInvalidArgument("%w", err) } @@ -133,10 +133,10 @@ func (s *Server) userSquash(ctx context.Context, req *gitalypb.UserSquashRequest quarantineRepo, string(req.GetAuthor().GetName()), string(req.GetAuthor().GetEmail()), - commitDate, + committerDate, string(req.GetUser().GetName()), string(req.GetUser().GetEmail()), - commitDate, + committerDate, message, startCommit.String(), endCommit.String(), diff --git a/internal/gitaly/service/operations/tags.go b/internal/gitaly/service/operations/tags.go index 23fa93d3b..d9c0d6d8a 100644 --- a/internal/gitaly/service/operations/tags.go +++ b/internal/gitaly/service/operations/tags.go @@ -125,7 +125,7 @@ func (s *Server) UserCreateTag(ctx context.Context, req *gitalypb.UserCreateTagR targetRevision := git.Revision(req.TargetRevision) referenceName := git.ReferenceName(fmt.Sprintf("refs/tags/%s", req.TagName)) - committerTime, err := dateFromProto(req) + taggerDate, err := dateFromProto(req) if err != nil { return nil, structerr.NewInvalidArgument("%w", err) } @@ -140,7 +140,7 @@ func (s *Server) UserCreateTag(ctx context.Context, req *gitalypb.UserCreateTagR return nil, fmt.Errorf("detecting object hash: %w", err) } - tag, tagID, err := s.createTag(ctx, quarantineRepo, targetRevision, req.TagName, req.Message, req.User, committerTime) + tag, tagID, err := s.createTag(ctx, quarantineRepo, targetRevision, req.TagName, req.Message, req.User, taggerDate) if err != nil { return nil, err } diff --git a/internal/gitaly/service/operations/testhelper_test.go b/internal/gitaly/service/operations/testhelper_test.go index a21c8b995..8d0a04074 100644 --- a/internal/gitaly/service/operations/testhelper_test.go +++ b/internal/gitaly/service/operations/testhelper_test.go @@ -41,7 +41,6 @@ func setupOperationsServiceWithCfg( tb testing.TB, ctx context.Context, cfg config.Cfg, options ...testserver.GitalyServerOpt, ) (context.Context, config.Cfg, gitalypb.OperationServiceClient) { testcfg.BuildGitalySSH(tb, cfg) - testcfg.BuildGitalyGit2Go(tb, cfg) testcfg.BuildGitalyHooks(tb, cfg) serverSocketPath := runOperationServiceServer(tb, cfg, options...) @@ -62,7 +61,6 @@ func setupOperationsService( cfg := testcfg.Build(tb) testcfg.BuildGitalySSH(tb, cfg) - testcfg.BuildGitalyGit2Go(tb, cfg) testcfg.BuildGitalyHooks(tb, cfg) serverSocketPath := runOperationServiceServer(tb, cfg, options...) @@ -86,7 +84,6 @@ func runOperationServiceServer(tb testing.TB, cfg config.Cfg, options ...testser deps.GetTxManager(), deps.GetLocator(), deps.GetConnsPool(), - deps.GetGit2goExecutor(), deps.GetGitCmdFactory(), deps.GetCatfileCache(), deps.GetUpdaterWithHooks(), @@ -108,7 +105,6 @@ func runOperationServiceServer(tb testing.TB, cfg config.Cfg, options ...testser deps.GetGitCmdFactory(), deps.GetCatfileCache(), deps.GetConnsPool(), - deps.GetGit2goExecutor(), deps.GetHousekeepingManager(), deps.GetBackupSink(), deps.GetBackupLocator(), diff --git a/internal/gitaly/service/operations/user_create_branch_test.go b/internal/gitaly/service/operations/user_create_branch_test.go index 5fdd93f16..c6e8aa854 100644 --- a/internal/gitaly/service/operations/user_create_branch_test.go +++ b/internal/gitaly/service/operations/user_create_branch_test.go @@ -128,7 +128,6 @@ func TestUserCreateBranch_transactions(t *testing.T) { deps.GetTxManager(), deps.GetLocator(), deps.GetConnsPool(), - deps.GetGit2goExecutor(), deps.GetGitCmdFactory(), deps.GetCatfileCache(), deps.GetUpdaterWithHooks(), diff --git a/internal/gitaly/service/operations/user_delete_branch_test.go b/internal/gitaly/service/operations/user_delete_branch_test.go index 686778e0a..e71780551 100644 --- a/internal/gitaly/service/operations/user_delete_branch_test.go +++ b/internal/gitaly/service/operations/user_delete_branch_test.go @@ -451,7 +451,6 @@ func TestUserDeleteBranch_transaction(t *testing.T) { deps.GetTxManager(), deps.GetLocator(), deps.GetConnsPool(), - deps.GetGit2goExecutor(), deps.GetGitCmdFactory(), deps.GetCatfileCache(), deps.GetUpdaterWithHooks(), diff --git a/internal/gitaly/service/ref/delete_refs_test.go b/internal/gitaly/service/ref/delete_refs_test.go index 48c12eb82..ccd53fb3c 100644 --- a/internal/gitaly/service/ref/delete_refs_test.go +++ b/internal/gitaly/service/ref/delete_refs_test.go @@ -119,7 +119,6 @@ func TestDeleteRefs_transaction(t *testing.T) { deps.GetGitCmdFactory(), deps.GetCatfileCache(), deps.GetConnsPool(), - deps.GetGit2goExecutor(), deps.GetHousekeepingManager(), deps.GetBackupSink(), deps.GetBackupLocator(), diff --git a/internal/gitaly/service/ref/testhelper_test.go b/internal/gitaly/service/ref/testhelper_test.go index d2f077855..a80dc7dcf 100644 --- a/internal/gitaly/service/ref/testhelper_test.go +++ b/internal/gitaly/service/ref/testhelper_test.go @@ -66,7 +66,6 @@ func runRefServiceServer(tb testing.TB, cfg config.Cfg) string { deps.GetGitCmdFactory(), deps.GetCatfileCache(), deps.GetConnsPool(), - deps.GetGit2goExecutor(), deps.GetHousekeepingManager(), deps.GetBackupSink(), deps.GetBackupLocator(), diff --git a/internal/gitaly/service/remote/testhelper_test.go b/internal/gitaly/service/remote/testhelper_test.go index 863d7f4d2..cf71f9860 100644 --- a/internal/gitaly/service/remote/testhelper_test.go +++ b/internal/gitaly/service/remote/testhelper_test.go @@ -39,7 +39,6 @@ func setupRemoteService(t *testing.T, ctx context.Context, opts ...testserver.Gi deps.GetGitCmdFactory(), deps.GetCatfileCache(), deps.GetConnsPool(), - deps.GetGit2goExecutor(), deps.GetHousekeepingManager(), deps.GetBackupSink(), deps.GetBackupLocator(), diff --git a/internal/gitaly/service/remote/update_remote_mirror_test.go b/internal/gitaly/service/remote/update_remote_mirror_test.go index d295fbe6e..fdf94702c 100644 --- a/internal/gitaly/service/remote/update_remote_mirror_test.go +++ b/internal/gitaly/service/remote/update_remote_mirror_test.go @@ -587,7 +587,6 @@ func TestUpdateRemoteMirror(t *testing.T) { deps.GetGitCmdFactory(), deps.GetCatfileCache(), deps.GetConnsPool(), - deps.GetGit2goExecutor(), deps.GetHousekeepingManager(), deps.GetBackupSink(), deps.GetBackupLocator(), diff --git a/internal/gitaly/service/repository/replicate.go b/internal/gitaly/service/repository/replicate.go index 8ee8f92d0..e3a0b406f 100644 --- a/internal/gitaly/service/repository/replicate.go +++ b/internal/gitaly/service/repository/replicate.go @@ -262,6 +262,11 @@ func fetchInternalRemote( localrepo.FetchOpts{ Prune: true, Stderr: &stderr, + // By default, Git will fetch any tags that point into the fetched references. This check + // requires time, and is ultimately a waste of compute because we already mirror all refs + // anyway, including tags. By adding `--no-tags` we can thus ask Git to skip that and thus + // accelerate the fetch. + Tags: localrepo.FetchOptsTagsNone, CommandOptions: []git.CmdOpt{ git.WithConfig(git.ConfigPair{Key: "fetch.negotiationAlgorithm", Value: "skipping"}), // Disable the consistency checks of objects fetched into the replicated repository. diff --git a/internal/gitaly/service/repository/server.go b/internal/gitaly/service/repository/server.go index aafcf2a9f..0e354b2d1 100644 --- a/internal/gitaly/service/repository/server.go +++ b/internal/gitaly/service/repository/server.go @@ -10,7 +10,6 @@ import ( "gitlab.com/gitlab-org/gitaly/v16/internal/git/housekeeping" "gitlab.com/gitlab-org/gitaly/v16/internal/git/localrepo" "gitlab.com/gitlab-org/gitaly/v16/internal/git/quarantine" - "gitlab.com/gitlab-org/gitaly/v16/internal/git2go" "gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/config" "gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/storage" "gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/storage/counter" @@ -29,7 +28,6 @@ type server struct { cfg config.Cfg loggingCfg config.Logging catfileCache catfile.Cache - git2goExecutor *git2go.Executor housekeepingManager housekeeping.Manager backupSink backup.Sink backupLocator backup.Locator @@ -46,7 +44,6 @@ func NewServer( gitCmdFactory git.CommandFactory, catfileCache catfile.Cache, connsPool *client.Pool, - git2goExecutor *git2go.Executor, housekeepingManager housekeeping.Manager, backupSink backup.Sink, backupLocator backup.Locator, @@ -60,7 +57,6 @@ func NewServer( cfg: cfg, loggingCfg: cfg.Logging, catfileCache: catfileCache, - git2goExecutor: git2goExecutor, housekeepingManager: housekeepingManager, backupSink: backupSink, backupLocator: backupLocator, diff --git a/internal/gitaly/service/repository/testhelper_test.go b/internal/gitaly/service/repository/testhelper_test.go index ec0bd90e5..ba57b32e7 100644 --- a/internal/gitaly/service/repository/testhelper_test.go +++ b/internal/gitaly/service/repository/testhelper_test.go @@ -63,7 +63,6 @@ func runRepositoryService(tb testing.TB, cfg config.Cfg, opts ...testserver.Gita deps.GetGitCmdFactory(), deps.GetCatfileCache(), deps.GetConnsPool(), - deps.GetGit2goExecutor(), deps.GetHousekeepingManager(), deps.GetBackupSink(), deps.GetBackupLocator(), diff --git a/internal/gitaly/service/setup/register.go b/internal/gitaly/service/setup/register.go index 08a047a7b..b14298f1a 100644 --- a/internal/gitaly/service/setup/register.go +++ b/internal/gitaly/service/setup/register.go @@ -79,7 +79,6 @@ func RegisterAll(srv *grpc.Server, deps *service.Dependencies) { deps.GetTxManager(), deps.GetLocator(), deps.GetConnsPool(), - deps.GetGit2goExecutor(), deps.GetGitCmdFactory(), deps.GetCatfileCache(), deps.GetUpdaterWithHooks(), @@ -98,7 +97,6 @@ func RegisterAll(srv *grpc.Server, deps *service.Dependencies) { deps.GetGitCmdFactory(), deps.GetCatfileCache(), deps.GetConnsPool(), - deps.GetGit2goExecutor(), deps.GetHousekeepingManager(), deps.GetBackupSink(), deps.GetBackupLocator(), diff --git a/internal/gitaly/service/smarthttp/testhelper_test.go b/internal/gitaly/service/smarthttp/testhelper_test.go index 451043ea5..e765b020e 100644 --- a/internal/gitaly/service/smarthttp/testhelper_test.go +++ b/internal/gitaly/service/smarthttp/testhelper_test.go @@ -40,7 +40,6 @@ func startSmartHTTPServerWithOptions(t *testing.T, cfg config.Cfg, opts []Server deps.GetGitCmdFactory(), deps.GetCatfileCache(), deps.GetConnsPool(), - deps.GetGit2goExecutor(), deps.GetHousekeepingManager(), deps.GetBackupSink(), deps.GetBackupLocator(), diff --git a/internal/gitaly/service/ssh/testhelper_test.go b/internal/gitaly/service/ssh/testhelper_test.go index a1620b6f3..0d93a5e9f 100644 --- a/internal/gitaly/service/ssh/testhelper_test.go +++ b/internal/gitaly/service/ssh/testhelper_test.go @@ -49,7 +49,6 @@ func startSSHServerWithOptions(t *testing.T, cfg config.Cfg, opts []ServerOpt, s deps.GetGitCmdFactory(), deps.GetCatfileCache(), deps.GetConnsPool(), - deps.GetGit2goExecutor(), deps.GetHousekeepingManager(), deps.GetBackupSink(), deps.GetBackupLocator(), diff --git a/internal/grpc/middleware/limithandler/middleware_test.go b/internal/grpc/middleware/limithandler/middleware_test.go index 65adad2be..b12748582 100644 --- a/internal/grpc/middleware/limithandler/middleware_test.go +++ b/internal/grpc/middleware/limithandler/middleware_test.go @@ -27,10 +27,6 @@ import ( "google.golang.org/protobuf/types/known/durationpb" ) -func TestMain(m *testing.M) { - testhelper.Run(m) -} - func fixedLockKey(ctx context.Context) string { return "fixed-id" } diff --git a/internal/grpc/middleware/limithandler/testhelper_test.go b/internal/grpc/middleware/limithandler/testhelper_test.go index 137c47cce..c32e97579 100644 --- a/internal/grpc/middleware/limithandler/testhelper_test.go +++ b/internal/grpc/middleware/limithandler/testhelper_test.go @@ -4,7 +4,9 @@ import ( "context" "io" "sync/atomic" + "testing" + "gitlab.com/gitlab-org/gitaly/v16/internal/testhelper" "google.golang.org/grpc/interop/grpc_testing" ) @@ -14,6 +16,10 @@ type server struct { blockCh chan struct{} } +func TestMain(m *testing.M) { + testhelper.Run(m) +} + func (s *server) registerRequest() { atomic.AddUint64(&s.requestCount, 1) } diff --git a/internal/log/log_testhelper_test.go b/internal/log/log_testhelper_test.go new file mode 100644 index 000000000..c96b77e61 --- /dev/null +++ b/internal/log/log_testhelper_test.go @@ -0,0 +1,17 @@ +package log + +import ( + "io" + + "github.com/sirupsen/logrus" +) + +// newLogger creates a new logger for testing purposes. Use of `logrus.New()` is forbidden globally, but required here +// to verify that we correctly configure our logging infrastructure. +// +//nolint:forbidigo +func newLogger() *logrus.Logger { + logger := logrus.New() + logger.Out = io.Discard + return logger +} diff --git a/internal/log/testhelper_test.go b/internal/log/testhelper_test.go index c96b77e61..b4c6298c6 100644 --- a/internal/log/testhelper_test.go +++ b/internal/log/testhelper_test.go @@ -1,17 +1,11 @@ -package log +package log_test import ( - "io" + "testing" - "github.com/sirupsen/logrus" + "gitlab.com/gitlab-org/gitaly/v16/internal/testhelper" ) -// newLogger creates a new logger for testing purposes. Use of `logrus.New()` is forbidden globally, but required here -// to verify that we correctly configure our logging infrastructure. -// -//nolint:forbidigo -func newLogger() *logrus.Logger { - logger := logrus.New() - logger.Out = io.Discard - return logger +func TestMain(m *testing.M) { + testhelper.Run(m) } diff --git a/internal/praefect/info_service_test.go b/internal/praefect/info_service_test.go index d07ede6aa..bb2d2395e 100644 --- a/internal/praefect/info_service_test.go +++ b/internal/praefect/info_service_test.go @@ -44,7 +44,6 @@ func TestInfoService_RepositoryReplicas(t *testing.T) { deps.GetGitCmdFactory(), deps.GetCatfileCache(), deps.GetConnsPool(), - deps.GetGit2goExecutor(), deps.GetHousekeepingManager(), deps.GetBackupSink(), deps.GetBackupLocator(), diff --git a/internal/praefect/repocleaner/repository_test.go b/internal/praefect/repocleaner/repository_test.go index 83c21ab17..cb4843959 100644 --- a/internal/praefect/repocleaner/repository_test.go +++ b/internal/praefect/repocleaner/repository_test.go @@ -208,12 +208,6 @@ func TestRunner_Run(t *testing.T) { } func TestRunner_Run_noAvailableStorages(t *testing.T) { - testhelper.SkipQuarantinedTest( - t, - "https://gitlab.com/gitlab-org/gitaly/-/issues/5504", - "TestRunner_Run_noAvailableStorages", - ) - t.Parallel() const ( @@ -241,8 +235,8 @@ func TestRunner_Run_noAvailableStorages(t *testing.T) { DB: dbConf, } cfg := Cfg{ - RunInterval: time.Minute, - LivenessInterval: time.Second, + RunInterval: time.Hour, + LivenessInterval: time.Hour, RepositoriesInBatch: 2, } diff --git a/internal/praefect/verifier_test.go b/internal/praefect/verifier_test.go index b7319df48..a489a1e4a 100644 --- a/internal/praefect/verifier_test.go +++ b/internal/praefect/verifier_test.go @@ -484,7 +484,6 @@ func TestVerifier(t *testing.T) { deps.GetGitCmdFactory(), deps.GetCatfileCache(), deps.GetConnsPool(), - deps.GetGit2goExecutor(), deps.GetHousekeepingManager(), deps.GetBackupSink(), deps.GetBackupLocator(), diff --git a/internal/testhelper/configure.go b/internal/testhelper/configure.go index 7788788a3..6dc326057 100644 --- a/internal/testhelper/configure.go +++ b/internal/testhelper/configure.go @@ -111,7 +111,7 @@ func configure() (_ func(), returnedErr error) { } } - // We need to make sure that we're gitconfig-clean: neither Git nor libgit2 should pick up + // We need to make sure that we're gitconfig-clean: Git should not pick up // gitconfig files from anywhere but the repository itself in case they're configured to // ignore them. We set that configuration by default in our tests to have a known-good // environment. diff --git a/internal/testhelper/testcfg/binaries.go b/internal/testhelper/testcfg/binaries.go index c3d7265c7..8d403e8b0 100644 --- a/internal/testhelper/testcfg/binaries.go +++ b/internal/testhelper/testcfg/binaries.go @@ -18,16 +18,11 @@ import ( var buildOnceByName sync.Map -// BuildGitalyGPG builds the gitaly-git2go command and installs it into the binary directory. +// BuildGitalyGPG builds the gitaly-gpg command and installs it into the binary directory. func BuildGitalyGPG(tb testing.TB, cfg config.Cfg) string { return buildGitalyCommand(tb, cfg, "gitaly-gpg") } -// BuildGitalyGit2Go builds the gitaly-git2go command and installs it into the binary directory. -func BuildGitalyGit2Go(tb testing.TB, cfg config.Cfg) string { - return buildGitalyCommand(tb, cfg, "gitaly-git2go") -} - // BuildGitalyWrapper builds the gitaly-wrapper command and installs it into the binary directory. func BuildGitalyWrapper(tb testing.TB, cfg config.Cfg) string { return buildGitalyCommand(tb, cfg, "gitaly-wrapper") @@ -107,7 +102,7 @@ func BuildBinary(tb testing.TB, targetDir, sourcePath string) string { } buildTags := []string{ - "static", "system_libgit2", "gitaly_test", + "gitaly_test", } if os.Getenv("GITALY_TESTING_ENABLE_FIPS") != "" { buildTags = append(buildTags, "fips") diff --git a/internal/testhelper/testdata/home/.config/git/config b/internal/testhelper/testdata/home/.config/git/config index aef95e47e..e6dc1af91 100644 --- a/internal/testhelper/testdata/home/.config/git/config +++ b/internal/testhelper/testdata/home/.config/git/config @@ -1,4 +1,4 @@ -# We're trying to catch any instances of libgit2 or Git that pick up the +# We're trying to catch any instances of Git that pick up the # gitconfig even though they're told not to. If they do pick up this file # though, then they would fail to parse it and thus return an error, which we # can detect quite easily. diff --git a/internal/testhelper/testdata/home/.gitconfig b/internal/testhelper/testdata/home/.gitconfig index aef95e47e..e6dc1af91 100644 --- a/internal/testhelper/testdata/home/.gitconfig +++ b/internal/testhelper/testdata/home/.gitconfig @@ -1,4 +1,4 @@ -# We're trying to catch any instances of libgit2 or Git that pick up the +# We're trying to catch any instances of Git that pick up the # gitconfig even though they're told not to. If they do pick up this file # though, then they would fail to parse it and thus return an error, which we # can detect quite easily. diff --git a/internal/testhelper/testserver/gitaly.go b/internal/testhelper/testserver/gitaly.go index 6baae7859..8cc44245c 100644 --- a/internal/testhelper/testserver/gitaly.go +++ b/internal/testhelper/testserver/gitaly.go @@ -18,7 +18,6 @@ import ( "gitlab.com/gitlab-org/gitaly/v16/internal/git/gittest" "gitlab.com/gitlab-org/gitaly/v16/internal/git/housekeeping" "gitlab.com/gitlab-org/gitaly/v16/internal/git/localrepo" - "gitlab.com/gitlab-org/gitaly/v16/internal/git2go" "gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/config" "gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/config/auth" "gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/hook" @@ -268,7 +267,6 @@ type gitalyServerDeps struct { packObjectsLimiter limiter.Limiter limitHandler *limithandler.LimiterMiddleware repositoryCounter *counter.RepositoryCounter - git2goExecutor *git2go.Executor updaterWithHooks *updateref.UpdaterWithHooks housekeepingManager housekeeping.Manager backupSink backup.Sink @@ -343,10 +341,6 @@ func (gsd *gitalyServerDeps) createDependencies(tb testing.TB, cfg config.Cfg) * gsd.repositoryCounter = counter.NewRepositoryCounter(cfg.Storages) } - if gsd.git2goExecutor == nil { - gsd.git2goExecutor = git2go.NewExecutor(cfg, gsd.gitCmdFactory, gsd.locator, gsd.logger) - } - if gsd.updaterWithHooks == nil { gsd.updaterWithHooks = updateref.NewUpdaterWithHooks(cfg, gsd.locator, gsd.hookMgr, gsd.gitCmdFactory, gsd.catfileCache) } @@ -388,7 +382,6 @@ func (gsd *gitalyServerDeps) createDependencies(tb testing.TB, cfg config.Cfg) * PackObjectsLimiter: gsd.packObjectsLimiter, LimitHandler: gsd.limitHandler, RepositoryCounter: gsd.repositoryCounter, - Git2goExecutor: gsd.git2goExecutor, UpdaterWithHooks: gsd.updaterWithHooks, HousekeepingManager: gsd.housekeepingManager, PartitionManager: partitionManager, diff --git a/packed_binaries.go b/packed_binaries.go index 1ae1a54a2..f589282c0 100644 --- a/packed_binaries.go +++ b/packed_binaries.go @@ -16,7 +16,7 @@ const buildDir = "_build/bin" // packedBinariesFS contains embedded binaries. If you modify the above embeddings, you must also update // GITALY_PACKED_EXECUTABLES in Makefile and packedBinaries in internal/gitaly/config/config.go. // -//go:embed _build/bin/gitaly-hooks _build/bin/gitaly-ssh _build/bin/gitaly-git2go _build/bin/gitaly-lfs-smudge _build/bin/gitaly-gpg +//go:embed _build/bin/gitaly-hooks _build/bin/gitaly-ssh _build/bin/gitaly-lfs-smudge _build/bin/gitaly-gpg var packedBinariesFS embed.FS // UnpackAuxiliaryBinaries unpacks the packed auxiliary binaries of Gitaly into destination directory. diff --git a/tools/golangci-lint/gitaly/testdata/src/testhelper_run_blackbox/testhelper_run_blackbox.go b/tools/golangci-lint/gitaly/testdata/src/testhelper_run_blackbox/testhelper_run_blackbox.go new file mode 100644 index 000000000..42818a757 --- /dev/null +++ b/tools/golangci-lint/gitaly/testdata/src/testhelper_run_blackbox/testhelper_run_blackbox.go @@ -0,0 +1,5 @@ +package testhelper_run_blackbox + +func Foo() string { + return "bar" +} diff --git a/tools/golangci-lint/gitaly/testdata/src/testhelper_run_blackbox/testhelper_run_blackbox_test.go b/tools/golangci-lint/gitaly/testdata/src/testhelper_run_blackbox/testhelper_run_blackbox_test.go new file mode 100644 index 000000000..d1cb4dd1c --- /dev/null +++ b/tools/golangci-lint/gitaly/testdata/src/testhelper_run_blackbox/testhelper_run_blackbox_test.go @@ -0,0 +1,12 @@ +package testhelper_run_blackbox_test // want "no TestMain in package testhelper_run_blackbox_test" + +import ( + "testhelper_run_blackbox" + "testing" +) + +func TestFoo(t *testing.T) { + if testhelper_run_blackbox.Foo() != "bar" { + t.FailNow() + } +} diff --git a/tools/golangci-lint/gitaly/testdata/src/testhelper_run_no_exec_testmain/testhelper_test.go b/tools/golangci-lint/gitaly/testdata/src/testhelper_run_no_exec_testmain/testhelper_test.go index 94eb334f0..932b0cf75 100644 --- a/tools/golangci-lint/gitaly/testdata/src/testhelper_run_no_exec_testmain/testhelper_test.go +++ b/tools/golangci-lint/gitaly/testdata/src/testhelper_run_no_exec_testmain/testhelper_test.go @@ -1,4 +1,4 @@ -package testhelper_run_no_exec_testmain +package testhelper_run_no_exec_testmain // want package:"package has TestMain" import ( "testing" diff --git a/tools/golangci-lint/gitaly/testdata/src/testhelper_run_not_testhelper/not_testhelper.go b/tools/golangci-lint/gitaly/testdata/src/testhelper_run_not_testhelper/not_testhelper.go index 28afec330..688ec1a36 100644 --- a/tools/golangci-lint/gitaly/testdata/src/testhelper_run_not_testhelper/not_testhelper.go +++ b/tools/golangci-lint/gitaly/testdata/src/testhelper_run_not_testhelper/not_testhelper.go @@ -1,4 +1,4 @@ -package testhelper_run_not_testhelper +package testhelper_run_not_testhelper // want package:"package has TestMain" import "testing" diff --git a/tools/golangci-lint/gitaly/testhelper_run.go b/tools/golangci-lint/gitaly/testhelper_run.go index aa9226c02..116ab4162 100644 --- a/tools/golangci-lint/gitaly/testhelper_run.go +++ b/tools/golangci-lint/gitaly/testhelper_run.go @@ -17,6 +17,17 @@ type testhelperRunAnalyzerSettings struct { IncludedFunctions []string `mapstructure:"included-functions"` } +// testmainFact is used to report if a package has defined a `TestMain` function. +type testmainFact struct { + HasTestMain bool +} + +// AFact is used to satisfy the `Fact` interface. +func (*testmainFact) AFact() {} + +// String returns the message expected by tests when a testmainFact is exported. +func (*testmainFact) String() string { return "package has TestMain" } + var toolPrefixPattern = regexp.MustCompile(`^gitlab.com/gitlab-org/gitaly(/v\d{2})?/tools`) // newTesthelperRunAnalyzer returns an analyzer to detect if a package that has tests does @@ -25,23 +36,17 @@ var toolPrefixPattern = regexp.MustCompile(`^gitlab.com/gitlab-org/gitaly(/v\d{2 // https://gitlab.com/gitlab-org/gitaly/-/blob/master/STYLE.md?ref_type=heads#common-setup func newTesthelperRunAnalyzer(settings *testhelperRunAnalyzerSettings) *analysis.Analyzer { return &analysis.Analyzer{ - Name: testhelperRunAnalyzerName, - Doc: `TestMain must be present and call testhelper.Run()`, - Run: runTesthelperRunAnalyzer(settings.IncludedFunctions), + Name: testhelperRunAnalyzerName, + Doc: `TestMain must be present and call testhelper.Run()`, + Run: runTesthelperRunAnalyzer(settings.IncludedFunctions), + FactTypes: []analysis.Fact{&testmainFact{}}, } } func runTesthelperRunAnalyzer(rules []string) func(*analysis.Pass) (interface{}, error) { return func(pass *analysis.Pass) (interface{}, error) { var hasTestMain, hasTests bool - - // Blackbox test packages ending with `_test` are considered to be - // part of the primary package for compilation, but are scanned in a - // separate pass by the analyzer. The primary and test packages cannot - // both define `TestMain`. Only require `TestMain` in the primary package. - if strings.HasSuffix(pass.Pkg.Name(), "_test") { - return nil, nil - } + var fact testmainFact // Don't lint tools, they can't import `testhelper`. if toolPrefixPattern.MatchString(pass.Pkg.Path()) { @@ -53,12 +58,23 @@ func runTesthelperRunAnalyzer(rules []string) func(*analysis.Pass) (interface{}, break } + // Blackbox test packages ending with `_test` are considered to be + // part of the primary package for compilation, but are scanned in a + // separate pass by the analyzer. The primary and test packages cannot + // both define `TestMain`. + if isTestPkg(pass) && primaryPkgHasTestMain(pass, file) { + // Primary package has already defined `TestMain`, no need to check + // `_test` package. + break + } + ast.Inspect(file, func(node ast.Node) bool { if decl, ok := node.(*ast.FuncDecl); ok { declName := decl.Name.Name if declName == "TestMain" { hasTestMain = true + fact.HasTestMain = true analyzeTestMain(pass, decl, rules) analyzeFilename(pass, file, decl) @@ -94,6 +110,9 @@ func runTesthelperRunAnalyzer(rules []string) func(*analysis.Pass) (interface{}, }) } + if hasTestMain { + pass.ExportPackageFact(&fact) + } return nil, nil } } @@ -134,3 +153,38 @@ func analyzeTestMain(pass *analysis.Pass, decl *ast.FuncDecl, rules []string) { }) } } + +func primaryPkgHasTestMain(pass *analysis.Pass, file *ast.File) bool { + var primaryPkg *types.Package + + ast.Inspect(file, func(node ast.Node) bool { + if spec, ok := node.(*ast.ImportSpec); ok { + obj, ok := pass.TypesInfo.Implicits[spec] + if !ok { + obj = pass.TypesInfo.Defs[spec.Name] + } + importedPkg := obj.(*types.PkgName).Imported() + + if importedPkg.Path() == strings.TrimSuffix(pass.Pkg.Path(), "_test") { + primaryPkg = importedPkg + } + } + + return true + }) + + if primaryPkg == nil { + return false + } + + var primaryFact testmainFact + if !pass.ImportPackageFact(primaryPkg, &primaryFact) { + return false + } + + return primaryFact.HasTestMain +} + +func isTestPkg(pass *analysis.Pass) bool { + return strings.HasSuffix(pass.Pkg.Name(), "_test") +} diff --git a/tools/golangci-lint/gitaly/testhelper_run_test.go b/tools/golangci-lint/gitaly/testhelper_run_test.go index e557d5287..7b13d5aa2 100644 --- a/tools/golangci-lint/gitaly/testhelper_run_test.go +++ b/tools/golangci-lint/gitaly/testhelper_run_test.go @@ -26,5 +26,6 @@ func TestTesthelperRun(t *testing.T) { "testhelper_run_no_testmain", "testhelper_run_no_exec_testmain", "testhelper_run_not_testhelper", + "testhelper_run_blackbox", ) } |