git: allow cloning commit shas not referenced by branch/tag #5441
Conversation
Signed-off-by: Justin Chadwell <[email protected]>
jedevc
force-pushed
the
git-clone-hidden-sha
branch
from
648caff to
57b4c9a
Compare
jedevc
changed the title
There was a problem hiding this comment.
Iiuc correctly then this is breaking for servers that don't support fetch by commit. The old behavior is that in case of commit, all refs are pulled and commit needs to exist somewhere in the commit-tree. In the case of pull/nr/merge shouldn't the pull always be by ref anyway as Github can change the merge base on unrelated changes and old commit would either start giving errors or pull wrong content.
jedevc
force-pushed
the
git-clone-hidden-sha
branch
3 times, most recently
from
8a0f32d to
7476ffc
Compare
The latest changes should fix this - we detect specific error messages in the output to detect this scenario:
Specifically, the error in In this case, we can detect these specific errors, and retry without the guilty specific commit in the fetch, which is a fallback to the previous behavior.
Sure. But IMO, we should be aiming for compatibility with the |
Signed-off-by: Justin Chadwell <[email protected]>
jedevc
force-pushed
the
git-clone-hidden-sha
branch
from
7476ffc to
90d2d8b
Compare
| // testFetchUnreferencedTagSha tests fetching a SHA that points to a tag that is not reachable from any branch. | ||
| func testFetchUnreferencedTagSha(t *testing.T, keepGitDir bool) { | ||
| func TestFetchUnreferencedRefSha(t *testing.T) { | ||
| testFetchUnreferencedRefSha(t, "refs/special", false) |
There was a problem hiding this comment.
Really not sure why this test actually passes 🤔 The config uploadpack.allowReachableSHA1InWant doesn't seem to be set anywhere.
Where's this error detection code? |
Exactly the same as how we fallback when we try and set |
| package gitutil | ||
|
|
||
| func IsCommitSHA(str string) bool { | ||
| if len(str) != 40 { |
There was a problem hiding this comment.
Relevant to https://git-scm.com/docs/hash-function-transition, this should probably also accept 64, right?
Edit: sent a PR at #5471
#5072 is only half the fix - it's still possible for a commit to not be tagged or part of a branch.
For a practical example, see
refs/pull/.../merge- the commit referenced by this ref is not tagged or part of any branch, and so isn't advertised byupload-pack, and so won't be part of clone.Modern git servers should support this though - https://git-scm.com/docs/protocol-capabilities#_allow_reachable_sha1_in_want - we should be able to specify a specific commit when
fetching. We can include a fallback for if the server doesn't support this capability (not 100% sure about the logic here though 🤔 - can we assume everyone is usingupload-pack?)This might feel a little edge-casey - but they're still important! The main reason is -
git fetchsupports this, so we should do as well. Another is that it's possible to break cache consistency without this fix, e.g.:Snapshot(<commit at refs/pull/.../merge>)- failsSnapshot(refs/pull/.../merge)- succeedsSnapshot(<commit at refs/pull/.../merge>)- succeeds, since this commit is now in the cloned repoThis doesn't really make much sense, and can break reproducibility.
(Originally reported in dagger/dagger#8584)