From bd6ba784b32f6f7bc56ed71d1f699035019fffb6 Mon Sep 17 00:00:00 2001
From: Sebastian Martinez <me@sebastinez.dev>
Date: Mon, 25 Sep 2023 11:09:52 +0200
Subject: [PATCH] Improve binary detection

libgit2 optimizes around not loading the content, when there's no
content callbacks configured.

So we aren't able to get a clear answer from radicle-surf if a file in a
diff is a binary or not.

A simple way without doing additional API requests is to check if the
file type is a executable and if there are no hunks (since we don't get
any deltas in binaries), this is a strong
indicator for a binary.

Signed-off-by: Sebastian Martinez <me@sebastinez.dev>
---
 httpd-client/index.ts               |  2 ++
 httpd-client/lib/project/commit.ts  | 15 ++++++++++++++-
 src/views/projects/Changeset.svelte | 23 +++++++++++++++++++----
 tests/e2e/project/commit.spec.ts    |  7 +++++++
 4 files changed, 42 insertions(+), 5 deletions(-)

diff --git a/httpd-client/index.ts b/httpd-client/index.ts
index a9ff5a755..2590179e2 100644
--- a/httpd-client/index.ts
+++ b/httpd-client/index.ts
@@ -13,6 +13,7 @@ import type {
   CommitHeader,
   Diff,
   DiffContent,
+  DiffFile,
   HunkLine,
 } from "./lib/project/commit.js";
 import type { Issue, IssueState } from "./lib/project/issue.js";
@@ -46,6 +47,7 @@ export type {
   CommitHeader,
   Diff,
   DiffContent,
+  DiffFile,
   DiffResponse,
   HunkLine,
   Issue,
diff --git a/httpd-client/lib/project/commit.ts b/httpd-client/lib/project/commit.ts
index e0dbcc132..ed0043c09 100644
--- a/httpd-client/lib/project/commit.ts
+++ b/httpd-client/lib/project/commit.ts
@@ -1,5 +1,14 @@
 import type { z } from "zod";
-export type { Commits, HunkLine, Commit, Diff, CommitHeader, DiffContent };
+export type {
+  Commits,
+  HunkLine,
+  Hunks,
+  Commit,
+  Diff,
+  CommitHeader,
+  DiffFile,
+  DiffContent,
+};
 
 import { array, literal, number, object, optional, string, union } from "zod";
 export { commitHeaderSchema, diffSchema, commitSchema, commitsSchema };
@@ -36,6 +45,8 @@ const deletionHunkLineSchema = object({
   type: literal("deletion"),
 });
 
+type DiffFile = z.infer<typeof diffFileSchema>;
+
 const diffFileSchema = object({
   oid: string(),
   mode: union([
@@ -64,6 +75,8 @@ const hunkLineSchema = union([
   contextHunkLineSchema,
 ]);
 
+type Hunks = z.infer<typeof changesetHunkSchema>;
+
 const changesetHunkSchema = object({
   header: string(),
   lines: array(hunkLineSchema),
diff --git a/src/views/projects/Changeset.svelte b/src/views/projects/Changeset.svelte
index 39abf3a48..3e29ae8c0 100644
--- a/src/views/projects/Changeset.svelte
+++ b/src/views/projects/Changeset.svelte
@@ -1,5 +1,11 @@
 <script lang="ts">
-  import type { BaseUrl, CommitBlob, Diff } from "@httpd-client";
+  import type {
+    BaseUrl,
+    Diff,
+    CommitBlob,
+    DiffContent,
+    DiffFile,
+  } from "@httpd-client";
 
   import { pluralize } from "@app/lib/pluralize";
 
@@ -42,6 +48,15 @@
     }
     return s.join(", ");
   };
+
+  // Since libgit2 optimizes around not loading the content, when no content callbacks are configured,
+  // and we don't want to loop over all diffs in radicle-surf to get a correct answer.
+  // We assume that a `blobExecutable` file with no hunks is a binary file.
+  function getFileType(diff: DiffContent, file: DiffFile): DiffContent["type"] {
+    return file.mode === "blobExecutable" && diff.hunks.length === 0
+      ? "binary"
+      : diff.type;
+  }
 </script>
 
 <style>
@@ -77,7 +92,7 @@
       {baseUrl}
       revision={revision ?? files[file.new.oid].lastCommit.id}
       filePath={file.path}
-      fileDiff={file.diff}
+      fileDiff={{ ...file.diff, type: getFileType(file.diff, file.new) }}
       headerBadgeCaption="added" />
   {/each}
   {#each diff.deleted as file}
@@ -86,7 +101,7 @@
       {baseUrl}
       revision={revision ?? files[file.old.oid].lastCommit.id}
       filePath={file.path}
-      fileDiff={file.diff}
+      fileDiff={{ ...file.diff, type: getFileType(file.diff, file.old) }}
       headerBadgeCaption="deleted" />
   {/each}
   {#each diff.modified as file}
@@ -95,7 +110,7 @@
       {baseUrl}
       revision={revision ?? files[file.new.oid].lastCommit.id}
       filePath={file.path}
-      fileDiff={file.diff} />
+      fileDiff={{ ...file.diff, type: getFileType(file.diff, file.new) }} />
   {/each}
   {#each diff.moved as file}
     {#if file.diff}
diff --git a/tests/e2e/project/commit.spec.ts b/tests/e2e/project/commit.spec.ts
index f43b685aa..8533ff563 100644
--- a/tests/e2e/project/commit.spec.ts
+++ b/tests/e2e/project/commit.spec.ts
@@ -103,6 +103,13 @@ test("copied file", async ({ page }) => {
   await expect(page.getByText("copied", { exact: true })).toBeVisible();
 });
 
+test("binary file detection in diffs", async ({ page }) => {
+  await page.goto(
+    `${sourceBrowsingUrl}/commits/335dd6dc89b535a4a31e9422c803199bb6b0a09a`,
+  );
+  await expect(page.getByText("Binary file")).toBeVisible();
+});
+
 test("navigation to source tree at specific revision", async ({ page }) => {
   await page.goto(
     `${sourceBrowsingUrl}/commits/0801aceeab500033f8d608778218657bd626ef73`,