Skip to content

Conversation

@pirDOL
Copy link

@pirDOL pirDOL commented Dec 29, 2025

Description

feat(dfget): add support for digest-manifest

Related Issue

#1561

Screenshots (if appropriate)

  1. digest not match
image
  1. check_every_file_entry_has_a_digest
image
  1. parse_digest_manifest
image

Copy link
Member

@gaius-qi gaius-qi left a comment

Choose a reason for hiding this comment

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

Please fix lint.

@codecov
Copy link

codecov bot commented Dec 30, 2025

Codecov Report

❌ Patch coverage is 93.99586% with 29 lines in your changes missing coverage. Please review.
✅ Project coverage is 44.56%. Comparing base (7f71dee) to head (c01b828).
⚠️ Report is 30 commits behind head on main.

Files with missing lines Patch % Lines
dragonfly-client/src/bin/dfget/main.rs 93.99% 29 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1570      +/-   ##
==========================================
+ Coverage   42.89%   44.56%   +1.67%     
==========================================
  Files          75       75              
  Lines       21159    21241      +82     
==========================================
+ Hits         9077     9467     +390     
+ Misses      12082    11774     -308     
Files with missing lines Coverage Δ
dragonfly-client-core/src/error/mod.rs 59.09% <ø> (ø)
dragonfly-client/src/bin/dfget/main.rs 62.15% <93.99%> (+12.07%) ⬆️

... and 17 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@pirDOL pirDOL force-pushed the main branch 7 times, most recently from 45b9e8c to d6d2560 Compare December 30, 2025 11:21
@pirDOL
Copy link
Author

pirDOL commented Dec 30, 2025

Please fix lint.

done.
I'll pay attention to pr checks later.

@gaius-qi gaius-qi added this to the v2.5.0 milestone Jan 5, 2026
let mut digests = HashMap::new();
let mut line_num = 0;

for line in content.lines() {
Copy link
Member

Choose a reason for hiding this comment

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

for (line_num, line) in content.lines().enumerate()

Copy link
Author

Choose a reason for hiding this comment

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

done

}

// Parse line: <algorithm>:<digest> <filepath>
let parts: Vec<&str> = line.split_whitespace().collect();
Copy link
Member

Choose a reason for hiding this comment

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

.split_once(char::is_whitespace)

Copy link
Author

Choose a reason for hiding this comment

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

done

}

// Remove trailing '/' from URL if present
let mut base_url = url.to_string();
Copy link
Member

Choose a reason for hiding this comment

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

let base_url = url.as_str().trim_end_matches('/');

Copy link
Author

Choose a reason for hiding this comment

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

done

) -> Result<()> {
for entry in file_entries {
// Check if the entry URL exists in digests hashmap using exact match
if digests.get(&entry.url).is_none() {
Copy link
Member

Choose a reason for hiding this comment

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

contains_key()

Copy link
Author

Choose a reason for hiding this comment

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

done

}

// Remove trailing '/' from URL if present
let base_url = url.as_str().trim_end_matches('/');
Copy link
Member

Choose a reason for hiding this comment

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

Move the 'base_url' out of the loop to improve performance.

Copy link
Author

Choose a reason for hiding this comment

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

done

Comment on lines 678 to 690
// Parse line: <algorithm>:<digest> <filepath>
let (digest_part, filepath_part) = match line.split_once(char::is_whitespace) {
Some((digest, filepath)) if !filepath.trim().is_empty() => (digest, filepath),
Some(_) | None => {
return Err(Error::DigestManifestParseFailed {
filepath: manifest_path.to_string_lossy().to_string(),
error: format!(
"invalid digest manifest format at line {}: expected '<algorithm>:<digest> <filepath>'",
line_num
),
});
}
};
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// Parse line: <algorithm>:<digest> <filepath>
let (digest_part, filepath_part) = match line.split_once(char::is_whitespace) {
Some((digest, filepath)) if !filepath.trim().is_empty() => (digest, filepath),
Some(_) | None => {
return Err(Error::DigestManifestParseFailed {
filepath: manifest_path.to_string_lossy().to_string(),
error: format!(
"invalid digest manifest format at line {}: expected '<algorithm>:<digest> <filepath>'",
line_num
),
});
}
};
// Parse line: <algorithm>:<digest> <filepath>
// Use `split_once` for cleaner parsing logic and clearer error handling.
let (digest_part, filepath_part) = line.split_once(char::is_whitespace)
.filter(|(_, fp)| !fp.trim().is_empty())
.ok_or_else(|| Error::DigestManifestParseFailed {
filepath: manifest_path.to_string_lossy().to_string(),
error: format!(
"invalid digest manifest format at line {}: expected '<algorithm>:<digest> <filepath>'",
display_line_num
),
})?;

Copy link
Member

Choose a reason for hiding this comment

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

To avoid duplicated match sentences.

Copy link
Author

Choose a reason for hiding this comment

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

done

format!("{}/{}", base_url, filepath)
};

digests.insert(key, digest_part.to_string());
Copy link
Member

Choose a reason for hiding this comment

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

Suggest avoiding the duplicate item before returning the parsed data.

Copy link
Author

Choose a reason for hiding this comment

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

done

Copy link
Member

@mingcheng mingcheng left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants