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

Push Remote Branch Based on Git Config #412

Closed
wants to merge 8 commits into from

Conversation

jhamet93
Copy link
Contributor

Currently, a user needs to set the upstream tracking branch explicitly before being allowed to push. However, there exist configuration options that can be set in the git configuration that automatically infers the remote branch to push to.

This change takes those options into account to not fail git push if the configuration options are present. This helps open up the workflow for less experienced git users to be able to perform the actions in their workflow strictly via the extension with no intervention needed in the CLI.

@fcollonval
Copy link
Member

@jhamet93 thx for working on this. Could you please add unit tests for this?

Josh Hamet and others added 3 commits May 26, 2020 16:44
@fcollonval fcollonval force-pushed the Push_Remote_From_Config branch from 9fe18af to cce0506 Compare May 26, 2020 17:09
@fcollonval
Copy link
Member

I rebooted this PR with the following behavior:

If the branch has no upstream:

  • Use the remote.pushdefault to determine on which remote to push
  • Else if there is only one remote defined, use it
  • Else if there are multiple or no remotes, raises an error

Some side actions:

  • Adding remote was not asynchronous => this corrects it
  • The configuration parameters are filtered in the handler and not in the Git object. So that the server can internally request parameters that are not visible by the frontend.
  • If the remote can not be determined, the list of remotes is returned with the error (empty list if no remote, or list of remotes) => this will allow to prompt the user to pick one up or to tell him to add a remote (following PR once Provide UI feedback during Git command execution #630 is merged as it has implication on the handling of push request in the frontend).

Returns:
List[str]: Known remotes
"""
command = ["git", "remote", "show"]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does it make sense to instead use git remote -v show here? As some future proofing of this function in case it is ever used to determine URLs or do something with determining fetch vs push urls?
For me git remote show:

origin
upstream

vs git remote -v show:

origin	[email protected]:ianhi/jupyterlab-git.git (fetch)
origin	[email protected]:ianhi/jupyterlab-git.git (push)
upstream	[email protected]:jupyterlab/jupyterlab-git.git (fetch)
upstream	[email protected]:jupyterlab/jupyterlab-git.git (push)

Copy link
Member

Choose a reason for hiding this comment

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

For now I'll stick to current behavior as there is no feature requiring more information.

@fcollonval
Copy link
Member

Thanks @ianhi for looking into this

@ianhi
Copy link
Collaborator

ianhi commented Jul 10, 2020

@fcollonval if I were to more fully review this could that qualify as adequate for merging? This is a great change that I would love to see in the extension.

FWIW I currently seem to have achieved stronger permissions by being added to jupyterlab-triage:
image
but I don't know if that was intended to give me those powers.

@fcollonval
Copy link
Member

Hey @ianhi thank you for your time. Yes definitely if you could review this, that will be great.

@fcollonval fcollonval added this to the 0.21.0 milestone Jul 10, 2020
@ianhi
Copy link
Collaborator

ianhi commented Jul 12, 2020

Yes definitely if you could review this, that will be great.

Are there any guidelines for what is involved in doing that? I certainly should check to make sure it does not break anything or doing anything terribly misguided - but beyond that anything specific?

@fcollonval
Copy link
Member

Are there any guidelines for what is involved in doing that? I certainly should check to make sure it does not break anything or doing anything terribly misguided - but beyond that anything specific?

I usually do the following:

  • Check that the JupyterLab code format rules is followed.
  • check that the code is understandable
  • check locally it works as much as possible
  • suggest code simplification if needed

Copy link
Collaborator

@ianhi ianhi left a comment

Choose a reason for hiding this comment

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

@fcollonval I think that there are non-trivial conflicts following the merge of #691 which also affected the usage of the remote determining commands. Are you able to fix them? If not I can as I definitely feel a little guilty for having caused of these conflicts...

Comment on lines +514 to +517
# TODO: once https://github.com/jupyterlab/jupyterlab-git/pull/630 is merged
# because this has implication on credentials request in the frontend
# if response["code"] != 0:
# self.set_status(500)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Bump of this now that #630 was merged

@liamraemclauchlan
Copy link

Would love this to be approved and merged 👍

fcollonval added a commit to fcollonval/jupyterlab-git that referenced this pull request Aug 9, 2020
fcollonval added a commit that referenced this pull request Aug 22, 2020
* Fresh rebase of #412

* Add Python logger

* Correct api response handling for push/pull/tag

* Reverse tag order to put newer on top

* Fix Python 3.5 error
@fcollonval
Copy link
Member

Superseeded by #721

@fcollonval fcollonval closed this Aug 22, 2020
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.

4 participants