desktop / desktop

Focus on what matters instead of fighting with Git.
https://desktop.github.com
MIT License
19.86k stars 9.45k forks source link

Changes from renamed files are ignored #6014

Open saschanaz opened 6 years ago

saschanaz commented 6 years ago

Description

Files can simultaneously be renamed and changed, but GitHub Desktop ignores content changes from renamed files.

Version

Steps to Reproduce

  1. Commit a file with 100 lines.
  2. Rename the file.
  3. Change a single line on the file.
  4. git add the file and see what GitHub Desktop says.

Expected Behavior

GitHub Desktop should show content changes, as VSCode does:

image

Actual Behavior

It ignores any content change.

image

Additional Information

Logs

tierninho commented 5 years ago

Can reproduce this issue on 1.7.0-beta6, mac.

Not sure if this error was related but it popped up at the same time:

http.ts:135 HEAD https://api.github.com/repos/tierninho/-/git 0 ()
t.request @ http.ts:135
request @ api.ts:559
getFetchPollInterval @ api.ts:572
getFetchInterval @ background-fetcher.ts:130
performAndScheduleFetch @ background-fetcher.ts:112
stopped.timeoutHandle.window.setTimeout @ background-fetcher.ts:120
setTimeout (async)
performAndScheduleFetch @ background-fetcher.ts:116
async function (async)
performAndScheduleFetch @ background-fetcher.ts:88
start @ background-fetcher.ts:70
startBackgroundFetching @ app-store.ts:1526
_selectRepositoryRefreshTasks @ app-store.ts:1336
_selectRepository @ app-store.ts:1246
async function (async)
_selectRepository @ app-store.ts:1203
selectRepository @ dispatcher.ts:219
Re.onSelectionChanged @ app.tsx:2310
onItemClick @ repositories-list.tsx:178
onRowClick @ filter-list.tsx:365
onRowClick @ list.tsx:972
onRowClick @ list-row.tsx:62
r @ react-dom.production.min.js:15
invokeGuardedCallback @ react-dom.production.min.js:16
invokeGuardedCallbackAndCatchFirstError @ react-dom.production.min.js:16
p @ react-dom.production.min.js:20
h @ react-dom.production.min.js:22
g @ react-dom.production.min.js:22
c @ react-dom.production.min.js:21
S @ react-dom.production.min.js:24
v @ react-dom.production.min.js:24
Kt @ react-dom.production.min.js:83
batchedUpdates @ react-dom.production.min.js:187
nt @ react-dom.production.min.js:42
Qt @ react-dom.production.min.js:85
interactiveUpdates @ react-dom.production.min.js:188
Xt @ react-dom.production.min.js:84
install.ts:27 getFetchPollInterval: failed for tierninho/-
TypeError: Failed to fetch

This is a UI issue, as the changes to the file are surfaced in the History tab and not lost.

Nyameliaaaa commented 4 years ago

This Issue Occurs On All Platforms.

ad-si commented 3 years ago

This is an absolute must have feature. Lack thereof makes GitHub Desktop unusable. I just missed some changes because the file was also renamed at the same time and GitHub Desktop just says The file was renamed but not changed.

saschanaz commented 3 years ago

https://github.com/desktop/desktop/blob/0fec3adca67e890e917ee3ba70c9dd394fc24493/app/src/ui/diff/index.tsx#L210-L216

This lines seemingly think that renamed files are always unchanged.

Edit: Ah no, diff.hunks.length === 0 means something upper than that is broken.

sergiou87 commented 3 years ago

Another instance of this issue in #12501

ssigwart commented 1 year ago

I just submitted #17364 which solves this. However, if there are both staged and unstaged changes to a renamed file, I couldn't figure out how to show both of them, but I added a message saying that you need to stage all the changes before you can see the diff. That seems much better than the current state that says that there were no changes.

ssigwart commented 1 year ago

@sergiou87, I see that app/src/lib/status-parser.ts sets workingTree to GitStatusEntry.Modified: https://github.com/desktop/desktop/blob/a1ece186bae3a742f206c5edeb0762e78fba2f7c/app/src/lib/status-parser.ts#L322-L329

If I could find a way to pass through that field to the diff renderer, I could have it change the message to " The file was renamed and modified." https://github.com/desktop/desktop/blob/0fec3adca67e890e917ee3ba70c9dd394fc24493/app/src/ui/diff/index.tsx#L210-L216

Is that a good approach?

sergiou87 commented 1 year ago

I think so, as long as this change is introduced in a safe way that doesn't affect the rest of well-supported scenarios. It definitely feels more reliable than the previous one 😄

ssigwart commented 1 year ago

Thanks, @sergiou87. I opened #17467. It's still a bit of passing around fields, but doesn't mess with the actual git diff command, so hopefully this PR is better.

0xdevalias commented 11 months ago

See also:

benneth08 commented 5 months ago

[@]()