From dde6d20b3fd1d7a14825649c9c89d1c4d2d5242b Mon Sep 17 00:00:00 2001
From: Fabian Meumertzheim <fabian@meumertzhe.im>
Date: Thu, 9 Feb 2023 22:31:26 -0800
Subject: [PATCH] Do not recommend `shallow_since` for `git_repository`

Git's `--shallow-since` feature can lead to broken checkouts and a fix isn't likely to happen:
https://public-inbox.org/git/CAPSG9dZV2EPpVKkOMcjv5z+NF7rUu=V-ZkZNx47rCv122HsiKg@mail.gmail.com/T/#u

This commit removes `shallow_since` from the canonical reproducible form of `git_repository` unless the attribute is provided explicitly. In cases where Bazel's attempt at fetching with `--depth=1` fail, e.g. if there is no server support for shallow fetches of arbitrary commits, the progress message is now used to inform the user about this potential for fetch time optimizations.

Fixes #10292
Fixes #12857

Closes #17356.

PiperOrigin-RevId: 508569071
Change-Id: I01e6662e38c236d1800d427f66d48a05df818800
---
 tools/build_defs/repo/git.bzl        | 23 ++++++++++++++++-------
 tools/build_defs/repo/git_worker.bzl | 11 +++++++++--
 2 files changed, 25 insertions(+), 9 deletions(-)

diff --git a/tools/build_defs/repo/git.bzl b/tools/build_defs/repo/git.bzl
index 88678daae163b2..6f21bcd7dcaf9c 100644
--- a/tools/build_defs/repo/git.bzl
+++ b/tools/build_defs/repo/git.bzl
@@ -42,7 +42,10 @@ def _clone_or_update_repo(ctx):
         for item in ctx.path(dest_link).readdir():
             ctx.symlink(item, root.get_child(item.basename))
 
-    return {"commit": git_.commit, "shallow_since": git_.shallow_since}
+    if ctx.attr.shallow_since:
+        return {"commit": git_.commit, "shallow_since": git_.shallow_since}
+    else:
+        return {"commit": git_.commit}
 
 def _update_git_attrs(orig, keys, override):
     result = update_attrs(orig, keys, override)
@@ -68,12 +71,14 @@ _common_attrs = {
     "shallow_since": attr.string(
         default = "",
         doc =
-            "an optional date, not after the specified commit; the " +
-            "argument is not allowed if a tag is specified (which allows " +
-            "cloning with depth 1). Setting such a date close to the " +
-            "specified commit allows for a more shallow clone of the " +
-            "repository, saving bandwidth " +
-            "and wall-clock time.",
+            "an optional date, not after the specified commit; the argument " +
+            "is not allowed if a tag or branch is specified (which can " +
+            "always be cloned with --depth=1). Setting such a date close to " +
+            "the specified commit may allow for a shallow clone of the " +
+            "repository even if the server does not support shallow fetches " +
+            "of arbitary commits. Due to bugs in git's --shallow-since " +
+            "implementation, using this attribute is not recommended as it " +
+            "may result in fetch failures.",
     ),
     "tag": attr.string(
         default = "",
@@ -183,6 +188,10 @@ makes its targets available for binding. Also determine the id of the
 commit actually checked out and its date, and return a dict with parameters
 that provide a reproducible version of this rule (which a tag not necessarily
 is).
+
+Bazel will first try to perform a shallow fetch of only the specified commit.
+If that fails (usually due to missing server support), it will fall back to a
+full fetch of the repository.
 """,
 )
 
diff --git a/tools/build_defs/repo/git_worker.bzl b/tools/build_defs/repo/git_worker.bzl
index c663cdaabf8032..3e9ba3a10e40bf 100644
--- a/tools/build_defs/repo/git_worker.bzl
+++ b/tools/build_defs/repo/git_worker.bzl
@@ -79,8 +79,8 @@ def git_repo(ctx, directory):
         recursive_init_submodules = ctx.attr.recursive_init_submodules,
     )
 
-    ctx.report_progress("Cloning %s of %s" % (reset_ref, ctx.attr.remote))
-    if (ctx.attr.verbose):
+    _report_progress(ctx, git_repo)
+    if ctx.attr.verbose:
         print("git.bzl: Cloning or updating %s repository %s using strip_prefix of [%s]" %
               (
                   " (%s)" % shallow if shallow else "",
@@ -95,6 +95,12 @@ def git_repo(ctx, directory):
 
     return struct(commit = actual_commit, shallow_since = shallow_date)
 
+def _report_progress(ctx, git_repo, *, shallow_failed = False):
+    warning = ""
+    if shallow_failed:
+        warning = " (shallow fetch failed, fetching full history)"
+    ctx.report_progress("Cloning %s of %s%s" % (git_repo.reset_ref, git_repo.remote, warning))
+
 def _update(ctx, git_repo):
     ctx.delete(git_repo.directory)
 
@@ -133,6 +139,7 @@ def fetch(ctx, git_repo):
         # "ignore what is specified and fetch all tags".
         # The arguments below work correctly for both before 1.9 and after 1.9,
         # as we directly specify the list of references to fetch.
+        _report_progress(ctx, git_repo, shallow_failed = True)
         _git(
             ctx,
             git_repo,