prepare-commit-message hook #520

dcluna · 2017-11-04T23:08:52Z

As mentioned in #516, this commit implements the prepare-commit-msg hook, with a default hook that prepends a message based on the branch name through regex substitution.

dcluna · 2017-11-04T23:10:10Z

lib/overcommit/hook_context/prepare_commit_msg.rb

+    # exists); or commit, followed by a commit SHA-1 (if a -c, -C or --amend
+    # option was given)
+    def commit_msg_source
+      (@args[1] || 'commit').to_sym


While testing locally, the args passed in a standard commit were just the filename in question (.git/COMMIT_EDITMSG), which wasn't expected as per the git docs. Do you have more context on this?

I believe the documentation states that the commit arg is only included if you are amending an existing commit (via --amend) or copying a commit message from another commit (via -c or -C). Thus we should not be defining a default value here.

dcluna · 2017-11-04T23:10:38Z

spec/overcommit/hook/prepare_commit_msg/replace_branch_spec.rb

@@ -0,0 +1,59 @@
+require 'spec_helper'
+
+describe Overcommit::Hook::PrepareCommitMsg::ReplaceBranch do


This file lacks repo tests. Let me know if these are required for this PR to be merged.

This is fine. As long as we have rigorous tests for the base class which validate the behavior of our commit message modifier helpers and the commit_message_source / commit_message_source_ref helpers, we can stub those in hook implementation tests.

dcluna · 2017-11-04T23:11:09Z

spec/overcommit/hook_context/prepare_commit_msg_spec.rb

+    subject { context.commit }
+
+    context "source isn't :commit" do
+      it { should == `git rev-parse HEAD` }


This is a bit ugly. Can you think of a better way to write this spec?

You'll be able to remove this particular test given my comment above.

dcluna · 2017-11-04T23:12:27Z

lib/overcommit/hook/prepare_commit_msg/replace_branch.rb

+    def prepend_commit_message
+      Overcommit::Utils.log.debug("Writing #{commit_msg_filename} with #{new_template}")
+      old_contents = File.read(commit_msg_filename)
+      File.open(commit_msg_filename, 'w') do |commit_file|


Perhaps this code should be extracted into a utility function in base? prepend_commit_msg seems like a common-enough occurrence.

You're absolutely right that we should provide a helper for modifying the commit message. The most important feature we must provide here is a mechanism for modifying a commit message atomically.

Since Overcommit parallelizes hooks, we have the risk of clobbering messages added by different hook runs (for example, if two hooks read the current commit message filename on disk at the same time, each update it to their liking, and then save, one update will be clobbered by the other).

We need to ensure we handle this case. Perhaps a block that obtains a lock, e.g.

modify_commit_message do |current_msg| # In here, hook has exclusive access to the commit message "My prepended message\n" + current_msg # Modified message is returned end

You can then implement prepend_commit_message and append_commit_message using modify_commit_message (whether these provide significant value to hook authors is a separate question).

We could also ensure any modifications to the commit message are made in memory, so we don't need to save to the file until the end of the hook run.

trotzig · 2018-01-09T20:31:36Z

Sorry about the delay here. I don't have enough context to give this a proper review, so I'm going to ask @sds to see if he does.

sds

Hey @dcluna, my sincere apologies for the delay in response. I know it's frustrating to put effort into an implementation only to have a pull request go unanswered.

I have a number of minor concerns with the implementation, but the general pattern here looks good. Thanks for taking the time to write tests and for putting up an initial draft of ideas.

If my comments are addressed I will be happy to merge this feature!

sds · 2018-01-15T19:33:51Z

config/default.yml

+    enabled: false
+    description: 'Prepends the commit message with text based on the branch name'
+    branch_pattern: '\A.*\w+[-_](\d+).*\z'
+    replacement_text: '[#\1] [ci skip]'


It seems odd to me that [ci skip] would be included by default here. Remember, we're defining a default configuration for anyone choosing to enable this hook.

Can you elaborate why the default user would want this?

This hook's main function is to match a ticket identifier in the beginning of the branch name, so if you follow a certain naming convention for your branches, like $BUGTRACKER_NAME-$ID_IN_BUGTRACKER-$BRANCH_DESCRIPTION, you'll have those already set up in the commit message name.

I use this for Github's JIRA integration, not sure if I should remove the [ci skip] to a hook of its own or just remove it altogether. WDYT?

sds · 2018-01-15T19:34:51Z

config/default.yml

+    replacement_text: '[#\1] [ci skip]'
+    on_fail: warn
+
+


nit: can remove the extra line.

sds · 2018-01-15T19:50:00Z

lib/overcommit/hook_context/prepare_commit_msg.rb

+    # exists); or commit, followed by a commit SHA-1 (if a -c, -C or --amend
+    # option was given)
+    def commit_msg_source
+      (@args[1] || 'commit').to_sym


I believe the documentation states that the commit arg is only included if you are amending an existing commit (via --amend) or copying a commit message from another commit (via -c or -C). Thus we should not be defining a default value here.

sds · 2018-01-15T19:50:55Z

lib/overcommit/hook/prepare_commit_msg/replace_branch.rb

+    def prepend_commit_message
+      Overcommit::Utils.log.debug("Writing #{commit_msg_filename} with #{new_template}")
+      old_contents = File.read(commit_msg_filename)
+      File.open(commit_msg_filename, 'w') do |commit_file|


You're absolutely right that we should provide a helper for modifying the commit message. The most important feature we must provide here is a mechanism for modifying a commit message atomically.

Since Overcommit parallelizes hooks, we have the risk of clobbering messages added by different hook runs (for example, if two hooks read the current commit message filename on disk at the same time, each update it to their liking, and then save, one update will be clobbered by the other).

We need to ensure we handle this case. Perhaps a block that obtains a lock, e.g.

modify_commit_message do |current_msg| # In here, hook has exclusive access to the commit message "My prepended message\n" + current_msg # Modified message is returned end

You can then implement prepend_commit_message and append_commit_message using modify_commit_message (whether these provide significant value to hook authors is a separate question).

We could also ensure any modifications to the commit message are made in memory, so we don't need to save to the file until the end of the hook run.

sds · 2018-01-15T20:02:17Z

lib/overcommit/hook_context/prepare_commit_msg.rb

+    # Returns the commit's SHA-1.
+    # If commit_msg_source is :commit, it's passed through the command-line.
+    def commit
+      @args[2] || `git rev-parse HEAD`


We need to be clear about the meaning of the value returned by this helper method. From my comment above, if the commit is defined then arg[2] will be the SHA-1 of the commit we're copying, but not the commit about to be created. I think this will be confusing and we should be explicit.

Let's call this method commit_message_source_ref. While it's not a true ref I think the wording helps emphasize that it's referring to a commit hash ref, and emphasizes the connection to the commit_message_source helper above.

sds · 2018-01-15T20:04:50Z

spec/overcommit/hook/prepare_commit_msg/replace_branch_spec.rb

@@ -0,0 +1,59 @@
+require 'spec_helper'
+
+describe Overcommit::Hook::PrepareCommitMsg::ReplaceBranch do


This is fine. As long as we have rigorous tests for the base class which validate the behavior of our commit message modifier helpers and the commit_message_source / commit_message_source_ref helpers, we can stub those in hook implementation tests.

sds · 2018-01-15T20:06:38Z

lib/overcommit/hook_context/prepare_commit_msg.rb

+    # merge or a .git/MERGE_MSG file exists); squash (if a .git/SQUASH_MSG file
+    # exists); or commit, followed by a commit SHA-1 (if a -c, -C or --amend
+    # option was given)
+    def commit_msg_source


Let's call this commit_message_source (we aren't using the short msg form in any other consumable API).

sds · 2018-01-15T20:10:28Z

spec/overcommit/hook_context/prepare_commit_msg_spec.rb

+    subject { context.commit }
+
+    context "source isn't :commit" do
+      it { should == `git rev-parse HEAD` }


You'll be able to remove this particular test given my comment above.

bpaul · 2018-08-07T17:15:59Z

@dcluna Is this still active? I'd love to see prepare-commit-message support merged. I'm happy to tidy up this PR if necessary...

dcluna · 2018-08-07T17:48:19Z

@bpaul sorry, I haven't had enough time to tidy this up. Feel free to move this forward

siwica · 2018-09-30T02:29:53Z

@bpaul Hey, are you still planning on tidying this up? I'd be very much interested in having this hook as part of overcommit

correctly configured, automatically generates a commit message template based on the branch name. Groups captured in the branch_pattern regex can be used in replacement_text; see the accompanying spec for details. Also, replacement_text can be a path to a file, whose text will be processed following the same rules.

and default.yml config file

dcluna · 2018-09-30T17:21:06Z

@sds I hope to have addressed your concerns with the latest updates.

I went with a lock-based solution for the prepare-commit-msg hook, since IMHO the most intuitive behavior for this is that the hooks are run sequentially, so the modifications performed on the commit message build on top of each other.

(For the sake of completeness, for an in-memory solution: I think it'd be possible to write a custom object that allowed add/delete/modify operations in-memory, but then that'd require a custom cleanup_environment call. Since this would probably require a little more code than wrapping the modify_commit_message with a Mutex, I went with the simpler solution)

This implementation requires that all prepare-commit-msg hooks share the same context (so they'll all be sharing this lock). Please let me know if that isn't the case so I can think of another solution, or if there is anything else I can do to improve this PR.

commit_message_source API

sds · 2018-10-08T06:16:44Z

Thanks @dcluna!

dcluna commented Nov 4, 2017

View reviewed changes

dcluna force-pushed the prepare-message-hook branch from 29c6b10 to 6916078 Compare November 5, 2017 06:57

trotzig requested a review from sds January 9, 2018 20:31

sds reviewed Jan 15, 2018

View reviewed changes

dcluna added 4 commits September 30, 2018 13:59

Add prepare-commit-msg hook

70854f2

Address code review comments relating to concurrency

225b7d1

and default.yml config file

Updating ReplaceBranch hook

8f2b48a

dcluna force-pushed the prepare-message-hook branch from fac41cf to 8f2b48a Compare September 30, 2018 17:00

Fixing CI errors

4aa26d6

dcluna force-pushed the prepare-message-hook branch from c959c6c to bdf7359 Compare September 30, 2018 17:29

Hopefully fixing Appveyor windows build

ae2d529

dcluna force-pushed the prepare-message-hook branch from bdf7359 to ae2d529 Compare September 30, 2018 17:39

dcluna added 2 commits September 30, 2018 18:58

Not removing test file in Appveyor due to build errors

5e338e7

Fixing replace_branch with the new

9ae0dc4

commit_message_source API

sds merged commit 47962f7 into sds:master Oct 8, 2018

sds added the enhancement label Oct 8, 2018

sds mentioned this pull request Oct 8, 2018

Add support for prepare-commit-msg hooks #470

Closed

		@@ -0,0 +1,59 @@
		require 'spec_helper'

		describe Overcommit::Hook::PrepareCommitMsg::ReplaceBranch do

prepare-commit-message hook #520

prepare-commit-message hook #520

Uh oh!

Conversation

dcluna commented Nov 4, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sds Jan 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sds Jan 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

trotzig commented Jan 9, 2018

Uh oh!

sds left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sds Jan 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sds Jan 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bpaul commented Aug 7, 2018

Uh oh!

dcluna commented Aug 7, 2018

Uh oh!

siwica commented Sep 30, 2018

Uh oh!

dcluna commented Sep 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sds commented Oct 8, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

sds Jan 15, 2018 •

edited

Loading

sds Jan 15, 2018 •

edited

Loading

sds Jan 15, 2018 •

edited

Loading

sds Jan 15, 2018 •

edited

Loading

dcluna commented Sep 30, 2018 •

edited

Loading