Diff view

[fstachura]

Note: this is work in progress, not ready for review. Comments are welcome, but I don't expect a full review just yet. Mostly posting this to show that there is some slow progress on this feature.

Right now, the feature only supports a split diff view. This neatly solves the "which version should an identifier link to" problem.

How the diffs are generated:

• Take sources of a and b, put both of them through the tokenizer and initial filters
• Take a diff of a and b in a special format (see get_diff in script.sh) that basically only shows where to add empty lines, and which lines to mark as added/deleted
• Pass sources of both files to a subclass of Pygments's HtmlFormatter, that adds empty lines/colorizes lines based on the diff information.

There is a slight efficiency problem with this approach - files can be very similar, but both will get tokenized and formatted separately. And IIRC formatting can take a lot of time.
I don't see a good way of solving that, maybe something smart could be done with the lexer/formatter.
The only optimization that could be worth it, is caching identifiers looked up in the first file for the second file (not implemented yet, and I don't know if it would make sense). I think it would also make sense to limit the size of diffed files - no one should do diffs on the 20 megabytes amdgpu register definitions.

Closes: #25

[fstachura]

I think it's more or less done. I'm not sure if identifier links and search should redirect to the diff or to the target version.

[tleb]

AI has comments! I've dropped one of its comment about merge-ability with another pending series. It'll be an easy merge, no worries there. Before that, I'm not sure I'm a fan of the UI.

First I like the idea of diffing directories. But the UI is hard to read. Then it makes me think we could diff symbol pages as well?

My main gripe is with the selection UI in the sidebar. Checkbox that does nothing on click. Then I had to guess to click another version.

So one proposal:

• Diff all with a text UI. So we turn directories into a text listing and the diff that. Same for identifiers. Then it's a single familiar diff view for all pages. And it's less code.
• For the sidebar, maybe we could have a diff toggle and then the sidebar contains two duplicate columns with the versions list? One is before, one is after.
• Diff checkbox that would show that sidebar would be a button full width at the bottom of the sidebar. This is to try to integrate it more nicely.

What do you think?

AI now:

---

Three issues on the diff routes. Same index (needs both diffed versions) for
everything:

⟩ # commit 717cb99
⟩ git checkout -q -f franek/diff

⟩ # change musl repo because it's inaccessible and fetch only the two tags we diff
⟩ git diff -U0 utils/index
diff --git a/utils/index b/utils/index
index 3298a87..6f7f15f 100755
--- a/utils/index
+++ b/utils/index
@@ -47 +47 @@ project_fetch() {
-    $git fetch --all --tags -j4
+    $git fetch --no-tags remote0 +refs/tags/v1.2.5:refs/tags/v1.2.5 +refs/tags/v1.2.6:refs/tags/v1.2.6
@@ -128 +128 @@ add_default_remotes $1 $# $2 mesa https://gitlab.freedesktop.org/mesa/mesa.git
-add_default_remotes $1 $# $2 musl https://git.musl-libc.org/git/musl
+add_default_remotes $1 $# $2 musl https://github.com/kraj/musl.git

⟩ docker build -q -t elixir-repro -f docker/Dockerfile .
⟩ sudo rm -rf data/

⟩ docker run --rm -v ./data:/srv/elixir-data --entrypoint index \
  elixir-repro /srv/elixir-data musl
... indexes v1.2.5 and v1.2.6 ...

⟩ docker run -d --name lxr-t -p 8080:80 -v ./data:/srv/elixir-data elixir-repro
⟩ curl -s -o /dev/null -w '%{http_code}\n' http://localhost:8080/musl/v1.2.6/diff/v1.2.5/crt/crt1.c
200

⟩ # file diffs themselves work

1. tree-diff warnings blame the wrong version

When the path is missing on one side of the diff, generate_diff_page()
renders a note per side. Both notes are built with
generate_warning(type, version) — the second one should pass
version_other but passes version again, so the note for the other side
always names the wrong version. Diffing v1.2.6 -> v1.2.5 over src/mq/x32
(a tree that only exists in v1.2.6), the missing side is v1.2.5:

⟩ curl -s http://localhost:8080/musl/v1.2.6/diff/v1.2.5/src/mq/x32 | grep -o 'Note: [^<]*'
Note: /src/mq/x32 does not exist in v1.2.6

⟩ # => wrong version: both generate_warning() calls get `version`

2. redirects drop the diff, and latest 500s

DiffResource.on_get() has two redirect cases — path ending in '/'
(canonical form, 301) and version == 'latest' (302 to the newest tag) —
and both build the target with stringify_source_path(), the URL builder
for the source view. The redirect therefore lands on /source/... and the
whole diff context (version_other, path) is lost:

⟩ curl -s -o /dev/null -D - http://localhost:8080/musl/v1.2.6/diff/v1.2.5/src/ | grep -i '^location'
location: /musl/v1.2.6/source/src

⟩ # => trailing slash redirects to /source: the diff context is lost

⟩ curl -s -o /dev/null -w '%{http_code}\n' http://localhost:8080/musl/latest/diff/v1.2.5/src/string/strlen.c
500

⟩ # apache log: TypeError: Query.get_latest_tag() missing 1 required
⟩ # positional argument: 'rc'  (the source path handles latest-rc too)

The latest case also crashes before redirecting: get_latest_tag() gained
an rc keyword in this PR (the source resource passes it), and the diff
resource still calls it bare — TypeError, so /musl/latest/diff/... is a
500. Last, stringify_diff_path() doesn't strip the leading '/' from the
path, so it emits // right after the version.

3. HTML error pages 500

The last commit of this PR makes sidebar.html unpack other_version_path
unconditionally, to highlight the compared version on diff pages.
get_error_page() builds its template context without that key, so
rendering any error page raises ValueError in the sidebar — unpacking an
undefined — and the original error is replaced by a 500. Trigger: any error
rendered as HTML, here a 400 from the API with Accept: text/html:

⟩ curl -s -o /dev/null -w '%{http_code}\n' -H 'Accept: text/html' http://localhost:8080/api/ident/musl/strlen
500

⟩ # apache log: ValueError: not enough values to unpack in sidebar.html line 31
⟩ # (`other_version_path`), masking the actual 400

Fixes for the three (cumulative, same file):

⟩ git apply ../pr396-1.patch ../pr396-2.patch ../pr396-3.patch
⟩ docker build -q -t elixir-repro -f docker/Dockerfile .
⟩ docker rm -f lxr-t
⟩ docker run -d --name lxr-t -p 8080:80 -v ./data:/srv/elixir-data elixir-repro
⟩ curl -s http://localhost:8080/musl/v1.2.6/diff/v1.2.5/src/mq/x32 | grep -o 'Note: [^<]*'
Note: /src/mq/x32 does not exist in v1.2.5

⟩ curl -s -o /dev/null -D - http://localhost:8080/musl/latest/diff/v1.2.5/src/ | grep -i '^location'
location: /musl/latest/diff/v1.2.5/src

⟩ curl -s -o /dev/null -w '%{http_code} %{url_effective}\n' -L http://localhost:8080/musl/latest/diff/v1.2.5/src/
200 http://localhost:8080/musl/v1.2.6/diff/v1.2.5/src

⟩ curl -s -o /dev/null -w '%{http_code}\n' -H 'Accept: text/html' http://localhost:8080/api/ident/musl/strlen
400

⟩ docker rm -f lxr-t
⟩ git checkout -q -f master