unify commit priority logic

Author: dmcilvaney

internal/providers/sourceproviders/fedorasourceprovider.go

@@ -221,64 +221,80 @@ func (g *FedoraSourcesProviderImpl) renameSpecIfNeeded(dir, upstreamName, compon
 	return nil
 }
 
-// checkoutTargetCommit determines the appropriate commit to use and checks it out.
-// Priority order:
-//  1. Explicit upstream commit hash - specified per-component via upstream-commit
-//  2. Upstream distro snapshot - snapshot time from the provider's resolved distro
-//  3. Default - use current HEAD (no checkout needed)
+// checkoutTargetCommit resolves the effective commit via [resolveEffectiveCommitHash]
+// and checks it out in the cloned repository.
 func (g *FedoraSourcesProviderImpl) checkoutTargetCommit(
 	ctx context.Context,
 	upstreamCommit string,
 	repoDir string,
 ) error {
-	// Case 1: Explicit upstream commit hash specified per-component
-	if upstreamCommit != "" {
-		slog.Info("Using explicit upstream commit hash",
-			"commitHash", upstreamCommit)
+	commitHash, err := g.resolveEffectiveCommitHash(ctx, repoDir, upstreamCommit, slog.LevelInfo)
+	if err != nil {
+		return err
+	}
 
-		if err := g.gitProvider.Checkout(ctx, repoDir, upstreamCommit); err != nil {
-			return fmt.Errorf("failed to checkout upstream commit %#q:\n%w", upstreamCommit, err)
-		}
+	if err := g.gitProvider.Checkout(ctx, repoDir, commitHash); err != nil {
+		return fmt.Errorf("failed to checkout commit %#q:\n%w", commitHash, err)
+	}
 
-		return nil
+	return nil
+}
+
+// resolveEffectiveCommitHash is the single source of truth for which commit a
+// component should use from a cloned repository.
+//
+// Priority:
+//  1. Explicit upstream commit hash (pinned per-component).
+//  2. Snapshot time — commit immediately before the snapshot date.
+//  3. Default — current HEAD.
+func (g *FedoraSourcesProviderImpl) resolveEffectiveCommitHash(
+	ctx context.Context,
+	repoDir string,
+	upstreamCommit string,
+	logLevel slog.Level,
+) (string, error) {
+	// Case 1: Explicit upstream commit hash specified per-component.
+	if upstreamCommit != "" {
+		slog.Log(ctx, logLevel, "Using explicit upstream commit hash", "commitHash", upstreamCommit)
+
+		return upstreamCommit, nil
 	}
 
-	// Case 2: Provider has a snapshot time configured from the resolved distro
+	// Case 2: Provider has a snapshot time configured from the resolved distro.
 	if g.snapshotTime != "" {
 		snapshotDateTime, err := time.Parse(time.RFC3339, g.snapshotTime)
 		if err != nil {
-			return fmt.Errorf("invalid snapshot time %#q:\n%w", g.snapshotTime, err)
+			return "", fmt.Errorf("invalid snapshot time %#q:\n%w", g.snapshotTime, err)
 		}
 
 		commitHash, err := g.gitProvider.GetCommitHashBeforeDate(ctx, repoDir, snapshotDateTime)
 		if err != nil {
-			return fmt.Errorf("failed to get commit hash for snapshot time %s:\n%w",
+			return "", fmt.Errorf("resolving commit for snapshot time %s:\n%w",
 				snapshotDateTime.Format(time.RFC3339), err)
 		}
 
-		slog.Info("Using upstream distro snapshot time",
+		slog.Log(ctx, logLevel, "Using upstream distro snapshot time",
 			"snapshotDateTime", snapshotDateTime.Format(time.RFC3339),
 			"commitHash", commitHash)
 
-		if err := g.gitProvider.Checkout(ctx, repoDir, commitHash); err != nil {
-			return fmt.Errorf("failed to checkout snapshot commit %#q:\n%w", commitHash, err)
-		}
+		return commitHash, nil
+	}
 
-		return nil
+	// Case 3: Default — use current HEAD.
+	commitHash, err := g.gitProvider.GetCurrentCommit(ctx, repoDir)
+	if err != nil {
+		return "", fmt.Errorf("resolving current HEAD commit:\n%w", err)
 	}
 
-	// Case 3: Default - use current HEAD (already checked out by clone)
-	slog.Info("Using current HEAD (no snapshot time configured)")
+	slog.Log(ctx, logLevel, "Using current HEAD", "commitHash", commitHash)
 
-	return nil
+	return commitHash, nil
 }
 
 // ResolveSourceIdentity implements [SourceIdentityProvider] by resolving the upstream
-// commit hash for the component. Resolution priority matches [checkoutTargetCommit]:
-//  1. Explicit upstream commit hash (pinned per-component) — returned directly.
-//  2. Snapshot time — perform a metadata-only clone of the dist-git branch and use the
-//     local git history to find the commit immediately before the snapshot date.
-//  3. Default — perform a metadata-only clone of the dist-git branch and use its current HEAD.
+// commit hash for the component. Resolution priority is defined by
+// [resolveEffectiveCommitHash]. Cases 2 and 3 require a metadata-only clone of the
+// dist-git branch; case 1 (pinned commit) is returned directly without a network call.
 func (g *FedoraSourcesProviderImpl) ResolveSourceIdentity(
 	ctx context.Context,
 	component components.Component,
@@ -307,8 +323,8 @@ func (g *FedoraSourcesProviderImpl) ResolveSourceIdentity(
 	return g.resolveCommit(ctx, gitRepoURL, upstreamName)
 }
 
-// resolveCommit clones the branch and determines the effective commit, either
-// at the snapshot time, or at the latest commit if no snapshot time is configured.
+// resolveCommit clones the branch and determines the effective commit via
+// [resolveEffectiveCommitHash].
 func (g *FedoraSourcesProviderImpl) resolveCommit(
 	ctx context.Context, gitRepoURL string, upstreamName string,
 ) (string, error) {
@@ -341,30 +357,12 @@ func (g *FedoraSourcesProviderImpl) resolveCommit(
 		return "", fmt.Errorf("partial clone for identity of %#q:\n%w", upstreamName, err)
 	}
 
-	var commitHash string
-
-	if g.snapshotTime != "" {
-		snapshotDateTime, parseErr := time.Parse(time.RFC3339, g.snapshotTime)
-		if parseErr != nil {
-			return "", fmt.Errorf("invalid snapshot time %#q:\n%w", g.snapshotTime, parseErr)
-		}
-
-		commitHash, err = g.gitProvider.GetCommitHashBeforeDate(ctx, tempDir, snapshotDateTime)
-		if err != nil {
-			return "", fmt.Errorf("resolving snapshot commit for %#q at %s:\n%w",
-				upstreamName, snapshotDateTime.Format(time.RFC3339), err)
-		}
-	} else {
-		commitHash, err = g.gitProvider.GetCurrentCommit(ctx, tempDir)
-		if err != nil {
-			return "", fmt.Errorf("resolving current commit for %#q:\n%w", upstreamName, err)
-		}
+	// upstreamCommit is "" here — the pinned-commit case is handled by the
+	// caller (ResolveSourceIdentity) before cloning.
+	commitHash, err := g.resolveEffectiveCommitHash(ctx, tempDir, "", slog.LevelDebug)
+	if err != nil {
+		return "", fmt.Errorf("resolving commit for %#q:\n%w", upstreamName, err)
 	}
 
-	slog.Debug("Resolved snapshot commit for identity",
-		"component", upstreamName,
-		"snapshot", g.snapshotTime,
-		"commit", commitHash)
-
 	return commitHash, nil
 }

internal/providers/sourceproviders/fedorasourceprovider_test.go

@@ -169,6 +169,14 @@ func TestGetComponentFromGit(t *testing.T) {
 				return fileutils.WriteFile(env.FS(), sourcesPath, []byte(sourcesContent), fileperms.PublicFile)
 			})
 
+		mockGitProvider.EXPECT().
+			GetCurrentCommit(gomock.Any(), gomock.Any()).
+			Return("head123abc", nil)
+
+		mockGitProvider.EXPECT().
+			Checkout(gomock.Any(), gomock.Any(), "head123abc").
+			Return(nil)
+
 		provider, err := sourceproviders.NewFedoraSourcesProviderImpl(
 			env.FS(),
 			env.DryRunnable,
@@ -255,6 +263,14 @@ func TestGetComponentFromGit(t *testing.T) {
 				return fileutils.WriteFile(env.FS(), sourcesPath, []byte(sourcesContent), fileperms.PublicFile)
 			})
 
+		mockGitProvider.EXPECT().
+			GetCurrentCommit(gomock.Any(), gomock.Any()).
+			Return("head123abc", nil)
+
+		mockGitProvider.EXPECT().
+			Checkout(gomock.Any(), gomock.Any(), "head123abc").
+			Return(nil)
+
 		const testDestDir = "/output-preexisting"
 
 		provider, err := sourceproviders.NewFedoraSourcesProviderImpl(
@@ -323,6 +339,14 @@ func TestGetComponentFromGit(t *testing.T) {
 				return fileutils.WriteFile(env.FS(), specPath, []byte("Name: "+testPackageName), fileperms.PublicFile)
 			})
 
+		mockGitProvider.EXPECT().
+			GetCurrentCommit(gomock.Any(), gomock.Any()).
+			Return("head123abc", nil)
+
+		mockGitProvider.EXPECT().
+			Checkout(gomock.Any(), gomock.Any(), "head123abc").
+			Return(nil)
+
 		provider, err := sourceproviders.NewFedoraSourcesProviderImpl(
 			env.FS(),
 			env.DryRunnable,
@@ -437,6 +461,14 @@ func TestGetComponentFromGit(t *testing.T) {
 			Clone(gomock.Any(), repoURL, gomock.Any(), gomock.Any()).
 			Return(nil)
 
+		mockGitProvider.EXPECT().
+			GetCurrentCommit(gomock.Any(), gomock.Any()).
+			Return("head123abc", nil)
+
+		mockGitProvider.EXPECT().
+			Checkout(gomock.Any(), gomock.Any(), "head123abc").
+			Return(nil)
+
 		// But extractor fails - note it receives the component name, not destDir
 		extractorError := errors.New("extraction failed")
 		mockExtractor.EXPECT().
@@ -490,6 +522,14 @@ func TestGetComponentFromGit(t *testing.T) {
 				)
 			})
 
+		mockGitProvider.EXPECT().
+			GetCurrentCommit(gomock.Any(), gomock.Any()).
+			Return("head123abc", nil)
+
+		mockGitProvider.EXPECT().
+			Checkout(gomock.Any(), gomock.Any(), "head123abc").
+			Return(nil)
+
 		// Extractor succeeds
 		mockExtractor.EXPECT().
 			ExtractSourcesFromRepo(gomock.Any(), gomock.Any(), upstreamName, gomock.Any(), gomock.Any()).
@@ -544,6 +584,14 @@ func TestGetComponentFromGit(t *testing.T) {
 			Clone(gomock.Any(), upstreamRepoURL, gomock.Any(), gomock.Any()).
 			Return(nil)
 
+		mockGitProvider.EXPECT().
+			GetCurrentCommit(gomock.Any(), gomock.Any()).
+			Return("head123abc", nil)
+
+		mockGitProvider.EXPECT().
+			Checkout(gomock.Any(), gomock.Any(), "head123abc").
+			Return(nil)
+
 		// Extractor succeeds
 		mockExtractor.EXPECT().
 			ExtractSourcesFromRepo(gomock.Any(), gomock.Any(), upstreamName, gomock.Any(), gomock.Any()).
@@ -652,7 +700,17 @@ func TestCheckoutTargetCommit(t *testing.T) {
 				return fileutils.WriteFile(env.FS(), specPath, []byte("Name: "+testPackageName), fileperms.PublicFile)
 			})
 
-		// Should NOT call GetCommitHashBeforeDate or Checkout - uses HEAD from clone
+		headCommitHash := "head123abc"
+
+		// GetCurrentCommit is called to resolve HEAD
+		mockGitProvider.EXPECT().
+			GetCurrentCommit(gomock.Any(), gomock.Any()).
+			Return(headCommitHash, nil)
+
+		// Then checkout that commit
+		mockGitProvider.EXPECT().
+			Checkout(gomock.Any(), gomock.Any(), headCommitHash).
+			Return(nil)
 
 		// Extractor succeeds
 		mockExtractor.EXPECT().
@@ -700,7 +758,7 @@ func TestCheckoutTargetCommit(t *testing.T) {
 
 		err = provider.GetComponent(context.Background(), mockComponent, destDir)
 		require.Error(t, err)
-		assert.Contains(t, err.Error(), "failed to get commit hash for snapshot time")
+		assert.Contains(t, err.Error(), "resolving commit for snapshot time")
 		assert.ErrorIs(t, err, hashError)
 	})
 
@@ -747,7 +805,7 @@ func TestCheckoutTargetCommit(t *testing.T) {
 
 		err = provider.GetComponent(context.Background(), mockComponent, destDir)
 		require.Error(t, err)
-		assert.Contains(t, err.Error(), "failed to checkout snapshot commit")
+		assert.Contains(t, err.Error(), "failed to checkout commit")
 		assert.ErrorIs(t, err, checkoutError)
 	})
 
@@ -929,7 +987,7 @@ func TestCheckoutTargetCommit_UpstreamCommit(t *testing.T) {
 
 		err = provider.GetComponent(context.Background(), mockComponent, destDir)
 		require.Error(t, err)
-		assert.Contains(t, err.Error(), "failed to checkout upstream commit")
+		assert.Contains(t, err.Error(), "failed to checkout commit")
 		assert.ErrorIs(t, err, checkoutError)
 	})
 }