Why is a 6-char git rev abbrev ok, but a 7-char generates a warning?

[hartzell]

I'm playing with dep for the first time.

I had used a 6-char revision string to identify a particular commit in github.com/tj/go-sms.

Then I noticed that the output of dep status used 7 digits in the REVISION column and added one more char to my revision constraint for go-sms in my Gopkg.toml. That generated this warning:

dep: WARNING: revision "e11c16a" should not be in abbreviated form

I've seen #830, which discusses [not] allowing short revision abbreviations.

I've also seen #671 (and #582, which led to it), which generates warnings on 7-digit git commits.

I'm wondering why 6 digits is "ok" and 7 generates a warning. Seems like the warning should be for "any length shorter than KNOWN_GOOD_LENGTH".

What version of dep are you using (dep version)?

dep:
 version     : v0.3.1
 build date  : 2017-09-19
 git hash    : 83789e2
 go version  : go1.9
 go compiler : gc
 platform    : darwin/amd64

[sdboyer]

hi, great question!

6 actually isn't "OK", as #830 notes - it won't work to identify the full commit. but we don't warn on it, because the domain of possible good input values for revision is large - without knowing the type of the underlying source, we can't do good validation of the contents. that's an answerable question, but it's not super easy to answer today.

it's also conceptually related to #302 - the general fact that we don't do a good job, at all, of informing users when their constraints won't work.

the only reason we warn specifically on 7 is because it's the standard shortening length that git itself, GitHub, etc. use, so we figured it was better than nothing to have warnings, at least for this case.

tl;dr: incomplete implementation 😛

[hartzell]

Make sense, thanks for the quick followup!

It might be more useful (less scary) to state something in the positive form, e.g.:

Abbreviated revision ids, like "e11c16a", are less reliable than a full commit id.

or

Abbreviated revision ids, like "e11c16a", increase the chance of a conflict.

I can PR it if you think it's useful.

[sdboyer]

@hartzell hmm...those might not be quite the right message, because they suggest to the user that this is a risk that they get to make a choice about. but that's not the case - we require the full length SHA1 so that we're guaranteed it cannot happen.

so, i'm open to a different error message, but we have to hint in the right direction.

[hartzell]

But you don't require the full length commit id. Six hex digits exits with no warnings and an exit status of 0. Seven digits generates a warning message and an exit status of 0. You want/need full commit id's lest dep not perform correctly but abbrevs still work.

If I understand your explanation, you'd like to require the full length commit id, but since that length varies depending on the particular scm you don't know what that length is. You're confident that seven is always a bad answer (no supported system only uses seven digits and git/GitHub/… are known to use it as an abbreviation so you warn when you see it. In the long run you'll have a better idea what the correct length is and then you'll actually throw an error for an abbreviated id. In the meantime, you warn on the common case and move on.

If that's an accurate summary I should just close this, breathe deeply and go back to learning more go. 😄

[sdboyer]

But you don't require the full length commit id. Six hex digits exits with no warnings and an exit status of 0. Seven digits generates a warning message and an exit status of 0. You want/need full commit id's lest dep not perform correctly but abbrevs still work.

if they work, that's...an incidental, unfortunate bug. and it does appear that they work. not only that, but they record the abbreviated SHA1 in the Gopkg.lock file, rather than the full one. 😡 🤦‍♂️😡 🤦‍♂️😡 🤦‍♂️

so many corners to cover 😢

in any case, yes, your summary is accurate, and this is just something that got by us in articulating the model. i've marked up this issue in hopes that some intrepid soul is willing to dive in and work on some warnings for these things.

[hartzell]

Ok, I'm intrepid. Is there a enough context to figure out what an acceptable length is for the commit id? Git commits are 40 chars. Can you point me at other background reading?

[sdboyer]

ugh, I'm so sorry - your response slipped through my filters! offers to do intrepid work are definitely the last thing I want to let sit 😢

the rule needs to enforce complete ids. that means forty hex chars for git and hg. for bzr, we can't tell by length alone, as it's variable - we can only apply the rules to see if it's well-formed. I know that we have some logic for those already, adapted straight from the bzr source, though I don't recall exactly where they are.