ExternalProject: Remote checkout needs to include the remote name

Commit 0aea435aa1 (ExternalProject: Provide choice of
git update strategies, 2020-02-12) added the git update
strategies, but the CHECKOUT strategy was not handling
remote refs correctly. The local ref would be checked out
instead and no warning or error would have been emitted.
The test that should have caught this was also malformed
and did not actually move the local master branch as intended.

2 files changed, 74 insertions, 56 deletions

diff --git a/Modules/ExternalProject-gitupdate.cmake.in b/Modules/ExternalProject-gitupdate.cmake.in
index e993c3c..eff39c1 100644
--- a/Modules/ExternalProject-gitupdate.cmake.in
+++ b/Modules/ExternalProject-gitupdate.cmake.in
@@ -59,7 +59,7 @@ if(error_code OR is_remote_ref OR NOT ("${tag_sha}" STREQUAL "${head_sha}"))
     message(FATAL_ERROR "Failed to fetch repository '@git_repository@'")
   endif()
 
-  if(is_remote_ref AND NOT "@git_update_strategy@" STREQUAL "CHECKOUT")
+  if(is_remote_ref)
     # Check if stash is needed
     execute_process(
       COMMAND "@git_EXECUTABLE@" status --porcelain
@@ -72,8 +72,8 @@ if(error_code OR is_remote_ref OR NOT ("${tag_sha}" STREQUAL "${head_sha}"))
     endif()
     string(LENGTH "${repo_status}" need_stash)
 
-    # If not in clean state, stash changes in order to be able to be able to
-    # perform git pull --rebase
+    # If not in clean state, stash changes in order to be able to perform a
+    # rebase or checkout without losing those changes permanently
     if(need_stash)
       execute_process(
         COMMAND "@git_EXECUTABLE@" stash save @git_stash_save_options@
@@ -85,66 +85,77 @@ if(error_code OR is_remote_ref OR NOT ("${tag_sha}" STREQUAL "${head_sha}"))
       endif()
     endif()
 
-    # Pull changes from the remote branch
-    execute_process(
-      COMMAND "@git_EXECUTABLE@" rebase "${git_remote}/${git_tag}"
-      WORKING_DIRECTORY "@work_dir@"
-      RESULT_VARIABLE error_code
-      OUTPUT_VARIABLE rebase_output
-      ERROR_VARIABLE  rebase_output
-      )
-    if(error_code)
-      # Rebase failed, undo the rebase attempt before continuing
+    if("@git_update_strategy@" STREQUAL "CHECKOUT")
       execute_process(
-        COMMAND "@git_EXECUTABLE@" rebase --abort
-        WORKING_DIRECTORY "@work_dir@"
-      )
-
-      if(NOT "@git_update_strategy@" STREQUAL "REBASE_CHECKOUT")
-        # Not allowed to do a checkout as a fallback, so cannot proceed
-        if(need_stash)
-          execute_process(
-            COMMAND "@git_EXECUTABLE@" stash pop --index --quiet
-            WORKING_DIRECTORY "@work_dir@"
-            )
-        endif()
-        message(FATAL_ERROR "\nFailed to rebase in: '@work_dir@'."
-                            "\nOutput from the attempted rebase follows:"
-                            "\n${rebase_output}"
-                            "\n\nYou will have to resolve the conflicts manually")
-      endif()
-
-      # Fall back to checkout. We create an annotated tag so that the user
-      # can manually inspect the situation and revert if required.
-      # We can't log the failed rebase output because MSVC sees it and
-      # intervenes, causing the build to fail even though it completes.
-      # Write it to a file instead.
-      string(TIMESTAMP tag_timestamp "%Y%m%dT%H%M%S" UTC)
-      set(tag_name _cmake_ExternalProject_moved_from_here_${tag_timestamp}Z)
-      set(error_log_file ${CMAKE_CURRENT_LIST_DIR}/rebase_error_${tag_timestamp}Z.log)
-      file(WRITE ${error_log_file} "${rebase_output}")
-      message(WARNING "Rebase failed, output has been saved to ${error_log_file}"
-                      "\nFalling back to checkout, previous commit tagged as ${tag_name}")
-      execute_process(
-        COMMAND "@git_EXECUTABLE@" tag -a
-                -m "ExternalProject attempting to move from here to ${git_remote}/${git_tag}"
-                ${tag_name}
+        COMMAND "@git_EXECUTABLE@" checkout "${git_remote}/${git_tag}"
         WORKING_DIRECTORY "@work_dir@"
         RESULT_VARIABLE error_code
-      )
+        )
       if(error_code)
-        message(FATAL_ERROR "Failed to add marker tag")
+        message(FATAL_ERROR "Failed to checkout tag: '${git_remote}/${git_tag}'")
       endif()
-
+    else()
+      # Pull changes from the remote branch
       execute_process(
-        COMMAND "@git_EXECUTABLE@" checkout ${git_remote}/${git_tag}
+        COMMAND "@git_EXECUTABLE@" rebase "${git_remote}/${git_tag}"
         WORKING_DIRECTORY "@work_dir@"
         RESULT_VARIABLE error_code
-      )
+        OUTPUT_VARIABLE rebase_output
+        ERROR_VARIABLE  rebase_output
+        )
       if(error_code)
