[tufaceous-lib] Reject duplicate artifact names #3990
Conversation
tufaceous-lib/src/repository.rs
Outdated
| @@ -276,15 +278,13 @@ impl OmicronRepoEditor { | |||
| artifact.target = filename.clone(); | |||
| } else { | |||
There was a problem hiding this comment.
In the if part of this branch, we're accepting (and replacing) an artifact if we're called with the same name+version+kind. I'm nervous about that - it certainly seems wrong if we're building from a manifest (since we shouldn't have duplicates like that, and the existence of a such a dup probably means we've made a mistake), but maybe it's necessary in general?
There was a problem hiding this comment.
I think this should probably error, yeah. You're right that the existence of a duplicate name-version-kind triple means we've more likely made a mistake.
There was a problem hiding this comment.
Changed to an error in 7898a35
tufaceous-lib/src/repository.rs
Outdated
| target_name.raw() == filename | ||
| && target_name.resolved() == filename | ||
| }) { | ||
| if !self.existing_target_filenames.insert(filename.clone()) { |
There was a problem hiding this comment.
It seems strange to me that we have to do this check ourselves to avoid clobbering another file, even if the metadata (its kind) and contents (including the hash) are different. Are we (ab)using tuf in some way that makes this necessary?
|
I think this is fine if we need to fix something quickly, but I don't think guarding against duplicate artifact names that belong to different kinds is correct.
I am also somewhat confused about the underlying issue. I am going to attempt to understand it a bit better. Footnotes
|
Fixes #3988, albeit in a quick-and-dirty way. I have a couple concerns; will leave them as PR comments below.