repo sync: Fix deleting wrong tracking branch for renamed branches (#401

)

If a branch is renamed after submission and then merged,
the tracking branch name does not match the local branch name.

This case must be correctly handled when deleting the tracking branch:
if we know the upstream branch name use `$remote/$upstreamBranch`
instead of `$remote/$branch`.

Also drop the outdated comment about how we detect merged branches
sinc we're indeed doing both (1) and (2) now.

.changes/unreleased/Fixed-20240914-120358.yaml

@@ -0,0 +1,3 @@
+kind: Fixed
+body: 'repo sync: Delete the correct tracking branch for renamed branches.'
+time: 2024-09-14T12:03:58.254791-07:00

repo_sync.go

@@ -204,19 +204,6 @@ func (cmd *repoSyncCmd) deleteMergedBranches(
 	remoteRepo forge.Repository,
 	opts *globalOptions,
 ) error {
-	// There are two options for detecting merged branches:
-	//
-	// 1. Query the CR status for each submitted branch.
-	//    This is more accurate, but requires a lot of API calls.
-	// 2. List recently merged PRs and match against tracked branches.
-	//    The number of API calls here is smaller,
-	//    but the matching is less accurate because the CR may have been
-	//    submitted by someone else.
-	//
-	// For now, we'll go for (1) with the assumption that the number of
-	// submitted branches is small enough that we can afford the API calls.
-	// In the future, we may need a hybrid approach that switches to (2).
-
 	knownBranches, err := svc.LoadBranches(ctx)
 	if err != nil {
 		return fmt.Errorf("list tracked branches: %w", err)
@@ -226,6 +213,9 @@ func (cmd *repoSyncCmd) deleteMergedBranches(
 		Name   string
 		Change forge.ChangeID
 		Merged bool
+
+		// Branch name pushed to the remote.
+		UpstreamBranch string
 	}
 
 	type trackedBranch struct {
@@ -235,6 +225,9 @@ func (cmd *repoSyncCmd) deleteMergedBranches(
 		Merged        bool
 		RemoteHeadSHA git.Hash
 		LocalHeadSHA  git.Hash
+
+		// Branch name pushed to the remote.
+		UpstreamBranch string
 	}
 
 	// There are two kinds of branches under consideration:
@@ -254,18 +247,27 @@ func (cmd *repoSyncCmd) deleteMergedBranches(
 		trackedBranches   []*trackedBranch
 	)
 	for _, b := range knownBranches {
+		upstreamBranch := b.UpstreamBranch
+		if upstreamBranch == "" {
+			upstreamBranch = b.Name
+		}
+
 		if b.Change != nil {
 			b := &submittedBranch{
-				Name:   b.Name,
-				Change: b.Change.ChangeID(),
+				Name:           b.Name,
+				Change:         b.Change.ChangeID(),
+				UpstreamBranch: upstreamBranch,
 			}
 			submittedBranches = append(submittedBranches, b)
 		} else {
 			// TODO:
 			// Filter down to only branches that have
 			// a remote tracking branch:
 			// either $remote/$UpstreamBranch or $remote/$branch exists.
-			b := &trackedBranch{Name: b.Name}
+			b := &trackedBranch{
+				Name:           b.Name,
+				UpstreamBranch: upstreamBranch,
+			}
 			trackedBranches = append(trackedBranches, b)
 		}
 	}
@@ -338,14 +340,22 @@ func (cmd *repoSyncCmd) deleteMergedBranches(
 	}
 	wg.Wait()
 
-	var branchesToDelete []string
+	type branchDeletion struct {
+		BranchName   string
+		UpstreamName string
+	}
+
+	var branchesToDelete []branchDeletion
 	for _, branch := range submittedBranches {
 		if !branch.Merged {
 			continue
 		}
 
 		log.Infof("%v: %v was merged", branch.Name, branch.Change)
-		branchesToDelete = append(branchesToDelete, branch.Name)
+		branchesToDelete = append(branchesToDelete, branchDeletion{
+			BranchName:   branch.Name,
+			UpstreamName: branch.UpstreamBranch,
+		})
 	}
 
 	for _, branch := range trackedBranches {
@@ -355,7 +365,10 @@ func (cmd *repoSyncCmd) deleteMergedBranches(
 
 		if branch.RemoteHeadSHA == branch.LocalHeadSHA {
 			log.Infof("%v: %v was merged", branch.Name, branch.Change)
-			branchesToDelete = append(branchesToDelete, branch.Name)
+			branchesToDelete = append(branchesToDelete, branchDeletion{
+				BranchName:   branch.Name,
+				UpstreamName: branch.UpstreamBranch,
+			})
 			continue
 		}
 
@@ -381,7 +394,10 @@ func (cmd *repoSyncCmd) deleteMergedBranches(
 		}
 
 		if shouldDelete {
-			branchesToDelete = append(branchesToDelete, branch.Name)
+			branchesToDelete = append(branchesToDelete, branchDeletion{
+				BranchName:   branch.Name,
+				UpstreamName: branch.UpstreamBranch,
+			})
 		}
 	}
 
@@ -390,7 +406,7 @@ func (cmd *repoSyncCmd) deleteMergedBranches(
 	// (e.g. from the bottom of the stack up)
 	for _, branch := range branchesToDelete {
 		err := (&branchDeleteCmd{
-			Branch: branch,
+			Branch: branch.BranchName,
 			Force:  true,
 		}).Run(ctx, log, opts)
 		if err != nil {
@@ -399,7 +415,7 @@ func (cmd *repoSyncCmd) deleteMergedBranches(
 
 		// Also delete the remote tracking branch for this branch
 		// if it still exists.
-		remoteBranch := remote + "/" + branch
+		remoteBranch := remote + "/" + branch.UpstreamName
 		if _, err := repo.PeelToCommit(ctx, remoteBranch); err == nil {
 			if err := repo.DeleteBranch(ctx, remoteBranch, git.BranchDeleteOptions{
 				Remote: true,

testdata/script/repo_sync_after_merging_renamed_branch.txt

@@ -0,0 +1,47 @@
+# If a branch is renamed after being submitted, and then merged,
+# repo sync should still delete the correct tracking branch.
+
+as 'Test <[email protected]>'
+at '2024-09-14T12:05:06Z'
+
+# setup
+cd repo
+git init
+git commit --allow-empty -m 'Initial commit'
+
+# set up a fake GitHub remote
+shamhub init
+shamhub new origin alice/example.git
+shamhub register alice
+git push origin main
+
+env SHAMHUB_USERNAME=alice
+gs auth login
+
+# create a new branch and submit it
+git add feature1.txt
+gs bc -m 'Add feature1' feature1
+gs branch submit --fill
+stderr 'Created #'
+
+# rename and re-submit
+gs branch rename feat1
+cp $WORK/extra/feature1-update.txt feature1.txt
+git add feature1.txt
+git commit -m 'update feature1'
+
+# merge the PR
+shamhub merge alice/example 1
+
+gs repo sync
+stderr 'feat1: deleted'
+
+# tracking branch must have been deleted
+! git rev-parse --verify origin/feature1
+
+-- repo/feature1.txt --
+Contents of feature1
+
+-- extra/feature1-update.txt --
+New contents of feature1
+