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

Git Init Menu #584

Closed
mlucool opened this issue Apr 1, 2020 · 7 comments · Fixed by #700
Closed

Git Init Menu #584

mlucool opened this issue Apr 1, 2020 · 7 comments · Fixed by #700

Comments

@mlucool
Copy link
Contributor

mlucool commented Apr 1, 2020

The menu has "Git Init" in it. I suggest two separate things:

  1. Remove git init when inside a git repo already (its rare to have one git repo int another)
  2. Allow users, via a config option, to change the default command from git init to something else.

The reason for 2, is I typically want git init && setup_notebook_filters.

What do people think?

@fcollonval
Copy link
Member

Remove git init when inside a git repo already (its rare to have one git repo int another)

👍
I'm not a big fan of hiding UI element. I would suggest disabling the command in such case as done for the add remote command:

isEnabled: () => model.pathRepository !== null,

Allow users, via a config option, to change the default command from `git init` to something else.

👎
In #494, it was decided to suppress the ability to run custom commands in terminal to follow the rules of JupyterLab. So I vote to reject this proposal for the same security reason.

@mlucool
Copy link
Contributor Author

mlucool commented Apr 2, 2020

In #494, it was decided to suppress the ability to run custom commands in terminal to follow the rules of JupyterLab. So I vote to reject this proposal for the same security reason.

That is a good point. Do you have any suggestions? What if we add the ability to set in in the server config (jupyter_notebook_config.py)? I think that should mitigate the security concern.

My use case is for init to setup nbstripout (update .gitattibutes) and on clone to rerun the setup to re-add to the git filters (they are not stored for security reasons). So I'd want some way to say when you init or clone - run this command (or run this command after init or clone is fine too).

@jaipreet-s
Copy link
Member

Remove git init when inside a git repo already (its rare to have one git repo int another)

👍 as well. For reference, the Git Clone button used to be disabled if already inside a Git repo, to discourage nesting of Git repos #260, though I'm not sure if this is still the case after the recent refactoring)

@fcollonval
Copy link
Member

There is a hanging feature request for cleaning notebooks (#392). So it may be better to introduce this feature specifically through an extension option - I imagine that using a tooglable menu item would be appropriate.

There is also some issues with nbdime and nbstripout on Windows (see #471 (comment)).
Another point often mentioned is the poor performance of nbstripout (see here). So some people prefer using jq (see previous link).

@mlucool
Copy link
Contributor Author

mlucool commented Apr 2, 2020

I don't think we'll find a one size fits all solution to cleaning notebooks (e.g. don't count out jupytext).

I think it's better to focus this solely on a git UI as any feature that is added here must also work via CLI anyway. That is not to say we shouldn't enable people to use these useful tools, but, IMO, there should be no choice made in this project that isn't generic.

FWIW, nbstripout isn't fast, but it has features jq make hard to implement making it my current preferred choice in this area.

@fcollonval
Copy link
Member

@mlucool would you like to propose a PR for this one?

@ianhi
Copy link
Collaborator

ianhi commented Apr 23, 2020

A PR modifying init in the menu the could be combined with: #456

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants