fix(git): handle empty delta bundles

Verify an empty delta against fresh upstream refs before returning the no-content outcome. Keep ref inspection and bundle creation consistent so concurrent fetches cannot turn an empty delta into an arbitrary Git failure.

Co-authored-by: Codex <noreply@openai.com>
Co-authored-by: Amp <amp@ampcode.com>
Amp-Thread-ID: https://ampcode.com/threads/T-01a0a667-aa3c-7519-8f18-52fce84e7724

Author: 3 people

internal/gitclone/manager.go

@@ -634,6 +634,7 @@ func (r *Repository) FetchVerified(ctx context.Context) error {
 }
 
 func (r *Repository) fetchInternal(ctx context.Context, timeout time.Duration, enforceSpeedLimit, coalesce bool) error {
+	lastFetch := r.LastFetch()
 	select {
 	case <-r.fetchSem:
 		defer func() {
@@ -642,22 +643,14 @@ func (r *Repository) fetchInternal(ctx context.Context, timeout time.Duration, e
 	case <-ctx.Done():
 		return errors.Wrap(ctx.Err(), "context cancelled before acquiring fetch semaphore")
 	default:
-		// The semaphore is held. Coalescing callers treat the holder's work as
-		// their fetch; verified callers wait their turn and fetch themselves.
-		if coalesce {
-			select {
-			case <-r.fetchSem:
-				r.fetchSem <- struct{}{}
-				return nil
-			case <-ctx.Done():
-				return errors.Wrap(ctx.Err(), "context cancelled while waiting for fetch")
-			}
-		}
 		select {
 		case <-r.fetchSem:
 			defer func() {
 				r.fetchSem <- struct{}{}
 			}()
+			if coalesce && r.LastFetch().After(lastFetch) {
+				return nil
+			}
 		case <-ctx.Done():
 			return errors.Wrap(ctx.Err(), "context cancelled before acquiring fetch semaphore")
 		}

internal/gitclone/manager_test.go

@@ -296,62 +296,65 @@ func TestRepository_NeedsFetch(t *testing.T) {
 	assert.False(t, repo.NeedsFetch(15*time.Minute))
 }
 
-func TestRepository_FetchVerifiedDoesNotCoalesce(t *testing.T) {
-	ctx := context.Background()
-	tmpDir := t.TempDir()
-	upstreamPath := createBareRepo(t, tmpDir)
-
-	clonePath := filepath.Join(tmpDir, "clone")
-	repo := &Repository{
-		state:       StateEmpty,
-		config:      testRepoConfig(),
-		path:        clonePath,
-		upstreamURL: upstreamPath,
-		fetchSem:    make(chan struct{}, 1),
-	}
-	repo.fetchSem <- struct{}{}
-	assert.NoError(t, repo.Clone(ctx))
+func TestRepository_FetchDoesNotCoalesceWithExclusion(t *testing.T) {
+	for _, verified := range []bool{false, true} {
+		t.Run(fmt.Sprintf("verified=%t", verified), func(t *testing.T) {
+			ctx := context.Background()
+			tmpDir := t.TempDir()
+			upstreamPath := createBareRepo(t, tmpDir)
+
+			clonePath := filepath.Join(tmpDir, "clone")
+			repo := &Repository{
+				state:       StateEmpty,
+				config:      testRepoConfig(),
+				path:        clonePath,
+				upstreamURL: upstreamPath,
+				fetchSem:    make(chan struct{}, 1),
+			}
+			repo.fetchSem <- struct{}{}
+			assert.NoError(t, repo.Clone(ctx))
+
+			workPath := filepath.Join(tmpDir, "work")
+			assert.NoError(t, os.WriteFile(filepath.Join(workPath, "f.txt"), []byte("y"), 0o644))
+			for _, args := range [][]string{
+				{"git", "-C", workPath, "commit", "-am", "update"},
+				{"git", "-C", workPath, "push", upstreamPath, "HEAD"},
+			} {
+				assert.NoError(t, exec.Command(args[0], args[1:]...).Run())
+			}
+			newSHAOut, err := exec.Command("git", "-C", workPath, "rev-parse", "HEAD").Output()
+			assert.NoError(t, err)
+			newSHA := strings.TrimSpace(string(newSHAOut))
+
+			holdSem := func(t *testing.T, fetch func() error) error {
+				t.Helper()
+				release := make(chan struct{})
+				entered := make(chan struct{})
+				holderDone := make(chan error, 1)
+				go func() {
+					holderDone <- repo.WithFetchExclusion(ctx, func() error {
+						close(entered)
+						<-release
+						return nil
+					})
+				}()
+				<-entered
+				fetchDone := make(chan error, 1)
+				go func() { fetchDone <- fetch() }()
+				time.Sleep(50 * time.Millisecond)
+				close(release)
+				assert.NoError(t, <-holderDone)
+				return <-fetchDone
+			}
 
-	// Advance upstream so the mirror is behind.
-	workPath := filepath.Join(tmpDir, "work")
-	assert.NoError(t, os.WriteFile(filepath.Join(workPath, "f.txt"), []byte("y"), 0o644))
-	for _, args := range [][]string{
-		{"git", "-C", workPath, "commit", "-am", "update"},
-		{"git", "-C", workPath, "push", upstreamPath, "HEAD"},
-	} {
-		assert.NoError(t, exec.Command(args[0], args[1:]...).Run())
+			fetch := repo.Fetch
+			if verified {
+				fetch = repo.FetchVerified
+			}
+			assert.NoError(t, holdSem(t, func() error { return fetch(ctx) }))
+			assert.True(t, repo.HasCommit(ctx, newSHA))
+		})
 	}
-	newSHAOut, err := exec.Command("git", "-C", workPath, "rev-parse", "HEAD").Output()
-	assert.NoError(t, err)
-	newSHA := strings.TrimSpace(string(newSHAOut))
-
-	holdSem := func(t *testing.T, fetch func() error) error {
-		t.Helper()
-		release := make(chan struct{})
-		holderDone := make(chan error, 1)
-		go func() {
-			holderDone <- repo.WithFetchExclusion(ctx, func() error {
-				<-release
-				return nil
-			})
-		}()
-		time.Sleep(20 * time.Millisecond)
-		fetchDone := make(chan error, 1)
-		go func() { fetchDone <- fetch() }()
-		time.Sleep(50 * time.Millisecond)
-		close(release)
-		assert.NoError(t, <-holderDone)
-		return <-fetchDone
-	}
-
-	// Fetch coalesces with the semaphore holder even though the holder was
-	// not fetching, so the mirror stays behind.
-	assert.NoError(t, holdSem(t, func() error { return repo.Fetch(ctx) }))
-	assert.False(t, repo.HasCommit(ctx, newSHA))
-
-	// FetchVerified waits for the holder and then runs its own fetch.
-	assert.NoError(t, holdSem(t, func() error { return repo.FetchVerified(ctx) }))
-	assert.True(t, repo.HasCommit(ctx, newSHA))
 }
 
 func TestParseGitRefs(t *testing.T) {