Skip to content
This repository was archived by the owner on Sep 9, 2020. It is now read-only.

Fixes #623. LockDiff now ignores hash/solvemeta. #788

Closed

Conversation

mikijov
Copy link
Contributor

@mikijov mikijov commented Jun 24, 2017

Fixes #623. LockDiff now uses only project dependencies to identify changes, thus ignoring any changes to the SolveMeta iformation. If required changes in the SolveMeta can still be seen using source diffs. Modified and added tests to check new behaviour.

The solution was suggested by @sdboyer in #623. The code change is simple. I am more looking for comments if you can think of any unintended consequences of not comparing hashes any more.

LockDiff now uses only project dependencies to identify changes,
thus ignoring any changes to the SolveMeta iformation. If required
changes in the SolveMeta can still be seen using source diffs.
Modified and added tests to check new behaviour.
@mikijov mikijov force-pushed the 623-lock-should-not-compare-solvemeta branch from fd4fa35 to 6cc42a8 Compare June 29, 2017 12:02
Copy link
Member

@sdboyer sdboyer left a comment

Choose a reason for hiding this comment

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

i'm looking this over and i think it looks good. i started in on the implications of not comparing the hashes, and i think there mostly aren't any, as we do all the necessary comparisons earlier on, prior to the txn writer (where this is actually used). that's most of the design.

however, i think we may need one manual check of the hashes, as even if nothing changes in the locked project set, we do still need to write out a new lock if the hash changes.

...hmmm. actually, i'm thinking about this, and i'm not sure about the UX now. i think i originally wanted this because i didn't want to bother the user when the input hash changes, but i'm not sure that's right. we always have to rewrite the lock with the new hash, and we should still probably tell the user that such a change occurred.

maybe i need to go back and revisit my original motivations. inputs/thoughts welcome 😄

@carolynvs
Copy link
Collaborator

carolynvs commented Jul 20, 2017

In my mind, the purpose of the lockdiff is to support dry-runs, and giving people fair, accurate warning of what dep is about to do. As a user, if I ran dep ensure -n and dep reported back that it would do nothing, then when I ran it, it rewrote the lock, I would feel that the dry-run can't be trusted. 😀

@sdboyer
Copy link
Member

sdboyer commented Jul 21, 2017

@carolynvs yeah, i think you're right. this was a bad idea 😞 sorry, @mikijov.

closing.

@sdboyer sdboyer closed this Jul 21, 2017
@mikijov
Copy link
Contributor Author

mikijov commented Jul 22, 2017

No problem. Please mark few more issues as help-wanted. :-)

@sdboyer
Copy link
Member

sdboyer commented Jul 22, 2017 via email

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants