Fix longest common prefix

[phongulus]

Description of the task

As pointed out here, there's something wrong with the logic of longest_common_prefix. See the following output from utop:

utop # longest_common_prefix ["/test/paths/not_here"; "/test/path/here"; "/test/paths/here"];;
- : string = "/test/paths/not_here"

This function is currently used to collate file changes in a commit (e.g. x files changed in example/dir). It's been working so far since GitHub API seems to return file paths in lexicographic order (that's just from observation, I can't find this confirmed anywhere).

We should either:

• have longest_common_prefix return the actual longest prefix without assuming anything about the order in which files appear (currently implemented in this PR).
• assume that GitHub API always returns file paths in lexicographic order and remove the unneeded List.sort (commented out under the above).

How to test

make test

[yasunariw]

this still returns

utop # longest_common_prefix ["/test/paths/not_here"; "/test/path/here"; "/test/paths/here"];;
- : string = "/test/path"

but I expect it to return /test

[yasunariw]

can remove the commented out code?
also maybe the function name is too general for its intended usecase/expected behavior

[phongulus]

maybe we should remove this from lib/common.ml entirely and just write the logic here in lib/slack_message.ml where it's called?

[yasunariw]

let's do that. This whole part can be replaced I think:

let prefix_path =
      List.map (fun f -> f.filename) files
      |> Common.longest_common_prefix
      |> String.split_on_char '/'
      |> drop_last
      |> String.concat "/"

so as not to rely on assumptions about what happens to the function output like:

we always drop the last path element

[phongulus]

this still returns

utop # longest_common_prefix ["/test/paths/not_here"; "/test/path/here"; "/test/paths/here"];;
- : string = "/test/path"

but I expect it to return /test

Right, longest_common_prefix is only called in condense_file_changes in line 150 /lib/slack_message.ml. It think it's assumed there that we only have the common string prefix and not the common path prefix, since we always drop the last path element:

let prefix_path =
      List.map (fun f -> f.filename) files
      |> Common.longest_common_prefix
      |> String.split_on_char '/'
      |> drop_last
      |> String.concat "/"

[yasunariw]

the change looks good, thanks!
however if adding a test, I think a unit test would be more appropriate

[phongulus]

the change looks good, thanks! however if adding a test, I think a unit test would be more appropriate

@yasunariw I added some unit tests.

[yasunariw]

thanks, please remove the other test case compare.files_modified_common_directory in favor of the unit test, rebase and cleanup commits

[yasunariw]

lgtm, thanks!