refactor rename Diff to Line_diff and line_diff to of_contents

Three modules were named Diff: this one computing diffs, Resolvers.Diff reading them out of a Git store, and Ui.Diff rendering them. Each was correct in its own scope, but repo.ml converts Resolvers.Diff.line to Ui.Diff.line in adjacent lines, and nothing in the name distinguished the computation from the two layers around it. Line_diff says what this module does and what it does not: it matches whole lines as opaque units, with no word-level refinement. Within it, line_diff stuttered against the module name, so it becomes of_contents, which also names its arguments — two file contents rather than two paths or two hashes. Resolvers.Diff still includes it, so Resolvers.Diff.of_contents remains the access path used by the tests. Also removes a duplicated mode line that the documentation commit introduced at the top of this file.

Commit
fb61cbac415b0ffdc628dead9c5e5dbee2482191
Author
Claude Sonnet 4 <claude@anthropic.invalid>
Author date
Committer
Marius Peter <dev@marius-peter.com>
Committer date
Changed files
lib/diff.ml
index 953fc313..00000000 100644..000000
@@ -1,179 +0,0 @@
1 Removed: (* -*- mode: tuareg; -*- *)
2 Removed:
3 Removed: (* -*- mode: tuareg; -*- *)
4 Removed:
5 Removed: (** Line diffs between two blobs.
6 Removed:
7 Removed: Computes a longest-common-subsequence diff and groups the result into hunks
8 Removed: with the surrounding context, which is the shape a reader expects from
9 Removed: [git diff]. Large inputs fall back to a coarser result rather than spending
10 Removed: unbounded time and memory on the LCS matrix.
11 Removed:
12 Removed: This module produces data only. Rendering it is {!module:Ui.Diff}'s job.
13 Removed:
14 Removed: TODO: document the exact size threshold and the shape of the fallback, once
15 Removed: the limit is settled. *)
16 Removed:
17 Removed: type line_kind = Context | Addition | Deletion
18 Removed:
19 Removed: type line = {
20 Removed: kind : line_kind;
21 Removed: old_number : int option;
22 Removed: new_number : int option;
23 Removed: text : string;
24 Removed: }
25 Removed:
26 Removed: type hunk = {
27 Removed: old_start : int;
28 Removed: old_count : int;
29 Removed: new_start : int;
30 Removed: new_count : int;
31 Removed: lines : line list;
32 Removed: }
33 Removed:
34 Removed: type file = {
35 Removed: path : string;
36 Removed: old_hash : string option;
37 Removed: new_hash : string option;
38 Removed: old_mode : int option;
39 Removed: new_mode : int option;
40 Removed: binary : bool;
41 Removed: hunks : hunk list;
42 Removed: }
43 Removed:
44 Removed: let split_lines content =
45 Removed: match String.split_on_char '\n' content with
46 Removed: | [] -> []
47 Removed: | lines ->
48 Removed: if content = "" then []
49 Removed: else if String.ends_with ~suffix:"\n" content then
50 Removed: List.rev lines |> List.tl |> List.rev
51 Removed: else lines
52 Removed:
53 Removed: let line_diff old_content new_content =
54 Removed: let old_lines = Array.of_list (split_lines old_content) in
55 Removed: let new_lines = Array.of_list (split_lines new_content) in
56 Removed: let old_length = Array.length old_lines in
57 Removed: let new_length = Array.length new_lines in
58 Removed: let matrix_size = old_length * new_length in
59 Removed: let rec all_deletions index acc =
60 Removed: if index = old_length then List.rev acc
61 Removed: else
62 Removed: all_deletions (index + 1)
63 Removed: ({
64 Removed: kind = Deletion;
65 Removed: old_number = Some (index + 1);
66 Removed: new_number = None;
67 Removed: text = old_lines.(index);
68 Removed: }
69 Removed: :: acc)
70 Removed: in
71 Removed: let rec all_additions index acc =
72 Removed: if index = new_length then List.rev acc
73 Removed: else
74 Removed: all_additions (index + 1)
75 Removed: ({
76 Removed: kind = Addition;
77 Removed: old_number = None;
78 Removed: new_number = Some (index + 1);
79 Removed: text = new_lines.(index);
80 Removed: }
81 Removed: :: acc)
82 Removed: in
83 Removed: if matrix_size > 4_000_000 then all_deletions 0 [] @ all_additions 0 []
84 Removed: else
85 Removed: let lengths = Array.make_matrix (old_length + 1) (new_length + 1) 0 in
86 Removed: for old_index = old_length - 1 downto 0 do
87 Removed: for new_index = new_length - 1 downto 0 do
88 Removed: lengths.(old_index).(new_index) <-
89 Removed: (if old_lines.(old_index) = new_lines.(new_index) then
90 Removed: lengths.(old_index + 1).(new_index + 1) + 1
91 Removed: else
92 Removed: max
93 Removed: lengths.(old_index + 1).(new_index)
94 Removed: lengths.(old_index).(new_index + 1))
95 Removed: done
96 Removed: done;
97 Removed: let rec build old_index new_index acc =
98 Removed: if old_index = old_length then List.rev acc @ all_additions new_index []
99 Removed: else if new_index = new_length then
100 Removed: List.rev acc @ all_deletions old_index []
101 Removed: else if old_lines.(old_index) = new_lines.(new_index) then
102 Removed: build (old_index + 1) (new_index + 1)
103 Removed: ({
104 Removed: kind = Context;
105 Removed: old_number = Some (old_index + 1);
106 Removed: new_number = Some (new_index + 1);
107 Removed: text = old_lines.(old_index);
108 Removed: }
109 Removed: :: acc)
110 Removed: else if
111 Removed: lengths.(old_index + 1).(new_index)
112 Removed: >= lengths.(old_index).(new_index + 1)
113 Removed: then
114 Removed: build (old_index + 1) new_index
115 Removed: ({
116 Removed: kind = Deletion;
117 Removed: old_number = Some (old_index + 1);
118 Removed: new_number = None;
119 Removed: text = old_lines.(old_index);
120 Removed: }
121 Removed: :: acc)
122 Removed: else
123 Removed: build old_index (new_index + 1)
124 Removed: ({
125 Removed: kind = Addition;
126 Removed: old_number = None;
127 Removed: new_number = Some (new_index + 1);
128 Removed: text = new_lines.(new_index);
129 Removed: }
130 Removed: :: acc)
131 Removed: in
132 Removed: build 0 0 []
133 Removed:
134 Removed: let hunks ?(context = 3) lines =
135 Removed: let lines = Array.of_list lines in
136 Removed: let length = Array.length lines in
137 Removed: let changed =
138 Removed: Array.to_list (Array.mapi (fun index line -> (index, line.kind)) lines)
139 Removed: |> List.filter_map (function
140 Removed: | index, (Addition | Deletion) -> Some index
141 Removed: | _, Context -> None)
142 Removed: in
143 Removed: let ranges =
144 Removed: let add_range ranges index =
145 Removed: let first = max 0 (index - context) in
146 Removed: let last = min (length - 1) (index + context) in
147 Removed: match ranges with
148 Removed: | (range_first, range_last) :: rest when first <= range_last + 1 ->
149 Removed: (range_first, max range_last last) :: rest
150 Removed: | _ -> (first, last) :: ranges
151 Removed: in
152 Removed: List.fold_left add_range [] changed |> List.rev
153 Removed: in
154 Removed: let number_or_zero get_number slice =
155 Removed: List.find_map get_number slice |> Option.value ~default:0
156 Removed: in
157 Removed: let make_hunk (first, last) =
158 Removed: let rec slice index acc =
159 Removed: if index > last then List.rev acc
160 Removed: else slice (index + 1) (lines.(index) :: acc)
161 Removed: in
162 Removed: let lines = slice first [] in
163 Removed: {
164 Removed: old_start = number_or_zero (fun line -> line.old_number) lines;
165 Removed: old_count =
166 Removed: List.fold_left
167 Removed: (fun count line ->
168 Removed: if Option.is_some line.old_number then count + 1 else count)
169 Removed: 0 lines;
170 Removed: new_start = number_or_zero (fun line -> line.new_number) lines;
171 Removed: new_count =
172 Removed: List.fold_left
173 Removed: (fun count line ->
174 Removed: if Option.is_some line.new_number then count + 1 else count)
175 Removed: 0 lines;
176 Removed: lines;
177 Removed: }
178 Removed: in
179 Removed: List.map make_hunk ranges
lib/line_diff.ml
index 00000000..53e8bea7 000000..100644
@@ -0,0 +1,180 @@
1 Added: (* -*- mode: tuareg; -*- *)
2 Added:
3 Added: (** Line-oriented diffs between two blobs.
4 Added:
5 Added: Computes a longest-common-subsequence diff and groups the result into hunks
6 Added: with the surrounding context, which is the shape a reader expects from
7 Added: [git diff]. Large inputs fall back to a coarser result rather than spending
8 Added: unbounded time and memory on the LCS matrix.
9 Added:
10 Added: The module is named for the granularity it works at: whole lines, matched as
11 Added: opaque units, with no word- or character-level refinement.
12 Added:
13 Added: It produces data only. Rendering it is {!module:Ui.Diff}'s job.
14 Added:
15 Added: TODO: document the exact size threshold and the shape of the fallback, once
16 Added: the limit is settled. *)
17 Added:
18 Added: type line_kind = Context | Addition | Deletion
19 Added:
20 Added: type line = {
21 Added: kind : line_kind;
22 Added: old_number : int option;
23 Added: new_number : int option;
24 Added: text : string;
25 Added: }
26 Added:
27 Added: type hunk = {
28 Added: old_start : int;
29 Added: old_count : int;
30 Added: new_start : int;
31 Added: new_count : int;
32 Added: lines : line list;
33 Added: }
34 Added:
35 Added: type file = {
36 Added: path : string;
37 Added: old_hash : string option;
38 Added: new_hash : string option;
39 Added: old_mode : int option;
40 Added: new_mode : int option;
41 Added: binary : bool;
42 Added: hunks : hunk list;
43 Added: }
44 Added:
45 Added: let split_lines content =
46 Added: match String.split_on_char '\n' content with
47 Added: | [] -> []
48 Added: | lines ->
49 Added: if content = "" then []
50 Added: else if String.ends_with ~suffix:"\n" content then
51 Added: List.rev lines |> List.tl |> List.rev
52 Added: else lines
53 Added:
54 Added: let of_contents old_content new_content =
55 Added: let old_lines = Array.of_list (split_lines old_content) in
56 Added: let new_lines = Array.of_list (split_lines new_content) in
57 Added: let old_length = Array.length old_lines in
58 Added: let new_length = Array.length new_lines in
59 Added: let matrix_size = old_length * new_length in
60 Added: let rec all_deletions index acc =
61 Added: if index = old_length then List.rev acc
62 Added: else
63 Added: all_deletions (index + 1)
64 Added: ({
65 Added: kind = Deletion;
66 Added: old_number = Some (index + 1);
67 Added: new_number = None;
68 Added: text = old_lines.(index);
69 Added: }
70 Added: :: acc)
71 Added: in
72 Added: let rec all_additions index acc =
73 Added: if index = new_length then List.rev acc
74 Added: else
75 Added: all_additions (index + 1)
76 Added: ({
77 Added: kind = Addition;
78 Added: old_number = None;
79 Added: new_number = Some (index + 1);
80 Added: text = new_lines.(index);
81 Added: }
82 Added: :: acc)
83 Added: in
84 Added: if matrix_size > 4_000_000 then all_deletions 0 [] @ all_additions 0 []
85 Added: else
86 Added: let lengths = Array.make_matrix (old_length + 1) (new_length + 1) 0 in
87 Added: for old_index = old_length - 1 downto 0 do
88 Added: for new_index = new_length - 1 downto 0 do
89 Added: lengths.(old_index).(new_index) <-
90 Added: (if old_lines.(old_index) = new_lines.(new_index) then
91 Added: lengths.(old_index + 1).(new_index + 1) + 1
92 Added: else
93 Added: max
94 Added: lengths.(old_index + 1).(new_index)
95 Added: lengths.(old_index).(new_index + 1))
96 Added: done
97 Added: done;
98 Added: let rec build old_index new_index acc =
99 Added: if old_index = old_length then List.rev acc @ all_additions new_index []
100 Added: else if new_index = new_length then
101 Added: List.rev acc @ all_deletions old_index []
102 Added: else if old_lines.(old_index) = new_lines.(new_index) then
103 Added: build (old_index + 1) (new_index + 1)
104 Added: ({
105 Added: kind = Context;
106 Added: old_number = Some (old_index + 1);
107 Added: new_number = Some (new_index + 1);
108 Added: text = old_lines.(old_index);
109 Added: }
110 Added: :: acc)
111 Added: else if
112 Added: lengths.(old_index + 1).(new_index)
113 Added: >= lengths.(old_index).(new_index + 1)
114 Added: then
115 Added: build (old_index + 1) new_index
116 Added: ({
117 Added: kind = Deletion;
118 Added: old_number = Some (old_index + 1);
119 Added: new_number = None;
120 Added: text = old_lines.(old_index);
121 Added: }
122 Added: :: acc)
123 Added: else
124 Added: build old_index (new_index + 1)
125 Added: ({
126 Added: kind = Addition;
127 Added: old_number = None;
128 Added: new_number = Some (new_index + 1);
129 Added: text = new_lines.(new_index);
130 Added: }
131 Added: :: acc)
132 Added: in
133 Added: build 0 0 []
134 Added:
135 Added: let hunks ?(context = 3) lines =
136 Added: let lines = Array.of_list lines in
137 Added: let length = Array.length lines in
138 Added: let changed =
139 Added: Array.to_list (Array.mapi (fun index line -> (index, line.kind)) lines)
140 Added: |> List.filter_map (function
141 Added: | index, (Addition | Deletion) -> Some index
142 Added: | _, Context -> None)
143 Added: in
144 Added: let ranges =
145 Added: let add_range ranges index =
146 Added: let first = max 0 (index - context) in
147 Added: let last = min (length - 1) (index + context) in
148 Added: match ranges with
149 Added: | (range_first, range_last) :: rest when first <= range_last + 1 ->
150 Added: (range_first, max range_last last) :: rest
151 Added: | _ -> (first, last) :: ranges
152 Added: in
153 Added: List.fold_left add_range [] changed |> List.rev
154 Added: in
155 Added: let number_or_zero get_number slice =
156 Added: List.find_map get_number slice |> Option.value ~default:0
157 Added: in
158 Added: let make_hunk (first, last) =
159 Added: let rec slice index acc =
160 Added: if index > last then List.rev acc
161 Added: else slice (index + 1) (lines.(index) :: acc)
162 Added: in
163 Added: let lines = slice first [] in
164 Added: {
165 Added: old_start = number_or_zero (fun line -> line.old_number) lines;
166 Added: old_count =
167 Added: List.fold_left
168 Added: (fun count line ->
169 Added: if Option.is_some line.old_number then count + 1 else count)
170 Added: 0 lines;
171 Added: new_start = number_or_zero (fun line -> line.new_number) lines;
172 Added: new_count =
173 Added: List.fold_left
174 Added: (fun count line ->
175 Added: if Option.is_some line.new_number then count + 1 else count)
176 Added: 0 lines;
177 Added: lines;
178 Added: }
179 Added: in
180 Added: List.map make_hunk ranges
lib/resolvers.ml
index 1378bc14..b054a901 100644..100644
@@ -601,7 +601,7 @@
601 601
602 602 module Diff = struct
603 603 module Path_map = Map.Make (String)
604 Removed: include Diff
604 Added: include Line_diff
605 605
606 606 type tree_file = { hash : string; perm : Git.Tree.perm }
607 607
@@ -687,7 +687,9 @@
687 687 binary;
688 688 hunks =
689 689 (if binary then []
690 Removed: else Diff.line_diff old_content new_content |> Diff.hunks);
690 Added: else
691 Added: Line_diff.of_contents old_content new_content
692 Added: |> Line_diff.hunks);
691 693 }
692 694 in
693 695 build (file :: files) rest
lib/resolvers.mli
index c4ae683c..b7202c93 100644..100644
@@ -120,7 +120,7 @@
120 120 (** {1 Diffs} *)
121 121
122 122 module Diff : sig
123 Removed: include module type of Diff
123 Added: include module type of Line_diff
124 124
125 125 val of_commit : repository -> Commit.t -> (file list, error) Lwt_result.t
126 126 end
test/test_diff.ml
index f1c79dfc..f9c77348 100644..100644
@@ -4,7 +4,7 @@
4 4
5 5 let test_line_diff () =
6 6 let open Ogit.Resolvers.Diff in
7 Removed: match line_diff "first\nold\nlast\n" "first\nnew\nlast\n" with
7 Added: match of_contents "first\nold\nlast\n" "first\nnew\nlast\n" with
8 8 | [ ctx1; del; add; ctx2 ] ->
9 9 Alcotest.(check string) "ctx1 text" "first" ctx1.text;
10 10 Alcotest.(check string) "del text" "old" del.text;
@@ -14,7 +14,7 @@
14 14
15 15 let test_empty_vs_empty () =
16 16 let open Ogit.Resolvers.Diff in
17 Removed: let lines = line_diff "" "" in
17 Added: let lines = of_contents "" "" in
18 18 Alcotest.(check int) "no lines" 0 (List.length lines);
19 19 let hunks = hunks lines in
20 20 Alcotest.(check int) "no hunks" 0 (List.length hunks)
@@ -22,7 +22,7 @@
22 22 let test_identical_content () =
23 23 let open Ogit.Resolvers.Diff in
24 24 let content = "line1\nline2\nline3\n" in
25 Removed: let lines = line_diff content content in
25 Added: let lines = of_contents content content in
26 26 List.iter
27 27 (fun line -> Alcotest.(check bool) "all context" true (line.kind = Context))
28 28 lines;
@@ -31,7 +31,7 @@
31 31
32 32 let test_entirely_new_file () =
33 33 let open Ogit.Resolvers.Diff in
34 Removed: let lines = line_diff "" "new1\nnew2\n" in
34 Added: let lines = of_contents "" "new1\nnew2\n" in
35 35 Alcotest.(check int) "2 additions" 2 (List.length lines);
36 36 List.iter
37 37 (fun line ->
@@ -40,7 +40,7 @@
40 40
41 41 let test_entirely_deleted_file () =
42 42 let open Ogit.Resolvers.Diff in
43 Removed: let lines = line_diff "old1\nold2\n" "" in
43 Added: let lines = of_contents "old1\nold2\n" "" in
44 44 Alcotest.(check int) "2 deletions" 2 (List.length lines);
45 45 List.iter
46 46 (fun line ->
@@ -49,8 +49,8 @@
49 49
50 50 let test_trailing_newline_handling () =
51 51 let open Ogit.Resolvers.Diff in
52 Removed: let with_newline = line_diff "a\n" "a\n" in
53 Removed: let without_newline = line_diff "a" "a" in
52 Added: let with_newline = of_contents "a\n" "a\n" in
53 Added: let without_newline = of_contents "a" "a" in
54 54 Alcotest.(check int) "with newline: 1 line" 1 (List.length with_newline);
55 55 Alcotest.(check int) "without newline: 1 line" 1 (List.length without_newline);
56 56 Alcotest.(check string) "text matches" "a" (List.hd with_newline).text;
@@ -58,7 +58,7 @@
58 58
59 59 let test_no_trailing_newline_diff () =
60 60 let open Ogit.Resolvers.Diff in
61 Removed: let lines = line_diff "a\nb" "a\nc" in
61 Added: let lines = of_contents "a\nb" "a\nc" in
62 62 Alcotest.(check int) "3 lines" 3 (List.length lines);
63 63 let kinds = List.map (fun l -> l.kind) lines in
64 64 Alcotest.(check bool)
@@ -73,7 +73,7 @@
73 73 in
74 74 match
75 75 hunks
76 Removed: (line_diff
76 Added: (of_contents
77 77 (String.concat "\n" old_content)
78 78 (String.concat "\n" new_content))
79 79 with
@@ -92,7 +92,7 @@
92 92 in
93 93 let result =
94 94 hunks
95 Removed: (line_diff
95 Added: (of_contents
96 96 (String.concat "\n" old_content)
97 97 (String.concat "\n" new_content))
98 98 in
@@ -105,7 +105,7 @@
105 105 let new_content =
106 106 String.concat "\n" (List.init 2001 (fun i -> string_of_int (i + 1000)))
107 107 in
108 Removed: let lines = line_diff old_content new_content in
108 Added: let lines = of_contents old_content new_content in
109 109 let has_deletions = List.exists (fun l -> l.kind = Deletion) lines in
110 110 let has_additions = List.exists (fun l -> l.kind = Addition) lines in
111 111 Alcotest.(check bool) "has deletions" true has_deletions;