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

Feature: Add commit template support #1713

Merged
merged 14 commits into from Oct 5, 2018

Conversation

Projects
None yet
4 participants
@gauravchl
Contributor

gauravchl commented Oct 3, 2018

Description of the Change

This changes bring the support for commit template file.
It will automatically populate the commit message from template file into commit message box.

Steps to test this feature:

  1. Create a template file (~/.gitmessage.txt) with some default commit message.
  2. Update git config git config commit.template ~/.gitmessage.txt.
  3. Reload the atom, it should load default commit message into commit input box.

More details about commit template is available here.

Alternate Designs

  • Show a button "load message from template" next to "add co-authors"
  • Don't load any commit message by default to input field, load only when user click on load button.
  • If there is a message in commit input field, show another button "save message to template". Clicking on this button should update the commit.template file.

Benefits

Users don't have to save common commit messages elsewhere and copy/paste manually.

Possible Drawbacks

N/A

Applicable Issues

#1469

Metrics

  • Added a new method 'getCommitMessageFromTemplate' to git-shell-out-stratagy
  • A new fixture repo added with commit.template for testing(35 files)

Tests

Added unit tests to verify that

  • It populates the commit message from template
  • Reload the commit message from template after committing
  • It correctly updates the state when switching repos
  • git.getCommitMessageFromTemplate() works
@coveralls

This comment has been minimized.

coveralls commented Oct 3, 2018

Coverage Status

Coverage increased (+0.05%) to 82.002% when pulling e516506 on gauravchl:feature/commit-template into b896401 on atom:master.

@annthurium

hi @gauravchl,

thanks for your contribution to our package! It looks really useful.

I'm not sure what's up with that test failure on windows, but it concerns me that it's a new test that you added. All builds need to be green before merging.

use warnings;
use IPC::Open2;
# An example hook script to integrate Watchman

This comment has been minimized.

@annthurium

annthurium Oct 4, 2018

Contributor

can you help me understand more about what this shell script is doing? bash isn't my strong suit, and I don't understand how this relates to the rest of your pull request.

This comment has been minimized.

@gauravchl

gauravchl Oct 5, 2018

Contributor

Hi @annthurium, This script actually came by default when i initialized the fixture repo. https://github.com/git/git/blob/master/templates/hooks--fsmonitor-watchman.sample
Seems introduced in git 2.16 https://git-scm.com/docs/githooks/2.16.0#_fsmonitor_watchman
Should i remove this script manually? or reinitialize the fixture repo with older git version?

Update: Removed manually: 651af9c

Update 2: Removed all other templates from fixture: e516506

@gauravchl

This comment has been minimized.

Contributor

gauravchl commented Oct 5, 2018

Hi @annthurium,
Thanks for the review! Tests fixed: e516506
It was an issue with testing async operation.

@annthurium

looks great - thanks for addressing my review comments!

I will merge your changes this afternoon 💥

@annthurium annthurium merged commit 01b187f into atom:master Oct 5, 2018

6 checks passed

ci/circleci: beta Your tests passed on CircleCI!
Details
ci/circleci: dev Your tests passed on CircleCI!
Details
ci/circleci: stable Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.05%) to 82.002%
Details
@kuychaco

This comment has been minimized.

Member

kuychaco commented Oct 22, 2018

@gauravchl thanks so much for your contribution! We're preparing the next Atom release and have identified a regression that was introduced by this PR. We're going to go ahead and revert it temporarily in order to move forward with the release. And we'll be sure to get the commit template feature into the next release so that everyone can benefit from your contribution :).

In the meantime, we thank you for your patience and understanding, and we'll see about sending some swag your way!

And note that your commits are still in the repo history even with the revert, so you're still getting credit for the great work you did

If you're curious about the regression, I've included the details below. No need for you to take action though. We've got a fix in progress, just can't get it in before the release.

Repro steps:

  1. Open repo in Atom and note that the commit message template is in the commit box
  2. Merge a branch that introduces conflicts

Expected result: Commit message box shows merge message
Actual result: Commit message box still shows commit message template. This could cause confusion for users and result in non-standard merge commit messages (and therefore potential loss of relevant merge information in commit history)

@gauravchl

This comment has been minimized.

Contributor

gauravchl commented Oct 23, 2018

@kuychaco Thanks for the updates, all good.
Following the conversation at #1752 looks like there is another issue with template feature. Let me know if i can help with anything.
I will give another look and try to test this feature more thoroughly.

Thanks for the swag again :)

@gauravchl gauravchl deleted the gauravchl:feature/commit-template branch Oct 23, 2018

@gauravchl gauravchl restored the gauravchl:feature/commit-template branch Oct 23, 2018

@kuychaco kuychaco referenced this pull request Oct 25, 2018

Merged

Re-introduce commit message template #1756

3 of 3 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment