Repository::commit should accept parents as &[Commit], not just &[&Commit]

[joshtriplett]

If you want to generate a list of commits to pass to .commit as parents, the type &[&Commit] proves inconvenient, because it requires the caller to maintain ownership of the Commit objects for long enough to pass them. For instance, this could require generating a Vec<Commit>, and then an additional Vec<&Commit>.

Could Repository::commit accept a &[T] where T=AsRef<Commit>, instead? That would allow both possibilities. It does, however, have the disadvantage that passing &[] will result in an ambiguous type, though. Another possibility would be always accepting &[Commit], but that would break existing code that passes &[&Commit].

I'd welcome other suggestions that would simplify code like this:

    let mut parents : Vec<git2::Commit> = Vec::new();
    parents.extend(tree.iter().filter_map(|e| {
        if e.kind() == Some(git2::ObjectType::Commit) {
            Some(repo.find_commit(e.id()).unwrap_or_else(die))
        } else {
            None
        }
    }));
    let new_commit_oid = repo.commit(Some("SHEAD"), &author, &committer, &msg, &tree, &parents).unwrap_or_else(die);

[alexcrichton]

Yeah I wonder if this could actually look like:

pub fn commit<I>(&self, ..., parents: I)
    where I: IntoIterator,
          I::Item: AsRef<Commit>

I think that'd be backwards compatible? I'll probably do a new major release soon anyway due to IndexEntry so strict compat may not matter too much.

[joshtriplett]

@alexcrichton That's not quite backward compatible, because it wouldn't know what type to use if the caller passes &[]. It's really unfortunate to have to write &[] as &[Commit]. Hopefully, when default type parameter fallback gets stabilized, that'll help simplify cases like this.

[alexcrichton]

Gah right, that seems to keep biting this...

[pwaller]

Just hit this. Rust newbie here. My use case was to create a commit based on another commit. Surprised to find I couldn't:

repo.commit(..., old_commit.parents())

I tried:

&*c.parents().collect::<Vec<_>>().into_boxed_slice(),

To discover:

   = note: expected reference `&[&git2::Commit<'_>]`
              found reference `&[git2::Commit<'_>]`

... and found my way to this issue :). Not sure whether my into_boxed_slice would have done the right thing had it been able to accept a commit.

Feels quite strange that commit() takes Commits as parents given that it's only going to write the commit ids, but there we go. Would be nice to have access to git_commit_create_from_ids for this.

[chetaldrich]

Just hit this as well. It's not ideal that we'd have to sacrifice backwards compatibility here, but maybe we just need to rip off the bandage. I'd be willing to push out a PR to address this if we're willing to up the version.

[extrawurst]

what we do usually is add a new method with the required signature and then we mark the old one to be removed once we do a major version bump which allows us to use the freed up name

[chetaldrich]

Ok, I can give that a go, sounds simple enough.

[chetaldrich]

Took a minute (or a few) to find some time for this, but I've given it a try. If any of the folks here would be willing to review I'd appreciate it. This is my first PR in this project, so let me know if I've missed anything.