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

Output is not deterministic #63

Closed
neersighted opened this issue May 9, 2021 · 5 comments · Fixed by #87
Closed

Output is not deterministic #63

neersighted opened this issue May 9, 2021 · 5 comments · Fixed by #87
Labels
bug Something isn't working

Comments

@neersighted
Copy link

neersighted commented May 9, 2021

The output of vivid generate is not deterministic -- subsequent runs differ. Scenarios where the output of Vivid is cached (or used in an idempotent script) could be improved by making the output deterministic.

@sharkdp
Copy link
Owner

sharkdp commented May 9, 2021

Thank you for reporting this.

output is not idempotent

Do you mean: the output is not deterministic?

If so, you are absolutely right. I did not know that. It's probably due to the use of hash maps. I agree that this would be worth fixing.

That's probably even a bug, because the order of entries in LS_COLORS matters in case of conflicts:

touch test.txt
LS_COLORS='*.txt=0;31:*.txt=0;33' ls --color
LS_COLORS='*.txt=0;33:*.txt=0;31' ls --color

@sharkdp sharkdp added the bug Something isn't working label May 9, 2021
@neersighted neersighted changed the title Output is not idempotent Output is not deterministic May 9, 2021
@neersighted
Copy link
Author

neersighted commented May 9, 2021

Err, yes, I crossed a wire there. Non-deterministic results make idempotent use of vivid in scripts difficult.

The use of HashMaps makes sense -- an ordered collection would probably easily fix this (but then, the real question becomes what the 'correct' order is).

@sharkdp
Copy link
Owner

sharkdp commented May 9, 2021

Err, yes, I crossed a wire there. Non-deterministic results make idempotent use of vivid in scripts difficult.

👍

The use of HashMaps makes sense -- an ordered collection would probably easily fix this

Yes. (Maybe it's also the YAML deserialization.)

but then, the real question becomes what the 'correct' order is

I think we shouldn't care too much about this question and rather implement a conflict-detection, if that is needed.

If there are two patterns like *.txt and *CMakeLists.txt (which needs the glob star in the beginning, unfortunately), we should make sure that the second one has precedence in case there is a file called CMakeLists.txt. But I think this is already guaranteed by the LS_COLORS "format" (or the reference implementation in ls) - but this should be checked again.

If there are really two *.txt patterns, this should be an error.

@tavianator
Copy link
Contributor

If there are two patterns like *.txt and *CMakeLists.txt (which needs the glob star in the beginning, unfortunately), we should make sure that the second one has precedence in case there is a file called CMakeLists.txt. But I think this is already guaranteed by the LS_COLORS "format" (or the reference implementation in ls) - but this should be checked again.

Not really, ls is strictly last-match-wins:

image

Vivid should probably sort the suffixes from shortest to longest so that the most specific one will match.

@tavianator
Copy link
Contributor

Vivid should probably sort the suffixes from shortest to longest so that the most specific one will match.

Oh, it already does:

vivid/src/main.rs

Lines 193 to 194 in 700c9a6

let mut filetypes_list = filetypes.mapping.keys().collect::<Vec<_>>();
filetypes_list.sort_unstable_by_key(|entry| entry.len());

tavianator added a commit to tavianator/vivid that referenced this issue Jun 10, 2022
tavianator added a commit to tavianator/vivid that referenced this issue Jun 10, 2022
sharkdp pushed a commit that referenced this issue Jun 13, 2022
@jaminthorns jaminthorns mentioned this issue Mar 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants