Generally I agree, but in this particular case mutating the param is a perfect fit, because in my opinion

add_new_attachments(&mut backup.attachment_holders, new_holders);

is cleaner than doing

backup.attachment_holders = add_new_attachments(backup.attachment_holders, new_holders);

Possibly I could rename this function to merge_attachments or sth like that

This was my first thought, but HashSet doesn't have join(), so I'd need to collect it into a Vec first

current_holders.iter().collect::<Vec<_>>().join(ATTACHMENT_HOLDER_SEPARATOR).

My solution is done on purpose, as creating a single string and mutating it each time is more efficient.
But maybe in this case efficiency isn't as important and we can do a tradeoff for readability.

Btw. I noticed I made a small mistake in reserve()

I think the fact that Rust makes you annotate at the callsite makes this much less of an anti-pattern than eg. in C++

Feel like marshalling of the string into a HashSet then an array can be done once we first receive the object, then we can serialize it back into a str before sending it over the wire.

Having a function go from String -> HashSet -> Vec -> String is a smell, in my opinion. Especially if this function may be called many times, it may become a performance concern.

We can do this as future work, as you have a pretty large stack going already.

Yeah, discussed a bit in https://phab.comm.dev/D6205?id=20737#inline-41744

Generally, performance is one of the reasons why I store HashSet<&str> instead of HashSet<String>. Fortunately, this function is called once per request and these strings are not that huge.
Also, my first version avoided the -> Vec step by implementing join() manually (HashSet doesn't implement Join), but finally gave up.