Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Guess default push target #721

Merged
merged 5 commits into from
Aug 22, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
73 changes: 51 additions & 22 deletions jupyterlab_git/git.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,13 +12,15 @@
import tornado.locks
import datetime

from .log import get_logger


# 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\-\.]+)\=")
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
Expand Down Expand Up @@ -99,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:
Expand All @@ -112,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,
Expand All @@ -125,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()

Expand Down Expand Up @@ -156,9 +164,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())
Expand All @@ -180,7 +186,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

Expand Down Expand Up @@ -839,15 +845,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),
Expand All @@ -856,7 +867,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),
)
Expand Down Expand Up @@ -1086,7 +1097,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
Expand All @@ -1097,19 +1108,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
Expand Down
87 changes: 75 additions & 12 deletions jupyterlab_git/handlers.py
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -265,13 +270,14 @@ async def post(self):
class GitRemoteAddHandler(GitHandler):
"""Handler for 'git remote add <name> <url>'."""

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:
Expand Down Expand Up @@ -422,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))


Expand All @@ -436,32 +445,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. <remote_name> or <remote_name>/<branch>
}
"""
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
Comment on lines 508 to 511
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe we should modify this error message to explain to the user why the guessing of the push target failed. Something along the lines of this

Suggested change
response = {
"code": 128,
"message": "fatal: The current branch {} has no upstream branch.".format(
current_local_branch
response = {
"code": 128,
"message": "The current branch {} has no upstream branch. You have {} remotes so a remote cannot be chosen automatically without having set a pushdefault.".format(
current_local_branch,
len(remotes)

Copy link
Member Author

Choose a reason for hiding this comment

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

That error message will be used also for a repository without remote.

So I will not change the error message. But prompt the user to choose from the remote list if it exists. Hence your use case will be covered. What do you think?

Copy link
Collaborator

Choose a reason for hiding this comment

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

But prompt the user to choose from the remote list if it exist

That is the best solution by far. My suggestion was to make it more clear if there were to be no further frontend changes.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Will having the option to prompt the user to choose from a remote list run into the same issues as the Git Credentials w.r.t the new modal?

),
"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))

Expand Down Expand Up @@ -504,8 +557,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)
Expand Down Expand Up @@ -601,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))


Expand All @@ -618,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))


Expand Down
18 changes: 18 additions & 0 deletions jupyterlab_git/log.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
import logging

from traitlets.config import Application


class _ExtensionLogger:
_LOGGER = None # type: Optional[logging.Logger]

@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
2 changes: 1 addition & 1 deletion jupyterlab_git/tests/test_config.py
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Expand Down