Merge pull request #148 from phongulus/phong/longest-common-prefix-fix

Fix longest common prefix

Author: yasunariw

lib/common.ml

@@ -61,11 +61,5 @@ let write_to_local_file ~data path =
   try Ok (Devkit.Files.save_as path (fun oc -> Printf.fprintf oc "%s" data))
   with exn -> fmt_error "%s" (Exn.to_string exn)
 
-let longest_common_prefix xs =
-  match xs with
-  | [] -> ""
-  | [ x ] -> x
-  | x :: _ -> List.sort (Fun.flip String.compare) xs |> List.hd |> Stre.common_prefix x |> String.sub x 0
-
 let sign_string_sha256 ~key ~basestring =
   Cstruct.of_string basestring |> Nocrypto.Hash.SHA256.hmac ~key:(Cstruct.of_string key) |> Hex.of_cstruct |> Hex.show

lib/slack_message.ml

@@ -133,16 +133,16 @@ let month = function
 let condense_file_changes files =
   match files with
   | [ f ] -> sprintf "_modified `%s` (+%d-%d)_" (escape_mrkdwn f.filename) f.additions f.deletions
-  | _ ->
-    let rec drop_last = function
-      | [ _ ] | [] -> [] (* Should raise when empty instead? *)
-      | v :: vs -> v :: drop_last vs
+  | [] -> "_no files modified_"
+  | first_file :: fl ->
+    let rec longest_prefix_of_two_lists l1 l2 =
+      match l1, l2 with
+      | e1 :: l1', e2 :: l2' when e1 = e2 -> e1 :: longest_prefix_of_two_lists l1' l2'
+      | _ -> []
     in
     let prefix_path =
-      List.map (fun f -> f.filename) files
-      |> Common.longest_common_prefix
-      |> String.split_on_char '/'
-      |> drop_last
+      List.map (fun f -> String.split_on_char '/' f.filename) fl
+      |> List.fold_left longest_prefix_of_two_lists (String.split_on_char '/' first_file.filename)
       |> String.concat "/"
     in
     sprintf "modified %d files%s" (List.length files) (if prefix_path = "" then "" else sprintf " in `%s/`" prefix_path)

test/dune

@@ -1,5 +1,5 @@
 (executables
- (names test github_link_test)
+ (names test github_link_test longest_prefix_test)
  (libraries lib devkit devkit.core extlib lwt.unix
    yojson)
  (preprocess
@@ -27,3 +27,8 @@
  (alias runtest)
  (action
    (run ./github_link_test.exe)))
+
+(rule
+ (alias runtest)
+ (action
+   (run ./longest_prefix_test.exe)))

test/longest_prefix_test.ml

@@ -0,0 +1,37 @@
+open Lib
+open Github_t
+open Slack_message
+
+let make_test_file filename =
+  {
+    sha = "something something";
+    filename;
+    status = "something something";
+    additions = 5;
+    deletions = 5;
+    changes = 10;
+    url = "test_link";
+  }
+
+let single_file = [ make_test_file "testdir1/testdir2/changed_file.txt" ]
+
+let multiple_files_same_dir =
+  List.map make_test_file
+    [
+      "testdir1/testdir2/changed_file2.txt";
+      "testdir1/testdir2/changed_file3.txt";
+      "testdir1/testdir2/changed_file1.txt";
+    ]
+
+let multiple_files_common_root_dir =
+  List.map make_test_file [ "testdir1/changed_file2.txt"; "backend/changed_file3.txt"; "changed_file1.txt" ]
+
+let multiple_files_common_dir =
+  List.map make_test_file
+    [ "backend/something/changed_file2.txt"; "backend/something/changed_file3.txt"; "backend/some/changed_file1.txt" ]
+
+let () =
+  assert (condense_file_changes single_file = "_modified `testdir1/testdir2/changed_file.txt` (+5-5)_");
+  assert (condense_file_changes multiple_files_same_dir = "modified 3 files in `testdir1/testdir2/`");
+  assert (condense_file_changes multiple_files_common_root_dir = "modified 3 files");
+  assert (condense_file_changes multiple_files_common_dir = "modified 3 files in `backend/`")