Fix files changed and reverting merge commits from the history panel bug (#1227)

basokant · fcollonval · web-flow · commit 2d186cd0d279 · 2023-04-06T17:40:38.000+02:00
* account for merge commit logs when retrieving the detailed log for a commit, as they have an extra line for the branch that was merged into the base branch

* add --cc option for detailed logging

* fix reverting merge commits, assuming the mainline branch is the first parent

* change test to reflect changes to detailed log

* correct detailed logger handler comment to include --cc -z

* init merge commit integration test

* add @jupyterlab/galata and @playwright/test

* finish merge commit integration test 1

* finish merge commit integration test 2

* finish merge commit integration test 3

* finish merge commit integration test 4 (revert merge commit)

* fix multiple installations of @playwright/test and upgrade @playwright/test dependency

* complete final working ui-tests for merge commits

* Bump nodejs

* fix jest config

* Fake crypto using node:crypto

* Fix node crypto import

* Use nodejs 16 on Binder

* fix selecting mergeCommit in playwright tests

---------

Co-authored-by: Frédéric Collonval &lt;fcollonval@gmail.com&gt;
Co-authored-by: Frédéric Collonval &lt;fcollonval@users.noreply.github.com&gt;
diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml
@@ -15,11 +15,11 @@ jobs:
         python-version: ['3.7', '3.8', '3.9', '3.10']
     steps:
       - name: Checkout
-        uses: actions/checkout@v2
+        uses: actions/checkout@v3
       - name: Install node
         uses: actions/setup-node@v1
         with:
-          node-version: '14.x'
+          node-version: '18.x'
       - name: Setup Python ${{ matrix.python-version }}
         uses: actions/setup-python@v2
         with:
@@ -101,11 +101,11 @@ jobs:
 
     steps:
       - name: Checkout
-        uses: actions/checkout@v2
+        uses: actions/checkout@v3
       - name: Install node
         uses: actions/setup-node@v1
         with:
-          node-version: '14.x'
+          node-version: '18.x'
       - uses: actions/download-artifact@v2
         with:
           name: extension
diff --git a/binder/environment.yml b/binder/environment.yml
@@ -14,7 +14,7 @@ dependencies:
   - python >=3.8,<3.9.0a0
   - jupyterlab >=3,<4.0.0a0
   # labextension build dependencies
-  - nodejs >=14,<15
+  - nodejs >=16,<17
   - pip
   - wheel
   # additional packages for demos
diff --git a/jest.config.js b/jest.config.js
@@ -7,6 +7,7 @@ tsOptions['inlineSourceMap'] = true;
 
 const esModules = [
   '.*@jupyterlab/',
+  '@jupyter/ydoc',
   'lib0',
   'y\\-protocols',
   'y\\-websocket',
diff --git a/jupyterlab_git/git.py b/jupyterlab_git/git.py
@@ -567,13 +567,16 @@ async def log(self, path, history_count=10, follow_path=None):
 
     async def detailed_log(self, selected_hash, path):
         """
-        Execute git log -1 --numstat --oneline -z command (used to get
+        Execute git log -m --cc -1 --numstat --oneline -z command (used to get
         insertions & deletions per file) & return the result.
         """
         cmd = [
             "git",
             "log",
+            "--cc",
+            "-m",
             "-1",
+            "--oneline",
             "--numstat",
             "--pretty=format:%b%x00",
             "-z",
@@ -596,35 +599,38 @@ async def detailed_log(self, selected_hash, path):
         for line in line_iterable:
             is_binary = line.startswith("-\t-\t")
             previous_file_path = ""
-            insertions, deletions, file = line.split("\t")
-            insertions = insertions.replace("-", "0")
-            deletions = deletions.replace("-", "0")
-
-            if file == "":
-                # file was renamed or moved, we need next two lines of output
-                from_path = next(line_iterable)
-                to_path = next(line_iterable)
-                previous_file_path = from_path
-                modified_file_name = from_path + " => " + to_path
-                modified_file_path = to_path
-            else:
-                modified_file_name = file.split("/")[-1]
-                modified_file_path = file
-
-            file_info = {
-                "modified_file_path": modified_file_path,
-                "modified_file_name": modified_file_name,
-                "insertion": insertions,
-                "deletion": deletions,
-                "is_binary": is_binary,
-            }
+            tokens = line.split("\t")
+
+            if len(tokens) == 3:
+                insertions, deletions, file = line.split("\t")
+                insertions = insertions.replace("-", "0")
+                deletions = deletions.replace("-", "0")
+
+                if file == "":
+                    # file was renamed or moved, we need next two lines of output
+                    from_path = next(line_iterable)
+                    to_path = next(line_iterable)
+                    previous_file_path = from_path
+                    modified_file_name = from_path + " => " + to_path
+                    modified_file_path = to_path
+                else:
+                    modified_file_name = file.split("/")[-1]
+                    modified_file_path = file
+
+                file_info = {
+                    "modified_file_path": modified_file_path,
+                    "modified_file_name": modified_file_name,
+                    "insertion": insertions,
+                    "deletion": deletions,
+                    "is_binary": is_binary,
+                }
 
-            if previous_file_path:
-                file_info["previous_file_path"] = previous_file_path
+                if previous_file_path:
+                    file_info["previous_file_path"] = previous_file_path
 
-            result.append(file_info)
-            total_insertions += int(insertions)
-            total_deletions += int(deletions)
+                result.append(file_info)
+                total_insertions += int(insertions)
+                total_deletions += int(deletions)
 
         modified_file_note = "{num_files} files changed, {insertions} insertions(+), {deletions} deletions(-)".format(
             num_files=len(result),
@@ -930,7 +936,7 @@ async def delete_commit(self, commit_id, path):
         """
         Delete a specified commit from the repository.
         """
-        cmd = ["git", "revert", "--no-commit", commit_id]
+        cmd = ["git", "revert", "-m", "1", "--no-commit", commit_id]
         code, _, error = await execute(cmd, cwd=path)
 
         if code != 0:
diff --git a/jupyterlab_git/handlers.py b/jupyterlab_git/handlers.py
@@ -245,7 +245,7 @@ async def post(self, path: str = ""):
 
 class GitDetailedLogHandler(GitHandler):
     """
-    Handler for 'git log -1 --stat --numstat --oneline' command.
+    Handler for 'git log -m --cc -1 --stat --numstat --oneline -z' command.
     Fetches file names of committed files, Number of insertions &
     deletions in that commit.
     """
diff --git a/jupyterlab_git/tests/test_detailed_log.py b/jupyterlab_git/tests/test_detailed_log.py
@@ -101,7 +101,10 @@ async def test_detailed_log():
             [
                 "git",
                 "log",
+                "--cc",
+                "-m",
                 "-1",
+                "--oneline",
                 "--numstat",
                 "--pretty=format:%b%x00",
                 "-z",
diff --git a/package.json b/package.json
@@ -55,6 +55,7 @@
     "@jupyterlab/coreutils": "^5.0.0",
     "@jupyterlab/docregistry": "^3.0.0",
     "@jupyterlab/filebrowser": "^3.0.0",
+    "@jupyterlab/galata": "^4.5.2",
     "@jupyterlab/mainmenu": "^3.0.0",
     "@jupyterlab/nbformat": "^3.0.0",
     "@jupyterlab/rendermime": "^3.0.0",
@@ -74,6 +75,7 @@
     "@material-ui/core": "^4.8.2",
     "@material-ui/icons": "^4.5.1",
     "@material-ui/lab": "^4.0.0-alpha.54",
+    "@playwright/test": "^1.32.1",
     "diff-match-patch": "^1.0.4",
     "nbdime": "^6.1.1",
     "nbdime-jupyterlab": "^2.1.0",
diff --git a/testutils/jest-setup-files.js b/testutils/jest-setup-files.js
@@ -1,4 +1,7 @@
-global.fetch = require('jest-fetch-mock');
+globalThis.fetch = require('jest-fetch-mock');
+// Use node crypto for crypto
+globalThis.crypto = require('crypto');
+
 require("enzyme").configure({
     adapter: new (require('@wojtekmaj/enzyme-adapter-react-17'))
-});
+});
diff --git a/ui-tests/package.json b/ui-tests/package.json
@@ -7,7 +7,6 @@
     "test": "playwright test"
   },
   "devDependencies": {
-    "@jupyterlab/galata": "^4.1.0",
-    "@playwright/test": "^1.17.0"
+    "@jupyterlab/galata": "^4.5.3"
   }
 }
diff --git a/ui-tests/tests/data/test-repository-merge-commits.tar.gz b/ui-tests/tests/data/test-repository-merge-commits.tar.gz
diff --git a/ui-tests/tests/merge-commit.spec.ts b/ui-tests/tests/merge-commit.spec.ts
@@ -0,0 +1,98 @@
+import { test } from '@jupyterlab/galata';
+import { expect } from '@playwright/test';
+import path from 'path';
+import { extractFile } from './utils';
+
+const baseRepositoryPath = 'test-repository-merge-commits.tar.gz';
+test.use({ autoGoto: false });
+
+test.describe('Merge commit tests', () => {
+  test.beforeEach(async ({ baseURL, page, tmpPath }) => {
+    await extractFile(
+      baseURL,
+      path.resolve(__dirname, 'data', baseRepositoryPath),
+      path.join(tmpPath, 'repository.tar.gz')
+    );
+
+    // URL for merge commit example repository
+    await page.goto(`tree/${tmpPath}/repository`);
+
+    await page.sidebar.openTab('jp-git-sessions');
+
+    await page.getByRole('tab', { name: 'History' }).click();
+  });
+
+  test('should correctly display num files changed, insertions, and deletions', async ({
+    page
+  }) => {
+    const mergeCommit = await page.getByText("Merge branch 'sort-names'");
+
+    await mergeCommit.click();
+
+    const filesChanged = await mergeCommit.getByTitle('# Files Changed');
+    const insertions = await mergeCommit.getByTitle('# Insertions');
+    const deletions = await mergeCommit.getByTitle('# Deletions');
+
+    await filesChanged.waitFor();
+
+    await expect(await filesChanged.innerText()).toBe('3');
+    await expect(await insertions.innerText()).toBe('18240');
+    await expect(await deletions.innerText()).toBe('18239');
+  });
+
+  test('should correctly display files changed', async ({ page }) => {
+    const mergeCommit = await page.getByText("Merge branch 'sort-names'");
+
+    await mergeCommit.click();
+
+    const helloWorldFile = page.getByRole('listitem', {
+      name: 'hello-world.py'
+    });
+    const namesFile = page.getByRole('listitem', { name: 'names.txt' });
+    const newFile = page.getByRole('listitem', { name: 'new-file.txt' });
+
+    expect(helloWorldFile).toBeTruthy();
+    expect(namesFile).toBeTruthy();
+    expect(newFile).toBeTruthy();
+  });
+
+  test('should diff file after clicking', async ({ page }) => {
+    const mergeCommit = await page.getByText("Merge branch 'sort-names'");
+
+    await mergeCommit.click();
+
+    const file = page.getByRole('listitem', { name: 'hello-world.py' });
+    await file.click();
+
+    await page
+      .getByRole('tab', { name: 'hello-world.py' })
+      .waitFor({ state: 'visible' });
+
+    expect(page.waitForSelector('.jp-git-diff-root')).toBeTruthy();
+  });
+
+  test('should revert merge commit', async ({ page }) => {
+    const mergeCommit = await page.getByText("Merge branch 'sort-names'");
+
+    await mergeCommit.click();
+    await page
+      .getByRole('button', { name: 'Revert changes introduced by this commit' })
+      .click();
+
+    const dialog = await page.getByRole('dialog');
+    await dialog.waitFor({ state: 'visible' });
+
+    await expect(dialog).toBeTruthy();
+
+    await dialog.getByRole('button', { name: 'Submit' }).click();
+    await dialog.waitFor({ state: 'detached' });
+
+    const revertMergeCommit = await page
+      .locator('#jp-git-sessions')
+      .getByText("Revert 'Merge branch 'sort-names''");
+
+    await revertMergeCommit.waitFor({ state: 'visible' });
+
+    expect(revertMergeCommit).toBeTruthy();
+  });
+});
diff --git a/ui-tests/yarn.lock b/ui-tests/yarn.lock
diff --git a/yarn.lock b/yarn.lock

Original file line number	Diff line number	Diff line change
`@@ -7,7 +7,6 @@`
`7`	`7`	`"test": "playwright test"`
`8`	`8`	`},`
`9`	`9`	`"devDependencies": {`
`10`		`- "@jupyterlab/galata": "^4.1.0",`
`11`		`- "@playwright/test": "^1.17.0"`
	`10`	`+ "@jupyterlab/galata": "^4.5.3"`
`12`	`11`	`}`
`13`	`12`	`}`