Show files as conflicted in git panel when discard undo creates merge conflicts, handle various edge cases by kuychaco · Pull Request #563 · atom/github

kuychaco · 2017-02-24T03:00:47Z

Display files under conflict in Merge Conflicts list (fixes #559)
Handle removed and added file edge cases (fixes #543)
Fix bug undoing discard for file in subdirectory (fixes #588)

…n-git-panel

For quick identificatin of files opened in editor that are previews of conflicted files

BinaryMuse · 2017-02-27T03:04:29Z

+          mergeResult = {filePath, resultPath, conflict: true};
+        }
+      } else if (theirsPath && !commonBasePath) { // added file
+        // TODO


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

BinaryMuse · 2017-02-27T03:04:34Z

+    `;
+    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});


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

BinaryMuse · 2017-02-27T03:06:20Z

-      } else if (!otherPath && basePath) { // deleted file
-        // TODO: handle this
-        return null;
+    const mergeFilePromises = filePaths.map(async (filePathInfo, i) => {


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?

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.

BinaryMuse · 2017-02-27T03:07:19Z

+  }
+
+  updateIndex(filePath, commonBaseSha, oursSha, theirsSha) {
+    return this.git.updateIndex(filePath, commonBaseSha, oursSha, theirsSha);


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?

BinaryMuse · 2017-02-27T03:07:51Z

+    notificationManager: React.PropTypes.object,
+    fetch: React.PropTypes.func,
+    push: React.PropTypes.func,
+    pull: React.PropTypes.func,


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

BinaryMuse · 2017-02-27T03:08:09Z

        commandRegistry={commandRegistry}
        notificationManager={notificationManager}
        confirm={confirm}
+        tooltips={tooltips}


Another note for me to remove this in #565

😆 Yeah we all fixed this independently in our own pull requests.

smashwilson

Just a few bits to add.

Also: today I learned how git update-index works. That's pretty cool 😁

smashwilson · 2017-02-27T19:59:59Z

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


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

smashwilson · 2017-02-27T20:04:04Z

-      } else if (!otherPath && basePath) { // deleted file
-        // TODO: handle this
-        return null;
+    const mergeFilePromises = filePaths.map(async (filePathInfo, i) => {


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.

smashwilson · 2017-02-27T20:05:18Z

+    describe('getFileMode(filePath)', () => {
+      it('returns the file mode of the specified file', async () => {
+        const workingDirPath = await cloneRepository('three-files');
+        const git = new GitShellOutStrategy(workingDirPath);


Should these use createTestStrategy?

smashwilson · 2017-02-27T20:06:11Z

        commandRegistry={commandRegistry}
        notificationManager={notificationManager}
        confirm={confirm}
+        tooltips={tooltips}


😆 Yeah we all fixed this independently in our own pull requests.

smashwilson · 2017-02-27T20:07:55Z

+          workdirPath = await cloneRepository('three-files');
          repository = await buildRepository(workdirPath);

+          getFileContents = filePath => {


Worth adapting into the readFile helper maybe?

This responsibility is a better fit for Repository#discardWorkDirChangesForPaths

…n-git-panel

smashwilson · 2017-03-03T13:01:55Z

        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/)) {


kuychaco added 6 commits February 22, 2017 18:57

Add test for GSOS#mergeFile

ca3687e

Implement GSOS#getFileMode(fileName)

0763353

Implement GSOS#updateIndex

c35db7b

Handle null oursSha or theirSha in updateIndex

5daf3a5

Update index for conflicts on restoration of discarded changes

84d451b

Merge remote-tracking branch 'origin/master' into ku-show-conflicts-i…

97c4d4d

…n-git-panel

kuychaco force-pushed the ku-show-conflicts-in-git-panel branch from 18438f1 to 97c4d4d Compare February 24, 2017 21:05

kuychaco added 11 commits February 24, 2017 13:28

Use consistent ours/theirs/commonBase language

bdc9de2

Add test for GitController#discardWorkDirChangesForPaths

61eaf73

Fix test description

1d5364a

🔥 empty line

79a6487

Pass tooltips to GitController in tests

b52b602

🔥 unused option from GSOS#diffFileStatus

65858ee

🎨 use git.exec rather than git.diffFileStatus

4cb9d0c

🎨 test to use getFileContents helper function

352b9af

Add ~ to beginning of filename for merge results

a27ed97

For quick identificatin of files opened in editor that are previews of conflicted files

More specific error catching in GSOS#createBlob

e9c46bf

Handle case of deleted files

22458df

kuychaco force-pushed the ku-show-conflicts-in-git-panel branch from 9e5c0f6 to 22458df Compare February 27, 2017 01:34

kuychaco added 2 commits February 26, 2017 17:42

👕

862c0a5

Add tests for undoing discard in an added file

d8686e1

kuychaco changed the title ~~Show files as conflicted in git panel when discard undo creates merge conflicts~~ Show files as conflicted in git panel when discard undo creates merge conflicts, handle deleted file edge case Feb 27, 2017

kuychaco requested review from BinaryMuse and smashwilson February 27, 2017 02:36

BinaryMuse suggested changes Feb 27, 2017

View reviewed changes

smashwilson suggested changes Feb 27, 2017

View reviewed changes

kuychaco added 4 commits March 2, 2017 20:18

Make GSOS#checkoutFiles no longer delete untracked files

8ee9dd2

This responsibility is a better fit for Repository#discardWorkDirChangesForPaths

Add isFileExecutable helper

79f901d

🎨 GSOS#getDiffForFilePath to use isFileExecutable helper

b950c38

Make GSOS#getFileMode handle untracked files

b247137

kuychaco added 7 commits March 2, 2017 22:09

Make GSOS#updateIndex handle the case when commonBaseSha is null

cd91dcd

Make DiscardHistory#mergeFiles handle case of added files

0440a0c

Merge remote-tracking branch 'origin/master' into ku-show-conflicts-i…

bc4477e

…n-git-panel

👕

d29cd8a

Make GSOS#updateIndex a write operation

73941ff

Rename updateIndex -> writeMergeConflictToIndex

65c38b5

Use createTestStrategy instead of instantiating GitShellOutStrategy

867f9b2

BinaryMuse approved these changes Mar 3, 2017

View reviewed changes

kuychaco added 2 commits March 2, 2017 22:34

Make getFileMode test work on windows

56f158c

Fix bug undoing discard for file in subdirectory (fixes #588)

0003447

kuychaco changed the title ~~Show files as conflicted in git panel when discard undo creates merge conflicts, handle deleted file edge case~~ Show files as conflicted in git panel when discard undo creates merge conflicts, handle various edge cases Mar 3, 2017

smashwilson approved these changes Mar 3, 2017

View reviewed changes

kuychaco merged commit fec0f20 into master Mar 3, 2017

kuychaco deleted the ku-show-conflicts-in-git-panel branch March 3, 2017 16:56

Conversation

kuychaco commented Feb 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

smashwilson left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

kuychaco commented Feb 24, 2017 •

edited

Loading