Skip to content
This repository has been archived by the owner on Sep 9, 2020. It is now read-only.

Allow short git SHAs as versions #830

Closed
wants to merge 1 commit into from

Conversation

chriswhelix
Copy link

What does this do / why do we need it?

Allow use of short git SHAs as versions; this is particularly useful because it is supported in Glide, so it allows seamlessly importing glide.yaml files with short SHAs. But more broadly, they are used pervasively in the git ecosystem, and it's inconvenient and counter-intuitive for them to be treated as invalid input.

@googlebot
Copy link
Collaborator

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If your company signed a CLA, they designated a Point of Contact who decides which employees are authorized to participate. You may need to contact the Point of Contact for your company and ask to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the project maintainer to go/cla#troubleshoot.
  • In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again.

@ibrasho
Copy link
Collaborator

ibrasho commented Jul 15, 2017

This is intentional (#567), although I'm not sure why. 🤔

@sdboyer should be able to elaborate.

@sdboyer
Copy link
Member

sdboyer commented Jul 18, 2017

Linus recognized that the 7-character abbreviation wasn't a great idea, and git got some configurability around this back in 2010.

The risk here is greater for our particular case, as the creation of commit objects in the future can result in these prefix collisions. That would be adding another vector (an existing one being leftpad) by which the world changing can invalidate existing code.

All the more reason to not use SHA1s in your Gopkg.toml, which we warn against anyway 😉

@sdboyer sdboyer closed this Jul 18, 2017
@sdboyer
Copy link
Member

sdboyer commented Jul 18, 2017

(i would accept a version of this that only allows the shortened SHA1 when doing e.g. a glide autoconversion)

@chriswhelix
Copy link
Author

It's not like dep would be /generating/ these short hashes. Why should it not be up to the individual user to determine what hash length they need?

The commit you link to does not at all say "short hashes were a bad idea, let's phase them out". What it says is "maybe 7 was too short a default, let's make it configurable by the user and think about eventually increasing the default".

If you don't want people to use SHAs in Gopkg.toml, fine, make it not supported. But if the tool is going to support a git SHA in a particular field, it seems like it should be git that determines the answer to "what is a valid git SHA?". How is that your job?

@sdboyer
Copy link
Member

sdboyer commented Jul 18, 2017

The risk here is greater for our particular case, as the creation of commit objects in the future can result in these prefix collisions. That would be adding another vector (an existing one being leftpad) by which the world changing can invalidate existing code.

@sdboyer
Copy link
Member

sdboyer commented Jul 18, 2017

If you don't want people to use SHAs in Gopkg.toml, fine, make it not supported.

And, to be clear, we discourage it because it's generally misused - just relying on the lock file is almost always adequate. But that doesn't mean we can make it unsupported, as it's necessary in some cases. So, we rely on nudging the user away from it, instead. It's less likely to matter in the cases where it actually is necessary, as those, pretty much by definition, don't change often.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants