Setters of `MetainfoBuilder` incorrectly works with result of `BTreeMap::insert` method.

[reeFridge]

case 0:

fn print_by_kind(v: &BencodeMut) { 
    /* just recursively parse & prints bencode contents using bip_bencode crate  */
}

fn main() {
    let builder = MetainfoBuilder::new().set_comment(Some("Just Some Comment"));
    // By default root prop is private but we ignore that now
    let bencode_root = builder.root;

    // I directly work with root dictionary
    // cause for build metainfo with build method we need additional info dict.
    print_by_kind(&bencode_root);
}

print_by_kind source run it and see nothing.

case 1:

fn main() {
    let builder = MetainfoBuilder::new()
        .set_comment(Some("any"))
        .set_comment(Some("way"));

    let bencode_root = builder.root;

    print_by_kind(&bencode_root);
}

output is the same.

I spent some time to understand why this is happening and here are the results of my digging:

src/builder/mod.rs

/// Set or unset a comment for the torrent file.
pub fn set_comment(mut self, opt_comment: Option<&'a str>) -> MetainfoBuilder<'a> {
   {

        let dict_access = self.root.dict_mut().unwrap();
        opt_comment
             // Problem here, we always fall into or_else and removing inserted key.
            .and_then(|comment| dict_access.insert(parse::COMMENT_KEY.into(), ben_bytes!(comment)))
            .or_else(|| dict_access.remove(parse::COMMENT_KEY));
    }

    self
}

type of dict_access is BDictAccess<Cow<'a, [u8]>, V>

bip_bencode/src/access/dict.rs

impl<'a, V> BDictAccess<Cow<'a, [u8]>, V> for BTreeMap<Cow<'a, [u8]>, V> {
    fn insert(&mut self, key: Cow<'a, [u8]>, value: V) -> Option<V> {
        self.insert(key, value)
    }
}

Quote from BTreeMap docs:

If the map did not have this key present, None is returned.

If the map did have this key present, the value is updated, and the old value is returned.

now return to the our code of set_comment method:

opt_comment
     // Problem here, we always fall into or_else and removing inserted key.
    .and_then(|comment| 
/* 
insert can return Some if key already exists in map else returns None 
but what about second case from the beginning?
*/
    dict_access.insert())
    .or_else(|| dict_access.remove(parse::COMMENT_KEY));

some tests on BTreeMap:

const COMMENT_KEY: &'static [u8] = b"comment";

fn main() { 
    let mut map = BTreeMap::<Cow<[u8]>, BencodeMut>::new();
    assert_eq!(map.insert(COMMENT_KEY.into(), ben_bytes!("any")), None);
    assert_eq!(map.remove(COMMENT_KEY), Some(ben_bytes!("any")));
    assert_eq!(map.insert(COMMENT_KEY.into(), ben_bytes!("way")), None);
}

But anyway current code in setters of MetainfoBuilder incorrectly working with result of insert method.

Fix it: patch for set_comments

/// Set or unset a comment for the torrent file.
pub fn set_comment(mut self, opt_comment: Option<&'a str>) -> MetainfoBuilder<'a> {
    {
        let dict_access = self.root.dict_mut().unwrap();

        if let Some(comment) = opt_comment {
            dict_access.insert(parse::COMMENT_KEY.into(), ben_bytes!(comment));
        } else {
            dict_access.remove(parse::COMMENT_KEY);
        }
    }

    self
}

[GGist]

@reeFridge Ah interesting, thanks for catching that!

If you want to push a PR, I could accept that, or I can push this when I get some time.

[reeFridge]

@GGist ok, i'll create an PR with fixes for all affected setters.

[reeFridge]

@GGist, PR #144

[reeFridge]

144 fixed this issue, can be closed