chunking: Fix specific component file allocation#1442
chunking: Fix specific component file allocation#1442cgwalters merged 1 commit intobootc-dev:mainfrom
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request fixes a bug where specific component layers were using the wrong metadata object for file allocation. The change correctly uses the specific_meta object for this purpose. However, this change introduces some code duplication. My review includes a suggestion to refactor the duplicated logic into a helper function to improve maintainability.
crates/ostree-ext/src/chunking.rs
Outdated
| let mut specific_map = IndexMap::<ContentID, Vec<&String>>::new(); | ||
| for (checksum, contentid) in specific_meta.map.iter() { | ||
| specific_map | ||
| .entry(Rc::clone(contentid)) | ||
| .or_default() | ||
| .push(checksum); |
There was a problem hiding this comment.
This logic for creating a reverse map is duplicated later in this function at lines 359-362. To avoid code duplication and improve maintainability, consider extracting this into a private helper function.
fn build_reverse_map(map: &IndexMap<ContentID, Vec<&String>>) -> IndexMap<ContentID, Vec<&String>> {
let mut rmap = IndexMap::new();
for (checksum, contentid) in map.iter() {
rmap.entry(Rc::clone(contentid)).or_default().push(checksum);
}
rmap
}
let specific_map = build_reverse_map(&specific_meta.map);
crates/ostree-ext/src/chunking.rs
Outdated
| let mut rmap = IndexMap::<ContentID, Vec<&String>>::new(); | ||
| for (checksum, contentid) in meta.map.iter() { | ||
| rmap.entry(Rc::clone(contentid)).or_default().push(checksum); | ||
| } |
There was a problem hiding this comment.
This logic for creating a reverse map is duplicated earlier in this function at lines 316-321. To avoid code duplication and improve maintainability, consider extracting this into a private helper function.
fn build_reverse_map(map: &ObjectMetaMap) -> IndexMap<ContentID, Vec<&String>> {
let mut rmap = IndexMap::new();
for (checksum, contentid) in map.iter() {
rmap.entry(Rc::clone(contentid)).or_default().push(checksum);
}
rmap
}
let rmap = build_reverse_map(&meta.map);
ckyrouac
force-pushed
the
chunking-xattrs-fix
branch
from
7491656 to
e0fc35d
Compare
cgwalters
left a comment
cgwalters
left a comment
There was a problem hiding this comment.
Hmm, how hard would a unit test for this be? AIUI this bug would be provoked when there's a common object between layers?
|
Yea I'll write a unit test. This was exposed by the integration test I'm working on in rpm-ostree. The specific components were being created but the files were landing in the extra stuff layer because this code was looking for them in the wrong bucket when doing the layer mapping. |
This fixes the bug where the specific layers were looking at the wrong meta object to allocate files to the layer. Assisted-by: Claude code Signed-off-by: ckyrouac <ckyrouac@redhat.com>
ckyrouac
force-pushed
the
chunking-xattrs-fix
branch
from
e0fc35d to
8da71a2
Compare
| let mut specific_components_objects_map = IndexMap::new(); | ||
|
|
||
| // SPECIFIC COMPONENT 1 (pkg1): owns 3 objects | ||
| let pkg1_objects = ["checksum_1_a", "checksum_1_b", "checksum_1_c"]; |
There was a problem hiding this comment.
OK as is but in theory we should actually require these to be at least string SHA256 digests.
2 participants
This fixes the bug where the specific layers were looking at the wrong meta object to allocate files to the layer.