Repository navigation
git: add Depth to CloneConfig - #1329
ruslan-shaydullin wants to merge 1 commit into
Conversation
CloneConfig only offers ShallowClone, which clones a single commit. Consumers that need a bounded amount of history, such as a controller checking whether a new revision descends from the last verified one, had to choose between a single commit and the full history. Depth limits a branch, tag or semver clone to the given depth from the tip and takes precedence over ShallowClone, which keeps its meaning of depth 1. This change does not touch checkouts by commit or by other reference names. A negative depth is rejected before any remote operation. Signed-off-by: Ruslan Shaydullin <shaydullin.r.d@outlook.com>
|
Thank you very much for putting the effort in to get this implemented. I'm just a bit worried that the clone depth of, for example, 500 would exhaust the configured controllers memory limit and trigger an OOM eviction if a special crafted git history would need to be cloned and checked. I was kind of waiting that the go-git partial clone would make progress and land in v6. This would be the most efficient way to clone the history without the binary blobs as they are not needed for signature verification. What is your opinion on that? |
|
Thanks @bb-Ricardo, and thanks for the groundwork in #2010. The two options you listed there were a configurable depth or a partial clone. This PR is the first one, except the depth isn't left to the user. I looked at partial clone first. pkg is on go-git v5.19.3, which has no You're right about memory, and that's why the source-controller side (fluxcd/source-controller#2188) is bounded the way it is. The numbers in the description are from argo-cd: 0.75 GiB peak at depth 1 and 1.7 GiB at 500. The controller clones 100 commits first. It clones 500 only when the last verified revision isn't within those 100, and only after the signature of the new tip has been verified with a trusted key, so a crafted history pushed without a trusted key never gets the deeper clone. A baseline more than 500 commits back is rejected without cloning further, and while the reference still points at the rejected revision it's retried without a clone. The option is off unless it's set on a GitRepository. Would that be enough for you, or would you prefer a lower second depth, say 300? I'm happy to move the deep clone to |
|
On the other hand, this is "only" implementing the possibility to specify the clone depth. For me this would be fine for now but I'm not a member of this project. And once this is merged (hopefully) we should straight away create a follow up issue to implement |
|
Agreed. Once this is merged I'll open the follow-up issue for partial clone and link go-git/go-git#1381, so it gets picked up when that lands. |
CloneConfigonly offersShallowClone, which clones a single commit. A consumer that needs a bounded amount of history has to choose between one commit and the full history.This adds
CloneConfig.Depth: the number of commits to clone along each parent chain from the tip of a branch, tag or semver tag. It takes precedence overShallowClone, which keeps its meaning of depth 1, and a negative value is rejected before any remote operation. Checkouts by commit and by reference names that are neither a branch nor a tag are not touched here. WithoutDepthnothing changes and the existing tests pass as they are.The consumer is a change in source-controller for fluxcd/source-controller#2010: a
.spec.verify.forwardOnlyoption onGitRepositorythat rejects a revision which does not descend from the last verified one. To find the previous revision the controller clones the last 100 commits instead of the tip and, if that is not enough, discards the clone and clones the last 500 into a fresh directory. Fetching more history into the existing clone was ruled out by the comment inclient.goand by the outcome of #1289: a fresh clone works on every provider. A draft PR in source-controller follows once this one has a number.The next step on the same field is fluxcd/source-controller#2146, applying
Depthto a checkout by commit when a branch is set. That is why this PR pins no behaviour for commit checkouts.One question for review: the ancestry walk itself (breadth first over both parents of a merge, tolerant of shallow boundaries) currently lives in the controller. If you would rather have it in
git/gogit, I have a variant ready with an exportedFindAncestor(path, head, ancestor git.Hash, limit int)and can switch the same day.A note from measuring this client on argoproj/argo-cd (11.7k commits) with go-git v5.19.3, which may interest you: the peak memory of a shallow clone grows with the depth, from 0.75 GiB at depth 1 to 1.7 GiB at 500 and 2.1 GiB at 1000, and from a depth of roughly 150 on it exceeds a full clone (1.1 GiB), while traffic and disk stay far below it (62 MB at 500 against 174 MB). The controller sizes its two depths with that in mind.
Tests:
TestClone_depthcovers branch, tag, semver andrefs/heads/*andrefs/tags/*names withDepth: 2on three commits (exactly two arrive, the third givesErrObjectNotFound, one entry in.git/shallow),DepthwithoutShallowClone,ShallowClonewithoutDepth, and a negative depth that creates nothing.make test-gitpasses.