2

I'm convinced there is a way to handle this 'cleanly', I am just not quite figuring it out.

use git2::Repository;

// Prints out the current branch and sha if it exists.
fn report_repo() -> () {
    Repository::open(".")
        .ok()
        .and_then(branch_and_sha)
        .and_then(|branch_sha| => {  // Fails here with E0061
            let (branch, sha) = branch_sha;
            println!("Branch={} sha={}", branch, sha);
            None
        });
}

fn branch_and_sha(repo: Repository) -> Option<(String, String)> {
    match repo.head().ok() {
        Some(reference) => {
            match (reference.name(),  reference.target()){
                (Some(branch), Some(sha)) => Some((branch.to_string(), sha.to_string())),
                _ => None
            }
        },
        None => None
    }
} 

The error that arises is E0061, and I think it's because the 'value' in the Option returned from branch_and_sha() is a tuple. branch_and_sha() effectively says, "If there is a repository, get it's reference, and if that exists, if it has both a name (branch) and target (sha), return an Option<(String, String)> with that info - otherwise return None. And the reporting function wants to do something if all of the Repository, branch and sha can be found - and nothing otherwise. (It shouldn't error or panic.)

To some degree this is contrived - it's an example of an optimistic reporting function similar to several I'd like to write. I'm looking for a clean, idiomatic way to do it. The key thrust is 'several depths and several branches could return None which should cause a no-op, and otherwise make specific (leaf) info available.' The specific error is how I should be handling the and_then function, which is surprisingly difficult to find similar problems about.

Nathaniel Ford
  • 20,545
  • 20
  • 91
  • 102

2 Answers2

4

First off, you have a minor typo. Closures in Rust don't use =>. So your closure should look more like

.and_then(|branch_sha|  {  // Note: No => here
  let (branch, sha) = branch_sha;
  println!("Branch={} sha={}", branch, sha);
  None
});

Then the error we get is

  --> so_cleanly.rs:15:10
   |
15 |         .and_then(|branch_sha| {
   |          ^^^^^^^^ cannot infer type for type parameter `U` declared on the associated function `and_then`
   |

and_then is declared with two generic arguments: U and F (technically, there's also T, but that's determined by the type of the receiver self, so we won't worry about it). Now, F is the type of the closure and is always determined by the argument. On the other hand, U is the return type of the closure.

The closure must return an Option<U>. Rust needs to look at the closure and determine what its return type is. What does the closure return? It returns None, and None can be Option<U> for any U in existence. Rust doesn't know which one to use. We need to tell it. We could do that on the line we return None from

None as Option<()>

or in the and_then call itself.

.and_then::<(), _>(|branch_sha| { ... })

However, the compiler is making a very valid point. and_then and company produce a result of type Option, which you're ignoring. You're writing a piece of code which has side effects and doesn't produce a value, which is sensible, but you're using a functional interface intended for returning values. It can be done, but it's probably not idiomatic. I had to look at your code a few times before realizing that the () return value was not a typo.

One option is to return Option<()> from your report_repo. The () on the inside indicates that we don't care about anything except the side effects, and the Option lets the caller of report_repo handle (or ignore) any errors that occur during the process, whereas your current function simply suppresses all errors unconditionally.

fn report_repo() -> Option<()> {
  Repository::open(".")
    .ok()
    .and_then(branch_and_sha)
    .map(|branch_sha| {
      let (branch, sha) = branch_sha;
      println!("Branch={} sha={}", branch, sha);
      // Implicit return of () here, which we could do explicitly if we wanted
    })
}

I've made several subtle changes here. The return type is Option<()> now. In accordance with that, there's no semicolon at the end of the line inside the function (we're returning that value). Finally, the last and_then is a map, since the final step can't fail and simply does some work on Some.

That's an improvement, but it's probably still not how I'd write this function.

Instead, if you're performing code for side effects, consider using the ? operator, which does and_then and map shenanigans but keeps the control flow relatively linear. and_then and its friends are great for constructing values, but the point of your function is that it should read like a sequence of instructions, not a constructor for a value. This is how I would write that function.

fn report_repo() -> Option<()> {
  let repo = Repository::open(".").ok()?;
  let (branch, sha) = branch_and_sha(repo)?;
  println!("Branch={} sha={}", branch, sha);
  Some(())
}

Each line that ends in a ? effectively says "If this thing is None, return None now. Otherwise, keep going." But a cursory glance of the code reads "open the repo, branch and sha, and then print", which is exactly what you want people to see at a glance.

If we wanted to be really proper about this, we should probably return Result<(), Error>, where Error is some more detailed error type, but that's overkill for this simple example snippet.

Silvio Mayolo
  • 62,821
  • 6
  • 74
  • 116
  • Thank you for the thorough explanation! The typo definitely misdirected me (I had been trying to use a match statement and must have left it in). The type signature is where I was getting hung up. – Nathaniel Ford Aug 11 '21 at 17:39
2

You can chose an if let style too, you do not need the option value so just stop using them at some point it feels more comfortable:

fn report_repo() {
    if let Some((branch, sha)) = Repository::open(".").ok().and_then(branch_and_sha) {
        println!("Branch={} sha={}", branch, sha);
    }
}
Netwave
  • 40,134
  • 6
  • 50
  • 93
  • I had not really internalized the if let construct... thanks for pointing this out! I think it's definitely a more concise option than the matching I'd been doing in, say, the branch_and_sha function. – Nathaniel Ford Aug 11 '21 at 17:43