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

Conversation

fcollonval
Copy link
Member

Fresh rebase of #412

@fcollonval fcollonval added this to the 0.21.0 milestone Aug 9, 2020
@fcollonval
Copy link
Member Author

/binder

@github-actions
Copy link

github-actions bot commented Aug 9, 2020

Binder 👈 Launch a binder JupyterLab on this branch

@fcollonval
Copy link
Member Author

@ianhi This should fix #725

@fcollonval fcollonval marked this pull request as ready for review August 10, 2020 19:28
@ianhi
Copy link
Collaborator

ianhi commented Aug 10, 2020

I don't think it fixed it for me:
pushing-4

Comment on lines 508 to 511
response = {
"code": 128,
"message": "fatal: The current branch {} has no upstream branch.".format(
current_local_branch
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?

@fcollonval
Copy link
Member Author

@ianhi, do you have multiple remotes? Could you post the push request response for the defunct case?

@ianhi
Copy link
Collaborator

ianhi commented Aug 11, 2020

I only have one remote on that repo

(jlab-git) ian@pop-os:~/Documents/jupyter/git-expt$ git remote -vv
origin  [email protected]:ianhi/git-expt.git (fetch)
origin  [email protected]:ianhi/git-expt.git (push)

I think this is due to a weird interaction between #630 and existing modal for git push/pull here:

export async function showGitOperationDialog(
model: IGitExtension,
operation: Operation
): Promise<void> {
const title = `Git ${operation}`;
let result = await showDialog({
title: title,
body: new GitPullPushDialog(model, operation),
buttons: [Dialog.okButton({ label: 'DISMISS' })]
});
let retry = false;
while (!result.button.accept) {
const credentials = await showDialog({
title: 'Git credentials required',
body: new GitCredentialsForm(
'Enter credentials for remote repository',
retry ? 'Incorrect username or password.' : ''
),
buttons: [Dialog.cancelButton(), Dialog.okButton({ label: 'OK' })]
});
if (!credentials.button.accept) {
break;
}
result = await showDialog({
title: title,
body: new GitPullPushDialog(model, operation, credentials.value),
buttons: [Dialog.okButton({ label: 'DISMISS' })]
});
retry = true;

Adding some console.logs shows that when I click off the dialog the await finishes but result.button.accept remains false. Then that same interaction can continue indefinitely due to the while loop

@ianhi
Copy link
Collaborator

ianhi commented Aug 11, 2020

Ooh actually this predates #630, checking out an arbitrary commit from before that (2f9ad78) I also see this issue.

I think the dialog is being closed due to a jupyterlab problem. See the unmerged jupyterlab/jupyterlab#8438 which stops dialogs from being closed by clicking outside them if they do not have a cancel button.

@ianhi
Copy link
Collaborator

ianhi commented Aug 11, 2020

I think the solution to #725 is to eliminate the old modal and adapt push/pull to use the new modal + toast notifications. But that could be done separately to this PR?

@fcollonval
Copy link
Member Author

I think the solution to #725 is to eliminate the old modal and adapt push/pull to use the new modal + toast notifications. But that could be done separately to this PR?

Thanks @ianhi for the detailed analysis. At the beginning I hoped to use the new model. But unfortunately it is not based on some global function that emits it. Moreover as the action is executed from a command and not a react component, this makes the switch quite complicated. So I try staying on the not robust actual code.

The ideal code word look like:

waitForCall("Pushing...", model.push, args).catch(error => 
if error due to auth then
  Request credentials
  Re try
else if multiple remotes then
  User needs to pick one
  Re try
)

And so this should be independent of the waiting ui component.

@fcollonval
Copy link
Member Author

As the bug is related to JupyterLab modal handling, it is already present in 0.20. I'll merge this to make the long due release. And we can release the fix when available as release is now easier.

@fcollonval fcollonval merged commit a7e5707 into jupyterlab:master Aug 22, 2020
@fcollonval
Copy link
Member Author

@meeseeksdev backport to 0.11.x

@fcollonval fcollonval deleted the default-push branch August 22, 2020 16:26
meeseeksmachine pushed a commit to meeseeksmachine/jupyterlab-git that referenced this pull request Aug 22, 2020
fcollonval added a commit that referenced this pull request Aug 22, 2020
…on-0.11.x

Backport PR #721 on branch 0.11.x (Guess default push target)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants