ExternalProject: Improve robustness of update step

Refactor the update logic to make it easier to follow. The following
fixes/improvements are some consequences of this change:

* Absorb a confusing git checkout failure message when the failure
  is allowed and we act on that failure appropriately.
* Fix an unnecessary fetch in some scenarios when checking out a
  git hash we already have locally.
* Stash and restore any local changes even when not rebasing.
* Avoid unsafe rebasing where we are not on a branch that is
  already tracking the requested branch.
* When fetching, use --tags --force to ensure we get all the tags
  and commits leading up to them regardless of whether the tags
  are on branches or not. Also update our local tags if they move
  on the remote.

Fixes: #20677

Modules/ExternalProject-gitupdate.cmake.in

@@ -3,63 +3,152 @@
 
 cmake_minimum_required(VERSION 3.5)
 
+function(get_hash_for_ref ref out_var err_var)
   execute_process(
-  COMMAND "@git_EXECUTABLE@" rev-list --max-count=1 HEAD
+    COMMAND "@git_EXECUTABLE@" rev-parse "${ref}"
     WORKING_DIRECTORY "@work_dir@"
     RESULT_VARIABLE error_code
-  OUTPUT_VARIABLE head_sha
+    OUTPUT_VARIABLE ref_hash
+    ERROR_VARIABLE error_msg
     OUTPUT_STRIP_TRAILING_WHITESPACE
   )
   if(error_code)
-  message(FATAL_ERROR "Failed to get the hash for HEAD")
+    set(${out_var} "" PARENT_SCOPE)
+  else()
+    set(${out_var} "${ref_hash}" PARENT_SCOPE)
   endif()
+  set(${err_var} "${error_msg}" PARENT_SCOPE)
+endfunction()
+
+get_hash_for_ref(HEAD head_sha error_msg)
+if(head_sha STREQUAL "")
+  message(FATAL_ERROR "Failed to get the hash for HEAD:\n${error_msg}")
+endif()
+
 
 execute_process(
   COMMAND "@git_EXECUTABLE@" show-ref "@git_tag@"
   WORKING_DIRECTORY "@work_dir@"
   OUTPUT_VARIABLE show_ref_output
 )
-# If a remote ref is asked for, which can possibly move around,
-# we must always do a fetch and checkout.
-if("${show_ref_output}" MATCHES "remotes")
-  set(is_remote_ref 1)
-else()
-  set(is_remote_ref 0)
+if(show_ref_output MATCHES "^[a-z0-9]+[ \\t]+refs/remotes/")
+  # Given a full remote/branch-name and we know about it already. Since
+  # branches can move around, we always have to fetch.
+  set(fetch_required YES)
+  set(checkout_name "@git_tag@")
+
+elseif(show_ref_output MATCHES "^[a-z0-9]+[ \\t]+refs/tags/")
+  # Given a tag name that we already know about. We don't know if the tag we
+  # have matches the remote though (tags can move), so we should fetch.
+  set(fetch_required YES)
+  set(checkout_name "@git_tag@")
+
+  # Special case to preserve backward compatibility: if we are already at the
+  # same commit as the tag we hold locally, don't do a fetch and assume the tag
+  # hasn't moved on the remote.
+  # FIXME: We should provide an option to always fetch for this case
+  get_hash_for_ref("@git_tag@" tag_sha error_msg)
+  if(tag_sha STREQUAL head_sha)
+    message(VERBOSE "Already at requested tag: ${tag_sha}")
+    return()
   endif()
 
-# Tag is in the form <remote>/<tag> (i.e. origin/master) we must strip
-# the remote from the tag.
-if("${show_ref_output}" MATCHES "refs/remotes/@git_tag@")
-  string(REGEX MATCH "^([^/]+)/(.+)$" _unused "@git_tag@")
-  set(git_remote "${CMAKE_MATCH_1}")
-  set(git_tag "${CMAKE_MATCH_2}")
+elseif(show_ref_output MATCHES "^[a-z0-9]+[ \\t]+refs/heads/")
+  # Given a branch name without any remote and we already have a branch by that
+  # name. We might already have that branch checked out or it might be a
+  # different branch. It isn't safe to use a bare branch name without the
+  # remote, so do a fetch and replace the ref with one that includes the remote.
+  set(fetch_required YES)
+  set(checkout_name "@git_remote_name@/@git_tag@")
+
 else()
