GH-165: Basic Git Extension #392

kittaakos · 2017-08-14T13:13:42Z

akosyakov · 2017-09-13T15:47:51Z

packages/git/src/common/model.ts

+/**
+ * Representation of an individual file change in the working directory.
+ */
+export interface FileChange {


It would be nice to prefix such common names with Git, there is already FileChange

Or we put everything into the Git namespace?

akosyakov · 2017-09-13T15:48:35Z

packages/git/src/node/dugite-git.spec.ts

+
+});
+
+async function createGit(fsRoot: string = ''): Promise<DugiteGit> {


It would be nice to avoid module functions and prefer class methods to be able to override them.

It is a test, did you notice that? I mean I can, but does it make sense?

I meant generally, I've seen unexported module functions in other modules.

for tests, it is indeed not important

Show me, and we replace it. I personally hate writing this. when it does nothing with the class' state at all.

it should be another class then

would be even longer though :) I understand your frustration, but we don't have dependency injection for modules, but for classes

Make sense.

akosyakov · 2017-09-13T15:49:27Z

packages/git/src/node/dugite-git.ts

+export class DugiteGit implements Git {
+
+    constructor(
+        @inject(WorkspaceServer) private readonly workspace: WorkspaceServer) {


it would be nice to prefer protected over private to allow subclasses to override and access the state.

If you prefer that, we use protected then.

it is just a pain, for example, to work around some bugs in JDT because everything is private.

svenefftinge · 2017-09-21T11:26:47Z

packages/git/src/browser/git-command.ts

+        registry.registerHandler(GIT_COMMANDS.STATUS.id, {
+            execute: (): any => {
+                this.git.repositories().then(repositories => {
+                    const first = repositories.shift();


I think we should have a global repository provider that returns the repository that is currently selected in the UI instead of always the first.

Also, what should this command do in future?
Open the git widget?

svenefftinge · 2017-09-21T11:27:38Z

packages/git/src/browser/git-command.ts

+        id: 'git.status',
+        label: 'Print Git Status'
+    };
+    export const REPOSITORIES = {


I don't see how this command is useful

It was just a placeholder command to see if the interface can be called from the browser. We will get rid of this for sure.

svenefftinge · 2017-09-21T11:28:27Z

packages/git/src/browser/git-command.ts

+                            console.info(status);
+                        });
+                    } else {
+                        console.info('No repositories were found.');


let's use an ILogger

svenefftinge · 2017-09-21T11:29:30Z

packages/git/src/browser/git-command.ts

+                });
+                return undefined;
+            },
+            isEnabled: () => true


Should obtain the repository from the provider. if there is none it should be disabled.

svenefftinge · 2017-09-21T11:31:48Z

packages/git/src/browser/git-keybinding.ts

+import { KeybindingContext, Keybinding, KeybindingContribution, KeybindingRegistry, KeyCode, Key, Modifier } from "@theia/core/lib/common";
+
+@injectable()
+export class GitKeybindingContext implements KeybindingContext {


do we really need a keybinding context?

Will be removed.

svenefftinge · 2017-09-21T12:00:18Z

packages/git/src/common/model.ts

+/**
+ * Representation of an individual file change in the working directory.
+ */
+export interface FileChange {


Or we put everything into the Git namespace?

svenefftinge · 2017-09-21T12:03:46Z

packages/git/src/node/git-repository-locator.ts

+ *
+ * @param path the FS path of the root to start the discovery.
+ */
+export async function locateRepositories(path: string): Promise<Repository[]> {


async is not used in the code

svenefftinge · 2017-09-21T12:05:05Z

packages/git/src/node/git-repository-locator.ts

+    return !parts[0].length ? parts.slice(1) : parts;
+}
+
+// function getOrigin(repository, cb) {


remove dead code

svenefftinge · 2017-09-21T12:06:53Z

packages/git/src/node/git-repository-locator.ts

+
+async function splitPath(path: string): Promise<string[]> {
+    const parts = path.split(/(\/|\\)/);
+    if (!parts.length) {


I'd prefer being more explicit about length === 0.

That is bad anyway. We need to call path.dirname() twice.

svenefftinge · 2017-09-21T12:07:31Z

packages/git/src/node/git-repository-locator.ts

+    if (!parts.length) {
+        return parts;
+    }
+    // When the `path` starts with a slash, the the first part is empty string.


svenefftinge · 2017-09-23T17:16:34Z

packages/git/src/browser/git-resource.ts

 import URI from "@theia/core/lib/common/uri";

 export const GIT_RESOURCE_SCHEME = 'gitrev';

 export class GitResource implements Resource {
+
+    protected readonly toDispose = new DisposableCollection();


can we remove it? doesn't seem to be used

svenefftinge · 2017-09-23T17:17:05Z

packages/git/src/browser/git-widget.ts

-                ressource
-                    .then(r => r.readContents())
-                    .then(content => console.log(content));
+                // try {


code should be deleted

svenefftinge · 2017-09-23T17:20:55Z

packages/monaco/src/browser/monaco-common-editor.ts

@@ -0,0 +1,235 @@
+/*


module can be removed, since it is not used.

svenefftinge · 2017-09-26T13:21:04Z

solved and verified

kittaakos · 2017-09-26T16:31:00Z

packages/git/src/browser/style/index.css

@@ -0,0 +1,153 @@
+/*


@jbicker, I was unable to figure out where do we set the id of the left panel, so I just add this as a comment. The problem is the following: both the Files and the Git has the same files ID. So the browser tests are broken.

Updated.

Of course, I was wrong. I have figured out what was the problem and it is unrelated to the id. The ID is just fine.

svenefftinge · 2017-09-27T08:38:09Z

I tested a rebase with merge conflicts.
Merge conflicts are shown twice:

One entry with a '-' one with a '+'.
Opening a diff editor on the '-' one fails and logs an error :

This is fixed with 2c5b969#diff-392c37f87fa84f7102bfb5248b4837d8R72

svenefftinge · 2017-09-27T10:13:39Z

Sometimes when double-clicking on a change a diff editor gets opened twice (I will take care of that).

Fixed in d1412d1

svenefftinge · 2017-09-27T11:11:29Z

On huge repositories, the initial load sometimes has all changes twice:

solved with c78e5dd

svenefftinge · 2017-09-27T11:15:33Z

Also on huge repos, the initial status request takes multiple seconds.

@kittaakos is looking into it

We have resolved a performance issue here. The selected repository was not set properly on the frontend.

kittaakos · 2017-09-27T14:39:09Z

Open diff editor, close, open again. A broken editor is shown with this in the browser:

6:37:06.690 console-window.ts:40 TypeScript: [Error - 4:37:06 PM] Server initialization failed.
16:37:06.690 console-window.ts:40 TypeScript: Error: Connection got disposed.
    at Object.dispose (http://localhost:3000/bundle.js:6791:25)
    at http://localhost:3000/bundle.js:64956:56
    at CallbackList.invoke (http://localhost:3000/bundle.js:12821:39)
    at Emitter.fire (http://localhost:3000/bundle.js:12885:36)
    at closeHandler (http://localhost:3000/bundle.js:6187:26)
    at CallbackList.invoke (http://localhost:3000/bundle.js:12821:39)
    at Emitter.fire (http://localhost:3000/bundle.js:12885:36)
    at WebSocketMessageReader.AbstractMessageReader.fireClose (http://localhost:3000/bundle.js:22284:27)
    at WebSocketMessageReader.fireClose (http://localhost:3000/bundle.js:33595:40)
    at http://localhost:3000/bundle.js:33552:19
16:37:06.691 main.js:129 Uncaught (in promise) Error: Connection is disposed.
    at new ConnectionError (main.js:129)
    at throwIfClosedOrDisposed (main.js:607)
    at Object.sendRequest (main.js:706)
    at Object.shutdown (connection.js:20)
    at resolveConnection.then.connection (base.js:845)
    at <anonymous>
ConnectionError @ main.js:129
throwIfClosedOrDisposed @ main.js:607
sendRequest @ main.js:706
shutdown @ connection.js:20
resolveConnection.then.connection @ base.js:845
Promise resolved (async)
stop @ base.js:844
connection.initialize.then @ base.js:831
Promise rejected (async)
initialize @ base.js:747
resolveConnection.then @ base.js:657
Promise resolved (async)
start @ base.js:624
handleConnectionClosed @ base.js:919
CallbackList.invoke @ events.js:71
Emitter.fire @ events.js:135
closeHandler @ main.js:212
CallbackList.invoke @ events.js:71
Emitter.fire @ events.js:135
AbstractMessageReader.fireClose @ messageReader.js:126
WebSocketMessageReader.fireClose @ reader.ts:72
(anonymous) @ reader.ts:31
webSocket.onclose @ connection.ts:32
16:37:06.691 main.js:816 Uncaught (in promise) Error: Connection got disposed.
    at Object.dispose (main.js:816)
    at connection.ts:14
    at CallbackList.invoke (events.js:71)
    at Emitter.fire (events.js:135)
    at closeHandler (main.js:212)
    at CallbackList.invoke (events.js:71)
    at Emitter.fire (events.js:135)
    at WebSocketMessageReader.AbstractMessageReader.fireClose (messageReader.js:126)
    at WebSocketMessageReader.fireClose (reader.ts:72)
    at reader.ts:31

svenefftinge · 2017-09-27T14:50:43Z

(removed, since it was due to broken build)

kittaakos · 2017-09-27T14:55:42Z

I do not know if it is a side-effect of something else but I had this in my console, when I have tried to open an untracked file in a "diff" editor:

index.js:380 TypeError: Cannot read property 'style' of null
    at t.layout (editor.main.js:31)
    at t.layout (editor.main.js:31)
    at t._doLayout (editor.main.js:31)
    at t._measureDomElement (editor.main.js:31)
    at t.layout (editor.main.js:31)
    at MonacoDiffEditor.webpackJsonp.524.MonacoDiffEditor.resize (monaco-diff-editor.ts:78)
    at MonacoDiffEditor.webpackJsonp.505.MonacoEditor.setSize (monaco-editor.ts:232)
    at EditorWidget.onResize (editor-widget.ts:44)
    at EditorWidget.Widget.processMessage (widget.js:485)
    at invokeHandler (index.js:433)

svenefftinge · 2017-09-27T15:04:41Z

Open diff editor, close, open again. A broken editor is shown with this in the browser:

fixed in d02890b

kittaakos · 2017-09-27T15:33:34Z

Editor (not the diff one) content does not get updated after discarding all the changes:

Open a file in the editor.
Change the file content. (You do not have to save, you know...)
Leave the file opened in the editor.
Go to Git tab, discard changes.
File restored to its HEAD state.
The opened editor still shows the modified content.

This is a general problem and tracked here: #74

kittaakos · 2017-09-27T15:49:13Z

I will look into the windows build issue and I try to fix it.

Solved via this.

svenefftinge · 2017-09-28T06:19:43Z

With the current state I get, the following on commit:

messages.js:46 Uncaught (in promise) Error: Request commit failed with message: Unable to find path to repository on disk.
    at new ResponseError (messages.js:46)
    at handleResponse (main.js:421)
    at processMessageQueue (main.js:249)
    at main.js:233
    at run (setImmediate.js:40)
    at runIfPresent (setImmediate.js:69)
    at onGlobalMessage (setImmediate.js:109)

Fixed with aaa9e41

svenefftinge · 2017-09-28T06:21:32Z

packages/git/src/browser/git-widget.ts

-                        branch: this.status.branch,
-                        paths: change.uri
-                    });
+                    const options: Git.Options.Checkout.WorkingTreeFile = { paths: change.uri };


Why do I not need to specify the branch from which I want to checkout? AFAIU the HEAD will be detached when I am in a conflicting rebase situation.

kittaakos · 2017-09-28T11:49:21Z

packages/monaco/src/browser/monaco-editor-provider.ts

@@ -123,7 +164,7 @@ export class MonacoEditorProvider {
        }
    }

-    protected installQuickOpenService(editor: MonacoEditor): void {
+    protected installQuickOpenService(editor: MonacoEditor | MonacoDiffEditor): void {


Why is that? As I see it is a subclass of the MonacoEditor. Is it just some leftover code?

kittaakos · 2017-09-28T15:52:14Z

I got this when cloned a repository inside my workspace and double clicked on the unstaged resource in the Git tab:

opener-service.ts:6 Uncaught (in promise) A resource provider for 'file:///Users/akos.kitta/git/theia/examples/browser/xtext-core/' is not registered.

#593.

kittaakos · 2017-09-28T16:09:00Z

I would like to see tooltips for the Git integration toolbar items:

Could be a Chrome issue, but I cannot see anything at all.

Done.

Signed-off-by: Sven Efftinge <sven.efftinge@typefox.io>

The frontend can still manually fetch the available repositories from the backend. Signed-off-by: Akos Kitta <kittaakos@gmail.com>