Skip to content

git, hg: Use full revision#5342

Merged
kit-ty-kate merged 1 commit intoocaml:masterfrom
reynir:revision-full-hash
Jan 4, 2023
Merged

git, hg: Use full revision#5342
kit-ty-kate merged 1 commit intoocaml:masterfrom
reynir:revision-full-hash

Conversation

@reynir
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Collaborator

rjbou commented Nov 8, 2022

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

@dra27
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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