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

compare version diff from crates.io code rather than git source in update review #104

Merged
merged 3 commits into from
Jul 20, 2021

Conversation

nasifimtiazohi
Copy link
Contributor

@nasifimtiazohi nasifimtiazohi commented Jul 19, 2021

Motivation

In update review, we present a diff analysis between two version of a given crate. Analyzing the diff by downloading code from crates.io for respective versions is a reliable way rather than checking out the two versions from its git source and analyzing diff from there, which fails at cases: i) ii) when the source repository is not present or it is not a git repository, ii) when we can't figure out the head commit for a given version, iii) when the repository contains files that are ignored in crates.io code but we also then count them in our diff, e.g., libc-test files in libc crate.

Therefore, getting diff directly from crates.io code will give a reliable analysis in dependency update review report. In case of packages not hosted on crates.io, we still make use of repository diff as before.

Improvement TODOs

for both versions, Enum guppy::graph::summaries::SummarySource gives the source from where the crate has been pulled from. Incorporate that information while fetching source code for a more accurate analysis in all the cases. However, this shall be part of a larger change scope in depdive where we could identify any type of versioning change in a dep, e.g., updating to a certain commit to another from git source. Currently depdive is designed around the notion of updating from one version to another for crates hosted on crates.io. #106

bmwill
bmwill previously approved these changes Jul 20, 2021
Comment on lines 962 to 968
assert_eq!(report.diff_stats.as_ref().unwrap().files_changed, 78);
assert_eq!(report.diff_stats.as_ref().unwrap().insertions, 1333);
assert_eq!(report.diff_stats.as_ref().unwrap().deletions, 4942);
Copy link
Contributor

Choose a reason for hiding this comment

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

So the diff is more accurate now it seems :)

@nasifimtiazohi
Copy link
Contributor Author

/land

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.

3 participants