Add diff support for renamed/copied file changes on commit page

[joseluisq]

• Gitea version (or commit ref): 1.13.7
• Git version: 2.31.1
• Operating system: Linux 5.11.11-arch1-1 x86_64
• Database (use [x]):
  • PostgreSQL
  • MySQL
  • MSSQL
  • SQLite
• Can you reproduce the bug at https://try.gitea.io:
  • Yes (provide example URL)
  • No
• Log gist:

Description

I have realized that Gitea is not supporting diff changes for renamed/copied files on the commit page view.
Page URL: /username/repo/commit/hash

Screenshots

For instance, I have a commit which contains a file renamed but with some additions and deletions.
Below an extract of my git show

src/core/{render.ts => dom.ts} | 74 +++++++++++++++++++++++++++++++++++++++++++++++++++++++-------------------
# ....
diff --git a/src/core/render.ts b/src/core/dom.ts
similarity index 78%
rename from src/core/render.ts
rename to src/core/dom.ts
index bc123cb..1230ccd 100644
--- a/src/core/render.ts
+++ b/src/core/dom.ts
@@ -3,28 +3,63 @@

My full Git diff patch file content:

0001-feat-tagged-html-templates-support.patch

From e3b3525a8568de55cd33c068755ff232af86ab25 Mon Sep 17 00:00:00 2001
From: Jose Quintana <[email protected]>
Date: Wed, 7 Apr 2021 23:13:52 +0200
Subject: [PATCH] feat: tagged html templates support

Signed-off-by: Jose Quintana <[email protected]>
---
 package.json                   |  2 +-
 src/components/button.ts       |  7 ++++
 src/core/component.ts          |  2 +-
 src/core/{render.ts => dom.ts} | 74 +++++++++++++++++++++++++---------
 src/core/index.ts              |  2 +-
 src/index.ts                   | 17 ++++----
 yarn.lock                      |  8 ++--
 7 files changed, 77 insertions(+), 35 deletions(-)
 rename src/core/{render.ts => dom.ts} (78%)

diff --git a/package.json b/package.json
index f97cb36..ff225bc 100644
--- a/package.json
+++ b/package.json
@@ -8,7 +8,7 @@
         "lint": "make lint"
     },
     "devDependencies": {
-        "esbuild": "^0.11.5",
+        "esbuild": "^0.11.6",
         "tslint": "^6.1.3",
         "tslint-config-standard-plus": "^2.3.0",
         "typescript": "^4.2.3"
diff --git a/src/components/button.ts b/src/components/button.ts
index b08fc26..e607aa8 100644
--- a/src/components/button.ts
+++ b/src/components/button.ts
@@ -11,9 +11,16 @@ export class Button implements Component {
         return html`
             <div>
                 <button @click="onClick">Child button click!</button>
+
+                <!-- 1. -->
                 <ul>
                     <li @for="(v, _) in alpha">{v}</li>
                 </ul>
+
+                <!-- 2. -->
+                <ol>
+                   ${this.alpha.map((v) => html`<li>${v}</li>`)}
+                </ol>
             </div>
         `
     }
diff --git a/src/core/component.ts b/src/core/component.ts
index 4c4cb02..b8e54c6 100644
--- a/src/core/component.ts
+++ b/src/core/component.ts
@@ -1,3 +1,3 @@
 export interface Component {
-    render (): HTMLElement | null
+    render (): string
 }
diff --git a/src/core/render.ts b/src/core/dom.ts
similarity index 78%
rename from src/core/render.ts
rename to src/core/dom.ts
index cb633bb..2781cfd 100644
--- a/src/core/render.ts
+++ b/src/core/dom.ts
@@ -3,28 +3,63 @@ import { Component } from "./component"
 const PLACEHOLDER = /{\s?([a-zA-Z_]+([0-9a-zA-Z_]+)?)\s?}/
 const DIR_FOR = /^\(([a-zA-Z_]+([0-9a-zA-Z_]+)?)(\s?,\s?)([a-zA-Z_]+([0-9a-zA-Z_]+)?)?\)\ in\ ([a-zA-Z_]+([0-9a-zA-Z_]+)?)$/
 
-/** It converts a HTML string literal into a DOM template element. */
-export function html (...html: any[]) {
-    if (!Array.isArray(html) || html.length === 0) {
+/** It converts a tagged HTML template into a valid HTML template string. */
+export function html (...literals: any[]) {
+    if (!Array.isArray(literals) || literals.length === 0) {
+        return ""
+    }
+
+    const rootArr = literals[0] as string[]
+
+    // TODO: Validate some data types
+    let str = ""
+    for (let i = 0; i < literals.length; i++) {
+        if (i === 0) {
+            str += rootArr[i]
+        } else {
+            const h = literals[i]
+            if (Array.isArray(h)) {
+                str += h.join("") + rootArr[i]
+            } else {
+                str += literals[i] + rootArr[i]
+            }
+        }
+    }
+
+    return str.trim()
+}
+
+/** It creates a document fragment for a given HTML markup string. */
+function createFragment (htmlStr: string) {
+    htmlStr = htmlStr.trim()
+
+    if (htmlStr === "") {
         return null
     }
-    console.log(html)
-    const str = html.join("").trim()
 
     const tmpl = document.createElement("template")
-    tmpl.innerHTML = str
+    tmpl.innerHTML = htmlStr
+
+    if (tmpl.content.childNodes.length === 0) {
+        throw new Error("HTML element has no root element.")
+    }
 
-    // TODO: validate to require only one root element.
-    const node = tmpl.content.firstChild
+    if (tmpl.content.childNodes.length === 1) {
+        return tmpl.content
+    }
+
+    if (tmpl.content.childNodes.length > 1) {
+        throw new Error("HTML element has many root elements. Only one is required.")
+    }
 
-    return node as HTMLElement | null
+    return null
 }
 
 /**
  * It handles an iterator which walks the DOM tree of the root component from top to bottom
- * filtering only element types.
+ * filtering only element types and returning a root fragment afterwards.
  */
-function renderComponentAsElement (baseComp: Component) {
+function componentAsFragment (baseComp: Component) {
     // tslint:disable-next-line
     if (typeof baseComp.render === "undefined") {
         throw new Error(
@@ -32,14 +67,14 @@ function renderComponentAsElement (baseComp: Component) {
         )
     }
 
-    const baseNode = baseComp.render()
+    const baseNode = createFragment(baseComp.render())
 
     if (!baseNode) return null
 
     let currentNode: HTMLElement | null
     const iterator = document.createNodeIterator(baseNode, NodeFilter.SHOW_ELEMENT)
 
-    const parentNodes: [Component | null, HTMLElement][] = [ [ baseComp, baseNode ] ]
+    const parentNodes: [Component | null, HTMLElement | DocumentFragment][] = [ [ baseComp, baseNode ] ]
     let counter = 0
 
     // TODO: Add proper error handling
@@ -61,9 +96,9 @@ function renderComponentAsElement (baseComp: Component) {
                         if (!bindComp) {
                             currentNode.remove()
                         } else {
-                            const bindNode = bindComp.render()
+                            const bindNode = createFragment(bindComp.render())
                             if (bindNode) {
-                                counter = parentNodes.push([ bindComp , currentNode ]) - 1
+                                counter = parentNodes.push([ bindComp, currentNode ]) - 1
                                 currentNode.appendChild(bindNode)
                                 currentNode.setAttribute("a:idx", counter.toString())
                             }
@@ -98,6 +133,7 @@ function renderComponentAsElement (baseComp: Component) {
                 // TODO: Add support for more events
 
                 // 2. Loops
+                // TODO: @for is possibly not needed since tagged templates is now supported
                 const loop = currentNode.getAttribute("@for") || ""
                 if (loop) {
                     const parts = loop.match(DIR_FOR)
@@ -172,12 +208,12 @@ function renderComponentAsElement (baseComp: Component) {
     return baseNode
 }
 
-/** It renders a component and then append it to a root element. */
-export function Render (component: Component, root: HTMLElement = document.body) {
-    const el = renderComponentAsElement(component)
+/** It renders a component and then append it to a given element. */
+export function render (component: Component, target: HTMLElement) {
+    const el = componentAsFragment(component)
 
     if (el) {
-        root.appendChild(el)
+        target.appendChild(el)
     } else {
         throw new Error(
             "A root HTML element is required for `"
diff --git a/src/core/index.ts b/src/core/index.ts
index f708bde..e3f08a3 100644
--- a/src/core/index.ts
+++ b/src/core/index.ts
@@ -1,2 +1,2 @@
+export * from "./dom"
 export * from "./component"
-export * from "./render"
diff --git a/src/index.ts b/src/index.ts
index a4b4326..5c49018 100644
--- a/src/index.ts
+++ b/src/index.ts
@@ -1,9 +1,9 @@
-import { Component, html, Render } from "./core"
+import { Component, html, render } from "./core"
 import { Button } from "./components"
 
 class App implements Component {
-    public bits = [ 16, 32, 64, 128, 256 ]
     public $btn: Button
+    public bits = [ { v: 16 }, { v: 32 }, { v: 64 }, { v: 128 }, { v: 256 } ]
 
     constructor () {
         this.$btn = new Button()
@@ -16,14 +16,12 @@ class App implements Component {
 
     render () {
         return html`
-            <main>
+            <div>
                 <h1>Component</h1>
-                <a href="#" @click="onClick">Click on link!</a> <br>
+                <a href="#" @click="onClick">Link!</a> <br>
                 <include @id="btn"></include>
-                <ul>
-                    <li @for="(v, _) in bits">{v}</li>
-                </ul>
-            </main>
+                <ul>${this.bits.map((item) => html`<li>${item.v}</li>`)}</ul>
+            </div>
         `
     }
 }
@@ -31,5 +29,6 @@ class App implements Component {
 window.addEventListener("load", () => {
     const root = document.getElementById("root") || document.createElement("div")
     root.setAttribute("id", "root")
-    Render(new App(), root)
+
+    render(new App(), root)
 })
diff --git a/yarn.lock b/yarn.lock
index f554130..a47ea17 100644
--- a/yarn.lock
+++ b/yarn.lock
@@ -99,10 +99,10 @@ [email protected]:
     esutils "^1.1.6"
     isarray "0.0.1"
 
-esbuild@^0.11.5:
-  version "0.11.5"
-  resolved "https://registry.yarnpkg.com/esbuild/-/esbuild-0.11.5.tgz#25b18a2ff2fb9580683edce26a48f64c08c2f2df"
-  integrity sha512-aRs6jAE+bVRp1tyfzUugAw1T/Y0Fwzp4Z2ROikF3h+UifoD5QlEbEYQGc6orNnnSIRhWR5VWBH7LozlAumaLHg==
+esbuild@^0.11.6:
+  version "0.11.6"
+  resolved "https://registry.yarnpkg.com/esbuild/-/esbuild-0.11.6.tgz#20961309c4cfed00b71027e18806150358d0cbb0"
+  integrity sha512-L+nKW9ftVS/N2CVJMR9YmXHbkm+vHzlNYuo09rzipQhF7dYNvRLfWoEPSDRTl10and4owFBV9rJ2CTFNtLIOiw==
 
 escape-string-regexp@^1.0.5:
   version "1.0.5"
-- 
2.31.1

However what I get in the GUI is this empty section:

So I would be great if Gitea could add support for this.

[lunny]

@joseluisq Could you provide the diff patch file?

[zeripath]

Yes, please replicate this on try.gitea.io

[zeripath]

And tell us what version of git you're using - it's highly relevant

[joseluisq]

Ok, I will try to replicate this on try.gitea.io.

I have locally git 2.31.1

[joseluisq]

@joseluisq Could you provide the diff patch file?

@lunny full diff patch file content in the issue description (updated).

[joseluisq]

@zeripath question

try.gitea.io is using 1.15.0+dev-81-g298d56fe8 do I still need to test it there?
Since I'm facing this issue in the stable 1.13.7 release?

[zeripath]

yes - it would be useful to know if it is present in master - and if you can present a case that would fail on your set-up but does not on try we can try it on various configurations to understand what is making it a heisenbug.

My suspicion is that the case is related to improvements in git's diff output so we wouldn't have seen it or handled it before.

I'll take another look at the patch parsing code to see if the issue is there - but as I say it would be excellent to have a testcase to that we can show where it fails. -- (ah I see you've updated your comment with the patch - that is extremely helpful!)

[joseluisq]

Alright, I will give it a test during the day then.

[zeripath]

OK on master and origin/release/v1.13 ParsePatch is handling this correctly - so the issue is going to be at the template level I suspect

[zeripath]

yup and looking at templates/repo/diff/box.tmpl:

gitea/templates/repo/diff/box.tmpl

Lines 52 to 54 in 05b7e32

	{{if not $file.IsRenamed}}
	{{template "repo/diff/stats" dict "file" . "root" $}}
	{{end}}

gitea/templates/repo/diff/box.tmpl

Lines 90 to 92 in 05b7e32

	{{else if not $file.IsRenamed}}
	{{template "repo/diff/stats" dict "file" . "root" $}}
	{{end}}

We can see that the compare box won't display if the file is marked as IsRenamed.

So in which case this is likely a really simple fix - we just need to figure out what happens if we drop that IsRenamed test and if that works fine when there is no diff then boom we're done.

---

So the above is just dealing with the header bar. We need to fix here to show the diff:

gitea/templates/repo/diff/box.tmpl

Line 116 in 05b7e32

{{if ne $file.Type 4}}

[joseluisq]

Great, you got it.
BTW testing on try.gitea.io is I guess no longer necessary ?

[zeripath]

Well if you have a simple reproducible example that would always be helpful.

[joseluisq]

Ok, I will not bother you more I will give it a test.

[zeripath]

Try #15340 (for 1.13)

Either download the requisite template and stick it in the correct place or compile from that PR.

[joseluisq]

Ok, since you have just modified the diff template and for closer verification and I will use it as a custom template directly in my server.

[joseluisq]

Confirmed, it shows now the renamed diff.
Tested with #15340 on Gitea 1.13.7