From 8f925c6754d569e4ac03fbfa9cef3f6e1c93fd2f Mon Sep 17 00:00:00 2001 From: "gcp-cherry-pick-bot[bot]" <98988430+gcp-cherry-pick-bot[bot]@users.noreply.github.com> Date: Wed, 26 Feb 2025 20:12:02 +0530 Subject: [PATCH] fix: fetch syncedRevision in UpdateRevisionForPaths (#21014) (cherry-pick #21015) (#22011) Signed-off-by: toyamagu2021 Signed-off-by: toyamagu-2021 Co-authored-by: gussan <83329336+toyamagu-2021@users.noreply.github.com> --- reposerver/repository/repository.go | 48 ++++++++++++++ reposerver/repository/repository_test.go | 80 +++++++++++++++++++++--- 2 files changed, 121 insertions(+), 7 deletions(-) diff --git a/reposerver/repository/repository.go b/reposerver/repository/repository.go index 11bc154d4f69c..286eb7eb0f215 100644 --- a/reposerver/repository/repository.go +++ b/reposerver/repository/repository.go @@ -2511,6 +2511,50 @@ func (s *Service) checkoutRevision(gitClient git.Client, revision string, submod return closer, err } +// fetch is a convenience function to fetch revisions +// We assumed that the caller has already initialized the git repo, i.e. gitClient.Init() has been called +func (s *Service) fetch(gitClient git.Client, targetRevisions []string) error { + err := fetch(gitClient, targetRevisions) + if err != nil { + for _, revision := range targetRevisions { + s.metricsServer.IncGitFetchFail(gitClient.Root(), revision) + } + } + return err +} + +func fetch(gitClient git.Client, targetRevisions []string) error { + revisionPresent := true + for _, revision := range targetRevisions { + revisionPresent = gitClient.IsRevisionPresent(revision) + if !revisionPresent { + break + } + } + // Fetching can be skipped if the revision is already present locally. + if revisionPresent { + return nil + } + // Fetching with no revision first. Fetching with an explicit version can cause repo bloat. https://github.com/argoproj/argo-cd/issues/8845 + err := gitClient.Fetch("") + if err != nil { + return err + } + for _, revision := range targetRevisions { + if !gitClient.IsRevisionPresent(revision) { + // When fetching with no revision, only refs/heads/* and refs/remotes/origin/* are fetched. If fetch fails + // for the given revision, try explicitly fetching it. + log.Infof("Failed to fetch revision %s: %v", revision, err) + log.Infof("Fallback to fetching specific revision %s. ref might not have been in the default refspec fetched.", revision) + + if err := gitClient.Fetch(revision); err != nil { + return status.Errorf(codes.Internal, "Failed to fetch revision %s: %v", revision, err) + } + } + } + return nil +} + func checkoutRevision(gitClient git.Client, revision string, submoduleEnabled bool) error { err := gitClient.Init() if err != nil { @@ -2855,6 +2899,10 @@ func (s *Service) UpdateRevisionForPaths(_ context.Context, request *apiclient.U } defer io.Close(closer) + if err := s.fetch(gitClient, []string{syncedRevision}); err != nil { + return nil, status.Errorf(codes.Internal, "unable to fetch git repo %s with syncedRevisions %s: %v", repo.Repo, syncedRevision, err) + } + files, err := gitClient.ChangedFiles(syncedRevision, revision) if err != nil { return nil, status.Errorf(codes.Internal, "unable to get changed files for repo %s with revision %s: %v", repo.Repo, revision, err) diff --git a/reposerver/repository/repository_test.go b/reposerver/repository/repository_test.go index d5f1061b3152c..ea10c92ad9183 100644 --- a/reposerver/repository/repository_test.go +++ b/reposerver/repository/repository_test.go @@ -3090,6 +3090,61 @@ func TestCheckoutRevisionNotPresentCallFetch(t *testing.T) { require.NoError(t, err) } +func TestFetch(t *testing.T) { + revision1 := "0123456789012345678901234567890123456789" + revision2 := "abcdefabcdefabcdefabcdefabcdefabcdefabcd" + + gitClient := &gitmocks.Client{} + gitClient.On("Init").Return(nil) + gitClient.On("IsRevisionPresent", revision1).Once().Return(true) + gitClient.On("IsRevisionPresent", revision2).Once().Return(false) + gitClient.On("Fetch", "").Return(nil) + gitClient.On("IsRevisionPresent", revision1).Once().Return(true) + gitClient.On("IsRevisionPresent", revision2).Once().Return(true) + + err := fetch(gitClient, []string{revision1, revision2}) + require.NoError(t, err) +} + +// TestFetchRevisionCanGetNonstandardRefs shows that we can fetch a revision that points to a non-standard ref. In +func TestFetchRevisionCanGetNonstandardRefs(t *testing.T) { + rootPath := t.TempDir() + + sourceRepoPath, err := os.MkdirTemp(rootPath, "") + require.NoError(t, err) + + // Create a repo such that one commit is on a non-standard ref _and nowhere else_. This is meant to simulate, for + // example, a GitHub ref for a pull into one repo from a fork of that repo. + runGit(t, sourceRepoPath, "init") + runGit(t, sourceRepoPath, "checkout", "-b", "main") // make sure there's a main branch to switch back to + runGit(t, sourceRepoPath, "commit", "-m", "empty", "--allow-empty") + runGit(t, sourceRepoPath, "checkout", "-b", "branch") + runGit(t, sourceRepoPath, "commit", "-m", "empty", "--allow-empty") + sha := runGit(t, sourceRepoPath, "rev-parse", "HEAD") + runGit(t, sourceRepoPath, "update-ref", "refs/pull/123/head", strings.TrimSuffix(sha, "\n")) + runGit(t, sourceRepoPath, "checkout", "main") + runGit(t, sourceRepoPath, "branch", "-D", "branch") + + destRepoPath, err := os.MkdirTemp(rootPath, "") + require.NoError(t, err) + + gitClient, err := git.NewClientExt("file://"+sourceRepoPath, destRepoPath, &git.NopCreds{}, true, false, "", "") + require.NoError(t, err) + + // We should initialize repository + err = gitClient.Init() + require.NoError(t, err) + + pullSha, err := gitClient.LsRemote("refs/pull/123/head") + require.NoError(t, err) + + err = fetch(gitClient, []string{"does-not-exist"}) + require.Error(t, err) + + err = fetch(gitClient, []string{pullSha}) + require.NoError(t, err) +} + // runGit runs a git command in the given working directory. If the command succeeds, it returns the combined standard // and error output. If it fails, it stops the test with a failure message. func runGit(t *testing.T, workDir string, args ...string) string { @@ -3765,9 +3820,13 @@ func TestUpdateRevisionForPaths(t *testing.T) { {name: "ChangedFilesDoNothing", fields: func() fields { s, _, c := newServiceWithOpt(t, func(gitClient *gitmocks.Client, helmClient *helmmocks.Client, paths *iomocks.TempPaths) { gitClient.On("Init").Return(nil) - gitClient.On("IsRevisionPresent", mock.Anything).Return(false) - gitClient.On("Fetch", mock.Anything).Return(nil) - gitClient.On("Checkout", mock.Anything, mock.Anything).Return("", nil) + gitClient.On("Fetch", mock.Anything).Once().Return(nil) + gitClient.On("IsRevisionPresent", "632039659e542ed7de0c170a4fcc1c571b288fc0").Once().Return(false) + gitClient.On("Checkout", "632039659e542ed7de0c170a4fcc1c571b288fc0", mock.Anything).Once().Return("", nil) + // fetch + gitClient.On("IsRevisionPresent", "1e67a504d03def3a6a1125d934cb511680f72555").Once().Return(false) + gitClient.On("Fetch", mock.Anything).Once().Return(nil) + gitClient.On("IsRevisionPresent", "1e67a504d03def3a6a1125d934cb511680f72555").Once().Return(true) gitClient.On("LsRemote", "HEAD").Once().Return("632039659e542ed7de0c170a4fcc1c571b288fc0", nil) gitClient.On("LsRemote", "SYNCEDHEAD").Once().Return("1e67a504d03def3a6a1125d934cb511680f72555", nil) paths.On("GetPath", mock.Anything).Return(".", nil) @@ -3794,9 +3853,13 @@ func TestUpdateRevisionForPaths(t *testing.T) { {name: "NoChangesUpdateCache", fields: func() fields { s, _, c := newServiceWithOpt(t, func(gitClient *gitmocks.Client, helmClient *helmmocks.Client, paths *iomocks.TempPaths) { gitClient.On("Init").Return(nil) - gitClient.On("IsRevisionPresent", mock.Anything).Return(false) - gitClient.On("Fetch", mock.Anything).Return(nil) + gitClient.On("Fetch", mock.Anything).Once().Return(nil) + gitClient.On("IsRevisionPresent", "632039659e542ed7de0c170a4fcc1c571b288fc0").Once().Return(false) gitClient.On("Checkout", mock.Anything, mock.Anything).Return("", nil) + gitClient.On("IsRevisionPresent", "1e67a504d03def3a6a1125d934cb511680f72555").Once().Return(false) + // fetch + gitClient.On("Fetch", mock.Anything).Once().Return(nil) + gitClient.On("IsRevisionPresent", "1e67a504d03def3a6a1125d934cb511680f72555").Once().Return(true) gitClient.On("LsRemote", "HEAD").Once().Return("632039659e542ed7de0c170a4fcc1c571b288fc0", nil) gitClient.On("LsRemote", "SYNCEDHEAD").Once().Return("1e67a504d03def3a6a1125d934cb511680f72555", nil) paths.On("GetPath", mock.Anything).Return(".", nil) @@ -3832,9 +3895,12 @@ func TestUpdateRevisionForPaths(t *testing.T) { {name: "NoChangesHelmMultiSourceUpdateCache", fields: func() fields { s, _, c := newServiceWithOpt(t, func(gitClient *gitmocks.Client, helmClient *helmmocks.Client, paths *iomocks.TempPaths) { gitClient.On("Init").Return(nil) - gitClient.On("IsRevisionPresent", mock.Anything).Return(false) - gitClient.On("Fetch", mock.Anything).Return(nil) + gitClient.On("IsRevisionPresent", "632039659e542ed7de0c170a4fcc1c571b288fc0").Once().Return(false) + gitClient.On("Fetch", mock.Anything).Once().Return(nil) gitClient.On("Checkout", mock.Anything, mock.Anything).Return("", nil) + // fetch + gitClient.On("IsRevisionPresent", "1e67a504d03def3a6a1125d934cb511680f72555").Once().Return(true) + gitClient.On("Fetch", mock.Anything).Once().Return(nil) gitClient.On("LsRemote", "HEAD").Once().Return("632039659e542ed7de0c170a4fcc1c571b288fc0", nil) gitClient.On("LsRemote", "SYNCEDHEAD").Once().Return("1e67a504d03def3a6a1125d934cb511680f72555", nil) paths.On("GetPath", mock.Anything).Return(".", nil)