Skip to content

Commit

Permalink
fix: fetch syncedRevision in UpdateRevisionForPaths (#21014) (cherry-…
Browse files Browse the repository at this point in the history 
…pick #21015) (#22011)

Signed-off-by: toyamagu2021 <toyamagu2021@gmail.com>
Signed-off-by: toyamagu-2021 <toyamagu2021@gmail.com>
Co-authored-by: gussan <83329336+toyamagu-2021@users.noreply.github.com>
  • Loading branch information
@gcp-cherry-pick-bot @toyamagu-2021
gcp-cherry-pick-bot[bot] and toyamagu-2021 authored Feb 26, 2025
1 parent b5be1df commit 8f925c6
Show file tree
Hide file tree
Showing 2 changed files with 121 additions and 7 deletions.
48 changes: 48 additions & 0 deletions reposerver/repository/repository.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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)
Expand Down
80 changes: 73 additions & 7 deletions reposerver/repository/repository_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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)
Expand All @@ -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)
Expand Down Expand Up @@ -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)
Expand Down

0 comments on commit 8f925c6

Please sign in to comment.