-
Notifications
You must be signed in to change notification settings - Fork 711
Stop if cannot attain requested index-state, #8076. #8077
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
base: master
Are you sure you want to change the base?
Stop if cannot attain requested index-state, #8076. #8077
Conversation
Yes, a tiny changelog file and a test would be necessary. Let me also repeat the questions about how breaking change this is and how can be mitigate the breakage, hoping that others would join the discussion:
Edit: the question was answered in the issue ticket. Some mitigation, though, e.g., an instruction how to get the old behaviour, would be great. |
We discussed this at the cabal advisors meeting and think this makes sense. Just needs a changelog and then we can merge. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After the further issue discussion i am for throwing the error, but i would ask to suggest --index-state=HEAD
as the more direct way to override the requested index state and ignore it.
Also maybe we should wait for #8086 or make the message taking in account.
then die' verbosity $ | ||
"Stopping this command as the requested index-state=" ++ prettyShow ts0 | ||
++ " is newer than (" ++ prettyShow (isiMaxTime isi) | ||
++ "), the most recent state of '" ++ unRepoName rname |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
taking in account #8086 maybe we could change it with something like:
++ "), the most recent state of '" ++ unRepoName rname | |
++ "), the local state of '" ++ unRepoName rname |
maybe HEAD
instead of local
? maybe it would be less clear for newcomers
"Stopping this command as the requested index-state=" ++ prettyShow ts0 | ||
++ " is newer than (" ++ prettyShow (isiMaxTime isi) | ||
++ "), the most recent state of '" ++ unRepoName rname | ||
++ "'. You could try 'cabal update' to bring down a later state or request an earlier timestamp for index-state." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
++ "'. You could try 'cabal update' to bring down a later state or request an earlier timestamp for index-state." | |
++ "'. You could try 'cabal update' to bring down a later state or force the use of current local state with `--index-state=HEAD`." |
you have to use a index-sate <= HEAD and it seems to me than is more frequent use HEAD than a older one
92ef1cf
to
b995ca2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you very much for the PR. I'm entering this early review to block this on changelog, on some mitigations/warnings about the breaking change (e.g., some ideas from #8076 (comment)) and, ideally, a test.
@philderbeast are you interested in finishing this off? That’d be super cool! |
Yes I would like to finish this. |
Marking this PR as draft for now 🙂 |
#8944 is a more recent take on the same problem. |
As this is a change of behaviour, we will need a changelog entry won't we?
I intend to supplement this by changing "this command" with the actual command name.
The rendered output: