Skip to content

Commit

Permalink
Merge pull request desktop#19626 from desktop/merge-tree-perf
Browse files Browse the repository at this point in the history
Accurately calculate number of conflicted files in a merge
  • Loading branch information
niik authored Dec 9, 2024
2 parents 9d14ae6 + fb15151 commit c6d71b3
Show file tree
Hide file tree
Showing 214 changed files with 100 additions and 703,803 deletions.
1 change: 1 addition & 0 deletions app/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@
"memoize-one": "^4.0.3",
"mri": "^1.1.0",
"p-limit": "^2.2.0",
"p-memoize": "^7.1.1",
"primer-support": "^4.0.0",
"prop-types": "^15.7.2",
"quick-lru": "^3.0.0",
Expand Down
10 changes: 10 additions & 0 deletions app/src/lib/git/core.ts
Original file line number Diff line number Diff line change
Expand Up @@ -157,6 +157,16 @@ export class GitError extends Error {
}
}

export const isGitError = (
e: unknown,
parsedError?: DugiteError
): e is GitError => {
return (
e instanceof GitError &&
(parsedError === undefined || e.result.gitError === parsedError)
)
}

/**
* Shell out to git with the given arguments, at the given path.
*
Expand Down
132 changes: 28 additions & 104 deletions app/src/lib/git/merge-tree.ts
Original file line number Diff line number Diff line change
@@ -1,117 +1,41 @@
import byline from 'byline'
import { Branch } from '../../models/branch'
import { ComputedAction } from '../../models/computed-action'
import { MergeTreeResult } from '../../models/merge'
import { Repository } from '../../models/repository'
import { isErrnoException } from '../errno-exception'
import { getMergeBase } from './merge'
import { spawnGit } from './spawn'

// the merge-tree output is a collection of entries like this
//
// changed in both
// base 100644 f69fbc5c40409a1db7a3f8353bfffe46a21d6054 atom/browser/resources/mac/Info.plist
// our 100644 9094f0f7335edf833d51f688851e6a105de60433 atom/browser/resources/mac/Info.plist
// their 100644 2dd8bc646cff3869557549a39477e30022e6cfdd atom/browser/resources/mac/Info.plist
// @@ -17,9 +17,15 @@
// <key>CFBundleIconFile</key>
// <string>electron.icns</string>
// <key>CFBundleVersion</key>
// +<<<<<<< .our
// <string>4.0.0</string>
// <key>CFBundleShortVersionString</key>
// <string>4.0.0</string>
// +=======
// + <string>1.4.16</string>
// + <key>CFBundleShortVersionString</key>
// + <string>1.4.16</string>
// +>>>>>>> .their
// <key>LSApplicationCategoryType</key>
//<string>public.app-category.developer-tools</string>
// <key>LSMinimumSystemVersion</key>

// The first line for each entry is what I'm referring to as the the header
// This regex filters on the known entries that can appear
const contextHeaderRe =
/^(merged|added in remote|removed in remote|changed in both|removed in local|added in both)$/

const conflictMarkerRe = /^\+[<>=]{7}$/
import { git, isGitError } from './core'
import { GitError } from 'dugite'

export async function determineMergeability(
repository: Repository,
ours: Branch,
theirs: Branch
): Promise<MergeTreeResult> {
const mergeBase = await getMergeBase(repository, ours.tip.sha, theirs.tip.sha)

if (mergeBase === null) {
return { kind: ComputedAction.Invalid }
}

if (mergeBase === ours.tip.sha || mergeBase === theirs.tip.sha) {
return { kind: ComputedAction.Clean }
}

const process = await spawnGit(
['merge-tree', mergeBase, ours.tip.sha, theirs.tip.sha],
) {
return git(
[
'merge-tree',
'--write-tree',
'--name-only',
'--no-messages',
'-z',
ours.tip.sha,
theirs.tip.sha,
],
repository.path,
'mergeTree'
'determineMergeability',
{ successExitCodes: new Set([0, 1]) }
)

return await new Promise<MergeTreeResult>((resolve, reject) => {
const mergeTreeResultPromise: Promise<MergeTreeResult> =
process.stdout !== null
? parseMergeTreeResult(process.stdout)
: Promise.reject(new Error('Failed reading merge-tree output'))

// If this is an exception thrown by Node.js while attempting to
// spawn let's keep the salient details but include the name of
// the operation.
process.on('error', e =>
reject(
isErrnoException(e) ? new Error(`merge-tree failed: ${e.code}`) : e
)
)

process.on('exit', code => {
if (code !== 0) {
reject(new Error(`merge-tree exited with code '${code}'`))
} else {
mergeTreeResultPromise.then(resolve, reject)
}
.then<MergeTreeResult>(({ stdout }) => {
// The output will be "<tree-id>\0[<filename>\0]*" so we can get the
// number of conflicted files by counting the number of null bytes and
// subtracting one for the tree id.
const conflictedFiles = (stdout.match(/\0/g)?.length ?? 0) - 1
return conflictedFiles > 0
? { kind: ComputedAction.Conflicts, conflictedFiles }
: { kind: ComputedAction.Clean }
})
})
}

export function parseMergeTreeResult(stream: NodeJS.ReadableStream) {
return new Promise<MergeTreeResult>(resolve => {
let seenConflictMarker = false
let conflictedFiles = 0

stream
.pipe(byline())
.on('data', (line: string) => {
// New header means new file, reset conflict flag and record if we've
// seen a conflict in this file or not
if (contextHeaderRe.test(line)) {
if (seenConflictMarker) {
conflictedFiles++
seenConflictMarker = false
}
} else if (conflictMarkerRe.test(line)) {
seenConflictMarker = true
}
})
.on('end', () => {
if (seenConflictMarker) {
conflictedFiles++
}

resolve(
conflictedFiles > 0
? { kind: ComputedAction.Conflicts, conflictedFiles }
: { kind: ComputedAction.Clean }
)
})
})
.catch<MergeTreeResult>(e =>
isGitError(e, GitError.CannotMergeUnrelatedHistories)
? Promise.resolve({ kind: ComputedAction.Invalid })
: Promise.reject(e)
)
}
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,12 @@ export function canStartOperation(
return false
}

// We can always start if there are conflicts, we'll just
// have to deal with the conflicts post the operation
if (statusKind === ComputedAction.Conflicts) {
return true
}

// Are there even commits to operate on?
if (commitCount === undefined || commitCount === 0) {
return false
Expand Down
Original file line number Diff line number Diff line change
@@ -1,8 +1,7 @@
import React from 'react'
import { getAheadBehind, revSymmetricDifference } from '../../../lib/git'
import { determineMergeability } from '../../../lib/git/merge-tree'
import { promiseWithMinimumTimeout } from '../../../lib/promise'
import { Branch } from '../../../models/branch'
import { Branch, IAheadBehind } from '../../../models/branch'
import { ComputedAction } from '../../../models/computed-action'
import { MergeTreeResult } from '../../../models/merge'
import { MultiCommitOperationKind } from '../../../models/multi-commit-operation'
Expand All @@ -14,6 +13,19 @@ import {
canStartOperation,
} from './base-choose-branch-dialog'
import { truncateWithEllipsis } from '../../../lib/truncate-with-ellipsis'
import QuickLRU from 'quick-lru'
import pMemoize from 'p-memoize'

const mergeTreeCache = pMemoize(determineMergeability, {
cache: new QuickLRU<string, MergeTreeResult>({ maxSize: 250 }),
cacheKey: ([_, ours, theirs]: Parameters<typeof determineMergeability>) =>
`${ours.tip.sha} <- ${theirs.tip.sha}`,
})

const aheadBehindCache = pMemoize(getAheadBehind, {
cache: new QuickLRU<string, IAheadBehind | null>({ maxSize: 250 }),
cacheKey: ([_, range]: Parameters<typeof getAheadBehind>) => range,
})

interface IMergeChooseBranchDialogState {
readonly commitCount: number
Expand Down Expand Up @@ -69,14 +81,18 @@ export class MergeChooseBranchDialog extends React.Component<
}

private onSelectionChanged = (selectedBranch: Branch | null) => {
this.setState({ selectedBranch })

if (selectedBranch === null) {
this.setState({ commitCount: 0, mergeStatus: null })
return
this.setState({ selectedBranch, commitCount: 0, mergeStatus: null })
} else {
this.setState(
{
selectedBranch,
commitCount: 0,
mergeStatus: { kind: ComputedAction.Loading },
},
() => this.updateStatus(selectedBranch)
)
}

this.updateStatus(selectedBranch)
}

private getDialogTitle = () => {
Expand All @@ -97,22 +113,23 @@ export class MergeChooseBranchDialog extends React.Component<

private updateStatus = async (branch: Branch) => {
const { currentBranch, repository } = this.props
this.setState({
commitCount: 0,
mergeStatus: { kind: ComputedAction.Loading },
})

const mergeStatus = await promiseWithMinimumTimeout(
() => determineMergeability(repository, currentBranch, branch),
500
const mergeStatus = await mergeTreeCache(
repository,
currentBranch,
branch
).catch<MergeTreeResult>(e => {
log.error('Failed determining mergeability', e)
return { kind: ComputedAction.Clean }
})

// The user has selected a different branch since we started, so don't
// update the preview with stale data.
if (this.state.selectedBranch !== branch) {
// The user has selected a different branch since we started or the branch
// has changed, so don't update the preview with stale data.
//
// We don't have to check if the state changed from underneath us if we
// loaded the status from cache, because that means we never kicked off an
// async operation.
if (this.state.selectedBranch?.tip.sha !== branch.tip.sha) {
return
}

Expand All @@ -125,10 +142,10 @@ export class MergeChooseBranchDialog extends React.Component<
// Commit count is used in the UI output as well as determining whether the
// submit button is enabled
const range = revSymmetricDifference('', branch.name)
const aheadBehind = await getAheadBehind(this.props.repository, range)
const aheadBehind = await aheadBehindCache(repository, range)
const commitCount = aheadBehind ? aheadBehind.behind : 0

if (this.state.selectedBranch !== branch) {
if (this.state.selectedBranch.tip.sha !== branch.tip.sha) {
return
}

Expand Down
Loading

0 comments on commit c6d71b3

Please sign in to comment.