Git "ref" parameter described as taking a ref, but is a tag or branch name only

[tomqwpl]

The documentation for the "ref" parameter is

The Git ref to checkout. This is a ref, so it can point to a commit SHA, a branch name, etc. If it is a named ref such as a branch name, go-getter will update it to the latest on each get.

However the implementation is to call the "--branch" option on the git command line, meaning that it has to be either a branch or tag name. The implementation of --branch in git is to list through the refs, finding either "refs/heads/" (for priority) or "refs/tags/".

Experimentation also shows that you can't pass a SHA, or string such as "refs/heads/master" etc, you can only pass a branch name or tag name. There are tests in go-getter that test the ref parameter being a branch name or a tag name, but nothing further also.

So I think the documentation needs clarifying here.

[tomqwpl]

On the face of it, commit 69e534e changed the behaviour here.

Previous to that commit, the clone was done without any arguments, so the default master would be checked out. Then a "checkout" is run passing the given ref. The "git checkout" command takes a proper ref as an argument, so you can pass a commit hash for example.

After that commit it doesn't get that far as the "ref" is passed as the value of the "--branch" argument to "git clone", and that only takes branch names or tag names, not an arbitrary ref.

So clarification on the intent here. If the intent is as per the documentation, and that an arbitrary ref be accepted, then this is broken. If that wasn't the intention, then the documentation needs updating.

[tomqwpl]

@zachwhaley Can you comment on the intent here, since you made the commit that seems to have altered the behaviour?
Thanks.

[zachwhaley]

@tomqwpl the intent was to fix the broken shallow cloning of git repos. If depth was provided, then one could only use the default branch (or possibly a commit depth commits behind the default branch).
See my commit message for more details.

It does appear that my changes have prevented the use of commit refs entirely. Any fix would have to take depth into account. Eg something like ref=SHA?depth=2 would likely not work.

A solution could be to add a new param branch that could use depth as well and would use the clone —branch command, and to return the ref param to use the checkout command; I would recommend not allowing ref and depth to be used together or to document the limitations.

That’s my 2¢ 🙂

[apparentlymart]

This behavior change seems to be the root cause of the Terraform v1.1.0 compatibility regression described in hashicorp/terraform#30119.

In order to work within the Terraform v1.0 Compatibility Promises we need Terraform to support assigning to ref anything that git checkout would accept, since that was the prior behavior.

It sounds from earlier discussion like the git clone behavior needs to vary depending on whether the specified value is a named ref or a raw commit, but I don't believe it's possible to distinguish those two syntactically because a branch can potentially be named as a sequence of hexadecimal digits.

It does make sense to me that depth would only be compatible with specifying a named ref because shallow clone is a clone option and cloning works with remote named refs, rather than anything that might end up being in the local repository after clone completes.

Therefore I think the following set of rules would make sense, which I believe agrees with @zachwhaley's earlier recommendation but please let me know if not:

• If the "URL" includes ref but not depth then it should do the old behavior of cloning the full set of refs from the remote repository and then git checkout to whatever was specified in ref, thus allowing the full set of possible syntaxes that git checkout would accept.

• If the "URL" includes both ref and depth then that opts in to the new shallow-clone mode, in which case ref is required to be a valid named ref existing in the remote repository (that would be accepted by git clone -branch=...).

  If the clone operation fails and the ref argument happens to be entirely hex digits then we might potentially include an additional hint in the error message that depth is only compatible with named branches, because otherwise the error message directly from git clone tends to be too deep in the implementation details to be readily understood by a user who isn't aware of exactly what Git commands go-getter is running.

What I'm not sure of here is whether implementing the above would represent a regression for some other go-getter caller, other than Terraform, that has already been using this newer behavior for some time now. I expect that this would at worst represent a performance regression for anyone who had previously benefited from specifying only a single branch in git clone and thus avoiding downloading other branches. However, that both assumes that such a performance regression is acceptable, and it also assumes that the acceptable values for the -branch=... option of git clone are a strict subset of what would be accepted by git checkout.

We have in the past forked some components of go-getter into the Terraform codebase in order to preserve backward compatibility even when go-getter didn't, so that'll be my backup plan here if we find ourselves at an empasse where there's no compromise that works for all callers, but I'm hopeful that because apparently the original motivation for this was a Terraform performance enhancement (over in hashicorp/terraform#10703) we still have some room here to compromise on the performance enhancement so that it's only activated by someone explicitly opting in by setting the depth argument.

[apparentlymart]

Just wanted to document here what I've learned so far. Thanks to some folks on other teams at HashiCorp who helped me fill in some details here.

• HashiCorp Nomad has an upgrade to go-getter v1.5.9 merged but not yet included in any stable release. Therefore Nomad is not currently a potential compatibility constraint here, and indeed Nomad would probably benefit from this being fixed before they do the release so that it won't also inherit the same breaking change.
• HashiCorp Waypoint seems to depend on go-getter but currently has v1.4.1 selected and so doesn't include the breaking change we're discussing here.
• HashiCorp Packer itself no longer uses go-getter directly since factoring out the plugins into separate repositories, but because several different packer plugins need to fetch files consistently the Packer plugin SDK includes some shared code that wraps go-getter for any plugin that needs to retrieve a file (e.g. an ISO image for a VM, or similar). At least some Packer plugins are already using a version of the SDK which includes go-getter v2.0.0.

Interestingly it seems that go-getter v2.0.0 doesn't actually include the changes from #266, which is curious but convenient in this case because it means that we don't need to worry about Packer plugins for the sake of this issue. I expect we will eventually want to rationalize these to both have the same behavior for the git getter but for our purposes here we can focus on fixing the v1 series and know that it won't affect Packer plugins at all, due to Go semantic import versioning.

With all of that said, I only have insight into HashiCorp project callers in particular here and so I can't guarantee there aren't external callers that might be using the new version, or other HashiCorp codebases I'm not considering, but based on what I've learned so far I'm feeling optimistic that we can safely move forward with the compromise I described previously, with it serving as a fix to the regression in Terraform and it avoiding a similar regression in Nomad, if we fix it prior to Nomad's next release and make sure Nomad adopts the new go-getter version containing the fix.

[nywilken]

@apparentlymart thanks for the detailed writeup here. I'm pinging @azr on the issue so that he can, maybe help, shed some light around the current state of go-getter v2, and help determine any potential next steps for v2 and Packer.

[azr]

Hello there, seems like it would be nice to add more tests here. The v2 branch always lags a bit from the commits in the v1/master one. I remember not feeling like reviewing that PR too much because of its potential impact and lack of tests. Good luck with the fixes :D

[apparentlymart]

FWIW, just to make sure this is more visible to those who might be affected by it: we needed to meet a (self-imposed) deadline for the Terraform CLI v1.1.1 release due to it including some other fixes too, and so we landed a local fork of the "git getter" in Terraform and patched it there as a temporary way to expedite it.

However, I am still planning to return here and propose upstreaming essentially the same logic (including some new tests!) when I get a little more time to work on it, since there are some other things going on for me this week. The diff against the Terraform-local fork is in Terraform's commit c4d46e7c6b9e, in case anyone wants to see a preview.

[azr]

Dope, I looked at the code, and it looks pretty good. I wanted to add that the v2 branch sort of moved plugins out of the main repo and made them optional and importable for other codebases. If we had the time it could also be a nice idea to try having a getter that uses https://github.com/go-git/go-git, this could allow us to avoid having to do weird hacks and using exec to use git. Probably not for right now, but I think this is a possibility to keep in mind for later.

[apparentlymart]

Yes, using a library-ized implementation of the Git protocols/conventions instead of the external Git tool would have some good benefits, presumably including us being able to properly detect when the given ref is a named branch vs. a direct commit ID and return a more specialized error message for it!

I am indeed not going to try to tackle that right now, because preserving backward-compatibility with all of the quirks of the current implementation seems like it'll take a lot of careful study and testing, but definitely an interesting avenue to explore for later!