Apply review comments

lib/livebook/file_system.ex

@@ -247,18 +247,6 @@ defprotocol Livebook.FileSystem do
   @spec external_metadata(t()) :: %{name: String.t(), error_field: String.t()}
   def external_metadata(file_system)
 
-  @doc """
-  Returns if file system is mountable.
-  """
-  @spec mountable?(t()) :: boolean()
-  def mountable?(file_system)
-
-  @doc """
-  Returns if file system is already mounted.
-  """
-  @spec mounted?(t()) :: boolean()
-  def mounted?(file_system)
-
   @doc """
   Mounts the given file system to be used furthermore.
   """
@@ -266,14 +254,8 @@ defprotocol Livebook.FileSystem do
   def mount(file_system)
 
   @doc """
-  Remounts the given file system to be used furthermore.
+  Unmounts the given file system from being used.
   """
-  @spec remount(t()) :: :ok | {:error, error()}
-  def remount(file_system)
-
-  @doc """
-  Umounts the given file system from being used.
-  """
-  @spec umount(t()) :: :ok | {:error, error()}
-  def umount(file_system)
+  @spec unmount(t()) :: :ok | {:error, error()}
+  def unmount(file_system)
 end

lib/livebook/file_system/git.ex

@@ -154,23 +154,15 @@ defimpl Livebook.FileSystem, for: Livebook.FileSystem.Git do
     %{name: file_system.repo_url, error_field: "repo_url"}
   end
 
-  def mountable?(_file_system), do: true
-
-  def mounted?(file_system) do
-    file_system
-    |> FileSystem.Git.git_dir()
-    |> File.exists?()
-  end
-
   def mount(file_system) do
-    FileSystem.Git.Client.init(file_system)
+    if mounted?(file_system) do
+      FileSystem.Git.Client.fetch(file_system)
+    else
+      FileSystem.Git.Client.init(file_system)
+    end
   end
 
-  def remount(file_system) do
-    FileSystem.Git.Client.fetch(file_system)
-  end
-
-  def umount(file_system) do
+  def unmount(file_system) do
     path = FileSystem.Git.git_dir(file_system)
     key_path = FileSystem.Git.key_path(file_system)
 
@@ -183,4 +175,10 @@ defimpl Livebook.FileSystem, for: Livebook.FileSystem.Git do
       {:error, reason, _file} -> FileSystem.Utils.posix_error(reason)
     end
   end
+
+  defp mounted?(file_system) do
+    file_system
+    |> FileSystem.Git.git_dir()
+    |> File.exists?()
+  end
 end

lib/livebook/file_system/git/client.ex

@@ -118,7 +118,7 @@ defmodule Livebook.FileSystem.Git.Client do
     with {:ok, git} <- fetch_executable("git") do
       case System.cmd(git, args, cmd_opts(git_dir, opts)) do
         {result, 0} -> {:ok, result}
-        {error, _} -> {:error, normalize_error_message(error)}
+        {error, _} -> {:error, String.trim(error)}
       end
     end
   end
@@ -168,25 +168,6 @@ defmodule Livebook.FileSystem.Git.Client do
     result
   end
 
-  defp normalize_error_message("Cloning" <> _ = error) do
-    [_, error] = String.split(error, "Permission denied", trim: true)
-
-    ("Permission denied" <> error)
-    |> String.replace("\r\n", "\s")
-    |> String.replace("\n\n", "\s")
-    |> String.replace("\n", "\s")
-    |> String.replace("fatal: ", "")
-    |> String.trim()
-  end
-
-  defp normalize_error_message(error) do
-    [error] = String.split(error, "fatal: ", trim: true)
-    # avoid MatchError when it has only one part
-    [error | _] = String.split(error, "\n", trim: true)
-
-    String.trim(error)
-  end
-
   defp normalize_dir_path(<<"blob ", path::binary>>), do: "/" <> path
   defp normalize_dir_path(<<"tree ", path::binary>>), do: "/" <> path <> "/"
 

lib/livebook/file_system/local.ex

@@ -287,13 +287,7 @@ defimpl Livebook.FileSystem, for: Livebook.FileSystem.Local do
 
   def external_metadata(_file_system), do: raise("not implemented")
 
-  def mountable?(_file_system), do: false
+  def mount(_file_system), do: :ok
 
-  def mounted?(_file_system), do: false
-
-  def mount(_file_system), do: raise("not implemented")
-
-  def remount(_file_system), do: raise("not implemented")
-
-  def umount(_file_system), do: raise("not implemented")
+  def unmount(_file_system), do: :ok
 end

lib/livebook/file_system/mounter.ex

@@ -5,13 +5,19 @@ defmodule Livebook.FileSystem.Mounter do
   alias Livebook.{FileSystem, Hubs}
 
   @name __MODULE__
-  @loop_delay 60 * 60
+  @loop_delay to_timeout(hour: 1)
 
   def start_link(opts \\ []) do
     {name, opts} = Keyword.pop(opts, :name, @name)
     GenServer.start_link(__MODULE__, opts, name: name)
   end
 
+  if Mix.env() == :test do
+    def subscribe(hub_id) do
+      Phoenix.PubSub.subscribe(Livebook.PubSub, "file_systems:#{hub_id}")
+    end
+  end
+
   @impl GenServer
   def init(opts) do
     loop_delay = Keyword.get(opts, :loop_delay, @loop_delay)
@@ -21,12 +27,14 @@ defmodule Livebook.FileSystem.Mounter do
   @impl GenServer
   def handle_continue(:boot, state) do
     Hubs.Broadcasts.subscribe([:connection, :crud, :file_systems])
-    {:noreply, init_hub_file_systems(state, Hubs.Personal.id())}
+    Process.send_after(self(), :remount, state.loop_delay)
+
+    {:noreply, mount_file_systems(state, Hubs.Personal.id())}
   end
 
   @impl GenServer
   def handle_info({:hub_connected, hub_id}, state) do
-    {:noreply, init_hub_file_systems(state, hub_id)}
+    {:noreply, mount_file_systems(state, hub_id)}
   end
 
   def handle_info({:file_system_created, file_system}, state) do
@@ -38,102 +46,60 @@ defmodule Livebook.FileSystem.Mounter do
   end
 
   def handle_info({:file_system_deleted, file_system}, state) do
-    {:noreply, umount_file_system(state, file_system)}
+    {:noreply, unmount_file_system(state, file_system)}
   end
 
   def handle_info({:hub_changed, hub_id}, state) do
-    if not Map.has_key?(state.hubs, hub_id) do
-      {:noreply, init_hub_file_systems(state, hub_id)}
-    else
-      {:noreply, state}
-    end
+    {:noreply, mount_file_systems(state, hub_id)}
   end
 
   def handle_info({:hub_deleted, hub_id}, state) do
-    {:noreply, umount_hub_file_systems(state, hub_id)}
+    {:noreply, unmount_file_systems(state, hub_id)}
   end
 
-  def handle_info({:update_file_system, file_system}, state) do
-    {:noreply, update_file_system(state, file_system)}
+  def handle_info(:remount, state) do
+    Process.send_after(self(), :remount, state.loop_delay)
+    {:noreply, remount_file_systems(state)}
   end
 
   def handle_info(_message, state), do: {:noreply, state}
 
-  defp init_hub_file_systems(state, hub_id) do
+  defp mount_file_systems(state, hub_id) do
     case Hubs.fetch_hub(hub_id) do
       {:ok, hub} ->
-        file_systems = get_mountable_file_systems(hub)
-        dispatch_file_systems(:file_system_created, file_systems)
-        %{state | hubs: Map.put(state.hubs, hub_id, %{file_systems: [], versions: %{}})}
+        file_systems = Hubs.get_file_systems(hub, hub_only: true)
+        state = put_hub(state, hub.id)
+        Enum.reduce(file_systems, state, &mount_file_system(&2, &1))
 
       :error ->
-        send(self(), {:hub_deleted, hub_id})
         state
     end
   end
 
-  defp umount_hub_file_systems(state, hub_id) do
+  defp remount_file_systems(state) do
+    Enum.reduce(state.hubs, state, fn {hub_id, hub_data}, acc ->
+      case Hubs.fetch_hub(hub_id) do
+        {:ok, _} -> Enum.reduce(hub_data.file_systems, acc, &update_file_system(&2, &1))
+        :error -> unmount_file_systems(acc, hub_id)
+      end
+    end)
+  end
+
+  defp unmount_file_systems(state, hub_id) do
     if metadata = state.hubs[hub_id] do
-      state = Enum.reduce(metadata.file_systems, state, &umount_file_system(&2, &1))
-      %{state | hubs: Map.delete(state.hubs, hub_id)}
+      metadata.file_systems
+      |> Enum.reduce(state, &unmount_file_system(&2, &1))
+      |> remove_hub(hub_id)
     else
       state
     end
   end
 
-  def get_mountable_file_systems(hub) do
-    hub
-    |> Hubs.get_file_systems(hub_only: true)
-    |> Enum.filter(&FileSystem.mountable?/1)
-  end
-
-  defp dispatch_file_systems(event, file_systems) do
-    for file_system <- file_systems do
-      send(self(), {event, file_system})
-    end
-  end
-
-  defp updatable?(state, file_system) do
-    case Map.get(state.hubs, file_system.hub_id) do
-      %{versions: versions} ->
-        if version = versions[file_system.id] do
-          :erlang.phash2(file_system) != version
-        else
-          false
-        end
-
-      _otherwise ->
-        false
-    end
-  end
-
   defp mount_file_system(state, file_system) do
-    cond do
-      not Map.has_key?(state.hubs, file_system.hub_id) ->
-        init_hub_file_systems(state, file_system.hub_id)
-
-      FileSystem.mountable?(file_system) and not FileSystem.mounted?(file_system) ->
-        do_mount_file_system(state, file_system)
-
-      true ->
-        state
-    end
-  end
-
-  defp do_mount_file_system(state, file_system) do
     case FileSystem.mount(file_system) do
       :ok ->
-        Hubs.Broadcasts.file_system_mounted(file_system)
-        Process.send_after(self(), {:update_file_system, file_system}, state.loop_delay)
-        metadata = Map.fetch!(state.hubs, file_system.hub_id)
-
-        metadata = %{
-          metadata
-          | file_systems: [file_system | metadata.file_systems],
-            versions: Map.put_new(metadata.versions, file_system.id, :erlang.phash2(file_system))
-        }
-
-        %{state | hubs: Map.replace!(state.hubs, file_system.hub_id, metadata)}
+        broadcast({:file_system_mounted, file_system})
+        put_hub_file_system(state, file_system)
 
       {:error, _reason} ->
         state
@@ -141,66 +107,70 @@ defmodule Livebook.FileSystem.Mounter do
   end
 
   defp update_file_system(state, file_system) do
-    cond do
-      not Map.has_key?(state.hubs, file_system.hub_id) ->
-        init_hub_file_systems(state, file_system.hub_id)
-
-      FileSystem.mountable?(file_system) and FileSystem.mounted?(file_system) and
-          updatable?(state, file_system) ->
-        do_update_file_system(state, file_system)
-
-      true ->
-        state
-    end
-  end
-
-  defp do_update_file_system(state, file_system) do
-    case FileSystem.remount(file_system) do
+    case FileSystem.mount(file_system) do
       :ok ->
-        Hubs.Broadcasts.file_system_mounted(file_system)
-        Process.send_after(self(), {:update_file_system, file_system}, state.loop_delay)
-        metadata = Map.fetch!(state.hubs, file_system.hub_id)
-
-        metadata = %{
-          metadata
-          | file_systems: [
-              file_system | Enum.reject(metadata.file_systems, &(&1.id == file_system.id))
-            ],
-            versions: Map.replace!(metadata.versions, file_system.id, :erlang.phash2(file_system))
-        }
-
-        %{state | hubs: Map.replace!(state.hubs, file_system.hub_id, metadata)}
+        broadcast({:file_system_mounted, file_system})
+        put_hub_file_system(state, file_system)
 
       {:error, _reason} ->
         state
     end
   end
 
-  defp umount_file_system(state, file_system) do
-    if Map.has_key?(state.hubs, file_system.hub_id) and FileSystem.mountable?(file_system) and
-         FileSystem.mounted?(file_system) do
-      do_umount_file_system(state, file_system)
+  defp unmount_file_system(state, file_system) do
+    case FileSystem.unmount(file_system) do
+      :ok ->
+        broadcast({:file_system_unmounted, file_system})
+        remove_hub_file_system(state, file_system)
+
+      {:error, _reason} ->
+        state
+    end
+  end
+
+  defp put_hub_file_system(state, file_system) do
+    if state.hubs[file_system.hub_id] do
+      update_in(state.hubs[file_system.hub_id], fn metadata ->
+        metadata
+        |> remove_file_system(file_system)
+        |> put_file_system(file_system)
+      end)
     else
       state
     end
   end
 
-  defp do_umount_file_system(state, file_system) do
-    case FileSystem.umount(file_system) do
-      :ok ->
-        Hubs.Broadcasts.file_system_umounted(file_system)
-        metadata = Map.fetch!(state.hubs, file_system.hub_id)
-
-        metadata = %{
-          metadata
-          | file_systems: Enum.reject(metadata.file_systems, &(&1.id == file_system.id)),
-            versions: Map.delete(metadata.versions, file_system.id)
-        }
-
-        %{state | hubs: Map.replace!(state.hubs, file_system.hub_id, metadata)}
-
-      {:error, _reason} ->
-        state
+  defp remove_hub_file_system(state, file_system) do
+    if state.hubs[file_system.hub_id] do
+      update_in(state.hubs[file_system.hub_id], &remove_file_system(&1, file_system))
+    else
+      state
     end
   end
+
+  defp put_hub(state, hub_id) do
+    update_in(state.hubs, &Map.put_new(&1, hub_id, %{file_systems: []}))
+  end
+
+  defp put_file_system(hub_data, file_system) do
+    hub_data = remove_file_system(hub_data, file_system)
+    put_in(hub_data.file_systems, [file_system | hub_data.file_systems])
+  end
+
+  defp remove_hub(state, hub_id) do
+    update_in(state.hubs, &Map.delete(&1, hub_id))
+  end
+
+  defp remove_file_system(hub_data, file_system) do
+    file_systems = Enum.reject(hub_data.file_systems, &(&1.id == file_system.id))
+    put_in(hub_data.file_systems, file_systems)
+  end
+
+  if Mix.env() == :test do
+    defp broadcast({_, %{external_id: _, hub_id: id}} = message) do
+      Phoenix.PubSub.broadcast(Livebook.PubSub, "file_systems:#{id}", message)
+    end
+  else
+    defp broadcast(_), do: :ok
+  end
 end

lib/livebook/file_system/s3.ex

@@ -450,13 +450,7 @@ defimpl Livebook.FileSystem, for: Livebook.FileSystem.S3 do
     %{name: file_system.bucket_url, error_field: "bucket_url"}
   end
 
-  def mountable?(_file_system), do: false
+  def mount(_file_system), do: :ok
 
-  def mounted?(_file_system), do: false
-
-  def mount(_file_system), do: raise("not implemented")
-
-  def remount(_file_system), do: raise("not implemented")
-
-  def umount(_file_system), do: raise("not implemented")
+  def unmount(_file_system), do: :ok
 end

lib/livebook/hubs/broadcasts.ex

@@ -36,8 +36,6 @@ defmodule Livebook.Hubs.Broadcasts do
     * `{:file_system_created, FileSystem.t()}`
     * `{:file_system_updated, FileSystem.t()}`
     * `{:file_system_deleted, FileSystem.t()}`
-    * `{:file_system_mounted, FileSystem.t()}`
-    * `{:file_system_umounted, FileSystem.t()}`
 
   """
   @spec subscribe(atom() | list(atom())) :: :ok | {:error, term()}
@@ -155,24 +153,6 @@ defmodule Livebook.Hubs.Broadcasts do
     broadcast(@file_systems_topic, {:file_system_deleted, file_system})
   end
 
-  @mountable_file_systems [FileSystem.Git]
-
-  @doc """
-  Broadcasts under `#{@file_systems_topic}` topic when a file system is mounted.
-  """
-  @spec file_system_mounted(FileSystem.t()) :: broadcast()
-  def file_system_mounted(%struct{} = file_system) when struct in @mountable_file_systems do
-    broadcast(@file_systems_topic, {:file_system_mounted, file_system})
-  end
-
-  @doc """
-  Broadcasts under `#{@file_systems_topic}` topic when a file system is umounted.
-  """
-  @spec file_system_umounted(FileSystem.t()) :: broadcast()
-  def file_system_umounted(%struct{} = file_system) when struct in @mountable_file_systems do
-    broadcast(@file_systems_topic, {:file_system_umounted, file_system})
-  end
-
   defp broadcast(topic, message) do
     Phoenix.PubSub.broadcast(Livebook.PubSub, topic, message)
   end

lib/livebook_web/live/hub/file_system_form_component.ex

@@ -208,11 +208,10 @@ defmodule LivebookWeb.Hub.FileSystemFormComponent do
   defp check_file_system_connectivity(file_system, socket) do
     default_path = FileSystem.default_path(file_system)
 
-    with :ok <- init_file_system(file_system, socket),
+    with :ok <- FileSystem.mount(file_system),
          {:ok, _} <- FileSystem.list(file_system, default_path, false) do
-      if FileSystem.mountable?(file_system) and FileSystem.mounted?(file_system) and
-           socket.assigns.mode == :new do
-        FileSystem.umount(file_system)
+      if socket.assigns.mode == :new do
+        FileSystem.unmount(file_system)
       end
 
       :ok
@@ -221,23 +220,6 @@ defmodule LivebookWeb.Hub.FileSystemFormComponent do
     end
   end
 
-  defp init_file_system(file_system, socket) do
-    cond do
-      FileSystem.mountable?(file_system) and not FileSystem.mounted?(file_system) and
-          socket.assigns.mode == :edit ->
-        {:error, "repository not found"}
-
-      FileSystem.mountable?(file_system) and FileSystem.mounted?(file_system) ->
-        FileSystem.remount(file_system)
-
-      FileSystem.mountable?(file_system) and not FileSystem.mounted?(file_system) ->
-        FileSystem.mount(file_system)
-
-      true ->
-        :ok
-    end
-  end
-
   defp save_file_system(file_system, changeset, socket) do
     result =
       case socket.assigns.mode do

test/livebook/file_system/git_test.exs

@@ -53,7 +53,7 @@ defmodule Livebook.FileSystem.GitTest do
     @describetag init: true
     test "returns an error when a nonexistent key is given", %{file_system: file_system} do
       assert FileSystem.read(file_system, "/another_file.txt") ==
-               {:error, "path 'another_file.txt' does not exist in 'main'"}
+               {:error, "fatal: path 'another_file.txt' does not exist in 'main'"}
     end
 
     test "returns object contents under the given key", %{file_system: file_system} do
@@ -122,7 +122,7 @@ defmodule Livebook.FileSystem.GitTest do
     end
 
     test "returns error with invalid path", %{file_system: file_system} do
-      assert {:error, "../../.bashrc: '../../.bashrc' is outside repository at" <> _} =
+      assert {:error, "fatal: ../../.bashrc: '../../.bashrc' is outside repository at" <> _} =
                FileSystem.exists?(file_system, "../../.bashrc")
     end
   end

test/livebook_teams/web/hub/edit_live_test.exs

@@ -14,6 +14,12 @@ defmodule LivebookWeb.Integration.Hub.EditLiveTest do
   describe "user" do
     @describetag teams_for: :user
 
+    setup %{team: team} = tags do
+      if tags[:git] do
+        Livebook.FileSystem.Mounter.subscribe(team.id)
+      end
+    end
+
     test "updates the hub", %{conn: conn, team: team} do
       {:ok, view, _html} = live(conn, ~p"/hub/#{team.id}")
       attrs = %{"hub_emoji" => "🐈"}
@@ -304,8 +310,7 @@ defmodule LivebookWeb.Integration.Hub.EditLiveTest do
     @tag :git
     test "updates existing Git file system",
          %{conn: conn, team: team, node: node, org_key: org_key} do
-      file_system = build(:fs_git)
-      file_system = TeamsRPC.create_file_system(node, team, org_key, file_system)
+      file_system = TeamsRPC.create_file_system(node, team, org_key, build(:fs_git))
       assert_receive {:file_system_created, %Livebook.FileSystem.Git{} = ^file_system}
       assert_receive {:file_system_mounted, ^file_system}, 10_000
 
@@ -384,8 +389,7 @@ defmodule LivebookWeb.Integration.Hub.EditLiveTest do
     @tag :git
     test "detaches existing Git file system",
          %{conn: conn, team: team, node: node, org_key: org_key} do
-      file_system = build(:fs_git)
-      file_system = TeamsRPC.create_file_system(node, team, org_key, file_system)
+      file_system = TeamsRPC.create_file_system(node, team, org_key, build(:fs_git))
       assert_receive {:file_system_created, %Livebook.FileSystem.Git{} = ^file_system}
       assert_receive {:file_system_mounted, ^file_system}, 10_000
 
@@ -402,7 +406,7 @@ defmodule LivebookWeb.Integration.Hub.EditLiveTest do
       render_confirm(view)
 
       assert_receive {:file_system_deleted, ^file_system}
-      assert_receive {:file_system_umounted, ^file_system}, 10_000
+      assert_receive {:file_system_unmounted, ^file_system}, 10_000
 
       assert_patch(view, "/hub/#{team.id}")
       assert render(view) =~ "File storage deleted successfully"

test/livebook_teams/web/open_live_test.exs

@@ -19,6 +19,7 @@ defmodule LivebookWeb.Integration.OpenLiveTest do
     @describetag :git
 
     setup %{test: test, team: team, node: node, org_key: org_key} do
+      Livebook.FileSystem.Mounter.subscribe(team.id)
       data = test |> to_string() |> Base.encode32(padding: false, case: :lower)
 
       file_system =
@@ -30,7 +31,7 @@ defmodule LivebookWeb.Integration.OpenLiveTest do
 
       file_system = TeamsRPC.create_file_system(node, team, org_key, file_system)
       assert_receive {:file_system_created, ^file_system}
-      assert_receive {:file_system_mounted, ^file_system}, 5_000
+      assert_receive {:file_system_mounted, ^file_system}, 10_000
 
       {:ok, file_system: file_system}
     end

test/livebook_web/live/hub/edit_live_test.exs

@@ -8,9 +8,15 @@ defmodule LivebookWeb.Hub.EditLiveTest do
   alias Livebook.Hubs
 
   describe "personal" do
-    setup do
+    setup tags do
       Livebook.Hubs.Broadcasts.subscribe([:crud, :secrets, :file_systems])
-      {:ok, hub: Hubs.fetch_hub!(Hubs.Personal.id())}
+      hub = Hubs.fetch_hub!(Hubs.Personal.id())
+
+      if tags[:git] do
+        Livebook.FileSystem.Mounter.subscribe(hub.id)
+      end
+
+      {:ok, hub: hub}
     end
 
     test "updates the hub", %{conn: conn, hub: hub} do
@@ -283,7 +289,6 @@ defmodule LivebookWeb.Hub.EditLiveTest do
         )
 
       :ok = Hubs.create_file_system(hub, file_system)
-
       assert_receive {:file_system_created, %Livebook.FileSystem.Git{} = ^file_system}
       assert_receive {:file_system_mounted, ^file_system}, 10_000
 
@@ -331,7 +336,7 @@ defmodule LivebookWeb.Hub.EditLiveTest do
       refute "/file.txt" in paths
       assert "/another_file.txt" in paths
 
-      assert Livebook.FileSystem.umount(file_system) == :ok
+      assert Livebook.FileSystem.unmount(file_system) == :ok
     end
 
     test "detaches existing S3 file system", %{conn: conn, hub: hub} do
@@ -369,7 +374,6 @@ defmodule LivebookWeb.Hub.EditLiveTest do
         )
 
       :ok = Hubs.create_file_system(hub, file_system)
-
       assert_receive {:file_system_created, %Livebook.FileSystem.Git{} = ^file_system}
       assert_receive {:file_system_mounted, ^file_system}, 10_000
 
@@ -386,7 +390,7 @@ defmodule LivebookWeb.Hub.EditLiveTest do
       render_confirm(view)
 
       assert_receive {:file_system_deleted, ^file_system}
-      assert_receive {:file_system_umounted, ^file_system}, 10_000
+      assert_receive {:file_system_unmounted, ^file_system}, 10_000
 
       assert_patch(view, "/hub/#{hub.id}")
       assert render(view) =~ "File storage deleted successfully"