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

Show files as conflicted in git panel when discard undo creates merge conflicts, handle various edge cases #563

Merged
merged 32 commits into from Mar 3, 2017
Merged
Show file tree
Hide file tree
Changes from 18 commits
Commits
Show all changes
32 commits
Select commit Hold shift + click to select a range
ca3687e
Add test for GSOS#mergeFile
kuychaco Feb 23, 2017
0763353
Implement GSOS#getFileMode(fileName)
kuychaco Feb 24, 2017
c35db7b
Implement GSOS#updateIndex
kuychaco Feb 24, 2017
5daf3a5
Handle null oursSha or theirSha in updateIndex
kuychaco Feb 24, 2017
84d451b
Update index for conflicts on restoration of discarded changes
kuychaco Feb 24, 2017
97c4d4d
Merge remote-tracking branch 'origin/master' into ku-show-conflicts-i…
kuychaco Feb 24, 2017
bdc9de2
Use consistent ours/theirs/commonBase language
kuychaco Feb 24, 2017
61eaf73
Add test for GitController#discardWorkDirChangesForPaths
kuychaco Feb 24, 2017
1d5364a
Fix test description
kuychaco Feb 24, 2017
79a6487
:fire: empty line
kuychaco Feb 24, 2017
b52b602
Pass tooltips to GitController in tests
kuychaco Feb 24, 2017
65858ee
:fire: unused option from GSOS#diffFileStatus
kuychaco Feb 24, 2017
4cb9d0c
:art: use git.exec rather than git.diffFileStatus
kuychaco Feb 24, 2017
352b9af
:art: test to use getFileContents helper function
kuychaco Feb 24, 2017
a27ed97
Add `~` to beginning of filename for merge results
kuychaco Feb 24, 2017
e9c46bf
More specific error catching in GSOS#createBlob
kuychaco Feb 25, 2017
22458df
Handle case of deleted files
kuychaco Feb 25, 2017
862c0a5
:shirt:
kuychaco Feb 27, 2017
d8686e1
Add tests for undoing discard in an added file
kuychaco Feb 27, 2017
8ee9dd2
Make GSOS#checkoutFiles no longer delete untracked files
kuychaco Mar 3, 2017
79f901d
Add isFileExecutable helper
kuychaco Mar 3, 2017
b950c38
:art: GSOS#getDiffForFilePath to use isFileExecutable helper
kuychaco Mar 3, 2017
b247137
Make GSOS#getFileMode handle untracked files
kuychaco Mar 3, 2017
cd91dcd
Make GSOS#updateIndex handle the case when commonBaseSha is null
kuychaco Mar 3, 2017
0440a0c
Make DiscardHistory#mergeFiles handle case of added files
kuychaco Mar 3, 2017
bc4477e
Merge remote-tracking branch 'origin/master' into ku-show-conflicts-i…
kuychaco Mar 3, 2017
d29cd8a
:shirt:
kuychaco Mar 3, 2017
73941ff
Make GSOS#updateIndex a write operation
kuychaco Mar 3, 2017
65c38b5
Rename `updateIndex` -> `writeMergeConflictToIndex`
kuychaco Mar 3, 2017
867f9b2
Use `createTestStrategy` instead of instantiating GitShellOutStrategy
kuychaco Mar 3, 2017
56f158c
Make getFileMode test work on windows
kuychaco Mar 3, 2017
0003447
Fix bug undoing discard for file in subdirectory (fixes #588)
kuychaco Mar 3, 2017
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
16 changes: 12 additions & 4 deletions lib/controllers/git-controller.js
Expand Up @@ -21,7 +21,7 @@ import RepositoryConflictController from './repository-conflict-controller';
import ModelObserver from '../models/model-observer';
import ModelStateRegistry from '../models/model-state-registry';
import Conflict from '../models/conflicts/conflict';
import {copyFile} from '../helpers';
import {copyFile, deleteFileOrFolder} from '../helpers';
import {GitError} from '../git-shell-out-strategy';

const nullFilePatchState = {
Expand Down Expand Up @@ -413,7 +413,7 @@ export default class GitController extends React.Component {
};
return await this.props.repository.storeBeforeAndAfterBlobs(
filePaths,
() => this.ensureNoUnsavedFiles(filePaths, 'Cannot discard lines.'),
() => this.ensureNoUnsavedFiles(filePaths, 'Cannot discard changes in selected files.'),
destructiveAction,
);
}
Expand Down Expand Up @@ -501,9 +501,17 @@ export default class GitController extends React.Component {

async proceedWithLastDiscardUndo(results, partialDiscardFilePath = null) {
const promises = results.map(async result => {
const {filePath, resultPath} = result;
const {filePath, resultPath, deleted, conflict, theirsSha, commonBaseSha} = result;
const absFilePath = path.join(this.props.repository.getWorkingDirectoryPath(), filePath);
await copyFile(resultPath, absFilePath);
if (deleted && resultPath === null) {
await deleteFileOrFolder(absFilePath);
} else {
await copyFile(resultPath, absFilePath);
}
if (conflict) {
const currentSha = await this.props.repository.createBlob({filePath});
await this.props.repository.updateIndex(filePath, commonBaseSha, currentSha, theirsSha);
}
});
await Promise.all(promises);
await this.props.repository.popDiscardHistory(partialDiscardFilePath);
Expand Down
4 changes: 2 additions & 2 deletions lib/controllers/repository-conflict-controller.js
Expand Up @@ -86,9 +86,9 @@ export default class RepositoryConflictController extends React.Component {
return [];
}

const basePath = this.props.workingDirectoryPath;
const commonBasePath = this.props.workingDirectoryPath;
const fullMergeConflictPaths = new Set(
this.props.mergeConflictPaths.map(relativePath => path.join(basePath, relativePath)),
this.props.mergeConflictPaths.map(relativePath => path.join(commonBasePath, relativePath)),
);

return this.state.openEditors.filter(editor => fullMergeConflictPaths.has(editor.getPath()));
Expand Down
30 changes: 25 additions & 5 deletions lib/git-shell-out-strategy.js
Expand Up @@ -5,6 +5,7 @@ import {CompositeDisposable} from 'atom';

import {GitProcess} from 'git-kitchen-sink';
import {parse as parseDiff} from 'what-the-diff';
import dedent from 'dedent-js';

import GitPromptServer from './git-prompt-server';
import AsyncQueue from './async-queue';
Expand Down Expand Up @@ -243,7 +244,6 @@ export default class GitShellOutStrategy {
const args = ['diff', '--name-status', '--no-renames'];
if (options.staged) { args.push('--staged'); }
if (options.target) { args.push(options.target); }
if (options.diffFilter === 'unmerged') { args.push('--diff-filter=U'); }
const output = await this.exec(args);

const statusMap = {
Expand Down Expand Up @@ -508,7 +508,11 @@ export default class GitShellOutStrategy {
try {
output = (await this.exec(['hash-object', '-w', filePath], {writeOperation: true})).trim();
} catch (e) {
output = null;
if (e.stdErr.match(/fatal: Cannot open .*: No such file or directory/)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

output = null;
} else {
throw e;
}
}
} else if (stdin) {
output = (await this.exec(['hash-object', '-w', '--stdin'], {stdin, writeOperation: true})).trim();
Expand All @@ -528,9 +532,9 @@ export default class GitShellOutStrategy {
return await this.exec(['cat-file', '-p', sha]);
}

async mergeFile(currentPath, basePath, otherPath, resultPath) {
async mergeFile(oursPath, commonBasePath, theirsPath, resultPath) {
const args = [
'merge-file', '-p', currentPath, basePath, otherPath,
'merge-file', '-p', oursPath, commonBasePath, theirsPath,
'-L', 'current', '-L', 'after discard', '-L', 'before discard',
];
let output;
Expand All @@ -546,7 +550,23 @@ export default class GitShellOutStrategy {
}
}
await writeFile(resultPath, output);
return {filePath: currentPath, resultPath, conflict};
return {filePath: oursPath, resultPath, conflict};
}

async updateIndex(filePath, commonBaseSha, oursSha, theirsSha) {
const fileMode = await this.getFileMode(filePath);
let indexInfo = dedent`
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice. I was wondering if there was a library for this but I hadn't bothered to actually look for one yet 😁

0 0000000000000000000000000000000000000000\t${filePath}
${fileMode} ${commonBaseSha} 1\t${filePath}\n
`;
if (oursSha) { indexInfo += `${fileMode} ${oursSha} 2\t${filePath}\n`; }
if (theirsSha) { indexInfo += `${fileMode} ${theirsSha} 3\t${filePath}\n`; }
return this.exec(['update-index', '--index-info'], {stdin: indexInfo});
Copy link
Contributor

Choose a reason for hiding this comment

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

This should probably be a write op so it doesn't run in parallel with other operations.

}

async getFileMode(filePath) {
const output = await this.exec(['ls-files', '--stage', '--', filePath]);
return output.slice(0, 6);
}
}

Expand Down
39 changes: 26 additions & 13 deletions lib/models/discard-history.js
Expand Up @@ -3,13 +3,14 @@ import os from 'os';

import {PartialFileDiscardHistory, WholeFileDiscardHistory} from './discard-history-stores';

import {getTempDir} from '../helpers';
import {getTempDir, copyFile} from '../helpers';

export default class DiscardHistory {
constructor(createBlob, expandBlobToFile, mergeFile, {maxHistoryLength} = {}) {
constructor(createBlob, expandBlobToFile, mergeFile, workdirPath, {maxHistoryLength} = {}) {
this.createBlob = createBlob;
this.expandBlobToFile = expandBlobToFile;
this.mergeFile = mergeFile;
this.workdirPath = workdirPath;
this.partialFileHistory = new PartialFileDiscardHistory(maxHistoryLength);
this.wholeFileHistory = new WholeFileDiscardHistory(maxHistoryLength);
}
Expand Down Expand Up @@ -114,25 +115,37 @@ export default class DiscardHistory {
async expandBlobsToFilesInTempFolder(snapshots) {
const tempFolderPath = await getTempDir(path.join(os.tmpdir(), 'github-discard-history-'));
const pathPromises = snapshots.map(async ({filePath, beforeSha, afterSha}) => {
const otherPath = !beforeSha ? null :
const theirsPath = !beforeSha ? null :
await this.expandBlobToFile(path.join(tempFolderPath, `${filePath}-before-discard`), beforeSha);
const basePath = !afterSha ? null :
const commonBasePath = !afterSha ? null :
await this.expandBlobToFile(path.join(tempFolderPath, `${filePath}-after-discard`), afterSha);
const resultPath = path.join(tempFolderPath, `${filePath}-merge-result`);
return {filePath, basePath, otherPath, resultPath};
const resultPath = path.join(tempFolderPath, `~${filePath}-merge-result`);
return {filePath, commonBasePath, theirsPath, resultPath, theirsSha: beforeSha, commonBaseSha: afterSha};
});
return await Promise.all(pathPromises);
}

async mergeFiles(filePaths) {
const mergeFilePromises = filePaths.map(({filePath, basePath, otherPath, resultPath}) => {
if (otherPath && basePath) {
return this.mergeFile(filePath, basePath, otherPath, resultPath);
} else if (!otherPath && basePath) { // deleted file
// TODO: handle this
return null;
const mergeFilePromises = filePaths.map(async (filePathInfo, i) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

This logic is starting to feel a little hairy again. Probably okay to merge, but I continue to wonder if there are more extractions we can do, especially around object modeling. E.g. would it make sense to have a Merge object? Or utilize Conflict-style objects like in @smashwilson's conflict resolution stuff?

Copy link
Contributor

Choose a reason for hiding this comment

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

We couldn't use Conflict directly here (that's a conflict marker within a file, not an entire file that contains conflict markers). Introducing some kind of model for conflicted files (ConflictedFile?) might be worthwhile. Maybe we could even refactor some of the conflict resolution stuff to use it too.

const {filePath, commonBasePath, theirsPath, resultPath, theirsSha, commonBaseSha} = filePathInfo;
let mergeResult;
if (theirsPath && commonBasePath) {
mergeResult = await this.mergeFile(filePath, commonBasePath, theirsPath, resultPath);
} else if (!theirsPath && commonBasePath) { // deleted file
const oursSha = await this.createBlob({filePath});
if (oursSha === commonBaseSha) { // no changes since discard, mark file to be deleted
mergeResult = {filePath, resultPath: null, deleted: true, conflict: false};
} else { // changes since discard result in conflict
await copyFile(path.join(this.workdirPath, filePath), resultPath);
mergeResult = {filePath, resultPath, conflict: true};
}
} else if (theirsPath && !commonBasePath) { // added file
// TODO
Copy link
Contributor

Choose a reason for hiding this comment

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

What will happen if this PR gets merged before this is implemented?

} else {
throw new Error('One of the following must be defined - theirsPath:' +
`${theirsPath} or commonBasePath: ${commonBasePath}`);
}
return null;
return {...mergeResult, theirsSha, commonBaseSha};
});
return await Promise.all(mergeFilePromises);
}
Expand Down
9 changes: 7 additions & 2 deletions lib/models/repository.js
Expand Up @@ -74,6 +74,7 @@ export default class Repository {
this.createBlob,
this.expandBlobToFile,
this.mergeFile,
this.workingDirectoryPath,
{maxHistoryLength: 60},
);
if (history) { this.discardHistory.updateHistory(history); }
Expand Down Expand Up @@ -428,8 +429,12 @@ export default class Repository {
}

@autobind
mergeFile(currentPath, basePath, otherPath, resultPath) {
return this.git.mergeFile(currentPath, basePath, otherPath, resultPath);
mergeFile(oursPath, commonBasePath, theirsPath, resultPath) {
return this.git.mergeFile(oursPath, commonBasePath, theirsPath, resultPath);
}

updateIndex(filePath, commonBaseSha, oursSha, theirsSha) {
return this.git.updateIndex(filePath, commonBaseSha, oursSha, theirsSha);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a better method name than updateIndex for this? Yes, we're updating the index, but in a specific way. Perhaps writeMergeConflictToIndex or similar?

}

async createDiscardHistoryBlob() {
Expand Down
4 changes: 4 additions & 0 deletions lib/views/push-pull-menu-view.js
Expand Up @@ -11,6 +11,10 @@ export default class PushPullMenuView extends React.Component {
remoteName: React.PropTypes.string,
aheadCount: React.PropTypes.number,
behindCount: React.PropTypes.number,
notificationManager: React.PropTypes.object,
fetch: React.PropTypes.func,
push: React.PropTypes.func,
pull: React.PropTypes.func,
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a note for me to remove this same change in #565

}

static defaultProps = {
Expand Down