Fix updating pins on crates which track Alire-generated files#1788
Conversation
ce9a5c8 to
1469174
Compare
|
Ready for review @mosteo. 587a1b2 is optional: users shouldn't really be making manual changes to the contents of |
|
I haven't reviewed it yet, but if the warning is going to be due to changes to Alire-generated files, maybe we might exclude those from the warnings. |
| and then Query | ||
| (Question => | ||
| "Do you want to discard local uncommitted changes in '" | ||
| & Destination | ||
| & "'?" | ||
| & New_Line | ||
| & "These changes were probably generated by Alire, so " | ||
| & "select 'Yes' unless you made them yourself.", | ||
| Valid => (Yes | No => True, others => False), | ||
| Default => Yes) | ||
| = Yes | ||
| then |
There was a problem hiding this comment.
I don't feel comfortable with telling the user it is probably OK to go ahead. What about:
-
Print something like "The update will overwrite these changed files in pinned crate blah:" and the output of
git status. -
If changes are confined to
alire/orconfig/, say something like "changes affect Alire autogenerated files that are safe to overwrite" and default to Yes. -
Else, say something like "changes affect user files, are you sure?" and default to No.
If detecting 2) is too complex, just say that changes within alire and config are OK to ignore and to make sure there are no other changes.
There was a problem hiding this comment.
My concern with going down this route was that the file location is only a rough heuristic for the source of the changes; changes made by actions like e.g. Alire's version_patcher will be categorised as "user files", while conversely users might make manual changes to the files in config/ which would then be confidently asserted as "safe to overwrite".
In particular, defaulting to No means that alr -n update will fail when actions change "user files", which may require workarounds in non-interactive use cases like CI pipelines.
I'm happy to implement as requested, but just checking you've considered these drawbacks.
There was a problem hiding this comment.
Usually what we (I) have done in the past is always default to Yes when --force is applied, so we could do the same here.
Editions inside config are normally overwritten (without warning in the normal use case!) anyway, so the only problem I can see here is having a disabled configuration but still using config for manual files, or extra files in there, which certainly could happen, as misguided as it sounds (to me)... We operate under the assumption that config/* is fair game, and we could narrow it even more to the actual files Alire is generating only. Still, it seems to me a corner case not meriting complicating life for the regular case, and the warning when interactive will happen anyway.
For actions touching user files under version control, I normally would want to review those, no matter that I didn't do the edition manually (e.g., something like version_patcher), although I haven't encountered this situation in actual development (changes by version_patcher don't go into VC), so again to me the warning as suggested is legit.
As for CI uses, I expect that's not the regular situation in which a user is editing several crates via pins for which changes are to be preserved. Again, I have not seen this happening (pins suggest a temporary situation anyway).
In the end, this has never happened to me, so if you encountered the situation, you may have a better grasp of the actual user expectations. We can also ping @Fabien-Chouteau for a third opinion.
There was a problem hiding this comment.
No, that all sounds good. The only occasion I've encountered this is the linked issue, so the possibility of users making their own changes is entirely speculative.
I'll make the default Yes if --forceed, and otherwise implement as suggested above.
There was a problem hiding this comment.
Sound like a good compromise to me 👍
|
Merged, thanks. |
Closes #1787.