From df33f546af4ff4df4a472291c9cb86930d62a381 Mon Sep 17 00:00:00 2001 From: Frederic Collonval Date: Sun, 9 Aug 2020 17:07:45 +0200 Subject: [PATCH 1/5] Fresh rebase of #412 --- jupyterlab_git/git.py | 61 +++++---- jupyterlab_git/handlers.py | 78 ++++++++++-- jupyterlab_git/tests/test_config.py | 2 +- jupyterlab_git/tests/test_handlers.py | 175 ++++++++++++++++++++++++-- jupyterlab_git/tests/test_pushpull.py | 2 +- jupyterlab_git/tests/test_remote.py | 68 +++------- 6 files changed, 294 insertions(+), 92 deletions(-) diff --git a/jupyterlab_git/git.py b/jupyterlab_git/git.py index bf0892866..ba4a5d2e9 100644 --- a/jupyterlab_git/git.py +++ b/jupyterlab_git/git.py @@ -13,8 +13,6 @@ import datetime -# Git configuration options exposed through the REST API -ALLOWED_OPTIONS = ['user.name', 'user.email'] # Regex pattern to capture (key, value) of Git configuration options. # See https://git-scm.com/docs/git-config#_syntax for git var syntax CONFIG_PATTERN = re.compile(r"(?:^|\n)([\w\-\.]+)\=") @@ -156,9 +154,7 @@ async def config(self, top_repo_path, **kwargs): if len(kwargs): output = [] - for k, v in filter( - lambda t: True if t[0] in ALLOWED_OPTIONS else False, kwargs.items() - ): + for k, v in kwargs.items(): cmd = ["git", "config", "--add", k, v] code, out, err = await execute(cmd, cwd=top_repo_path) output.append(out.strip()) @@ -180,7 +176,7 @@ async def config(self, top_repo_path, **kwargs): else: raw = output.strip() s = CONFIG_PATTERN.split(raw) - response["options"] = {k:v for k, v in zip(s[1::2], s[2::2]) if k in ALLOWED_OPTIONS} + response["options"] = {k:v for k, v in zip(s[1::2], s[2::2])} return response @@ -839,15 +835,20 @@ async def pull(self, curr_fb_path, auth=None, cancel_on_conflict=False): return response - async def push(self, remote, branch, curr_fb_path, auth=None): + async def push(self, remote, branch, curr_fb_path, auth=None, set_upstream=False): """ Execute `git push $UPSTREAM $BRANCH`. The choice of upstream and branch is up to the caller. - """ + """ + command = ["git", "push"] + if set_upstream: + command.append("--set-upstream") + command.extend([remote, branch]) + env = os.environ.copy() if auth: env["GIT_TERMINAL_PROMPT"] = "1" code, _, error = await execute( - ["git", "push", remote, branch], + command, username=auth["username"], password=auth["password"], cwd=os.path.join(self.root_dir, curr_fb_path), @@ -856,7 +857,7 @@ async def push(self, remote, branch, curr_fb_path, auth=None): else: env["GIT_TERMINAL_PROMPT"] = "0" code, _, error = await execute( - ["git", "push", remote, branch], + command, env=env, cwd=os.path.join(self.root_dir, curr_fb_path), ) @@ -1086,7 +1087,7 @@ async def _is_binary(self, filename, ref, top_repo_path): # For binary files, `--numstat` outputs two `-` characters separated by TABs: return output.startswith('-\t-\t') - def remote_add(self, top_repo_path, url, name=DEFAULT_REMOTE_NAME): + async def remote_add(self, top_repo_path, url, name=DEFAULT_REMOTE_NAME): """Handle call to `git remote add` command. top_repo_path: str @@ -1097,19 +1098,37 @@ def remote_add(self, top_repo_path, url, name=DEFAULT_REMOTE_NAME): Remote name; default "origin" """ cmd = ["git", "remote", "add", name, url] - p = subprocess.Popen(cmd, stdout=subprocess.PIPE, stderr=subprocess.PIPE, cwd=top_repo_path) - _, my_error = p.communicate() - if p.returncode == 0: - return { - "code": p.returncode, + code, _, error = await execute(cmd, cwd=top_repo_path) + response = { + "code": code, "command": " ".join(cmd) } - else: - return { - "code": p.returncode, - "command": " ".join(cmd), - "message": my_error.decode("utf-8").strip() + + if code != 0: + response["message"] = error + + return response + + async def remote_show(self, path): + """Handle call to `git remote show` command. + Args: + path (str): Git repository path + + Returns: + List[str]: Known remotes + """ + command = ["git", "remote", "show"] + code, output, error = await execute(command, cwd=path) + response = { + "code": code, + "command": " ".join(command) } + if code == 0: + response["remotes"] = [r.strip() for r in output.splitlines()] + else: + response["message"] = error + + return response async def ensure_gitignore(self, top_repo_path): """Handle call to ensure .gitignore file exists and the diff --git a/jupyterlab_git/handlers.py b/jupyterlab_git/handlers.py index 22bada74d..1a3251cea 100644 --- a/jupyterlab_git/handlers.py +++ b/jupyterlab_git/handlers.py @@ -13,6 +13,11 @@ from ._version import __version__ from .git import DEFAULT_REMOTE_NAME + +# Git configuration options exposed through the REST API +ALLOWED_OPTIONS = ['user.name', 'user.email'] + + class GitHandler(APIHandler): """ Top-level parent class. @@ -265,13 +270,14 @@ async def post(self): class GitRemoteAddHandler(GitHandler): """Handler for 'git remote add '.""" - def post(self): + @web.authenticated + async def post(self): """POST request handler to add a remote.""" data = self.get_json_body() top_repo_path = data["top_repo_path"] name = data.get("name", DEFAULT_REMOTE_NAME) url = data["url"] - output = self.git.remote_add(top_repo_path, url, name) + output = await self.git.remote_add(top_repo_path, url, name) if(output["code"] == 0): self.set_status(201) else: @@ -436,32 +442,76 @@ async def post(self): """ POST request handler, pushes committed files from your current branch to a remote branch + + Request body: + { + current_path: string, # Git repository path + remote?: string # Remote to push to; i.e. or / + } """ data = self.get_json_body() current_path = data["current_path"] + known_remote = data.get("remote") current_local_branch = await self.git.get_current_branch(current_path) - upstream = await self.git.get_upstream_branch( + + set_upstream = False + current_upstream_branch = await self.git.get_upstream_branch( current_path, current_local_branch ) - if upstream['code'] == 0: - branch = ":".join(["HEAD", upstream['remote_branch']]) + if known_remote is not None: + set_upstream = current_upstream_branch['code'] != 0 + + remote_name, _, remote_branch = known_remote.partition("/") + + current_upstream_branch = { + "code": 0, + "remote_branch": remote_branch or current_local_branch, + "remote_short_name": remote_name + } + + if current_upstream_branch['code'] == 0: + branch = ":".join(["HEAD", current_upstream_branch['remote_branch']]) response = await self.git.push( - upstream['remote_short_name'], branch, current_path, data.get("auth", None) + current_upstream_branch['remote_short_name'], branch, current_path, data.get("auth", None), set_upstream ) else: - if ("no upstream configured for branch" in upstream['message'].lower() - or 'unknown revision or path' in upstream['message'].lower()): + # Allow users to specify upstream through their configuration + # https://git-scm.com/docs/git-config#Documentation/git-config.txt-pushdefault + # Or use the remote defined if only one remote exists + config = await self.git.config(current_path) + config_options = config["options"] + list_remotes = await self.git.remote_show(current_path) + remotes = list_remotes.get("remotes", list()) + push_default = config_options.get('remote.pushdefault') + + default_remote = None + if push_default is not None and push_default in remotes: + default_remote = push_default + elif len(remotes) == 1: + default_remote = remotes[0] + + if default_remote is not None: + response = await self.git.push( + default_remote, + current_local_branch, + current_path, + data.get("auth", None), + set_upstream=True, + ) + else: response = { "code": 128, "message": "fatal: The current branch {} has no upstream branch.".format( current_local_branch ), + "remotes": remotes # Returns the list of known remotes } - else: - self.set_status(500) + + if response["code"] != 0: + self.set_status(500) self.finish(json.dumps(response)) @@ -504,8 +554,12 @@ async def post(self): """ data = self.get_json_body() top_repo_path = data["path"] - options = data.get("options", {}) - response = await self.git.config(top_repo_path, **options) + options = data.get("options", {}) + + filtered_options = {k: v for k, v in options.items() if k in ALLOWED_OPTIONS} + response = await self.git.config(top_repo_path, **filtered_options) + if "options" in response: + response["options"] = {k:v for k, v in response["options"].items() if k in ALLOWED_OPTIONS} if response["code"] != 0: self.set_status(500) diff --git a/jupyterlab_git/tests/test_config.py b/jupyterlab_git/tests/test_config.py index ceb6f2f40..313285636 100644 --- a/jupyterlab_git/tests/test_config.py +++ b/jupyterlab_git/tests/test_config.py @@ -75,7 +75,7 @@ def test_git_get_config_multiline(self, mock_execute): @patch("jupyterlab_git.git.execute") @patch( - "jupyterlab_git.git.ALLOWED_OPTIONS", + "jupyterlab_git.handlers.ALLOWED_OPTIONS", ["alias.summary", "alias.topic-base-branch-name"], ) def test_git_get_config_accepted_multiline(self, mock_execute): diff --git a/jupyterlab_git/tests/test_handlers.py b/jupyterlab_git/tests/test_handlers.py index f1739941b..10e836f41 100644 --- a/jupyterlab_git/tests/test_handlers.py +++ b/jupyterlab_git/tests/test_handlers.py @@ -2,6 +2,7 @@ import os from unittest.mock import ANY, Mock, call, patch +import requests import tornado from jupyterlab_git.git import Git @@ -183,7 +184,7 @@ def test_push_handler_localbranch(self, mock_git): # Then mock_git.get_current_branch.assert_called_with("test_path") mock_git.get_upstream_branch.assert_called_with("test_path", "localbranch") - mock_git.push.assert_called_with(".", "HEAD:localbranch", "test_path", None) + mock_git.push.assert_called_with(".", "HEAD:localbranch", "test_path", None, False) assert response.status_code == 200 payload = response.json() @@ -207,7 +208,7 @@ def test_push_handler_remotebranch(self, mock_git): mock_git.get_current_branch.assert_called_with("test_path") mock_git.get_upstream_branch.assert_called_with("test_path", "foo/bar") mock_git.push.assert_called_with( - "origin/something", "HEAD:remote-branch-name", "test_path", None + "origin/something", "HEAD:remote-branch-name", "test_path", None, False ) assert response.status_code == 200 @@ -224,24 +225,184 @@ def test_push_handler_noupstream(self, mock_git): "message": "fatal: no upstream configured for branch 'foo'" } mock_git.get_upstream_branch.return_value = maybe_future(upstream) + mock_git.config.return_value = maybe_future({"options": dict()}) + mock_git.remote_show.return_value = maybe_future({}) mock_git.push.return_value = maybe_future({"code": 0}) + path = "test_path" + # When - body = {"current_path": "test_path"} - response = self.tester.post(["push"], body=body) + body = {"current_path": path} + try: + self.tester.post(["push"], body=body) + except requests.exceptions.HTTPError as error: + response = error.response # Then - mock_git.get_current_branch.assert_called_with("test_path") - mock_git.get_upstream_branch.assert_called_with("test_path", "foo") + mock_git.get_current_branch.assert_called_with(path) + mock_git.get_upstream_branch.assert_called_with(path, "foo") + mock_git.config.assert_called_with(path) + mock_git.remote_show.assert_called_with(path) mock_git.push.assert_not_called() - assert response.status_code == 200 + assert response.status_code == 500 + payload = response.json() + assert payload == { + "code": 128, + "message": "fatal: The current branch foo has no upstream branch.", + "remotes": list() + } + + @patch("jupyterlab_git.handlers.GitPushHandler.git", spec=Git) + def test_push_handler_multipleupstream(self, mock_git): + # Given + remotes = ["origin", "upstream"] + mock_git.get_current_branch.return_value = maybe_future("foo") + upstream = {"code": -1, "message": "oups"} + mock_git.get_upstream_branch.return_value = maybe_future(upstream) + mock_git.config.return_value = maybe_future({"options": dict()}) + mock_git.remote_show.return_value = maybe_future({"remotes": remotes}) + mock_git.push.return_value = maybe_future({"code": 0}) + + path = "test_path" + + # When + body = {"current_path": path} + try: + self.tester.post(["push"], body=body) + except requests.exceptions.HTTPError as error: + response = error.response + + # Then + mock_git.get_current_branch.assert_called_with(path) + mock_git.get_upstream_branch.assert_called_with(path, "foo") + mock_git.config.assert_called_with(path) + mock_git.remote_show.assert_called_with(path) + mock_git.push.assert_not_called() + + assert response.status_code == 500 payload = response.json() assert payload == { "code": 128, "message": "fatal: The current branch foo has no upstream branch.", + "remotes": remotes } + @patch("jupyterlab_git.handlers.GitPushHandler.git", spec=Git) + def test_push_handler_noupstream_unique_remote(self, mock_git): + # Given + remote = "origin" + mock_git.get_current_branch.return_value = maybe_future("foo") + upstream = {"code": -1, "message": "oups"} + mock_git.get_upstream_branch.return_value = maybe_future(upstream) + mock_git.config.return_value = maybe_future({"options": dict()}) + mock_git.remote_show.return_value = maybe_future({"remotes": [remote]}) + mock_git.push.return_value = maybe_future({"code": 0}) + + path = "test_path" + + # When + body = {"current_path": path} + response = self.tester.post(["push"], body=body) + + # Then + mock_git.get_current_branch.assert_called_with(path) + mock_git.get_upstream_branch.assert_called_with(path, "foo") + mock_git.config.assert_called_with(path) + mock_git.remote_show.assert_called_with(path) + mock_git.push.assert_called_with(remote, "foo", "test_path", None, set_upstream=True) + + assert response.status_code == 200 + payload = response.json() + assert payload == {"code": 0} + + @patch("jupyterlab_git.handlers.GitPushHandler.git", spec=Git) + def test_push_handler_noupstream_pushdefault(self, mock_git): + # Given + remote = "rorigin" + mock_git.get_current_branch.return_value = maybe_future("foo") + upstream = {"code": -1, "message": "oups"} + mock_git.get_upstream_branch.return_value = maybe_future(upstream) + mock_git.config.return_value = maybe_future({"options": {"remote.pushdefault": remote}}) + mock_git.remote_show.return_value = maybe_future({"remotes": [remote, "upstream"]}) + mock_git.push.return_value = maybe_future({"code": 0}) + + path = "test_path" + + # When + body = {"current_path": path} + response = self.tester.post(["push"], body=body) + + # Then + mock_git.get_current_branch.assert_called_with(path) + mock_git.get_upstream_branch.assert_called_with(path, "foo") + mock_git.config.assert_called_with(path) + mock_git.remote_show.assert_called_with(path) + mock_git.push.assert_called_with(remote, "foo", "test_path", None, set_upstream=True) + + assert response.status_code == 200 + payload = response.json() + assert payload == {"code": 0} + + @patch("jupyterlab_git.handlers.GitPushHandler.git", spec=Git) + def test_push_handler_noupstream_pass_remote_nobranch(self, mock_git): + # Given + mock_git.get_current_branch.return_value = maybe_future("foo") + upstream = {"code": -1, "message": "oups"} + mock_git.get_upstream_branch.return_value = maybe_future(upstream) + mock_git.config.return_value = maybe_future({"options": dict()}) + mock_git.remote_show.return_value = maybe_future({}) + mock_git.push.return_value = maybe_future({"code": 0}) + + path = "test_path" + remote = "online" + + # When + body = {"current_path": path, "remote": remote} + response = self.tester.post(["push"], body=body) + + # Then + mock_git.get_current_branch.assert_called_with(path) + mock_git.get_upstream_branch.assert_called_with(path, "foo") + mock_git.config.assert_not_called() + mock_git.remote_show.assert_not_called() + mock_git.push.assert_called_with(remote, "HEAD:foo", "test_path", None, True) + + assert response.status_code == 200 + payload = response.json() + assert payload == {"code": 0} + + @patch("jupyterlab_git.handlers.GitPushHandler.git", spec=Git) + def test_push_handler_noupstream_pass_remote_branch(self, mock_git): + # Given + mock_git.get_current_branch.return_value = maybe_future("foo") + upstream = {"code": -1, "message": "oups"} + mock_git.get_upstream_branch.return_value = maybe_future(upstream) + mock_git.config.return_value = maybe_future({"options": dict()}) + mock_git.remote_show.return_value = maybe_future({}) + mock_git.push.return_value = maybe_future({"code": 0}) + + path = "test_path" + remote = "online" + remote_branch = "onfoo" + + # When + body = {"current_path": path, "remote": "/".join((remote, remote_branch))} + response = self.tester.post(["push"], body=body) + + # Then + mock_git.get_current_branch.assert_called_with(path) + mock_git.get_upstream_branch.assert_called_with(path, "foo") + mock_git.config.assert_not_called() + mock_git.remote_show.assert_not_called() + mock_git.push.assert_called_with( + remote, "HEAD:" + remote_branch, "test_path", None, True + ) + + assert response.status_code == 200 + payload = response.json() + assert payload == {"code": 0} + class TestUpstream(ServerTest): @patch("jupyterlab_git.handlers.GitUpstreamHandler.git", spec=Git) diff --git a/jupyterlab_git/tests/test_pushpull.py b/jupyterlab_git/tests/test_pushpull.py index 87fdafc3c..d8628bb78 100644 --- a/jupyterlab_git/tests/test_pushpull.py +++ b/jupyterlab_git/tests/test_pushpull.py @@ -49,7 +49,7 @@ async def test_git_pull_with_conflict_fail(): cwd="/bin/test_curr_path", env={"TEST": "test", "GIT_TERMINAL_PROMPT": "0"}, ) - ]); + ]) assert {"code": 1, "message": "Automatic merge failed; fix conflicts and then commit the result."} == actual_response diff --git a/jupyterlab_git/tests/test_remote.py b/jupyterlab_git/tests/test_remote.py index 29020c5ee..76c52c0aa 100644 --- a/jupyterlab_git/tests/test_remote.py +++ b/jupyterlab_git/tests/test_remote.py @@ -3,22 +3,16 @@ from jupyterlab_git.handlers import GitRemoteAddHandler -from .testutils import assert_http_error, ServerTest +from .testutils import assert_http_error, ServerTest, maybe_future class TestAddRemote(ServerTest): - @patch("subprocess.Popen") - def test_git_add_remote_success_no_name(self, popen): + @patch("jupyterlab_git.git.execute") + def test_git_add_remote_success_no_name(self, mock_execute): # Given path = "test_path" url = "http://github.com/myid/myrepository.git" - process_mock = Mock() - attrs = { - "communicate": Mock(return_value=(b"", b"",)), - "returncode": 0, - } - process_mock.configure_mock(**attrs) - popen.return_value = process_mock + mock_execute.return_value = maybe_future((0, "", "")) # When body = { @@ -28,69 +22,47 @@ def test_git_add_remote_success_no_name(self, popen): response = self.tester.post(["remote", "add"], body=body) # Then - popen.assert_called_once_with( - ["git", "remote", "add", "origin", url], - stdout=subprocess.PIPE, - stderr=subprocess.PIPE, - cwd=path, - ) - process_mock.communicate.assert_called_once_with() + command = ["git", "remote", "add", "origin", url] + mock_execute.assert_called_once_with(command, cwd=path) assert response.status_code == 201 payload = response.json() assert payload == { "code": 0, - "command": " ".join(["git", "remote", "add", "origin", url]), + "command": " ".join(command), } - @patch("subprocess.Popen") - def test_git_add_remote_success(self, popen): + @patch("jupyterlab_git.git.execute") + def test_git_add_remote_success(self, mock_execute): # Given path = "test_path" url = "http://github.com/myid/myrepository.git" name = "distant" - process_mock = Mock() - attrs = { - "communicate": Mock(return_value=(b"", b"",)), - "returncode": 0, - } - process_mock.configure_mock(**attrs) - popen.return_value = process_mock + mock_execute.return_value = maybe_future((0, "", "")) # When body = {"top_repo_path": path, "url": url, "name": name} response = self.tester.post(["remote", "add"], body=body) # Then - popen.assert_called_once_with( - ["git", "remote", "add", name, url], - stdout=subprocess.PIPE, - stderr=subprocess.PIPE, - cwd=path, - ) - process_mock.communicate.assert_called_once_with() + command = ["git", "remote", "add", name, url] + mock_execute.assert_called_once_with(command, cwd=path) assert response.status_code == 201 payload = response.json() assert payload == { "code": 0, - "command": " ".join(["git", "remote", "add", name, url]), + "command": " ".join(command), } - @patch("subprocess.Popen") - def test_git_add_remote_failure(self, popen): + @patch("jupyterlab_git.git.execute") + def test_git_add_remote_failure(self, mock_execute): # Given path = "test_path" url = "http://github.com/myid/myrepository.git" - process_mock = Mock() error_msg = "Fake failure" error_code = 128 - attrs = { - "communicate": Mock(return_value=(b"", bytes(error_msg, encoding="utf-8"))), - "returncode": error_code, - } - process_mock.configure_mock(**attrs) - popen.return_value = process_mock + mock_execute.return_value = maybe_future((error_code, "", error_msg)) # When body = { @@ -101,10 +73,6 @@ def test_git_add_remote_failure(self, popen): self.tester.post(["remote", "add"], body=body) # Then - popen.assert_called_once_with( - ["git", "remote", "add", "origin", url], - stdout=subprocess.PIPE, - stderr=subprocess.PIPE, - cwd=path, + mock_execute.assert_called_once_with( + ["git", "remote", "add", "origin", url], cwd=path ) - process_mock.communicate.assert_called_once_with() From 25ede4d1fa27afd3e5f56163a73b8b2278175edc Mon Sep 17 00:00:00 2001 From: Frederic Collonval Date: Mon, 10 Aug 2020 21:20:12 +0200 Subject: [PATCH 2/5] Add Python logger --- jupyterlab_git/git.py | 12 +++++++++++- jupyterlab_git/log.py | 19 +++++++++++++++++++ 2 files changed, 30 insertions(+), 1 deletion(-) create mode 100644 jupyterlab_git/log.py diff --git a/jupyterlab_git/git.py b/jupyterlab_git/git.py index ba4a5d2e9..65e0fd19c 100644 --- a/jupyterlab_git/git.py +++ b/jupyterlab_git/git.py @@ -12,11 +12,15 @@ import tornado.locks import datetime +from .log import get_logger + # Regex pattern to capture (key, value) of Git configuration options. # See https://git-scm.com/docs/git-config#_syntax for git var syntax CONFIG_PATTERN = re.compile(r"(?:^|\n)([\w\-\.]+)\=") DEFAULT_REMOTE_NAME = "origin" +# Maximum number of character of command output to print in debug log +MAX_LOG_OUTPUT = 500 # type: int # How long to wait to be executed or finished your execution before timing out MAX_WAIT_FOR_EXECUTE_S = 20 # Ensure on NFS or similar, that we give the .git/index.lock time to be removed @@ -97,7 +101,7 @@ def call_subprocess( try: await execution_lock.acquire(timeout=datetime.timedelta(seconds=MAX_WAIT_FOR_EXECUTE_S)) - except tornado.util.TimeoutError: + except tornado.util.TimeoutError: return (1, "", "Unable to get the lock on the directory") try: @@ -110,6 +114,7 @@ def call_subprocess( # If the lock still exists at this point, we will likely fail anyway, but let's try anyway + get_logger().debug("Execute {!s} in {!s}.".format(cmdline, cwd)) if username is not None and password is not None: code, output, error = await call_subprocess_with_authentication( cmdline, @@ -123,6 +128,11 @@ def call_subprocess( code, output, error = await current_loop.run_in_executor( None, call_subprocess, cmdline, cwd, env ) + log_output = output[:MAX_LOG_OUTPUT] + "..." if len(output) > MAX_LOG_OUTPUT else output + log_error = error[:MAX_LOG_OUTPUT] + "..." if len(error) > MAX_LOG_OUTPUT else error + get_logger().debug("Code: {}\nOutput: {}\nError: {}".format(code, log_output, log_error)) + except BaseException: + get_logger().warning("Fail to execute {!s}".format(cmdline), exc_info=True) finally: execution_lock.release() diff --git a/jupyterlab_git/log.py b/jupyterlab_git/log.py new file mode 100644 index 000000000..50434bfe7 --- /dev/null +++ b/jupyterlab_git/log.py @@ -0,0 +1,19 @@ +import logging +from typing import Optional + +from traitlets.config import Application + + +class _ExtensionLogger: + _LOGGER: Optional[logging.Logger] = None + + @classmethod + def get_logger(cls) -> logging.Logger: + if cls._LOGGER is None: + app = Application.instance() + cls._LOGGER = logging.getLogger("{!s}.jupyterlab_git".format(app.log.name)) + + return cls._LOGGER + + +get_logger = _ExtensionLogger.get_logger From d14ee74f7630ed2c389183a840b4243d780a93d3 Mon Sep 17 00:00:00 2001 From: Frederic Collonval Date: Mon, 10 Aug 2020 21:25:21 +0200 Subject: [PATCH 3/5] Correct api response handling for push/pull/tag --- jupyterlab_git/handlers.py | 9 +++++++++ src/widgets/TagList.ts | 10 +++++++--- src/widgets/gitPushPull.ts | 29 +++++++++++++++-------------- 3 files changed, 31 insertions(+), 17 deletions(-) diff --git a/jupyterlab_git/handlers.py b/jupyterlab_git/handlers.py index 1a3251cea..690050970 100644 --- a/jupyterlab_git/handlers.py +++ b/jupyterlab_git/handlers.py @@ -428,6 +428,9 @@ async def post(self): data = self.get_json_body() response = await self.git.pull(data["current_path"], data.get("auth", None), data.get("cancel_on_conflict", False)) + if response["code"] != 0: + self.set_status(500) + self.finish(json.dumps(response)) @@ -655,6 +658,9 @@ async def post(self): """ current_path = self.get_json_body()["current_path"] result = await self.git.tags(current_path) + + if result["code"] != 0: + self.set_status(500) self.finish(json.dumps(result)) @@ -672,6 +678,9 @@ async def post(self): current_path = data["current_path"] tag = data["tag_id"] result = await self.git.tag_checkout(current_path, tag) + + if result["code"] != 0: + self.set_status(500) self.finish(json.dumps(result)) diff --git a/src/widgets/TagList.ts b/src/widgets/TagList.ts index 21983cd79..9daf5f168 100644 --- a/src/widgets/TagList.ts +++ b/src/widgets/TagList.ts @@ -30,7 +30,7 @@ export class GitTagDialog extends Widget { .then(response => { this._handleResponse(response); }) - .catch(() => this._handleError()); + .catch(error => this._handleError(error.message)); } /** @@ -40,8 +40,6 @@ export class GitTagDialog extends Widget { * @param response the response from the server API call */ private async _handleResponse(response: Git.ITagResult) { - this.node.removeChild(this._spinner.node); - this._spinner.dispose(); if (response.code === 0) { this._handleSuccess(response); } else { @@ -57,6 +55,9 @@ export class GitTagDialog extends Widget { private _handleError( message = 'Unexpected failure. Please check your Jupyter server logs for more details.' ): void { + this.node.removeChild(this._spinner.node); + this._spinner.dispose(); + const label = document.createElement('label'); const text = document.createElement('span'); text.textContent = 'Tag list fetch failed with error:'; @@ -78,6 +79,9 @@ export class GitTagDialog extends Widget { * @param response Git REST API response */ private _handleSuccess(response: Git.ITagResult): void { + this.node.removeChild(this._spinner.node); + this._spinner.dispose(); + const label = document.createElement('label'); const text = document.createElement('span'); text.textContent = 'Select the tag to checkout : '; diff --git a/src/widgets/gitPushPull.ts b/src/widgets/gitPushPull.ts index 50c0dd021..77c6db8a0 100644 --- a/src/widgets/gitPushPull.ts +++ b/src/widgets/gitPushPull.ts @@ -46,7 +46,7 @@ export class GitPullPushDialog extends Widget { .then(response => { this.handleResponse(response); }) - .catch(() => this.handleError()); + .catch(error => this.handleError(error.message)); break; case Operation.Push: this._model @@ -54,7 +54,7 @@ export class GitPullPushDialog extends Widget { .then(response => { this.handleResponse(response); }) - .catch(() => this.handleError()); + .catch(error => this.handleError(error.message)); break; default: throw new Error(`Invalid _operation type: ${this._operation}`); @@ -67,19 +67,8 @@ export class GitPullPushDialog extends Widget { * @param response the response from the server API call */ private async handleResponse(response: Git.IPushPullResult) { - this.node.removeChild(this._spinner.node); - this._spinner.dispose(); if (response.code !== 0) { - if ( - AUTH_ERROR_MESSAGES.map( - message => response.message.indexOf(message) > -1 - ).indexOf(true) > -1 - ) { - this.handleError(response.message); - this.parent!.parent!.close(); // eslint-disable-line @typescript-eslint/no-non-null-assertion - } else { - this.handleError(response.message); - } + this.handleError(response.message); } else { this.handleSuccess(); } @@ -88,6 +77,16 @@ export class GitPullPushDialog extends Widget { private handleError( message = 'Unexpected failure. Please check your Jupyter server logs for more details.' ): void { + if ( + AUTH_ERROR_MESSAGES.map( + errorMessage => message.indexOf(errorMessage) > -1 + ).indexOf(true) > -1 + ) { + this.parent!.parent!.close(); // eslint-disable-line @typescript-eslint/no-non-null-assertion + } + + this.node.removeChild(this._spinner.node); + this._spinner.dispose(); const label = document.createElement('label'); const text = document.createElement('span'); text.textContent = `Git ${this._operation} failed with error:`; @@ -104,6 +103,8 @@ export class GitPullPushDialog extends Widget { } private handleSuccess(): void { + this.node.removeChild(this._spinner.node); + this._spinner.dispose(); const label = document.createElement('label'); const text = document.createElement('span'); text.textContent = `Git ${this._operation} completed successfully`; From 9f45a22ea46d13f0fb5f1b2c3ecdf7d2b09a5995 Mon Sep 17 00:00:00 2001 From: Frederic Collonval Date: Mon, 10 Aug 2020 21:26:28 +0200 Subject: [PATCH 4/5] Reverse tag order to put newer on top --- src/widgets/TagList.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/widgets/TagList.ts b/src/widgets/TagList.ts index 9daf5f168..89e25b8f0 100644 --- a/src/widgets/TagList.ts +++ b/src/widgets/TagList.ts @@ -85,7 +85,8 @@ export class GitTagDialog extends Widget { const label = document.createElement('label'); const text = document.createElement('span'); text.textContent = 'Select the tag to checkout : '; - const tags = response.tags; + // Reverse the tag list to get the more recent on top + const tags = response.tags.reverse(); this._list = document.createElement('select'); tags.forEach(tag => { if (tag) { From 232f5757180dfbc6424af8746bae7c1bee892233 Mon Sep 17 00:00:00 2001 From: Frederic Collonval Date: Mon, 10 Aug 2020 21:34:54 +0200 Subject: [PATCH 5/5] Fix Python 3.5 error --- jupyterlab_git/log.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/jupyterlab_git/log.py b/jupyterlab_git/log.py index 50434bfe7..a82333cbd 100644 --- a/jupyterlab_git/log.py +++ b/jupyterlab_git/log.py @@ -1,11 +1,10 @@ import logging -from typing import Optional from traitlets.config import Application class _ExtensionLogger: - _LOGGER: Optional[logging.Logger] = None + _LOGGER = None # type: Optional[logging.Logger] @classmethod def get_logger(cls) -> logging.Logger: