Fix commit hash comparison with abbreviated hash #386
Conversation
|
Thanks @straight-shoota! I was working on a similar fix already 😅, but I like the operator you used for the comparison. However, I'm not sure about displaying the full hash when installing. I think it's unnecessary because if it works correctly it's because there was a match with the ref. I might only want to see the value so I can make a quick check if the right commits was selected (and GitHub shows the first 7 digits as well, so the visual match is easier). Bundler does a similar thing. It shows the first 7 chars of the hash, no matter how many digits were given in the Gemfile: When there is an error, it's different, because the user needs to understand why the value didn't match: The two values live in different objects in Shards. The value that was matched is always a |
| @@ -46,12 +46,16 @@ module Shards | |||
| def initialize(@commit : String) | |||
| end | |||
|
|
|||
| def =~(other : GitCommitRef) | |||
| commit.size >= 7 && other.commit.starts_with?(commit) | |||
There was a problem hiding this comment.
Why impose this minimum limit? I think git accepts a minimum of 4 digits if there is no conflicts.
There was a problem hiding this comment.
At this point it's not trivial to check whether there are no conflicts. With a minimum size of 7 the probability for a conflict is very low.
I don't think that's the correct solution though. If we keep this behaviour it should already fail when reading a hash that is too short for comparison.
But a better solution would probably to check whether the abbreviated commit is actually conflict-free by asking git to resolve it. Then we can be sure that the starts_with? match is valid.
There was a problem hiding this comment.
Because at that point it shouldn't make any assumptions at all. It would be correct even allowing no minimum size for the hash. The resolution will fail at a different place if there is not matching commit or if there is a conflict.
The second part is already being done, although it could handle the error differently to show a more meaningful message. For example, when the hash is incorrect or have conflicts it's currently showing:
Unable to satisfy the following requirements:
- `db (commit abcdef)` required by `shard.yml`
That's because GitResolver already returned an empty set for the available versions.
| def to_git_ref | ||
| @commit | ||
| end | ||
|
|
||
| def to_s(io) | ||
| io << "commit " << @commit[0...7] | ||
| def to_s(io, *, commit_length = nil) |
There was a problem hiding this comment.
I don't see where this commit_length argument is in use
There was a problem hiding this comment.
I originally wanted to use that to display different lengths depending on context, but decided that's too complex for this fix PR.
There was a problem hiding this comment.
Please, check my comment: #386 (comment)
Actually I think the right way to solve this is raise an error with a better message at latest_version_for_ref when there is no available matches, (instead of returning nil). That message should contain the exact input read from the shard.yml. And leave here the abbreviated commit. At that point it will be used only to show conflicts with other dependencies, but already knowing the commit exists.
|
@waj do you want to take over? You probably know better how to deal with this. |
|
@straight-shoota absolutely! 😄 I'll schedule some time to write some docs too. |
This also enables full-length commit hash reporting for dependencies. Maybe we could use abbreviated hashes in non-critical situations (such as error messages), but I think it might actually be better to always be explicit. Otherwise there's no indication whether the actual hash value is abbreviated or it's only abbreviated for display.
Resolves #384