Repository: bazelbuild/bazel
Issue: shallow_since appears to be a trap when used with commit, debug line should be removed
Labels: type: bug
P2
team-OSS

### Description of the problem / feature request:

The `shallow_since` option to `new_git_repository` appears to be a trap, at least when used with the `commit` option. When shallow_since is provided, instead of using `depth 1` to check out only the relevant commit, the entire repo state since some timestamp is cloned. On one of our repos, this causes a *25x* difference in clone time (7s -> 175s) and presumably a large size disparity as well.

Now that we have removed shallow_since, bazel emits a debug line encouraging us to add it back.
```
DEBUG: Rule 'foo' indicated that a canonical reproducible form can be obtained by modifying arguments shallow_since = "1611092265 -0800"
```

This seems wrong -- as far as I understand, the repo at a specific commit hash _is_ a canonical reproducible form, so it seems like bazel should not be suggesting this change.

### Feature requests: what underlying problem are you trying to solve with this feature?

Bazel should not recommend adding shallow_since since it is dramatically slower in some cases, and seems generally unnecessary to obtain a snapshot of the repository.

### Bugs: what's the simplest, easiest way to reproduce this bug? Please provide a minimal example if possible.

Create a `new_git_repository` rule for a large git repository at a commit that is not close to HEAD. Providing shallow_since will slow down the clone substantially.

### What operating system are you running Bazel on?

Linux, Ubuntu 20.04

### What's the output of `bazel info release`?

```
INFO: Invocation ID: 6c8f5dce-cf89-4b6f-a335-93ff77612968
release 3.7.1
```

### If `bazel info release` returns "development version" or "(@non-git)", tell us how you built Bazel.

n/a

### What's the output of `git remote get-url origin ; git rev-parse master ; git rev-parse HEAD` ?

n/a (private repo)

###  Have you found anything relevant by searching the web?

No

### Any other information, logs, or outputs that you want to share?

n/a

Issue comments:
+1

Also git's `--shallow-since` has a bug that can sometimes make git impossible to fetch the desired commit using its suggested timestamp: https://github.com/bazelbuild/bazel/issues/10292.
There is no way to persuade git to fetch a specific commit with --depth=1. If you try fetching a commit hash, most servers will just give you "server does not allow request for unadvertised object", and if you fetch a ref name then it can return anything and is not reproducible.

shallow_since is the only feature git offers to do this. Unfortunately, it's completely broken.
Correction: --depth=1 on a single commit only works with protocol v2, see https://github.com/bazelbuild/bazel/pull/12199 
@asuffield I don't think we're on protocol v2 (although I'm not completely sure how to verify that) and we've been using the commit only since I posted the issue without any apparent problems or non-reproducibility. As far as I can tell, the actions bazel is taking are equivalent to the following commands, which appear to work for me.

```
mkdir ruby ; cd ruby
git init
git remote add origin git@github.com:ruby/ruby.git
# A random old commit
git fetch origin 5de38c41ae7bf17ae599fdfa9f8face87f16d8bb --depth 1
git checkout 5de38c41ae7bf17ae599fdfa9f8face87f16d8bb
```

 Do you mean that it doesn't work reliably with all git servers, so maybe it works on github but not elsewhere?

```
$ git --version
git version 2.25.1
$ echo $GIT_PROTOCOL

```
It looks like maybe the server config option `uploadpack.allowReachableSHA1InWant` needs to be set to enable this, but the repository rules already seem to handle falling back when the server doesn't support this option anyway. `shallow_since` is perhaps better than the fallback option, but it's probably worse than fetching the commit directly when supported, so it shouldn't be recommended when fetching by commit is supported.

https://github.com/bazelbuild/bazel/blob/8346ea4cfdd9fbd170d51a528fee26f912dad2d5/tools/build_defs/repo/git_worker.bzl#L129
It definitely fails with git 2.18 (ugh, upgrading that right now) against gitlab, I haven't tested exhaustively. But the main problem in the PR linked above is that one of bazel's target platforms has an old git version, and that's going to sabotage us here.
Are there ways to improve this situation, even given bazel's support for ubuntu 18.04 and its associated git version?

One option is simply to drop the warning from this rule (by making the rule output a canonical version that omits `shallow_since` if the caller did not specify it).

Another is to add something like an explicit `depth` arg to the `git_repository` rules: I don't like that from an API perspective, but it could allow callers to explicitly use a depth-based canonical form (rather than a `shallow_since`-based form) if that works for their environments.

I think that any change to the canonical form of this rule would involve changing what's returned from the [`git_repo` function](https://github.com/bazelbuild/bazel/blob/master/tools/build_defs/repo/git_worker.bzl#L94-L96).

Are there other ideas that could remove warnings from a correct repository setup that's relying on `depth=1` rather than a `shallow_since` date?

Pull request review comments:

Patch that resolved the issue:
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,


Test patch: