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

Added Two Reset method Overloads. #6

Merged
merged 1 commit into from
Jun 22, 2016
Merged

Added Two Reset method Overloads. #6

merged 1 commit into from
Jun 22, 2016

Conversation

DavidBrower
Copy link

Hi Guys,
I've forked Cake.Git and added two reset overloads that call into the corresponding libgit2sharp methods. The reason for making this addition is that we want to be able to roll back the updates made by GitVersion to our AssemblyInfo.cs files at the end of a build.

Ideally, we'd like to be able to specify a pattern to target */AssemblyInfo.cs and no other files since there may be build artifacts that we wish to retain (and for which .gitignore is too brittle a protection). However, I see that libgit2sharp doesn't offer calling into the git_reset_default method in libgit2.

Summary of Additions

GitReset(ResetMode) which resets the current branch HEAD to the latest
commit and, optionally, modifies the index and working tree.

GitReset(ResetMode, CommitId) which resets the current branch HEAD to the
specified commit and, optionally, modifies the index and working tree.

@devlead
Copy link
Member

devlead commented Jun 17, 2016

@DavidBrower please add a test case for these new aliases in test.cake and I'll try to review asap.

@DavidBrower
Copy link
Author

@devlead Apologies for not having mentioned this in my OP. I'm afraid I am unable to run the tests in test.cake due to an issue with running git clone using lilbgit2sharp from behind a (corporate) proxy:

libgit2/libgit2#2106

I will see if there is a way around this to add a test case.

@devlead
Copy link
Member

devlead commented Jun 17, 2016

Currently it runs target Default-Tests but you could add an Local-Tests target that executes the tests that work locally and specify that target when executing test.cake.

Refactor would basically be add Local-Tests task, move all locally working tests from Default-Tests to dependencies for Local-Tests, finally make Default-Tests dependent on Local-Tests.

To be able to run all you could add an test-target argument to build.cake, which specifies the target test.cake gets invoked with from build.cake test task.

Regardless you could add tests for your added aliases and invoke test.cake with target argument to exercise just those.

@DavidBrower
Copy link
Author

DavidBrower commented Jun 20, 2016

I've almost completed the refactoring along the lines that you have suggested. However, I've noticed that Git-Set-Tag is failing due it not finding the specified file. It's difficult understanding what is wrong here as it seems to want to shell out to an exe that I don't have at that location (possibly as a consequence of Git-Clone not being executed).

Any thoughts?


///////////////////////////////////////////////////////////////////////////////
// ARGUMENTS
///////////////////////////////////////////////////////////////////////////////
var target = Argument<string>("target", "Default-Tests");
var localTests = Argument<bool>("local-tests", false);
Copy link
Member

Choose a reason for hiding this comment

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

No need for this people can specify target to Local-Tests

Copy link
Author

Choose a reason for hiding this comment

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

Ok, will amend.

Copy link
Author

Choose a reason for hiding this comment

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

So, do you want to be able to call the PS script like this:

.\build.ps1 -Target Local-Tests?

@devlead
Copy link
Member

devlead commented Jun 20, 2016

Something's broken in the Git-Clone maybe some ordering issue, haven't checked details yet, just saw CI failed.

@DavidBrower
Copy link
Author

DavidBrower commented Jun 20, 2016

@devlead I see that it's failing with the message Failed to find parentFullDirectoryPath. I'll check what I've done that could have caused this.

Update

Yup, it's an ordering issue. Hopefully, will get a fix up soon.

@DavidBrower
Copy link
Author

DavidBrower commented Jun 20, 2016

I am concerned that Git-Set-Tag keeps failing on my machine. Is it meant to be shelling out to an installation of Git?

@devlead
Copy link
Member

devlead commented Jun 20, 2016

@DavidBrower Currently Git-Set-Tag requires git command line in path preferably it would've been replaced by an native alias in the addin, isn't very complicated
https://github.com/libgit2/libgit2sharp/wiki/git-tag
But that's out of scope for this PR.

I'll review at PR in a couple of hours, meanwhile could you please rebase/squash into 1 commit.

@@ -36,6 +36,7 @@ Param(
[switch]$WhatIf
)


Copy link
Member

Choose a reason for hiding this comment

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

This file should be reverted to original state as it hasn't changed.

Copy link
Author

Choose a reason for hiding this comment

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

I missed this for some reason. Will rectify.

GitReset(ResetMode) which resets the current branch HEAD to the latest
commit and, optionally, modifies the index and working tree.

GitReset(ResetMode, CommitId) which resets the current branch HEAD to the
specified commit and, optionally, modifies the index and working tree.

$Target can be passed a value of Local-Tests which runs all tests except
Git-Clone. The purpose of this is to allow for the situation where
someone is behind a corporate proxy and the test repository on GitHub
cannot be cloned (see libgit2/libgit2#2106).
@DavidBrower
Copy link
Author

@devlead Is there anything else that you can think needs to be added to the PR?

@devlead devlead merged commit a39d113 into cake-contrib:master Jun 22, 2016
@devlead
Copy link
Member

devlead commented Jun 22, 2016

@DavidBrower LGTM, merged 👍

@devlead
Copy link
Member

devlead commented Jun 22, 2016

@DavidBrower it's now on Nuget
https://www.nuget.org/packages/Cake.Git/0.4.0

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.

None yet

2 participants