gitserver: Remove revspec updates

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/packages_syncer.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.