Listing commits for an empty repository throws a null reference exception

[davidfowl]

Repro steps:

Create a new git repository:

mkdir folder
cd folder
git init

Create a repository instance and try to list commits.

var repo = new Repository(@"C:\dev\folder\.git");
foreach (var c in repo.Commits) {
    Console.WriteLine(c.Id);
}

Expected:

Empty commit collection

Actual:

Null reference exception

I actually dug a little deeper into why it throws. It seems that this line throws (CommitCollection.StartingAt):

return new CommitCollection(repo) { sortOptions = sortOptions, pushedSha =   reference.ResolveToDirectReference().Target.Sha };

ResolveToDirectReference() returns null since the reference is the head which doesn't point to any commits by default.

[nulltoken]

Hey David,

Thanks a lot for spotting this. This is a nice catch!

This NullReferenceException has been fixed in the vNext branch. One can expect an official NuGet package within the next few days.

However, an empty commit collection is not returned (yet?). Another exception is raised (Dammit!). This exception will state that "No valid git object pointed at by 'HEAD' exists in the repository."

Some work has been pushed in the vNext branch which allow to define the top boundary of a CommitCollection by a canonical reference name or a sha.

Considering an empty repository, current implementation throws when

• repo.Commits
• repo.Commits.StartingAt("HEAD")
• repo.Commits.StartingAt("refs/heads/master")
• repo.Commits.StartingAt("refs/heads/i-do-not-exist")

Considering a repository containing commits, current implementation throws when

• repo.Commits.StartingAt("refs/heads/i-do-not-exist")
• repo.Commits.StartingAt("refs/heads/i-exists-but-point-to-a-unknown-sha")
• repo.Commits.StartingAt("deadbeefdeadbeefdeadbeefdeadbeefdeadbeef")

This makes the CommitCollection more or less in sync with git log behavior, while throwing in very specific cases.

What's your opinion about this?

[davidfowl]

It logically makes sense to sync with git log behavior but it does feel wrong to throw when trying to list commits for an empty repository. If I'm trying to build UI that shows commits I'd need to have logic that checked if the repo is empty before attempting to list commits, which feels like a fail.

[nulltoken]

which feels like a fail.

I do agree. :) This is why I took the time to list all the use cases we could think about.

According to you which one(s) shouldn't throw?

[davidfowl]

Considering an empty repository:

repo.Commits -> Don't throw
repo.Commits.StartingAt("HEAD")  -> Don't throw
repo.Commits.StartingAt("refs/heads/master")  -> Don't throw
repo.Commits.StartingAt("refs/heads/i-do-not-exist") -> throw 

Is that reasonable?

[nulltoken]

Is that reasonable?

Indeed, it is. But, I'm biased ;-)

@tclem, how does this feel to you?

[tclem]

I always go back to the idea that an exception should be thrown only for truly exceptional behavior. In that vein, I'd like an empty repo to behave like any other repo as much as possible.

For an empty repo (you just ran git init):

repo.Commits // dont throw. return empty list.
repo.Commits.StartingAt("HEAD")  // dont throw. HEAD pointer always exists. 

// this one is more complicated...
// If the branch actually exists, dont throw and return an empty list.
// However, if you don't have or have deleted your master branch, then I think this should throw.
// i.e. if refs/heads/master does not exist, then this behaves the same as ref/heads/i-do-not-exist
repo.Commits.StartingAt("refs/heads/master")

repo.Commits.StartingAt("refs/heads/i-do-not-exist") // throw
repo.Commits.StartingAt("deadbeefdeadbeefdeadbeefdeadbeefdeadbeef") // throw

[nulltoken]

Ok. I think we all agree, there. :)

However, here's the trick. There is no "refs/heads/master" in an empty repository. Only a symbolic "HEAD" which points to a non existent "refs/heads/master". So we can't rely on a generic rule.. and we'll have to hard code something like this

if ((repo.Info.IsEmpty) and (shaOrReferenceName == "HEAD" || shaOrReferenceName == "refs/heads/master"))
{
    return new EmptyCommitCollection();
}

This basically means that the "If you don't have or have deleted your master branch, then this should throw." rule can not be applied to an empty repo (it will however throw against a repo with commits)..

If this is OK with everyone, I'll implement this.

[tclem]

👍

[davidfowl]

Looks good

[nulltoken]

@davidfowl: Fixed, merged and published (http://nuget.org/List/Packages/LibGit2Sharp)

Thanks for the report!