Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

git, hg: Use full revision #5342

Merged
merged 1 commit into from
Jan 4, 2023
Merged

Conversation

reynir
Copy link
Contributor

@reynir reynir commented Nov 8, 2022

Before, the revision was at most the first 8 bytes in the revision. This can be problematic for reproducibility since the revision may then be ambiguous. I don't know much about mercurial and how their revisions look like.

CC @hannesm

@hannesm
Copy link
Member

hannesm commented Nov 8, 2022

I completely agree with this PR. I do not understand the reason for using only the first 8 characters.

For reference, the code was introduced nearly 10 years ago and never changed fb8fece

For reference, dune uses 7 characters (since 3.0.0 ocaml/dune@6b69c61, also ocaml/dune#4855).

I understand the desire to have a stable output (i.e. a fixed length), but then I also don't quite understand why the conditional may ever hit the case <= 8 characters.

@hannesm
Copy link
Member

hannesm commented Nov 8, 2022

Please let me note that git increased at some point from using 6 characters to 7 (in git describe), and with more sophisticated attacks on SHA1, I don't see any value in truncating the output.

Before, the revision was at most the first 8 bytes in the revision. This
can be problematic for reproducibility since the revision may then be
ambiguous.
@rjbou
Copy link
Collaborator

rjbou commented Nov 8, 2022

Agree on the idea, it needs some testing for mercurial part

@dra27
Copy link
Member

dra27 commented Nov 9, 2022

I agree, too - it obviously makes sense to display the shortest unique prefix for a clearer display to a user, but certainly not to be using that internally.

@hannesm
Copy link
Member

hannesm commented Dec 27, 2022

I'm wondering what the status of this PR is and what can be done to get it merged? Since three opam developers are supporting this PR, I propose to merge it rather sooner than being delayed for unclear reasons ("it needs some testing for mercurial part" -- so who's using mercurial and opam these days? If this is a showstopper, maybe revert the changes to opamHg.ml?)

@kit-ty-kate
Copy link
Member

I've just tested locally using mercurial and it works fine (the default hash size for mercurial is 12)

Thanks a lot!

@kit-ty-kate kit-ty-kate merged commit 601e244 into ocaml:master Jan 4, 2023
@kit-ty-kate kit-ty-kate added this to the 2.2.0~alpha milestone Jan 4, 2023
@rjbou rjbou mentioned this pull request Feb 15, 2023
kit-ty-kate added a commit to kit-ty-kate/opam that referenced this pull request Feb 17, 2023
rjbou pushed a commit to kit-ty-kate/opam that referenced this pull request May 12, 2023
@rjbou rjbou modified the milestones: 2.2.0~alpha, 2.1.5 Jul 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants