From ff474b96b74f321e6159c6b5bcabc60359e36120 Mon Sep 17 00:00:00 2001
From: Vector Li <huanli@redhat.com>
Date: Tue, 12 Dec 2023 17:55:05 +0800
Subject: [PATCH] Reorg git_clone() according to comments from Petr

Signed-off-by: Vector Li <huanli@redhat.com>
---
 tmt/steps/discover/shell.py |  2 +-
 tmt/utils.py                | 95 +++++++++++++++++++++----------------
 2 files changed, 55 insertions(+), 42 deletions(-)

diff --git a/tmt/steps/discover/shell.py b/tmt/steps/discover/shell.py
index 5e81d093b9..6018ebb578 100644
--- a/tmt/steps/discover/shell.py
+++ b/tmt/steps/discover/shell.py
@@ -280,7 +280,7 @@ def fetch_remote_repository(
             url=url,
             destination=testdir,
             common=self,
-            gc_logger=self._logger,
+            logger=self._logger,
             env={"GIT_ASKPASS": "echo"},
             shallow=ref is None)
 
diff --git a/tmt/utils.py b/tmt/utils.py
index 1bafffc202..2e908cfd47 100644
--- a/tmt/utils.py
+++ b/tmt/utils.py
@@ -4888,17 +4888,37 @@ def distgit_download(
                 logger=logger)
 
 
+def _git_clone(
+        url: str,
+        destination: Path,
+        common: Common,
+        env: Optional[EnvironmentType] = None,
+        timeout: Optional[int] = None,
+        shallow: bool = False
+        ) -> CommandOutput:
+    """ Git clone url to destination """
+
+    depth = ['--depth=1'] if shallow else []
+
+    try:
+        return common.run(Command('git', 'clone', *depth, url, destination),
+                          env=env,
+                          timeout=timeout)
+    except KeyError:
+        raise
+
+
 def git_clone(
         url: str,
         destination: Path,
         common: Common,
-        gc_logger: tmt.log.Logger,
+        logger: tmt.log.Logger,
         env: Optional[EnvironmentType] = None,
         shallow: bool = False,
         can_change: bool = True,
         timeout: Optional[int] = None,
-        gc_attempts: Optional[int] = None,
-        gc_interval: Optional[int] = None
+        attempts: Optional[int] = None,
+        interval: Optional[int] = None
         ) -> CommandOutput:
     """
     Git clone url to destination, retry without shallow if necessary
@@ -4911,10 +4931,6 @@ def git_clone(
 
     Url can be modified with hardcode rules unless can_change=False is set.
     """
-    depth = ['--depth=1'] if shallow else []
-
-    if can_change:
-        url = clonable_git_url(url)
 
     def get_env(env: str, default_value: Optional[int]) -> Optional[int]:
         """ Get the value of an environment variable and convert it to be integer """
@@ -4927,48 +4943,45 @@ def get_env(env: str, default_value: Optional[int]) -> Optional[int]:
             return default_value
 
     timeout = timeout or get_env('TMT_GIT_CLONE_TIMEOUT', None)
-    # Note that `gc_attempts = gc_attempts or ...` should not be used right here
-    # because `bool(0)` is `False`. And if gc_attempts is 0, we should not reset
-    # it. Or it will fall into endless loop once retry() is called.
-    if gc_attempts is None:
-        gc_attempts = cast(int, get_env('TMT_GIT_CLONE_ATTEMPTS', DEFAULT_GIT_CLONE_ATTEMPTS))
-    if gc_interval is None:
-        gc_interval = cast(int, get_env('TMT_GIT_CLONE_INTERVAL', DEFAULT_GIT_CLONE_INTERVAL))
+    attempts = attempts or cast(int, get_env('TMT_GIT_CLONE_ATTEMPTS', DEFAULT_GIT_CLONE_ATTEMPTS))
+    interval = cast(int, get_env('TMT_GIT_CLONE_INTERVAL', DEFAULT_GIT_CLONE_INTERVAL))
+
+    # Update url only once
+    if can_change:
+        url = clonable_git_url(url)
+
+    # Do an extra shallow clone first
+    if shallow:
+        try:
+            return _git_clone(
+                shallow=True,
+                url=url,
+                destination=destination,
+                common=common,
+                env=env,
+                timeout=timeout
+                )
+        except KeyError:
+            # Do nothing
+            pass
 
+    # Finish with whatever number attempts requested (deep)
     try:
-        return common.run(
-            Command(
-                'git', 'clone',
-                *depth,
-                url, destination
-                ), env=env, timeout=timeout)
-    except RunError:
-        if gc_attempts == 0:
-            raise
-        if not shallow:
-            # Do not retry if shallow was not used
-            raise
-        # Note that the `url` is changed even if `can_change` is `True` when the function is
-        # called at the first time, it is not necessary to changed it again when retrying.
-        # Hence, `can_change` is always set as `False` right here.
-        # And once retry() is called, `gc_attempts` should be set to `0` to avoid endless loop.
         return retry(
-            func=git_clone,
-            attempts=gc_attempts,
-            interval=gc_interval,
-            label=f"git clone {' '.join(depth)} {url} {destination}",
-            logger=gc_logger,
+            func=_git_clone,
+            attempts=attempts,
+            interval=interval,
+            label=f"git clone {url} {destination}",
+            logger=logger,
+            shallow=False,
             url=url,
             destination=destination,
             common=common,
-            gc_logger=gc_logger,
             env=env,
-            shallow=True,
-            can_change=False,
-            timeout=timeout,
-            gc_attempts=0,
-            gc_interval=gc_interval
+            timeout=timeout
             )
+    except RetryError:
+        raise
 
 
 # ignore[type-arg]: base class is a generic class, but we cannot list its parameter type, because