Add metrics for completing/aborting a merge after conflicts by iAmWillShepherd · Pull Request #5810 · desktop/desktop

iAmWillShepherd · 2018-10-04T09:27:18Z

⚠️ This PR has associated work: #5394 (comment) ⚠️

Fixes #5394

How to test

Create a repo and produce some conflicts
Open that repo in Desktop (you should be notified that conflicts exist
Note the values of mergeAbortedAfterConflictsCount and mergeSuccessAfterConflictsCount from the database as n and m respectfully.

Abort

Open terminal and execute git merge --abort
Check the database: mergeAbortedAfterConflictsCount should be n + 1

Complete

Fix the conflicts
Make a commit
Check the database: mergeSuccessAfterConflictsCount should be m + 1

Co-Authored-By: William Shepherd <iamwillshepherd@users.noreply.github.com>

This is necessary because some non-standard git terminology was introduced. Add docs and clean up Lint

outofambit

thanks for taking this the last mile, @iAmWillShepherd! a few blocking requests.

how can we easily test that this works?

app/src/lib/stores/app-store.ts

+    const repoState = this.repositoryStateCache.get(repository)
+
+    // check if we're in a conflicted state
+    if (repoState.conflictState === null) {


app/src/lib/stores/app-store.ts

app/src/lib/app-state.ts

app/src/lib/stores/app-store.ts

app/src/lib/stats/stats-store.ts

  mergedWithLoadingHintCount: 0,
  mergedWithCleanMergeHintCount: 0,
  mergedWithConflictWarningHintCount: 0,
+  mergeSuccesfulAfterConflicts: 0,


outofambit

tested this out locally and i think we're good 👍

shiftkey

In case this gets merged before #5810 is clarified I've added some further feedback and assigned myself to it.

app/src/lib/app-state.ts

app/src/lib/dispatcher/error-handlers.ts

    switch (command) {
      case 'pull':
        dispatcher.recordMergeConflictFromPull()
+        dispatcher.mergeConflictDetected()


app/src/lib/stores/app-store.ts

+    const repoState = this.repositoryStateCache.get(repository)
+
+    // check if we're in a conflicted state
+    if (repoState.conflictState === null) {


iAmWillShepherd · 2018-10-15T22:11:13Z

🍏

Documents new merge success metrics

…tate Moves Conflict state into Changes state

iAmWillShepherd · 2018-10-16T16:59:45Z

🍏

shiftkey

Just a couple of suggestions

app/src/lib/stores/app-store.ts

+    const currentBranchName = status.currentBranch
+
+    // no current branch means something happened so bail
+    if (currentBranchName === undefined) {


app/src/lib/stores/app-store.ts

+        file.status === AppFileStatus.Resolved
+    )
+
+    // we are still in a conflict, so bail


app/src/lib/stores/app-store.ts

+      return
+    }
+
+    // no more conflicts!


iAmWillShepherd · 2018-10-16T20:05:04Z

💯

shiftkey

One suggestion to cleanup, but I'll let @outofambit merge this at their leisure so we can wrap this up 🎉

app/src/lib/stores/app-store.ts

+      return
+    }
+
+    const repository = selection.repository


outofambit · 2018-10-16T20:12:39Z

thanks @shiftkey! 🚀 going to merge this after tests pass so we can include this in the beta ✨

app/src/lib/stores/app-store.ts

+      return
+    }
+
+    const repository = selection.repository


ghost

#14275

outofambit and others added 7 commits October 4, 2018 04:17

wiring up stats boilerplate

6d50730

Co-Authored-By: William Shepherd <iamwillshepherd@users.noreply.github.com>

create new interface for conflict state

00b8b6b

Add function to update conflict state when a merge conflict happens

2c8f33c

determine if a merge was aborted or succesful and record it

f469914

Add conflcit state to repo state cache

cbab53e

Wire up

6d89234

Include resolved files

ed7ef71

This is necessary because some non-standard git terminology was introduced. Add docs and clean up Lint

iAmWillShepherd mentioned this pull request Oct 4, 2018

Adds stats for when a merge is completed/aborted after a merge conflict #5742

Closed

write valid code

8092cbb

iAmWillShepherd added the ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Oct 4, 2018

outofambit self-requested a review October 8, 2018 16:52

outofambit added this to the 1.4.3 milestone Oct 8, 2018

outofambit suggested changes Oct 9, 2018

View reviewed changes

outofambit modified the milestones: 1.4.2, 1.4.3 Oct 9, 2018

outofambit self-assigned this Oct 9, 2018

iAmWillShepherd commented Oct 10, 2018

View reviewed changes

app/src/lib/stats/stats-store.ts Outdated

mergedWithLoadingHintCount: 0,

mergedWithCleanMergeHintCount: 0,

mergedWithConflictWarningHintCount: 0,

mergeSuccesfulAfterConflicts: 0,

This comment was marked as spam.

Sign in to view

outofambit modified the milestone: 1.4.3 Oct 10, 2018

iAmWillShepherd added 3 commits October 11, 2018 11:24

Update docs

2b7ef81

Rename metric

38be4b4

document new metrics

71029ec

iAmWillShepherd mentioned this pull request Oct 11, 2018

Documents new merge success metrics #5879

Merged

outofambit previously approved these changes Oct 11, 2018

View reviewed changes

Add justification for post-merge conflict metrics

9669533

billygriffin mentioned this pull request Oct 11, 2018

[Metrics] Merge conflict resolution #5394

Closed

7 tasks

shiftkey self-assigned this Oct 14, 2018

shiftkey suggested changes Oct 14, 2018

View reviewed changes

iAmWillShepherd added 2 commits October 15, 2018 14:43

Extract logic into variable to aide in readability

ee4e718

Rename metric names

13673f3

Update metric names

1110cab

This comment was marked as spam.

Sign in to view

iAmWillShepherd added 3 commits October 15, 2018 15:47

Move conflictState to changesState

2b39db1

Update the right part of state

c1096be

Don't forget to update initial state

32221ce

iAmWillShepherd mentioned this pull request Oct 15, 2018

Moves Conflict state into Changes state #5901

Merged

iAmWillShepherd added 2 commits October 15, 2018 16:08

format

190bf4e

Use better name;

11da859

shiftkey and others added 4 commits October 16, 2018 09:01

Merge pull request #5879 from desktop/merge-completetion-metrics

72e5977

Documents new merge success metrics

Use updateChangeState function

35d84d1

Add comment

d7587d7

Merge branch 'did-you-merge-it' into put-conflict-state-in-changes-state

d028cc3

shiftkey mentioned this pull request Oct 16, 2018

Code Coverage Metrics #5511

Closed

iAmWillShepherd and others added 2 commits October 16, 2018 11:10

Merge functions

ab842ab

Merge pull request #5901 from desktop/put-conflict-state-in-changes-s…

8308247

…tate Moves Conflict state into Changes state

shiftkey reviewed Oct 16, 2018

View reviewed changes

iAmWillShepherd added 2 commits October 16, 2018 15:04

Move invariant to top of function

0ac8d13

Remove unnecessary comments

d900e4b

outofambit requested a review from shiftkey October 16, 2018 20:08

outofambit approved these changes Oct 16, 2018

View reviewed changes

shiftkey approved these changes Oct 16, 2018

View reviewed changes

app/src/lib/stores/app-store.ts

return

}

const repository = selection.repository

This comment was marked as spam.

Sign in to view

outofambit reviewed Oct 16, 2018

View reviewed changes

app/src/lib/stores/app-store.ts

return

}

const repository = selection.repository

This comment was marked as spam.

Sign in to view

outofambit merged commit b8a7bc2 into master Oct 16, 2018

outofambit deleted the did-you-merge-it branch October 16, 2018 22:50

ghost reviewed Apr 3, 2022

View reviewed changes

This comment was marked as off-topic.

Sign in to view

Conversation

iAmWillShepherd commented Oct 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

How to test

Abort

Complete

Uh oh!

outofambit left a comment

Choose a reason for hiding this comment

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

This comment was marked as spam.

Uh oh!

outofambit left a comment

Choose a reason for hiding this comment

Uh oh!

shiftkey left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

iAmWillShepherd commented Oct 15, 2018

Uh oh!

iAmWillShepherd commented Oct 16, 2018

Uh oh!

shiftkey left a comment

Choose a reason for hiding this comment

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

iAmWillShepherd commented Oct 16, 2018

Uh oh!

shiftkey left a comment

Choose a reason for hiding this comment

Uh oh!

This comment was marked as spam.

Uh oh!

outofambit commented Oct 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment was marked as spam.

Uh oh!

ghost left a comment

Choose a reason for hiding this comment

Uh oh!

This comment was marked as off-topic.

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

iAmWillShepherd commented Oct 4, 2018 •

edited

Loading

outofambit commented Oct 16, 2018 •

edited

Loading