-  set(git_remote "@git_remote_name@")
-  set(git_tag "@git_tag@")
+  get_hash_for_ref("@git_tag@" tag_sha error_msg)
+  if(tag_sha STREQUAL head_sha)
+    # Have the right commit checked out already
+    message(VERBOSE "Already at requested ref: ${tag_sha}")
+    return()
+
+  elseif(tag_sha STREQUAL "")
+    # We don't know about this ref yet, so we have no choice but to fetch.
+    # We deliberately swallow any error message at the default log level
+    # because it can be confusing for users to see a failed git command.
+    # That failure is being handled here, so it isn't an error.
+    set(fetch_required YES)
+    set(checkout_name "@git_tag@")
+    if(NOT error_msg STREQUAL "")
+      message(VERBOSE "${error_msg}")
     endif()
 
-# This will fail if the tag does not exist (it probably has not been fetched
-# yet).
+  else()
+    # We have the commit, so we know we were asked to find a commit hash
+    # (otherwise it would have been handled further above), but we don't
+    # have that commit checked out yet
+    set(fetch_required NO)
+    set(checkout_name "@git_tag@")
+    if(NOT error_msg STREQUAL "")
+      message(WARNING "${error_msg}")
+    endif()
+
+  endif()
+endif()
+
+if(fetch_required)
+  message(VERBOSE "Fetching latest from the remote @git_remote_name@")
   execute_process(
-  COMMAND "@git_EXECUTABLE@" rev-list --max-count=1 "${git_tag}"
+    COMMAND "@git_EXECUTABLE@" fetch --tags --force "@git_remote_name@"
     WORKING_DIRECTORY "@work_dir@"
-  RESULT_VARIABLE error_code
-  OUTPUT_VARIABLE tag_sha
+    COMMAND_ERROR_IS_FATAL ANY
+  )
+endif()
+
+set(git_update_strategy "@git_update_strategy@")
+if(git_update_strategy STREQUAL "")
+  # Backward compatibility requires REBASE as the default behavior
+  set(git_update_strategy REBASE)
+endif()
+
+if(git_update_strategy MATCHES "^REBASE(_CHECKOUT)?$")
+  # Asked to potentially try to rebase first, maybe with fallback to checkout.
+  # We can't if we aren't already on a branch and we shouldn't if that local
+  # branch isn't tracking the one we want to checkout.
+  execute_process(
+    COMMAND "@git_EXECUTABLE@" symbolic-ref -q HEAD
+    WORKING_DIRECTORY "@work_dir@"
+    OUTPUT_VARIABLE current_branch
     OUTPUT_STRIP_TRAILING_WHITESPACE
+    # Don't test for an error. If this isn't a branch, we get a non-zero error
+    # code but empty output.
   )
 
-# Is the hash checkout out that we want?
-if(error_code OR is_remote_ref OR NOT ("${tag_sha}" STREQUAL "${head_sha}"))
+  if(current_branch STREQUAL "")
+    # Not on a branch, checkout is the only sensible option since any rebase
+    # would always fail (and backward compatibility requires us to checkout in
+    # this situation)
+    set(git_update_strategy CHECKOUT)
+
+  else()
     execute_process(
-    COMMAND "@git_EXECUTABLE@" fetch
+      COMMAND "@git_EXECUTABLE@" for-each-ref "--format='%(upstream:short)'" "${current_branch}"
       WORKING_DIRECTORY "@work_dir@"
-    RESULT_VARIABLE error_code
+      OUTPUT_VARIABLE upstream_branch
+      OUTPUT_STRIP_TRAILING_WHITESPACE
+      COMMAND_ERROR_IS_FATAL ANY  # There is no error if no upstream is set
     )
-  if(error_code)
-    message(FATAL_ERROR "Failed to fetch repository '@git_repository@'")
+    if(NOT upstream_branch STREQUAL checkout_name)
+      # Not safe to rebase when asked to checkout a different branch to the one
+      # we are tracking. If we did rebase, we could end up with arbitrary
+      # commits added to the ref we were asked to checkout if the current local
+      # branch happens to be able to rebase onto the target branch. There would
+      # be no error message and the user wouldn't know this was occurring.
+      set(git_update_strategy CHECKOUT)
     endif()
 
-  if(is_remote_ref)
+  endif()
+elseif(NOT git_update_strategy STREQUAL "CHECKOUT")
+  message(FATAL_ERROR "Unsupported git update strategy: ${git_update_strategy}")
+endif()
+
+
 # Check if stash is needed
 execute_process(
   COMMAND "@git_EXECUTABLE@" status --porcelain
@@ -78,26 +167,19 @@ if(error_code OR is_remote_ref OR NOT ("${tag_sha}" STREQUAL "${head_sha}"))
   execute_process(
     COMMAND "@git_EXECUTABLE@" stash save @git_stash_save_options@
     WORKING_DIRECTORY "@work_dir@"
-        RESULT_VARIABLE error_code
+    COMMAND_ERROR_IS_FATAL ANY
   )
-      if(error_code)
-        message(FATAL_ERROR "Failed to stash changes")
-      endif()
 endif()
 
-    if("@git_update_strategy@" STREQUAL "CHECKOUT")
+if(git_update_strategy STREQUAL "CHECKOUT")
   execute_process(
-        COMMAND "@git_EXECUTABLE@" checkout "${git_remote}/${git_tag}"
+    COMMAND "@git_EXECUTABLE@" checkout "${checkout_name}"
     WORKING_DIRECTORY "@work_dir@"
-        RESULT_VARIABLE error_code
+    COMMAND_ERROR_IS_FATAL ANY
   )
-      if(error_code)
-        message(FATAL_ERROR "Failed to checkout tag: '${git_remote}/${git_tag}'")
-      endif()
 else()
-      # Pull changes from the remote branch
   execute_process(
-        COMMAND "@git_EXECUTABLE@" rebase "${git_remote}/${git_tag}"
+    COMMAND "@git_EXECUTABLE@" rebase "${checkout_name}"
     WORKING_DIRECTORY "@work_dir@"
     RESULT_VARIABLE error_code
     OUTPUT_VARIABLE rebase_output
@@ -110,7 +192,7 @@ if(error_code OR is_remote_ref OR NOT ("${tag_sha}" STREQUAL "${head_sha}"))
       WORKING_DIRECTORY "@work_dir@"
     )
 
-        if(NOT "@git_update_strategy@" STREQUAL "REBASE_CHECKOUT")
+    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(
@@ -137,28 +219,22 @@ if(error_code OR is_remote_ref OR NOT ("${tag_sha}" STREQUAL "${head_sha}"))
                     "\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}"
+              -m "ExternalProject attempting to move from here to ${checkout_name}"
               ${tag_name}
       WORKING_DIRECTORY "@work_dir@"
-          RESULT_VARIABLE error_code
+      COMMAND_ERROR_IS_FATAL ANY
     )
-        if(error_code)
-          message(FATAL_ERROR "Failed to add marker tag")
-        endif()
 
     execute_process(
-          COMMAND "@git_EXECUTABLE@" checkout "${git_remote}/${git_tag}"
+      COMMAND "@git_EXECUTABLE@" checkout "${checkout_name}"
       WORKING_DIRECTORY "@work_dir@"
-          RESULT_VARIABLE error_code
+      COMMAND_ERROR_IS_FATAL ANY
     )
-        if(error_code)
-          message(FATAL_ERROR "Failed to checkout : '${git_remote}/${git_tag}'")
-        endif()
-
   endif()
 endif()
 
 if(need_stash)
+  # Put back the stashed changes
   execute_process(
     COMMAND "@git_EXECUTABLE@" stash pop --index --quiet
     WORKING_DIRECTORY "@work_dir@"
@@ -169,7 +245,6 @@ if(error_code OR is_remote_ref OR NOT ("${tag_sha}" STREQUAL "${head_sha}"))
     execute_process(
       COMMAND "@git_EXECUTABLE@" reset --hard --quiet
       WORKING_DIRECTORY "@work_dir@"
-          RESULT_VARIABLE error_code
     )
     execute_process(
       COMMAND "@git_EXECUTABLE@" stash pop --quiet
@@ -191,26 +266,12 @@ if(error_code OR is_remote_ref OR NOT ("${tag_sha}" STREQUAL "${head_sha}"))
     endif()
   endif()
 endif()
-  else()
-    execute_process(
-      COMMAND "@git_EXECUTABLE@" checkout "${git_tag}"
-      WORKING_DIRECTORY "@work_dir@"
-      RESULT_VARIABLE error_code
-      )
-    if(error_code)
-      message(FATAL_ERROR "Failed to checkout tag: '${git_tag}'")
-    endif()
-  endif()
 
 set(init_submodules "@init_submodules@")
 if(init_submodules)
   execute_process(
     COMMAND "@git_EXECUTABLE@" submodule update @git_submodules_recurse@ --init @git_submodules@
     WORKING_DIRECTORY "@work_dir@"
-      RESULT_VARIABLE error_code
+    COMMAND_ERROR_IS_FATAL ANY
   )
 endif()
-  if(error_code)
-    message(FATAL_ERROR "Failed to update submodules in: '@work_dir@'")
-  endif()
-endif()

Tests/ExternalProjectUpdate/ExternalProjectUpdateTest.cmake

@@ -185,7 +185,6 @@ if(do_git_tests)
   # 'git fetch'
   check_a_tag(tag1          d1970730310fe8bc07e73f15dc570071f9f9654a 0 REBASE)
   check_a_tag(tag2          5842b503ba4113976d9bb28d57b5aee1ad2736b7 1 REBASE)
-  check_a_tag(d19707303     d1970730310fe8bc07e73f15dc570071f9f9654a 1 REBASE)
   check_a_tag(d19707303     d1970730310fe8bc07e73f15dc570071f9f9654a 0 REBASE)
   check_a_tag(origin/master b5752a26ae448410926b35c275af3c192a53722e 1 REBASE)
   # This is a remote symbolic ref, so it will always trigger a 'git fetch'

Tests/RunCMake/ExternalProject/FetchGitTags.cmake

@@ -0,0 +1,67 @@
+find_package(Git QUIET REQUIRED)
+
+include(ExternalProject)
+
+set(srcRepo ${CMAKE_CURRENT_BINARY_DIR}/srcRepo)
+set(srcDir  ${CMAKE_CURRENT_BINARY_DIR}/src)
+set(binDir  ${CMAKE_CURRENT_BINARY_DIR}/build)
+file(MAKE_DIRECTORY ${srcRepo})
+file(MAKE_DIRECTORY ${srcDir})
+
+file(GLOB entries ${srcRepo}/*)
+file(REMOVE_RECURSE ${entries} ${binDir})
+file(TOUCH ${srcRepo}/firstFile.txt)
+configure_file(${CMAKE_CURRENT_LIST_DIR}/FetchGitTags/CMakeLists.txt
+    ${srcDir}/CMakeLists.txt COPYONLY)
+
+function(execGitCommand)
+  execute_process(
+    WORKING_DIRECTORY ${srcRepo}
+    COMMAND ${GIT_EXECUTABLE} ${ARGN}
+    COMMAND_ECHO STDOUT
+    COMMAND_ERROR_IS_FATAL ANY
+  )
+endfunction()
+
+function(configureAndBuild tag)
+  execute_process(COMMAND ${CMAKE_COMMAND}
+      -G ${CMAKE_GENERATOR} -T "${CMAKE_GENERATOR_TOOLSET}"
+      -A "${CMAKE_GENERATOR_PLATFORM}"
+      -D repoDir:PATH=${srcRepo}
+      -D gitTag:STRING=${tag}
+      -B ${binDir}
+      -S ${srcDir}
+    COMMAND_ECHO STDOUT
+    COMMAND_ERROR_IS_FATAL ANY
+  )
+
+  execute_process(COMMAND ${CMAKE_COMMAND} --build ${binDir} --target fetcher
+    WORKING_DIRECTORY ${binDir}
+    COMMAND_ECHO STDOUT
+    COMMAND_ERROR_IS_FATAL ANY
+  )
+endfunction()
+
+# Setup a fresh source repo with a predictable default branch across all
+# git versions
+execGitCommand(-c init.defaultBranch=master init)
+execGitCommand(config --add user.email "testauthor@cmake.org")
+execGitCommand(config --add user.name testauthor)
+
+# Create the initial repo structure
+execGitCommand(add firstFile.txt)
+execGitCommand(commit -m "First file")
+
+message(STATUS "First configure-and-build")
+configureAndBuild(master)
+
+# Create a tagged commit that is not on any branch. With git 2.20 or later,
+# this commit won't be fetched without the --tags option.
+file(TOUCH ${srcRepo}/secondFile.txt)
+execGitCommand(add secondFile.txt)
+execGitCommand(commit -m "Second file")
+execGitCommand(tag -a -m "Adding tag" tag_of_interest)
+execGitCommand(reset --hard HEAD~1)
+
+message(STATUS "Second configure-and-build")
+configureAndBuild(tag_of_interest)

Tests/RunCMake/ExternalProject/FetchGitTags/CMakeLists.txt

@@ -0,0 +1,15 @@
+cmake_minimum_required(VERSION 3.19)
+project(FetchTags LANGUAGES NONE)
+
+include(ExternalProject)
+
+# repoDir and gitTag are expected to be set as cache variables
+
+ExternalProject_Add(fetcher
+    GIT_REPOSITORY ${repoDir}
+    GIT_TAG ${gitTag}
+    GIT_REMOTE_UPDATE_STRATEGY CHECKOUT
+    CONFIGURE_COMMAND ""
+    BUILD_COMMAND ""
+    INSTALL_COMMAND ""
+)

Tests/RunCMake/ExternalProject/RunCMakeTest.cmake

@@ -181,3 +181,12 @@ endfunction()
 if(NOT RunCMake_GENERATOR MATCHES "Visual Studio 9 ")
   __ep_test_CONFIGURE_HANDLED_BY_BUILD()
 endif()
+
+find_package(Git QUIET)
+if(GIT_EXECUTABLE)
+  # Note that there appear to be differences in where git writes its output to
+  # on some platforms. It may go to stdout or stderr, so force it to be merged.
+  set(RunCMake_TEST_OUTPUT_MERGE TRUE)
+  run_cmake(FetchGitTags)
+  set(RunCMake_TEST_OUTPUT_MERGE FALSE)
+endif()