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

show both staged and unstaged changes for a file #629

Merged
merged 10 commits into from Jun 14, 2020

Conversation

ianhi
Copy link
Collaborator

@ianhi ianhi commented May 1, 2020

Fixes: #627

As discussed in the issue if you have both staged and unstaged changes to a file:
image

then the file will only show in the Staged Files list in jupyterlab-git.

I added another option to the Status token that accounts for this state and modified the FileList and FileItem accordingly. Note how the same file appears in both Staged and Unstaged lists with different status codes, and these give different results for the diff.
staged-unstaged

When you select one instance of the file both list items are selected. I can't decide if this is a feature or a bug. But it seems that this didn't affected any of the context menu commands that depend on tracking which file is selected.

In simple staging mode it reduces to showing the diff from WORKING to HEAD:
image

Copy link
Member

@fcollonval fcollonval left a comment

Choose a reason for hiding this comment

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

@ianhi nice work

I propose to rename the new category to 'partially-staged' this may be less ambiguous.

Then I add a trick to remove the need for a new component property.

Regarding the double selection, you should add the status attribute in the comparison src/components/FileList#_isSelectedFile

       this.state.selectedFile.x === candidate.x &&
       this.state.selectedFile.y === candidate.y &&
       this.state.selectedFile.from === candidate.from &&
-      this.state.selectedFile.to === candidate.to
+      this.state.selectedFile.to === candidate.to  &&
+      this.state.selectedFile.status === candidate.status

This should fix the double selection.

Do you have time to work on this and to rebase on the master?

src/utils.ts Outdated Show resolved Hide resolved
src/tokens.ts Outdated Show resolved Hide resolved
src/components/FileList.tsx Outdated Show resolved Hide resolved
src/components/FileList.tsx Outdated Show resolved Hide resolved
src/components/FileList.tsx Outdated Show resolved Hide resolved
src/components/FileItem.tsx Outdated Show resolved Hide resolved
src/components/FileItem.tsx Outdated Show resolved Hide resolved
src/components/FileList.tsx Outdated Show resolved Hide resolved
src/components/FileList.tsx Outdated Show resolved Hide resolved
@ianhi
Copy link
Collaborator Author

ianhi commented May 24, 2020

Do you have time to work on this and to rebase on the master?

Yes, will try to do this evening.

@ianhi
Copy link
Collaborator Author

ianhi commented May 25, 2020

What should Discard changes do to partially staged files? Currently it also discards both staged and unstaged changes.

ianhi added 2 commits May 24, 2020 21:45
previously if you add staged a file and then made subsequent changes to it then the file will only show up in the staged files list. This adds another option to the Status used internally that notes when a file has both staged and unstaged changes and modifies the filelist to account for this.
@ianhi
Copy link
Collaborator Author

ianhi commented May 25, 2020

Rebased. Not sure what to do about discarding changes. There is a fun failure mode where if you stage the creation of a file, make further changes, then revert those changes you get the below error:
image
image

I think this happens because the discard function first calls reset then tries to checkout a file that no longer exists.

@fcollonval
Copy link
Member

@ianhi thanks a lot for the quick action. I modify the discardChanges function to skip calling checkout in the case you mentioned. I also modify the case where the index contains modifications but the file was deleted in the working folder to be coherent.

Is this PR good to go for you like that?

@ianhi
Copy link
Collaborator Author

ianhi commented May 26, 2020

Oops sorry I accidentally commented out your action button lines! Looking at this again there are two things I might change:

  1. Put the discard changes action button on the left of the buttons. This will make the button positions line up for staged and unstaged files.
  2. Make discarding changes to a partially staged file only remove the unstaged changes.

My intuition as user is that discarding changes through a button in the unstaged section would only remove the the unstaged changes. So removing both staged and unstaged changes is dangerous because I can lose more changes than I expect.
discarding-changes

I think my position boils down to: Hitting discard changes on a FileItem in the unstaged section should call git checkout but not git reset. It looks like the git reset is in there because

/** Discard changes in a specific unstaged or staged file */
the function is meant to apply to both staged and unstaged files. Though there doesn't currently seem to be a way to unstage changes to a staged file. Can we just get rid of the git reset and change DiscardChanges -> DiscardUnstagedChanges?

Or perhaps discard changes should result in a git restore command. Unfortunately that was only introduced in git version 2.23, But we can easily get the same effect with:

git stash push file
git stash drop

fcollonval
fcollonval previously approved these changes Jun 7, 2020
@fcollonval
Copy link
Member

  • Put the discard changes action button on the left of the buttons. This will make the button positions line up for staged and unstaged files.

I will not change it. The current order follows the one of VS code. And this seems to be ok if double clicking opens the diff view, then the user action for a given file will be adding or discarding the changes. So it sounds logical the two buttons are next to each other.

  • Make discarding changes to a partially staged file only remove the unstaged changes.

Done, staged file are reset only and unstaged file are checkout only.

We cannot get ride of the method as we need to cover the simple UI case in which discarding changes means reset + checkout for partially staged files (although this should not happen when using only the UI).

@ianhi
Copy link
Collaborator Author

ianhi commented Jun 7, 2020

🎉 Just tried it out again and everything seems to work as expected, so good to go for me.

Thanks for the help on this one.

@ianhi
Copy link
Collaborator Author

ianhi commented Jun 7, 2020

Oops wait I take that back, if the only staged file is a partially staged file then you will be unable to commit. I will fix and then I think it will be good to go

@ianhi
Copy link
Collaborator Author

ianhi commented Jun 13, 2020

I also realized that there was no way to access the option to discard changes to a staged files in simple mode. So I added a discard action to files in simple staging mode and also added a contextmenu.

Copy link
Member

@fcollonval fcollonval left a comment

Choose a reason for hiding this comment

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

Thanks @ianhi for the latest updates. The context menu should at some point be simplified to not have to deal with 5 menu but to use selectors for the actions. But let's keep it aside for now.

@fcollonval fcollonval merged commit 20d9627 into jupyterlab:master Jun 14, 2020
@fcollonval
Copy link
Member

@meeseeksdev backport to 0.11.x

meeseeksmachine pushed a commit to meeseeksmachine/jupyterlab-git that referenced this pull request Jun 14, 2020
@ianhi ianhi deleted the staged branch June 14, 2020 17:11
fcollonval added a commit that referenced this pull request Jun 21, 2020
…ges for a file) (#679)

* Backport PR #629: show both staged and unstaged changes for a file

* Correction for JLab 1

* Forget to update jest mock package

Co-authored-by: Ian Hunt-Isaak <ianhuntisaak@gmail.com>
Co-authored-by: Frédéric Collonval <fcollonval@gmail.com>
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.

modifying a staged file does not add it to Changed files list
2 participants