feat: Quality of life improvements for Changelog, History and Diff views

- History: now displayed via GET and /../history/rev_a/rev_b instead of
  POST submitted reuqest. To not mess up the history and be able to link
  the diff.
- Diff: The displayed chunks/the lines of the chunks were not correctly
  ordered. Now what is displayed matches what a unidiff looks like.
- Changelog/Show commit: Now the /commit/ can handle renamed, deleted or
  created files much better. In case of an edit the diff is shown.

Author: redimp

otterwiki/helper.py

@@ -9,6 +9,8 @@
 
 """
 
+import re
+from collections import namedtuple
 from otterwiki.server import app, mail, storage, Preferences
 from otterwiki.gitstorage import StorageError
 from flask import flash, url_for, session
@@ -190,3 +192,26 @@ def get_pagename_prefixes(filter=[]):
                 pagename_prefixes.append(crumb)
             if len(pagename_prefixes)>3: break
     return pagename_prefixes
+
+
+
+def patchset2urlmap(patchset, rev_b, rev_a=None):
+    url_map = {}
+    for file in patchset:
+        source_file = re.sub('^(a|b)/','',file.source_file)
+        target_file = re.sub('^(a|b)/','',file.target_file)
+        if rev_a is None:
+            #import ipdb; ipdb.set_trace()
+            try:
+                rev_a = storage.get_parent_revision(filename=source_file,revision=rev_b)
+            except StorageError:
+                rev_a = rev_b
+        d = {
+            'source_file' : source_file,
+            'target_file' : target_file,
+            'source_url' : auto_url(source_file,revision=rev_a)[1] if source_file != "/dev/null" else None,
+            'target_url' : auto_url(target_file,revision=rev_b)[1] if target_file != "/dev/null" else None,
+        }
+        url_map[file.path] = namedtuple('UrlData', d.keys())(*d.values())
+    return url_map
+

otterwiki/static/css/partials/history.css

@@ -9,9 +9,11 @@ table.diff td {
 table.diff td.filename {
     padding-top: 1rem;
     padding-left: 0;
+    border-bottom: 1px solid rgba(128,128,128,0.25);
 }
 table.diff tr.added,
 table.diff tr.removed,
+table.diff td.value,
 table.diff td.hunk {
     font-family: SFMono-Regular,Menlo,Monaco,Consolas,"Liberation Mono","Courier New",monospace;
 }
@@ -89,4 +91,4 @@ a:hover span.page-match {
     color: rgba(0,0,0,.85);
     text-decoration: underline;
     padding: 0.2em;
-}
+}

otterwiki/templates/diff.html

@@ -1,61 +1,79 @@
 {# vim: set et ts=8 sts=4 sw=4 ai: #}
+{% if pagepath %}
 {% extends "page.html" %}
+{% else %}
+{% extends "wiki.html" %}
+{% endif %}
+
+
+{% block menu %}
+{{ super() }}
+{% if not pagepath and menutree %}
+{% include 'snippets/menutree.html' %}
+{% endif %}
+{% endblock %}
+
 {% block content %}
 <div class="w-full mw-full p-0 clearfix">
 {% if rev_a and rev_b %}
     <h3>Comparing <tt>{{rev_a}}</tt> to <tt>{{rev_b}}</tt></h3>
-{% endif %}
-{% if revision %}
+{% elif revision %}
     <h3>Commit <tt>{{revision}}</tt></h3>
 {% endif %}
+{% if metadata %}
+<span class="datetime" title="{{metadata.datetime|format_datetime("deltanow")}} ago">{{metadata.datetime|format_datetime}}</span>
+{%if not metadata.author_email%}{{metadata.author_name}}{%else%}<a href="mailto:{{metadata.author_email}}">{{metadata.author_name}}</a>{%endif%}:
+{{metadata.message or '-/-'|safe}}
+{%endif%}
 <table class="diff">
-{% if patchset %}
 {% for file in patchset %}
-{# file.path #} {# file.added #} {# file.removed #}
+{% with filename=file.path,lines=file_diffs[file.path],urlobj=url_map[file.path] %}
+{# {{filename}} {{lines}} {{file}} #}
+{%- if page_filename|length == 0 or filename == page_filename %}
 <tr>
 <td class="filename" colspan="{%if withlinenumbers %}4{%else%}2{%endif%}">
-<a href="{{url_map[file.path][1]}}">{{url_map[file.path][0]}}</a>
+{%- if urlobj.source_file == urlobj.target_file %}
+    <tt>{{urlobj.source_file}}</tt> <a href="{{urlobj.source_url}}" class="revision-small">{{rev_a}}</a> ..
+    <a href="{{urlobj.target_url}}" class="revision-small">{{rev_b}}</a>
+{%- else %}
+    {%- if urlobj.source_url %}
+    <a href="{{urlobj.source_url}}"><tt>{{urlobj.source_file}}</tt></a>
+    {%- else %}
+    <tt>{{urlobj.source_file}}</tt>
+    {%- endif %}
+    ..
+    {%- if urlobj.target_url %}
+    <a href="{{urlobj.target_url}}"><tt>{{urlobj.target_file}}</tt></a>
+    {%- else %}
+    <tt>{{urlobj.target_file}}</tt>
+    {%- endif %}
+{%- endif %}
+{#
+{%- elif rev_a and rev_b %}
+    <tt>{{url_map[filename][0][0]}}</tt> <a href="{{url_map[filename][0][1]}}" class="revision-small">{{rev_a}}</a> .. <a href="{{url_map[filename][1][1]}}" class="revision-small">{{rev_b}}</a>
+    {%- endif %}
+#}
 </td>
 </tr>
-{% for hunk in file %}
-<tr>
-    <td class="hunk" colspan="{%if withlinenumbers %}4{%else%}2{%endif%}">@@ {{hunk.source_start}},{{hunk.source_length}} {{hunk.target_start}},{{hunk.target_length}}@@ </td>
-</tr>
-{% for i,lines in hunk_helper[(file.source_file, file.target_file, hunk.source_start, hunk.source_length)].items() -%}
 {% for l in lines %}
 <tr class="{{l.style}}">
+{%- if l.style == "hunk" %}
+<td class="hunk" colspan="{%if withlinenumbers %}4{%else%}2{%endif%}">{{l.value}}</td>
+{%- else %}
 {%-if withlinenumbers -%}
     <td class="diff-decoration">{{l.source}}</td>
     <td class="diff-decoration">{{l.target}}</td>
 {%- endif -%}
     <td class="diff-decoration">{{l.type}}</td>
-    <td>{{l.value | replace('\n', '')}}</td>
+    <td class="value">{{l.value | replace('\n', '')}}</td>
+{%- endif -%}
 </tr>
 {%- endfor -%}{# l in lines #}
-{%- endfor %}{# lines in hunk_helper #}
-
-{#
-{% for l in hunk.source_lines()-%}
-<tr style="background-color:#a00;">
-    <td style="width:1%;">{{l.source_line_no}}</td>
-    <td style="width:1%;">{{l.target_line_no}}</td>
-    <td style="width:1%;">{{l.line_type}}</td>
-    <td>{{l.value}}</td>
-</tr>
-{% endfor %}
-{% for l in hunk.target_lines()-%}
-<tr style="background-color:#0a0;">
-    <td style="width:1%;">{{l.source_line_no}}</td>
-    <td style="width:1%;">{{l.target_line_no}}</td>
-    <td style="width:1%;">{{l.line_type}}</td>
-    <td>{{l.value}}</td>
-</tr>
-{% endfor %}
-#}
-
-{% endfor %}{# hunk in file #}
-{% endfor %}{# file in patchset #}
+{%- else -%}
+<!-- skipped page_filename={{page_filename}} filename={{filename}} -->
+{%- endif %}
+{% endwith %}{# filenname,lines in file_diffs #}
+{% endfor %}{# filenname,lines in file_diffs #}
 </table>
-{% endif %}
+{######}
 {% endblock %}
-

otterwiki/util.py

@@ -154,51 +154,38 @@ def mkdir(path):
     pathlib.Path(path).mkdir(parents=True, exist_ok=True)
 
 
-def patchset2hunkdict(patchset):
-    hunk_helper = {}
+def patchset2filedict(patchset):
     _line_type_style = {
         " ": "",
         "+": "added",
         "-": "removed",
     }
+    files = {}
     for file in patchset:
+        line_data = []
         for hunk in file:
-            lines = {}
-            for l in hunk.source_lines():
-                if not l.source_line_no in lines:
-                    lines[l.source_line_no] = []
-                lines[l.source_line_no].append(
+            line_data.append(
+                {
+                    "source": "",
+                    "target": "",
+                    "value": f"@@ {hunk.source_start},{hunk.source_length} {hunk.target_start},{hunk.target_length} @@",
+                    "style": "hunk",
+                }
+            )
+            for line in hunk:
+                line_data.append(
                     {
-                        "source": l.source_line_no,
-                        "target": l.target_line_no or "",
-                        "type": l.line_type,
-                        "style": _line_type_style[l.line_type],
-                        "value": l.value,
+                        "source": line.source_line_no or "",
+                        "target": line.target_line_no or "",
+                        "type": line.line_type,
+                        "style": _line_type_style[line.line_type],
+                        "value": line.value,
+                        "hunk": False,
                     }
                 )
-            for l in hunk.target_lines():
-                if l.source_line_no is not None:
-                    continue
-                if not l.target_line_no in lines:
-                    lines[l.target_line_no] = []
-                lines[l.target_line_no].append(
-                    {
-                        "source": l.source_line_no or "",
-                        "target": l.target_line_no,
-                        "type": l.line_type,
-                        "style": _line_type_style[l.line_type],
-                        "value": l.value,
-                    }
-                )
-            hunk_helper[
-                (
-                    file.source_file,
-                    file.target_file,
-                    hunk.source_start,
-                    hunk.source_length,
-                )
-            ] = lines
-    return hunk_helper
+        files[file.path] = line_data
+
+    return files
 
 
 def get_local_timezone():

otterwiki/views.py

@@ -259,6 +259,15 @@ def history(path):
         rev_b=request.form.get("rev_b"),
     )
 
+@app.route("/<path:path>/diff/<string:rev_a>/<string:rev_b>", methods=["POST", "GET"])
+def diff(path, rev_a, rev_b):
+    # return "path={}".format(path)
+    p = Page(path)
+    return p.diff(
+        rev_a=rev_a,
+        rev_b=rev_b,
+    )
+
 
 @app.route("/<path:path>/rename/", methods=["POST", "GET"])
 @app.route("/<path:path>/rename", methods=["POST", "GET"])

otterwiki/wiki.py

@@ -29,7 +29,7 @@
     get_pagepath,
     get_page_directoryname,
     sanitize_pagename,
-    patchset2hunkdict,
+    patchset2filedict,
     get_header,
 )
 from otterwiki.helper import (
@@ -39,6 +39,7 @@
     get_attachment_directoryname,
     get_pagename,
     get_pagename_prefixes,
+    patchset2urlmap,
 )
 from otterwiki.auth import has_permission, current_user
 from otterwiki.plugins import chain_hooks
@@ -55,6 +56,8 @@
 
 
 def get_breadcrumbs(pagepath):
+    if not pagepath or len(pagepath)<1:
+        return []
     # strip trailing slashes
     pagepath = pagepath.rstrip("/")
     parents = []
@@ -386,25 +389,33 @@ def show_commit(self, revision):
         if not has_permission("READ"):
             abort(403)
         try:
-            _, diff = storage.show_commit(revision)
+            metadata, diff = storage.show_commit(revision)
         except StorageError as e:
             app.logger.error(e)
             abort(404)
+
         patchset = PatchSet(diff)
-        url_map = {}
-        for file in patchset:
-            url_map[file.path] = auto_url(
-                file.path,
-                revision=revision,
-            )
-        hunk_helper = patchset2hunkdict(patchset)
+        pagepath = None
+
+        url_map=patchset2urlmap(patchset, revision)
+        if len(url_map) == 1:
+            pagepath = get_pagepath(list(url_map.keys())[0])
+
+        menutree = SidebarNavigation(get_page_directoryname(pagepath or "/"))
+
+        file_diffs = patchset2filedict(patchset)
         return render_template(
             "diff.html",
             title="commit {}".format(revision),
+            metadata=metadata,
             url_map=url_map,
+            file_diffs=file_diffs,
             patchset=patchset,
-            hunk_helper=hunk_helper,
             revision=revision,
+            withlinenumbers=False,
+            pagepath=pagepath,
+            breadcrumbs=get_breadcrumbs(pagepath),
+            menutree=menutree.query(),
         )
 
 
@@ -743,25 +754,26 @@ def diff(self, rev_a=None, rev_b=None):
         # handle case that the page doesn't exists
         self.exists_or_404()
 
-        diff = storage.diff(self.filename, rev_b, rev_a)
+        diff = storage.diff(rev_a, rev_b)
         patchset = PatchSet(diff)
-        url_map = {}
-        for file in patchset:
-            url_map[file.path] = auto_url(
-                file.path,
-                revision=rev_a,
-            )
-        hunk_helper = patchset2hunkdict(patchset)
+        url_map=patchset2urlmap(patchset, rev_b, rev_a)
+        file_diffs = patchset2filedict(patchset)
+
+        menutree = SidebarNavigation(get_page_directoryname(self.pagepath))
         return render_template(
             "diff.html",
             title="{} - diff {} {}".format(self.pagename, rev_a, rev_b),
             pagepath=self.pagepath,
             pagename=self.pagename,
+            page_filename=self.filename,
+            file_diffs=file_diffs,
             patchset=patchset,
             url_map=url_map,
-            hunk_helper=hunk_helper,
             rev_a=rev_a,
             rev_b=rev_b,
+            withlinenumbers=False,
+            menutree=menutree.query(),
+            breadcrumbs=self.breadcrumbs(),
         )
 
     def history(self, rev_a=None, rev_b=None):
@@ -779,7 +791,7 @@ def history(self, rev_a=None, rev_b=None):
                 404,
             )
         if rev_a is not None and rev_b is not None and rev_a != rev_b:
-            return self.diff(rev_a=rev_a, rev_b=rev_b)
+            return redirect(url_for("diff", path=self.pagepath, rev_a=rev_a, rev_b=rev_b))
 
         log = []
         for i,orig_entry in enumerate(orig_log):

tests/test_otterwiki.py

@@ -280,6 +280,7 @@ def test_blame_and_history_and_diff(test_client):
     rv = test_client.post(
         "/{}/history".format(pagename),
         data={"rev_a": revision[1], "rev_b": revision[0]},
+        follow_redirects=True,
     )
     assert rv.status_code == 200
     html = rv.data.decode()