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

Discard lines in file diff #487

Merged
merged 60 commits into from
Feb 10, 2017
Merged
Show file tree
Hide file tree
Changes from 36 commits
Commits
Show all changes
60 commits
Select commit Hold shift + click to select a range
5178dbf
Make Unstaged Changes div focusable
kuychaco Jan 24, 2017
e10790b
Rename command for discarding changes in file
kuychaco Jan 24, 2017
eb6dcbe
Add discardChangesInBuffer function with tests
kuychaco Jan 26, 2017
476e12a
Modify buffer in transaction to allow undoing discard actions
kuychaco Jan 26, 2017
e3c4daf
Implement GitController#discardLines and test
kuychaco Jan 26, 2017
eb6d14e
Add discard selection context menu option for untaged file diff
kuychaco Jan 26, 2017
6ff442f
Add TODO to handle no-new-line case
kuychaco Jan 26, 2017
0263946
:fire: console.log
kuychaco Jan 26, 2017
9bd128a
Add createBlob and restoreBlob to GitShellOutStrategy
kuychaco Jan 27, 2017
569a1fa
Create FileDiscardHistory model
kuychaco Jan 27, 2017
2044feb
Add ability to undo last discard action
kuychaco Jan 27, 2017
4a5a489
Fix borked test
kuychaco Jan 27, 2017
3d0df0f
Throw error if there's no match when discarding added lines
kuychaco Jan 27, 2017
097782b
Add FilePatch-header
simurai Jan 24, 2017
803e8d5
:shirt:
kuychaco Jan 27, 2017
853b630
Merge remote-tracking branch 'origin/master' into ku-discard-lines
kuychaco Jan 27, 2017
5093966
Wire up buttons in file diff header
kuychaco Jan 27, 2017
e2a9737
Move discard context menu items to separate section
kuychaco Jan 27, 2017
8455e3f
Add to GitShellOutStrategy#isPartiallyStaged
kuychaco Jan 27, 2017
2b1fdc0
Only show diff view button for viewing corresonding diff if one exists
kuychaco Jan 27, 2017
42c6a61
Align FilePatchView-title
simurai Jan 27, 2017
d1862a8
Add `github:view-corresponding-diff` command
kuychaco Jan 27, 2017
2650171
Add command `github:undo-last-file-diff-discard`
kuychaco Jan 27, 2017
d672567
:shirt:
kuychaco Jan 27, 2017
3f4ce84
Undo discard with `cmd-z`
kuychaco Jan 27, 2017
9a5747e
:fire: Remove toggle styles
simurai Jan 27, 2017
bba2bfc
Truncate FilePatchView-title
simurai Jan 27, 2017
cac3ccf
Merge branch 'ku-discard-lines' of https://github.com/atom/github int…
simurai Jan 27, 2017
357a813
Perform safety check before performing destructive action to avoid races
kuychaco Jan 27, 2017
2adf514
Dispose of newly created buffer after discard
kuychaco Jan 27, 2017
d72302d
:fire: comment
kuychaco Jan 27, 2017
fb0d10a
:art: use `until` in tests
kuychaco Feb 1, 2017
038c09f
Fix test
kuychaco Feb 1, 2017
2f218fd
Fix flakey tests using `until`
kuychaco Feb 1, 2017
9c8a2e0
:art:
kuychaco Feb 1, 2017
455cf45
:shirt:
kuychaco Feb 1, 2017
9ebd6a7
Quietly select corresponding item in StagingView
kuychaco Feb 2, 2017
be5e400
Use `core:undo` instead of `cmd-z`
kuychaco Feb 5, 2017
1738ea5
:art: Create `CannotRestoreError`
kuychaco Feb 5, 2017
34bdd81
Log non-CannotRestoreError errors
kuychaco Feb 5, 2017
d6799a0
:art: use assert.async.isTrue rather than until
kuychaco Feb 5, 2017
5281ad9
:art: use assert.async.equal instead of until
kuychaco Feb 5, 2017
8073c37
:shirt:
kuychaco Feb 5, 2017
bb3d953
Allow blob creation with stdin in GSOS#createBlob
kuychaco Feb 5, 2017
771c582
Fix test description
kuychaco Feb 5, 2017
03a9a68
Add GSOS#getBlobContents(sha)
kuychaco Feb 5, 2017
11960f9
Persist discard history in git config across refresh and Atom windows
kuychaco Feb 8, 2017
3f3633d
Limit discard history to prevent unbounded growth
kuychaco Feb 8, 2017
0a222b3
Merge remote-tracking branch 'origin/master' into ku-discard-lines
kuychaco Feb 8, 2017
4057109
Clear discard history if snapshot is expired
kuychaco Feb 9, 2017
18d768e
:shirt:
kuychaco Feb 9, 2017
3dd6f96
Add button in file diff to stage or unstage all
kuychaco Feb 9, 2017
1a048a0
Clean up window focus listener
kuychaco Feb 9, 2017
7ffcfb4
Set default discard history to empty object
kuychaco Feb 9, 2017
684d729
getHistoryForPath -> getLastHistorySnapshotsForPath
kuychaco Feb 10, 2017
50b306d
Open pre-discard versin of file in new buffer
kuychaco Feb 10, 2017
a219518
Move hasUndoHistory method from GitController to FilePatchController
kuychaco Feb 10, 2017
7e62f54
Add test for openFileInNewBuffer
kuychaco Feb 10, 2017
b788334
Fix tests
kuychaco Feb 10, 2017
06cfcd5
:art:
kuychaco Feb 10, 2017
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
1 change: 1 addition & 0 deletions keymaps/git.cson
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
'shift-tab': 'github:select-previous-hunk'
'right': 'core:move-right'
'o': 'github:open-file'
'cmd-z': 'github:undo-last-file-diff-discard'
Copy link
Contributor

Choose a reason for hiding this comment

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

What are the tradeoffs here between using the core:undo event, versus creating our own github-specific undo command and binding it to the same default key that core:undo uses?

Using core:undo

  • 👍 Inherit customized keybindings for core:undo. If someone's customized their core:undo keystroke for whatever reason, we could automatically be consistent.
  • 👎 Once we're working in TextEditors and more tightly integrated with other Atom components, core:undo will collide with existing implementations. I think this means that users would have no way to do a "real" undo without some keymap shenanigans.

Creating new github:-specific undo command

  • 👍 Users can customize these actions specifically.
  • 👎 Will likely fill up the github: namespace in the command palette with "undo" actions.


'.github-Prompt-input':
'enter': 'core:confirm'
Expand Down
19 changes: 19 additions & 0 deletions lib/controllers/file-patch-controller.js
Original file line number Diff line number Diff line change
Expand Up @@ -40,18 +40,25 @@ export default class FilePatchController {
);
} else {
// NOTE: Outer div is required for etch to render elements correctly
const filePath = this.props.filePatch.getPath();
return (
<div className="github-PaneView pane-item">
<FilePatchView
ref="filePatchView"
commandRegistry={this.props.commandRegistry}
attemptLineStageOperation={this.attemptLineStageOperation}
didSurfaceFile={this.didSurfaceFile}
didDiveIntoCorrespondingFilePatch={this.diveIntoCorrespondingFilePatch}
attemptHunkStageOperation={this.attemptHunkStageOperation}
hunks={hunks}
filePath={filePath}
stagingStatus={this.props.stagingStatus}
isPartiallyStaged={this.props.isPartiallyStaged}
registerHunkView={this.props.registerHunkView}
openCurrentFile={this.openCurrentFile}
discardLines={this.props.discardLines}
undoLastDiscard={this.undoLastDiscard}
hasUndoHistory={this.props.hasUndoHistory}
/>
</div>
);
Expand Down Expand Up @@ -164,6 +171,13 @@ export default class FilePatchController {
}
}

@autobind
diveIntoCorrespondingFilePatch() {
const filePath = this.props.filePatch.getPath();
const stagingStatus = this.props.stagingStatus === 'staged' ? 'unstaged' : 'staged';
return this.props.didDiveIntoFilePath(filePath, stagingStatus, {amending: this.props.isAmending});
}

didUpdateFilePatch() {
// FilePatch was mutated so all we need to do is re-render
return etch.update(this);
Expand All @@ -187,4 +201,9 @@ export default class FilePatchController {
textEditor.setCursorBufferPosition(position);
return textEditor;
}

@autobind
undoLastDiscard() {
return this.props.undoLastDiscard(this.props.filePatch.getPath());
}
}
80 changes: 78 additions & 2 deletions lib/controllers/git-controller.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import path from 'path';

import {CompositeDisposable, Disposable, File} from 'atom';
import {CompositeDisposable, Disposable, File, TextBuffer} from 'atom';

import React from 'react';
import {autobind} from 'core-decorators';
Expand All @@ -18,11 +18,13 @@ import GitPanelController from './git-panel-controller';
import StatusBarTileController from './status-bar-tile-controller';
import ModelObserver from '../models/model-observer';
import ModelStateRegistry from '../models/model-state-registry';
import discardChangesInBuffer from '../discard-changes-in-buffer';

const nullFilePatchState = {
filePath: null,
filePatch: null,
stagingStatus: null,
partiallyStaged: null,
};

export default class GitController extends React.Component {
Expand Down Expand Up @@ -166,6 +168,7 @@ export default class GitController extends React.Component {
}

renderFilePatchController() {
const hasUndoHistory = this.hasUndoHistory();
return (
<div>
<Commands registry={this.props.commandRegistry} target="atom-workspace">
Expand All @@ -182,9 +185,15 @@ export default class GitController extends React.Component {
commandRegistry={this.props.commandRegistry}
filePatch={this.state.filePatch}
stagingStatus={this.state.stagingStatus}
isAmending={this.state.amending}
isPartiallyStaged={this.state.partiallyStaged}
onRepoRefresh={this.onRepoRefresh}
didSurfaceFile={this.surfaceFromFileAtPath}
didDiveIntoFilePath={this.diveIntoFilePatchForPath}
openFiles={this.openFiles}
discardLines={this.discardLines}
undoLastDiscard={this.undoLastDiscard}
hasUndoHistory={hasUndoHistory}
/>
</EtchWrapper>
</PaneItem>
Expand All @@ -210,9 +219,10 @@ export default class GitController extends React.Component {
if (!repository) { return null; }

const filePatch = await repository.getFilePatchForPath(filePath, {staged: stagingStatus === 'staged', amending});
const partiallyStaged = await repository.isPartiallyStaged(filePath);
return new Promise(resolve => {
if (filePatch) {
this.setState({filePath, filePatch, stagingStatus}, () => {
this.setState({filePath, filePatch, stagingStatus, partiallyStaged}, () => {
// TODO: can be better done w/ a prop?
if (activate && this.filePatchControllerPane) {
this.filePatchControllerPane.activate();
Expand Down Expand Up @@ -335,4 +345,70 @@ export default class GitController extends React.Component {
handleChangeTab(activeTab) {
this.setState({activeTab});
}

@autobind
async discardLines(lines) {
const relFilePath = this.state.filePatch.getPath();
const absfilePath = path.join(this.props.repository.getWorkingDirectoryPath(), relFilePath);
let buffer, disposable;
const isSafe = async () => {
const editor = this.props.workspace.getTextEditors().find(e => e.getPath() === absfilePath);
if (editor) {
buffer = editor.getBuffer();
if (buffer.isModified()) {
this.props.notificationManager.addError('Cannot discard lines.', {description: 'You have unsaved changes.'});
Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch!

return false;
}
} else {
buffer = new TextBuffer({filePath: absfilePath, load: true});
await new Promise(resolve => {
disposable = buffer.onDidReload(() => {
disposable.dispose();
resolve();
});
});
}
return true;
};
await this.props.repository.storeBeforeAndAfterBlobs(relFilePath, isSafe, () => {
this.discardChangesInBuffer(buffer, this.state.filePatch, lines);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Potential race condition here with user interaction. If we create a text buffer for an unopened file, and the user modified the file, when we save the buffer at the end of discardChangesInBuffer we could overwrite their changes.

To mitigate this risk, we can take the before snapshot before checking if the buffer is modified, bail out if it is, and then discard changes and take the after snapshot. That way the safety check and the destructive operation are right next to each other leaving less room for race issues.

});
if (disposable) { disposable.dispose(); }
}

discardChangesInBuffer(buffer, filePatch, lines) {
discardChangesInBuffer(buffer, filePatch, lines);
}

@autobind
async undoLastDiscard(filePath) {
const relFilePath = this.state.filePatch.getPath();
const absfilePath = path.join(this.props.repository.getWorkingDirectoryPath(), relFilePath);
const isSafe = () => {
const editor = this.props.workspace.getTextEditors().find(e => e.getPath() === absfilePath);
if (editor && editor.getBuffer().isModified()) {
this.props.notificationManager.addError(
'Cannot undo last discard.',
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like there's some duplication here that could be refactored out, maybe an ensureBufferUnmodified() method or some such.

{description: 'You have unsaved changes.'},
);
return false;
}
return true;
};
try {
await this.props.repository.attemptToRestoreBlob(filePath, isSafe);
} catch (e) {
if (e.message === 'Cannot restore file. Contents have been modified since last discard.') {
Copy link
Contributor

Choose a reason for hiding this comment

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

Rather than key on the exact error message here I'd personally add another attribute to the Error object you could test for:

const e = new Error('Cannot undo last discard.');
e.userFacing = true;
throw e;
try {
} catch (e) {
  if (e.isUserFacing) {
    // ...
  }

Feels a little less fragile to me. If we change the wording of the error message later -- or introduce some internationalization -- we don't need to update it in two places.

this.props.notificationManager.addError(
'Cannot undo last discard.',
{description: 'Contents have been modified since last discard.'},
);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, also, this drops any other errors on the floor completely. Maybe at least console.error() them until we settle on a more consistent error-handling experience... ?

}
}

@autobind
hasUndoHistory() {
return this.props.repository.hasUndoHistory(this.state.filePatch.getPath());
}
}
35 changes: 35 additions & 0 deletions lib/discard-changes-in-buffer.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
export default function discardChangesInBuffer(buffer, filePatch, discardedLines) {
buffer.transact(() => {
let addedCount = 0;
let removedCount = 0;
let deletedCount = 0;
filePatch.getHunks().forEach(hunk => {
Copy link
Contributor

Choose a reason for hiding this comment

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

chipmunk hunk?!?!

hunk.getLines().forEach(line => {
if (discardedLines.has(line)) {
if (line.status === 'deleted') {
const row = (line.oldLineNumber - deletedCount) + addedCount - removedCount - 1;
buffer.insert([row, 0], line.text + '\n');
addedCount++;
} else if (line.status === 'added') {
const row = line.newLineNumber + addedCount - removedCount - 1;
if (buffer.lineForRow(row) === line.text) {
buffer.deleteRow(row);
removedCount++;
} else {
throw new Error(buffer.lineForRow(row) + ' does not match ' + line.text);
}
} else if (line.status === 'nonewline') {
// TODO: handle no new line case
} else {
throw new Error(`unrecognized status: ${line.status}. Must be 'added' or 'deleted'`);
}
}
if (line.getStatus() === 'deleted') {
deletedCount++;
}
});
});
});

buffer.save();
}
29 changes: 27 additions & 2 deletions lib/git-shell-out-strategy.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import {parse as parseDiff} from 'what-the-diff';

import GitPromptServer from './git-prompt-server';
import AsyncQueue from './async-queue';
import {readFile, fsStat, deleteFileOrFolder} from './helpers';
import {readFile, writeFile, fsStat, deleteFileOrFolder} from './helpers';

const LINE_ENDING_REGEX = /\r?\n/;

Expand Down Expand Up @@ -36,7 +36,7 @@ export default class GitShellOutStrategy {
}

// Execute a command and read the output using the embedded Git environment
exec(args, stdin = null, useGitPromptServer = false) {
exec(args, stdin = null, useGitPromptServer = false, redirectFilePath = null) {
/* eslint-disable no-console */
const subscriptions = new CompositeDisposable();
return this.commandQueue.push(async () => {
Expand Down Expand Up @@ -99,6 +99,9 @@ export default class GitShellOutStrategy {
err.command = formattedArgs;
return Promise.reject(err);
}
if (redirectFilePath) {
return writeFile(redirectFilePath, stdout);
}
return stdout;
});
});
Expand Down Expand Up @@ -268,6 +271,19 @@ export default class GitShellOutStrategy {
return rawDiffs[0];
}

async isPartiallyStaged(filePath) {
const args = ['status', '--short', '--', filePath];
const output = await this.exec(args);
const results = output.trim().split(LINE_ENDING_REGEX);
if (results.length === 2) {
return true;
} else if (results.length === 1) {
return ['MM', 'AM', 'MD'].includes(results[0].slice(0, 2));
} else {
throw new Error(`Unexpected output for ${args.join(' ')}: ${output}`);
}
}

/**
* Miscellaneous getters
*/
Expand Down Expand Up @@ -445,6 +461,15 @@ export default class GitShellOutStrategy {
return [];
}
}

async createBlob(filePath) {
const output = await this.exec(['hash-object', '-w', filePath]);
Copy link
Contributor

Choose a reason for hiding this comment

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

So convenient ✨

return output.trim();
}

async restoreBlob(filePath, sha) {
await this.exec(['cat-file', '-p', sha], null, null, path.join(this.workingDir, filePath));
}
}

function buildAddedFilePatch(filePath, contents, stats) {
Expand Down
8 changes: 8 additions & 0 deletions lib/helpers.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,14 @@ export function readFile(absoluteFilePath, encoding = 'utf8') {
});
}

export function writeFile(absoluteFilePath, contents) {
return new Promise((resolve, reject) => {
fs.writeFile(absoluteFilePath, contents, err => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I could swear I saw a pull request on node.js that would make fs.writeFile and friends return Promises automatically if you omitted the callback, but now I can't find it. It's certainly not in Node 6.5.0, though.

Fake edit: here it is; nodejs/node#5020. Looks like it won't be in Node 6 in any case.

if (err) { return reject(err); } else { return resolve(); }
});
});
}

export function deleteFileOrFolder(path) {
return new Promise((resolve, reject) => {
fs.remove(path, err => {
Expand Down
39 changes: 39 additions & 0 deletions lib/models/file-discard-history.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
export default class FileDiscardHistory {
constructor(createBlob, restoreBlob) {
this.createBlob = createBlob;
this.restoreBlob = restoreBlob;
this.blobHistoryByFilePath = new Map();
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, these will vanish if you relaunch Atom. Any way we could store them in the serialized package state, or maybe .git/config or something, so we don't lose track of the SHA mappings?

The specific scenario where this would break would be:

  1. Discard changes
  2. Close and re-open Atom
  3. Invoke the undo command

}

async storeBlobs(filePath, isSafe, destructiveAction) {
const beforeSha = await this.createBlob(filePath);
const isNotSafe = !(await isSafe());
if (isNotSafe) { return; }
destructiveAction();
const afterSha = await this.createBlob(filePath);
const snapshots = {beforeSha, afterSha};
const history = this.blobHistoryByFilePath.get(filePath);
if (history) {
history.push(snapshots);
} else {
this.blobHistoryByFilePath.set(filePath, [snapshots]);
}
}

async attemptToRestoreBlob(filePath, isSafe) {
const history = this.blobHistoryByFilePath.get(filePath);
const {beforeSha, afterSha} = history[history.length - 1];
const currentSha = await this.createBlob(filePath);
if (currentSha === afterSha && isSafe()) {
await this.restoreBlob(filePath, beforeSha);
history.pop();
} else {
throw new Error('Cannot restore file. Contents have been modified since last discard.');
}
}

hasUndoHistory(filePath) {
const history = this.blobHistoryByFilePath.get(filePath);
return !!history && history.length > 0;
}
}
Loading