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

Re-introduce commit message template #1756

Merged
merged 46 commits into from Nov 7, 2018

Conversation

4 participants
@kuychaco
Member

kuychaco commented Oct 25, 2018

Description of the Change

This PR re-introduces the commit message template feature, originally introduced in #1713. It re-works some of the implementation, cleans up tests, and additionally fixes the merge message regression.

With these change, the repository model now manages and updates the commit message based on file system events indicating that the config has changed or a merge has been initiated/completed.

/cc @annthurium

Alternate Designs

N/A

Benefits

Folks will be able to make commits with the aid of commit message templates

Possible Drawbacks

The usual drawbacks associated with code changes. This feature in particular touches a part of the system that is complex and fairly edge-casey...

Applicable Issues

Original PR - #1713
Reverted PR - #1754

Metrics

N/A

Tests

Added unit tests for:

  • restores template after committing
  • config commit.template change
  • when merge is initiated
    • merge message replaces
      • empty commit message
      • untouched commit template
    • dirty message is left untouched
  • when merge is aborted
    • clean commit message is restored
    • commit template is restored

Manually tested:

  • When tabbing to commit box, cursor is at beginning of template (rather than at the very end)
  • Commit button is disabled when commit message contains a template. It is enabled only when commit message contains non-commented lines
  • Commit message updates correctly when expanding/closing expanded commit message editors
  • Commit message updates correctly to show merge message when merge is initiated and aborted
  • Commit message template is restored after committing

@smashwilson to test manually on Linux and Windows

Documentation

User Experience Research (Optional)

N/A

TODO:

  • clean up git strategies tests
  • de-dupe file system watching tests
  • verify template path logic works on windows and linux

kuychaco and others added some commits Oct 23, 2018

Revert "Merge pull request #1754 from atom/revert-1713-feature/commit…
…-template"

This reverts commit c96687a, reversing
changes made to 4bc7fde.
🎨 getCommitMessageFromTemplate tests. Return null if config not set
Co-Authored-By: Tilde Ann Thurium <annthurium@github.com>
Restore state from last commit only if undo is successful
Repro steps: 

open repo with merge conflict
click undo
see error (cannot do a soft reset in the middle of a merge)

expected: no change to commit message box
actual: commit message from last commit appears in box
Don't trim commit message template
It's best to assume that template users craft their template exactly as they want it. For example, I have two empty lines at the beginning of mine so that I can quickly type my message.
Set cursor to beginning of commit message template
We don't want it to be at the end
Reset commit message after a successful abort triggered through the UI
According to @smashwilson
> it may be worth triggering some of that behavior manually if a user aborts (or, eventually, initiates) a merge through our UI. in terms of reliability - on all three platforms there is some risk of dropped events that increases with the number of events that arrive in quick succession (like if a ton of files are created at once). merges in huge repositories _might_ trigger this but it shouldn't be common.

Co-Authored-By: Ash Wilson <smashwilson@gmail.com>
Fix CommitController tests
Co-Authored-By: Matt <mattmattmatt@users.noreply.github.com>
Clean up cloneRepository
We don't want to be setting up a commit template for every single test that runs. We do it locally for the tests that need it
🔥 comment
Co-Authored-By: Matt <mattmattmatt@users.noreply.github.com>

@kuychaco kuychaco added this to In Progress 🔧 in Feature Sprint : 1 October - 19 November 2018 : v0.21.0 via automation Oct 25, 2018

Show resolved Hide resolved lib/git-shell-out-strategy.js Outdated
Show resolved Hide resolved lib/models/repository-states/present.js Outdated
Show resolved Hide resolved lib/models/repository-states/present.js
Show resolved Hide resolved lib/views/commit-view.js Outdated
Show resolved Hide resolved lib/views/commit-view.js Outdated
add comment explaining regex
Co-Authored-By: Katrina Uychaco <katrina@github.com>
const regex = new RegExp('^~([^/]*)/');
templatePath = templatePath.trim().replace(regex, (_, user) => {
// if no user is specified, fall back to using the home directory.
return `${user ? path.join(path.dirname(homeDir), user) : homeDir}/`;

This comment has been minimized.

@annthurium

annthurium Oct 25, 2018

Contributor

@smashwilson: can you test this on non-Mac operating systems? We want to make sure if a username is specified, it does the right thing for our Linux and or Windows users.

This comment has been minimized.

@smashwilson

smashwilson Oct 29, 2018

Member

Can do, and will report back 🙇

One thing that I can see right away is that this won't handle ~root/ correctly; on most *nix systems, that should expand to /root, not /users/root. In general the right thing to do on Unix-y systems is to use getpwnam(3) to read the definitive information, taking into account any weird local network drive setups and so on. Windows I'm less familiar with.

There are a few tilde-expansion libraries on npm, but unfortunately looking at their source it doesn't look like many of them handle all of this correctly either! I found one that uses etc-passwd to read its info, but I don't believe it has any accommodations for Windows. This may be a sign that this isn't important enough to hold up the 🚢 for 😉

@coveralls

This comment has been minimized.

coveralls commented Oct 26, 2018

Coverage Status

Coverage decreased (-0.003%) to 84.869% when pulling 4e400bd on ku-tt-commit-msg-template into 0e906e2 on master.

annthurium and others added some commits Oct 26, 2018

test merged changes + cleanup
Co-Authored-By: Katrina Uychaco <katrina@github.com>
fix issue with setting cursor buffer position
This only works for commit message templates where every line starts 
with a #.  We explored using isCommitMessageClean to try and handle 
commit message templates that have uncommented lines.  We're not calling 
didUpdate every time the commit message is updated, so it's out of sync, 
and we decided it was getting too edge casey out there.

Co-Authored-By: Katrina Uychaco <katrina@github.com>

@kuychaco kuychaco requested a review from smashwilson Oct 26, 2018

@smashwilson

I have a bunch of assorted feedback, but no bugs or blockers 😄

I'll test the ~ expansion logic on Windows and Linux when I get the chance and leave a 👍 or accordingly.

Show resolved Hide resolved lib/controllers/commit-controller.js
Show resolved Hide resolved lib/controllers/commit-controller.js
Show resolved Hide resolved lib/get-repo-pipeline-manager.js
// Regex translation:
// ^~ line starts with tilde
// ([^/]*)/ captures non-forwardslash characters before first slash
const regex = new RegExp('^~([^/]*)/');

This comment has been minimized.

@smashwilson

smashwilson Oct 29, 2018

Member

Is there a reason this is constructed with new RegExp() rather than as a regexp literal? Is it just for readability so you don't have to escape all of the / and get into leaning-toothpick-syndrome?

A more descriptive name than regex may be nice too 😄 How about something like tildeExpansionPattern?

This comment has been minimized.

@annthurium

annthurium Oct 29, 2018

Contributor

We didn't really have a strong reason for preferring the regexp constructor over literals here. We went with what the original author had.

Out of curiosity, we looked to see if there was a performance difference. Answer: not really.
https://www.measurethat.net/Benchmarks/Show/1734/1/regexp-constructor-vs-literal

We did give it a more descriptive name, and moved it out of this function so as to avoid creating a new regex every time.

const regex = new RegExp('^~([^/]*)/');
templatePath = templatePath.trim().replace(regex, (_, user) => {
// if no user is specified, fall back to using the home directory.
return `${user ? path.join(path.dirname(homeDir), user) : homeDir}/`;

This comment has been minimized.

@smashwilson

smashwilson Oct 29, 2018

Member

Can do, and will report back 🙇

One thing that I can see right away is that this won't handle ~root/ correctly; on most *nix systems, that should expand to /root, not /users/root. In general the right thing to do on Unix-y systems is to use getpwnam(3) to read the definitive information, taking into account any weird local network drive setups and so on. Windows I'm less familiar with.

There are a few tilde-expansion libraries on npm, but unfortunately looking at their source it doesn't look like many of them handle all of this correctly either! I found one that uses etc-passwd to read its info, but I don't believe it has any accommodations for Windows. This may be a sign that this isn't important enough to hold up the 🚢 for 😉

async updateCommitMessageAfterFileSystemChange(events) {
for (let i = 0; i < events.length; i++) {
const event = events[i];
const endsWith = (...segments) => event.path.endsWith(path.join(...segments));

This comment has been minimized.

@smashwilson

smashwilson Oct 29, 2018

Member

Might be worth extracting this to helpers.js?

It is a one-liner, but eh.

This comment has been minimized.

@annthurium

annthurium Oct 29, 2018

Contributor

sure! Good suggestion. Extracted.

Show resolved Hide resolved lib/views/commit-view.js
Show resolved Hide resolved lib/views/commit-view.js
Show resolved Hide resolved test/integration/file-patch.test.js Outdated
return {repository, observer};
}
function expectEvents(repository, ...suffixes) {

This comment has been minimized.

@smashwilson

smashwilson Oct 29, 2018

Member

Yeah, these would be great to extract to a helper and clean up a bit... We have a few places that test Real Filesystem Events ™️ where these would be nice.

This comment has been minimized.

@kuychaco

kuychaco Oct 30, 2018

Member

How does this look b3c9ec7?

annthurium and others added some commits Oct 29, 2018

extract regex and give it a better name
Co-Authored-By: Katrina Uychaco <katrina@github.com>
rename getCommitMessageTemplate to fetchCommitMessageTemplate
Co-Authored-By: Katrina Uychaco <katrina@github.com>
Extract wireUpObserver and expectEvents to test helpers file
Co-Authored-By: Tilde Ann Thurium <annthurium@github.com>
Access local config in spec mode
Tests were failing because my global config was being used to look up the commit template
Don't break snapshotting by accessing `atom` in global scope
Co-Authored-By: Ash Wilson <smashwilson@gmail.com>
@smashwilson

This comment has been minimized.

Member

smashwilson commented Oct 30, 2018

I just found a bug on Windows. If I run git config --unset commit.template at the command line, the git tab errors out and leaves this stack in the console:

C:\Users\smash\src\atom\node_modules\text-buffer\lib\text-buffer.js:1319 Uncaught TypeError: Cannot read property 'split' of null
    at Diff.lineDiff.tokenize (C:\Users\smash\src\atom\node_modules\text-buffer\node_modules\diff\lib\diff\line.js:22:32)
    at Diff.diff (C:\Users\smash\src\atom\node_modules\text-buffer\node_modules\diff\lib\diff\base.js:36:39)
    at Object.diffLines (C:\Users\smash\src\atom\node_modules\text-buffer\node_modules\diff\lib\diff\line.js:47:19)
    at transact (C:\Users\smash\src\atom\node_modules\text-buffer\lib\text-buffer.js:801:29)
    at TextBuffer.transact (C:\Users\smash\src\atom\node_modules\text-buffer\lib\text-buffer.js:1316:16)
    at TextBuffer.setTextViaDiff (C:\Users\smash\src\atom\node_modules\text-buffer\lib\text-buffer.js:796:10)
    at CommitController.componentDidUpdate (C:/Users/smash/src/github/lib/controllers/commit-controller.js:113:30)
    at commitLifeCycles (C:\Users\smash\src\github\node_modules\react-dom\cjs\react-dom.development.js:14256:22)
    at commitAllLifeCycles (C:\Users\smash\src\github\node_modules\react-dom\cjs\react-dom.development.js:15342:7)
    at HTMLUnknownElement.callCallback (C:\Users\smash\src\github\node_modules\react-dom\cjs\react-dom.development.js:100:14)
    at Object.invokeGuardedCallbackDev (C:\Users\smash\src\github\node_modules\react-dom\cjs\react-dom.development.js:138:16)
    at invokeGuardedCallback (C:\Users\smash\src\github\node_modules\react-dom\cjs\react-dom.development.js:187:29)
    at commitRoot (C:\Users\smash\src\github\node_modules\react-dom\cjs\react-dom.development.js:15481:7)
    at completeRoot (C:\Users\smash\src\github\node_modules\react-dom\cjs\react-dom.development.js:16496:34)
    at performWorkOnRoot (C:\Users\smash\src\github\node_modules\react-dom\cjs\react-dom.development.js:16440:9)
    at performWork (C:\Users\smash\src\github\node_modules\react-dom\cjs\react-dom.development.js:16358:7)
    at performSyncWork (C:\Users\smash\src\github\node_modules\react-dom\cjs\react-dom.development.js:16330:3)
    at requestWork (C:\Users\smash\src\github\node_modules\react-dom\cjs\react-dom.development.js:16230:5)
    at scheduleWork$1 (C:\Users\smash\src\github\node_modules\react-dom\cjs\react-dom.development.js:16096:11)
    at Object.enqueueSetState (C:\Users\smash\src\github\node_modules\react-dom\cjs\react-dom.development.js:11185:5)
    at ObserveModel.Component.setState (C:\Users\smash\src\github\node_modules\react\cjs\react.development.js:273:16)
    at ObserveModel.didUpdate (C:/Users/smash/src/github/lib/views/observe-model.js:39:12)
    at ModelObserver._refreshModelData (C:/Users/smash/src/github/lib/models/model-observer.js:52:14)
    at <anonymous>
    at process._tickCallback (internal/process/next_tick.js:188:7)

From the message, I'm guessing that the fetched commit template is null instead of an empty string unexpectedly?

Oddly it's fine if I ctrl-F5 with no commit.template set.

@smashwilson

This comment has been minimized.

Member

smashwilson commented Oct 30, 2018

Path logic is 👍 on Windows: I verified both ~/... and ~username/... paths. Checking Linux next...

@smashwilson

This comment has been minimized.

Member

smashwilson commented Oct 31, 2018

👍 to path logic on Linux as well. (And I was able to reproduce the --unset bug I mentioned above, so it must not be platform-specific.)

Feature Sprint : 1 October - 19 November 2018 : v0.21.0 automation moved this from In Progress 🔧 to QA Review 🔬 Oct 31, 2018

@smashwilson

We should probably fix that stack trace I found when running git config --unset commit.template with an Atom window open. Other than that, looks good - all of the path wrangling worked correctly on Windows and Linux 👍

this bug has been addressed

@annthurium annthurium merged commit 26c39ee into master Nov 7, 2018

7 checks passed

ci/circleci: beta Your tests passed on CircleCI!
Details
ci/circleci: dev Your tests passed on CircleCI!
Details
ci/circleci: snapshot 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 decreased (-0.003%) to 84.869%
Details

Feature Sprint : 1 October - 19 November 2018 : v0.21.0 automation moved this from QA Review 🔬 to Merged ☑️ Nov 7, 2018

@annthurium annthurium deleted the ku-tt-commit-msg-template branch Nov 7, 2018

@vanessayuenn vanessayuenn referenced this pull request Nov 21, 2018

Closed

v0.23.0-0 QA Review #1806

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