-        message(FATAL_ERROR "Failed to checkout : '${git_remote}/${git_tag}'")
-      endif()
+        # Rebase failed, undo the rebase attempt before continuing
+        execute_process(
+          COMMAND "@git_EXECUTABLE@" rebase --abort
+          WORKING_DIRECTORY "@work_dir@"
+        )
+
+        if(NOT "@git_update_strategy@" STREQUAL "REBASE_CHECKOUT")
+          # Not allowed to do a checkout as a fallback, so cannot proceed
+          if(need_stash)
+            execute_process(
+              COMMAND "@git_EXECUTABLE@" stash pop --index --quiet
+              WORKING_DIRECTORY "@work_dir@"
+              )
+          endif()
+          message(FATAL_ERROR "\nFailed to rebase in: '@work_dir@'."
+                              "\nOutput from the attempted rebase follows:"
+                              "\n${rebase_output}"
+                              "\n\nYou will have to resolve the conflicts manually")
+        endif()
+
+        # Fall back to checkout. We create an annotated tag so that the user
+        # can manually inspect the situation and revert if required.
+        # We can't log the failed rebase output because MSVC sees it and
+        # intervenes, causing the build to fail even though it completes.
+        # Write it to a file instead.
+        string(TIMESTAMP tag_timestamp "%Y%m%dT%H%M%S" UTC)
+        set(tag_name _cmake_ExternalProject_moved_from_here_${tag_timestamp}Z)
+        set(error_log_file ${CMAKE_CURRENT_LIST_DIR}/rebase_error_${tag_timestamp}Z.log)
+        file(WRITE ${error_log_file} "${rebase_output}")
+        message(WARNING "Rebase failed, output has been saved to ${error_log_file}"
+                        "\nFalling back to checkout, previous commit tagged as ${tag_name}")
+        execute_process(
+          COMMAND "@git_EXECUTABLE@" tag -a
+                  -m "ExternalProject attempting to move from here to ${git_remote}/${git_tag}"
+                  ${tag_name}
+          WORKING_DIRECTORY "@work_dir@"
+          RESULT_VARIABLE error_code
+        )
+        if(error_code)
+          message(FATAL_ERROR "Failed to add marker tag")
+        endif()
+
+        execute_process(
+          COMMAND "@git_EXECUTABLE@" checkout "${git_remote}/${git_tag}"
+          WORKING_DIRECTORY "@work_dir@"
+          RESULT_VARIABLE error_code
+        )
+        if(error_code)
+          message(FATAL_ERROR "Failed to checkout : '${git_remote}/${git_tag}'")
+        endif()
 
+      endif()
     endif()
 
     if(need_stash)
diff --git a/Tests/ExternalProjectUpdate/ExternalProjectUpdateTest.cmake b/Tests/ExternalProjectUpdate/ExternalProjectUpdateTest.cmake
index ba0c598..ddc513f 100644
--- a/Tests/ExternalProjectUpdate/ExternalProjectUpdateTest.cmake
+++ b/Tests/ExternalProjectUpdate/ExternalProjectUpdateTest.cmake
@@ -199,8 +199,14 @@ if(do_git_tests)
 
   foreach(strategy IN ITEMS CHECKOUT REBASE_CHECKOUT)
     # Move local master back, then apply a change that will cause a conflict
-    # during rebase. We want to test the fallback to checkout.
-    check_a_tag(master 5842b503ba4113976d9bb28d57b5aee1ad2736b7 1 REBASE)
+    # during rebase
+    execute_process(COMMAND ${GIT_EXECUTABLE} checkout master
+      WORKING_DIRECTORY ${ExternalProjectUpdate_BINARY_DIR}/CMakeExternals/Source/TutorialStep1-GIT
+      RESULT_VARIABLE error_code
+      )
+    if(error_code)
+      message(FATAL_ERROR "Could not reset local master back to tag1.")
+    endif()
     execute_process(COMMAND ${GIT_EXECUTABLE} reset --hard tag1
       WORKING_DIRECTORY ${ExternalProjectUpdate_BINARY_DIR}/CMakeExternals/Source/TutorialStep1-GIT
       RESULT_VARIABLE error_code
@@ -208,6 +214,7 @@ if(do_git_tests)
     if(error_code)
       message(FATAL_ERROR "Could not reset local master back to tag1.")
     endif()
+
     set(cmlFile ${ExternalProjectUpdate_BINARY_DIR}/CMakeExternals/Source/TutorialStep1-GIT/CMakeLists.txt)
     file(READ ${cmlFile} contents)
     string(REPLACE "find TutorialConfig.h" "find TutorialConfig.h (conflict here)"
@@ -222,7 +229,7 @@ if(do_git_tests)
       message(FATAL_ERROR "Could not commit conflicting change.")
     endif()
     # This should discard our commit but leave behind an annotated tag
-    check_a_tag(master 5842b503ba4113976d9bb28d57b5aee1ad2736b7 1 ${strategy})
+    check_a_tag(origin/master 5842b503ba4113976d9bb28d57b5aee1ad2736b7 1 ${strategy})
   endforeach()
 
 endif()