Repository: bazelbuild/bazel
Issue: shallow_since appears to be broken
Labels: type: bug
P2
team-ExternalDeps

### Description of the problem / feature request:

`shallow_since` appears to be broken.

### Bugs: what's the simplest, easiest way to reproduce this bug? Please provide a minimal example if possible.

Try to build `//:BUILD.boost` from https://github.com/nelhage/rules_boost/commit/df908358c605a7d5b8bbacde07afbaede5ac12cf. You'll receive a recommendation for `shallow_since`. Insert that recommendation, and suddenly Bazel fails to find that commit at all.

### What operating system are you running Bazel on?

Windows

### What's the output of `bazel info release`?

release 1.1.0

### What's the output of `git remote get-url origin ; git rev-parse master ; git rev-parse HEAD` ?

N/A

###  Have you found anything relevant by searching the web?

No

### Any other information, logs, or outputs that you want to share?


Create an empty `BUILD.bazel` alongside the following `WORKSPACE`:

```
load("@bazel_tools//tools/build_defs/repo:git.bzl", "git_repository")
git_repository(name="rules_boost", commit="df908358c605a7d5b8bbacde07afbaede5ac12cf", remote="https://github.com/nelhage/rules_boost")
```

Now run `bazel --ignore_all_rc_files query "deps(@rules_boost//:BUILD.boost)"`:

```
bazel --ignore_all_rc_files query "deps(@rules_boost//:BUILD.boost)"
@bazel --ignore_all_rc_files clean --expunge
```

It outputs something like the following (after formatting/redactions):

```
INFO: Writing tracer profile to 'command.profile.gz'
DEBUG: Rule 'rules_boost' indicated that a canonical reproducible form can be obtained by modifying arguments shallow_since = "1569511515 +0200"
DEBUG: Call stack for the definition of repository 'rules_boost' which is a git_repository (rule definition at external/bazel_tools/tools/build_defs/repo/git.bzl:195:18):
 - WORKSPACE:2:1
ERROR: no such target '@rules_boost//:BUILD.boost': target 'BUILD.boost' not declared in package ''; however, a source file of this name exists.  (Perhaps add 'exports_files(["BUILD.boost"])' to /BUILD?) defined by external/rules_boost/BUILD
Loading: 1 packages loaded
```

Now add `shallow_since="1569511515 +0200"` as recommended and re-run the query:

```
INFO: Writing tracer profile to 'command.profile.gz'
INFO: Call stack for the definition of repository 'rules_boost' which is a git_repository (rule definition at external/bazel_tools/tools/build_defs/repo/git.bzl:195:18):
 - WORKSPACE:2:1
ERROR: An error occurred during the fetch of repository 'rules_boost':
error running 'git reset --hard df908358c605a7d5b8bbacde07afbaede5ac12cf' while working with @rules_boost:
fatal: Could not parse object 'df908358c605a7d5b8bbacde07afbaede5ac12cf'.
ERROR: no such package '@rules_boost//'
Loading: 0 packages loaded
```

Suddenly Bazel seems to be unable to find the commit altogether.


Issue comments:
We got the same issue on macOS, but it seems to be a problem with git (tried with `git clone --shallow-since`).
Yeah, seems like it. Though I'd say Bazel is still responsible for avoiding recommending `shallow_since` unless/until this is fixed (a version check on git might make sense). Otherwise it's really confusing for the user to be recommended an option that's broken.
@aehlig Do you know what causes it? 
I did some digging here, and reported [an upstream bug to `git`](https://public-inbox.org/git/CAPSG9dZV2EPpVKkOMcjv5z+NF7rUu=V-ZkZNx47rCv122HsiKg@mail.gmail.com/T/#u)

The upshot is that `--shallow-since` is subtly incorrect when the provided timestamp would cause us to include _some_ parents of a merge, but not others. In the case provided here, nelhage/rules_boost@df90835 has a commit timestamp of 1569511515. That commit is the second part of a merge, nelhage/rules_boost@76f3064; that merge's first parent is nelhage/rules_boost@a83197e, with an older commit timestamp of 1569487118

The bug I reported results in `git` discarding _all_ parents of the merge (nelhage/rules_boost@76f3064), including the desired commit.

For now, the workaround for bazel _users_ -- where available -- is to switch your `commit=` line to point to a commit on the first-parent spine, e.g. nelhage/rules_boost@76f3064. If you add the suggested `shallow_since` line for that commit, things work fine.

For Bazel itself, it might be nice to recognize this situation and adjust the `shallow_since` recommendation, at least until the upstream bug can be sorted out. I'm not certain offhand what the right/easiest heuristic to detect it is, though.
It turns out that the workaround proposed here doesn't work either.

The shallow_since bug is even worse than I expected. It stops at the first commit which has any parents before the cutoff time. This means we have the following horrible scenario:

1. Latest commit on main branch is aaaa, with parents baaa and bbbb. aaaa is at 8am, baaa is at 6am and bbbb is at 4am
2. I create a git_repository rule with commit = "aaaa", shallow_since = "4am"
3. This works fine when I build it
4. Somebody adds a new commit caaa to main branch, with parents aaaa and cbbb. caaa is at 9am, cbbb is at 2am
5. git clone --shallow_since=4am will now fetch only caaa because it has a parent before 4am, and aaaa will not be fetched

This is completely unusable and I don't see a workaround.
Yeah, I agree with @asuffield's analysis -- I ran into that situation since posting https://github.com/bazelbuild/bazel/issues/10292#issuecomment-680205748. In light of this and of #12857 it really seems like the right path forward is to stop suggesting `--shallow-since`. It might be appropriate in cases where the developers know neither of these problems will apply -- or are able to debug their way out if necessary -- but it's clearly too broken to suggest as a default.
I'm piecing together an alternative solution - for cases where a tag or commit hash is given, we can use point fetch with --depth=1 as long as we verify the results and correctly fallback to deep fetch if we don't get what we needed. It doesn't work on all git versions, but I don't believe there is a solution other than deep fetch for older git.
If you auto-detect GitHub URLs you can instead just suggest HTTPS cloning. It's probably a good suggestion in any case; we switched to that as it's faster too.
> If you auto-detect GitHub URLs you can instead just suggest HTTPS cloning. It's probably a good suggestion in any case; we switched to that as it's faster too.

This is an interesting idea which would likely be best implemented in git itself - the protocol's rich enough to pull it off.
`git archive` already exists in git. GitHub has refused to support it.
I would like to get some progress on this issue. Would it be fine if I create a PR to remove the debug log saying:

> indicated that a canonical reproducible form can be obtained by modifying arguments shallow_since = 

The issue with this log is that one a user see it the first thing will do is to set up the shallow_since, but because of this bug it is something that should consider twice.


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: