Skip to content

Support only_follow_first_parent #1190

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Sep 21, 2015

Conversation

ninjeff
Copy link
Contributor

@ninjeff ninjeff commented Sep 17, 2015

My team has been using git describe --first-parent to find the most recent tag on the current branch, allowing us to mine the following commits on that branch for issue IDs in our issue tracker. We recently switched to libgit2sharp, but discovered that the equivalent option from libgit2 (only_follow_first_parent) is not supported in libgit2sharp - false is always passed for this struct field.

This pull request adds an OnlyFollowFirstParent property to DescribeOptions and a corresponding test.

I've split the changes across three commits (adding the property, adding a failing test that uses the property, and adding the implementation) to support verifying that the test exercises the implementation. If this is excessive then let me know and I'll squash the changes into one commit.

@ninjeff
Copy link
Contributor Author

ninjeff commented Sep 17, 2015

CI builds are failing because I didn't configure a user for the test, so it can't sign commits; will fix and update.

@ninjeff ninjeff force-pushed the ninjeff/describe-offp branch from 8ccbca4 to f29f226 Compare September 17, 2015 08:28
{
string path = SandboxStandardTestRepo();
string configPath = CreateConfigurationWithDummyUser(Constants.Identity);
var options = new RepositoryOptions { GlobalConfigurationLocation = configPath };
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You shouldn't need the two lines above

@nulltoken
Copy link
Member

Very clean PR ‼️

I've split the changes across three commits (adding the property, adding a failing test that uses the property, and adding the implementation) to support verifying that the test exercises the implementation. If this is excessive then let me know and I'll squash the changes into one commit.

Thanks a lot for asking ❤️ Indeed, we tend to prefer cohesive (tests + new feature) commits. Once we agree on the content of the PR, before the merge, I'll eventually request you to squash them together.

@ninjeff
Copy link
Contributor Author

ninjeff commented Sep 18, 2015

Thanks for the review, @nulltoken!

I hadn't spotted Constants.Signature but I agree that that's a better way to get a signature. Will incorporate this change on Monday when I'm at the customer site where I need this.

Regarding the MinimumCommitIdAbbreviatedSize thing: I wanted to ensure that the commit IDs, which change between test runs, don't cause the test to fail. Would you prefer a different approach, like comparing the describe output up to the second-last -?

@nulltoken
Copy link
Member

@ninjeff Using Constants.Signature should allow you to have stable commit shas.

@ninjeff ninjeff force-pushed the ninjeff/describe-offp branch from f29f226 to bd2f790 Compare September 21, 2015 02:58
@ninjeff
Copy link
Contributor Author

ninjeff commented Sep 21, 2015

@nulltoken Pull request updated with stable commit IDs as discussed. Should I go ahead with squashing the commits?

@nulltoken
Copy link
Member

@nulltoken Pull request updated with stable commit IDs as discussed. Should I go ahead with squashing the commits?

👍

The GitDescribeOptions struct used by git_describe_commit has an OnlyFollowFirstParent field, which corresponds to the --first-parent option in command-line Git.

This commit adds the same option as a new property on the DescribeOptions class, with XML documentation from Git docs.
@ninjeff ninjeff force-pushed the ninjeff/describe-offp branch from bd2f790 to 758f428 Compare September 21, 2015 06:56
@ninjeff
Copy link
Contributor Author

ninjeff commented Sep 21, 2015

Done

nulltoken added a commit that referenced this pull request Sep 21, 2015
@nulltoken nulltoken merged commit bd4e4c9 into libgit2:vNext Sep 21, 2015
@nulltoken
Copy link
Member

🎉

@nulltoken nulltoken added this to the v0.22 milestone Sep 21, 2015
@nulltoken
Copy link
Member

@ninjeff Awesome contribution. Thanks! ❤️

@ninjeff
Copy link
Contributor Author

ninjeff commented Sep 21, 2015

@nulltoken Thanks for merging! Looking forward to seeing it in a release 🚢

@ninjeff ninjeff deleted the ninjeff/describe-offp branch September 21, 2015 09:27
@nulltoken
Copy link
Member

@nulltoken Thanks for merging! Looking forward to seeing it in a release 🚢

@ninjeff 🚢d‼️ Thanks for the cool addition ❤️

Published as NuGet pre-release package LibGit2Sharp.0.22.0-pre20151003111314

@ninjeff
Copy link
Contributor Author

ninjeff commented Oct 3, 2015

Good stuff 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants