Skip to content

Adding Pre-Push Callback Support - #1061

Merged
nulltoken merged 1 commit into
libgit2:vNextfrom
whoisj:pre-push-hook-support
Jun 10, 2015
Merged

Adding Pre-Push Callback Support#1061
nulltoken merged 1 commit into
libgit2:vNextfrom
whoisj:pre-push-hook-support

Conversation

@whoisj

@whoisj whoisj commented May 29, 2015

Copy link
Copy Markdown

added PushUpdate class, updated GitPushUpdate struct

completed plumbing from libgit2 => API layer

added a positive test, failed to get a negative test to work correctly

@whoisj

whoisj commented May 29, 2015

Copy link
Copy Markdown
Author

Currently return anything except 0 from the hook will cause an exception to be thrown in the managed layer. I'm not sure this is what we want to be seeing. Perhaps it is. Having a hook block a push for any reason might not be considered exceptional. I'm looking for feedback on this.

Once we have the answer to the above, I'll complete the negative test case to insure than returning a failing value blocks the upload portion of the push.

Comment thread LibGit2Sharp/PushOptions.cs Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Negotiation is misspelled 😄

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

As a senior member of the American Bad Spellers Society Id like to thank you for recognition of my (in)abilities. 😆

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@whoisj FWIW, most of the spell checking tools delegate the hard work to @jamill

@jamill

jamill commented Jun 1, 2015

Copy link
Copy Markdown
Member

Having a hook block a push for any reason might not be considered exceptional. I'm looking for feedback on this.

I think it is reasonable for an exception generated in the push handler to block the push. If someone is relying on the hook to approve changes, then I would like for an error to block the push.

@jamill

jamill commented Jun 1, 2015

Copy link
Copy Markdown
Member

@whoisj Thanks for the PR 🎆 ! I think this will be nice addition. I had some feedback and weighed in on your question.

@whoisj

whoisj commented Jun 1, 2015

Copy link
Copy Markdown
Author

I believe the PR is all fixed up based on the feedback so far.

I found a handy Assert.Throws<UserCancelledException> utility function that cleared the way for a successful negative unit test.

@whoisj whoisj closed this Jun 1, 2015
@whoisj whoisj reopened this Jun 1, 2015
@whoisj

whoisj commented Jun 1, 2015

Copy link
Copy Markdown
Author

darn github close and comment button. It's larger than the comment button and there's no confirmation dialog. I think I've close and reopened every PR I've started accidentally at this point 😖

@whoisj whoisj changed the title Adding Support for Pre-Push Hooks Adding Pre-Push Callback Support Jun 1, 2015
@whoisj

whoisj commented Jun 3, 2015

Copy link
Copy Markdown
Author

I believe everything is hunky-dory

/CC @nulltoken @jamill

Comment thread LibGit2Sharp.Tests/PushFixture.cs Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

received?

@whoisj

whoisj commented Jun 5, 2015

Copy link
Copy Markdown
Author

@nulltoken spelling errors corrected in the test code 😜

@nulltoken

Copy link
Copy Markdown
Member

🆒 Before we merge this in, could you please sprinkle a bit of your squashing magic on this PR?

Added `PushUpdate` class, updated `GitPushUpdate` struct

Completed plumbing from libgit2 => API layer

Added both positive and negative tests to the `PushFixture`
@whoisj

whoisj commented Jun 10, 2015

Copy link
Copy Markdown
Author

🆒 Before we merge this in, could you please sprinkle a bit of your squashing magic on this PR?

Done sir, all squashed up. 😇

nulltoken added a commit that referenced this pull request Jun 10, 2015
@nulltoken
nulltoken merged commit a4f7831 into libgit2:vNext Jun 10, 2015
@nulltoken

Copy link
Copy Markdown
Member

💥 ‼️

@nulltoken nulltoken added this to the v0.22 milestone Jun 10, 2015
@whoisj
whoisj deleted the pre-push-hook-support branch June 10, 2015 17:18
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.

5 participants