From 1ce393cbc47c159aeb43da0be1bbba04e8ab5412 Mon Sep 17 00:00:00 2001
From: Bruno Meneguele <bmeneg@redhat.com>
Date: Mon, 7 Feb 2022 16:49:51 -0300
Subject: [PATCH] cmd/mr_checkout: fix fetch reference existence check

When the tracking mode was enabled (`-t`) the reference being passed to
`git show-ref` was being wrongly composed, since the code was always
expecting a simple branch name instead of a full remote ref. This patch
fixes it by separating the check in two steps: 1) check if the local
branch to which the MR will be checked-out exists and 2) if tracking is
enabled, check if the remote ref already exists. Both are influenced by
`--force`.

Signed-off-by: Bruno Meneguele <bmeneg@redhat.com>
---
 cmd/mr_checkout.go      | 40 +++++++++++++++++++++++++++-------------
 cmd/mr_checkout_test.go |  2 +-
 2 files changed, 28 insertions(+), 14 deletions(-)

diff --git a/cmd/mr_checkout.go b/cmd/mr_checkout.go
index c6c123e5..d191e67a 100644
--- a/cmd/mr_checkout.go
+++ b/cmd/mr_checkout.go
@@ -2,7 +2,6 @@ package cmd
 
 import (
 	"fmt"
-	"os"
 
 	"github.com/MakeNowJust/heredoc/v2"
 	"github.com/rsteube/carapace"
@@ -63,6 +62,18 @@ var checkoutCmd = &cobra.Command{
 		if mrCheckoutCfg.branch == "" {
 			mrCheckoutCfg.branch = mr.SourceBranch
 		}
+
+		err = git.New("show-ref", "--verify", "--quiet", "refs/heads/"+mrCheckoutCfg.branch).Run()
+		if err == nil {
+			if mrCheckoutCfg.force {
+				if err := git.New("branch", "-D", mrCheckoutCfg.branch).Run(); err != nil {
+					log.Fatal(err)
+				}
+			} else {
+				log.Fatalf("branch %s already exists", mrCheckoutCfg.branch)
+			}
+		}
+
 		// By default, fetch to configured branch
 		fetchToRef := mrCheckoutCfg.branch
 
@@ -101,26 +112,29 @@ var checkoutCmd = &cobra.Command{
 				}
 			}
 
-			fetchToRef = fmt.Sprintf("refs/remotes/%s/%s", remoteName, mr.SourceBranch)
-		}
-
-		if err := git.New("show-ref", "--verify", "--quiet", "refs/heads/"+fetchToRef).Run(); err == nil {
-			if mrCheckoutCfg.force {
-				if err := git.New("branch", "-D", mrCheckoutCfg.branch).Run(); err != nil {
-					log.Fatal(err)
+			trackRef := fmt.Sprintf("refs/remotes/%s/%s", remoteName, mr.SourceBranch)
+			err = git.New("show-ref", "--verify", "--quiet", trackRef).Run()
+			if err == nil {
+				if mrCheckoutCfg.force {
+					err = git.New("update-ref", "-d", trackRef).Run()
+					if err != nil {
+						log.Fatal(err)
+					}
+				} else {
+					log.Fatalf("remote reference %s already exists", trackRef)
 				}
-			} else {
-				fmt.Println("ERROR: mr", mrID, "branch", fetchToRef, "already exists.")
-				os.Exit(1)
 			}
+
+			fetchToRef = trackRef
 		}
 
-		// https://docs.gitlab.com/ce/user/project/merge_requests/#checkout-merge-requests-locally
+		// https://docs.gitlab.com/ee/user/project/merge_requests/reviews/#checkout-merge-requests-locally-through-the-head-ref
 		mrRef := fmt.Sprintf("refs/merge-requests/%d/head", mrID)
 		fetchRefSpec := fmt.Sprintf("%s:%s", mrRef, fetchToRef)
 		if err := git.New("fetch", targetRemote, fetchRefSpec).Run(); err != nil {
 			log.Fatal(err)
 		}
+
 		if mrCheckoutCfg.track {
 			// Create configured branch with tracking from fetchToRef
 			// git branch --flags <branchname> [<start-point>]
@@ -141,7 +155,7 @@ func init() {
 	checkoutCmd.Flags().BoolVarP(&mrCheckoutCfg.track, "track", "t", false, "set branch to track remote branch, adds remote if needed")
 	// useHTTP is defined in "project_create.go"
 	checkoutCmd.Flags().BoolVar(&useHTTP, "http", false, "checkout using HTTP protocol instead of SSH")
-	checkoutCmd.Flags().BoolVarP(&mrCheckoutCfg.force, "force", "f", false, "force branch checkout and override existing branch")
+	checkoutCmd.Flags().BoolVarP(&mrCheckoutCfg.force, "force", "f", false, "force branch and remote reference override")
 	mrCmd.AddCommand(checkoutCmd)
 	carapace.Gen(checkoutCmd).PositionalCompletion(
 		carapace.ActionCallback(func(c carapace.Context) carapace.Action {
diff --git a/cmd/mr_checkout_test.go b/cmd/mr_checkout_test.go
index 366f47f8..bd676d2a 100644
--- a/cmd/mr_checkout_test.go
+++ b/cmd/mr_checkout_test.go
@@ -151,7 +151,7 @@ func Test_mrDoubleCheckoutFailCmdRun(t *testing.T) {
 		t.Log(eLog)
 		t.Fatal(err)
 	}
-	require.Contains(t, eLog, "ERROR: mr 1 branch mrtest already exists")
+	require.Contains(t, eLog, "branch mrtest already exists")
 }
 
 func Test_mrDoubleCheckoutForceRun(t *testing.T) {