Add revwalk globs #184

Closed
wants to merge 6 commits into from

2 participants

@tclem
libgit2 member

This adds in the ability to filter your revwalk by pushing or hiding globs enabling functionality like:

git log --not --remotes or git log --branches

I'm open to suggestions on the filter API. The glob patterns didn't make sense to add in with the rest of the list of stuff that Since and Until know how to deal with because they are never really git objects of any sort. Improving the Filter object gave me the chance to refactor how it was use internally a little bit and pass the filter down a few more layers to get rid of a few ctors with growing parameter lists.

@nulltoken nulltoken and 1 other commented on an outdated diff Jun 19, 2012
LibGit2Sharp.Tests/CommitFixture.cs
@@ -98,7 +98,6 @@ public void QueryingTheCommitHistoryWithUnknownShaOrInvalidEntryPointThrows()
{
Assert.Throws<LibGit2SharpException>(() => repo.Commits.QueryBy(new Filter { Since = Constants.UnknownSha }).Count());
Assert.Throws<LibGit2SharpException>(() => repo.Commits.QueryBy(new Filter { Since = "refs/heads/deadbeef" }).Count());
- Assert.Throws<ArgumentNullException>(() => repo.Commits.QueryBy(new Filter { Since = null }).Count());
@nulltoken
libgit2 member

Why removing this? What should the following statements be expected to return?

  • repo.Commits.QueryBy(new Filter { Since = null })
  • repo.Commits.QueryBy(new Filter { Since = string.Empty })
@tclem
libgit2 member
tclem added a note Jun 21, 2012

Good catch - see aec0563

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@nulltoken
libgit2 member

I like the refactoring proposal which pushes the logic inside the Filter type. This is where it belongs.
However, I'm not a fan of publicly exposing the SinceList|UntilList properties. Could you make them internal?

@nulltoken
libgit2 member

Rather than adding a SinceGlob|UntilGlob property to the Filter type, I I'd prefer adding to the ReferenceCollection type a new IEnumerable<Reference> FromGlob(string pattern) method.

This would allow such construct

[Fact]
public void CanEnumerateCommitsUsingGlob()
{
    AssertEnumerationOfCommits(
        repo => new Filter { Since = repo.Refs.FromGlob("heads") },
        new[]
             {
                 "4c062a6", "e90810b", "6dcf9bf", "a4a7dce", "be3563a", "c47800c", "9fd738e", "4a202b3", "41bc8c6", "5001298", "5b5b025", "8496071"
             });
}
@tclem
libgit2 member

@nulltoken isn't that going to be expensive in terms of having to lookup all the refs that match the glob pattern? Wouldn't you rather have the existing libgit2 apis for pushing and hiding a glob do that?

@nulltoken
libgit2 member

isn't that going to be expensive in terms of having to lookup all the refs that match the glob pattern? Wouldn't you rather have the existing libgit2 apis for pushing and hiding a glob do that?

Well, behind the scene, this is what git_revwalk_push_glob() does. I'd really like to have this unfolded and allow the API consumer to retrieve a list of refs from a glob, which could be eventually used to retrieve a commit log (or used to a different end).

The fast way would be to quickly implement that in C# on top of the current Refs enumerator (porting the libgit2 normalization algorithm and regex love). If you feel performance may become a concern, instead of the Refs enumerator, one could work against the result of Libgit2UnsafeHelper.ListAllReferenceNames() which would avoid the parsing/building of every reference.

Eventually, once this code is proven ok and covered with some tests, it'll be pushed down in libgit2.

@nulltoken nulltoken referenced this pull request in libgit2/libgit2 Jun 22, 2012
Merged

Topic/refs fromglob #785

@nulltoken
libgit2 member

Very nice proposal and awesome refactoring!

Manually merged in vNext.

@nulltoken nulltoken closed this Aug 12, 2012
@tclem
libgit2 member

Awesome! Thanks @nulltoken!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment