Skip to content

Conversation

@thewtex
Copy link
Member

@thewtex thewtex commented Feb 2, 2018

Disable the Gerrit commit message Change-Id addition hook.

Add the .hooks-config.bash script to call additional custom hooks in
ITKSoftwareGuide/Utilities/Hooks.

Add a pre-commit hook that checks the SetupForDevelopment.sh script version.
The helps force updates to the Git configuration on existing systems when
required.

Add a commit-msg hook that checks for the format of the commit message (copied
from ITK).

Add git alias for git review-push. This replaces git gerrit-push.
review-push is a common alias and ensures we do not have change the alias
name if we ever switch review platforms again. gerrit-push give a
deprecation message.

git pr alias is added to easily check out existing pull requests locally.
git pr-clean cleans up the pr checkouts. From:

https://github.com/TeamPorcupine/ProjectPorcupine/wiki/How-to-Test-a-Pull-Request

Utilites/GitSetup/{setup-github,setup-upstream} are added to help configure
origin, which points to the user's forked repository and upstream, which
points to the upstream repository. This follows standard GitHub remote naming
conventions:

https://gist.github.com/Chaser324/ce0505fbed06b947d962

A different name for the forked repository remote name other than origin can
be specified if desired.

@thewtex
Copy link
Member Author

thewtex commented Feb 2, 2018

Addresses #44 and #5

@thewtex thewtex force-pushed the github-client-hooks branch 2 times, most recently from 827c5f2 to b2aef66 Compare February 2, 2018 15:54
# Alias to push the current topic branch to GitHub
git config alias.review-push "!bash Utilities/GitSetup/git-review-push"

git config alias.gerrit-push \
Copy link
Member

Choose a reason for hiding this comment

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

Is this gerrit alias still necessary?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is to help transition for folks with old documentation or habits.

Copy link
Member

Choose a reason for hiding this comment

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

OK.

Copy link
Member

@jhlegarreta jhlegarreta left a comment

Choose a reason for hiding this comment

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

Hey @thewtex this is awesome !! Do you need me to test it locally? If yes, how should I proceed?

@thewtex
Copy link
Member Author

thewtex commented Feb 2, 2018

@jhlegarreta Yes, your help testing would be appreciated!

This should enable testing:

git remote add thewtex https://github.com/thewtex/ITKSoftwareGuide.git
git fetch thewtex
git checkout -b github-client-hooks thewtex/github-client-hooks

After running ./Utilities/SetupForDevelopment.sh, the new git pr alias could be used instead :-).

@thewtex thewtex force-pushed the github-client-hooks branch 2 times, most recently from 8de360c to 5b06e00 Compare February 2, 2018 19:09
@jhlegarreta
Copy link
Member

Thanks @thewtex.
Review:

  • Created a dummy test.txt file to test git review-push:
$ git checkout -b GitHubClientHookTest thewtex/github-client-hooks
$ git add test.txt
$ git commit
$ git gerrit-push
"git gerrit-push" is deprecated. Please use "git review-push" instead.
$ git review-push
Pushing to origin
Counting objects: 17, done.
Delta compression using up to 8 threads.
Compressing objects: 100% (16/16), done.
Writing objects: 100% (17/17), 8.90 KiB | 0 bytes/s, done.
Total 17 (delta 5), reused 0 (delta 0)
remote: Resolving deltas: 100% (5/5), completed with 3 local objects.
To https://github.com/jhlegarreta/ITKSoftwareGuide.git
*       HEAD:refs/heads/GitHubClientHookTest    [new branch]
Done
  • The ITK standard prefix message did show up 😟

  • Got the branch on all my remotes (upstream -InsightSoftwareConsortium-, origin -own fork-, and thewtex) ready to Compare & pull request (deleted it) 👌

  • As for git pr

$ git pr
fatal: Invalid refspec 'refs/pull//head:pr/'

😑

@thewtex
Copy link
Member Author

thewtex commented Feb 4, 2018

Thanks for the review, @jhlegarreta !

The ITK standard prefix message did show up 

I don't follow -- what is the ITK standard prefix?

Got the branch on all my remotes (upstream -InsightSoftwareConsortium-, origin -own fork-, and thewtex) ready to Compare & pull request (deleted it) 

Good.

$ git pr
fatal: Invalid refspec 'refs/pull//head:pr/'

git pr, a great suggestion from @addisonElliott, requires the pull request number as an argument to the command. But, I will extend it so it possibly fetches and presents a list of potential pull requests if a number is not provided.

@jhlegarreta
Copy link
Member

The ITK standard prefix message did show up
I don't follow -- what is the ITK standard prefix?

Sorry, I meant The ITK commit message standard prefix message did not show up:

BUG:    - fix for runtime crash or incorrect result
COMP:   - compiler error or warning fix
DOC:    - documentation change
ENH:    - new functionality
PERF:   - performance improvement
STYLE:  - no logic impact (indentation, comments)
WIP:    - Work In Progress not ready for merge

in Utilities/Hooks/commit-msg

git pr, a great suggestion from @addisonElliott, requires the pull request number as an argument to the command. But, I will extend it so it possibly fetches and presents a list of potential pull requests if a number is not provided.

OK, then I missed the PR number. I've tried with a given PR number:

$ git pr 49
From https://github.com/InsightSoftwareConsortium/ITKSoftwareGuide
 * [new ref]         refs/pull/49/head -> pr/49
Switched to branch 'pr/49'

$ git prepush
* commit 5b06e007ddb146b71e0c1630177ef0b703103145
  Author: Matt McCormick <matt.mccormick@kitware.com>
  Date:   Thu Feb 1 16:37:35 2018 -0500

      ENH: Migrate Gerrit to GitHub hooks

      Disable the Gerrit commit message Change-Id addition hook.

      Add the `.hooks-config.bash` script to call additional custom hooks in
      ITKSoftwareGuide/Utilities/Hooks.

      Add a pre-commit hook that checks the SetupForDevelopment.sh script version.
      The helps force updates to the Git configuration on existing systems when
      required.

      Add a commit-msg hook that checks for the format of the commit message (copied
      from ITK).

      Add git alias for `git review-push`. This replaces `git gerrit-push`.
      `review-push` is a common alias and ensures we do not have change the alias
      name if we ever switch review platforms again. `gerrit-push` give a
      deprecation message.

      `git pr` alias is added to easily check out existing pull requests locally.
      `git pr-clean` cleans up the pr checkouts. From:

        https://github.com/TeamPorcupine/ProjectPorcupine/wiki/How-to-Test-a-Pull-Request

      Utilites/GitSetup/{setup-github,setup-upstream} are added to help configure
      `origin`, which points to the user's forked repository and `upstream`, which
      points to the upstream repository. This follows standard GitHub remote naming
      conventions:

        https://gist.github.com/Chaser324/ce0505fbed06b947d962

      A different name for the forked repository remote name other than `origin` can
      be specified if desired.

   .hooks-config.bash                    |  25 +++++
   Utilities/GitSetup/SetupGitAliases.sh |   4 -
   Utilities/GitSetup/config             |  10 +-
   Utilities/GitSetup/git-review-push    |  97 +++++++++++++++++++
   Utilities/GitSetup/setup-gerrit       | 147 -----------------------------
   Utilities/GitSetup/setup-git-aliases  |  12 +++
   Utilities/GitSetup/setup-github       | 159 ++++++++++++++++++++++++++++++++
   Utilities/GitSetup/setup-upstream     | 104 +++++++++++++++++++++
   Utilities/Hooks/commit-msg            |  72 +++++++++++++++
   Utilities/Hooks/pre-commit            |  44 +++++++++
   Utilities/SetupForDevelopment.sh      |  23 ++++-
   11 files changed, 539 insertions(+), 158 deletions(-)

So it does work 👌

Just to ensure that the system is wise-enough, I did:

$ git pr 50
fatal: Couldn't find remote ref refs/pull/50/head
$ git pr 48
$ git prepush

And nothing is displayed, since #48 has been merged already.

So this is quite nice !

If an argumentless git pr command is added, then the user may not be aware of the git pr {number} command, unless we document it, or add a hint saying that all pr's will be downloaded, and specifying an argument will fetch the specified PR. Shall I add a message?

@thewtex
Copy link
Member Author

thewtex commented Feb 6, 2018

Sorry, I meant The ITK commit message standard prefix message did not show up:
[...]

Ah, good point! I copied the prepare-commit-message hook from ITK, which adds this. We may want to update it later to suggest using the GitHub issue tracker if / when we move to that.

$ git prepush

I added an updated alias for this, too, in the next revision.

I added support to git pr to prompt with a list of open pull requests if no argument is specified. This requires curl and python -- if these are not found, it just displays the usage.

@thewtex thewtex force-pushed the github-client-hooks branch from 5b06e00 to 8703297 Compare February 6, 2018 16:55
Disable the Gerrit commit message Change-Id addition hook.

Add the `.hooks-config.bash` script to call additional custom hooks in
ITKSoftwareGuide/Utilities/Hooks.

Add a pre-commit hook that checks the SetupForDevelopment.sh script version.
The helps force updates to the Git configuration on existing systems when
required.

Add a commit-msg hook that checks for the format of the commit message (copied
from ITK).

Add git alias for `git review-push`. This replaces `git gerrit-push`.
`review-push` is a common alias and ensures we do not have change the alias
name if we ever switch review platforms again. `gerrit-push` give a
deprecation message.

`git pr` alias is added to easily check out existing pull requests locally.
`git pr-clean` cleans up the pr checkouts. From:

  https://github.com/TeamPorcupine/ProjectPorcupine/wiki/How-to-Test-a-Pull-Request

Utilites/GitSetup/{setup-github,setup-upstream} are added to help configure
`origin`, which points to the user's forked repository and `upstream`, which
points to the upstream repository. This follows standard GitHub remote naming
conventions:

  https://gist.github.com/Chaser324/ce0505fbed06b947d962

A different name for the forked repository remote name other than `origin` can
be specified if desired.
@thewtex thewtex force-pushed the github-client-hooks branch from 8703297 to 5669105 Compare February 6, 2018 16:56
@jhlegarreta
Copy link
Member

I checked that:

$ git pr
git pr <pull-request-number>

I'm using the git bash. Some checks:

$ python --version
Python 3.6.0 :: Anaconda 4.3.1 (64-bit)

and

$ curl --version
curl 7.47.1 (x86_64-w64-mingw32) libcurl/7.47.1 OpenSSL/1.0.2g zlib/1.2.8 libidn/1.32 libssh2/1.7.0 librtmp/2.3
Protocols: dict file ftp ftps gopher http https imap imaps ldap ldaps pop3 pop3s rtmp rtsp scp sftp smtp smtps telnet tftp
Features: IDN IPv6 Largefile SSPI Kerberos SPNEGO NTLM SSL libz TLS-SRP

It indeed makes sense to print the usage instead of fetching all PR's as I previously imagined/suggested. So 👌

While in a local branch, I did

$ git pr-clean
Switched to branch 'master'
Your branch is up-to-date with 'origin/master'.

👌

A for the prepare-commit-message hook, proceeded as previously, and the message hints are displayed 👌 , and in case the message does not conform to the prefix, the corresponding message is displayed.

Simply brilliant.

Then, this is ready to be merged. Thanks @thewtex !!

@thewtex
Copy link
Member Author

thewtex commented Feb 7, 2018

Thanks for the testing and feedback, @jhlegarreta !

@thewtex thewtex merged commit d922f6a into InsightSoftwareConsortium:master Feb 7, 2018
@thewtex thewtex deleted the github-client-hooks branch February 7, 2018 18:29
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