gitserver: Remove revspec updates (#61861)

Revspec updates never worked reliably. When there was _any_ operation in flight, we would just not fetch this revspec and return "all good".

This is only used by packages, an experimental feature.

Gitserver should not modify the authoritative data source for what versions exist. Instead, callers should add the version to the list of desired versions, and they'll eventually be part of the package when the next fetch occurred.

Anyhow, experimental, didn't work reliably, will be removed for now.

Test plan:

Existing test suites.

Author: eseliger

cmd/gitserver/internal/ensurerevision.go

@@ -32,7 +32,7 @@ func (s *Server) EnsureRevision(ctx context.Context, repo api.RepoName, rev stri
 	}
 
 	// Revision not found, update before returning.
-	err := s.doRepoUpdate(ctx, repo, rev)
+	_, _, err := s.RepoUpdate(ctx, repo)
 	if err != nil {
 		if ctx.Err() == nil {
 			ensureRevisionCounter.WithLabelValues("update_failed").Inc()

cmd/gitserver/internal/server.go

@@ -371,7 +371,7 @@ func (s *Server) repoUpdateOrClone(ctx context.Context, repoName api.RepoName) e
 				repoClonedCounter.Inc()
 				logger.Info("cloned repo", log.String("repo", string(repoName)))
 			} else {
-				if err := s.doRepoUpdate(ctx, repoName, ""); err != nil {
+				if err := s.doRepoUpdate(ctx, repoName); err != nil {
 					// The repo update might have failed due to the repo being corrupt
 					s.LogIfCorrupt(ctx, repoName, err)
 
@@ -691,7 +691,7 @@ var (
 
 var doBackgroundRepoUpdateMock func(api.RepoName) error
 
-func (s *Server) doRepoUpdate(ctx context.Context, repo api.RepoName, revspec string) (err error) {
+func (s *Server) doRepoUpdate(ctx context.Context, repo api.RepoName) (err error) {
 	logger := s.logger.Scoped("repoUpdate").With(log.String("repo", string(repo)))
 
 	if doBackgroundRepoUpdateMock != nil {
@@ -722,7 +722,7 @@ func (s *Server) doRepoUpdate(ctx context.Context, repo api.RepoName, revspec st
 		// ensure the background update doesn't hang forever
 		fetchCtx, cancelTimeout := context.WithTimeout(ctx, fetchTimeout)
 		defer cancelTimeout()
-		output, err := syncer.Fetch(fetchCtx, repo, dir, revspec)
+		output, err := syncer.Fetch(fetchCtx, repo, dir)
 		// best-effort update the output of the fetch
 		if err := s.db.GitserverRepos().SetLastOutput(ctx, repo, string(output)); err != nil {
 			s.logger.Warn("Setting last output in DB", log.Error(err))

cmd/gitserver/internal/server_test.go

@@ -1141,7 +1141,7 @@ type mockVCSSyncer struct {
 	mockTypeFunc    func() string
 	mockIsCloneable func(ctx context.Context, repoName api.RepoName) error
 	mockClone       func(ctx context.Context, repo api.RepoName, targetDir common.GitDir, tmpPath string, progressWriter io.Writer) error
-	mockFetch       func(ctx context.Context, repoName api.RepoName, dir common.GitDir, revspec string) ([]byte, error)
+	mockFetch       func(ctx context.Context, repoName api.RepoName, dir common.GitDir) ([]byte, error)
 }
 
 func (m *mockVCSSyncer) Type() string {
@@ -1168,9 +1168,9 @@ func (m *mockVCSSyncer) Clone(ctx context.Context, repo api.RepoName, targetDir
 	return errors.New("no mock for Clone() is set")
 }
 
-func (m *mockVCSSyncer) Fetch(ctx context.Context, repoName api.RepoName, dir common.GitDir, revspec string) ([]byte, error) {
+func (m *mockVCSSyncer) Fetch(ctx context.Context, repoName api.RepoName, dir common.GitDir) ([]byte, error) {
 	if m.mockFetch != nil {
-		return m.mockFetch(ctx, repoName, dir, revspec)
+		return m.mockFetch(ctx, repoName, dir)
 	}
 
 	return nil, errors.New("no mock for Fetch() is set")

cmd/gitserver/internal/vcssyncer/git.go

@@ -162,7 +162,7 @@ func (s *gitRepoSyncer) Clone(ctx context.Context, repo api.RepoName, _ common.G
 }
 
 // Fetch tries to fetch updates of a Git repository.
-func (s *gitRepoSyncer) Fetch(ctx context.Context, repoName api.RepoName, dir common.GitDir, _ string) ([]byte, error) {
+func (s *gitRepoSyncer) Fetch(ctx context.Context, repoName api.RepoName, dir common.GitDir) ([]byte, error) {
 	source, err := s.getRemoteURLSource(ctx, repoName)
 	if err != nil {
 		return nil, errors.Wrapf(err, "failed to get remote URL source for %s", repoName)

cmd/gitserver/internal/vcssyncer/instrumented_syncer.go

@@ -2,14 +2,16 @@ package vcssyncer
 
 import (
 	"context"
-	"github.com/prometheus/client_golang/prometheus"
-	"github.com/prometheus/client_golang/prometheus/promauto"
-	"github.com/sourcegraph/sourcegraph/cmd/gitserver/internal/common"
-	"github.com/sourcegraph/sourcegraph/internal/api"
 	"io"
 	"strconv"
 	"strings"
 	"time"
+
+	"github.com/prometheus/client_golang/prometheus"
+	"github.com/prometheus/client_golang/prometheus/promauto"
+
+	"github.com/sourcegraph/sourcegraph/cmd/gitserver/internal/common"
+	"github.com/sourcegraph/sourcegraph/internal/api"
 )
 
 // fetchBuckets are the buckets used for the fetch and clone duration histograms.
@@ -94,9 +96,9 @@ func (i *instrumentedSyncer) Clone(ctx context.Context, repo api.RepoName, targe
 	return i.base.Clone(ctx, repo, targetDir, tmpPath, progressWriter)
 }
 
-func (i *instrumentedSyncer) Fetch(ctx context.Context, repoName api.RepoName, dir common.GitDir, revspec string) (output []byte, err error) {
+func (i *instrumentedSyncer) Fetch(ctx context.Context, repoName api.RepoName, dir common.GitDir) (output []byte, err error) {
 	if !i.shouldObserve() {
-		return i.base.Fetch(ctx, repoName, dir, revspec)
+		return i.base.Fetch(ctx, repoName, dir)
 	}
 
 	start := time.Now()
@@ -107,7 +109,7 @@ func (i *instrumentedSyncer) Fetch(ctx context.Context, repoName api.RepoName, d
 		metricFetchDuration.WithLabelValues(i.formattedTypeLabel, strconv.FormatBool(succeeded)).Observe(duration)
 	}()
 
-	return i.base.Fetch(ctx, repoName, dir, revspec)
+	return i.base.Fetch(ctx, repoName, dir)
 }
 
 func (i *instrumentedSyncer) shouldObserve() bool {

cmd/gitserver/internal/vcssyncer/mock.go

@@ -8,7 +8,6 @@ import (
 	"path"
 	"sort"
 	"strings"
-	"time"
 
 	"github.com/sourcegraph/log"
 
@@ -98,14 +97,14 @@ func (s *vcsPackagesSyncer) Clone(ctx context.Context, repo api.RepoName, _ comm
 	// The Fetch method is responsible for cleaning up temporary directories.
 	// TODO: We should have more fine-grained progress reporting here.
 	tryWrite(s.logger, progressWriter, "Fetching package revisions\n")
-	if _, err := s.Fetch(ctx, repo, common.GitDir(tmpPath), ""); err != nil {
+	if _, err := s.Fetch(ctx, repo, common.GitDir(tmpPath)); err != nil {
 		return errors.Wrapf(err, "failed to fetch repo for %s", repo)
 	}
 
 	return nil
 }
 
-func (s *vcsPackagesSyncer) Fetch(ctx context.Context, repo api.RepoName, dir common.GitDir, revspec string) ([]byte, error) {
+func (s *vcsPackagesSyncer) Fetch(ctx context.Context, repo api.RepoName, dir common.GitDir) ([]byte, error) {
 	source, err := s.getRemoteURLSource(ctx, repo)
 	if err != nil {
 		return nil, errors.Wrap(err, "getting remote URL source")
@@ -128,76 +127,9 @@ func (s *vcsPackagesSyncer) Fetch(ctx context.Context, repo api.RepoName, dir co
 		return nil, err
 	}
 
-	if revspec != "" {
-		return nil, s.fetchRevspec(ctx, name, dir, versions, revspec)
-	}
-
 	return nil, s.fetchVersions(ctx, name, dir, versions)
 }
 
-// fetchRevspec fetches the given revspec if it's not contained in
-// existingVersions. If download and upserting the new version into database
-// succeeds, it calls s.fetchVersions with the newly-added version and the old
-// ones, to possibly update the "latest" tag.
-func (s *vcsPackagesSyncer) fetchRevspec(ctx context.Context, name reposource.PackageName, dir common.GitDir, existingVersions []string, revspec string) error {
-	// Optionally try to resolve the version of the user-provided revspec (formatted as `"v${VERSION}^0"`).
-	// This logic lives inside `vcsPackagesSyncer` meaning this repo must be a package repo where all
-	// the git tags are created by our npm/crates/pypi/maven integrations (no human commits/branches/tags).
-	// Package repos only create git tags using the format `"v${VERSION}"`.
-	//
-	// Unlike other versions, we silently ignore all errors from resolving requestedVersion because it could
-	// be any random user-provided string, with no guarantee that it's a valid version string that resolves
-	// to an existing dependency version.
-	//
-	// We assume the revspec is formatted as `"v${VERSION}^0"` but it could be any random string or
-	// a git commit SHA. It should be harmless if the string is invalid, worst case the resolution fails
-	// and we silently ignore the error.
-	requestedVersion := strings.TrimSuffix(strings.TrimPrefix(revspec, "v"), "^0")
-
-	for _, existingVersion := range existingVersions {
-		if existingVersion == requestedVersion {
-			return nil
-		}
-	}
-
-	dep, err := s.source.ParseVersionedPackageFromNameAndVersion(name, requestedVersion)
-	if err != nil {
-		// Invalid version. Silently ignore error, see comment above why.
-		return nil
-	}
-
-	// if the next check passes, we know that any filters added/updated before this timestamp did not block it
-	instant := time.Now()
-
-	if allowed, err := s.svc.IsPackageRepoVersionAllowed(ctx, s.scheme, dep.PackageSyntax(), dep.PackageVersion()); !allowed || err != nil {
-		// if err == nil && !allowed, this will return nil
-		return errors.Wrap(err, "error checking if package repo version is allowed")
-	}
-
-	err = s.gitPushDependencyTag(ctx, string(dir), dep)
-	if err != nil {
-		// Package could not be downloaded. Silently ignore error, see comment above why.
-		return nil
-	}
-
-	if _, _, err = s.svc.InsertPackageRepoRefs(ctx, []dependencies.MinimalPackageRepoRef{
-		{
-			Scheme:        dep.Scheme(),
-			Name:          dep.PackageSyntax(),
-			Versions:      []dependencies.MinimalPackageRepoRefVersion{{Version: dep.PackageVersion(), LastCheckedAt: &instant}},
-			LastCheckedAt: &instant,
-		},
-	}); err != nil {
-		// We don't want to ignore when writing to the database failed, since
-		// we've already downloaded the package successfully.
-		return err
-	}
-
-	existingVersions = append(existingVersions, requestedVersion)
-
-	return s.fetchVersions(ctx, name, dir, existingVersions)
-}
-
 // fetchVersions checks whether the given versions are all valid version
 // specifiers, then checks whether they've already been downloaded and, if not,
 // downloads them.