Skip to content
This repository was archived by the owner on Dec 15, 2022. It is now read-only.

Conversation

@annthurium
Copy link

@annthurium annthurium commented Jan 10, 2019

Description of the Change

This change fixes a bug where commit message templates that include commented lines were included in the body of the commit.

If a commit template exists, istrip commented lines from the commit message.

Alternate Designs

  • My initial approach had us ignoring the verbatim flag if a commit template was present. However, we want to preserve the verbatim flag because otherwise commits made from the mini editor might be formatted weirdly in some cases. See Revisit commit message processing #1500 for more info.

  • We discussed ripping out the verbatim flag entirely, to aid complexity, but wanted to maintain the hard wrapping so as not to break formatting for users who are using the mini editor but not using a template.

Benefits

  • Users can make commits with message templates, without including commented lines in the commit output, because probably they do not want that.
  • Consistent with command line git behavior

Possible Drawbacks

  • Adds more complexity to some code that's already rife with special cases.
  • There might be some users who have commit templates and want to keep lines that start with #

Applicable Issues

#1817

Metrics

N/A

Tests

  • Manually tested making a commit with a commit template set, where the template included # characters. Verified that those commented lines did not show up when using git log to inspect the commit.
  • unit test in git-shell-out-strategy to verify that commented lines are stripped when using commit message templates. (We already have a unit test for validating that commented lines are not stripped, if a commit template does not exist.)

Documentation

Added some code comments.

Release Notes

Fixed an issue where commits made with commit message templates were not stripping comment characters.

Tilde Ann Thurium added 2 commits January 10, 2019 13:19
in #1817, there is a bug where we are not stripping commented lines if a git message template exists.

The `verbatim` flag is passed if the commit is made from a mini editor.  The mini editor is meant to be analagous of `git commit -m`, a quick and dirty sort of affair.

As much as I don't want to add special cases here, it seemed like the cleanest to ignore `verbatim` if you have a template set.
@codecov
Copy link

codecov bot commented Jan 10, 2019

Codecov Report

Merging #1902 into master will decrease coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1902      +/-   ##
==========================================
- Coverage   91.09%   91.08%   -0.02%     
==========================================
  Files         185      185              
  Lines       10719    10722       +3     
  Branches     1575     1576       +1     
==========================================
+ Hits         9765     9766       +1     
- Misses        954      956       +2
Impacted Files Coverage Δ
lib/git-shell-out-strategy.js 87.58% <100%> (+0.06%) ⬆️
lib/models/repository-states/present.js 95.02% <0%> (-0.39%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4786390...3cde57a. Read the comment docs.

@annthurium annthurium requested a review from a team January 11, 2019 21:24
Copy link
Contributor

@kuychaco kuychaco left a comment

Choose a reason for hiding this comment

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

Looks good to me! 👍 :shipit:

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants