Skip to content
This repository was archived by the owner on Dec 15, 2022. It is now read-only.
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
46 commits
Select commit Hold shift + click to select a range
f5ccd16
Revert "Merge pull request #1754 from atom/revert-1713-feature/commit…
kuychaco Oct 23, 2018
0a24012
:art: getCommitMessageFromTemplate tests. Return null if config not set
kuychaco Oct 23, 2018
49a1d31
Restore state from last commit only if undo is successful
kuychaco Oct 24, 2018
3ebda2f
Pass events to `observeFilesystemChange` in order to use action property
kuychaco Oct 24, 2018
b6ef6d2
Update commit message after file system change for MERGE_HEAD or config
kuychaco Oct 24, 2018
d895691
:shirt:
kuychaco Oct 24, 2018
bcbbff8
Load initial commit message based on template or merge state
kuychaco Oct 24, 2018
b8e55ab
:art: variable message -> template
kuychaco Oct 24, 2018
ca10d13
Don't set merge message in controller, now handled in repository
kuychaco Oct 24, 2018
99fe542
Fix repo test now that we are passing events and not paths
kuychaco Oct 24, 2018
40b015e
WIP tests for CommitController
kuychaco Oct 24, 2018
ade7de8
Suppress repo update when typing commit message
kuychaco Oct 24, 2018
7c18ddb
Don't trim commit message template
kuychaco Oct 24, 2018
7a73138
:art: rename getCommitMessageFromTemplate to getCommitMessageTemplate
kuychaco Oct 24, 2018
b16525c
Enable commit button only if message contains non-comment lines
kuychaco Oct 24, 2018
b3cc2c3
Set cursor to beginning of commit message template
kuychaco Oct 24, 2018
5a7b2d4
Reset commit message after a successful abort triggered through the UI
kuychaco Oct 24, 2018
b3588ef
Add repository tests for commit message updating due to file system e…
kuychaco Oct 24, 2018
4a1525b
Trim message before checking if clean
kuychaco Oct 24, 2018
2c2857e
:fire: merge message tests in CommitController (now handled in repo)
kuychaco Oct 24, 2018
f780ad1
Fix WorkdirContext test
kuychaco Oct 24, 2018
5ee83db
Fix CommitController tests
kuychaco Oct 25, 2018
6e45f0c
:fire: commit-template test fixture
kuychaco Oct 25, 2018
56d9f5e
Clean up cloneRepository
kuychaco Oct 25, 2018
d145084
:fire: comment
kuychaco Oct 25, 2018
8a40a74
Fix and clean up GitStrategies tests
kuychaco Oct 25, 2018
63ffa69
add comment explaining regex
Oct 25, 2018
c15f1d0
cleanup some dead code
Oct 25, 2018
fd1d1cf
add comment about isValidMessage regex
Oct 25, 2018
3d94405
:fire: unused dependencies.
Oct 25, 2018
acf502c
Merge branch 'master' into pr-1756/atom/ku-tt-commit-msg-template
Oct 25, 2018
9b606ba
test merged changes + cleanup
Oct 26, 2018
6379f30
fix issue with setting cursor buffer position
Oct 26, 2018
d331372
Retry flakey file patch test
kuychaco Oct 26, 2018
13e7342
extract regex and give it a better name
Oct 29, 2018
218f193
rename getCommitMessageTemplate to fetchCommitMessageTemplate
Oct 29, 2018
e2cc423
extract filePathEndsWith into helper function
Oct 29, 2018
b3c9ec7
Extract wireUpObserver and expectEvents to test helpers file
kuychaco Oct 30, 2018
37fbb44
Access local config in spec mode
kuychaco Oct 30, 2018
6299ae2
Don't break snapshotting by accessing `atom` in global scope
kuychaco Oct 30, 2018
8aaf2ac
Test flakes
kuychaco Oct 30, 2018
e8e39db
Merge remote-tracking branch 'origin/master' into ku-tt-commit-msg-te…
kuychaco Oct 30, 2018
3e1cb40
Retry file-patch test flakes
kuychaco Oct 30, 2018
a87b3a4
handle case where template is unset
Nov 6, 2018
6f88f76
add unit test for unsetting commit template
Nov 6, 2018
4e400bd
address code review feedback.
Nov 6, 2018
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 4 additions & 13 deletions lib/controllers/commit-controller.js
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,6 @@ export default class CommitController extends React.Component {

repository: PropTypes.object.isRequired,
isMerging: PropTypes.bool.isRequired,
mergeMessage: PropTypes.string,
mergeConflictsExist: PropTypes.bool.isRequired,
stagedChangesExist: PropTypes.bool.isRequired,
lastCommit: PropTypes.object.isRequired,
Expand Down Expand Up @@ -78,10 +77,6 @@ export default class CommitController extends React.Component {
}
}),
);

if (this.props.isMerging && !this.getCommitMessage()) {
this.commitMessageBuffer.setText(this.props.mergeMessage || '');
}
}

render() {
Expand Down Expand Up @@ -115,11 +110,7 @@ export default class CommitController extends React.Component {
}

componentDidUpdate(prevProps) {
if (!prevProps.isMerging && this.props.isMerging && !this.getCommitMessage()) {
this.commitMessageBuffer.setText(this.props.mergeMessage || '');
} else {
this.commitMessageBuffer.setTextViaDiff(this.getCommitMessage());
}
this.commitMessageBuffer.setTextViaDiff(this.getCommitMessage());
}

componentWillUnmount() {
Expand All @@ -140,9 +131,9 @@ export default class CommitController extends React.Component {
return this.props.commit(msg.trim(), {amend, coAuthors, verbatim});
}

setCommitMessage(message) {
setCommitMessage(message, options) {
if (!this.props.repository.isPresent()) { return; }
this.props.repository.setCommitMessage(message);
this.props.repository.setCommitMessage(message, options);
}

getCommitMessage() {
Expand All @@ -157,7 +148,7 @@ export default class CommitController extends React.Component {
if (!this.props.repository.isPresent()) {
return;
}
this.setCommitMessage(this.commitMessageBuffer.getText());
this.setCommitMessage(this.commitMessageBuffer.getText(), {suppressUpdate: true});
}

getCommitMessageEditors() {
Expand Down
4 changes: 3 additions & 1 deletion lib/controllers/git-tab-controller.js
Original file line number Diff line number Diff line change
Expand Up @@ -269,13 +269,15 @@ export default class GitTabController extends React.Component {
const repo = this.props.repository;
const lastCommit = await repo.getLastCommit();
if (lastCommit.isUnbornRef()) { return null; }

await repo.undoLastCommit();
repo.setCommitMessage(lastCommit.getFullMessage());

const coAuthors = lastCommit.getCoAuthors().map(author =>
new Author(author.email, author.name));

this.updateSelectedCoAuthors(coAuthors);
return repo.undoLastCommit();
return null;
}

async abortMerge() {
Expand Down
3 changes: 2 additions & 1 deletion lib/get-repo-pipeline-manager.js
Original file line number Diff line number Diff line change
Expand Up @@ -210,7 +210,8 @@ export default function({confirm, notificationManager, workspace}) {
commitPipeline.addMiddleware('failed-to-commit-error', async (next, repository) => {
try {
const result = await next();
repository.setCommitMessage('');
const template = await repository.fetchCommitMessageTemplate();
repository.setCommitMessage(template || '');
destroyFilePatchPaneItems({onlyStaged: true}, workspace);
return result;
} catch (error) {
Expand Down
36 changes: 35 additions & 1 deletion lib/git-shell-out-strategy.js
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,17 @@ const DISABLE_COLOR_FLAGS = [
return acc;
}, []);

/**
* Expand config path name per
* https://git-scm.com/docs/git-config#git-config-pathname
* this regex attempts to get the specified user's home directory
* Ex: on Mac ~kuychaco/ is expanded to the specified user’s home directory (/Users/kuychaco)
* Regex translation:
* ^~ line starts with tilde
* ([^/]*)/ captures non-forwardslash characters before first slash
*/
const EXPAND_TILDE_REGEX = new RegExp('^~([^/]*)/');

export default class GitShellOutStrategy {
static defaultExecArgs = {
stdin: null,
Expand Down Expand Up @@ -422,6 +433,29 @@ export default class GitShellOutStrategy {
return this.exec(args, {writeOperation: true});
}

async fetchCommitMessageTemplate() {
let templatePath = await this.getConfig('commit.template');
if (!templatePath) {
return null;
}

const homeDir = os.homedir();

templatePath = templatePath.trim().replace(EXPAND_TILDE_REGEX, (_, user) => {
// if no user is specified, fall back to using the home directory.
return `${user ? path.join(path.dirname(homeDir), user) : homeDir}/`;

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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 😉

});

if (!path.isAbsolute(templatePath)) {
templatePath = path.join(this.workingDir, templatePath);
}

if (!await fileExists(templatePath)) {
throw new Error(`Invalid commit template path set in Git config: ${templatePath}`);
}
return await fs.readFile(templatePath, {encoding: 'utf8'});
}

unstageFiles(paths, commit = 'HEAD') {
if (paths.length === 0) { return Promise.resolve(null); }
const args = ['reset', commit, '--'].concat(paths.map(toGitPathSep));
Expand Down Expand Up @@ -889,7 +923,7 @@ export default class GitShellOutStrategy {
let output;
try {
let args = ['config'];
if (local) { args.push('--local'); }
if (local || atom.inSpecMode()) { args.push('--local'); }
args = args.concat(option);
output = await this.exec(args);
} catch (err) {
Expand Down
3 changes: 3 additions & 0 deletions lib/helpers.js
Original file line number Diff line number Diff line change
Expand Up @@ -320,6 +320,9 @@ export function toGitPathSep(rawPath) {
}
}

export function filePathEndsWith(filePath, ...segments) {
return filePath.endsWith(path.join(...segments));
}

/**
* Turns an array of things @kuychaco cannot eat
Expand Down
85 changes: 78 additions & 7 deletions lib/models/repository-states/present.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import RemoteSet from '../remote-set';
import Commit from '../commit';
import OperationStates from '../operation-states';
import {addEvent} from '../../reporter-proxy';
import {filePathEndsWith} from '../../helpers';

/**
* State used when the working directory contains a valid git repository and can be interacted with. Performs
Expand All @@ -39,20 +40,46 @@ export default class Present extends State {
this.operationStates = new OperationStates({didUpdate: this.didUpdate.bind(this)});

this.commitMessage = '';
this.commitMessageTemplate = null;
this.fetchInitialMessage();
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if the commit message, commit message template, and merge message tracking belong more naturally in Repository rather than Present. Most methods in Present are intended to call a single git command with GitShellOutStrategy, manipulate the cache, and translate input and output to our model objects, while Repository methods implement higher-order composite behavior. Commit message derivation feels like a "composite" behavior to me 🤔

With that said: we already have a bunch of behavior in Repository and Present that blurs these lines, and it would not be trivial to move all of this there. So maybe we could leave this as a future refactoring as part of a broader effort to make the division among those abstraction layers clear.

Choose a reason for hiding this comment

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

cool - should I open an issue to refactor this in the future? I'm on board with cleaning it up.


if (history) {
this.discardHistory.updateHistory(history);
}
}

setCommitMessage(message) {
setCommitMessage(message, {suppressUpdate} = {suppressUpdate: false}) {
this.commitMessage = message;
if (!suppressUpdate) {
this.didUpdate();
}
}

setCommitMessageTemplate(template) {
this.commitMessageTemplate = template;
}

async fetchInitialMessage() {
const mergeMessage = await this.repository.getMergeMessage();
const template = await this.fetchCommitMessageTemplate();
if (template) {
this.commitMessageTemplate = template;
}
if (mergeMessage) {
this.setCommitMessage(mergeMessage);
} else if (template) {
this.setCommitMessage(template);
}
}

getCommitMessage() {
return this.commitMessage;
}

fetchCommitMessageTemplate() {
return this.git().fetchCommitMessageTemplate();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

It's odd to me that getCommitMessageTemplate() is a git operation, but setCommitMessageTemplate() is an accessor for an instance variable. Can we clarify the names to be less misleading somehow?

Also: should the result of this git operation be cached?

Fake edit: on reflection, caching this would cause some weird behavior because we don't have a filesystem watcher on the commit message template itself, so we wouldn't know when to invalidate that cache entry.

Choose a reason for hiding this comment

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

hahaha yup Katrina and I went through that exact conversation re caching.

renamed the method to fetchCommitMessageTemplate. I'm not 100% sure that's better, but perhaps folks won't think it's the inverse operation of setCommitMessageTemplate?


getOperationStates() {
return this.operationStates;
}
Expand All @@ -75,7 +102,8 @@ export default class Present extends State {
this.didUpdate();
}

observeFilesystemChange(paths) {
invalidateCacheAfterFilesystemChange(events) {
const paths = events.map(e => e.special || e.path);
const keys = new Set();
for (let i = 0; i < paths.length; i++) {
const fullPath = paths[i];
Expand All @@ -88,18 +116,17 @@ export default class Present extends State {
continue;
}

const endsWith = (...segments) => fullPath.endsWith(path.join(...segments));
const includes = (...segments) => fullPath.includes(path.join(...segments));

if (endsWith('.git', 'index')) {
if (filePathEndsWith(fullPath, '.git', 'index')) {
keys.add(Keys.stagedChangesSinceParentCommit);
keys.add(Keys.filePatch.all);
keys.add(Keys.index.all);
keys.add(Keys.statusBundle);
continue;
}

if (endsWith('.git', 'HEAD')) {
if (filePathEndsWith(fullPath, '.git', 'HEAD')) {
keys.add(Keys.branches);
keys.add(Keys.lastCommit);
keys.add(Keys.recentCommits);
Expand All @@ -125,7 +152,7 @@ export default class Present extends State {
continue;
}

if (endsWith('.git', 'config')) {
if (filePathEndsWith(fullPath, '.git', 'config')) {
keys.add(Keys.remotes);
keys.add(Keys.config.all);
keys.add(Keys.statusBundle);
Expand All @@ -144,6 +171,47 @@ export default class Present extends State {
}
}

isCommitMessageClean() {
if (this.commitMessage.trim() === '') {
return true;
} else if (this.commitMessageTemplate) {
return this.commitMessage === this.commitMessageTemplate;
}
return false;
}

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

if (filePathEndsWith(event.path, '.git', 'MERGE_HEAD')) {
if (event.action === 'created') {
if (this.isCommitMessageClean()) {
this.setCommitMessage(await this.repository.getMergeMessage());
}
} else if (event.action === 'deleted') {
this.setCommitMessage(this.commitMessageTemplate || '');
}
}

if (filePathEndsWith(event.path, '.git', 'config')) {
// this won't catch changes made to the template file itself...
const template = await this.fetchCommitMessageTemplate();
if (template === null) {
this.setCommitMessage('');
} else if (this.commitMessageTemplate !== template) {
this.setCommitMessage(template);
}
this.setCommitMessageTemplate(template);
}
}
}

observeFilesystemChange(events) {
this.invalidateCacheAfterFilesystemChange(events);
this.updateCommitMessageAfterFileSystemChange(events);
}

refresh() {
this.cache.clear();
this.didUpdate();
Expand Down Expand Up @@ -280,7 +348,10 @@ export default class Present extends State {
Keys.filePatch.all,
Keys.index.all,
],
() => this.git().abortMerge(),
async () => {
await this.git().abortMerge();
this.setCommitMessage(this.commitMessageTemplate || '');
},
);
}

Expand Down
4 changes: 4 additions & 0 deletions lib/models/repository-states/state.js
Original file line number Diff line number Diff line change
Expand Up @@ -374,6 +374,10 @@ export default class State {
return '';
}

fetchCommitMessageTemplate() {
return unsupportedOperationPromise(this, 'fetchCommitMessageTemplate');
}

// Cache

getCache() {
Expand Down
1 change: 1 addition & 0 deletions lib/models/repository.js
Original file line number Diff line number Diff line change
Expand Up @@ -358,6 +358,7 @@ const delegates = [

'setCommitMessage',
'getCommitMessage',
'fetchCommitMessageTemplate',
'getCache',
];

Expand Down
3 changes: 1 addition & 2 deletions lib/models/workdir-context.js
Original file line number Diff line number Diff line change
Expand Up @@ -50,8 +50,7 @@ export default class WorkdirContext {
// Wire up event forwarding among models
this.subs.add(this.repository.onDidChangeState(this.repositoryChangedState));
this.subs.add(this.observer.onDidChange(events => {
const paths = events.map(e => e.special || e.path);
this.repository.observeFilesystemChange(paths);
this.repository.observeFilesystemChange(events);
}));
this.subs.add(this.observer.onDidChangeWorkdirOrHead(() => this.emitter.emit('did-change-workdir-or-head')));

Expand Down
14 changes: 12 additions & 2 deletions lib/views/commit-view.js
Original file line number Diff line number Diff line change
Expand Up @@ -460,13 +460,18 @@ export default class CommitView extends React.Component {
}
}

isValidMessage() {
// ensure that there are at least some non-comment lines in the commit message.
// Commented lines are stripped out of commit messages by git, by default configuration.
return this.props.messageBuffer.getText().replace(/^#.*$/gm, '').trim().length !== 0;
}

commitIsEnabled(amend) {
const messageExists = this.props.messageBuffer.getLength() > 0;
return !this.props.isCommitting &&
(amend || this.props.stagedChangesExist) &&
!this.props.mergeConflictsExist &&
this.props.lastCommit.isPresent() &&
(this.props.deactivateCommitBox || (amend || messageExists));
(this.props.deactivateCommitBox || (amend || this.isValidMessage()));
}

commitButtonText() {
Expand Down Expand Up @@ -580,6 +585,11 @@ export default class CommitView extends React.Component {

if (focus === CommitView.focus.EDITOR) {
if (this.refEditorComponent.map(focusElement).getOr(false)) {
if (this.props.messageBuffer.getText().length > 0 && !this.isValidMessage()) {
// there is likely a commit message template present
// we want the cursor to be at the beginning, not at the and of the template
this.refEditorComponent.get().getModel().setCursorBufferPosition([0, 0]);
}
return true;
}
}
Expand Down
Loading