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

Load gitignore file #1273

Merged
Merged
Show file tree
Hide file tree
Changes from 9 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 42 additions & 0 deletions jupyterlab_git/git.py
Original file line number Diff line number Diff line change
Expand Up @@ -1681,6 +1681,22 @@ async def remote_remove(self, path, name):

return response

async def read_file(self, path):
Copy link
Member

Choose a reason for hiding this comment

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

This is actually not asynchronous 😉

Suggested change
async def read_file(self, path):
def read_file(self, path):

"""
Reads file content located at path and returns it as a string

path: str
The path of the file
"""
try:
file = pathlib.Path(path)
if file.stat().st_size > 0:
Copy link
Member

Choose a reason for hiding this comment

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

Is there a specific reason for testing this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just wanted to check if the file was empty and then return an empty string if it was. I guess since if the file is empty, then it would be an empty string regardless. I will remove it.

content = file.read_text()
return content
return ""
except BaseException as error:
return ""

async def ensure_gitignore(self, path):
"""Handle call to ensure .gitignore file exists and the
next append will be on a new line (this means an empty file
Expand Down Expand Up @@ -1721,6 +1737,32 @@ async def ignore(self, path, file_path):
return {"code": -1, "message": str(error)}
return {"code": 0}

async def writeGitignore(self, path, content):
Copy link
Member

Choose a reason for hiding this comment

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

Let's be consistent with the naming of Python (even if I find myself often bitten by this one too):

Suggested change
async def writeGitignore(self, path, content):
async def write_gitignore(self, path, content):

"""
Handle call to overwrite .gitignore.
Takes the .gitignore file and clears its previous contents
Writes the new content onto the file

path: str
Top Git repository path
content: str
New file contents
"""
try:
res = await self.ensure_gitignore(path)
if res["code"] != 0:
return res
gitignore = pathlib.Path(path) / ".gitignore"
with gitignore.open("a") as f:
f.truncate(0)
f.seek(0)
f.write(content)
if content[-1] != "\n":
f.write("\n")
Copy link
Member

Choose a reason for hiding this comment

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

You could write directly the file content:

Suggested change
with gitignore.open("a") as f:
f.truncate(0)
f.seek(0)
f.write(content)
if content[-1] != "\n":
f.write("\n")
if content and content[-1] != "\n":
content += "\n"
gitignore.write_text(content)

See the doc for more info: https://docs.python.org/3/library/pathlib.html#pathlib.Path.write_text

except BaseException as error:
return {"code": -1, "message": str(error)}
return {"code": 0}

async def version(self):
"""Return the Git command version.

Expand Down
8 changes: 7 additions & 1 deletion jupyterlab_git/handlers.py
Original file line number Diff line number Diff line change
Expand Up @@ -818,15 +818,21 @@ async def post(self, path: str = ""):
local_path = self.url2localpath(path)
data = self.get_json_body()
file_path = data.get("file_path", None)
content = data.get("content", None)
use_extension = data.get("use_extension", False)
if file_path:
if content:
body = await self.git.writeGitignore(local_path, content)
elif file_path:
if use_extension:
suffixes = Path(file_path).suffixes
if len(suffixes) > 0:
file_path = "**/*" + ".".join(suffixes)
body = await self.git.ignore(local_path, file_path)
else:
body = await self.git.ensure_gitignore(local_path)
if not self.serverapp.contents_manager.allow_hidden and not content:
self.set_status(403, "hidden files cannot be accessed")
body["content"] = await self.git.read_file(local_path + "/.gitignore")
Copy link
Member

Choose a reason for hiding this comment

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

We need to change the logic because that code is increasing the complexity of the endpoint and returns the file content in any case even if the user wants to respect the configuration of not accessing the hidden file.


if body["code"] != 0:
self.set_status(500)
Expand Down
126 changes: 123 additions & 3 deletions src/commandsAndMenu.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ import { Contents, ContentsManager } from '@jupyterlab/services';
import { ISettingRegistry } from '@jupyterlab/settingregistry';
import { ITerminal } from '@jupyterlab/terminal';
import { ITranslator, TranslationBundle } from '@jupyterlab/translation';
import { closeIcon, ContextMenuSvg } from '@jupyterlab/ui-components';
import { closeIcon, ContextMenuSvg, saveIcon } from '@jupyterlab/ui-components';
import { ArrayExt, find, toArray } from '@lumino/algorithm';
import { CommandRegistry } from '@lumino/commands';
import { PromiseDelegate } from '@lumino/coreutils';
Expand Down Expand Up @@ -52,6 +52,9 @@ import { AdvancedPushForm } from './widgets/AdvancedPushForm';
import { GitCredentialsForm } from './widgets/CredentialsBox';
import { discardAllChanges } from './widgets/discardAllChanges';
import { CheckboxForm } from './widgets/GitResetToRemoteForm';
import { CodeEditor } from '@jupyterlab/codeeditor/lib/editor';
import { CodeEditorWrapper } from '@jupyterlab/codeeditor/lib/widget';
import { editorServices } from '@jupyterlab/codemirror';

export interface IGitCloneArgs {
/**
Expand Down Expand Up @@ -303,13 +306,124 @@ export function addCommands(
}
});

async function showGitignore(error: any) {
const model = new CodeEditor.Model({});
Copy link
Member

Choose a reason for hiding this comment

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

You can skip passing an empty object. And this is probably better after the logic for looking if a widget already exists.

Suggested change
const model = new CodeEditor.Model({});
const model = new CodeEditor.Model();

const id = 'git-ignore';
//
const gitIgnoreWidget = find(shell.widgets(), shellWidget => {
if (shellWidget.id === id) {
return true;
}
});
if (gitIgnoreWidget) {
shell.activateById(id);
return;
}
Copy link
Member

@fcollonval fcollonval Oct 26, 2023

Choose a reason for hiding this comment

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

There is a small issue with that code; it will block opening gitignore files for different repositories.

The idea is good but you probably should add the repository path to the id to all that.

model.sharedModel.setSource(error.content ? error.content : '');
const editor = new CodeEditorWrapper({
factory: editorServices.factoryService.newDocumentEditor,
model: model
});
const modelChangedSignal = model.sharedModel.changed;
editor.disposed.connect(() => {
model.dispose();
});
const preview = new PreviewMainAreaWidget({
Copy link
Member

Choose a reason for hiding this comment

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

Let's use a MainAreaWidget directly here as the intention is to edit the file content:

Suggested change
const preview = new PreviewMainAreaWidget({
const preview = new MainAreaWidget({

content: editor
});
preview.title.label = '.gitignore';
preview.id = id;
preview.title.icon = gitIcon;
preview.title.closable = true;
const saveButton = new ToolbarButton({
icon: saveIcon,
onClick: async () => {
if (saved) {
return;
}
const newContent = model.sharedModel.getSource();
try {
await gitModel.writeGitIgnore(newContent);
preview.title.className = '';
saved = true;
} catch (error) {
console.log('Could not save .gitignore');
}
},
tooltip: trans.__('Saves .gitignore')
});
let saved = true;
preview.toolbar.addItem('save', saveButton);
shell.add(preview);
modelChangedSignal.connect(() => {
if (saved) {
saved = false;
preview.title.className = 'not-saved';
}
});
}

/* Helper: Show gitignore hidden file */
async function showGitignoreHiddenFile(error: any) {
const result = await showDialog({
title: trans.__('Warning: The .gitignore file is a hidden file.'),
body: (
<div>
{trans.__(
'Hidden files by default cannot be accessed with the regular code editor. In order to open the .gitignore file you must:'
)}
<ol>
<li>
{trans.__(
'Print the command below to create a jupyter_server_config.py file with defaults commented out. If you already have the file located in .jupyter, skip this step.'
)}
<div style={{ padding: '0.5rem' }}>
{trans.__('jupyter server --generate-config')}
fcollonval marked this conversation as resolved.
Show resolved Hide resolved
</div>
</li>
<li>
{trans.__(
'Open jupyter_server_config.py, uncomment out the following line and set it to True:'
)}
<div style={{ padding: '0.5rem' }}>
{trans.__('c.ContentsManager.allow_hidden = False')}
fcollonval marked this conversation as resolved.
Show resolved Hide resolved
</div>
</li>
</ol>
</div>
),
buttons: [
Dialog.cancelButton({ label: trans.__('Cancel') }),
Dialog.okButton({ label: trans.__('Show .gitignore file anyways') })
],
checkbox: {
label: 'Do not show this warning again',
fcollonval marked this conversation as resolved.
Show resolved Hide resolved
checked: false
}
});
if (result.button.accept) {
settings.set('hideHiddenFileWarning', result.isChecked);
Copy link
Member

Choose a reason for hiding this comment

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

💯

You are only missing one element for dealing with settings; they require to be defined in a JSON schema. For this extension, the schema is defined in schema/plugin.json. You will find existing example for boolean setting in that file.

showGitignore(error);
}
}

/** Add git open gitignore command */
commands.addCommand(CommandIDs.gitOpenGitignore, {
label: trans.__('Open .gitignore'),
caption: trans.__('Open .gitignore'),
isEnabled: () => gitModel.pathRepository !== null,
execute: async () => {
await gitModel.ensureGitignore();
try {
await gitModel.ensureGitignore();
} catch (error: any) {
if (error?.name === 'hiddenFile') {
if (settings.composite['hideHiddenFileWarning']) {
await showGitignore(error);
} else {
await showGitignoreHiddenFile(error);
Copy link
Member

Choose a reason for hiding this comment

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

Could you change a bit the signature of showGitignoreHiddenFile to take a second optional argument that will decide if the user should be prompted with a dialog or not? showGitignore will therefore be an implementation detail of showGitignoreHiddenFile; the goal is to reduce the API between the various part of the code.

}
}
}
}
});

Expand Down Expand Up @@ -1456,7 +1570,13 @@ export function addCommands(
const { files } = args as any as CommandArguments.IGitContextAction;
for (const file of files) {
if (file) {
await gitModel.ignore(file.to, false);
try {
await gitModel.ignore(file.to, false);
} catch (error: any) {
if (error?.name === 'hiddenFile') {
await showGitignoreHiddenFile(error);
}
}
}
}
}
Expand Down
3 changes: 3 additions & 0 deletions src/git.ts
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,9 @@ export async function requestAPI<T>(
if (!response.ok) {
if (isJSON) {
const { message, traceback, ...json } = data;
if (response.status === 403) {
throw new Git.HiddenFile(data.content);
}
Copy link
Member

Choose a reason for hiding this comment

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

We unfortunately gonna need to change the logic because this is not robust (we may get a 403 code for other reasons).

throw new Git.GitResponseError(
response,
message ||
Expand Down
16 changes: 16 additions & 0 deletions src/model.ts
Original file line number Diff line number Diff line change
Expand Up @@ -860,6 +860,7 @@ export class GitExtension implements IGitExtension {
* @throws {Git.NotInRepository} If the current path is not a Git repository
* @throws {Git.GitResponseError} If the server response is not ok
* @throws {ServerConnection.NetworkError} If the request cannot be made
* @throws {Git.HiddenFile} If the file is hidden
*/
async ensureGitignore(): Promise<void> {
const path = await this._getPathRepository();
fcollonval marked this conversation as resolved.
Show resolved Hide resolved
Expand All @@ -869,6 +870,20 @@ export class GitExtension implements IGitExtension {
await this.refreshStatus();
}

/**
* Overwrites content onto .gitignore file
*
* @throws {Git.NotInRepository} If the current path is not a Git repository
* @throws {Git.GitResponseError} If the server response is not ok
* @throws {ServerConnection.NetworkError} If the request cannot be made
*/
async writeGitIgnore(content: string): Promise<void> {
const path = await this._getPathRepository();

await requestAPI(URLExt.join(path, 'ignore'), 'POST', { content: content });
await this.refreshStatus();
}

/**
* Fetch to get ahead/behind status
*
Expand Down Expand Up @@ -923,6 +938,7 @@ export class GitExtension implements IGitExtension {
* @throws {Git.NotInRepository} If the current path is not a Git repository
* @throws {Git.GitResponseError} If the server response is not ok
* @throws {ServerConnection.NetworkError} If the request cannot be made
* @throws {Git.HiddenFile} If hidden files are not enabled
*/
async ignore(filePath: string, useExtension: boolean): Promise<void> {
const path = await this._getPathRepository();
Expand Down
10 changes: 10 additions & 0 deletions src/tokens.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1300,6 +1300,16 @@ export namespace Git {
}
}

export class HiddenFile extends Error {
content: string;
constructor(content: string) {
super('File is hidden');
this.name = 'hiddenFile';
this.message = 'File is hidden and cannot be accessed.';
this.content = content;
Copy link
Member

Choose a reason for hiding this comment

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

content will not be needed

Suggested change
content: string;
constructor(content: string) {
super('File is hidden');
this.name = 'hiddenFile';
this.message = 'File is hidden and cannot be accessed.';
this.content = content;
constructor(content: string) {
super('File is hidden');
this.name = 'hiddenFile';
this.message = 'File is hidden and cannot be accessed.';

}
}

/**
* Interface for dialog with one checkbox.
*/
Expand Down
14 changes: 14 additions & 0 deletions style/diff-common.css
Original file line number Diff line number Diff line change
Expand Up @@ -180,3 +180,17 @@ button.jp-git-diff-resolve .jp-ToolbarButtonComponent-label {
var(--jp-border-color0) 12px
);
}

.not-saved
> .lm-TabBar-tabCloseIcon
> :not(:hover)
> .jp-icon-busy[fill] {
fill: var(--jp-inverse-layout-color3);
}

.not-saved
> .lm-TabBar-tabCloseIcon
> :not(:hover)
> .jp-icon3[fill] {
fill: none;
}
Loading