git URLs fail when ref includes a '+' character

[jgiles]

In the Git getter, the 'ref' value used to determine which Git reference to check out is url-query-decoded, meaning that '+' signs in the ref are replaced with spaces (' ').

https://github.com/hashicorp/go-getter/blob/master/get_git.go#L35

You could argue that folks should escape '+' characters in the ref query value (with '%2B'). However, there are two very good reasons to pass through '+' to Git:

1. '+' is the official semver mechanism for appending version metadata, and server versions (prefixed with v) are widely used as GitHub tag names for releases.
2. ' ' (space) is illegal in git refs anyway

so, it would be a shame to force people to encode '+' in git refs.

[mitchellh]

I can understand this point of view but we use RFC compliant URIs and according to RFC3986 (section 2.1 to 2.4) we must percent-encode the + because it is in the set of reserved. In my experience breaking with a standard usually leads to long term pain...

By remaining compliant we retain the broadest interoperability with other software and avoid strange edge case behavior. There is an argument to be made that ` is not a legal ref value so+withinref` itself is not ambiguous (and you made this argument). While this is true, the URI parsing happens before the "getter" implementation knows what is being downloaded and the query params are therefore opaque.

Further, we use Go's standard net/url for parsing the query parameter which automatically handles the + to a . So its not as simple, but obviously not insurmountable. My argument is only that I'm still not sure its worth it breaking from a standard specification for one character.

One thing we can do in the Git getter is warn people if we see a space in the ref that they might mean %2B (if it errors). I think that'd be pretty good UX and I'm happy to do that.

[jgiles]

Yeah, I definitely understand that take.

Honestly, my implementation plan here was pretty much the most naive thing: In the Git getter, when we go to check out the ref:

https://github.com/hashicorp/go-getter/blob/3f60ec5cfbb2a39731571b9ddae54b303bb0a969/get_git.go#L97

do a string replace " " -> "+".

At that point, we know we are operating on a git ref, we know that the space is illegal, and we know that the space is actually the result of url-decoding (very likely from +, since it would be very surprising if people were sabotaging their git refs with %20)

Practically speaking, I don't think we can tenably get humans to remember to put "%2B" in all the module references across our terraform/terragrunt codebase, which puts that part of the semver spec pretty much off-limits for versioning our modules.

So, despite my normal inclination toward strict standards adherence, I find myself making a plea for this human affordance :-)

[mitchellh]

That's quite a good solution as well. So I think its either: automatically replace " " to "+" on checkout, or return an error that is more informative if we find a " " in the ref (asking the user to use %2B). Those are the two lowest friction solutions that don't impact go-getter more broadly.

The former has the negative effect of the illusion of + being a valid URI character and therefore you might run into issues in other parts of your URL. However it has the positive effect of just working.

Tough call. Any input is helpful, I'll think about it.

[jgiles]

From our point of view, if a ref with "+" requires people to escape it, we will forbid using it in terraform module version tags. It's too annoyingly error-prone for humans doing editing and it negatively impacts readability.