Skip to content
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

switch from LibGit2Sharp to Git #256

Merged
merged 1 commit into from
Sep 14, 2019
Merged

switch from LibGit2Sharp to Git #256

merged 1 commit into from
Sep 14, 2019

Conversation

adamralph
Copy link
Owner

@adamralph adamralph commented Aug 13, 2019

fixes #149
closes #269

Breaking changes (which are unlikely to break anyone):

  • Introduces a dependency on Git
  • Drops support for versioning from a Git directory. That is, the .git subdirectory of a working directory, or a bare repo.

@adamralph
Copy link
Owner Author

adamralph commented Aug 13, 2019

@bording it's a little hacky but it seems to work!

I don't think it will work for annotated tags yet. - works now

@adamralph adamralph added this to the 2.0.0 milestone Aug 14, 2019
@adamralph adamralph added the breaking This change could break current consumers label Aug 14, 2019
@adamralph adamralph force-pushed the bye-libgit2sharp branch 3 times, most recently from 3d36602 to bfb32eb Compare August 18, 2019 17:22
@adamralph adamralph changed the title use git.exe instead of LibGit2Sharp switch from LibGit2Sharp to git Aug 18, 2019
@adamralph adamralph force-pushed the bye-libgit2sharp branch 12 times, most recently from e34a869 to 57ca758 Compare August 25, 2019 18:57
@adamralph adamralph force-pushed the bye-libgit2sharp branch 2 times, most recently from aea7a11 to be5b231 Compare September 7, 2019 08:42
@adamralph
Copy link
Owner Author

@alexeyzimarev @bartelink @blairconrad @bording @damianh @enzian @g8rdev @leastprivilege @mauroservienti @Mpdreamz @Selmirrrrr @shaynevanasperen @slang25 @thefringeninja @yreynhout: as prolific MinVer users/contributors, do you see any pitfalls with this?

It runs the following Git commands:

  1. git status --short - check whether the path is contained in a working directory
  2. git log --pretty=format:"%H %P" - get the commit graph
  3. git show-ref --tags --dereference - get the tags

All of these commands have been available since some point during the Git 1.x range, so they will all work in the 2.x range.

Unfortunately, I have to swallow exceptions when running 2 and 3 because I've found no way to determine whether they will succeed or fail before running them. For example:

  • 2 fails in most versions of Git when there are no commits. In some versions, git rev-list --count will return 0 if there are no commits, so I can run that first, but in other versions (e.g. 2.11.0) it will also fail when there are no commits. 🙄
  • 3 fails in most versions of Git when there are no tags. In some versions, git log --tags will return an empty string if there are no commits, so I can run that first, but in other versions (e.g. 2.11.0) it will return the commits regardless. 🙄

However, I believe swallowing exceptions and returning the default version is fine in both cases. For all the versions of Git I tried, git status --short, unsurprisingly, succeeds when the path is contained in a working directory, and fails when it is not. So if that has succeeded, I believe it's reasonable to assume that a failure of command 2 means "no commits" and a failure of command 3 means "no tags".

BTW - performance seems to be unaffected. I ran a local build against master on https:/dotnet/cli, which has ~12,000 commits, and it returned "instantly". Or at least, as fast as the current LibGit2Sharp-based MinVer.

@adamralph adamralph changed the title switch from LibGit2Sharp to git switch from LibGit2Sharp to Git Sep 7, 2019
Copy link

@blairconrad blairconrad left a comment

Choose a reason for hiding this comment

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

Yeah, it's probably fine, right?

@bording
Copy link
Collaborator

bording commented Sep 7, 2019

Overall I think the concept is fine, and there are benefits in moving away from LibGit2Sharp.

This does mean you're taking an external dependency that you don't control, though. Sure, it seems reasonable to assume that git is on the path somewhere, but MinVer is currently self-contained.

@adamralph adamralph force-pushed the bye-libgit2sharp branch 2 times, most recently from ca4deae to 7a4e142 Compare September 7, 2019 19:54
@adamralph
Copy link
Owner Author

This does mean you're taking an external dependency that you don't control, though. Sure, it seems reasonable to assume that git is on the path somewhere, but MinVer is currently self-contained.

Yes, that gave me pause, but I think it's probably OK. If it is a problem, I expect it be shown during the alpha phase. I'll ensure an alpha is tested in all the cloud CI providers I can think of, and in Docker builds such as https:/SQLStreamStore/SQLStreamStore, but I'm optimistic that all those environments will have git in the path. I guess some container images may not have it, and in those circumstances, I guess bootstrapping Git will be required, which I don't think should be too much of a problem. And it's probably an edge case anyway. 🤞

@slang25
Copy link
Contributor

slang25 commented Sep 7, 2019

I like the idea of libgit2sharp, however the reality doesn't live up. It's missing features, and the native binaries are problematic in many environments such as some docker images.

Assuming git in the environment makes sense here, and the implementation looks sounds :)

@damianh
Copy link
Contributor

damianh commented Sep 8, 2019

Wait, this won't work on my perforce repositories??

@damianh
Copy link
Contributor

damianh commented Sep 8, 2019

J/K, fine with this. The project had an hard dependency on git anyway.

@adamralph adamralph force-pushed the bye-libgit2sharp branch 2 times, most recently from 8414fce to 7e7131a Compare September 8, 2019 21:51
@adamralph adamralph force-pushed the bye-libgit2sharp branch 3 times, most recently from d0eaa3d to 68418e2 Compare September 13, 2019 10:34
@adamralph adamralph merged commit c9b3ae9 into master Sep 14, 2019
@adamralph adamralph deleted the bye-libgit2sharp branch September 14, 2019 09:23
@adamralph
Copy link
Owner Author

Released in 2.0.0-alpha.1.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking This change could break current consumers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support shallow clones Segmentation Fault in Alpine 3.8
5 participants