From f3327b738995ea49503394a6a5d58c007596ac2f Mon Sep 17 00:00:00 2001 From: Dat Quach Date: Tue, 8 Mar 2022 04:25:56 -0800 Subject: [PATCH 01/44] Adjust git command handlers - 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. --- jupyterlab_git/handlers.py | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-) diff --git a/jupyterlab_git/handlers.py b/jupyterlab_git/handlers.py index 14784043c..50cc4aaf3 100644 --- a/jupyterlab_git/handlers.py +++ b/jupyterlab_git/handlers.py @@ -23,7 +23,7 @@ from .log import get_logger # Git configuration options exposed through the REST API -ALLOWED_OPTIONS = ["user.name", "user.email"] +ALLOWED_OPTIONS = ["user.name", "user.email", "credential.helper"] # REST API namespace NAMESPACE = "/git" @@ -73,7 +73,10 @@ async def post(self, path: str = ""): """ data = self.get_json_body() response = await self.git.clone( - self.url2localpath(path), data["clone_url"], data.get("auth", None) + self.url2localpath(path), + data["clone_url"], + data.get("auth", None), + data.get("cache_credentials", False), ) if response["code"] != 0: @@ -170,7 +173,12 @@ async def post(self, path: str = ""): """ POST request handler, fetch from remotes. """ - result = await self.git.fetch(self.url2localpath(path)) + data = self.get_json_body() + result = await self.git.fetch( + self.url2localpath(path), + data.get("auth", None), + data.get("cache_credentials", False), + ) if result["code"] != 0: self.set_status(500) @@ -533,6 +541,7 @@ async def post(self, path: str = ""): self.url2localpath(path), data.get("auth", None), data.get("cancel_on_conflict", False), + data.get("cache_credentials", False), ) if response["code"] != 0: @@ -563,6 +572,7 @@ async def post(self, path: str = ""): data = self.get_json_body() known_remote = data.get("remote") force = data.get("force", False) + cache_credentials = data.get("cache_credentials", False) current_local_branch = await self.git.get_current_branch(local_path) @@ -591,6 +601,7 @@ async def post(self, path: str = ""): data.get("auth", None), set_upstream, force, + cache_credentials, ) else: @@ -617,6 +628,7 @@ async def post(self, path: str = ""): data.get("auth", None), set_upstream=True, force=force, + cache_credentials=cache_credentials, ) else: response = { From 38b30ff3c23bb61cca2e5cc3441613a5dd7d4fff Mon Sep 17 00:00:00 2001 From: Dat Quach Date: Tue, 8 Mar 2022 04:29:32 -0800 Subject: [PATCH 02/44] Add `credential_helper` to server settings Allow users to configure the value of `credential_helper` to be used in the credentials caching logic. --- jupyterlab_git/__init__.py | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/jupyterlab_git/__init__.py b/jupyterlab_git/__init__.py index 81ae25033..e90f67cb0 100644 --- a/jupyterlab_git/__init__.py +++ b/jupyterlab_git/__init__.py @@ -4,7 +4,7 @@ import json from pathlib import Path -from traitlets import List, Dict, Unicode +from traitlets import List, Dict, Unicode, default from traitlets.config import Configurable from ._version import __version__ @@ -37,6 +37,18 @@ class JupyterLabGit(Configurable): # TODO Validate ) + credential_helper = Unicode( + help=""" + The value of Git credential helper will be set to this value when the Git credential caching mechanism is activated by this extension. + By default it is 3600 seconds (1 hour). + """, + config=True, + ) + + @default("credential_helper") + def _credential_helper_default(self): + return "cache --timeout=3600" + def _jupyter_server_extension_points(): return [{"module": "jupyterlab_git"}] From 0bad2a8242877af321c82deefcf027b6d18bc5cf Mon Sep 17 00:00:00 2001 From: Dat Quach Date: Tue, 8 Mar 2022 04:32:50 -0800 Subject: [PATCH 03/44] Implement logic for setting git credential helper Implemennt methods that ensure `credential.helper` in `git config` to be used for credentials caching. --- jupyterlab_git/git.py | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/jupyterlab_git/git.py b/jupyterlab_git/git.py index 8d3ad92a4..a5d8a98b9 100644 --- a/jupyterlab_git/git.py +++ b/jupyterlab_git/git.py @@ -1540,3 +1540,34 @@ async def tag_checkout(self, path, tag): "command": " ".join(command), "message": error, } + + async def check_credential_helper(self, path): + git_config_response = await self.config(path) + if git_config_response["code"] != 0: + return git_config_response + + git_config_kv_pairs = git_config_response["options"] + has_credential_helper = "credential.helper" in git_config_kv_pairs + + return {"code": 0, "result": has_credential_helper} + + async def ensure_credential_helper(self, path): + """ + Check whether `git config --list` contains `credential.helper`. + If it is not set, then it will be set to the value string for `credential.helper` + defined in the server settings. + """ + + check_credential_helper = await self.check_credential_helper(path) + + if check_credential_helper["code"] != 0: + return check_credential_helper + has_credential_helper = check_credential_helper["result"] + + if not has_credential_helper: + config_response = await self.config( + path, **{"credential.helper": self._config.credential_helper} + ) + return config_response + else: + return {"code": 0} From bf13b83278fabc46980df68df65049783d1fc391 Mon Sep 17 00:00:00 2001 From: Dat Quach Date: Tue, 8 Mar 2022 04:38:57 -0800 Subject: [PATCH 04/44] Extend git commands to support credentials caching Extend git command logics (push, pull, fetch, clone) to support credentials caching when `auth` is provided. --- jupyterlab_git/git.py | 40 ++++++++++++++++++++++++++++++++++------ 1 file changed, 34 insertions(+), 6 deletions(-) diff --git a/jupyterlab_git/git.py b/jupyterlab_git/git.py index a5d8a98b9..6d5a2e7df 100644 --- a/jupyterlab_git/git.py +++ b/jupyterlab_git/git.py @@ -261,7 +261,7 @@ async def changed_files(self, path, base=None, remote=None, single_commit=None): return response - async def clone(self, path, repo_url, auth=None): + async def clone(self, path, repo_url, auth=None, cache_credentials=None): """ Execute `git clone`. When no auth is provided, disables prompts for the password to avoid the terminal hanging. @@ -273,6 +273,8 @@ async def clone(self, path, repo_url, auth=None): """ env = os.environ.copy() if auth: + if cache_credentials: + await self.ensure_credential_helper(path) env["GIT_TERMINAL_PROMPT"] = "1" code, output, error = await execute( ["git", "clone", unquote(repo_url), "-q"], @@ -296,7 +298,7 @@ async def clone(self, path, repo_url, auth=None): return response - async def fetch(self, path): + async def fetch(self, path, auth=None, cache_credentials=False): """ Execute git fetch command """ @@ -308,8 +310,21 @@ async def fetch(self, path): "--all", "--prune", ] # Run prune by default to help beginners - - code, _, fetch_error = await execute(cmd, cwd=cwd) + env = os.environ.copy() + if auth: + if cache_credentials: + await self.ensure_credential_helper(path) + env["GIT_TERMINAL_PROMPT"] = "1" + code, _, fetch_error = await execute( + cmd, + cwd=cwd, + username=auth["username"], + password=auth["password"], + env=env, + ) + else: + env["GIT_TERMINAL_PROMPT"] = "0" + code, _, fetch_error = await execute(cmd, cwd=cwd, env=env) result = { "code": code, @@ -998,13 +1013,17 @@ async def commit(self, commit_msg, amend, path): return {"code": code, "command": " ".join(cmd), "message": error} return {"code": code} - async def pull(self, path, auth=None, cancel_on_conflict=False): + async def pull( + self, path, auth=None, cancel_on_conflict=False, cache_credentials=False + ): """ Execute git pull --no-commit. Disables prompts for the password to avoid the terminal hanging while waiting for auth. """ env = os.environ.copy() if auth: + if cache_credentials: + await self.ensure_credential_helper(path) env["GIT_TERMINAL_PROMPT"] = "1" code, output, error = await execute( ["git", "pull", "--no-commit"], @@ -1048,7 +1067,14 @@ async def pull(self, path, auth=None, cancel_on_conflict=False): return response async def push( - self, remote, branch, path, auth=None, set_upstream=False, force=False + self, + remote, + branch, + path, + auth=None, + set_upstream=False, + force=False, + cache_credentials=False, ): """ Execute `git push $UPSTREAM $BRANCH`. The choice of upstream and branch is up to the caller. @@ -1062,6 +1088,8 @@ async def push( env = os.environ.copy() if auth: + if cache_credentials: + await self.ensure_credential_helper(path) env["GIT_TERMINAL_PROMPT"] = "1" code, output, error = await execute( command, From c88d70a9a51069b46fc29e1284a0be2066297895 Mon Sep 17 00:00:00 2001 From: Dat Quach Date: Tue, 8 Mar 2022 04:42:42 -0800 Subject: [PATCH 05/44] Add tests for the new credentials caching logic Add tests to test the newly-implemented credentials caching logic for `git push`, `git pull`, `git fetch` and `git clone`. --- jupyterlab_git/tests/test_clone.py | 100 ++++++++++++- jupyterlab_git/tests/test_fetch.py | 195 +++++++++++++++++++++++++ jupyterlab_git/tests/test_pushpull.py | 196 ++++++++++++++++++++++++++ 3 files changed, 490 insertions(+), 1 deletion(-) create mode 100644 jupyterlab_git/tests/test_fetch.py diff --git a/jupyterlab_git/tests/test_clone.py b/jupyterlab_git/tests/test_clone.py index 00d369678..bdc717f18 100644 --- a/jupyterlab_git/tests/test_clone.py +++ b/jupyterlab_git/tests/test_clone.py @@ -1,8 +1,10 @@ +import os from pathlib import Path -from unittest.mock import patch +from unittest.mock import call, patch import pytest +from jupyterlab_git import JupyterLabGit from jupyterlab_git.git import Git from .testutils import maybe_future @@ -156,3 +158,99 @@ async def test_git_clone_with_auth_auth_failure_from_git(): "code": 128, "message": "remote: Invalid username or password.\r\nfatal: Authentication failed for 'ghjkhjkl'", } == actual_response + + +@pytest.mark.asyncio +async def test_git_clone_with_cache_credentials(): + with patch("jupyterlab_git.git.execute") as mock_execute: + # Given + test_path = str(Path("/bin") / "test_curr_path") + mock_execute.side_effect = [ + maybe_future((0, "", "")), + maybe_future((0, "", "")), + ] + + # When + actual_response = await Git().clone( + path=test_path, repo_url="ghjkhjkl", cache_credentials=True + ) + + # Then + mock_execute.assert_called_once_with( + ["git", "clone", "ghjkhjkl"], + cwd=test_path, + env={**os.environ, "GIT_TERMINAL_PROMPT": "0"}, + ) + assert {"code": 0, "message": ""} == actual_response + + +@pytest.mark.asyncio +async def test_git_clone_with_auth_and_cache_credentials(): + with patch("jupyterlab_git.git.execute") as mock_authentication: + # Given + default_config = JupyterLabGit() + credential_helper = default_config.credential_helper + test_path = str(Path("/bin") / "test_curr_path") + mock_authentication.side_effect = [ + maybe_future((0, "", "")), + maybe_future((0, "", "")), + maybe_future((0, "", "")), + ] + # When + auth = {"username": "asdf", "password": "qwerty"} + actual_response = await Git(config=default_config).clone( + path=test_path, repo_url="ghjkhjkl", auth=auth, cache_credentials=True + ) + + # Then + assert mock_authentication.call_count == 3 + mock_authentication.assert_has_calls( + [ + call(["git", "config", "--list"], cwd=test_path), + call( + ["git", "config", "--add", "credential.helper", credential_helper], + cwd=test_path, + ), + call( + ["git", "clone", "ghjkhjkl", "-q"], + username="asdf", + password="qwerty", + cwd=test_path, + env={**os.environ, "GIT_TERMINAL_PROMPT": "1"}, + ), + ] + ) + assert {"code": 0, "message": ""} == actual_response + + +@pytest.mark.asyncio +async def test_git_clone_with_auth_and_cache_credentials_and_existing_credential_helper(): + with patch("jupyterlab_git.git.execute") as mock_authentication: + # Given + default_config = JupyterLabGit() + test_path = str(Path("/bin") / "test_curr_path") + mock_authentication.side_effect = [ + maybe_future((0, "credential.helper=something", "")), + maybe_future((0, "", "")), + ] + # When + auth = {"username": "asdf", "password": "qwerty"} + actual_response = await Git(config=default_config).clone( + path=test_path, repo_url="ghjkhjkl", auth=auth, cache_credentials=True + ) + + # Then + assert mock_authentication.call_count == 2 + mock_authentication.assert_has_calls( + [ + call(["git", "config", "--list"], cwd=test_path), + call( + ["git", "clone", "ghjkhjkl", "-q"], + username="asdf", + password="qwerty", + cwd=test_path, + env={**os.environ, "GIT_TERMINAL_PROMPT": "1"}, + ), + ] + ) + assert {"code": 0, "message": ""} == actual_response diff --git a/jupyterlab_git/tests/test_fetch.py b/jupyterlab_git/tests/test_fetch.py new file mode 100644 index 000000000..bbd40b455 --- /dev/null +++ b/jupyterlab_git/tests/test_fetch.py @@ -0,0 +1,195 @@ +import os +from unittest.mock import call, patch + +import pytest + +from jupyterlab_git import JupyterLabGit +from jupyterlab_git.git import Git + +from .testutils import maybe_future + + +@pytest.mark.asyncio +async def test_git_fetch_success(): + with patch("jupyterlab_git.git.execute") as mock_execute: + # Given + mock_execute.return_value = maybe_future((0, "", "")) + + # When + actual_response = await Git().fetch(path="test_path") + + # Then + mock_execute.assert_called_once_with( + ["git", "fetch", "--all", "--prune"], + cwd="test_path", + env={**os.environ, "GIT_TERMINAL_PROMPT": "0"}, + ) + assert {"code": 0} == actual_response + + +@pytest.mark.asyncio +async def test_git_fetch_fail(): + with patch("jupyterlab_git.git.execute") as mock_execute: + # Given + mock_execute.return_value = maybe_future((1, "", "error")) + + # When + actual_response = await Git().fetch(path="test_path") + + # Then + mock_execute.assert_called_once_with( + ["git", "fetch", "--all", "--prune"], + cwd="test_path", + env={**os.environ, "GIT_TERMINAL_PROMPT": "0"}, + ) + assert { + "code": 1, + "command": "git fetch --all --prune", + "error": "error", + } == actual_response + + +@pytest.mark.asyncio +async def test_git_fetch_with_auth_success(): + with patch("jupyterlab_git.git.execute") as mock_execute: + # Given + mock_execute.return_value = maybe_future((0, "", "")) + + # When + actual_response = await Git().fetch( + path="test_path", auth={"username": "test_user", "password": "test_pass"} + ) + + # Then + mock_execute.assert_called_once_with( + ["git", "fetch", "--all", "--prune"], + username="test_user", + password="test_pass", + cwd="test_path", + env={**os.environ, "GIT_TERMINAL_PROMPT": "1"}, + ) + assert {"code": 0} == actual_response + + +@pytest.mark.asyncio +async def test_git_fetch_with_auth_fail(): + with patch("jupyterlab_git.git.execute") as mock_execute: + # Given + mock_execute.return_value = maybe_future( + ( + 128, + "", + "remote: Invalid username or password.\r\nfatal: Authentication failed for 'test_repo'", + ) + ) + + # When + actual_response = await Git().fetch( + path="test_path", auth={"username": "test_user", "password": "test_pass"} + ) + + # Then + mock_execute.assert_called_once_with( + ["git", "fetch", "--all", "--prune"], + username="test_user", + password="test_pass", + cwd="test_path", + env={**os.environ, "GIT_TERMINAL_PROMPT": "1"}, + ) + assert { + "code": 128, + "command": "git fetch --all --prune", + "error": "remote: Invalid username or password.\r\nfatal: Authentication failed for 'test_repo'", + } == actual_response + + +@pytest.mark.asyncio +async def test_git_fetch_with_cache_credentials(): + with patch("jupyterlab_git.git.execute") as mock_execute: + # Given + mock_execute.return_value = maybe_future((0, "", "")) + + # When + actual_response = await Git().fetch(path="test_path", cache_credentials=True) + + # Then + mock_execute.assert_called_once_with( + ["git", "fetch", "--all", "--prune"], + cwd="test_path", + env={**os.environ, "GIT_TERMINAL_PROMPT": "0"}, + ) + assert {"code": 0} == actual_response + + +@pytest.mark.asyncio +async def test_git_fetch_with_auth_and_cache_credentials(): + with patch("jupyterlab_git.git.execute") as mock_authentication: + # Given + default_config = JupyterLabGit() + credential_helper = default_config.credential_helper + test_path = "test_path" + mock_authentication.side_effect = [ + maybe_future((0, "", "")), + maybe_future((0, "", "")), + maybe_future((0, "", "")), + ] + # When + actual_response = await Git(config=default_config).fetch( + path=test_path, + auth={"username": "test_user", "password": "test_pass"}, + cache_credentials=True, + ) + + # Then + assert mock_authentication.call_count == 3 + mock_authentication.assert_has_calls( + [ + call(["git", "config", "--list"], cwd=test_path), + call( + ["git", "config", "--add", "credential.helper", credential_helper], + cwd=test_path, + ), + call( + ["git", "fetch", "--all", "--prune"], + username="test_user", + password="test_pass", + cwd=test_path, + env={**os.environ, "GIT_TERMINAL_PROMPT": "1"}, + ), + ] + ) + assert {"code": 0} == actual_response + + +@pytest.mark.asyncio +async def test_git_fetch_with_auth_and_cache_credentials_and_existing_credential_helper(): + with patch("jupyterlab_git.git.execute") as mock_authentication: + # Given + default_config = JupyterLabGit() + test_path = "test_path" + mock_authentication.side_effect = [ + maybe_future((0, "credential.helper=something", "")), + maybe_future((0, "", "")), + ] + # When + actual_response = await Git(config=default_config).fetch( + path="test_path", + auth={"username": "test_user", "password": "test_pass"}, + cache_credentials=True, + ) + + # Then + assert mock_authentication.call_count == 2 + mock_authentication.assert_has_calls( + [ + call(["git", "config", "--list"], cwd=test_path), + call( + ["git", "fetch", "--all", "--prune"], + username="test_user", + password="test_pass", + cwd=test_path, + env={**os.environ, "GIT_TERMINAL_PROMPT": "1"}, + ), + ] + ) + assert {"code": 0} == actual_response diff --git a/jupyterlab_git/tests/test_pushpull.py b/jupyterlab_git/tests/test_pushpull.py index 04cf9288e..f3ecb4fe9 100644 --- a/jupyterlab_git/tests/test_pushpull.py +++ b/jupyterlab_git/tests/test_pushpull.py @@ -1,7 +1,9 @@ +import os from unittest.mock import call, patch import pytest +from jupyterlab_git import JupyterLabGit from jupyterlab_git.git import Git from .testutils import maybe_future @@ -173,6 +175,102 @@ async def test_git_pull_with_auth_success_and_conflict_fail(): } == actual_response +@pytest.mark.asyncio +async def test_git_pull_with_cache_credentials(): + with patch("jupyterlab_git.git.execute") as mock_execute: + # Given + mock_execute.return_value = maybe_future((0, "", "")) + + # When + actual_response = await Git().pull("test_curr_path", cache_credentials=True) + + # Then + mock_execute.assert_called_once_with( + ["git", "pull", "--no-commit"], + cwd="test_curr_path", + env={**os.environ, "GIT_TERMINAL_PROMPT": "0"}, + ) + assert {"code": 0, "message": ""} == actual_response + + +@pytest.mark.asyncio +async def test_git_pull_with_auth_and_cache_credentials(): + with patch("jupyterlab_git.git.execute") as mock_execute_with_authentication: + # Given + default_config = JupyterLabGit() + credential_helper = default_config.credential_helper + mock_execute_with_authentication.side_effect = [ + maybe_future((0, "", "")), + maybe_future((0, "", "")), + maybe_future((0, "", "")), + ] + + # When + auth = {"username": "asdf", "password": "qwerty"} + actual_response = await Git(config=default_config).pull( + "test_curr_path", auth, cache_credentials=True + ) + + # Then + assert mock_execute_with_authentication.call_count == 3 + mock_execute_with_authentication.assert_has_calls( + [ + call( + ["git", "config", "--list"], + cwd="test_curr_path", + ), + call( + ["git", "config", "--add", "credential.helper", credential_helper], + cwd="test_curr_path", + ), + call( + ["git", "pull", "--no-commit"], + username="asdf", + password="qwerty", + cwd="test_curr_path", + env={**os.environ, "GIT_TERMINAL_PROMPT": "1"}, + ), + ] + ) + assert {"code": 0, "message": ""} == actual_response + + +@pytest.mark.asyncio +async def test_git_pull_with_auth_and_cache_credentials_and_existing_credential_helper(): + with patch("jupyterlab_git.git.execute") as mock_execute_with_authentication: + # Given + default_config = JupyterLabGit() + mock_execute_with_authentication.side_effect = [ + maybe_future((0, "credential.helper=something", "")), + maybe_future((0, "", "")), + ] + + # When + auth = {"username": "asdf", "password": "qwerty"} + actual_response = await Git(config=default_config).pull( + "test_curr_path", auth, cache_credentials=True + ) + + # Then + assert mock_execute_with_authentication.call_count == 2 + mock_execute_with_authentication.assert_has_calls( + [ + call( + ["git", "config", "--list"], + cwd="test_curr_path", + ), + call( + ["git", "pull", "--no-commit"], + username="asdf", + password="qwerty", + cwd="test_curr_path", + env={**os.environ, "GIT_TERMINAL_PROMPT": "1"}, + ), + ] + ) + assert {"code": 0, "message": ""} == actual_response + + @pytest.mark.asyncio async def test_git_push_fail(): with patch("os.environ", {"TEST": "test"}): @@ -279,3 +377,101 @@ async def test_git_push_with_auth_success(): env={"TEST": "test", "GIT_TERMINAL_PROMPT": "1"}, ) assert {"code": 0, "message": output} == actual_response + + +@pytest.mark.asyncio +async def test_git_push_with_cache_credentials(): + with patch("jupyterlab_git.git.execute") as mock_execute: + # Given + mock_execute.return_value = maybe_future((0, "", "")) + + # When + actual_response = await Git().push( + ".", "HEAD:test_master", "test_curr_path", cache_credentials=True + ) + + # Then + mock_execute.assert_called_once_with( + ["git", "push", ".", "HEAD:test_master"], + cwd="test_curr_path", + env={**os.environ, "GIT_TERMINAL_PROMPT": "0"}, + ) + assert {"code": 0, "message": ""} == actual_response + + +@pytest.mark.asyncio +async def test_git_push_with_auth_and_cache_credentials(): + with patch("jupyterlab_git.git.execute") as mock_execute_with_authentication: + # Given + default_config = JupyterLabGit() + credential_helper = default_config.credential_helper + mock_execute_with_authentication.side_effect = [ + maybe_future((0, "", "")), + maybe_future((0, "", "")), + maybe_future((0, "", "")), + ] + + # When + auth = {"username": "asdf", "password": "qwerty"} + actual_response = await Git(config=default_config).push( + ".", "HEAD:test_master", "test_curr_path", auth, cache_credentials=True + ) + + # Then + assert mock_execute_with_authentication.call_count == 3 + mock_execute_with_authentication.assert_has_calls( + [ + call( + ["git", "config", "--list"], + cwd="test_curr_path", + ), + call( + ["git", "config", "--add", "credential.helper", credential_helper], + cwd="test_curr_path", + ), + call( + ["git", "push", ".", "HEAD:test_master"], + username="asdf", + password="qwerty", + cwd="test_curr_path", + env={**os.environ, "GIT_TERMINAL_PROMPT": "1"}, + ), + ] + ) + assert {"code": 0, "message": ""} == actual_response + + +@pytest.mark.asyncio +async def test_git_push_with_auth_and_cache_credentials_and_existing_credential_helper(): + with patch("jupyterlab_git.git.execute") as mock_execute_with_authentication: + # Given + default_config = JupyterLabGit() + mock_execute_with_authentication.side_effect = [ + maybe_future((0, "credential.helper=something", "")), + maybe_future((0, "", "")), + ] + + # When + auth = {"username": "asdf", "password": "qwerty"} + actual_response = await Git(config=default_config).push( + ".", "HEAD:test_master", "test_curr_path", auth, cache_credentials=True + ) + + # Then + assert mock_execute_with_authentication.call_count == 2 + mock_execute_with_authentication.assert_has_calls( + [ + call( + ["git", "config", "--list"], + cwd="test_curr_path", + ), + call( + ["git", "push", ".", "HEAD:test_master"], + username="asdf", + password="qwerty", + cwd="test_curr_path", + env={**os.environ, "GIT_TERMINAL_PROMPT": "1"}, + ), + ] + ) + assert {"code": 0, "message": ""} == actual_response From 4199e4838b7e7bda8d76b447ccb8110a09c5c1e8 Mon Sep 17 00:00:00 2001 From: Dat Quach Date: Wed, 23 Mar 2022 03:11:11 -0700 Subject: [PATCH 06/44] Extend `Git.IAuth` and `CredentialsBox` 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. --- src/tokens.ts | 3 ++- src/widgets/CredentialsBox.tsx | 23 ++++++++++++++++++++--- style/base.css | 1 + style/credentials-box.css | 20 ++++++++++++++++++++ 4 files changed, 43 insertions(+), 4 deletions(-) create mode 100644 style/credentials-box.css diff --git a/src/tokens.ts b/src/tokens.ts index f682749ac..6d83b621b 100644 --- a/src/tokens.ts +++ b/src/tokens.ts @@ -919,11 +919,12 @@ export namespace Git { } /** - * Interface for the Git Auth request. + * Interface for the Git Auth request with credentials caching option. */ export interface IAuth { username: string; password: string; + cacheCredentials?: boolean; } /** diff --git a/src/widgets/CredentialsBox.tsx b/src/widgets/CredentialsBox.tsx index e20d4b337..c56011673 100755 --- a/src/widgets/CredentialsBox.tsx +++ b/src/widgets/CredentialsBox.tsx @@ -23,15 +23,21 @@ export class GitCredentialsForm private createBody(textContent: string, warningContent: string): HTMLElement { const node = document.createElement('div'); const label = document.createElement('label'); + + const checkboxLabel = document.createElement('label'); + this._checkboxCacheCredentials = document.createElement('input'); + const checkboxText = document.createElement('span'); + this._user = document.createElement('input'); + this._user.type = 'text'; this._password = document.createElement('input'); this._password.type = 'password'; const text = document.createElement('span'); const warning = document.createElement('div'); - node.className = 'jp-RedirectForm'; - warning.className = 'jp-RedirectForm-warning'; + node.className = 'jp-CredentialsBox'; + warning.className = 'jp-CredentialsBox-warning'; text.textContent = textContent; warning.textContent = warningContent; this._user.placeholder = this._trans.__('username'); @@ -39,11 +45,20 @@ export class GitCredentialsForm 'password / personal access token' ); + checkboxLabel.className = 'jp-CredentialsBox-label-checkbox'; + this._checkboxCacheCredentials.type = 'checkbox'; + checkboxText.textContent = this._trans.__('Save my login temporarily'); + label.appendChild(text); label.appendChild(this._user); label.appendChild(this._password); node.appendChild(label); node.appendChild(warning); + + checkboxLabel.appendChild(this._checkboxCacheCredentials); + checkboxLabel.appendChild(checkboxText); + node.appendChild(checkboxLabel); + return node; } @@ -53,10 +68,12 @@ export class GitCredentialsForm getValue(): Git.IAuth { return { username: this._user.value, - password: this._password.value + password: this._password.value, + cacheCredentials: this._checkboxCacheCredentials.checked }; } protected _trans: TranslationBundle; private _user: HTMLInputElement; private _password: HTMLInputElement; + private _checkboxCacheCredentials: HTMLInputElement; } diff --git a/style/base.css b/style/base.css index af3ce1e8c..7bbb128ca 100644 --- a/style/base.css +++ b/style/base.css @@ -3,6 +3,7 @@ | Distributed under the terms of the Modified BSD License. |----------------------------------------------------------------------------*/ +@import url('credentials-box.css'); @import url('diff-common.css'); @import url('diff-nb.css'); @import url('diff-text.css'); diff --git a/style/credentials-box.css b/style/credentials-box.css new file mode 100644 index 000000000..68f11f3fe --- /dev/null +++ b/style/credentials-box.css @@ -0,0 +1,20 @@ +.jp-CredentialsBox input[type='text'], +.jp-CredentialsBox input[type='password'] { + display: block; + width: 100%; + margin-top: 10px; + margin-bottom: 10px; +} + +.jp-CredentialsBox input[type='checkbox'] { + display: inline-block; +} + +.jp-CredentialsBox-warning { + color: var(--jp-warn-color0); +} + +.jp-CredentialsBox-label-checkbox { + display: flex; + align-items: center; +} From e718d4b34ea89cb6f7fa510f7eeb015d833893f3 Mon Sep 17 00:00:00 2001 From: Dat Quach Date: Wed, 23 Mar 2022 03:23:36 -0700 Subject: [PATCH 07/44] Extend `clone()`, `push()`, `pull()` in model Let each of these methods also include `auth?.cacheCredentials` to POST to the backend. --- src/model.ts | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/src/model.ts b/src/model.ts index 80543b844..e895bd25a 100644 --- a/src/model.ts +++ b/src/model.ts @@ -587,7 +587,8 @@ export class GitExtension implements IGitExtension { 'POST', { clone_url: url, - auth: auth as any + auth: auth as any, + cache_credentials: auth?.cacheCredentials } ); } @@ -835,7 +836,8 @@ export class GitExtension implements IGitExtension { cancel_on_conflict: (this._settings?.composite[ 'cancelPullMergeConflict' - ] as boolean) || false + ] as boolean) || false, + cache_credentials: auth?.cacheCredentials } ); } @@ -865,7 +867,8 @@ export class GitExtension implements IGitExtension { 'POST', { auth: auth as any, - force: force + force: force, + cache_credentials: auth?.cacheCredentials } ); } From 9a236c384a723570c1a49ec860afe8e32e604233 Mon Sep 17 00:00:00 2001 From: Dat Quach Date: Wed, 23 Mar 2022 03:41:14 -0700 Subject: [PATCH 08/44] Extend model for `_fetchRemotes()` Add extra class members to take care of the fetch poll when credentials are required. --- src/model.ts | 35 +++++++++++++++++++++++++++++++++++ src/tokens.ts | 15 +++++++++++++++ 2 files changed, 50 insertions(+) diff --git a/src/model.ts b/src/model.ts index e895bd25a..ec7191d86 100644 --- a/src/model.ts +++ b/src/model.ts @@ -291,6 +291,38 @@ export class GitExtension implements IGitExtension { return this._dirtyStagedFilesStatusChanged; } + /** + * Boolean indicating whether credentials are required from the user. + */ + get credentialsRequired(): boolean { + return this._credentialsRequired; + } + + set credentialsRequired(value: boolean) { + if (this._credentialsRequired !== value) { + this._credentialsRequired = value; + this._credentialsRequiredSignal.emit(value); + } + } + + /** + * A signal emitted whenever credentials are required, or are not required anymore. + */ + get credentialsRequiredSignal(): ISignal { + return this._credentialsRequiredSignal; + } + + /** + * Boolean indicating whether the fetch poll is blocked. + */ + get fetchBlocked(): boolean { + return this._fetchBlocked; + } + + set fetchBlocked(value: boolean) { + this._fetchBlocked = value; + } + /** * Get the current markers * @@ -1609,6 +1641,8 @@ export class GitExtension implements IGitExtension { private _changeUpstreamNotified: Git.IStatusFile[] = []; private _selectedHistoryFile: Git.IStatusFile | null = null; private _hasDirtyStagedFiles = false; + private _credentialsRequired = false; + private _fetchBlocked = false; private _headChanged = new Signal(this); private _markChanged = new Signal(this); @@ -1628,6 +1662,7 @@ export class GitExtension implements IGitExtension { private _dirtyStagedFilesStatusChanged = new Signal( this ); + private _credentialsRequiredSignal = new Signal(this); } export class BranchMarker implements Git.IBranchMarker { diff --git a/src/tokens.ts b/src/tokens.ts index 6d83b621b..57e5274ea 100644 --- a/src/tokens.ts +++ b/src/tokens.ts @@ -67,6 +67,21 @@ export interface IGitExtension extends IDisposable { */ hasDirtyStagedFiles: boolean; + /** + * Boolean indicating whether credentials are required from the user. + */ + credentialsRequired: boolean; + + /** + * A signal emitted whenever credentials are required, or are not required anymore. + */ + readonly credentialsRequiredSignal: ISignal; + + /** + * Boolean indicating whether the fetch poll is blocked. + */ + fetchBlocked: boolean; + /** * Git repository status. */ From 292b25c822bbad2c7d12c28aa0553fead005c276 Mon Sep 17 00:00:00 2001 From: Dat Quach Date: Wed, 23 Mar 2022 03:44:56 -0700 Subject: [PATCH 09/44] Implement `fetch()` 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. --- src/model.ts | 42 +++++++++++++++++++++++++++++++++++++++--- src/tokens.ts | 12 ++++++++++++ 2 files changed, 51 insertions(+), 3 deletions(-) diff --git a/src/model.ts b/src/model.ts index ec7191d86..276e8b0b4 100644 --- a/src/model.ts +++ b/src/model.ts @@ -5,7 +5,7 @@ import { ISettingRegistry } from '@jupyterlab/settingregistry'; import { JSONObject } from '@lumino/coreutils'; import { Poll } from '@lumino/polling'; import { ISignal, Signal } from '@lumino/signaling'; -import { requestAPI } from './git'; +import { AUTH_ERROR_MESSAGES, requestAPI } from './git'; import { TaskHandler } from './taskhandler'; import { Git, IGitExtension } from './tokens'; import { decodeStage } from './utils'; @@ -763,6 +763,34 @@ export class GitExtension implements IGitExtension { await this.refreshStatus(); } + /** + * Fetch to get ahead/behind status + * + * @param auth - remote authentication information + * @returns promise which resolves upon fetching + * + * @throws {Git.NotInRepository} If the current path is not a Git repository + * @throws {Git.GitResponseError} If the server response is not ok + * @throws {ServerConnection.NetworkError} If the request cannot be made + */ + async fetch(auth?: Git.IAuth): Promise { + const path = await this._getPathRepository(); + const data = this._taskHandler.execute( + 'git:fetch:remote', + async () => { + return await requestAPI( + URLExt.join(path, 'remote', 'fetch'), + 'POST', + { + auth: auth as any, + cache_credentials: auth?.cacheCredentials + } + ); + } + ); + return data; + } + /** * Return the path of a file relative to the Jupyter server root. * @@ -1525,13 +1553,21 @@ export class GitExtension implements IGitExtension { /** * Fetch poll action. + * This is blocked if Git credentials are required. */ private _fetchRemotes = async (): Promise => { + if (this.credentialsRequired) return; try { - const path = await this._getPathRepository(); - await requestAPI(URLExt.join(path, 'remote', 'fetch'), 'POST'); + await this.fetch(); } catch (error) { console.error('Failed to fetch remotes', error); + if ( + AUTH_ERROR_MESSAGES.some( + errorMessage => (error as Error).message.indexOf(errorMessage) > -1 + ) + ) { + this.credentialsRequired = true; + } } }; diff --git a/src/tokens.ts b/src/tokens.ts index 57e5274ea..fdecbb687 100644 --- a/src/tokens.ts +++ b/src/tokens.ts @@ -300,6 +300,18 @@ export interface IGitExtension extends IDisposable { */ ensureGitignore(): Promise; + /** + * Fetch to get ahead/behind status + * + * @param auth - remote authentication information + * @returns promise which resolves upon fetching + * + * @throws {Git.NotInRepository} If the current path is not a Git repository + * @throws {Git.GitResponseError} If the server response is not ok + * @throws {ServerConnection.NetworkError} If the request cannot be made + */ + fetch(auth?: Git.IAuth): Promise; + /** * Match files status information based on a provided file path. * From 8e2c78063c7c265cd0d27f5b53e3e1c95a43d3f8 Mon Sep 17 00:00:00 2001 From: Dat Quach Date: Wed, 23 Mar 2022 04:00:07 -0700 Subject: [PATCH 10/44] Remove a redundant member `_fetchBlocked` is not needed. --- src/model.ts | 12 ------------ src/tokens.ts | 5 ----- 2 files changed, 17 deletions(-) diff --git a/src/model.ts b/src/model.ts index 276e8b0b4..3fac915a9 100644 --- a/src/model.ts +++ b/src/model.ts @@ -312,17 +312,6 @@ export class GitExtension implements IGitExtension { return this._credentialsRequiredSignal; } - /** - * Boolean indicating whether the fetch poll is blocked. - */ - get fetchBlocked(): boolean { - return this._fetchBlocked; - } - - set fetchBlocked(value: boolean) { - this._fetchBlocked = value; - } - /** * Get the current markers * @@ -1678,7 +1667,6 @@ export class GitExtension implements IGitExtension { private _selectedHistoryFile: Git.IStatusFile | null = null; private _hasDirtyStagedFiles = false; private _credentialsRequired = false; - private _fetchBlocked = false; private _headChanged = new Signal(this); private _markChanged = new Signal(this); diff --git a/src/tokens.ts b/src/tokens.ts index fdecbb687..fa3486f2c 100644 --- a/src/tokens.ts +++ b/src/tokens.ts @@ -77,11 +77,6 @@ export interface IGitExtension extends IDisposable { */ readonly credentialsRequiredSignal: ISignal; - /** - * Boolean indicating whether the fetch poll is blocked. - */ - fetchBlocked: boolean; - /** * Git repository status. */ From 84bd60e5e2f6660b8197ede977a73ff32ab954c8 Mon Sep 17 00:00:00 2001 From: Dat Quach Date: Wed, 23 Mar 2022 04:03:17 -0700 Subject: [PATCH 11/44] Extend `showGitOperationDialog()` This dialog now supports `fetch` operation, and upon successful authentication, unblocks the fetch poll by changing the credentials requirement state to `false`. --- src/commandsAndMenu.tsx | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/commandsAndMenu.tsx b/src/commandsAndMenu.tsx index 8c588be17..d3d3cd8d7 100644 --- a/src/commandsAndMenu.tsx +++ b/src/commandsAndMenu.tsx @@ -66,7 +66,8 @@ export enum Operation { Clone = 'Clone', Pull = 'Pull', Push = 'Push', - ForcePush = 'ForcePush' + ForcePush = 'ForcePush', + Fetch = 'Fetch' } interface IFileDiffArgument { @@ -1388,6 +1389,10 @@ export async function showGitOperationDialog( case Operation.ForcePush: result = await model.push(authentication, true); break; + case Operation.Fetch: + result = await model.fetch(authentication); + model.credentialsRequired = false; + break; default: result = { code: -1, message: 'Unknown git command' }; break; From 996a5e7d2f4c97d0b5d00e019266f0000a195908 Mon Sep 17 00:00:00 2001 From: Dat Quach Date: Wed, 23 Mar 2022 04:05:58 -0700 Subject: [PATCH 12/44] Match the `IGitExtension` with `push()` in model Also let `showGitOperationDialog()` accept `IGitExtension` `model` instead of the concrete class `GitExtension`. --- src/commandsAndMenu.tsx | 2 +- src/tokens.ts | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/src/commandsAndMenu.tsx b/src/commandsAndMenu.tsx index d3d3cd8d7..8d49f87f3 100644 --- a/src/commandsAndMenu.tsx +++ b/src/commandsAndMenu.tsx @@ -1364,7 +1364,7 @@ export function addFileBrowserContextMenu( * @returns Promise for displaying a dialog */ export async function showGitOperationDialog( - model: GitExtension, + model: IGitExtension, operation: Operation, trans: TranslationBundle, args?: T, diff --git a/src/tokens.ts b/src/tokens.ts index fa3486f2c..061695763 100644 --- a/src/tokens.ts +++ b/src/tokens.ts @@ -398,13 +398,14 @@ export interface IGitExtension extends IDisposable { * Push local changes to a remote repository. * * @param auth - remote authentication information + * @param force - whether or not to force the push * @returns promise which resolves upon pushing changes * * @throws {Git.NotInRepository} If the current path is not a Git repository * @throws {Git.GitResponseError} If the server response is not ok * @throws {ServerConnection.NetworkError} If the request cannot be made */ - push(auth?: Git.IAuth): Promise; + push(auth?: Git.IAuth, force?: boolean): Promise; /** * General Git refresh From 4f0badf8664c77658788f65f3df33eb544579075 Mon Sep 17 00:00:00 2001 From: Dat Quach Date: Wed, 23 Mar 2022 04:13:24 -0700 Subject: [PATCH 13/44] Extend `StatusWidget` The component now has a boolean for Git credentials requirement. Also add a callback connected to the model to update that boolean. --- src/components/StatusWidget.tsx | 52 +++++++++++++++++++++++++++++++++ 1 file changed, 52 insertions(+) diff --git a/src/components/StatusWidget.tsx b/src/components/StatusWidget.tsx index 880f2881e..d576109e3 100644 --- a/src/components/StatusWidget.tsx +++ b/src/components/StatusWidget.tsx @@ -32,6 +32,20 @@ export class StatusWidget extends ReactWidget { } } + /** + * Boolean indicating whether credentials are required. + */ + get waitingForCredentials(): boolean { + return this._waitingForCredentials; + } + + /** + * Sets the boolean indicating whether credentials are required. + */ + set waitingForCredentials(value: boolean) { + this._waitingForCredentials = value; + } + render(): JSX.Element { return (
@@ -72,6 +86,11 @@ export class StatusWidget extends ReactWidget { */ private _status = ''; + /** + * Boolean indicating whether credentials are required. + */ + private _waitingForCredentials: boolean; + private _trans: TranslationBundle; } @@ -92,8 +111,14 @@ export function addStatusBarWidget( const callback = Private.createEventCallback(statusWidget); model.taskChanged.connect(callback); + + const credentialsRequiredCallback = + Private.createCredentialsRequiredCallback(statusWidget); + model.credentialsRequiredSignal.connect(credentialsRequiredCallback); + statusWidget.disposed.connect(() => { model.taskChanged.disconnect(callback); + model.credentialsRequiredSignal.disconnect(credentialsRequiredCallback); }); } /* eslint-disable no-inner-declarations */ @@ -200,5 +225,32 @@ namespace Private { return settings.composite.displayStatus as boolean; } } + + /** + * Returns a callback invoked when credentials are required. + * + * @private + * @param widget - status widget + * @returns callback + */ + export function createCredentialsRequiredCallback( + widget: StatusWidget + ): (model: IGitExtension, value: boolean) => void { + /** + * Callback invoked when credentials are required. + * + * @private + * @param model - extension model + * @param value - boolean indicating whether credentials are required + * @returns void + */ + function callbackCredentialsRequired( + model: IGitExtension, + value: boolean + ): void { + widget.waitingForCredentials = value; + } + return callbackCredentialsRequired; + } } /* eslint-enable no-inner-declarations */ From 5abed4d15c4ee685228b333930a26b5cf4dc2a75 Mon Sep 17 00:00:00 2001 From: Dat Quach Date: Wed, 23 Mar 2022 04:27:27 -0700 Subject: [PATCH 14/44] Reuse `showGitOperationDialog()` in `StatusWidget` Inject the model to the component and reuse `showGitOperationDialog()` for the `fetch` operation. --- src/components/StatusWidget.tsx | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/src/components/StatusWidget.tsx b/src/components/StatusWidget.tsx index d576109e3..d20dd2f6d 100644 --- a/src/components/StatusWidget.tsx +++ b/src/components/StatusWidget.tsx @@ -3,6 +3,7 @@ import { ISettingRegistry } from '@jupyterlab/settingregistry'; import { IStatusBar } from '@jupyterlab/statusbar'; import { TranslationBundle } from '@jupyterlab/translation'; import React from 'react'; +import { Operation, showGitOperationDialog } from '../commandsAndMenu'; import { gitIcon } from '../style/icons'; import { statusAnimatedIconClass, @@ -17,8 +18,9 @@ export class StatusWidget extends ReactWidget { * @param trans - The language translator * @returns widget */ - constructor(trans: TranslationBundle) { + constructor(model: IGitExtension, trans: TranslationBundle) { super(); + this._model = model; this._trans = trans; } @@ -61,6 +63,14 @@ export class StatusWidget extends ReactWidget { ); } + async _showGitOperationDialog(): Promise { + try { + await showGitOperationDialog(this._model, Operation.Fetch, this._trans); + } catch (error) { + console.error('Encountered an error when fetching. Error:', error); + } + } + /** * Locks the status widget to prevent updates. * @@ -91,6 +101,7 @@ export class StatusWidget extends ReactWidget { */ private _waitingForCredentials: boolean; + private _model: IGitExtension; private _trans: TranslationBundle; } @@ -101,7 +112,7 @@ export function addStatusBarWidget( trans: TranslationBundle ): void { // Add a status bar widget to provide Git status updates: - const statusWidget = new StatusWidget(trans); + const statusWidget = new StatusWidget(model, trans); statusBar.registerStatusItem('git-status', { align: 'left', item: statusWidget, From 71deeec02e0a1a708c17eb389bfc20a066b427d0 Mon Sep 17 00:00:00 2001 From: Dat Quach Date: Wed, 23 Mar 2022 04:32:47 -0700 Subject: [PATCH 15/44] Add a notification dot to `StatusWidget` 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. --- src/components/StatusWidget.tsx | 33 ++++++++++++++++++++++++++------- src/style/StatusWidget.ts | 10 ++++++++++ 2 files changed, 36 insertions(+), 7 deletions(-) diff --git a/src/components/StatusWidget.tsx b/src/components/StatusWidget.tsx index d20dd2f6d..0da48b989 100644 --- a/src/components/StatusWidget.tsx +++ b/src/components/StatusWidget.tsx @@ -2,15 +2,20 @@ import { ReactWidget } from '@jupyterlab/apputils'; import { ISettingRegistry } from '@jupyterlab/settingregistry'; import { IStatusBar } from '@jupyterlab/statusbar'; import { TranslationBundle } from '@jupyterlab/translation'; +import { Badge } from '@material-ui/core'; import React from 'react'; +import { classes } from 'typestyle'; import { Operation, showGitOperationDialog } from '../commandsAndMenu'; import { gitIcon } from '../style/icons'; import { + badgeClass, statusAnimatedIconClass, statusIconClass } from '../style/StatusWidget'; +import { toolbarButtonClass } from '../style/Toolbar'; import { IGitExtension } from '../tokens'; import { sleep } from '../utils'; +import { ActionButton } from './ActionButton'; export class StatusWidget extends ReactWidget { /** @@ -50,16 +55,30 @@ export class StatusWidget extends ReactWidget { render(): JSX.Element { return ( -
- + this._showGitOperationDialog() + : undefined + } + title={ + this._waitingForCredentials + ? `Git: ${this._trans.__('credentials required')}` + : `Git: ${this._trans.__(this._status)}` } - left={'1px'} - top={'3px'} - stylesheet={'statusBar'} /> -
+ ); } diff --git a/src/style/StatusWidget.ts b/src/style/StatusWidget.ts index 8eac4483d..c00d0d4a8 100644 --- a/src/style/StatusWidget.ts +++ b/src/style/StatusWidget.ts @@ -28,3 +28,13 @@ export const statusAnimatedIconClass = style({ } } }); + +export const badgeClass = style({ + $nest: { + '& > .MuiBadge-badge': { + top: 6, + right: 15, + backgroundColor: 'var(--jp-warn-color1)' + } + } +}); From a57827227a8d7c4b1a6e3ccdea639f160af1ad7e Mon Sep 17 00:00:00 2001 From: Dat Quach Date: Wed, 23 Mar 2022 05:09:16 -0700 Subject: [PATCH 16/44] Implement `ensure_git_credential_cache_daemon()` This method spawns a Git credential cache daemon so that the caching mechanism can work. --- jupyterlab_git/git.py | 72 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 72 insertions(+) diff --git a/jupyterlab_git/git.py b/jupyterlab_git/git.py index 6d5a2e7df..16a3a89ea 100644 --- a/jupyterlab_git/git.py +++ b/jupyterlab_git/git.py @@ -8,6 +8,7 @@ import shlex import subprocess import traceback +from typing import Any, Dict, List, Optional from urllib.parse import unquote import nbformat @@ -174,6 +175,8 @@ class Git: A single parent class containing all of the individual git methods in it. """ + _GIT_CREDENTIAL_CACHE_DAEMON_PROCESS: subprocess.Popen = None + def __init__(self, config=None): self._config = config @@ -1599,3 +1602,72 @@ async def ensure_credential_helper(self, path): return config_response else: return {"code": 0} + + def ensure_git_credential_cache_daemon( + self, + socket: Optional[str] = None, + debug: bool = False, + new: bool = False, + cwd: Optional[str] = None, + env: Dict[str, str] = None, + ) -> int: + """ + Spawn a Git credential cache daemon with the socket file being `socket` if it does not exist. + If `debug` is `True`, the daemon will be spawned with `--debug` flag. + If `socket` is empty, it is set to `~/.git-credential-cache-daemon`. + If `new` is `True`, a daemon will be spawned, and if the daemon process is accessible, + the existing daemon process will be terminated before spawning a new one. + Otherwise, if `new` is `False`, the PID of the existing daemon process is returned. + If the daemon process is not accessible, `-1` is returned. + `cwd` and `env` are passed to the process that spawns the daemon. + """ + + if not socket: + socket = os.path.join( + os.path.expanduser("~"), ".git-credential-cache", "socket" + ) + + if socket and os.path.exists(socket): + return -1 + + if self._GIT_CREDENTIAL_CACHE_DAEMON_PROCESS is None or new is True: + + if new is True and self._GIT_CREDENTIAL_CACHE_DAEMON_PROCESS: + self._GIT_CREDENTIAL_CACHE_DAEMON_PROCESS.terminate() + + if not socket: + raise ValueError() + + socket_dir = os.path.split(socket)[0] + if socket_dir and len(socket_dir) > 0 and not os.path.isdir(socket_dir): + os.makedirs(socket_dir) + os.chmod(socket_dir, 0o700) + + args: List[str] = ["git", "credential-cache--daemon"] + + if debug: + args.append("--debug") + + args.append(socket) + + self._GIT_CREDENTIAL_CACHE_DAEMON_PROCESS = subprocess.Popen( + args, + cwd=cwd, + env=env, + ) + + get_logger().debug( + "A credential cache daemon has been spawned with PID %s", + str(self._GIT_CREDENTIAL_CACHE_DAEMON_PROCESS.pid), + ) + + elif self._GIT_CREDENTIAL_CACHE_DAEMON_PROCESS.poll(): + return self.ensure_git_credential_cache_daemon( + socket, debug, True, cwd, env + ) + + return self._GIT_CREDENTIAL_CACHE_DAEMON_PROCESS.pid + + def __del__(self): + if self._GIT_CREDENTIAL_CACHE_DAEMON_PROCESS: + self._GIT_CREDENTIAL_CACHE_DAEMON_PROCESS.terminate() From 2754f04aad34ecace7b32eae234efb2b209f6a55 Mon Sep 17 00:00:00 2001 From: Dat Quach Date: Wed, 23 Mar 2022 05:16:15 -0700 Subject: [PATCH 17/44] Extend `check/ensure_credential_helper()` 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. --- jupyterlab_git/git.py | 66 +++++++++++++++++++++++++++++++++++-------- 1 file changed, 54 insertions(+), 12 deletions(-) diff --git a/jupyterlab_git/git.py b/jupyterlab_git/git.py index 16a3a89ea..1f48f3996 100644 --- a/jupyterlab_git/git.py +++ b/jupyterlab_git/git.py @@ -7,6 +7,7 @@ import re import shlex import subprocess +import sys import traceback from typing import Any, Dict, List, Optional from urllib.parse import unquote @@ -38,6 +39,8 @@ GIT_BRANCH_STATUS = re.compile( r"^## (?P([\w\-/]+|HEAD \(no branch\)|No commits yet on \w+))(\.\.\.(?P[\w\-/]+)( \[(ahead (?P\d+))?(, )?(behind (?P\d+))?\])?)?$" ) +# Git cache as a credential helper +GIT_CREDENTIAL_HELPER_CACHE = re.compile(r"cache\b") execution_lock = tornado.locks.Lock() @@ -1572,36 +1575,75 @@ async def tag_checkout(self, path, tag): "message": error, } - async def check_credential_helper(self, path): - git_config_response = await self.config(path) + async def check_credential_helper(self, path: str) -> Dict[str, Any]: + git_config_response: Dict[str, str] = await self.config(path) if git_config_response["code"] != 0: return git_config_response git_config_kv_pairs = git_config_response["options"] has_credential_helper = "credential.helper" in git_config_kv_pairs - return {"code": 0, "result": has_credential_helper} + response = { + "code": 0, + "result": has_credential_helper, + } + + if has_credential_helper and GIT_CREDENTIAL_HELPER_CACHE.match( + git_config_kv_pairs["credential.helper"].strip() + ): + response["cache_daemon_required"] = True + + return response - async def ensure_credential_helper(self, path): + async def ensure_credential_helper( + self, path: str, env: Dict[str, str] = None + ) -> Dict[str, Any]: """ Check whether `git config --list` contains `credential.helper`. If it is not set, then it will be set to the value string for `credential.helper` defined in the server settings. + + path: str + Git path repository + env: Dict[str, str] + Environment variables """ - check_credential_helper = await self.check_credential_helper(path) + check_credential_helper_response = await self.check_credential_helper(path) - if check_credential_helper["code"] != 0: - return check_credential_helper - has_credential_helper = check_credential_helper["result"] + if check_credential_helper_response["code"] != 0: + return check_credential_helper_response + has_credential_helper = check_credential_helper_response["result"] + + cache_daemon_required = check_credential_helper_response.get( + "cache_daemon_required", False + ) + + response = {"code": -1} if not has_credential_helper: - config_response = await self.config( - path, **{"credential.helper": self._config.credential_helper} + credential_helper: str = self._config.credential_helper + response.update( + await self.config(path, **{"credential.helper": credential_helper}) ) - return config_response + if GIT_CREDENTIAL_HELPER_CACHE.match(credential_helper.strip()): + cache_daemon_required = True else: - return {"code": 0} + response["code"] = 0 + + # special case: Git credential cache + if cache_daemon_required: + if not sys.platform.startswith("win32"): + try: + self.ensure_git_credential_cache_daemon(cwd=path, env=env) + except Exception as e: + response["code"] = 2 + response["error"] = f"Unhandled error: {str(e)}" + else: + response["code"] = 1 + response["error"] = "Git credential cache cannot operate on Windows!" + + return response def ensure_git_credential_cache_daemon( self, From 7ed190805181e3dbb02ab027c76cad58427c017a Mon Sep 17 00:00:00 2001 From: Dat Quach Date: Wed, 23 Mar 2022 05:19:28 -0700 Subject: [PATCH 18/44] Refactor `push/pull/fetch/clone` Let them return an error response if the credential helper cannot be ensured. --- jupyterlab_git/git.py | 28 ++++++++++++++++++++++++---- 1 file changed, 24 insertions(+), 4 deletions(-) diff --git a/jupyterlab_git/git.py b/jupyterlab_git/git.py index 1f48f3996..8a2bbf515 100644 --- a/jupyterlab_git/git.py +++ b/jupyterlab_git/git.py @@ -280,7 +280,12 @@ async def clone(self, path, repo_url, auth=None, cache_credentials=None): env = os.environ.copy() if auth: if cache_credentials: - await self.ensure_credential_helper(path) + ensured_response = await self.ensure_credential_helper(path) + if ensured_response["code"] != 0: + return { + "code": ensured_response["code"], + "message": ensured_response.get("error", "Unknown error!"), + } env["GIT_TERMINAL_PROMPT"] = "1" code, output, error = await execute( ["git", "clone", unquote(repo_url), "-q"], @@ -319,7 +324,12 @@ async def fetch(self, path, auth=None, cache_credentials=False): env = os.environ.copy() if auth: if cache_credentials: - await self.ensure_credential_helper(path) + ensured_response = await self.ensure_credential_helper(path) + if ensured_response["code"] != 0: + return { + "code": ensured_response["code"], + "message": ensured_response.get("error", "Unknown error!"), + } env["GIT_TERMINAL_PROMPT"] = "1" code, _, fetch_error = await execute( cmd, @@ -1029,7 +1039,12 @@ async def pull( env = os.environ.copy() if auth: if cache_credentials: - await self.ensure_credential_helper(path) + ensured_response = await self.ensure_credential_helper(path) + if ensured_response["code"] != 0: + return { + "code": ensured_response["code"], + "message": ensured_response.get("error", "Unknown error!"), + } env["GIT_TERMINAL_PROMPT"] = "1" code, output, error = await execute( ["git", "pull", "--no-commit"], @@ -1095,7 +1110,12 @@ async def push( env = os.environ.copy() if auth: if cache_credentials: - await self.ensure_credential_helper(path) + ensured_response = await self.ensure_credential_helper(path) + if ensured_response["code"] != 0: + return { + "code": ensured_response["code"], + "message": ensured_response.get("error", "Unknown error!"), + } env["GIT_TERMINAL_PROMPT"] = "1" code, output, error = await execute( command, From 2a1e7baab65eb2b5dee7c9b567954a1756d1c17f Mon Sep 17 00:00:00 2001 From: Dat Quach Date: Wed, 23 Mar 2022 05:25:20 -0700 Subject: [PATCH 19/44] Refactor and adjust tests to adopt new changes - 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. --- jupyterlab_git/tests/test_clone.py | 94 ++++++----- jupyterlab_git/tests/test_fetch.py | 107 ++++++------ jupyterlab_git/tests/test_pushpull.py | 226 +++++++++++++++----------- 3 files changed, 243 insertions(+), 184 deletions(-) diff --git a/jupyterlab_git/tests/test_clone.py b/jupyterlab_git/tests/test_clone.py index bdc717f18..4bd981ff0 100644 --- a/jupyterlab_git/tests/test_clone.py +++ b/jupyterlab_git/tests/test_clone.py @@ -176,7 +176,7 @@ async def test_git_clone_with_cache_credentials(): ) # Then - mock_execute.assert_called_once_with( + mock_execute.assert_awaited_once_with( ["git", "clone", "ghjkhjkl"], cwd=test_path, env={**os.environ, "GIT_TERMINAL_PROMPT": "0"}, @@ -186,50 +186,66 @@ async def test_git_clone_with_cache_credentials(): @pytest.mark.asyncio async def test_git_clone_with_auth_and_cache_credentials(): - with patch("jupyterlab_git.git.execute") as mock_authentication: - # Given - default_config = JupyterLabGit() - credential_helper = default_config.credential_helper - test_path = str(Path("/bin") / "test_curr_path") - mock_authentication.side_effect = [ - maybe_future((0, "", "")), - maybe_future((0, "", "")), - maybe_future((0, "", "")), - ] - # When - auth = {"username": "asdf", "password": "qwerty"} - actual_response = await Git(config=default_config).clone( - path=test_path, repo_url="ghjkhjkl", auth=auth, cache_credentials=True - ) + with patch("sys.platform", "linux"): + with patch( + "jupyterlab_git.git.Git.ensure_git_credential_cache_daemon" + ) as mock_ensure_daemon: + mock_ensure_daemon.return_value = 0 + with patch("jupyterlab_git.git.execute") as mock_execute: + # Given + default_config = JupyterLabGit() + default_config.credential_helper = "cache" + credential_helper = default_config.credential_helper + test_path = "test_curr_path" + mock_execute.side_effect = [ + maybe_future((0, "", "")), + maybe_future((0, "", "")), + maybe_future((0, "", "")), + ] + # When + auth = {"username": "asdf", "password": "qwerty"} + actual_response = await Git(config=default_config).clone( + path=test_path, + repo_url="ghjkhjkl", + auth=auth, + cache_credentials=True, + ) - # Then - assert mock_authentication.call_count == 3 - mock_authentication.assert_has_calls( - [ - call(["git", "config", "--list"], cwd=test_path), - call( - ["git", "config", "--add", "credential.helper", credential_helper], - cwd=test_path, - ), - call( - ["git", "clone", "ghjkhjkl", "-q"], - username="asdf", - password="qwerty", - cwd=test_path, - env={**os.environ, "GIT_TERMINAL_PROMPT": "1"}, - ), - ] - ) - assert {"code": 0, "message": ""} == actual_response + # Then + assert mock_execute.await_count == 3 + mock_execute.assert_has_awaits( + [ + call(["git", "config", "--list"], cwd=test_path), + call( + [ + "git", + "config", + "--add", + "credential.helper", + credential_helper, + ], + cwd=test_path, + ), + call( + ["git", "clone", "ghjkhjkl", "-q"], + username="asdf", + password="qwerty", + cwd=test_path, + env={**os.environ, "GIT_TERMINAL_PROMPT": "1"}, + ), + ] + ) + mock_ensure_daemon.assert_called_once_with(cwd=test_path, env=None) + assert {"code": 0, "message": ""} == actual_response @pytest.mark.asyncio async def test_git_clone_with_auth_and_cache_credentials_and_existing_credential_helper(): - with patch("jupyterlab_git.git.execute") as mock_authentication: + with patch("jupyterlab_git.git.execute") as mock_execute: # Given default_config = JupyterLabGit() test_path = str(Path("/bin") / "test_curr_path") - mock_authentication.side_effect = [ + mock_execute.side_effect = [ maybe_future((0, "credential.helper=something", "")), maybe_future((0, "", "")), ] @@ -240,8 +256,8 @@ async def test_git_clone_with_auth_and_cache_credentials_and_existing_credential ) # Then - assert mock_authentication.call_count == 2 - mock_authentication.assert_has_calls( + assert mock_execute.await_count == 2 + mock_execute.assert_has_awaits( [ call(["git", "config", "--list"], cwd=test_path), call( diff --git a/jupyterlab_git/tests/test_fetch.py b/jupyterlab_git/tests/test_fetch.py index bbd40b455..fe2c7a5de 100644 --- a/jupyterlab_git/tests/test_fetch.py +++ b/jupyterlab_git/tests/test_fetch.py @@ -19,7 +19,7 @@ async def test_git_fetch_success(): actual_response = await Git().fetch(path="test_path") # Then - mock_execute.assert_called_once_with( + mock_execute.assert_awaited_once_with( ["git", "fetch", "--all", "--prune"], cwd="test_path", env={**os.environ, "GIT_TERMINAL_PROMPT": "0"}, @@ -37,7 +37,7 @@ async def test_git_fetch_fail(): actual_response = await Git().fetch(path="test_path") # Then - mock_execute.assert_called_once_with( + mock_execute.assert_awaited_once_with( ["git", "fetch", "--all", "--prune"], cwd="test_path", env={**os.environ, "GIT_TERMINAL_PROMPT": "0"}, @@ -61,7 +61,7 @@ async def test_git_fetch_with_auth_success(): ) # Then - mock_execute.assert_called_once_with( + mock_execute.assert_awaited_once_with( ["git", "fetch", "--all", "--prune"], username="test_user", password="test_pass", @@ -75,11 +75,12 @@ async def test_git_fetch_with_auth_success(): async def test_git_fetch_with_auth_fail(): with patch("jupyterlab_git.git.execute") as mock_execute: # Given + error_message = "remote: Invalid username or password.\r\nfatal: Authentication failed for 'test_repo'" mock_execute.return_value = maybe_future( ( 128, "", - "remote: Invalid username or password.\r\nfatal: Authentication failed for 'test_repo'", + error_message, ) ) @@ -89,7 +90,7 @@ async def test_git_fetch_with_auth_fail(): ) # Then - mock_execute.assert_called_once_with( + mock_execute.assert_awaited_once_with( ["git", "fetch", "--all", "--prune"], username="test_user", password="test_pass", @@ -99,7 +100,7 @@ async def test_git_fetch_with_auth_fail(): assert { "code": 128, "command": "git fetch --all --prune", - "error": "remote: Invalid username or password.\r\nfatal: Authentication failed for 'test_repo'", + "error": error_message, } == actual_response @@ -113,7 +114,7 @@ async def test_git_fetch_with_cache_credentials(): actual_response = await Git().fetch(path="test_path", cache_credentials=True) # Then - mock_execute.assert_called_once_with( + mock_execute.assert_awaited_once_with( ["git", "fetch", "--all", "--prune"], cwd="test_path", env={**os.environ, "GIT_TERMINAL_PROMPT": "0"}, @@ -123,51 +124,63 @@ async def test_git_fetch_with_cache_credentials(): @pytest.mark.asyncio async def test_git_fetch_with_auth_and_cache_credentials(): - with patch("jupyterlab_git.git.execute") as mock_authentication: - # Given - default_config = JupyterLabGit() - credential_helper = default_config.credential_helper - test_path = "test_path" - mock_authentication.side_effect = [ - maybe_future((0, "", "")), - maybe_future((0, "", "")), - maybe_future((0, "", "")), - ] - # When - actual_response = await Git(config=default_config).fetch( - path=test_path, - auth={"username": "test_user", "password": "test_pass"}, - cache_credentials=True, - ) - - # Then - assert mock_authentication.call_count == 3 - mock_authentication.assert_has_calls( - [ - call(["git", "config", "--list"], cwd=test_path), - call( - ["git", "config", "--add", "credential.helper", credential_helper], - cwd=test_path, - ), - call( - ["git", "fetch", "--all", "--prune"], - username="test_user", - password="test_pass", - cwd=test_path, - env={**os.environ, "GIT_TERMINAL_PROMPT": "1"}, - ), - ] - ) - assert {"code": 0} == actual_response + with patch("sys.platform", "linux"): + with patch( + "jupyterlab_git.git.Git.ensure_git_credential_cache_daemon" + ) as mock_ensure_daemon: + mock_ensure_daemon.return_value = 0 + with patch("jupyterlab_git.git.execute") as mock_execute: + # Given + default_config = JupyterLabGit() + credential_helper = default_config.credential_helper + test_path = "test_path" + mock_execute.side_effect = [ + maybe_future((0, "", "")), + maybe_future((0, "", "")), + maybe_future((0, "", "")), + ] + # When + actual_response = await Git(config=default_config).fetch( + path=test_path, + auth={"username": "test_user", "password": "test_pass"}, + cache_credentials=True, + ) + + # Then + assert mock_execute.await_count == 3 + mock_execute.assert_has_awaits( + [ + call(["git", "config", "--list"], cwd=test_path), + call( + [ + "git", + "config", + "--add", + "credential.helper", + credential_helper, + ], + cwd=test_path, + ), + call( + ["git", "fetch", "--all", "--prune"], + username="test_user", + password="test_pass", + cwd=test_path, + env={**os.environ, "GIT_TERMINAL_PROMPT": "1"}, + ), + ] + ) + mock_ensure_daemon.assert_called_once_with(cwd=test_path, env=None) + assert {"code": 0} == actual_response @pytest.mark.asyncio async def test_git_fetch_with_auth_and_cache_credentials_and_existing_credential_helper(): - with patch("jupyterlab_git.git.execute") as mock_authentication: + with patch("jupyterlab_git.git.execute") as mock_execute: # Given default_config = JupyterLabGit() test_path = "test_path" - mock_authentication.side_effect = [ + mock_execute.side_effect = [ maybe_future((0, "credential.helper=something", "")), maybe_future((0, "", "")), ] @@ -179,8 +192,8 @@ async def test_git_fetch_with_auth_and_cache_credentials_and_existing_credential ) # Then - assert mock_authentication.call_count == 2 - mock_authentication.assert_has_calls( + assert mock_execute.await_count == 2 + mock_execute.assert_has_awaits( [ call(["git", "config", "--list"], cwd=test_path), call( diff --git a/jupyterlab_git/tests/test_pushpull.py b/jupyterlab_git/tests/test_pushpull.py index f3ecb4fe9..e44e6a7ff 100644 --- a/jupyterlab_git/tests/test_pushpull.py +++ b/jupyterlab_git/tests/test_pushpull.py @@ -179,15 +179,16 @@ async def test_git_pull_with_auth_success_and_conflict_fail(): async def test_git_pull_with_cache_credentials(): with patch("jupyterlab_git.git.execute") as mock_execute: # Given + test_path = "test_path" mock_execute.return_value = maybe_future((0, "", "")) # When - actual_response = await Git().pull("test_curr_path", cache_credentials=True) + actual_response = await Git().pull(test_path, cache_credentials=True) # Then - mock_execute.assert_called_once_with( + mock_execute.assert_awaited_once_with( ["git", "pull", "--no-commit"], - cwd="test_curr_path", + cwd=test_path, env={**os.environ, "GIT_TERMINAL_PROMPT": "0"}, ) assert {"code": 0, "message": ""} == actual_response @@ -195,75 +196,89 @@ async def test_git_pull_with_cache_credentials(): @pytest.mark.asyncio async def test_git_pull_with_auth_and_cache_credentials(): - with patch("jupyterlab_git.git.execute") as mock_execute_with_authentication: - # Given - default_config = JupyterLabGit() - credential_helper = default_config.credential_helper - mock_execute_with_authentication.side_effect = [ - maybe_future((0, "", "")), - maybe_future((0, "", "")), - maybe_future((0, "", "")), - ] + with patch("sys.platform", "linux"): + with patch( + "jupyterlab_git.git.Git.ensure_git_credential_cache_daemon" + ) as mock_ensure_daemon: + mock_ensure_daemon.return_value = 0 + with patch("jupyterlab_git.git.execute") as mock_execute: + # Given + default_config = JupyterLabGit() + credential_helper = default_config.credential_helper + test_path = "test_path" + mock_execute.side_effect = [ + maybe_future((0, "", "")), + maybe_future((0, "", "")), + maybe_future((0, "", "")), + ] - # When - auth = {"username": "asdf", "password": "qwerty"} - actual_response = await Git(config=default_config).pull( - "test_curr_path", auth, cache_credentials=True - ) + # When + auth = {"username": "user", "password": "pass"} + actual_response = await Git(config=default_config).pull( + test_path, auth, cache_credentials=True + ) - # Then - assert mock_execute_with_authentication.call_count == 3 - mock_execute_with_authentication.assert_has_calls( - [ - call( - ["git", "config", "--list"], - cwd="test_curr_path", - ), - call( - ["git", "config", "--add", "credential.helper", credential_helper], - cwd="test_curr_path", - ), - call( - ["git", "pull", "--no-commit"], - username="asdf", - password="qwerty", - cwd="test_curr_path", - env={**os.environ, "GIT_TERMINAL_PROMPT": "1"}, - ), - ] - ) - assert {"code": 0, "message": ""} == actual_response + # Then + assert mock_execute.await_count == 3 + mock_execute.assert_has_awaits( + [ + call( + ["git", "config", "--list"], + cwd=test_path, + ), + call( + [ + "git", + "config", + "--add", + "credential.helper", + credential_helper, + ], + cwd=test_path, + ), + call( + ["git", "pull", "--no-commit"], + username="user", + password="pass", + cwd=test_path, + env={**os.environ, "GIT_TERMINAL_PROMPT": "1"}, + ), + ] + ) + mock_ensure_daemon.assert_called_once_with(cwd=test_path, env=None) + assert {"code": 0, "message": ""} == actual_response @pytest.mark.asyncio async def test_git_pull_with_auth_and_cache_credentials_and_existing_credential_helper(): - with patch("jupyterlab_git.git.execute") as mock_execute_with_authentication: + with patch("jupyterlab_git.git.execute") as mock_execute: # Given default_config = JupyterLabGit() - mock_execute_with_authentication.side_effect = [ + test_path = "test_path" + mock_execute.side_effect = [ maybe_future((0, "credential.helper=something", "")), maybe_future((0, "", "")), ] # When - auth = {"username": "asdf", "password": "qwerty"} + auth = {"username": "user", "password": "pass"} actual_response = await Git(config=default_config).pull( - "test_curr_path", auth, cache_credentials=True + test_path, auth, cache_credentials=True ) # Then - assert mock_execute_with_authentication.call_count == 2 - mock_execute_with_authentication.assert_has_calls( + assert mock_execute.await_count == 2 + mock_execute.assert_has_awaits( [ call( ["git", "config", "--list"], - cwd="test_curr_path", + cwd=test_path, ), call( ["git", "pull", "--no-commit"], - username="asdf", - password="qwerty", - cwd="test_curr_path", + username="user", + password="pass", + cwd=test_path, env={**os.environ, "GIT_TERMINAL_PROMPT": "1"}, ), ] @@ -383,17 +398,18 @@ async def test_git_push_with_auth_success(): async def test_git_push_with_cache_credentials(): with patch("jupyterlab_git.git.execute") as mock_execute: # Given + test_path = "test_path" mock_execute.return_value = maybe_future((0, "", "")) # When actual_response = await Git().push( - ".", "HEAD:test_master", "test_curr_path", cache_credentials=True + ".", "HEAD:test_master", test_path, cache_credentials=True ) # Then - mock_execute.assert_called_once_with( + mock_execute.assert_awaited_once_with( ["git", "push", ".", "HEAD:test_master"], - cwd="test_curr_path", + cwd=test_path, env={**os.environ, "GIT_TERMINAL_PROMPT": "0"}, ) assert {"code": 0, "message": ""} == actual_response @@ -401,75 +417,89 @@ async def test_git_push_with_cache_credentials(): @pytest.mark.asyncio async def test_git_push_with_auth_and_cache_credentials(): - with patch("jupyterlab_git.git.execute") as mock_execute_with_authentication: - # Given - default_config = JupyterLabGit() - credential_helper = default_config.credential_helper - mock_execute_with_authentication.side_effect = [ - maybe_future((0, "", "")), - maybe_future((0, "", "")), - maybe_future((0, "", "")), - ] + with patch("sys.platform", "linux"): + with patch( + "jupyterlab_git.git.Git.ensure_git_credential_cache_daemon" + ) as mock_ensure_daemon: + mock_ensure_daemon.return_value = 0 + with patch("jupyterlab_git.git.execute") as mock_execute: + # Given + default_config = JupyterLabGit() + credential_helper = default_config.credential_helper + test_path = "test_path" + mock_execute.side_effect = [ + maybe_future((0, "", "")), + maybe_future((0, "", "")), + maybe_future((0, "", "")), + ] - # When - auth = {"username": "asdf", "password": "qwerty"} - actual_response = await Git(config=default_config).push( - ".", "HEAD:test_master", "test_curr_path", auth, cache_credentials=True - ) + # When + auth = {"username": "user", "password": "pass"} + actual_response = await Git(config=default_config).push( + ".", "HEAD:test_master", test_path, auth, cache_credentials=True + ) - # Then - assert mock_execute_with_authentication.call_count == 3 - mock_execute_with_authentication.assert_has_calls( - [ - call( - ["git", "config", "--list"], - cwd="test_curr_path", - ), - call( - ["git", "config", "--add", "credential.helper", credential_helper], - cwd="test_curr_path", - ), - call( - ["git", "push", ".", "HEAD:test_master"], - username="asdf", - password="qwerty", - cwd="test_curr_path", - env={**os.environ, "GIT_TERMINAL_PROMPT": "1"}, - ), - ] - ) - assert {"code": 0, "message": ""} == actual_response + # Then + assert mock_execute.await_count == 3 + mock_execute.assert_has_awaits( + [ + call( + ["git", "config", "--list"], + cwd=test_path, + ), + call( + [ + "git", + "config", + "--add", + "credential.helper", + credential_helper, + ], + cwd=test_path, + ), + call( + ["git", "push", ".", "HEAD:test_master"], + username="user", + password="pass", + cwd=test_path, + env={**os.environ, "GIT_TERMINAL_PROMPT": "1"}, + ), + ] + ) + mock_ensure_daemon.assert_called_once_with(cwd=test_path, env=None) + assert {"code": 0, "message": ""} == actual_response @pytest.mark.asyncio async def test_git_push_with_auth_and_cache_credentials_and_existing_credential_helper(): - with patch("jupyterlab_git.git.execute") as mock_execute_with_authentication: + with patch("jupyterlab_git.git.execute") as mock_execute: # Given default_config = JupyterLabGit() - mock_execute_with_authentication.side_effect = [ + test_path = "test_path" + mock_execute.side_effect = [ maybe_future((0, "credential.helper=something", "")), maybe_future((0, "", "")), ] # When - auth = {"username": "asdf", "password": "qwerty"} + auth = {"username": "user", "password": "pass"} actual_response = await Git(config=default_config).push( - ".", "HEAD:test_master", "test_curr_path", auth, cache_credentials=True + ".", "HEAD:test_master", test_path, auth, cache_credentials=True ) # Then - assert mock_execute_with_authentication.call_count == 2 - mock_execute_with_authentication.assert_has_calls( + assert mock_execute.await_count == 2 + mock_execute.assert_has_awaits( [ call( ["git", "config", "--list"], - cwd="test_curr_path", + cwd=test_path, ), call( ["git", "push", ".", "HEAD:test_master"], - username="asdf", - password="qwerty", - cwd="test_curr_path", + username="user", + password="pass", + cwd=test_path, env={**os.environ, "GIT_TERMINAL_PROMPT": "1"}, ), ] From e87b6ffeb3fc731357fdd680b6fa739116ce3fdf Mon Sep 17 00:00:00 2001 From: Dat Quach Date: Wed, 23 Mar 2022 05:28:44 -0700 Subject: [PATCH 20/44] Include `"message"` in `fetch()`'s response This is so that the front-end can recognize this response with `IResultWithMessage`. --- jupyterlab_git/git.py | 1 + jupyterlab_git/tests/test_fetch.py | 2 ++ 2 files changed, 3 insertions(+) diff --git a/jupyterlab_git/git.py b/jupyterlab_git/git.py index 8a2bbf515..17e6aa66b 100644 --- a/jupyterlab_git/git.py +++ b/jupyterlab_git/git.py @@ -348,6 +348,7 @@ async def fetch(self, path, auth=None, cache_credentials=False): if code != 0: result["command"] = " ".join(cmd) result["error"] = fetch_error + result["message"] = fetch_error return result diff --git a/jupyterlab_git/tests/test_fetch.py b/jupyterlab_git/tests/test_fetch.py index fe2c7a5de..2ccb53add 100644 --- a/jupyterlab_git/tests/test_fetch.py +++ b/jupyterlab_git/tests/test_fetch.py @@ -46,6 +46,7 @@ async def test_git_fetch_fail(): "code": 1, "command": "git fetch --all --prune", "error": "error", + "message": "error", } == actual_response @@ -101,6 +102,7 @@ async def test_git_fetch_with_auth_fail(): "code": 128, "command": "git fetch --all --prune", "error": error_message, + "message": error_message, } == actual_response From 17eed00f8a16fdc7bf5707d43b61e3da24711679 Mon Sep 17 00:00:00 2001 From: Dat Quach Date: Wed, 23 Mar 2022 06:20:00 -0700 Subject: [PATCH 21/44] Lint files with ESLint --- src/model.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/model.ts b/src/model.ts index 3fac915a9..21e2fe09a 100644 --- a/src/model.ts +++ b/src/model.ts @@ -1545,7 +1545,9 @@ export class GitExtension implements IGitExtension { * This is blocked if Git credentials are required. */ private _fetchRemotes = async (): Promise => { - if (this.credentialsRequired) return; + if (this.credentialsRequired) { + return; + } try { await this.fetch(); } catch (error) { From be51d019921f2656ee2101c1e315c340b0dbc2bf Mon Sep 17 00:00:00 2001 From: Dat Quach Date: Wed, 23 Mar 2022 06:38:26 -0700 Subject: [PATCH 22/44] Adjust `test_handlers.py` to adopt new changes Add argument `cache_credentials` to assertions. --- jupyterlab_git/tests/test_handlers.py | 23 ++++++++++++++++++----- 1 file changed, 18 insertions(+), 5 deletions(-) diff --git a/jupyterlab_git/tests/test_handlers.py b/jupyterlab_git/tests/test_handlers.py index 30f5e152d..e946134d7 100644 --- a/jupyterlab_git/tests/test_handlers.py +++ b/jupyterlab_git/tests/test_handlers.py @@ -327,7 +327,7 @@ async def test_push_handler_localbranch(mock_git, jp_fetch, jp_root_dir): mock_git.get_current_branch.assert_called_with(str(local_path)) mock_git.get_upstream_branch.assert_called_with(str(local_path), "localbranch") mock_git.push.assert_called_with( - ".", "HEAD:localbranch", str(local_path), None, False, False + ".", "HEAD:localbranch", str(local_path), None, False, False, False ) assert response.code == 200 @@ -363,6 +363,7 @@ async def test_push_handler_remotebranch(mock_git, jp_fetch, jp_root_dir): None, False, False, + False, ) assert response.code == 200 @@ -463,7 +464,13 @@ async def test_push_handler_noupstream_unique_remote(mock_git, jp_fetch, jp_root mock_git.config.assert_called_with(str(local_path)) mock_git.remote_show.assert_called_with(str(local_path)) mock_git.push.assert_called_with( - remote, "foo", str(local_path), None, set_upstream=True, force=False + remote, + "foo", + str(local_path), + None, + set_upstream=True, + force=False, + cache_credentials=False, ) assert response.code == 200 @@ -496,7 +503,13 @@ async def test_push_handler_noupstream_pushdefault(mock_git, jp_fetch, jp_root_d mock_git.config.assert_called_with(str(local_path)) mock_git.remote_show.assert_called_with(str(local_path)) mock_git.push.assert_called_with( - remote, "foo", str(local_path), None, set_upstream=True, force=False + remote, + "foo", + str(local_path), + None, + set_upstream=True, + force=False, + cache_credentials=False, ) assert response.code == 200 @@ -531,7 +544,7 @@ async def test_push_handler_noupstream_pass_remote_nobranch( mock_git.config.assert_not_called() mock_git.remote_show.assert_not_called() mock_git.push.assert_called_with( - remote, "HEAD:foo", str(local_path), None, True, False + remote, "HEAD:foo", str(local_path), None, True, False, False ) assert response.code == 200 @@ -567,7 +580,7 @@ async def test_push_handler_noupstream_pass_remote_branch( mock_git.config.assert_not_called() mock_git.remote_show.assert_not_called() mock_git.push.assert_called_with( - remote, "HEAD:" + remote_branch, str(local_path), None, True, False + remote, "HEAD:" + remote_branch, str(local_path), None, True, False, False ) assert response.code == 200 From dff54f21f6d14714fb46e44da641f3332857f26d Mon Sep 17 00:00:00 2001 From: Dat Quach Date: Wed, 23 Mar 2022 06:51:01 -0700 Subject: [PATCH 23/44] Revert `await` back to `call` This is for compatibility with Python 3.7's `unittest`. --- jupyterlab_git/tests/test_clone.py | 10 +++++----- jupyterlab_git/tests/test_fetch.py | 18 +++++++++--------- jupyterlab_git/tests/test_pushpull.py | 20 ++++++++++---------- 3 files changed, 24 insertions(+), 24 deletions(-) diff --git a/jupyterlab_git/tests/test_clone.py b/jupyterlab_git/tests/test_clone.py index 4bd981ff0..aab6f7924 100644 --- a/jupyterlab_git/tests/test_clone.py +++ b/jupyterlab_git/tests/test_clone.py @@ -176,7 +176,7 @@ async def test_git_clone_with_cache_credentials(): ) # Then - mock_execute.assert_awaited_once_with( + mock_execute.assert_called_once_with( ["git", "clone", "ghjkhjkl"], cwd=test_path, env={**os.environ, "GIT_TERMINAL_PROMPT": "0"}, @@ -212,8 +212,8 @@ async def test_git_clone_with_auth_and_cache_credentials(): ) # Then - assert mock_execute.await_count == 3 - mock_execute.assert_has_awaits( + assert mock_execute.call_count == 3 + mock_execute.assert_has_calls( [ call(["git", "config", "--list"], cwd=test_path), call( @@ -256,8 +256,8 @@ async def test_git_clone_with_auth_and_cache_credentials_and_existing_credential ) # Then - assert mock_execute.await_count == 2 - mock_execute.assert_has_awaits( + assert mock_execute.call_count == 2 + mock_execute.assert_has_calls( [ call(["git", "config", "--list"], cwd=test_path), call( diff --git a/jupyterlab_git/tests/test_fetch.py b/jupyterlab_git/tests/test_fetch.py index 2ccb53add..728f341f8 100644 --- a/jupyterlab_git/tests/test_fetch.py +++ b/jupyterlab_git/tests/test_fetch.py @@ -19,7 +19,7 @@ async def test_git_fetch_success(): actual_response = await Git().fetch(path="test_path") # Then - mock_execute.assert_awaited_once_with( + mock_execute.assert_called_once_with( ["git", "fetch", "--all", "--prune"], cwd="test_path", env={**os.environ, "GIT_TERMINAL_PROMPT": "0"}, @@ -37,7 +37,7 @@ async def test_git_fetch_fail(): actual_response = await Git().fetch(path="test_path") # Then - mock_execute.assert_awaited_once_with( + mock_execute.assert_called_once_with( ["git", "fetch", "--all", "--prune"], cwd="test_path", env={**os.environ, "GIT_TERMINAL_PROMPT": "0"}, @@ -62,7 +62,7 @@ async def test_git_fetch_with_auth_success(): ) # Then - mock_execute.assert_awaited_once_with( + mock_execute.assert_called_once_with( ["git", "fetch", "--all", "--prune"], username="test_user", password="test_pass", @@ -91,7 +91,7 @@ async def test_git_fetch_with_auth_fail(): ) # Then - mock_execute.assert_awaited_once_with( + mock_execute.assert_called_once_with( ["git", "fetch", "--all", "--prune"], username="test_user", password="test_pass", @@ -116,7 +116,7 @@ async def test_git_fetch_with_cache_credentials(): actual_response = await Git().fetch(path="test_path", cache_credentials=True) # Then - mock_execute.assert_awaited_once_with( + mock_execute.assert_called_once_with( ["git", "fetch", "--all", "--prune"], cwd="test_path", env={**os.environ, "GIT_TERMINAL_PROMPT": "0"}, @@ -149,8 +149,8 @@ async def test_git_fetch_with_auth_and_cache_credentials(): ) # Then - assert mock_execute.await_count == 3 - mock_execute.assert_has_awaits( + assert mock_execute.call_count == 3 + mock_execute.assert_has_calls( [ call(["git", "config", "--list"], cwd=test_path), call( @@ -194,8 +194,8 @@ async def test_git_fetch_with_auth_and_cache_credentials_and_existing_credential ) # Then - assert mock_execute.await_count == 2 - mock_execute.assert_has_awaits( + assert mock_execute.call_count == 2 + mock_execute.assert_has_calls( [ call(["git", "config", "--list"], cwd=test_path), call( diff --git a/jupyterlab_git/tests/test_pushpull.py b/jupyterlab_git/tests/test_pushpull.py index e44e6a7ff..b366f7fa2 100644 --- a/jupyterlab_git/tests/test_pushpull.py +++ b/jupyterlab_git/tests/test_pushpull.py @@ -186,7 +186,7 @@ async def test_git_pull_with_cache_credentials(): actual_response = await Git().pull(test_path, cache_credentials=True) # Then - mock_execute.assert_awaited_once_with( + mock_execute.assert_called_once_with( ["git", "pull", "--no-commit"], cwd=test_path, env={**os.environ, "GIT_TERMINAL_PROMPT": "0"}, @@ -219,8 +219,8 @@ async def test_git_pull_with_auth_and_cache_credentials(): ) # Then - assert mock_execute.await_count == 3 - mock_execute.assert_has_awaits( + assert mock_execute.call_count == 3 + mock_execute.assert_has_calls( [ call( ["git", "config", "--list"], @@ -267,8 +267,8 @@ async def test_git_pull_with_auth_and_cache_credentials_and_existing_credential_ ) # Then - assert mock_execute.await_count == 2 - mock_execute.assert_has_awaits( + assert mock_execute.call_count == 2 + mock_execute.assert_has_calls( [ call( ["git", "config", "--list"], @@ -407,7 +407,7 @@ async def test_git_push_with_cache_credentials(): ) # Then - mock_execute.assert_awaited_once_with( + mock_execute.assert_called_once_with( ["git", "push", ".", "HEAD:test_master"], cwd=test_path, env={**os.environ, "GIT_TERMINAL_PROMPT": "0"}, @@ -440,8 +440,8 @@ async def test_git_push_with_auth_and_cache_credentials(): ) # Then - assert mock_execute.await_count == 3 - mock_execute.assert_has_awaits( + assert mock_execute.call_count == 3 + mock_execute.assert_has_calls( [ call( ["git", "config", "--list"], @@ -488,8 +488,8 @@ async def test_git_push_with_auth_and_cache_credentials_and_existing_credential_ ) # Then - assert mock_execute.await_count == 2 - mock_execute.assert_has_awaits( + assert mock_execute.call_count == 2 + mock_execute.assert_has_calls( [ call( ["git", "config", "--list"], From b4ae04afa574ef2ed0aa3a28d7b8e5f2aa507ced Mon Sep 17 00:00:00 2001 From: Dat Quach Date: Mon, 28 Mar 2022 20:29:01 -0700 Subject: [PATCH 24/44] Adopt PR review suggestions jupyterlab/jupyterlab-git/pull/1099#pullrequestreview-919970442 --- jupyterlab_git/__init__.py | 2 +- jupyterlab_git/git.py | 167 ++++++++++++----------------- jupyterlab_git/handlers.py | 11 +- jupyterlab_git/tests/test_clone.py | 35 ++---- jupyterlab_git/tests/test_fetch.py | 25 +---- src/components/StatusWidget.tsx | 108 ++++++------------- src/model.ts | 12 +-- src/tokens.ts | 4 +- src/widgets/CredentialsBox.tsx | 2 +- 9 files changed, 122 insertions(+), 244 deletions(-) diff --git a/jupyterlab_git/__init__.py b/jupyterlab_git/__init__.py index e90f67cb0..60afb7bf3 100644 --- a/jupyterlab_git/__init__.py +++ b/jupyterlab_git/__init__.py @@ -40,7 +40,7 @@ class JupyterLabGit(Configurable): credential_helper = Unicode( help=""" The value of Git credential helper will be set to this value when the Git credential caching mechanism is activated by this extension. - By default it is 3600 seconds (1 hour). + By default it is a in-memory cache of 3600 seconds (1 hour); `cache --timeout=3600`. """, config=True, ) diff --git a/jupyterlab_git/git.py b/jupyterlab_git/git.py index 17e6aa66b..e275fb3c5 100644 --- a/jupyterlab_git/git.py +++ b/jupyterlab_git/git.py @@ -7,9 +7,9 @@ import re import shlex import subprocess -import sys import traceback -from typing import Any, Dict, List, Optional +from typing import Dict, List, Optional +from unittest.mock import NonCallableMock from urllib.parse import unquote import nbformat @@ -183,6 +183,10 @@ class Git: def __init__(self, config=None): self._config = config + def __del__(self): + if self._GIT_CREDENTIAL_CACHE_DAEMON_PROCESS: + self._GIT_CREDENTIAL_CACHE_DAEMON_PROCESS.terminate() + async def config(self, path, **kwargs): """Get or set Git options. @@ -267,7 +271,7 @@ async def changed_files(self, path, base=None, remote=None, single_commit=None): return response - async def clone(self, path, repo_url, auth=None, cache_credentials=None): + async def clone(self, path, repo_url, auth=None): """ Execute `git clone`. When no auth is provided, disables prompts for the password to avoid the terminal hanging. @@ -279,13 +283,8 @@ async def clone(self, path, repo_url, auth=None, cache_credentials=None): """ env = os.environ.copy() if auth: - if cache_credentials: - ensured_response = await self.ensure_credential_helper(path) - if ensured_response["code"] != 0: - return { - "code": ensured_response["code"], - "message": ensured_response.get("error", "Unknown error!"), - } + if auth.get("cache_credentials", None): + await self.ensure_credential_helper(path) env["GIT_TERMINAL_PROMPT"] = "1" code, output, error = await execute( ["git", "clone", unquote(repo_url), "-q"], @@ -309,7 +308,7 @@ async def clone(self, path, repo_url, auth=None, cache_credentials=None): return response - async def fetch(self, path, auth=None, cache_credentials=False): + async def fetch(self, path, auth=None): """ Execute git fetch command """ @@ -323,13 +322,8 @@ async def fetch(self, path, auth=None, cache_credentials=False): ] # Run prune by default to help beginners env = os.environ.copy() if auth: - if cache_credentials: - ensured_response = await self.ensure_credential_helper(path) - if ensured_response["code"] != 0: - return { - "code": ensured_response["code"], - "message": ensured_response.get("error", "Unknown error!"), - } + if auth.get("cache_credentials", None): + await self.ensure_credential_helper(path) env["GIT_TERMINAL_PROMPT"] = "1" code, _, fetch_error = await execute( cmd, @@ -1030,22 +1024,15 @@ async def commit(self, commit_msg, amend, path): return {"code": code, "command": " ".join(cmd), "message": error} return {"code": code} - async def pull( - self, path, auth=None, cancel_on_conflict=False, cache_credentials=False - ): + async def pull(self, path, auth=None, cancel_on_conflict=False): """ Execute git pull --no-commit. Disables prompts for the password to avoid the terminal hanging while waiting for auth. """ env = os.environ.copy() if auth: - if cache_credentials: - ensured_response = await self.ensure_credential_helper(path) - if ensured_response["code"] != 0: - return { - "code": ensured_response["code"], - "message": ensured_response.get("error", "Unknown error!"), - } + if auth.get("cache_credentials", None): + await self.ensure_credential_helper(path) env["GIT_TERMINAL_PROMPT"] = "1" code, output, error = await execute( ["git", "pull", "--no-commit"], @@ -1096,7 +1083,6 @@ async def push( auth=None, set_upstream=False, force=False, - cache_credentials=False, ): """ Execute `git push $UPSTREAM $BRANCH`. The choice of upstream and branch is up to the caller. @@ -1110,13 +1096,8 @@ async def push( env = os.environ.copy() if auth: - if cache_credentials: - ensured_response = await self.ensure_credential_helper(path) - if ensured_response["code"] != 0: - return { - "code": ensured_response["code"], - "message": ensured_response.get("error", "Unknown error!"), - } + if auth.get("cache_credentials", None): + await self.ensure_credential_helper(path) env["GIT_TERMINAL_PROMPT"] = "1" code, output, error = await execute( command, @@ -1596,29 +1577,39 @@ async def tag_checkout(self, path, tag): "message": error, } - async def check_credential_helper(self, path: str) -> Dict[str, Any]: + async def check_credential_helper(self, path: str) -> Optional[bool]: + """ + Check if the credential helper exists, and whether we need to setup a Git credential cache daemon in case the credential helper is Git credential cache. + + path: str + Git path repository + + Return None if the credential helper is not set. + Otherwise, return True if we need to setup a Git credential cache daemon, else False. + + Raise an exception if `git config` errored. + """ + git_config_response: Dict[str, str] = await self.config(path) if git_config_response["code"] != 0: - return git_config_response + raise RuntimeError(git_config_response["message"]) git_config_kv_pairs = git_config_response["options"] has_credential_helper = "credential.helper" in git_config_kv_pairs - response = { - "code": 0, - "result": has_credential_helper, - } + if not has_credential_helper: + return None if has_credential_helper and GIT_CREDENTIAL_HELPER_CACHE.match( git_config_kv_pairs["credential.helper"].strip() ): - response["cache_daemon_required"] = True + return True - return response + return False async def ensure_credential_helper( self, path: str, env: Dict[str, str] = None - ) -> Dict[str, Any]: + ) -> None: """ Check whether `git config --list` contains `credential.helper`. If it is not set, then it will be set to the value string for `credential.helper` @@ -1630,81 +1621,63 @@ async def ensure_credential_helper( Environment variables """ - check_credential_helper_response = await self.check_credential_helper(path) - - if check_credential_helper_response["code"] != 0: - return check_credential_helper_response - has_credential_helper = check_credential_helper_response["result"] - - cache_daemon_required = check_credential_helper_response.get( - "cache_daemon_required", False - ) + try: + has_credential_helper = await self.check_credential_helper(path) + if has_credential_helper == False: + return + except RuntimeError as e: + get_logger().error("Error checking credential helper: %s", e, exc_info=True) + return - response = {"code": -1} + cache_daemon_required = has_credential_helper == True if not has_credential_helper: credential_helper: str = self._config.credential_helper - response.update( - await self.config(path, **{"credential.helper": credential_helper}) - ) if GIT_CREDENTIAL_HELPER_CACHE.match(credential_helper.strip()): cache_daemon_required = True - else: - response["code"] = 0 # special case: Git credential cache if cache_daemon_required: - if not sys.platform.startswith("win32"): - try: - self.ensure_git_credential_cache_daemon(cwd=path, env=env) - except Exception as e: - response["code"] = 2 - response["error"] = f"Unhandled error: {str(e)}" - else: - response["code"] = 1 - response["error"] = "Git credential cache cannot operate on Windows!" - - return response + try: + self.ensure_git_credential_cache_daemon(cwd=path, env=env) + except Exception as e: + get_logger().error( + "Error setting up Git credential cache daemon: %s", e, exc_info=True + ) def ensure_git_credential_cache_daemon( self, - socket: Optional[str] = None, + socket: Optional[pathlib.Path] = None, debug: bool = False, - new: bool = False, + force: bool = False, cwd: Optional[str] = None, env: Dict[str, str] = None, - ) -> int: + ) -> None: """ Spawn a Git credential cache daemon with the socket file being `socket` if it does not exist. If `debug` is `True`, the daemon will be spawned with `--debug` flag. If `socket` is empty, it is set to `~/.git-credential-cache-daemon`. - If `new` is `True`, a daemon will be spawned, and if the daemon process is accessible, + If `force` is `True`, a daemon will be spawned, and if the daemon process is accessible, the existing daemon process will be terminated before spawning a new one. - Otherwise, if `new` is `False`, the PID of the existing daemon process is returned. + Otherwise, if `force` is `False`, the PID of the existing daemon process is returned. If the daemon process is not accessible, `-1` is returned. `cwd` and `env` are passed to the process that spawns the daemon. """ if not socket: - socket = os.path.join( - os.path.expanduser("~"), ".git-credential-cache", "socket" - ) + socket = pathlib.Path.home() / ".git-credential-cache" / "socket" - if socket and os.path.exists(socket): - return -1 + if socket.exists(): + return - if self._GIT_CREDENTIAL_CACHE_DAEMON_PROCESS is None or new is True: + if self._GIT_CREDENTIAL_CACHE_DAEMON_PROCESS is None or force: - if new is True and self._GIT_CREDENTIAL_CACHE_DAEMON_PROCESS: + if force and self._GIT_CREDENTIAL_CACHE_DAEMON_PROCESS: self._GIT_CREDENTIAL_CACHE_DAEMON_PROCESS.terminate() - if not socket: - raise ValueError() - - socket_dir = os.path.split(socket)[0] - if socket_dir and len(socket_dir) > 0 and not os.path.isdir(socket_dir): - os.makedirs(socket_dir) - os.chmod(socket_dir, 0o700) + if not socket.parent.exists(): + socket.parent.mkdir(parents=True, exist_ok=True) + socket.parent.chmod(0o700) args: List[str] = ["git", "credential-cache--daemon"] @@ -1720,17 +1693,9 @@ def ensure_git_credential_cache_daemon( ) get_logger().debug( - "A credential cache daemon has been spawned with PID %s", - str(self._GIT_CREDENTIAL_CACHE_DAEMON_PROCESS.pid), + "A credential cache daemon has been spawned with PID %d", + self._GIT_CREDENTIAL_CACHE_DAEMON_PROCESS.pid, ) elif self._GIT_CREDENTIAL_CACHE_DAEMON_PROCESS.poll(): - return self.ensure_git_credential_cache_daemon( - socket, debug, True, cwd, env - ) - - return self._GIT_CREDENTIAL_CACHE_DAEMON_PROCESS.pid - - def __del__(self): - if self._GIT_CREDENTIAL_CACHE_DAEMON_PROCESS: - self._GIT_CREDENTIAL_CACHE_DAEMON_PROCESS.terminate() + self.ensure_git_credential_cache_daemon(socket, debug, True, cwd, env) diff --git a/jupyterlab_git/handlers.py b/jupyterlab_git/handlers.py index 50cc4aaf3..68aa9bab2 100644 --- a/jupyterlab_git/handlers.py +++ b/jupyterlab_git/handlers.py @@ -23,7 +23,7 @@ from .log import get_logger # Git configuration options exposed through the REST API -ALLOWED_OPTIONS = ["user.name", "user.email", "credential.helper"] +ALLOWED_OPTIONS = ["user.name", "user.email"] # REST API namespace NAMESPACE = "/git" @@ -67,7 +67,8 @@ async def post(self, path: str = ""): { 'repo_url': 'https://github.com/path/to/myrepo', OPTIONAL 'auth': '{ 'username': '', - 'password': '' + 'password': '', + 'cacheCredentials': true/false }' } """ @@ -76,7 +77,6 @@ async def post(self, path: str = ""): self.url2localpath(path), data["clone_url"], data.get("auth", None), - data.get("cache_credentials", False), ) if response["code"] != 0: @@ -177,7 +177,6 @@ async def post(self, path: str = ""): result = await self.git.fetch( self.url2localpath(path), data.get("auth", None), - data.get("cache_credentials", False), ) if result["code"] != 0: @@ -541,7 +540,6 @@ async def post(self, path: str = ""): self.url2localpath(path), data.get("auth", None), data.get("cancel_on_conflict", False), - data.get("cache_credentials", False), ) if response["code"] != 0: @@ -572,7 +570,6 @@ async def post(self, path: str = ""): data = self.get_json_body() known_remote = data.get("remote") force = data.get("force", False) - cache_credentials = data.get("cache_credentials", False) current_local_branch = await self.git.get_current_branch(local_path) @@ -601,7 +598,6 @@ async def post(self, path: str = ""): data.get("auth", None), set_upstream, force, - cache_credentials, ) else: @@ -628,7 +624,6 @@ async def post(self, path: str = ""): data.get("auth", None), set_upstream=True, force=force, - cache_credentials=cache_credentials, ) else: response = { diff --git a/jupyterlab_git/tests/test_clone.py b/jupyterlab_git/tests/test_clone.py index aab6f7924..8af13476b 100644 --- a/jupyterlab_git/tests/test_clone.py +++ b/jupyterlab_git/tests/test_clone.py @@ -160,30 +160,6 @@ async def test_git_clone_with_auth_auth_failure_from_git(): } == actual_response -@pytest.mark.asyncio -async def test_git_clone_with_cache_credentials(): - with patch("jupyterlab_git.git.execute") as mock_execute: - # Given - test_path = str(Path("/bin") / "test_curr_path") - mock_execute.side_effect = [ - maybe_future((0, "", "")), - maybe_future((0, "", "")), - ] - - # When - actual_response = await Git().clone( - path=test_path, repo_url="ghjkhjkl", cache_credentials=True - ) - - # Then - mock_execute.assert_called_once_with( - ["git", "clone", "ghjkhjkl"], - cwd=test_path, - env={**os.environ, "GIT_TERMINAL_PROMPT": "0"}, - ) - assert {"code": 0, "message": ""} == actual_response - - @pytest.mark.asyncio async def test_git_clone_with_auth_and_cache_credentials(): with patch("sys.platform", "linux"): @@ -203,12 +179,15 @@ async def test_git_clone_with_auth_and_cache_credentials(): maybe_future((0, "", "")), ] # When - auth = {"username": "asdf", "password": "qwerty"} + auth = { + "username": "asdf", + "password": "qwerty", + "cache_credentials": True, + } actual_response = await Git(config=default_config).clone( path=test_path, repo_url="ghjkhjkl", auth=auth, - cache_credentials=True, ) # Then @@ -250,9 +229,9 @@ async def test_git_clone_with_auth_and_cache_credentials_and_existing_credential maybe_future((0, "", "")), ] # When - auth = {"username": "asdf", "password": "qwerty"} + auth = {"username": "asdf", "password": "qwerty", "cache_credentials": True} actual_response = await Git(config=default_config).clone( - path=test_path, repo_url="ghjkhjkl", auth=auth, cache_credentials=True + path=test_path, repo_url="ghjkhjkl", auth=auth ) # Then diff --git a/jupyterlab_git/tests/test_fetch.py b/jupyterlab_git/tests/test_fetch.py index 728f341f8..f2b6211da 100644 --- a/jupyterlab_git/tests/test_fetch.py +++ b/jupyterlab_git/tests/test_fetch.py @@ -106,24 +106,6 @@ async def test_git_fetch_with_auth_fail(): } == actual_response -@pytest.mark.asyncio -async def test_git_fetch_with_cache_credentials(): - with patch("jupyterlab_git.git.execute") as mock_execute: - # Given - mock_execute.return_value = maybe_future((0, "", "")) - - # When - actual_response = await Git().fetch(path="test_path", cache_credentials=True) - - # Then - mock_execute.assert_called_once_with( - ["git", "fetch", "--all", "--prune"], - cwd="test_path", - env={**os.environ, "GIT_TERMINAL_PROMPT": "0"}, - ) - assert {"code": 0} == actual_response - - @pytest.mark.asyncio async def test_git_fetch_with_auth_and_cache_credentials(): with patch("sys.platform", "linux"): @@ -144,8 +126,11 @@ async def test_git_fetch_with_auth_and_cache_credentials(): # When actual_response = await Git(config=default_config).fetch( path=test_path, - auth={"username": "test_user", "password": "test_pass"}, - cache_credentials=True, + auth={ + "username": "test_user", + "password": "test_pass", + "cache_credentials": True, + }, ) # Then diff --git a/src/components/StatusWidget.tsx b/src/components/StatusWidget.tsx index 0da48b989..9be3e83ac 100644 --- a/src/components/StatusWidget.tsx +++ b/src/components/StatusWidget.tsx @@ -1,4 +1,4 @@ -import { ReactWidget } from '@jupyterlab/apputils'; +import { ReactWidget, UseSignal } from '@jupyterlab/apputils'; import { ISettingRegistry } from '@jupyterlab/settingregistry'; import { IStatusBar } from '@jupyterlab/statusbar'; import { TranslationBundle } from '@jupyterlab/translation'; @@ -39,46 +39,41 @@ export class StatusWidget extends ReactWidget { } } - /** - * Boolean indicating whether credentials are required. - */ - get waitingForCredentials(): boolean { - return this._waitingForCredentials; - } - - /** - * Sets the boolean indicating whether credentials are required. - */ - set waitingForCredentials(value: boolean) { - this._waitingForCredentials = value; - } - render(): JSX.Element { return ( - - this._showGitOperationDialog() - : undefined - } - title={ - this._waitingForCredentials - ? `Git: ${this._trans.__('credentials required')}` - : `Git: ${this._trans.__(this._status)}` - } - /> - + {(_, needsCredentials) => ( + + this._showGitOperationDialog() + : undefined + } + title={ + needsCredentials + ? `Git: ${this._trans.__('credentials required')}` + : `Git: ${this._trans.__(this._status)}` + } + /> + + )} + ); } @@ -115,11 +110,6 @@ export class StatusWidget extends ReactWidget { */ private _status = ''; - /** - * Boolean indicating whether credentials are required. - */ - private _waitingForCredentials: boolean; - private _model: IGitExtension; private _trans: TranslationBundle; } @@ -142,13 +132,8 @@ export function addStatusBarWidget( const callback = Private.createEventCallback(statusWidget); model.taskChanged.connect(callback); - const credentialsRequiredCallback = - Private.createCredentialsRequiredCallback(statusWidget); - model.credentialsRequiredSignal.connect(credentialsRequiredCallback); - statusWidget.disposed.connect(() => { model.taskChanged.disconnect(callback); - model.credentialsRequiredSignal.disconnect(credentialsRequiredCallback); }); } /* eslint-disable no-inner-declarations */ @@ -255,32 +240,5 @@ namespace Private { return settings.composite.displayStatus as boolean; } } - - /** - * Returns a callback invoked when credentials are required. - * - * @private - * @param widget - status widget - * @returns callback - */ - export function createCredentialsRequiredCallback( - widget: StatusWidget - ): (model: IGitExtension, value: boolean) => void { - /** - * Callback invoked when credentials are required. - * - * @private - * @param model - extension model - * @param value - boolean indicating whether credentials are required - * @returns void - */ - function callbackCredentialsRequired( - model: IGitExtension, - value: boolean - ): void { - widget.waitingForCredentials = value; - } - return callbackCredentialsRequired; - } } /* eslint-enable no-inner-declarations */ diff --git a/src/model.ts b/src/model.ts index 21e2fe09a..30ec8ca26 100644 --- a/src/model.ts +++ b/src/model.ts @@ -301,15 +301,15 @@ export class GitExtension implements IGitExtension { set credentialsRequired(value: boolean) { if (this._credentialsRequired !== value) { this._credentialsRequired = value; - this._credentialsRequiredSignal.emit(value); + this._credentialsRequiredChanged.emit(value); } } /** * A signal emitted whenever credentials are required, or are not required anymore. */ - get credentialsRequiredSignal(): ISignal { - return this._credentialsRequiredSignal; + get credentialsRequiredChanged(): ISignal { + return this._credentialsRequiredChanged; } /** @@ -609,7 +609,6 @@ export class GitExtension implements IGitExtension { { clone_url: url, auth: auth as any, - cache_credentials: auth?.cacheCredentials } ); } @@ -772,7 +771,6 @@ export class GitExtension implements IGitExtension { 'POST', { auth: auth as any, - cache_credentials: auth?.cacheCredentials } ); } @@ -886,7 +884,6 @@ export class GitExtension implements IGitExtension { (this._settings?.composite[ 'cancelPullMergeConflict' ] as boolean) || false, - cache_credentials: auth?.cacheCredentials } ); } @@ -917,7 +914,6 @@ export class GitExtension implements IGitExtension { { auth: auth as any, force: force, - cache_credentials: auth?.cacheCredentials } ); } @@ -1688,7 +1684,7 @@ export class GitExtension implements IGitExtension { private _dirtyStagedFilesStatusChanged = new Signal( this ); - private _credentialsRequiredSignal = new Signal(this); + private _credentialsRequiredChanged = new Signal(this); } export class BranchMarker implements Git.IBranchMarker { diff --git a/src/tokens.ts b/src/tokens.ts index 061695763..4e97da79a 100644 --- a/src/tokens.ts +++ b/src/tokens.ts @@ -75,7 +75,7 @@ export interface IGitExtension extends IDisposable { /** * A signal emitted whenever credentials are required, or are not required anymore. */ - readonly credentialsRequiredSignal: ISignal; + readonly credentialsRequiredChanged: ISignal; /** * Git repository status. @@ -947,7 +947,7 @@ export namespace Git { export interface IAuth { username: string; password: string; - cacheCredentials?: boolean; + cache_credentials?: boolean; } /** diff --git a/src/widgets/CredentialsBox.tsx b/src/widgets/CredentialsBox.tsx index c56011673..9e39c24e7 100755 --- a/src/widgets/CredentialsBox.tsx +++ b/src/widgets/CredentialsBox.tsx @@ -69,7 +69,7 @@ export class GitCredentialsForm return { username: this._user.value, password: this._password.value, - cacheCredentials: this._checkboxCacheCredentials.checked + cache_credentials: this._checkboxCacheCredentials.checked }; } protected _trans: TranslationBundle; From 66421b96ec0ceb93e9418de365041c8b52a5781d Mon Sep 17 00:00:00 2001 From: Dat Quach Date: Thu, 31 Mar 2022 06:43:25 -0700 Subject: [PATCH 25/44] Fix CI - jupyterlab/jupyterlab-git/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 --- jupyterlab_git/git.py | 11 +- jupyterlab_git/handlers.py | 2 +- jupyterlab_git/tests/test_clone.py | 105 ++++++----- jupyterlab_git/tests/test_fetch.py | 106 +++++------ jupyterlab_git/tests/test_handlers.py | 9 +- jupyterlab_git/tests/test_pushpull.py | 242 +++++++++++--------------- 6 files changed, 214 insertions(+), 261 deletions(-) diff --git a/jupyterlab_git/git.py b/jupyterlab_git/git.py index e275fb3c5..6bf3a9d42 100644 --- a/jupyterlab_git/git.py +++ b/jupyterlab_git/git.py @@ -283,7 +283,7 @@ async def clone(self, path, repo_url, auth=None): """ env = os.environ.copy() if auth: - if auth.get("cache_credentials", None): + if auth.get("cache_credentials"): await self.ensure_credential_helper(path) env["GIT_TERMINAL_PROMPT"] = "1" code, output, error = await execute( @@ -322,7 +322,7 @@ async def fetch(self, path, auth=None): ] # Run prune by default to help beginners env = os.environ.copy() if auth: - if auth.get("cache_credentials", None): + if auth.get("cache_credentials"): await self.ensure_credential_helper(path) env["GIT_TERMINAL_PROMPT"] = "1" code, _, fetch_error = await execute( @@ -1031,7 +1031,7 @@ async def pull(self, path, auth=None, cancel_on_conflict=False): """ env = os.environ.copy() if auth: - if auth.get("cache_credentials", None): + if auth.get("cache_credentials"): await self.ensure_credential_helper(path) env["GIT_TERMINAL_PROMPT"] = "1" code, output, error = await execute( @@ -1096,7 +1096,7 @@ async def push( env = os.environ.copy() if auth: - if auth.get("cache_credentials", None): + if auth.get("cache_credentials"): await self.ensure_credential_helper(path) env["GIT_TERMINAL_PROMPT"] = "1" code, output, error = await execute( @@ -1631,8 +1631,9 @@ async def ensure_credential_helper( cache_daemon_required = has_credential_helper == True - if not has_credential_helper: + if has_credential_helper is None: credential_helper: str = self._config.credential_helper + await self.config(path, **{"credential.helper": credential_helper}) if GIT_CREDENTIAL_HELPER_CACHE.match(credential_helper.strip()): cache_daemon_required = True diff --git a/jupyterlab_git/handlers.py b/jupyterlab_git/handlers.py index 68aa9bab2..7b16ef009 100644 --- a/jupyterlab_git/handlers.py +++ b/jupyterlab_git/handlers.py @@ -68,7 +68,7 @@ async def post(self, path: str = ""): 'repo_url': 'https://github.com/path/to/myrepo', OPTIONAL 'auth': '{ 'username': '', 'password': '', - 'cacheCredentials': true/false + 'cache_credentials': true/false }' } """ diff --git a/jupyterlab_git/tests/test_clone.py b/jupyterlab_git/tests/test_clone.py index 8af13476b..3b2c0ddd6 100644 --- a/jupyterlab_git/tests/test_clone.py +++ b/jupyterlab_git/tests/test_clone.py @@ -162,60 +162,59 @@ async def test_git_clone_with_auth_auth_failure_from_git(): @pytest.mark.asyncio async def test_git_clone_with_auth_and_cache_credentials(): - with patch("sys.platform", "linux"): - with patch( - "jupyterlab_git.git.Git.ensure_git_credential_cache_daemon" - ) as mock_ensure_daemon: - mock_ensure_daemon.return_value = 0 - with patch("jupyterlab_git.git.execute") as mock_execute: - # Given - default_config = JupyterLabGit() - default_config.credential_helper = "cache" - credential_helper = default_config.credential_helper - test_path = "test_curr_path" - mock_execute.side_effect = [ - maybe_future((0, "", "")), - maybe_future((0, "", "")), - maybe_future((0, "", "")), - ] - # When - auth = { - "username": "asdf", - "password": "qwerty", - "cache_credentials": True, - } - actual_response = await Git(config=default_config).clone( - path=test_path, - repo_url="ghjkhjkl", - auth=auth, - ) + with patch( + "jupyterlab_git.git.Git.ensure_git_credential_cache_daemon" + ) as mock_ensure_daemon: + mock_ensure_daemon.return_value = 0 + with patch("jupyterlab_git.git.execute") as mock_execute: + # Given + default_config = JupyterLabGit() + default_config.credential_helper = "cache" + credential_helper = default_config.credential_helper + test_path = "test_curr_path" + mock_execute.side_effect = [ + maybe_future((0, "", "")), + maybe_future((0, "", "")), + maybe_future((0, "", "")), + ] + # When + auth = { + "username": "asdf", + "password": "qwerty", + "cache_credentials": True, + } + actual_response = await Git(config=default_config).clone( + path=test_path, + repo_url="ghjkhjkl", + auth=auth, + ) - # Then - assert mock_execute.call_count == 3 - mock_execute.assert_has_calls( - [ - call(["git", "config", "--list"], cwd=test_path), - call( - [ - "git", - "config", - "--add", - "credential.helper", - credential_helper, - ], - cwd=test_path, - ), - call( - ["git", "clone", "ghjkhjkl", "-q"], - username="asdf", - password="qwerty", - cwd=test_path, - env={**os.environ, "GIT_TERMINAL_PROMPT": "1"}, - ), - ] - ) - mock_ensure_daemon.assert_called_once_with(cwd=test_path, env=None) - assert {"code": 0, "message": ""} == actual_response + # Then + assert mock_execute.call_count == 3 + mock_execute.assert_has_calls( + [ + call(["git", "config", "--list"], cwd=test_path), + call( + [ + "git", + "config", + "--add", + "credential.helper", + credential_helper, + ], + cwd=test_path, + ), + call( + ["git", "clone", "ghjkhjkl", "-q"], + username="asdf", + password="qwerty", + cwd=test_path, + env={**os.environ, "GIT_TERMINAL_PROMPT": "1"}, + ), + ] + ) + mock_ensure_daemon.assert_called_once_with(cwd=test_path, env=None) + assert {"code": 0, "message": ""} == actual_response @pytest.mark.asyncio diff --git a/jupyterlab_git/tests/test_fetch.py b/jupyterlab_git/tests/test_fetch.py index f2b6211da..5265d610f 100644 --- a/jupyterlab_git/tests/test_fetch.py +++ b/jupyterlab_git/tests/test_fetch.py @@ -108,57 +108,56 @@ async def test_git_fetch_with_auth_fail(): @pytest.mark.asyncio async def test_git_fetch_with_auth_and_cache_credentials(): - with patch("sys.platform", "linux"): - with patch( - "jupyterlab_git.git.Git.ensure_git_credential_cache_daemon" - ) as mock_ensure_daemon: - mock_ensure_daemon.return_value = 0 - with patch("jupyterlab_git.git.execute") as mock_execute: - # Given - default_config = JupyterLabGit() - credential_helper = default_config.credential_helper - test_path = "test_path" - mock_execute.side_effect = [ - maybe_future((0, "", "")), - maybe_future((0, "", "")), - maybe_future((0, "", "")), + with patch( + "jupyterlab_git.git.Git.ensure_git_credential_cache_daemon" + ) as mock_ensure_daemon: + mock_ensure_daemon.return_value = 0 + with patch("jupyterlab_git.git.execute") as mock_execute: + # Given + default_config = JupyterLabGit() + credential_helper = default_config.credential_helper + test_path = "test_path" + mock_execute.side_effect = [ + maybe_future((0, "", "")), + maybe_future((0, "", "")), + maybe_future((0, "", "")), + ] + # When + actual_response = await Git(config=default_config).fetch( + path=test_path, + auth={ + "username": "test_user", + "password": "test_pass", + "cache_credentials": True, + }, + ) + + # Then + assert mock_execute.call_count == 3 + mock_execute.assert_has_calls( + [ + call(["git", "config", "--list"], cwd=test_path), + call( + [ + "git", + "config", + "--add", + "credential.helper", + credential_helper, + ], + cwd=test_path, + ), + call( + ["git", "fetch", "--all", "--prune"], + username="test_user", + password="test_pass", + cwd=test_path, + env={**os.environ, "GIT_TERMINAL_PROMPT": "1"}, + ), ] - # When - actual_response = await Git(config=default_config).fetch( - path=test_path, - auth={ - "username": "test_user", - "password": "test_pass", - "cache_credentials": True, - }, - ) - - # Then - assert mock_execute.call_count == 3 - mock_execute.assert_has_calls( - [ - call(["git", "config", "--list"], cwd=test_path), - call( - [ - "git", - "config", - "--add", - "credential.helper", - credential_helper, - ], - cwd=test_path, - ), - call( - ["git", "fetch", "--all", "--prune"], - username="test_user", - password="test_pass", - cwd=test_path, - env={**os.environ, "GIT_TERMINAL_PROMPT": "1"}, - ), - ] - ) - mock_ensure_daemon.assert_called_once_with(cwd=test_path, env=None) - assert {"code": 0} == actual_response + ) + mock_ensure_daemon.assert_called_once_with(cwd=test_path, env=None) + assert {"code": 0} == actual_response @pytest.mark.asyncio @@ -174,8 +173,11 @@ async def test_git_fetch_with_auth_and_cache_credentials_and_existing_credential # When actual_response = await Git(config=default_config).fetch( path="test_path", - auth={"username": "test_user", "password": "test_pass"}, - cache_credentials=True, + auth={ + "username": "test_user", + "password": "test_pass", + "cache_credentials": True, + }, ) # Then diff --git a/jupyterlab_git/tests/test_handlers.py b/jupyterlab_git/tests/test_handlers.py index e946134d7..42072a717 100644 --- a/jupyterlab_git/tests/test_handlers.py +++ b/jupyterlab_git/tests/test_handlers.py @@ -327,7 +327,7 @@ async def test_push_handler_localbranch(mock_git, jp_fetch, jp_root_dir): mock_git.get_current_branch.assert_called_with(str(local_path)) mock_git.get_upstream_branch.assert_called_with(str(local_path), "localbranch") mock_git.push.assert_called_with( - ".", "HEAD:localbranch", str(local_path), None, False, False, False + ".", "HEAD:localbranch", str(local_path), None, False, False ) assert response.code == 200 @@ -363,7 +363,6 @@ async def test_push_handler_remotebranch(mock_git, jp_fetch, jp_root_dir): None, False, False, - False, ) assert response.code == 200 @@ -470,7 +469,6 @@ async def test_push_handler_noupstream_unique_remote(mock_git, jp_fetch, jp_root None, set_upstream=True, force=False, - cache_credentials=False, ) assert response.code == 200 @@ -509,7 +507,6 @@ async def test_push_handler_noupstream_pushdefault(mock_git, jp_fetch, jp_root_d None, set_upstream=True, force=False, - cache_credentials=False, ) assert response.code == 200 @@ -544,7 +541,7 @@ async def test_push_handler_noupstream_pass_remote_nobranch( mock_git.config.assert_not_called() mock_git.remote_show.assert_not_called() mock_git.push.assert_called_with( - remote, "HEAD:foo", str(local_path), None, True, False, False + remote, "HEAD:foo", str(local_path), None, True, False ) assert response.code == 200 @@ -580,7 +577,7 @@ async def test_push_handler_noupstream_pass_remote_branch( mock_git.config.assert_not_called() mock_git.remote_show.assert_not_called() mock_git.push.assert_called_with( - remote, "HEAD:" + remote_branch, str(local_path), None, True, False, False + remote, "HEAD:" + remote_branch, str(local_path), None, True, False ) assert response.code == 200 diff --git a/jupyterlab_git/tests/test_pushpull.py b/jupyterlab_git/tests/test_pushpull.py index b366f7fa2..8821373c9 100644 --- a/jupyterlab_git/tests/test_pushpull.py +++ b/jupyterlab_git/tests/test_pushpull.py @@ -175,78 +175,56 @@ async def test_git_pull_with_auth_success_and_conflict_fail(): } == actual_response -@pytest.mark.asyncio -async def test_git_pull_with_cache_credentials(): - with patch("jupyterlab_git.git.execute") as mock_execute: - # Given - test_path = "test_path" - mock_execute.return_value = maybe_future((0, "", "")) - - # When - actual_response = await Git().pull(test_path, cache_credentials=True) - - # Then - mock_execute.assert_called_once_with( - ["git", "pull", "--no-commit"], - cwd=test_path, - env={**os.environ, "GIT_TERMINAL_PROMPT": "0"}, - ) - assert {"code": 0, "message": ""} == actual_response - - @pytest.mark.asyncio async def test_git_pull_with_auth_and_cache_credentials(): - with patch("sys.platform", "linux"): - with patch( - "jupyterlab_git.git.Git.ensure_git_credential_cache_daemon" - ) as mock_ensure_daemon: - mock_ensure_daemon.return_value = 0 - with patch("jupyterlab_git.git.execute") as mock_execute: - # Given - default_config = JupyterLabGit() - credential_helper = default_config.credential_helper - test_path = "test_path" - mock_execute.side_effect = [ - maybe_future((0, "", "")), - maybe_future((0, "", "")), - maybe_future((0, "", "")), - ] + with patch( + "jupyterlab_git.git.Git.ensure_git_credential_cache_daemon" + ) as mock_ensure_daemon: + mock_ensure_daemon.return_value = 0 + with patch("jupyterlab_git.git.execute") as mock_execute: + # Given + default_config = JupyterLabGit() + credential_helper = default_config.credential_helper + test_path = "test_path" + mock_execute.side_effect = [ + maybe_future((0, "", "")), + maybe_future((0, "", "")), + maybe_future((0, "", "")), + ] - # When - auth = {"username": "user", "password": "pass"} - actual_response = await Git(config=default_config).pull( - test_path, auth, cache_credentials=True - ) + # When + auth = {"username": "user", "password": "pass", "cache_credentials": True} + actual_response = await Git(config=default_config).pull(test_path, auth) - # Then - assert mock_execute.call_count == 3 - mock_execute.assert_has_calls( - [ - call( - ["git", "config", "--list"], - cwd=test_path, - ), - call( - [ - "git", - "config", - "--add", - "credential.helper", - credential_helper, - ], - cwd=test_path, - ), - call( - ["git", "pull", "--no-commit"], - username="user", - password="pass", - cwd=test_path, - env={**os.environ, "GIT_TERMINAL_PROMPT": "1"}, - ), - ] - ) - mock_ensure_daemon.assert_called_once_with(cwd=test_path, env=None) - assert {"code": 0, "message": ""} == actual_response + # Then + assert mock_execute.call_count == 3 + mock_execute.assert_has_calls( + [ + call( + ["git", "config", "--list"], + cwd=test_path, + ), + call( + [ + "git", + "config", + "--add", + "credential.helper", + credential_helper, + ], + cwd=test_path, + ), + call( + ["git", "pull", "--no-commit"], + username="user", + password="pass", + cwd=test_path, + env={**os.environ, "GIT_TERMINAL_PROMPT": "1"}, + ), + ] + ) + mock_ensure_daemon.assert_called_once_with(cwd=test_path, env=None) + assert {"code": 0, "message": ""} == actual_response @pytest.mark.asyncio @@ -261,10 +239,8 @@ async def test_git_pull_with_auth_and_cache_credentials_and_existing_credential_ ] # When - auth = {"username": "user", "password": "pass"} - actual_response = await Git(config=default_config).pull( - test_path, auth, cache_credentials=True - ) + auth = {"username": "user", "password": "pass", "cache_credentials": True} + actual_response = await Git(config=default_config).pull(test_path, auth) # Then assert mock_execute.call_count == 2 @@ -394,80 +370,58 @@ async def test_git_push_with_auth_success(): assert {"code": 0, "message": output} == actual_response -@pytest.mark.asyncio -async def test_git_push_with_cache_credentials(): - with patch("jupyterlab_git.git.execute") as mock_execute: - # Given - test_path = "test_path" - mock_execute.return_value = maybe_future((0, "", "")) - - # When - actual_response = await Git().push( - ".", "HEAD:test_master", test_path, cache_credentials=True - ) - - # Then - mock_execute.assert_called_once_with( - ["git", "push", ".", "HEAD:test_master"], - cwd=test_path, - env={**os.environ, "GIT_TERMINAL_PROMPT": "0"}, - ) - assert {"code": 0, "message": ""} == actual_response - - @pytest.mark.asyncio async def test_git_push_with_auth_and_cache_credentials(): - with patch("sys.platform", "linux"): - with patch( - "jupyterlab_git.git.Git.ensure_git_credential_cache_daemon" - ) as mock_ensure_daemon: - mock_ensure_daemon.return_value = 0 - with patch("jupyterlab_git.git.execute") as mock_execute: - # Given - default_config = JupyterLabGit() - credential_helper = default_config.credential_helper - test_path = "test_path" - mock_execute.side_effect = [ - maybe_future((0, "", "")), - maybe_future((0, "", "")), - maybe_future((0, "", "")), - ] + with patch( + "jupyterlab_git.git.Git.ensure_git_credential_cache_daemon" + ) as mock_ensure_daemon: + mock_ensure_daemon.return_value = 0 + with patch("jupyterlab_git.git.execute") as mock_execute: + # Given + default_config = JupyterLabGit() + credential_helper = default_config.credential_helper + test_path = "test_path" + mock_execute.side_effect = [ + maybe_future((0, "", "")), + maybe_future((0, "", "")), + maybe_future((0, "", "")), + ] - # When - auth = {"username": "user", "password": "pass"} - actual_response = await Git(config=default_config).push( - ".", "HEAD:test_master", test_path, auth, cache_credentials=True - ) + # When + auth = {"username": "user", "password": "pass", "cache_credentials": True} + actual_response = await Git(config=default_config).push( + ".", "HEAD:test_master", test_path, auth + ) - # Then - assert mock_execute.call_count == 3 - mock_execute.assert_has_calls( - [ - call( - ["git", "config", "--list"], - cwd=test_path, - ), - call( - [ - "git", - "config", - "--add", - "credential.helper", - credential_helper, - ], - cwd=test_path, - ), - call( - ["git", "push", ".", "HEAD:test_master"], - username="user", - password="pass", - cwd=test_path, - env={**os.environ, "GIT_TERMINAL_PROMPT": "1"}, - ), - ] - ) - mock_ensure_daemon.assert_called_once_with(cwd=test_path, env=None) - assert {"code": 0, "message": ""} == actual_response + # Then + assert mock_execute.call_count == 3 + mock_execute.assert_has_calls( + [ + call( + ["git", "config", "--list"], + cwd=test_path, + ), + call( + [ + "git", + "config", + "--add", + "credential.helper", + credential_helper, + ], + cwd=test_path, + ), + call( + ["git", "push", ".", "HEAD:test_master"], + username="user", + password="pass", + cwd=test_path, + env={**os.environ, "GIT_TERMINAL_PROMPT": "1"}, + ), + ] + ) + mock_ensure_daemon.assert_called_once_with(cwd=test_path, env=None) + assert {"code": 0, "message": ""} == actual_response @pytest.mark.asyncio @@ -482,9 +436,9 @@ async def test_git_push_with_auth_and_cache_credentials_and_existing_credential_ ] # When - auth = {"username": "user", "password": "pass"} + auth = {"username": "user", "password": "pass", "cache_credentials": True} actual_response = await Git(config=default_config).push( - ".", "HEAD:test_master", test_path, auth, cache_credentials=True + ".", "HEAD:test_master", test_path, auth ) # Then From d11c2dd00abd7e6afd74b2fed9a7ec866cd585db Mon Sep 17 00:00:00 2001 From: Dat Quach Date: Thu, 31 Mar 2022 06:51:14 -0700 Subject: [PATCH 26/44] Reformat `model.ts` with Prettier Also fix a minor typo. --- jupyterlab_git/__init__.py | 2 +- src/model.ts | 12 +++++++----- 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/jupyterlab_git/__init__.py b/jupyterlab_git/__init__.py index 60afb7bf3..69c04979f 100644 --- a/jupyterlab_git/__init__.py +++ b/jupyterlab_git/__init__.py @@ -40,7 +40,7 @@ class JupyterLabGit(Configurable): credential_helper = Unicode( help=""" The value of Git credential helper will be set to this value when the Git credential caching mechanism is activated by this extension. - By default it is a in-memory cache of 3600 seconds (1 hour); `cache --timeout=3600`. + By default it is an in-memory cache of 3600 seconds (1 hour); `cache --timeout=3600`. """, config=True, ) diff --git a/src/model.ts b/src/model.ts index 30ec8ca26..321266eb8 100644 --- a/src/model.ts +++ b/src/model.ts @@ -608,7 +608,7 @@ export class GitExtension implements IGitExtension { 'POST', { clone_url: url, - auth: auth as any, + auth: auth as any } ); } @@ -770,7 +770,7 @@ export class GitExtension implements IGitExtension { URLExt.join(path, 'remote', 'fetch'), 'POST', { - auth: auth as any, + auth: auth as any } ); } @@ -883,7 +883,7 @@ export class GitExtension implements IGitExtension { cancel_on_conflict: (this._settings?.composite[ 'cancelPullMergeConflict' - ] as boolean) || false, + ] as boolean) || false } ); } @@ -913,7 +913,7 @@ export class GitExtension implements IGitExtension { 'POST', { auth: auth as any, - force: force, + force: force } ); } @@ -1684,7 +1684,9 @@ export class GitExtension implements IGitExtension { private _dirtyStagedFilesStatusChanged = new Signal( this ); - private _credentialsRequiredChanged = new Signal(this); + private _credentialsRequiredChanged = new Signal( + this + ); } export class BranchMarker implements Git.IBranchMarker { From ac5cd9de6f4345057a3520c5e182189fa650af0b Mon Sep 17 00:00:00 2001 From: Dat Quach Date: Wed, 13 Apr 2022 21:31:10 -0700 Subject: [PATCH 27/44] Add new icons for commit comparison --- src/style/icons.ts | 10 ++++++++++ style/icons/compare-with-selected.svg | 6 ++++++ style/icons/select-for-compare.svg | 6 ++++++ 3 files changed, 22 insertions(+) create mode 100644 style/icons/compare-with-selected.svg create mode 100644 style/icons/select-for-compare.svg diff --git a/src/style/icons.ts b/src/style/icons.ts index 690b5d795..0af7070a3 100644 --- a/src/style/icons.ts +++ b/src/style/icons.ts @@ -5,6 +5,7 @@ import addSvg from '../../style/icons/add.svg'; import branchSvg from '../../style/icons/branch.svg'; import clockSvg from '../../style/icons/clock.svg'; import cloneSvg from '../../style/icons/clone.svg'; +import compareWithSelectedSvg from '../../style/icons/compare-with-selected.svg'; import deletionsMadeSvg from '../../style/icons/deletions.svg'; import desktopSvg from '../../style/icons/desktop.svg'; import diffSvg from '../../style/icons/diff.svg'; @@ -17,6 +18,7 @@ import pullSvg from '../../style/icons/pull.svg'; import pushSvg from '../../style/icons/push.svg'; import removeSvg from '../../style/icons/remove.svg'; import rewindSvg from '../../style/icons/rewind.svg'; +import selectForCompareSvg from '../../style/icons/select-for-compare.svg'; import tagSvg from '../../style/icons/tag.svg'; import trashSvg from '../../style/icons/trash.svg'; import verticalMoreSvg from '../../style/icons/vertical-more.svg'; @@ -34,6 +36,10 @@ export const cloneIcon = new LabIcon({ name: 'git:clone', svgstr: cloneSvg }); +export const compareWithSelectedIcon = new LabIcon({ + name: 'git:compare-with-selected', + svgstr: compareWithSelectedSvg +}); export const deletionsMadeIcon = new LabIcon({ name: 'git:deletions', svgstr: deletionsMadeSvg @@ -82,6 +88,10 @@ export const rewindIcon = new LabIcon({ name: 'git:rewind', svgstr: rewindSvg }); +export const selectForCompareIcon = new LabIcon({ + name: 'git:select-for-compare', + svgstr: selectForCompareSvg +}); export const tagIcon = new LabIcon({ name: 'git:tag', svgstr: tagSvg diff --git a/style/icons/compare-with-selected.svg b/style/icons/compare-with-selected.svg new file mode 100644 index 000000000..8ace65ea2 --- /dev/null +++ b/style/icons/compare-with-selected.svg @@ -0,0 +1,6 @@ + + + \ No newline at end of file diff --git a/style/icons/select-for-compare.svg b/style/icons/select-for-compare.svg new file mode 100644 index 000000000..a46860636 --- /dev/null +++ b/style/icons/select-for-compare.svg @@ -0,0 +1,6 @@ + + + \ No newline at end of file From 5cee9bfec6dafe0dfcc826cc83b34e2b61bc3b8b Mon Sep 17 00:00:00 2001 From: Dat Quach Date: Wed, 13 Apr 2022 21:39:18 -0700 Subject: [PATCH 28/44] Extend `Git.diff()` to accept two commit hashes --- jupyterlab_git/git.py | 23 +++++++++++++++-------- jupyterlab_git/handlers.py | 11 ++++++++++- 2 files changed, 25 insertions(+), 9 deletions(-) diff --git a/jupyterlab_git/git.py b/jupyterlab_git/git.py index 6bf3a9d42..acbbff58a 100644 --- a/jupyterlab_git/git.py +++ b/jupyterlab_git/git.py @@ -616,11 +616,17 @@ async def detailed_log(self, selected_hash, path): "modified_files": result, } - async def diff(self, path): + async def diff(self, path, previous=None, current=None): """ Execute git diff command & return the result. """ cmd = ["git", "diff", "--numstat", "-z"] + + if previous: + cmd.append(previous) + if current: + cmd.append(current) + code, my_output, my_error = await execute(cmd, cwd=path) if code != 0: @@ -630,13 +636,14 @@ async def diff(self, path): line_array = strip_and_split(my_output) for line in line_array: linesplit = line.split() - result.append( - { - "insertions": linesplit[0], - "deletions": linesplit[1], - "filename": linesplit[2], - } - ) + if len(linesplit) == 3: + result.append( + { + "insertions": linesplit[0], + "deletions": linesplit[1], + "filename": linesplit[2], + } + ) return {"code": code, "result": result} async def branch(self, path): diff --git a/jupyterlab_git/handlers.py b/jupyterlab_git/handlers.py index 7b16ef009..efbf73b4c 100644 --- a/jupyterlab_git/handlers.py +++ b/jupyterlab_git/handlers.py @@ -258,7 +258,16 @@ async def post(self, path: str = ""): POST request handler, fetches differences between commits & current working tree. """ - my_output = await self.git.diff(self.url2localpath(path)) + data = self.get_json_body() + + if data: + my_output = await self.git.diff( + self.url2localpath(path), + data.get("previous"), + data.get("current"), + ) + else: + my_output = await self.git.diff(self.url2localpath(path)) if my_output["code"] != 0: self.set_status(500) From 7e2e20c4dbfa38132794d73e835e1fbdcbecdb50 Mon Sep 17 00:00:00 2001 From: Dat Quach Date: Wed, 13 Apr 2022 21:42:30 -0700 Subject: [PATCH 29/44] Implement `IGitExtension.diff()` in `src/model.ts` --- src/model.ts | 36 ++++++++++++++++++++++++++++++++++++ src/tokens.ts | 31 +++++++++++++++++++++++++++++++ 2 files changed, 67 insertions(+) diff --git a/src/model.ts b/src/model.ts index 321266eb8..df83ffb31 100644 --- a/src/model.ts +++ b/src/model.ts @@ -721,6 +721,42 @@ export class GitExtension implements IGitExtension { return data; } + /** + * Get the diff of two commits. + * If no commit is provided, the diff of HEAD and INDEX is returned. + * If the current commit (the commit to compare) is not provided, + * the diff of the previous commit and INDEX is returned. + * + * @param previous - the commit to compare against + * @param current - the commit to compare + * @returns promise which resolves upon retrieving the diff + * + * @throws {Git.NotInRepository} If the current path is not a Git repository + * @throws {Git.GitResponseError} If the server response is not ok + * @throws {ServerConnection.NetworkError} If the request cannot be made + */ + async diff(previous?: string, current?: string): Promise { + const path = await this._getPathRepository(); + const data = await this._taskHandler.execute( + 'git:diff', + async () => { + return await requestAPI( + URLExt.join(path, 'diff'), + 'POST', + { + previous, + current + } + ); + } + ); + data.result = data.result.map(f => { + f.filetype = this._resolveFileType(f.filename); + return f; + }); + return data; + } + /** * Dispose of model resources. */ diff --git a/src/tokens.ts b/src/tokens.ts index 4e97da79a..6c38e99a2 100644 --- a/src/tokens.ts +++ b/src/tokens.ts @@ -286,6 +286,22 @@ export interface IGitExtension extends IDisposable { */ detailedLog(hash: string): Promise; + /** + * Get the diff of two commits. + * If no commit is provided, the diff of HEAD and INDEX is returned. + * If the current commit (the commit to compare) is not provided, + * the diff of the previous commit and INDEX is returned. + * + * @param previous - the commit to compare against + * @param current - the commit to compare + * @returns promise which resolves upon retrieving the diff + * + * @throws {Git.NotInRepository} If the current path is not a Git repository + * @throws {Git.GitResponseError} If the server response is not ok + * @throws {ServerConnection.NetworkError} If the request cannot be made + */ + diff(previous?: string, current?: string): Promise; + /** * Ensure a .gitignore file exists * @@ -808,6 +824,21 @@ export namespace Git { content: string; } + /** + * Interface for GitDiff request result + */ + export interface IDiffResult { + code: number; + command?: string; + message?: string; + result?: { + insertions: string; + deletions: string; + filename: string; + filetype?: DocumentRegistry.IFileType; + }[]; + } + /** * Git repository status */ From 3b715ed69cd8080467dd3e6816907a38dfa5c2f1 Mon Sep 17 00:00:00 2001 From: Dat Quach Date: Wed, 13 Apr 2022 21:44:58 -0700 Subject: [PATCH 30/44] Implement new React component `CommitComparisonBox` This component displays a comparison between two commits. --- src/components/CommitComparisonBox.tsx | 258 +++++++++++++++++++++++++ src/style/CommitComparisonBox.ts | 33 ++++ 2 files changed, 291 insertions(+) create mode 100644 src/components/CommitComparisonBox.tsx create mode 100644 src/style/CommitComparisonBox.ts diff --git a/src/components/CommitComparisonBox.tsx b/src/components/CommitComparisonBox.tsx new file mode 100644 index 000000000..4a7603b1d --- /dev/null +++ b/src/components/CommitComparisonBox.tsx @@ -0,0 +1,258 @@ +import { TranslationBundle } from '@jupyterlab/translation'; +import { + caretDownIcon, + caretRightIcon, + fileIcon +} from '@jupyterlab/ui-components'; +import { CommandRegistry } from '@lumino/commands'; +import * as React from 'react'; +import { FixedSizeList, ListChildComponentProps } from 'react-window'; +import { classes } from 'typestyle'; +import { getDiffProvider } from '../model'; +import { + clickableSpanStyle, + commitComparisonBoxChangedFileListStyle, + commitComparisonBoxDetailStyle +} from '../style/CommitComparisonBox'; +import { + changeStageButtonStyle, + sectionAreaStyle, + sectionHeaderLabelStyle +} from '../style/GitStageStyle'; +import { + deletionsMadeIcon, + diffIcon, + insertionsMadeIcon +} from '../style/icons'; +import { + commitClass, + commitDetailFileClass, + commitDetailHeaderClass, + commitOverviewNumbersClass, + deletionsIconClass, + fileListClass, + iconClass, + insertionsIconClass +} from '../style/SinglePastCommitInfo'; +import { Git, IGitExtension } from '../tokens'; +import { ActionButton } from './ActionButton'; +import { FilePath } from './FilePath'; + +const ITEM_HEIGHT = 24; // File list item height + +interface ICommitComparisonBoxHeaderProps { + collapsible: boolean; + collapsed?: boolean; + label?: string; + onCollapseExpand?: (event: React.MouseEvent) => void; + onClickCancel?: (event: React.MouseEvent) => void; +} + +const CommitComparisonBoxHeader: React.VFC = + props => { + return ( +
+ {props.collapsible && ( + + )} + {props.label} + {props.onClickCancel && ( + + Cancel + + )} +
+ ); + }; + +interface ICommitComparisonBoxOverviewProps { + totalFiles: number; + totalInsertions: number; + totalDeletions: number; + trans: TranslationBundle; +} + +const CommitComparisonBoxOverview: React.VFC = + props => { + return ( +
+
+ + + {props.totalFiles} + + + + {props.totalInsertions} + + + + {props.totalDeletions} + +
+
+ ); + }; + +interface ICommitComparisonBoxChangedFileListProps { + files: Git.ICommitModifiedFile[]; + trans: TranslationBundle; + onOpenDiff?: ( + filePath: string, + isText: boolean, + previousFilePath?: string + ) => (event: React.MouseEvent) => void; +} + +class CommitComparisonBoxChangedFileList extends React.Component { + render() { + return ( +
+
+ {this.props.trans.__('Changed')} +
+ {this.props.files.length > 0 && ( + data[index].modified_file_path} + itemSize={ITEM_HEIGHT} + style={{ overflowX: 'hidden' }} + width={'auto'} + > + {this._renderFile} + + )} +
+ ); + } + /** + * Renders a modified file. + * + * @param props Row properties + * @returns React element + */ + private _renderFile = ( + props: ListChildComponentProps + ): JSX.Element => { + const { data, index, style } = props; + const file = data[index]; + const path = file.modified_file_path; + const previous = file.previous_file_path; + const flg = !!getDiffProvider(path) || !file.is_binary; + return ( +
  • + + {flg ? ( + + ) : null} +
  • + ); + }; +} + +interface ICommitComparisonBoxBodyProps { + files: Git.ICommitModifiedFile[]; + show: boolean; + trans: TranslationBundle; + onOpenDiff?: ( + filePath: string, + isText: boolean, + previousFilePath?: string + ) => (event: React.MouseEvent) => void; +} + +const CommitComparisonBoxBody: React.VFC = + props => { + const totalInsertions = props.files.reduce((acc, file) => { + const insertions = Number.parseInt(file.insertion, 10); + return acc + (Number.isNaN(insertions) ? 0 : insertions); + }, 0); + const totalDeletions = props.files.reduce((acc, file) => { + const deletions = Number.parseInt(file.deletion, 10); + return acc + (Number.isNaN(deletions) ? 0 : deletions); + }, 0); + return ( + + + + + ); + }; + +export interface ICommitComparisonBoxProps { + collapsible: boolean; + commands: CommandRegistry; + comparison: Git.ICommitComparison | null; + header: string; + model: IGitExtension; + trans: TranslationBundle; + onCancel?: (event: React.MouseEvent) => void; + onOpenDiff?: ( + filePath: string, + isText: boolean, + previousFilePath?: string + ) => (event: React.MouseEvent) => void; +} + +/** + * A component which displays a comparison between two commits. + */ +export const CommitComparisonBox: React.VFC = + props => { + const [collapsed, setCollapsed] = React.useState(false); + return ( + + setCollapsed(!collapsed)} + onClickCancel={props.onCancel} + /> + {!collapsed && props.comparison?.changedFiles && ( + + )} + + ); + }; diff --git a/src/style/CommitComparisonBox.ts b/src/style/CommitComparisonBox.ts new file mode 100644 index 000000000..18a032827 --- /dev/null +++ b/src/style/CommitComparisonBox.ts @@ -0,0 +1,33 @@ +import { style } from 'typestyle'; + +export const clickableSpanStyle = style({ + cursor: 'pointer' +}); + +export const commitComparisonBoxStyle = style({ + display: 'flex', + flexDirection: 'column', + + minHeight: '200px', + + marginBlockStart: 0, + marginBlockEnd: 0, + paddingLeft: 0, + + overflowY: 'auto', + + $nest: { + '& button:disabled': { + opacity: 0.5 + } + } +}); + +export const commitComparisonBoxDetailStyle = style({ + maxHeight: '25%', + overflowY: 'hidden' +}); + +export const commitComparisonBoxChangedFileListStyle = style({ + maxHeight: '100%' +}); From 462e30926f08783a88ee6a4c13018d5bb9512615 Mon Sep 17 00:00:00 2001 From: Dat Quach Date: Wed, 13 Apr 2022 21:51:26 -0700 Subject: [PATCH 31/44] Add two buttons to `PastCommitNode` These buttons allow selecting commits for comparison. --- src/components/PastCommitNode.tsx | 55 +++++++++++++++++++++++++++++-- src/style/PastCommitNode.ts | 8 +++++ 2 files changed, 61 insertions(+), 2 deletions(-) diff --git a/src/components/PastCommitNode.tsx b/src/components/PastCommitNode.tsx index f9cb2eaa2..3305c7521 100644 --- a/src/components/PastCommitNode.tsx +++ b/src/components/PastCommitNode.tsx @@ -4,11 +4,17 @@ import { CommandRegistry } from '@lumino/commands'; import * as React from 'react'; import { classes } from 'typestyle'; import { GitExtension } from '../model'; -import { diffIcon } from '../style/icons'; +import { + compareWithSelectedIcon, + diffIcon, + selectForCompareIcon +} from '../style/icons'; import { branchClass, branchWrapperClass, commitBodyClass, + commitCompareLhsNodeClass, + commitCompareRhsNodeClass, commitExpandedClass, commitHeaderClass, commitHeaderItemClass, @@ -20,6 +26,7 @@ import { workingBranchClass } from '../style/PastCommitNode'; import { Git } from '../tokens'; +import { ActionButton } from './ActionButton'; /** * Interface describing component properties. @@ -50,6 +57,16 @@ export interface IPastCommitNodeProps { */ trans: TranslationBundle; + /** + * The commit to compare against. + */ + isCommitCompareLhs?: boolean; + + /** + * The commit to compare. + */ + isCommitCompareRhs?: boolean; + /** * Callback invoked upon clicking to display a file diff. * @@ -58,6 +75,22 @@ export interface IPastCommitNodeProps { onOpenDiff?: ( event: React.MouseEvent ) => Promise; + + /** + * Callback invoked upon clicking to select a commit for comparison. + * @param event - event object + */ + onSelectForCompare?: ( + event: React.MouseEvent + ) => Promise; + + /** + * Callback invoked upon clicking to compare a commit against the selected. + * @param event - event object + */ + onCompareWithSelected?: ( + event: React.MouseEvent + ) => Promise; } /** @@ -104,7 +137,9 @@ export class PastCommitNode extends React.Component< ? singleFileCommitClass : this.state.expanded ? commitExpandedClass - : null + : null, + this.props.isCommitCompareLhs && commitCompareLhsNodeClass, + this.props.isCommitCompareRhs && commitCompareRhsNodeClass )} title={ this.props.children @@ -125,6 +160,22 @@ export class PastCommitNode extends React.Component< {this.props.commit.date} + {!this.props.commit.is_binary && ( + + + + + )} {this.props.children ? ( this.state.expanded ? ( diff --git a/src/style/PastCommitNode.ts b/src/style/PastCommitNode.ts index 33cea04a1..a95cad398 100644 --- a/src/style/PastCommitNode.ts +++ b/src/style/PastCommitNode.ts @@ -83,3 +83,11 @@ export const singleFileCommitClass = style({ } } }); + +export const commitCompareLhsNodeClass = style({ + borderLeft: '3px solid darkred' +}); + +export const commitCompareRhsNodeClass = style({ + borderLeft: '3px solid darkgreen' +}); From 836d4381a7b11d5731da9da1c4c7624018206a3c Mon Sep 17 00:00:00 2001 From: Dat Quach Date: Wed, 13 Apr 2022 21:56:44 -0700 Subject: [PATCH 32/44] Implement interface `ICommitComparison` This interface is used for comparison results. --- src/tokens.ts | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/src/tokens.ts b/src/tokens.ts index 6c38e99a2..331dc4907 100644 --- a/src/tokens.ts +++ b/src/tokens.ts @@ -1026,6 +1026,25 @@ export namespace Git { toggle(fname: string): void; } + /** + * Interface for the result of comparing two commits + */ + export interface ICommitComparison { + /** + * The commit to compare against + */ + lhs: ISingleCommitInfo; + /** + * The commit to compare + */ + rhs: ISingleCommitInfo; + /** + * The list of files that have been modified + * in the compared commits + */ + changedFiles: ICommitModifiedFile[]; + } + export type Status = | 'untracked' | 'staged' From 46f5f8c2d49196fa81641597c744a49f2702318a Mon Sep 17 00:00:00 2001 From: Dat Quach Date: Wed, 13 Apr 2022 22:08:09 -0700 Subject: [PATCH 33/44] Move the function `openDiff()` out to `util.ts` This will also be used by the commit comparison logic that will exist in `src/components/GitPanel.tsx` later. --- src/components/HistorySideBar.tsx | 54 +++------------------------- src/utils.ts | 59 ++++++++++++++++++++++++++++++- 2 files changed, 62 insertions(+), 51 deletions(-) diff --git a/src/components/HistorySideBar.tsx b/src/components/HistorySideBar.tsx index b13913e56..8500a7d1f 100644 --- a/src/components/HistorySideBar.tsx +++ b/src/components/HistorySideBar.tsx @@ -2,7 +2,6 @@ import { TranslationBundle } from '@jupyterlab/translation'; import { closeIcon } from '@jupyterlab/ui-components'; import { CommandRegistry } from '@lumino/commands'; import * as React from 'react'; -import { CommandArguments } from '../commandsAndMenu'; import { GitExtension } from '../model'; import { hiddenButtonStyle } from '../style/ActionButtonStyle'; import { @@ -10,7 +9,8 @@ import { noHistoryFoundStyle, selectedHistoryFileStyle } from '../style/HistorySideBarStyle'; -import { ContextCommandIDs, Git } from '../tokens'; +import { Git } from '../tokens'; +import { openFileDiff } from '../utils'; import { ActionButton } from './ActionButton'; import { FileItem } from './FileItem'; import { PastCommitNode } from './PastCommitNode'; @@ -62,52 +62,6 @@ export const HistorySideBar: React.FunctionComponent = ( props.model.selectedHistoryFile = null; }; - /** - * Curried callback function to display a file diff. - * - * @param commit Commit data. - */ - const openDiff = - (commit: Git.ISingleCommitInfo) => - /** - * Returns a callback to be invoked on click to display a file diff. - * - * @param filePath file path - * @param isText indicates whether the file supports displaying a diff - * @param previousFilePath when file has been relocated - * @returns callback - */ - (filePath: string, isText: boolean, previousFilePath?: string) => - /** - * Callback invoked upon clicking to display a file diff. - * - * @param event - event object - */ - async (event: React.MouseEvent) => { - // Prevent the commit component from being collapsed: - event.stopPropagation(); - - if (isText) { - try { - props.commands.execute(ContextCommandIDs.gitFileDiff, { - files: [ - { - filePath, - previousFilePath, - isText, - context: { - previousRef: commit.pre_commit, - currentRef: commit.commit - } - } - ] - } as CommandArguments.IGitFileDiff as any); - } catch (err) { - console.error(`Failed to open diff view for ${filePath}.\n${err}`); - } - } - }; - /** * Commit info for 'Uncommitted Changes' history. */ @@ -165,7 +119,7 @@ export const HistorySideBar: React.FunctionComponent = ( // and its diff is viewable const onOpenDiff = props.model.selectedHistoryFile && !commit.is_binary - ? openDiff(commit)( + ? openFileDiff(props.commands)(commit)( commit.file_path ?? props.model.selectedHistoryFile.to, !commit.is_binary, commit.previous_file_path @@ -181,7 +135,7 @@ export const HistorySideBar: React.FunctionComponent = ( {!props.model.selectedHistoryFile && ( )} diff --git a/src/utils.ts b/src/utils.ts index 0d1818842..299e86d27 100644 --- a/src/utils.ts +++ b/src/utils.ts @@ -1,5 +1,7 @@ import { PathExt } from '@jupyterlab/coreutils'; -import { Git } from './tokens'; +import { CommandRegistry } from '@lumino/commands'; +import { CommandArguments } from './commandsAndMenu'; +import { ContextCommandIDs, Git } from './tokens'; /** Get the filename from a path */ export function extractFilename(path: string): string { @@ -52,3 +54,58 @@ export function decodeStage(x: string, y: string): Git.Status { export function sleep(ms: number): Promise { return new Promise(resolve => setTimeout(resolve, ms)); } + +/** + * A callback function to display a file diff between two commits. + * @param commands the command registry. + * @returns a callback function to display a file diff. + */ +export const openFileDiff = + (commands: CommandRegistry) => + /** + * A callback function to display a file diff between two commits. + * + * @param commit Commit data. + * @param previousCommit Previous commit data to display the diff against. If not specified, the diff will be against the preceding commit. + * + * @returns A callback function. + */ + (commit: Git.ISingleCommitInfo, previousCommit?: Git.ISingleCommitInfo) => + /** + * Returns a callback to be invoked on click to display a file diff. + * + * @param filePath file path. + * @param isText indicates whether the file supports displaying a diff. + * @param previousFilePath when file has been relocated. + * @returns callback. + */ + (filePath: string, isText: boolean, previousFilePath?: string) => + /** + * Callback invoked upon clicking to display a file diff. + * + * @param event - event object + */ + async (event: React.MouseEvent) => { + // Prevent the commit component from being collapsed: + event.stopPropagation(); + + if (isText) { + try { + commands.execute(ContextCommandIDs.gitFileDiff, { + files: [ + { + filePath, + previousFilePath, + isText, + context: { + previousRef: previousCommit?.commit ?? commit.pre_commit, + currentRef: commit.commit + } + } + ] + } as CommandArguments.IGitFileDiff as any); + } catch (err) { + console.error(`Failed to open diff view for ${filePath}.\n${err}`); + } + } + }; From 582773aa4d83c495f26fb05c817f960858a985d3 Mon Sep 17 00:00:00 2001 From: Dat Quach Date: Wed, 13 Apr 2022 22:10:17 -0700 Subject: [PATCH 34/44] Extend `HistorySideBar` for commit comparison --- src/components/HistorySideBar.tsx | 34 +++++++++++++++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/src/components/HistorySideBar.tsx b/src/components/HistorySideBar.tsx index 8500a7d1f..d99d9973f 100644 --- a/src/components/HistorySideBar.tsx +++ b/src/components/HistorySideBar.tsx @@ -44,6 +44,32 @@ export interface IHistorySideBarProps { * The application language translator. */ trans: TranslationBundle; + + /** + * The commit to compare against. + */ + commitCompareLhs?: Git.ISingleCommitInfo; + + /** + * The commit to compare. + */ + commitCompareRhs?: Git.ISingleCommitInfo; + + /** + * Callback invoked upon clicking to select a commit for comparison. + * @param event - event object + */ + onSelectForCompare?: ( + commit: Git.ISingleCommitInfo + ) => (event: React.MouseEvent) => Promise; + + /** + * Callback invoked upon clicking to compare a commit against the selected. + * @param event - event object + */ + onCompareWithSelected?: ( + commit: Git.ISingleCommitInfo + ) => (event: React.MouseEvent) => Promise; } /** @@ -130,7 +156,15 @@ export const HistorySideBar: React.FunctionComponent = ( {!props.model.selectedHistoryFile && ( Date: Wed, 13 Apr 2022 22:11:48 -0700 Subject: [PATCH 35/44] Extend `GitPanel` to allow commit comparison The comparison status is invalidated when the branch status gets refreshed. --- src/components/GitPanel.tsx | 190 ++++++++++++++++++++++++++++++++++-- 1 file changed, 181 insertions(+), 9 deletions(-) diff --git a/src/components/GitPanel.tsx b/src/components/GitPanel.tsx index 694f1c53e..bf6c47ec9 100644 --- a/src/components/GitPanel.tsx +++ b/src/components/GitPanel.tsx @@ -21,8 +21,10 @@ import { warningTextClass } from '../style/GitPanel'; import { CommandIDs, Git, ILogMessage, Level } from '../tokens'; +import { openFileDiff } from '../utils'; import { GitAuthorForm } from '../widgets/AuthorBox'; import { CommitBox } from './CommitBox'; +import { CommitComparisonBox } from './CommitComparisonBox'; import { FileList } from './FileList'; import { HistorySideBar } from './HistorySideBar'; import { Toolbar } from './Toolbar'; @@ -130,6 +132,21 @@ export interface IGitPanelState { * Whether there are dirty (e.g., unsaved) staged files. */ hasDirtyStagedFiles: boolean; + + /** + * The commit to compare against + */ + commitCompareLhs: Git.ISingleCommitInfo | null; + + /** + * The commit to compare + */ + commitCompareRhs: Git.ISingleCommitInfo | null; + + /** + * The commit comparison result + */ + commitComparison: Git.ICommitComparison | null; } /** @@ -160,7 +177,10 @@ export class GitPanel extends React.Component { commitSummary: '', commitDescription: '', commitAmend: false, - hasDirtyStagedFiles: hasDirtyStagedFiles + hasDirtyStagedFiles: hasDirtyStagedFiles, + commitCompareLhs: null, + commitCompareRhs: null, + commitComparison: null }; } @@ -210,6 +230,30 @@ export class GitPanel extends React.Component { }); } + async componentDidUpdate( + _: Readonly, + { + commitCompareLhs: prevCommitCompareLhs, + commitCompareRhs: prevCommitCompareRhs, + commitComparison: prevCommitComparison + }: Readonly, + __?: any + ): Promise { + const { + commitCompareLhs: currCommitCompareLhs, + commitCompareRhs: currCommitCompareRhs + } = this.state; + + const commitsReady = currCommitCompareLhs && currCommitCompareRhs; + const commitChanged = + prevCommitCompareLhs !== currCommitCompareLhs || + prevCommitCompareRhs !== currCommitCompareRhs; + + if (commitsReady && (!!prevCommitComparison || commitChanged)) { + await this._doCommitComparsion(); + } + } + componentWillUnmount(): void { // Clear all signal connections Signal.clearData(this); @@ -220,7 +264,10 @@ export class GitPanel extends React.Component { this.setState({ branches: this.props.model.branches, - currentBranch: currentBranch ? currentBranch.name : 'master' + currentBranch: currentBranch ? currentBranch.name : 'master', + commitCompareLhs: null, + commitCompareRhs: null, + commitComparison: null }); }; @@ -440,13 +487,64 @@ export class GitPanel extends React.Component { */ private _renderHistory(): React.ReactElement { return ( - + + async event => { + event.stopPropagation(); + this._setCommitComparisonState({ lhs: commit }); + }} + onCompareWithSelected={commit => async event => { + event.stopPropagation(); + this._setCommitComparisonState({ rhs: commit }); + }} + /> + {(this.state.commitCompareLhs || this.state.commitCompareRhs) && ( + { + event.stopPropagation(); + this._setCommitComparisonState({ + lhs: null, + rhs: null, + res: null + }); + }} + onOpenDiff={ + this.state.commitComparison + ? openFileDiff(this.props.commands)( + this.state.commitComparison.rhs, + this.state.commitComparison.lhs + ) + : undefined + } + /> + )} + ); } @@ -752,4 +850,78 @@ export class GitPanel extends React.Component { } return
    {elem}
    ; } + + private _setCommitComparisonState(state: { + lhs?: Git.ISingleCommitInfo; + rhs?: Git.ISingleCommitInfo; + res?: Git.ICommitComparison; + }): void { + this.setState(currentState => ({ + commitCompareLhs: + typeof state.lhs !== 'undefined' + ? state.lhs + : currentState.commitCompareLhs, + commitCompareRhs: + typeof state.rhs !== 'undefined' + ? state.rhs + : currentState.commitCompareRhs, + commitComparison: + typeof state.res !== 'undefined' + ? state.res + : currentState.commitComparison + })); + } + + private async _doCommitComparsion(): Promise { + let diffResult: Git.IDiffResult = null; + try { + diffResult = await this.props.model.diff( + this.state.commitCompareLhs.commit, + this.state.commitCompareRhs.commit + ); + if (diffResult.code !== 0) throw new Error(diffResult.message); + } catch (err) { + console.error( + `Error while getting the diff for commit ${this.state.commitCompareLhs} and commit ${this.state.commitCompareRhs}!`, + err + ); + this.props.logger.log({ + level: Level.ERROR, + message: `Error while getting the diff for commit ${this.state.commitCompareLhs} and commit ${this.state.commitCompareRhs}!`, + error: err + }); + return; + } + if (diffResult) { + this.setState(state => { + return { + commitComparison: { + lhs: state.commitCompareLhs, + rhs: state.commitCompareRhs, + changedFiles: diffResult.result.map(changedFile => { + const pathParts = changedFile.filename.split('/'); + const fileName = pathParts[pathParts.length - 1]; + const filePath = changedFile.filename; + return { + deletion: changedFile.deletions, + insertion: changedFile.insertions, + is_binary: + changedFile.deletions === '-' || + changedFile.insertions === '-', + modified_file_name: fileName, + modified_file_path: filePath, + type: changedFile.filetype + } as Git.ICommitModifiedFile; + }) + } as Git.ICommitComparison + }; + }); + } else { + this.setState({ + commitCompareLhs: null, + commitCompareRhs: null, + commitComparison: null + }); + } + } } From e0b4b4648c5a31ad9bbfede459dec15e4c9b71e7 Mon Sep 17 00:00:00 2001 From: Dat Quach Date: Wed, 13 Apr 2022 23:21:39 -0700 Subject: [PATCH 36/44] Fix linting and format problems --- src/components/CommitComparisonBox.tsx | 141 ++++++++++++------------- src/components/GitPanel.tsx | 7 +- src/utils.ts | 2 +- 3 files changed, 75 insertions(+), 75 deletions(-) diff --git a/src/components/CommitComparisonBox.tsx b/src/components/CommitComparisonBox.tsx index 4a7603b1d..084d167a9 100644 --- a/src/components/CommitComparisonBox.tsx +++ b/src/components/CommitComparisonBox.tsx @@ -48,28 +48,25 @@ interface ICommitComparisonBoxHeaderProps { onClickCancel?: (event: React.MouseEvent) => void; } -const CommitComparisonBoxHeader: React.VFC = - props => { - return ( -
    - {props.collapsible && ( - - )} - {props.label} - {props.onClickCancel && ( - - Cancel - - )} -
    - ); - }; +const CommitComparisonBoxHeader: React.VFC = ( + props: ICommitComparisonBoxHeaderProps +) => { + return ( +
    + {props.collapsible && ( + + )} + {props.label} + {props.onClickCancel && ( + + Cancel + + )} +
    + ); +}; interface ICommitComparisonBoxOverviewProps { totalFiles: number; @@ -79,7 +76,7 @@ interface ICommitComparisonBoxOverviewProps { } const CommitComparisonBoxOverview: React.VFC = - props => { + (props: ICommitComparisonBoxOverviewProps) => { return (
    @@ -188,32 +185,33 @@ interface ICommitComparisonBoxBodyProps { ) => (event: React.MouseEvent) => void; } -const CommitComparisonBoxBody: React.VFC = - props => { - const totalInsertions = props.files.reduce((acc, file) => { - const insertions = Number.parseInt(file.insertion, 10); - return acc + (Number.isNaN(insertions) ? 0 : insertions); - }, 0); - const totalDeletions = props.files.reduce((acc, file) => { - const deletions = Number.parseInt(file.deletion, 10); - return acc + (Number.isNaN(deletions) ? 0 : deletions); - }, 0); - return ( - - - - - ); - }; +const CommitComparisonBoxBody: React.VFC = ( + props: ICommitComparisonBoxBodyProps +) => { + const totalInsertions = props.files.reduce((acc, file) => { + const insertions = Number.parseInt(file.insertion, 10); + return acc + (Number.isNaN(insertions) ? 0 : insertions); + }, 0); + const totalDeletions = props.files.reduce((acc, file) => { + const deletions = Number.parseInt(file.deletion, 10); + return acc + (Number.isNaN(deletions) ? 0 : deletions); + }, 0); + return ( + + + + + ); +}; export interface ICommitComparisonBoxProps { collapsible: boolean; @@ -233,26 +231,27 @@ export interface ICommitComparisonBoxProps { /** * A component which displays a comparison between two commits. */ -export const CommitComparisonBox: React.VFC = - props => { - const [collapsed, setCollapsed] = React.useState(false); - return ( - - setCollapsed(!collapsed)} - onClickCancel={props.onCancel} +export const CommitComparisonBox: React.VFC = ( + props: ICommitComparisonBoxProps +) => { + const [collapsed, setCollapsed] = React.useState(false); + return ( + + setCollapsed(!collapsed)} + onClickCancel={props.onCancel} + /> + {!collapsed && props.comparison?.changedFiles && ( + - {!collapsed && props.comparison?.changedFiles && ( - - )} - - ); - }; + )} + + ); +}; diff --git a/src/components/GitPanel.tsx b/src/components/GitPanel.tsx index b5450026c..9f92d66f7 100644 --- a/src/components/GitPanel.tsx +++ b/src/components/GitPanel.tsx @@ -239,8 +239,7 @@ export class GitPanel extends React.Component { commitCompareLhs: prevCommitCompareLhs, commitCompareRhs: prevCommitCompareRhs, commitComparison: prevCommitComparison - }: Readonly, - __?: any + }: Readonly ): Promise { const { commitCompareLhs: currCommitCompareLhs, @@ -887,7 +886,9 @@ export class GitPanel extends React.Component { this.state.commitCompareLhs.commit, this.state.commitCompareRhs.commit ); - if (diffResult.code !== 0) throw new Error(diffResult.message); + if (diffResult.code !== 0) { + throw new Error(diffResult.message); + } } catch (err) { console.error( `Error while getting the diff for commit ${this.state.commitCompareLhs} and commit ${this.state.commitCompareRhs}!`, diff --git a/src/utils.ts b/src/utils.ts index 299e86d27..f4c0a9f17 100644 --- a/src/utils.ts +++ b/src/utils.ts @@ -85,7 +85,7 @@ export const openFileDiff = * * @param event - event object */ - async (event: React.MouseEvent) => { + async (event: React.MouseEvent): Promise => { // Prevent the commit component from being collapsed: event.stopPropagation(); From 547b1cde21dbae4a8440d86a907aa9d7803129b0 Mon Sep 17 00:00:00 2001 From: Dat Quach Date: Wed, 13 Apr 2022 23:46:21 -0700 Subject: [PATCH 37/44] Mock new props in `HistorySideBar` test file --- tests/test-components/HistorySideBar.spec.tsx | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/tests/test-components/HistorySideBar.spec.tsx b/tests/test-components/HistorySideBar.spec.tsx index c7e78e6eb..6aecf7b0f 100644 --- a/tests/test-components/HistorySideBar.spec.tsx +++ b/tests/test-components/HistorySideBar.spec.tsx @@ -23,7 +23,7 @@ describe('HistorySideBar', () => { author: null, date: null, commit_msg: null, - pre_commit: null + pre_commit: null, } ], branches: [], @@ -31,7 +31,11 @@ describe('HistorySideBar', () => { selectedHistoryFile: null } as GitExtension, commands: null, - trans + trans, + commitCompareLhs: null, + commitCompareRhs: null, + onSelectForCompare: _ => async _ => null, + onCompareWithSelected: _ => async _ => null }; it('renders the commit nodes', () => { From 0da102486dbffdc4deaed4c3f19d0432a458b2da Mon Sep 17 00:00:00 2001 From: Zeshan Fayyaz <43391249+ZeshanFayyaz@users.noreply.github.com> Date: Tue, 19 Apr 2022 03:16:02 -0400 Subject: [PATCH 38/44] Implemented naming convention changes --- src/components/CommitComparisonBox.tsx | 6 +- src/components/GitPanel.tsx | 96 +++++++++---------- src/components/HistorySideBar.tsx | 12 +-- src/components/PastCommitNode.tsx | 12 +-- src/style/PastCommitNode.ts | 8 +- src/tokens.ts | 4 +- tests/test-components/HistorySideBar.spec.tsx | 4 +- 7 files changed, 73 insertions(+), 69 deletions(-) diff --git a/src/components/CommitComparisonBox.tsx b/src/components/CommitComparisonBox.tsx index 084d167a9..42de5f128 100644 --- a/src/components/CommitComparisonBox.tsx +++ b/src/components/CommitComparisonBox.tsx @@ -44,6 +44,7 @@ interface ICommitComparisonBoxHeaderProps { collapsible: boolean; collapsed?: boolean; label?: string; + trans: TranslationBundle; onCollapseExpand?: (event: React.MouseEvent) => void; onClickCancel?: (event: React.MouseEvent) => void; } @@ -61,7 +62,7 @@ const CommitComparisonBoxHeader: React.VFC = ( {props.label} {props.onClickCancel && ( - Cancel + {props.trans.__('Cancel')} )}
    @@ -213,6 +214,9 @@ const CommitComparisonBoxBody: React.VFC = ( ); }; +/** + * Interface describing ComparisonBox component properties. + */ export interface ICommitComparisonBoxProps { collapsible: boolean; commands: CommandRegistry; diff --git a/src/components/GitPanel.tsx b/src/components/GitPanel.tsx index 9f92d66f7..b18364436 100644 --- a/src/components/GitPanel.tsx +++ b/src/components/GitPanel.tsx @@ -136,12 +136,12 @@ export interface IGitPanelState { /** * The commit to compare against */ - commitCompareLhs: Git.ISingleCommitInfo | null; + referenceCommit: Git.ISingleCommitInfo | null; /** * The commit to compare */ - commitCompareRhs: Git.ISingleCommitInfo | null; + challengerCommit: Git.ISingleCommitInfo | null; /** * The commit comparison result @@ -178,8 +178,8 @@ export class GitPanel extends React.Component { commitDescription: '', commitAmend: false, hasDirtyStagedFiles: hasDirtyStagedFiles, - commitCompareLhs: null, - commitCompareRhs: null, + referenceCommit: null, + challengerCommit: null, commitComparison: null }; } @@ -236,20 +236,20 @@ export class GitPanel extends React.Component { async componentDidUpdate( _: Readonly, { - commitCompareLhs: prevCommitCompareLhs, - commitCompareRhs: prevCommitCompareRhs, + referenceCommit: prevReferenceCommit, + challengerCommit: prevChallengerCommit, commitComparison: prevCommitComparison }: Readonly ): Promise { const { - commitCompareLhs: currCommitCompareLhs, - commitCompareRhs: currCommitCompareRhs + referenceCommit: currReferenceCommit, + challengerCommit: currChallengerCommit } = this.state; - const commitsReady = currCommitCompareLhs && currCommitCompareRhs; + const commitsReady = currReferenceCommit && currChallengerCommit; const commitChanged = - prevCommitCompareLhs !== currCommitCompareLhs || - prevCommitCompareRhs !== currCommitCompareRhs; + prevReferenceCommit !== currReferenceCommit || + prevChallengerCommit !== currChallengerCommit; if (commitsReady && (!!prevCommitComparison || commitChanged)) { await this._doCommitComparsion(); @@ -272,8 +272,8 @@ export class GitPanel extends React.Component { this.setState({ currentBranch: currentBranch ? currentBranch.name : 'master', - commitCompareLhs: null, - commitCompareRhs: null, + referenceCommit: null, + challengerCommit: null, commitComparison: null }); }; @@ -501,18 +501,18 @@ export class GitPanel extends React.Component { model={this.props.model} commands={this.props.commands} trans={this.props.trans} - commitCompareLhs={this.state.commitCompareLhs} - commitCompareRhs={this.state.commitCompareRhs} + referenceCommit={this.state.referenceCommit} + challengerCommit={this.state.challengerCommit} onSelectForCompare={commit => async event => { event.stopPropagation(); - this._setCommitComparisonState({ lhs: commit }); + this._setCommitComparisonState({ reference: commit }); }} onCompareWithSelected={commit => async event => { event.stopPropagation(); - this._setCommitComparisonState({ rhs: commit }); + this._setCommitComparisonState({ challenger: commit }); }} /> - {(this.state.commitCompareLhs || this.state.commitCompareRhs) && ( + {(this.state.referenceCommit || this.state.challengerCommit) && ( { header={` Compare ${ - this.state.commitCompareLhs - ? this.state.commitCompareLhs.commit.substring(0, 7) + this.state.referenceCommit + ? this.state.referenceCommit.commit.substring(0, 7) : '...' } and ${ - this.state.commitCompareRhs - ? this.state.commitCompareRhs.commit.substring(0, 7) + this.state.challengerCommit + ? this.state.challengerCommit.commit.substring(0, 7) : '...' } `} @@ -536,16 +536,16 @@ export class GitPanel extends React.Component { onCancel={event => { event.stopPropagation(); this._setCommitComparisonState({ - lhs: null, - rhs: null, - res: null + reference: null, + challenger: null, + comparison: null }); }} onOpenDiff={ this.state.commitComparison ? openFileDiff(this.props.commands)( - this.state.commitComparison.rhs, - this.state.commitComparison.lhs + this.state.commitComparison.challenger, + this.state.commitComparison.reference ) : undefined } @@ -859,22 +859,22 @@ export class GitPanel extends React.Component { } private _setCommitComparisonState(state: { - lhs?: Git.ISingleCommitInfo; - rhs?: Git.ISingleCommitInfo; - res?: Git.ICommitComparison; + reference?: Git.ISingleCommitInfo; + challenger?: Git.ISingleCommitInfo; + comparison?: Git.ICommitComparison; }): void { this.setState(currentState => ({ - commitCompareLhs: - typeof state.lhs !== 'undefined' - ? state.lhs - : currentState.commitCompareLhs, - commitCompareRhs: - typeof state.rhs !== 'undefined' - ? state.rhs - : currentState.commitCompareRhs, + referenceCommit: + typeof state.reference !== 'undefined' + ? state.reference + : currentState.referenceCommit, + challengerCommit: + typeof state.challenger !== 'undefined' + ? state.challenger + : currentState.challengerCommit, commitComparison: - typeof state.res !== 'undefined' - ? state.res + typeof state.comparison !== 'undefined' + ? state.comparison : currentState.commitComparison })); } @@ -883,20 +883,20 @@ export class GitPanel extends React.Component { let diffResult: Git.IDiffResult = null; try { diffResult = await this.props.model.diff( - this.state.commitCompareLhs.commit, - this.state.commitCompareRhs.commit + this.state.referenceCommit.commit, + this.state.challengerCommit.commit ); if (diffResult.code !== 0) { throw new Error(diffResult.message); } } catch (err) { console.error( - `Error while getting the diff for commit ${this.state.commitCompareLhs} and commit ${this.state.commitCompareRhs}!`, + `Error while getting the diff for commit ${this.state.referenceCommit} and commit ${this.state.challengerCommit}!`, err ); this.props.logger.log({ level: Level.ERROR, - message: `Error while getting the diff for commit ${this.state.commitCompareLhs} and commit ${this.state.commitCompareRhs}!`, + message: `Error while getting the diff for commit ${this.state.referenceCommit} and commit ${this.state.challengerCommit}!`, error: err }); return; @@ -905,8 +905,8 @@ export class GitPanel extends React.Component { this.setState(state => { return { commitComparison: { - lhs: state.commitCompareLhs, - rhs: state.commitCompareRhs, + reference: state.referenceCommit, + challenger: state.challengerCommit, changedFiles: diffResult.result.map(changedFile => { const pathParts = changedFile.filename.split('/'); const fileName = pathParts[pathParts.length - 1]; @@ -927,8 +927,8 @@ export class GitPanel extends React.Component { }); } else { this.setState({ - commitCompareLhs: null, - commitCompareRhs: null, + referenceCommit: null, + challengerCommit: null, commitComparison: null }); } diff --git a/src/components/HistorySideBar.tsx b/src/components/HistorySideBar.tsx index d99d9973f..78aa01446 100644 --- a/src/components/HistorySideBar.tsx +++ b/src/components/HistorySideBar.tsx @@ -48,12 +48,12 @@ export interface IHistorySideBarProps { /** * The commit to compare against. */ - commitCompareLhs?: Git.ISingleCommitInfo; + referenceCommit?: Git.ISingleCommitInfo; /** * The commit to compare. */ - commitCompareRhs?: Git.ISingleCommitInfo; + challengerCommit?: Git.ISingleCommitInfo; /** * Callback invoked upon clicking to select a commit for comparison. @@ -156,11 +156,11 @@ export const HistorySideBar: React.FunctionComponent = ( { } as GitExtension, commands: null, trans, - commitCompareLhs: null, - commitCompareRhs: null, + referenceCommit: null, + challengerCommit: null, onSelectForCompare: _ => async _ => null, onCompareWithSelected: _ => async _ => null }; From bcd4bf2616de417a0b1725402f07e81bfd1e73bf Mon Sep 17 00:00:00 2001 From: Dat Quach Date: Tue, 19 Apr 2022 20:38:15 -0700 Subject: [PATCH 39/44] Add docstring for `ICommitComparisonBoxProps` --- src/components/CommitComparisonBox.tsx | 36 ++++++++++++++++++++++++++ 1 file changed, 36 insertions(+) diff --git a/src/components/CommitComparisonBox.tsx b/src/components/CommitComparisonBox.tsx index 42de5f128..e3b7fda6f 100644 --- a/src/components/CommitComparisonBox.tsx +++ b/src/components/CommitComparisonBox.tsx @@ -218,13 +218,49 @@ const CommitComparisonBoxBody: React.VFC = ( * Interface describing ComparisonBox component properties. */ export interface ICommitComparisonBoxProps { + /** + * Is this collapsible? + */ collapsible: boolean; + + /** + * Jupyter App commands registry + */ commands: CommandRegistry; + + /** + * Commit comparison result + */ comparison: Git.ICommitComparison | null; + + /** + * Header text + */ header: string; + + /** + * Extension data model. + */ model: IGitExtension; + + /** + * The application language translator. + */ trans: TranslationBundle; + + /** + * Returns a callback to be invoked on clicking cancel. + */ onCancel?: (event: React.MouseEvent) => void; + + /** + * Returns a callback to be invoked on click to display a file diff. + * + * @param filePath file path + * @param isText indicates whether the file supports displaying a diff + * @param previousFilePath when file has been relocated + * @returns callback + */ onOpenDiff?: ( filePath: string, isText: boolean, From 4ad2774ec36e59e75b99fc21bca1bc4d789db218 Mon Sep 17 00:00:00 2001 From: Dat Quach Date: Tue, 19 Apr 2022 20:56:26 -0700 Subject: [PATCH 40/44] Add a missing `trans` prop for CommitComparisonBox --- src/components/CommitComparisonBox.tsx | 1 + 1 file changed, 1 insertion(+) diff --git a/src/components/CommitComparisonBox.tsx b/src/components/CommitComparisonBox.tsx index e3b7fda6f..c0f890d33 100644 --- a/src/components/CommitComparisonBox.tsx +++ b/src/components/CommitComparisonBox.tsx @@ -283,6 +283,7 @@ export const CommitComparisonBox: React.VFC = ( label={props.header} onCollapseExpand={() => setCollapsed(!collapsed)} onClickCancel={props.onCancel} + trans={props.trans} /> {!collapsed && props.comparison?.changedFiles && ( Date: Tue, 19 Apr 2022 20:57:25 -0700 Subject: [PATCH 41/44] Remove the `linesplit` check --- jupyterlab_git/git.py | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/jupyterlab_git/git.py b/jupyterlab_git/git.py index e9e030f6e..9e27650a8 100644 --- a/jupyterlab_git/git.py +++ b/jupyterlab_git/git.py @@ -636,14 +636,13 @@ async def diff(self, path, previous=None, current=None): line_array = strip_and_split(my_output) for line in line_array: linesplit = line.split() - if len(linesplit) == 3: - result.append( - { - "insertions": linesplit[0], - "deletions": linesplit[1], - "filename": linesplit[2], - } - ) + result.append( + { + "insertions": linesplit[0], + "deletions": linesplit[1], + "filename": linesplit[2], + } + ) return {"code": code, "result": result} async def branch(self, path): From 4ca40090a20d26a0163e0dfdc3702364015484dd Mon Sep 17 00:00:00 2001 From: Dat Quach Date: Tue, 19 Apr 2022 20:59:11 -0700 Subject: [PATCH 42/44] Slight increase of the border width when comparing The left border width for commits being compared is 6px now. --- src/style/PastCommitNode.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/style/PastCommitNode.ts b/src/style/PastCommitNode.ts index df233f4a1..2b81b82c7 100644 --- a/src/style/PastCommitNode.ts +++ b/src/style/PastCommitNode.ts @@ -85,9 +85,9 @@ export const singleFileCommitClass = style({ }); export const referenceCommitNodeClass = style({ - borderLeft: '3px solid var(--jp-git-diff-deleted-color)' + borderLeft: '6px solid var(--jp-git-diff-deleted-color)' }); export const challengerCommitNodeClass = style({ - borderLeft: '3px solid var(--jp-git-diff-added-color)' + borderLeft: '6px solid var(--jp-git-diff-added-color)' }); From 06f11c1f616b38b0737de8ac330e8135e13cc0e0 Mon Sep 17 00:00:00 2001 From: Dat Quach Date: Tue, 19 Apr 2022 21:21:51 -0700 Subject: [PATCH 43/44] Remove commit comparison state redundancy Address the commit comparison state redundancy in `GitPanel`. jupyterlab/jupyterlab-git/pull/1108#discussion_r851098378 --- src/components/CommitComparisonBox.tsx | 22 +++++--- src/components/GitPanel.tsx | 71 ++++++++++++-------------- src/tokens.ts | 19 ------- 3 files changed, 49 insertions(+), 63 deletions(-) diff --git a/src/components/CommitComparisonBox.tsx b/src/components/CommitComparisonBox.tsx index c0f890d33..2bbec46b2 100644 --- a/src/components/CommitComparisonBox.tsx +++ b/src/components/CommitComparisonBox.tsx @@ -224,17 +224,27 @@ export interface ICommitComparisonBoxProps { collapsible: boolean; /** - * Jupyter App commands registry + * Jupyter App commands registry. */ commands: CommandRegistry; /** - * Commit comparison result + * The commit to compare against. */ - comparison: Git.ICommitComparison | null; + referenceCommit: Git.ISingleCommitInfo | null; /** - * Header text + * The commit to compare. + */ + challengerCommit: Git.ISingleCommitInfo | null; + + /** + * The commit comparison result. + */ + changedFiles: Git.ICommitModifiedFile[] | null; + + /** + * Header text. */ header: string; @@ -285,9 +295,9 @@ export const CommitComparisonBox: React.VFC = ( onClickCancel={props.onCancel} trans={props.trans} /> - {!collapsed && props.comparison?.changedFiles && ( + {!collapsed && props.changedFiles && ( { hasDirtyStagedFiles: hasDirtyStagedFiles, referenceCommit: null, challengerCommit: null, - commitComparison: null + comparedFiles: null }; } @@ -238,7 +238,7 @@ export class GitPanel extends React.Component { { referenceCommit: prevReferenceCommit, challengerCommit: prevChallengerCommit, - commitComparison: prevCommitComparison + comparedFiles: prevComparedFiles }: Readonly ): Promise { const { @@ -251,7 +251,7 @@ export class GitPanel extends React.Component { prevReferenceCommit !== currReferenceCommit || prevChallengerCommit !== currChallengerCommit; - if (commitsReady && (!!prevCommitComparison || commitChanged)) { + if (commitsReady && (!!prevComparedFiles || commitChanged)) { await this._doCommitComparsion(); } } @@ -274,7 +274,7 @@ export class GitPanel extends React.Component { currentBranch: currentBranch ? currentBranch.name : 'master', referenceCommit: null, challengerCommit: null, - commitComparison: null + comparedFiles: null }); }; @@ -514,9 +514,11 @@ export class GitPanel extends React.Component { /> {(this.state.referenceCommit || this.state.challengerCommit) && ( { this._setCommitComparisonState({ reference: null, challenger: null, - comparison: null + comparedFiles: null }); }} onOpenDiff={ - this.state.commitComparison + this.state.referenceCommit && this.state.challengerCommit ? openFileDiff(this.props.commands)( - this.state.commitComparison.challenger, - this.state.commitComparison.reference + this.state.challengerCommit, + this.state.referenceCommit ) : undefined } @@ -861,7 +863,7 @@ export class GitPanel extends React.Component { private _setCommitComparisonState(state: { reference?: Git.ISingleCommitInfo; challenger?: Git.ISingleCommitInfo; - comparison?: Git.ICommitComparison; + comparedFiles?: Git.ICommitModifiedFile[]; }): void { this.setState(currentState => ({ referenceCommit: @@ -872,10 +874,10 @@ export class GitPanel extends React.Component { typeof state.challenger !== 'undefined' ? state.challenger : currentState.challengerCommit, - commitComparison: - typeof state.comparison !== 'undefined' - ? state.comparison - : currentState.commitComparison + comparedFiles: + typeof state.comparedFiles !== 'undefined' + ? state.comparedFiles + : currentState.comparedFiles })); } @@ -902,34 +904,27 @@ export class GitPanel extends React.Component { return; } if (diffResult) { - this.setState(state => { - return { - commitComparison: { - reference: state.referenceCommit, - challenger: state.challengerCommit, - changedFiles: diffResult.result.map(changedFile => { - const pathParts = changedFile.filename.split('/'); - const fileName = pathParts[pathParts.length - 1]; - const filePath = changedFile.filename; - return { - deletion: changedFile.deletions, - insertion: changedFile.insertions, - is_binary: - changedFile.deletions === '-' || - changedFile.insertions === '-', - modified_file_name: fileName, - modified_file_path: filePath, - type: changedFile.filetype - } as Git.ICommitModifiedFile; - }) - } as Git.ICommitComparison - }; + this.setState({ + comparedFiles: diffResult.result.map(changedFile => { + const pathParts = changedFile.filename.split('/'); + const fileName = pathParts[pathParts.length - 1]; + const filePath = changedFile.filename; + return { + deletion: changedFile.deletions, + insertion: changedFile.insertions, + is_binary: + changedFile.deletions === '-' || changedFile.insertions === '-', + modified_file_name: fileName, + modified_file_path: filePath, + type: changedFile.filetype + } as Git.ICommitModifiedFile; + }) }); } else { this.setState({ referenceCommit: null, challengerCommit: null, - commitComparison: null + comparedFiles: null }); } } diff --git a/src/tokens.ts b/src/tokens.ts index 80762aaa9..740f34c27 100644 --- a/src/tokens.ts +++ b/src/tokens.ts @@ -1031,25 +1031,6 @@ export namespace Git { toggle(fname: string): void; } - /** - * Interface for the result of comparing two commits - */ - export interface ICommitComparison { - /** - * The commit to compare against - */ - reference: ISingleCommitInfo; - /** - * The commit to compare - */ - challenger: ISingleCommitInfo; - /** - * The list of files that have been modified - * in the compared commits - */ - changedFiles: ICommitModifiedFile[]; - } - export type Status = | 'untracked' | 'staged' From fa2096644a3d10e3a5e15ad0c5cddca8147d9c67 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fr=C3=A9d=C3=A9ric=20Collonval?= Date: Wed, 20 Apr 2022 09:26:36 +0200 Subject: [PATCH 44/44] Apply suggestions from code review --- jupyterlab_git/handlers.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/jupyterlab_git/handlers.py b/jupyterlab_git/handlers.py index efbf73b4c..769f93516 100644 --- a/jupyterlab_git/handlers.py +++ b/jupyterlab_git/handlers.py @@ -255,7 +255,7 @@ class GitDiffHandler(GitHandler): @tornado.web.authenticated async def post(self, path: str = ""): """ - POST request handler, fetches differences between commits & current working + POST request handler, fetches differences between two states tree. """ data = self.get_json_body()