AnnotatedCommit::refname panicks on option unwrapping

[Ekleog]

Hello all,

I'm in the process of trying to use git2 to create a worktree pointing to the merge-base of two references.

I figured out how to get the merge-base, but this gives me an Oid, and AFAIU I need a Reference to be able to create a worktree pointing to it, because WorktreeAddOptions requires it. So I'm trying to get a Reference back from this Oid that merge_base returned me.

The way I'm currently trying this is:

    let base_oid = repo
        .merge_base(
            base_ref
                .target()
                .context("getting target for base reference")?,
            to_check_ref
                .target()
                .context("getting target for to-check reference")?,
        )
        .context("finding the merge-base of the base branch and the to-check reference")?;
    let base_annotated_commit = repo
        .find_annotated_commit(base_oid)
        .context("creating an annotated commit for the merge-base")?;
    let base_refname = base_annotated_commit
        .refname()
        .context("retrieving refname from annotated commit for the merge-base")?;
    let base_ref = repo
        .resolve_reference_from_short_name(base_refname)
        .context("retrieving reference for the merge-base")?
        .resolve()
        .context("resolving merge-base reference")?;

However, this panics at the base_annotated_commit.refname() step, with the following backtrace:

thread 'main' panicked at 'called `Option::unwrap()` on a `None` value', /home/ekleog/.cargo/registry/src/github.com-1ecc6299db9ec823/git2-0.13.17/src/merge.rs:40:84
stack backtrace:
   0: rust_begin_unwind
   1: core::panicking::panic_fmt
   2: core::panicking::panic
   3: core::option::Option<T>::unwrap
             at /build/rustc-1.49.0-src/library/core/src/option.rs:386:21
   4: git2::merge::AnnotatedCommit::refname_bytes
             at /home/ekleog/.cargo/registry/src/github.com-1ecc6299db9ec823/git2-0.13.17/src/merge.rs:40:18
   5: git2::merge::AnnotatedCommit::refname
             at /home/ekleog/.cargo/registry/src/github.com-1ecc6299db9ec823/git2-0.13.17/src/merge.rs:35:24
   6: nixpkgs_check::run
             at ./src/main.rs:80:24
   7: nixpkgs_check::main
             at ./src/main.rs:142:11
   8: core::ops::function::FnOnce::call_once
             at /build/rustc-1.49.0-src/library/core/src/ops/function.rs:227:5
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.

The full reproducer is at https://github.com/Ekleog/nixpkgs-check/tree/libgit2-repro ; it can be reproduced by doing eg. cargo run -- HEAD main, that will get the merge-base between HEAD and main, and then try to check it out in a worktree created by tempdir, without success.

What do you think about this issue? Also, do you know of a way to do what I'm trying to do?

Anyway, thank you for git2, it looks quite neat to use from my documentation-side look at it, even though for now my attempt was cut short :)

[ethomson]

Pretty sure your this is None:

    let base_refname = base_annotated_commit
        .refname()

because you're generating an annotated commit from a sha. You're not generating an annotated commit from a reference. So it doesn't have a reference name.

Generating an annotated commit doesn't mean that you look up a commit in some way, and then we intuit all the other things that it might be called. Ie, you don't create an annotated commit by ID and then libgit2 figures out that it has this tag, and it's the head of this branch, and give you all that information.

An annotated commit is a commit with the metadata about how you looked it up. It allows you to tell us where this commit came from. In merge, for example, we write commit messages differently based on whether you want to merge a branch or a tag or a commit by ID. So you need to tell us which of those you're trying to merge.

In git2-rs, this system is a bit of a leaky abstraction over the fact that libgit2 is written in C. If libgit2 were to have bene built in rust, then merge would take things with a Mergeable trait, that way you wouldn't have to create an annotated commit at all. But it wasn't, so you create an annotated commit to pass to merge; this is a wrapper to provide some sort of polymorphism in C.

But anyway, rust is panic'ing trying to unwrap your None, which is expected. You didn't create an annotated commit from a ref, so you don't have a refname, so it's returning None, which is also expected.

---

Having said that, I don't think I understand what you're trying to do:

I'm in the process of trying to use git2 to create a worktree pointing to the merge-base of two references.

If you want to check out the merge base of two references into the worktree, then you just want to call checkout the OID of the merge base. You don't need annotated commits here at all.

[Ekleog]

Hmm… then, maybe fn refname_bytes() could return an Option<&[u8]> instead of a &[u8], so that it doesn't do an unwrap internally? And in the meantime, maybe document that refname / refname_bytes will panic when being used on an AnnotatedCommit that wasn't searched from a refname?

All I can say is I've got no idea of the internals of git2 or libgit2, so I only have the documentation to figure out how to do things, and this was the best I found and documentation didn't tell me off :sweat_smile:

---

Hmm… when I look at https://docs.rs/git2/0.13.17/git2/struct.WorktreeAddOptions.html#method.reference, it looks to me like I need a Reference to be able to directly check out a commit?

Right now what i'm doing is creating a worktree and then using checkout_tree, but IIUC it means that I'm checking out two commits, and so things take twice as long? (worktree checkout is currently a non-negligible time of my tool's runtime) And also checkout_tree doesn't actually get a checkout and I'm not really sure how to do that but that's a much lesser deal for my use case anyway :)

[alexcrichton]

Thanks for the background @ethomson! I think it's also safe to say that many of the bindings in this crate are naively done by me and I don't fully understand git and all its internals as well!

In any case though I would agree @Ekleog that the fix here is to not unwrap what's actually optional from libgit2, instead we should expose things as Option from Rust where possible. I think I often don't know when a pointer is nullable or when it isn't coming from C and bind the API mistakenly.

[ethomson]

I think I often don't know when a pointer is nullable or when it isn't coming from C and bind the API mistakenly.

This feels like a place where we should be giving you better documentation in libgit2. 😢 (Especially since "annotated commits" are one of the few concepts unique to libgit2 that don't really exist in git.)