Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve reset the workspace SCM command #1915

Merged
merged 4 commits into from
Jun 17, 2022
Merged

Conversation

mattseddon
Copy link
Member

@mattseddon mattseddon commented Jun 16, 2022

1/2 main <- this <- #1916

The second attempt at #1900 which was reverted in #1910 (reasons for the revert are inline).

I will write up the incident.

@mattseddon mattseddon added the product PR that affects product label Jun 16, 2022
@mattseddon mattseddon self-assigned this Jun 16, 2022
return this.model.getState()
return {
...this.model.getDecorationState(),
...this.model.getSourceControlManagementState()
Copy link
Member Author

Choose a reason for hiding this comment

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

[F] This function is only for testing. When I rewrite this I will get rid of the need for it.

this.sourceControlManagement.setState(
this.model.getSourceControlManagementState()
)
this.decorationProvider.setState(this.model.getDecorationState())
Copy link
Member Author

Choose a reason for hiding this comment

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

[F] Bug two.

Was a great example of trying to do something clever that turned out to be incredibly dumb.

Previous code:

export type DecorationState = Record<Status, Set<string>>

export interface DecorationModel {
  getState: () => DecorationState
}

enum Status {
  ADDED = 'added',
  DELETED = 'deleted',
  MODIFIED = 'modified',
  NOT_IN_CACHE = 'notInCache',
  RENAMED = 'renamed',
  GIT_MODIFIED = 'gitModified',
  TRACKED = 'tracked'
}

export class DecorationProvider
  extends Disposable
  implements FileDecorationProvider
{
...

  public setState(state: DecorationState) {
    const urisToUpdate = this.getUnion(this.state, state)
    this.state = state
    this.decorationsChanged.fire(urisToUpdate)
  }

  private getUnion(existingState: DecorationState, newState: DecorationState) {
    return flattenUnique([
      ...Object.values(existingState).map(status => [...(status || [])]),
      ...Object.values(newState).map(status => [...(status || [])])
    ]).map(path => Uri.file(path))
  }
export class RepositoryModel
  extends Disposable
  implements DecorationModel, SourceControlManagementModel
{
...
  public getState() {
    const acc = []
    for (const relTrackedOut of this.relTrackedOuts) {
      acc.push(join(this.dvcRoot, relTrackedOut))
    }

    return {
      ...omit(this.state, ['trackedLeafs', 'trackedNonLeafs']),
      hasRemote: new Set([...acc, ...this.state.trackedLeafs]),
      tracked: this.getTracked()
    }
  }

The way that this was setup meant that the compiler couldn't/didn't warn me that I was putting a boolean into Uri.file which causes a silently rejected promise and stopped any decorations from being provided.

I've changed the code to be very explicit in terms of what the DecorationProvider and SourceContorlManagement expect for state.

repositoryRoot: string
): Promise<boolean> => {
const output = await executeProcess({
args: ['diff', '--name-only', '-z'],
Copy link
Member Author

@mattseddon mattseddon Jun 16, 2022

Choose a reason for hiding this comment

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

[F] The first bug was here. git status touches .git/index which fired the watcher which ran git status which...

@@ -45,7 +48,10 @@ export class Repository extends DeferredDisposable {
}

public getState() {
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm going to follow up and remove this. It is only used for testing purposes. This change is big enough now though.

@mattseddon mattseddon marked this pull request as ready for review June 16, 2022 06:22
@mattseddon mattseddon requested a review from shcheklein June 16, 2022 07:07
@mattseddon mattseddon force-pushed the update-reset-workspace branch from 38fe26e to 9ae60af Compare June 17, 2022 00:12
@mattseddon mattseddon enabled auto-merge (squash) June 17, 2022 00:14
@codeclimate
Copy link

codeclimate bot commented Jun 17, 2022

Code Climate has analyzed commit 9ae60af and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 100.0% (85% is the threshold).

This pull request will bring the total coverage in the repository to 96.6% (0.0% change).

View more on Code Climate.

@mattseddon mattseddon merged commit 4b47aa8 into main Jun 17, 2022
@mattseddon mattseddon deleted the update-reset-workspace branch June 17, 2022 00:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
product PR that affects product
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants