[solved] Git2-rs: to much level of code indention

help
1

Hi I wrote code which deletes branches on submodules of a git-repository. My code is functional but uses 13 level of indention to delete the branches. Can I write this simpler, nicer or better?

pub fn forget(y: &clap::ArgMatches) {
    let branchname = y.value_of("branchname").unwrap();

    match Repository::discover(".") {
        Ok(repo) => {
            /* remove branch on the current repository */
            match repo.branches(Some(BranchType::Local)) {
                Ok(branches) => {
                    for b in branches {
                        match b {
                            Ok((mut b, _)) => {
                                if b.name().unwrap().unwrap() == branchname {
                                    println!("delete {} in current folder", branchname);
                                    b.delete();
                                }
                            },
                            Err(e) => { println!("fila ({})", e); return; },
                        }
                    }
                },
                Err(e) => { println!("fila ({})", e); return; },
            }

            /* remove branch on all submodules */
            match repo.submodules() {
                Ok(submodules) => {
                    for sm in submodules.iter() {
                        match sm.open() {
                            Ok(reposub) => {
                                match reposub.branches(Some(BranchType::Local)) {
                                    Ok(branches) => {
                                        for b in branches {
                                            match b {
                                                Ok((mut b, _)) => {
                                                    if b.name().unwrap().unwrap() == branchname {
                                                        println!("delete {} in {}", branchname, sm.name().unwrap());
                                                        b.delete();
                                                    }
                                                },
                                                Err(e) => { println!("fila ({})", e); return; },
                                            }
                                        }
                                    },
                                    Err(e) => { println!("fila ({})", e); return; },
                                }
                            },
                            Err(e) => { println!("fila ({})", e); return; },
                        }
                    }
                },
                Err(e) => { println!("fila ({})", e); return; },
            }
        },
        Err(e) => { println!("fila ({})", e); return; },
    }
}
2

You can use match as an expression to keep the success cases at the same level: (similar to this example from the book)

let submodules = match repo.submodules() {
    Ok(submodules) => submodules,
    Err(e) => { println!("fila ({})", e); return; },
};
for sm in &submodules {
    let reposub = match sm.open() {
        Ok(reposub) => reposub,
        Err(e) => { println!("fila ({})", e); return; },
    }
    ...

You can make it even simpler with the standard try! macro:

let submodules = try!(repo.submodules());
for sm in &submodules {
    let reposub = try!(sm.open());
    ...

But this changes the behavior — try! does a return in the failure case, so you would need forget to return Result<(), ...something specific to libgit2...> and have a match in whatever function is calling forget.

1 Like
3

Thank you @sorear! I have changed my code and saved 20 lines with your suggestions. The fn forget has now a return type and I was able to use try!.

pub fn forget(y: &clap::ArgMatches) -> Result<(), Error> {
    let branchname = y.value_of("branchname").unwrap();

    let repo = try!(Repository::discover("."));

    /* remove branch on the current repository */
    let mut branch = try!(get_branch(branchname, &repo));
    print!("delete {} in current folder ... ", branchname);
    if branch.delete().is_err() {
        println!("fail");
    } else {
        println!("done");
    }

    /* remove branch on all submodules */
    if y.is_present("recursive") {
        let submodules = try!(repo.submodules());
        for sm in &submodules {
            let reposub = try!(sm.open());
            let mut branch = try!(get_branch(branchname, &reposub));
            print!("delete {} in {} ... ", branchname, sm.name().unwrap());
            if branch.delete().is_err() {
                println!("fail");
            } else {
                println!("done");
            }
        }
    }
    Ok(())
}

fn get_branch<'repo>(branchname: &str, repo: &'repo Repository) -> Result<Branch<'repo>, Error> {
    for possible_branch in try!(repo.branches(Some(BranchType::Local))) {
        let mut branch = match possible_branch {
            Ok((b, _)) => b,
            Err(_) => continue,
        };

        if branch.name().unwrap().unwrap() == branchname {
            return Ok(branch)
        }
    }
    Err(Error::from_str("Branch not found"))
}

Exists some more room for improvements here?

4
  1. The 7-line sequence after let mut branch ... appears twice, you could make it a function.
  2. Iterators like repo.branches which return with a Result usually don't return anything useful after the first error, so you can replace the match possible_branch with a try!.
  3. The loop in get_branch can be replaced with a call to the overloaded iterator Iterator::collect, followed by an Iterator::find and Option::ok_or; YMMV whether this actually improves readability.
  4. Actually, you probably don't need get_branch at all because there's already a find_branch method which sounds like it does the right thing. (I have never used git2-rs.)
1 Like
5

I cannot like your answer two times ^^. With git2::Branch::find_branch I don't need the get_branch function.