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

Change diff renamed files #1040

Merged
merged 22 commits into from Apr 24, 2022
Merged

Conversation

Mifom
Copy link
Contributor

@Mifom Mifom commented Dec 12, 2021

This Pull Request closes #1038.

It changes the following:

  • Add status change on rename
  • Change diff title to represent rename
  • Change diff to show only rewrites on rename

I followed the checklist:

  • I added and fixed current unittests.
  • I ran make check without errors
  • I tested the overall application
  • I added an appropriate item to the changelog

@Mifom Mifom changed the title WIP: Change diff renamed files Change diff renamed files Dec 12, 2021
@extrawurst
Copy link
Owner

@Mifom Thanks for looking into this. I played a bit around with you branch. did not check the code yet but two observations jump out:

a) we still do not detect renames against the workdir, this should be easy to add:
Screenshot 2021-12-12 at 09 57 20

b) and then there is an issue staging hunks in the renamed&changed file:
Screenflick Movie 26

b) current hunk staging code is a bit wonky, so don't worry If thats the reason, then this might be better done in a separate PR

Copy link
Owner

@extrawurst extrawurst left a comment

Choose a reason for hiding this comment

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

lets add a dedicated unittest into asyncgit for the changed diff mechanic please

CHANGELOG.md Outdated Show resolved Hide resolved
src/tabs/status.rs Outdated Show resolved Hide resolved
@Mifom Mifom changed the title Change diff renamed files WIP: Change diff renamed files Dec 13, 2021
@Mifom
Copy link
Contributor Author

Mifom commented Dec 13, 2021

I'll mark it ready when I deal with hunks

@extrawurst extrawurst marked this pull request as draft December 14, 2021 12:48
@Mifom Mifom force-pushed the change-diff-renamed-files branch 2 times, most recently from 385cf2c to 21e81ec Compare December 17, 2021 14:02
@Mifom
Copy link
Contributor Author

Mifom commented Dec 17, 2021

For now hunks mostly done. But there are 2 1 issue:

1) Because destination file is untracked, diff in workdir is shown only for source file:
Screenshot from 2021-12-17 16-58-49
git currently works the same way.
(fixed)

  1. In changed renamed file is only one hunk - the whole file, so changes cannot be staged/unstaged without staging/unstaging full file:
    Screenshot from 2021-12-17 17-18-37

@extrawurst is this issue need to be fixed in this PR? Currently it's not supported by git2 and libgit2.

@Mifom Mifom changed the title WIP: Change diff renamed files Change diff renamed files Dec 17, 2021
@extrawurst
Copy link
Owner

is this issue need to be fixed in this PR? Currently it's not supported by git2 and libgit2.

i guess then not :)
But then we should just go for regular file-level stagin/upstaging when enter is used on a hunk in a renamed file I stead of creating an error

@extrawurst
Copy link
Owner

Then this is ready ?

@Mifom
Copy link
Contributor Author

Mifom commented Dec 17, 2021

Then this is ready ?

Yes

@Mifom
Copy link
Contributor Author

Mifom commented Dec 17, 2021

is this issue need to be fixed in this PR? Currently it's not supported by git2 and libgit2.

i guess then not :)
But then we should just go for regular file-level stagin/upstaging when enter is used on a hunk in a renamed file I stead of creating an error

If I understand correctly this is how it works now

@Mifom Mifom marked this pull request as ready for review December 18, 2021 23:32
@extrawurst
Copy link
Owner

extrawurst commented Dec 21, 2021

@Mifom so functionality wise this looks awesome to me. but I am not sure what I am missing, cause I cannot reproduce the single-hunk-issue:
Screenflick Movie 27

looks pretty good to me, with the only glitch that removing a hunk from the index removes the rename-part of the change aswell. but I guess thats a mini thing we can followup later with. especially cause its a philosophical decision of what should take precendence I guess :)

what do you think?

Copy link
Owner

@extrawurst extrawurst left a comment

Choose a reason for hiding this comment

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

just a few minor things left and a proper test for the rename case

asyncgit/src/sync/diff.rs Show resolved Hide resolved
asyncgit/src/sync/diff.rs Show resolved Hide resolved
asyncgit/src/sync/status.rs Outdated Show resolved Hide resolved
asyncgit/src/sync/status.rs Show resolved Hide resolved
src/tabs/status.rs Outdated Show resolved Hide resolved
@extrawurst
Copy link
Owner

I found another issue left:

Screenflick Movie 28

So discarding a renamed file item (now only a single entry in the status list) should discard fully, old-path and new-path. please also add a test for this case.

@extrawurst
Copy link
Owner

@Mifom let me know when I should review it again

@extrawurst
Copy link
Owner

@Mifom how can I help you get this over the finish line?

@Mifom
Copy link
Contributor Author

Mifom commented Jan 6, 2022

@extrawurst I just need some time. I think I'll do it before the end of the week.

@Mifom Mifom force-pushed the change-diff-renamed-files branch 2 times, most recently from 741d7d2 to 79a41ca Compare January 8, 2022 18:12
@extrawurst
Copy link
Owner

@Mifom thanks for your continued effort, let me know when you think its ready

@extrawurst
Copy link
Owner

@Mifom ping. feel free to join the discord if you need some feedback somewhere

@extrawurst
Copy link
Owner

extrawurst commented Jan 21, 2022

here is an issue when you want to discard a hunk in a renamed file:

image Screenshot 2022-01-21 at 22 04 32

this will error-close with:

Error: `hunk not found`

if thats nothing we can easily fix before we merge we can disable hunk based commands on entries with renamed-status

@extrawurst
Copy link
Owner

extrawurst commented Jan 21, 2022

here is an issue with line-staging:

image Screenshot 2022-01-21 at 22 07 16

leads to:
Screenshot 2022-01-21 at 22 07 54

as with above hunk error we can workaround this (if we don't wanna fix it now) by not allowing line based staging on renamed entries.

@Mifom Mifom force-pushed the change-diff-renamed-files branch from 13482e1 to 74232f9 Compare April 1, 2022 12:29
@Mifom
Copy link
Contributor Author

Mifom commented Apr 1, 2022

@extrawurst sorry for this long period without work on this PR.

There are no good way for discarding hunks in renamed files at the moment, so this was disabled.

About line staging: it works at least for the changes i made. Maybe there are cases where it will not work, so i'll check that.

About renamed folder staging: at the moment when user mark folder as staged it passes to staging on the side of git2, so we haven't control this. That's should be reimplemented in gitui side to accurately remove them and not only add new files.

@Mifom Mifom requested a review from extrawurst April 1, 2022 19:49
@extrawurst
Copy link
Owner

@Mifom can you bring the branch up to date with master, so it could in theory be merged?

@Mifom
Copy link
Contributor Author

Mifom commented Apr 11, 2022

@Mifom can you bring the branch up to date with master, so it could in theory be merged?

Now it's up to date

@Mifom Mifom force-pushed the change-diff-renamed-files branch from 5182c47 to a45351b Compare April 11, 2022 21:07
@Mifom
Copy link
Contributor Author

Mifom commented Apr 18, 2022

@extrawurst the branch is now up to date
In the comment above I explain status of all issues with this PR. If you have ideas how to surpass them I can implement them or we can open new issue for them where they will be resolved.

@extrawurst extrawurst merged commit 5f466ff into extrawurst:master Apr 24, 2022
@extrawurst
Copy link
Owner

Thanks for sticking with this! 🥳

@extrawurst
Copy link
Owner

I think I have to revert this. after the merge no return on any file in the file list works anymore to stage the whole file

@extrawurst
Copy link
Owner

ok damn I don't get the changes made to index_add_remove now that I try to debug this. you seem to sync::reset_stage anything that was just staged in the same function. sorry I did not catch this before the merge. I will revert the change. feel free to reopen the PR

extrawurst added a commit that referenced this pull request Apr 24, 2022
@extrawurst
Copy link
Owner

@Mifom let me know if you need help figuring this out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Renamed files show diff as new files, don't show changes from old file or path to old file
2 participants