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 git SHAs comparison for update. #1513

Merged
merged 1 commit into from
May 20, 2017
Merged

Conversation

alinpopa
Copy link
Contributor

@alinpopa alinpopa commented Mar 12, 2017

Let me give you a bit of a context here:

Conditions to replicate this issue:

Some other things I've considered while having this issue:

  • why that is_lock restriction in place? if the responsibility of needs_update will stay entirely within the resolver, would that restriction still be needed?
  • test dependencies and plugins are not locked, and this makes it quite hard to refresh the code for those using the resolver - is there any plan of addressing these sort of things?

86e883b always returns the full length SHA,
therefore when using a dependency having the short SHA,
it'll always consider that the SHAs are different,
hence it'll alway return true for .
@ferd ferd self-requested a review March 12, 2017 14:56
Ref1
case Length >= 7 of
true -> lists:sublist(Ref1, Length);
false -> Ref1
Copy link
Contributor

Choose a reason for hiding this comment

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

is this to allow backwards compatibility with older lock files? i think i'd prefer if we forced users to explicitly update the lock file but i'm not sure how strongly i prefer that

Copy link
Collaborator

Choose a reason for hiding this comment

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

Doesn't it need to sublist it because the user can put any ref they want in the rebar.config? So it isn't an issue for the lock file, but it is for what the user may put in their on dep list.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If these are questions for me, I believe the answers are based on some historical context that I'm not aware of; all I can say, is that the code the way it is, doesn't seem to work properly.

@ferd
Copy link
Collaborator

ferd commented May 11, 2017

@talentdeficit @tsloughter are you both okay with merging this? It's been there a while and it looks okay by me.

@tsloughter
Copy link
Collaborator

Yea, this looks good.

@tsloughter tsloughter merged commit 486cfa2 into erlang:master May 20, 2017
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.

4 participants