GitGetter: Re-allow ref=COMMIT_ID #345
Conversation
|
We seem to have some tests that lack a |
There was a problem hiding this comment.
Thanks for fixing this up properly @apparentlymart! Sorry for the mess.
get_git.go
Outdated
| autoBranch := false | ||
| if ref == "" { | ||
| ref = findRemoteDefaultBranch(u) | ||
| autoBranch = true |
There was a problem hiding this comment.
i think this is either a very subjective nit or me not understanding (or both), so feel free to ignore:
is autoBranch essentially a way to tell if we're using the default branch (so ref == "")?
if so, i think it might be clearer to either name it a little differently (maybe like onDefaultBranch or something) or use ref == "" directly.
There was a problem hiding this comment.
I guess the subtlety I was trying to capture with this name is that we're recording whether the branch was explicitly chosen, vs. it being detected automatically. Specifically, the user can potentially write ?ref=main even if main happens to be the default branch name, in which case autoBranch would be false even though we're going to end up on the default branch in the end.
Really the only motivation for the separate variable here is that we just overwrote the original ref on the line above, and so we no longer have the information about whether ref was originally "" or not. I didn't really like it when I wrote it, but was trying to change what was already here as little as possible. With that said, I think it could be clearer for the extra variable to be originalRef := ref (a string) and then test if originalRef == "" below, which I think more directly captures the intended meaning of "did the caller specify a ref?". What do you think? 🤔
There was a problem hiding this comment.
ooooo yeah. i totally missed that we were overwriting ref.
in that case, i think your proposal of adding originalRef captures what you explained really well!
Earlier versions of this code allowed "ref" to take any value that would be accepted by "git checkout" as a valid target of a symbolic ref. We inadvertently made a breaking change that broke that as part of introducing a shallow clone optimization, because shallow clone requires selecting a single branch. To restore the previous capabilities while retaining the "depth" argument, here we accept a compromise where "ref" has the stronger requirement of being a valid named ref in the remote repository if and only if "depth" is set to a value greater than zero. If depth isn't set or is less than one, we will do the old behavior of just cloning all of the refs in the remote repository in full and then switching to refer to the selected branch, tag, or naked commit ID as a separate step. This includes a heuristic to generate an additional error message hint if we get an error from "git clone" and it looks like the user might've been trying to use "depth" and "ref=COMMIT" together. We can't recognize that error accurately because it's only reported as human-oriented git command output, but this heuristic should hopefully minimize situations where we show it inappropriately.
apparentlymart
force-pushed
the
b-git-getter-ref-commit
branch
from
7c4115b to
f1c466b
Compare
Earlier versions of this code allowed
refto take any value that would be accepted bygit checkoutas a valid target of a symbolic ref. We inadvertently made a breaking change that broke that as part of introducing a shallow clone optimization, because shallow clone requires selecting a single branch. (#266)To restore the previous capabilities while retaining the
depthargument, here we accept a compromise whererefhas the stronger requirement of being a valid named ref in the remote repository if and only ifdepthis set to a value greater than zero. If depth isn't set or is less than one, we will do the old behavior of just cloning all of the refs in the remote repository in full and then switching to refer to the selected branch, tag, or naked commit ID as a separate step.This includes a heuristic to generate an additional error message hint if we get an error from
git cloneand it looks like the user might've been trying to usedepthandref=COMMITtogether. We can't recognize that error accurately because it's only reported as human-oriented git command output, but this heuristic should hopefully minimize situations where we show it inappropriately.Since this regression was found in Terraform v1.1 first, we previously merged a local fork of
GitGetterinto Terraform to expedite a fix, in hashicorp/terraform#30155.This set of changes is functionally equivalent to what we implemented and shipped in Terraform v1.1.1. My intent is to switch Terraform back to using the upstream
GitGetterin this codebase once we have changes like these included in a tagged release.This closes #341. This will also contribute to closing the Terraform issue hashicorp/terraform#30170, once something like this is included in a tagged release that we can switch Terraform to.