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

Fix updating pins on crates which track Alire-generated files #1788

Merged
merged 4 commits into from
Nov 22, 2024

Conversation

Seb-MCaw
Copy link
Contributor

@Seb-MCaw Seb-MCaw commented Nov 4, 2024

Closes #1787.

@Seb-MCaw Seb-MCaw force-pushed the fix/update-dirty-pin branch from ce9a5c8 to 1469174 Compare November 5, 2024 14:46
@Seb-MCaw Seb-MCaw marked this pull request as ready for review November 5, 2024 15:37
@Seb-MCaw
Copy link
Contributor Author

Seb-MCaw commented Nov 5, 2024

Ready for review @mosteo.

587a1b2 is optional: users shouldn't really be making manual changes to the contents of alire/cache/, but some might anyway, so it adds a user confirmation prompt before permanently discarding any changes. Most such prompts will arise from Alire's own changes, though, so I'm not sure whether it creates more problems than it solves. Let me know which version you'd prefer.

@mosteo
Copy link
Member

mosteo commented Nov 11, 2024

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.

Comment on lines 234 to 245
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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't feel comfortable with telling the user it is probably OK to go ahead. What about:

  1. Print something like "The update will overwrite these changed files in pinned crate blah:" and the output of git status.

  2. If changes are confined to alire/ or config/, say something like "changes affect Alire autogenerated files that are safe to overwrite" and default to Yes.

  3. 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sound like a good compromise to me 👍

@Seb-MCaw Seb-MCaw requested a review from mosteo November 21, 2024 14:48
@mosteo mosteo merged commit 0bf0571 into alire-project:master Nov 22, 2024
24 checks passed
@mosteo
Copy link
Member

mosteo commented Nov 22, 2024

Merged, thanks.

@Seb-MCaw Seb-MCaw deleted the fix/update-dirty-pin branch November 22, 2024 11:56
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.

Change of pin branch may fail if checkout becomes dirty
3 participants