From bf8f6d18622f791b6c9f63fad4eda77146133e62 Mon Sep 17 00:00:00 2001 From: Stephen Sigwart Date: Fri, 29 Sep 2023 22:37:53 -0400 Subject: [PATCH 1/5] Fix staged file renamed with changes showing no diff - Closes desktop/desktop#6014 - Fixes the case where a staged file is moved and modified. Previously, it would say it was moved with no changes. --- app/src/lib/git/log.ts | 12 ++++++++---- app/src/lib/git/status.ts | 13 ++++++++++++- app/src/lib/status-parser.ts | 11 ++++++++++- app/src/models/status.ts | 3 +++ app/src/ui/diff/index.tsx | 25 ++++++++++++++++++++----- 5 files changed, 53 insertions(+), 11 deletions(-) diff --git a/app/src/lib/git/log.ts b/app/src/lib/git/log.ts index 289c49b3cbb..83c3096700a 100644 --- a/app/src/lib/git/log.ts +++ b/app/src/lib/git/log.ts @@ -69,20 +69,24 @@ function mapStatus( return { kind: AppFileStatusKind.Deleted, submoduleStatus } } // deleted if (status === 'R' && oldPath != null) { - return { kind: AppFileStatusKind.Renamed, oldPath, submoduleStatus } + const renameIncludesModifications = false; + return { kind: AppFileStatusKind.Renamed, oldPath, submoduleStatus, renameIncludesModifications } } // renamed if (status === 'C' && oldPath != null) { - return { kind: AppFileStatusKind.Copied, oldPath, submoduleStatus } + const renameIncludesModifications = false; + return { kind: AppFileStatusKind.Copied, oldPath, submoduleStatus, renameIncludesModifications } } // copied // git log -M --name-status will return a RXXX - where XXX is a percentage if (status.match(/R[0-9]+/) && oldPath != null) { - return { kind: AppFileStatusKind.Renamed, oldPath, submoduleStatus } + const renameIncludesModifications = (status !== "R100"); + return { kind: AppFileStatusKind.Renamed, oldPath, submoduleStatus, renameIncludesModifications } } // git log -C --name-status will return a CXXX - where XXX is a percentage if (status.match(/C[0-9]+/) && oldPath != null) { - return { kind: AppFileStatusKind.Copied, oldPath, submoduleStatus } + const renameIncludesModifications = false; + return { kind: AppFileStatusKind.Copied, oldPath, submoduleStatus, renameIncludesModifications } } return { kind: AppFileStatusKind.Modified, submoduleStatus } diff --git a/app/src/lib/git/status.ts b/app/src/lib/git/status.ts index acc20a292f6..94b2b3588c4 100644 --- a/app/src/lib/git/status.ts +++ b/app/src/lib/git/status.ts @@ -155,12 +155,23 @@ function convertToAppStatus( kind: AppFileStatusKind.Copied, oldPath, submoduleStatus: entry.submoduleStatus, + renameIncludesModifications: false, } } else if (entry.kind === 'renamed' && oldPath != null) { + // The renamed files includes modifications if the working tree has + // modifications of the rename score is not 100%. + let renameIncludesModifications = false; + if ( + entry.workingTree === GitStatusEntry.Modified || + (entry.renameOrCopyScore !== undefined && entry.renameOrCopyScore < 100) + ) { + renameIncludesModifications = true; + } return { kind: AppFileStatusKind.Renamed, oldPath, submoduleStatus: entry.submoduleStatus, + renameIncludesModifications, } } else if (entry.kind === 'untracked') { return { @@ -276,7 +287,7 @@ function buildStatusMap( entry: IStatusEntry, conflictDetails: ConflictFilesDetails ): Map { - const status = mapStatus(entry.statusCode, entry.submoduleStatusCode) + const status = mapStatus(entry.statusCode, entry.submoduleStatusCode, entry.renameOrCopyScore) if (status.kind === 'ordinary') { // when a file is added in the index but then removed in the working diff --git a/app/src/lib/status-parser.ts b/app/src/lib/status-parser.ts index fa6ec870470..9df0ca64783 100644 --- a/app/src/lib/status-parser.ts +++ b/app/src/lib/status-parser.ts @@ -28,6 +28,9 @@ export interface IStatusEntry { /** The original path in the case of a renamed file */ readonly oldPath?: string + + /** The rename or copy score in the case of a renamed file */ + readonly renameOrCopyScore?: number } export function isStatusHeader( @@ -141,6 +144,7 @@ function parsedRenamedOrCopiedEntry( statusCode: match[1], submoduleStatusCode: match[2], oldPath, + renameOrCopyScore: parseInt(match[8].substring(1), 10), path: match[9], } } @@ -196,7 +200,8 @@ function mapSubmoduleStatus( */ export function mapStatus( statusCode: string, - submoduleStatusCode: string + submoduleStatusCode: string, + renameOrCopyScore: number | undefined ): FileEntry { const submoduleStatus = mapSubmoduleStatus(submoduleStatusCode) @@ -269,6 +274,7 @@ export function mapStatus( kind: 'renamed', index: GitStatusEntry.Renamed, workingTree: GitStatusEntry.Unchanged, + renameOrCopyScore, submoduleStatus, } } @@ -278,6 +284,7 @@ export function mapStatus( kind: 'renamed', index: GitStatusEntry.Unchanged, workingTree: GitStatusEntry.Renamed, + renameOrCopyScore, submoduleStatus, } } @@ -325,6 +332,7 @@ export function mapStatus( kind: 'renamed', index: GitStatusEntry.Renamed, workingTree: GitStatusEntry.Modified, + renameOrCopyScore, submoduleStatus, } } @@ -334,6 +342,7 @@ export function mapStatus( kind: 'renamed', index: GitStatusEntry.Renamed, workingTree: GitStatusEntry.Deleted, + renameOrCopyScore, submoduleStatus, } } diff --git a/app/src/models/status.ts b/app/src/models/status.ts index 1a40444d294..2edc61c102f 100644 --- a/app/src/models/status.ts +++ b/app/src/models/status.ts @@ -47,6 +47,7 @@ export type PlainFileStatus = { export type CopiedOrRenamedFileStatus = { kind: AppFileStatusKind.Copied | AppFileStatusKind.Renamed oldPath: string + renameIncludesModifications: boolean; submoduleStatus?: SubmoduleStatus } @@ -146,6 +147,8 @@ type RenamedOrCopiedEntry = { readonly workingTree?: GitStatusEntry /** the submodule status for this entry */ readonly submoduleStatus?: SubmoduleStatus + /** The rename or copy score in the case of a renamed file */ + readonly renameOrCopyScore?: number } export enum UnmergedEntrySummary { diff --git a/app/src/ui/diff/index.tsx b/app/src/ui/diff/index.tsx index ea0dc85431f..684aa11571e 100644 --- a/app/src/ui/diff/index.tsx +++ b/app/src/ui/diff/index.tsx @@ -31,6 +31,8 @@ import { BinaryFile } from './binary-file' import { SideBySideDiff } from './side-by-side-diff' import { IFileContents } from './syntax-highlighting' import { SubmoduleDiff } from './submodule-diff' +import { Octicon } from '../octicons' +import * as OcticonSymbol from '../octicons/octicons.generated' // image used when no diff is displayed const NoDiffImage = encodePathAsUrl(__dirname, 'static/ufo-alert.svg') @@ -224,11 +226,24 @@ export class Diff extends React.Component { } if (this.props.file.status.kind === AppFileStatusKind.Renamed) { - return ( -
- The file was renamed but not changed -
- ) + // Check if it was changed too + if (this.props.file.status.renameIncludesModifications) + { + return ( +
+ + The file was renamed and includes changes. +
+ ) + } + else + { + return ( +
+ The file was renamed but not changed +
+ ) + } } if ( From 015759ff3aab847f3d0c15e465952c0ded0d4465 Mon Sep 17 00:00:00 2001 From: Stephen Sigwart Date: Tue, 26 Nov 2024 22:12:26 -0500 Subject: [PATCH 2/5] Review updates --- app/src/lib/git/log.ts | 12 ++++-------- app/src/lib/git/status.ts | 12 ++---------- app/src/ui/diff/index.tsx | 13 +++++-------- 3 files changed, 11 insertions(+), 26 deletions(-) diff --git a/app/src/lib/git/log.ts b/app/src/lib/git/log.ts index 83c3096700a..41d35933e2f 100644 --- a/app/src/lib/git/log.ts +++ b/app/src/lib/git/log.ts @@ -69,24 +69,20 @@ function mapStatus( return { kind: AppFileStatusKind.Deleted, submoduleStatus } } // deleted if (status === 'R' && oldPath != null) { - const renameIncludesModifications = false; - return { kind: AppFileStatusKind.Renamed, oldPath, submoduleStatus, renameIncludesModifications } + return { kind: AppFileStatusKind.Renamed, oldPath, submoduleStatus, renameIncludesModifications: false } } // renamed if (status === 'C' && oldPath != null) { - const renameIncludesModifications = false; - return { kind: AppFileStatusKind.Copied, oldPath, submoduleStatus, renameIncludesModifications } + return { kind: AppFileStatusKind.Copied, oldPath, submoduleStatus, renameIncludesModifications: false } } // copied // git log -M --name-status will return a RXXX - where XXX is a percentage if (status.match(/R[0-9]+/) && oldPath != null) { - const renameIncludesModifications = (status !== "R100"); - return { kind: AppFileStatusKind.Renamed, oldPath, submoduleStatus, renameIncludesModifications } + return { kind: AppFileStatusKind.Renamed, oldPath, submoduleStatus, renameIncludesModifications: (status !== "R100") } } // git log -C --name-status will return a CXXX - where XXX is a percentage if (status.match(/C[0-9]+/) && oldPath != null) { - const renameIncludesModifications = false; - return { kind: AppFileStatusKind.Copied, oldPath, submoduleStatus, renameIncludesModifications } + return { kind: AppFileStatusKind.Copied, oldPath, submoduleStatus, renameIncludesModifications: false } } return { kind: AppFileStatusKind.Modified, submoduleStatus } diff --git a/app/src/lib/git/status.ts b/app/src/lib/git/status.ts index 94b2b3588c4..2d2dc6c70a5 100644 --- a/app/src/lib/git/status.ts +++ b/app/src/lib/git/status.ts @@ -158,20 +158,12 @@ function convertToAppStatus( renameIncludesModifications: false, } } else if (entry.kind === 'renamed' && oldPath != null) { - // The renamed files includes modifications if the working tree has - // modifications of the rename score is not 100%. - let renameIncludesModifications = false; - if ( - entry.workingTree === GitStatusEntry.Modified || - (entry.renameOrCopyScore !== undefined && entry.renameOrCopyScore < 100) - ) { - renameIncludesModifications = true; - } return { kind: AppFileStatusKind.Renamed, oldPath, submoduleStatus: entry.submoduleStatus, - renameIncludesModifications, + renameIncludesModifications: entry.workingTree === GitStatusEntry.Modified || + (entry.renameOrCopyScore !== undefined && entry.renameOrCopyScore < 100), } } else if (entry.kind === 'untracked') { return { diff --git a/app/src/ui/diff/index.tsx b/app/src/ui/diff/index.tsx index 684aa11571e..d1cdc36ea5b 100644 --- a/app/src/ui/diff/index.tsx +++ b/app/src/ui/diff/index.tsx @@ -236,14 +236,11 @@ export class Diff extends React.Component { ) } - else - { - return ( -
- The file was renamed but not changed -
- ) - } + return ( +
+ The file was renamed but not changed +
+ ) } if ( From 60ab35e6cf52914de282db09e8acb5d7271992e6 Mon Sep 17 00:00:00 2001 From: Stephen Sigwart Date: Tue, 26 Nov 2024 22:23:22 -0500 Subject: [PATCH 3/5] Lint auto-fixes --- app/src/lib/git/log.ts | 28 ++++++++++++++++++++++++---- app/src/lib/git/status.ts | 12 +++++++++--- app/src/models/status.ts | 2 +- app/src/ui/diff/index.tsx | 5 ++--- 4 files changed, 36 insertions(+), 11 deletions(-) diff --git a/app/src/lib/git/log.ts b/app/src/lib/git/log.ts index 41d35933e2f..b54f2924b9b 100644 --- a/app/src/lib/git/log.ts +++ b/app/src/lib/git/log.ts @@ -69,20 +69,40 @@ function mapStatus( return { kind: AppFileStatusKind.Deleted, submoduleStatus } } // deleted if (status === 'R' && oldPath != null) { - return { kind: AppFileStatusKind.Renamed, oldPath, submoduleStatus, renameIncludesModifications: false } + return { + kind: AppFileStatusKind.Renamed, + oldPath, + submoduleStatus, + renameIncludesModifications: false, + } } // renamed if (status === 'C' && oldPath != null) { - return { kind: AppFileStatusKind.Copied, oldPath, submoduleStatus, renameIncludesModifications: false } + return { + kind: AppFileStatusKind.Copied, + oldPath, + submoduleStatus, + renameIncludesModifications: false, + } } // copied // git log -M --name-status will return a RXXX - where XXX is a percentage if (status.match(/R[0-9]+/) && oldPath != null) { - return { kind: AppFileStatusKind.Renamed, oldPath, submoduleStatus, renameIncludesModifications: (status !== "R100") } + return { + kind: AppFileStatusKind.Renamed, + oldPath, + submoduleStatus, + renameIncludesModifications: status !== 'R100', + } } // git log -C --name-status will return a CXXX - where XXX is a percentage if (status.match(/C[0-9]+/) && oldPath != null) { - return { kind: AppFileStatusKind.Copied, oldPath, submoduleStatus, renameIncludesModifications: false } + return { + kind: AppFileStatusKind.Copied, + oldPath, + submoduleStatus, + renameIncludesModifications: false, + } } return { kind: AppFileStatusKind.Modified, submoduleStatus } diff --git a/app/src/lib/git/status.ts b/app/src/lib/git/status.ts index 2d2dc6c70a5..c2837aa562f 100644 --- a/app/src/lib/git/status.ts +++ b/app/src/lib/git/status.ts @@ -162,8 +162,10 @@ function convertToAppStatus( kind: AppFileStatusKind.Renamed, oldPath, submoduleStatus: entry.submoduleStatus, - renameIncludesModifications: entry.workingTree === GitStatusEntry.Modified || - (entry.renameOrCopyScore !== undefined && entry.renameOrCopyScore < 100), + renameIncludesModifications: + entry.workingTree === GitStatusEntry.Modified || + (entry.renameOrCopyScore !== undefined && + entry.renameOrCopyScore < 100), } } else if (entry.kind === 'untracked') { return { @@ -279,7 +281,11 @@ function buildStatusMap( entry: IStatusEntry, conflictDetails: ConflictFilesDetails ): Map { - const status = mapStatus(entry.statusCode, entry.submoduleStatusCode, entry.renameOrCopyScore) + const status = mapStatus( + entry.statusCode, + entry.submoduleStatusCode, + entry.renameOrCopyScore + ) if (status.kind === 'ordinary') { // when a file is added in the index but then removed in the working diff --git a/app/src/models/status.ts b/app/src/models/status.ts index 2edc61c102f..13aba603479 100644 --- a/app/src/models/status.ts +++ b/app/src/models/status.ts @@ -47,7 +47,7 @@ export type PlainFileStatus = { export type CopiedOrRenamedFileStatus = { kind: AppFileStatusKind.Copied | AppFileStatusKind.Renamed oldPath: string - renameIncludesModifications: boolean; + renameIncludesModifications: boolean submoduleStatus?: SubmoduleStatus } diff --git a/app/src/ui/diff/index.tsx b/app/src/ui/diff/index.tsx index d1cdc36ea5b..f344d03f626 100644 --- a/app/src/ui/diff/index.tsx +++ b/app/src/ui/diff/index.tsx @@ -227,11 +227,10 @@ export class Diff extends React.Component { if (this.props.file.status.kind === AppFileStatusKind.Renamed) { // Check if it was changed too - if (this.props.file.status.renameIncludesModifications) - { + if (this.props.file.status.renameIncludesModifications) { return (
- + The file was renamed and includes changes.
) From 308ba10aeb04d036d7b3b6659f5f22ae8bfd868f Mon Sep 17 00:00:00 2001 From: Stephen Sigwart Date: Wed, 27 Nov 2024 22:04:36 -0500 Subject: [PATCH 4/5] Unit test updates --- app/test/unit/git/log-test.ts | 8 ++++++++ app/test/unit/git/status-test.ts | 4 ++++ 2 files changed, 12 insertions(+) diff --git a/app/test/unit/git/log-test.ts b/app/test/unit/git/log-test.ts index c34017c144c..58d4fef7dc2 100644 --- a/app/test/unit/git/log-test.ts +++ b/app/test/unit/git/log-test.ts @@ -83,6 +83,8 @@ describe('git/log', () => { expect(first.files[0].status).toEqual({ kind: AppFileStatusKind.Renamed, oldPath: 'NEW.md', + "renameIncludesModifications": true, + "submoduleStatus": undefined, }) const second = await getChangedFiles(repository, 'c898ca8') @@ -92,6 +94,8 @@ describe('git/log', () => { expect(second.files[0].status).toEqual({ kind: AppFileStatusKind.Renamed, oldPath: 'OLD.md', + "renameIncludesModifications": false, + "submoduleStatus": undefined, }) }) @@ -111,12 +115,16 @@ describe('git/log', () => { expect(changesetData.files[0].status).toEqual({ kind: AppFileStatusKind.Copied, oldPath: 'initial.md', + "renameIncludesModifications": false, + "submoduleStatus": undefined, }) expect(changesetData.files[1].path).toBe('duplicate.md') expect(changesetData.files[1].status).toEqual({ kind: AppFileStatusKind.Copied, oldPath: 'initial.md', + "renameIncludesModifications": false, + "submoduleStatus": undefined, }) }) diff --git a/app/test/unit/git/status-test.ts b/app/test/unit/git/status-test.ts index d549493f8ae..f3bf5100b31 100644 --- a/app/test/unit/git/status-test.ts +++ b/app/test/unit/git/status-test.ts @@ -247,6 +247,8 @@ describe('git/status', () => { expect(files[0].status).toEqual({ kind: AppFileStatusKind.Renamed, oldPath: 'foo', + "renameIncludesModifications": false, + "submoduleStatus": undefined, }) }) @@ -275,6 +277,8 @@ describe('git/status', () => { expect(files[1].status).toEqual({ kind: AppFileStatusKind.Copied, oldPath: 'CONTRIBUTING.md', + "renameIncludesModifications": false, + "submoduleStatus": undefined, }) }) From e1ea97c0ac0585b3168c2ce997aa1a2ec7a4a8b2 Mon Sep 17 00:00:00 2001 From: Stephen Sigwart Date: Wed, 4 Dec 2024 20:23:30 -0500 Subject: [PATCH 5/5] Automated lint fixes --- app/test/unit/git/log-test.ts | 16 ++++++++-------- app/test/unit/git/status-test.ts | 8 ++++---- 2 files changed, 12 insertions(+), 12 deletions(-) diff --git a/app/test/unit/git/log-test.ts b/app/test/unit/git/log-test.ts index 58d4fef7dc2..ee8a758375d 100644 --- a/app/test/unit/git/log-test.ts +++ b/app/test/unit/git/log-test.ts @@ -83,8 +83,8 @@ describe('git/log', () => { expect(first.files[0].status).toEqual({ kind: AppFileStatusKind.Renamed, oldPath: 'NEW.md', - "renameIncludesModifications": true, - "submoduleStatus": undefined, + renameIncludesModifications: true, + submoduleStatus: undefined, }) const second = await getChangedFiles(repository, 'c898ca8') @@ -94,8 +94,8 @@ describe('git/log', () => { expect(second.files[0].status).toEqual({ kind: AppFileStatusKind.Renamed, oldPath: 'OLD.md', - "renameIncludesModifications": false, - "submoduleStatus": undefined, + renameIncludesModifications: false, + submoduleStatus: undefined, }) }) @@ -115,16 +115,16 @@ describe('git/log', () => { expect(changesetData.files[0].status).toEqual({ kind: AppFileStatusKind.Copied, oldPath: 'initial.md', - "renameIncludesModifications": false, - "submoduleStatus": undefined, + renameIncludesModifications: false, + submoduleStatus: undefined, }) expect(changesetData.files[1].path).toBe('duplicate.md') expect(changesetData.files[1].status).toEqual({ kind: AppFileStatusKind.Copied, oldPath: 'initial.md', - "renameIncludesModifications": false, - "submoduleStatus": undefined, + renameIncludesModifications: false, + submoduleStatus: undefined, }) }) diff --git a/app/test/unit/git/status-test.ts b/app/test/unit/git/status-test.ts index f3bf5100b31..40c5d498f2b 100644 --- a/app/test/unit/git/status-test.ts +++ b/app/test/unit/git/status-test.ts @@ -247,8 +247,8 @@ describe('git/status', () => { expect(files[0].status).toEqual({ kind: AppFileStatusKind.Renamed, oldPath: 'foo', - "renameIncludesModifications": false, - "submoduleStatus": undefined, + renameIncludesModifications: false, + submoduleStatus: undefined, }) }) @@ -277,8 +277,8 @@ describe('git/status', () => { expect(files[1].status).toEqual({ kind: AppFileStatusKind.Copied, oldPath: 'CONTRIBUTING.md', - "renameIncludesModifications": false, - "submoduleStatus": undefined, + renameIncludesModifications: false, + submoduleStatus: undefined, }) })