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

Add the feature for comparing two arbitrary commits #1108

Merged
merged 46 commits into from
Apr 20, 2022

Conversation

quachtridat
Copy link
Contributor

@quachtridat quachtridat commented Apr 14, 2022

Summary

This PR resolves #985 by implementing the necessary components and logic that will allow users to select two commits in the History tab and see the Git diff for every changed file.

Details

Backend

5cee9bf

Frontend

Each entry in History tab now has two buttons, each with an icon of a document containing an arrow pointing right/left. Clicking one of these will make the commit comparison box pop up, and the diff will be evaluated when there are two commits selected for comparison. Stylings are similar to how the Changed section for a commit entry in History is styled.

Caveats

The commit comparison box can be collapsed, but cannot be resized for now.

Demos

image
The commit that is selected for compare has a little red color on the left side.

image
The commit that is to be compared with selected has a little green color on the left side.

image
The comparison results are shown in the commit comparison box at the bottom of the History tab.

image
The diff for the file README.md is shown when the diff button for that file is clicked.

image
The commit comparison box when it is collapsed.

Notes

This PR may need more tests.

Linked issues

#985

- Adjust git push, pull, fetch, clone handlers, allowing `auth` and
`cache_credentials` to be passed in in requests.
- Allow `credential.helper` to pass through `git config` handler's filters.
Allow users to configure the value of `credential_helper` to be used in
the credentials caching logic.
Implemennt methods that ensure `credential.helper` in `git config` to be
used for credentials caching.
Extend git command logics (push, pull, fetch, clone) to support
credentials caching when `auth` is provided.
Add tests to test the newly-implemented credentials caching logic for
`git push`, `git pull`, `git fetch` and `git clone`.
Extend `Git.IAuth` and `CredentialsBox` for credentials caching. There
is a checkbox for saving Git credentials temporarily.

Stylings are also made for `CredentialsBox`. `jp-RedirectForm` does not
work well with `input` checkboxes.
Let each of these methods also include `auth?.cacheCredentials` to POST
to the backend.
Add extra class members to take care of the fetch poll when credentials
are required.
Add method `fetch()` that is similar to how `push()` or `pull()` works.

`_fetchRemotes()` now calls `fetch()`, and if credentials are required,
blocks further calls and signals a credentials requirement status change.
`_fetchBlocked` is not needed.
This dialog now supports `fetch` operation, and upon successful
authentication, unblocks the fetch poll by changing the credentials
requirement state to `false`.
Also let `showGitOperationDialog()` accept `IGitExtension` `model`
instead of the concrete class `GitExtension`.
The component now has a boolean for Git credentials requirement.
Also add a callback connected to the model to update that boolean.
Inject the model to the component and reuse `showGitOperationDialog()`
for the `fetch` operation.
Add a notification dot to the component so that it looks like the `push`
(when the local branch is ahead) button and `pull` button (when the
local branch is behind) on the top toolbar in the Git panel.
This method spawns a Git credential cache daemon so that the caching
mechanism can work.
Let these methods check if a cache daemon is required, and invoke
`ensure_git_credential_cache_daemon()` if on Linux, or else respond with
an error message.
Let them return an error response if the credential helper cannot be
ensured.
- Use `assert_awaited_` instead of `assert_called_` in some places.
- Use `assert_has_awaits` instead of `assert_has_calls` in some places.
- Use name `mock_execute` instead of `mock_authentication`.
- Mock `sys.platform` as `"linux"`.
- Make sure the `ensure_git_credential_cache_daemon()` is called
accordingly.
This is so that the front-end can recognize this response with
`IResultWithMessage`.
Add argument `cache_credentials` to assertions.
This is for compatibility with Python 3.7's `unittest`.
- jupyterlab/pull/1099#pullrequestreview-926143691
- Remove redundant tests
- Adjust tests to match changed method signatures
- Add a missing `self.config()` call when ensuring Git credential
helper
Also fix a minor typo.
This component displays a comparison between two commits.
These buttons allow selecting commits for comparison.
This interface is used for comparison results.
This will also be used by the commit comparison logic that will exist in
`src/components/GitPanel.tsx` later.
The comparison status is invalidated when the branch status gets
refreshed.
@github-actions
Copy link

Binder 👈 Launch a binder notebook on branch quachtridat/jupyterlab-git/quachtridat/issue985

@quachtridat quachtridat marked this pull request as ready for review April 14, 2022 07:02
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 @quachtridat and @ZeshanFayyaz

I mainly have suggestions for a better wording (using reference/challenger instead of lhs/rhs) and a small rework to avoid storing twice the same information in the panel state.

jupyterlab_git/git.py Outdated Show resolved Hide resolved
src/components/CommitComparisonBox.tsx Show resolved Hide resolved
src/components/CommitComparisonBox.tsx Outdated Show resolved Hide resolved
src/components/GitPanel.tsx Outdated Show resolved Hide resolved
src/components/GitPanel.tsx Outdated Show resolved Hide resolved
src/components/GitPanel.tsx Outdated Show resolved Hide resolved
src/components/GitPanel.tsx Outdated Show resolved Hide resolved
src/tokens.ts Outdated Show resolved Hide resolved
src/style/PastCommitNode.ts Outdated Show resolved Hide resolved
src/style/PastCommitNode.ts Outdated Show resolved Hide resolved
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 a lot @quachtridat and @ZeshanFayyaz

This is a great addition

@fcollonval fcollonval merged commit 6f10777 into jupyterlab:master Apr 20, 2022
@fcollonval
Copy link
Member

@all-contributors please add quachtridat for code

@allcontributors
Copy link
Contributor

@fcollonval

I've put up a pull request to add @quachtridat! 🎉

@fcollonval
Copy link
Member

@all-contributors please add @ZeshanFayyaz for code

@allcontributors
Copy link
Contributor

@fcollonval

I've put up a pull request to add @ZeshanFayyaz! 🎉

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.

Add ability to display the diff of any two commits in the file history panel
3 participants