From 7ba18dfb52b30ed0ee0631ad2d75597dbc2fafdd Mon Sep 17 00:00:00 2001 From: Rodolfo Carvalho Date: Tue, 9 Jul 2024 10:45:36 +0200 Subject: [PATCH] Bring back support for Git :ref and :depth used together An optimization to fetch only the about-to-be-used refs introduced when the :depth option was added broke support for specifying a SHA1 prefix as :ref. Such possibility was neither documented nor tested, but certainly used in the wild. This use case is now explicitly supported with a new test case. We maintain this fetch optimization whenever :branch, :tag or :depth is specified. We fetch all refs when :ref is used without :depth to be able to resolve short SHA1 refs, and require full commit hashes to use :ref and :depth together. When using :ref with :depth, a full SHA1 is required by Git and surfaced in our docs. We enforce it when validating the options, leaving room for longer hashes. This commit brings back two tests to cover the :ref and :depth functionality, and adds a new one to cover the validation mentioned above. --- lib/mix/lib/mix/scm/git.ex | 48 ++++++++++----- lib/mix/lib/mix/tasks/deps.ex | 4 +- lib/mix/test/mix/tasks/deps.git_test.exs | 76 ++++++++++++++++++++++++ 3 files changed, 111 insertions(+), 17 deletions(-) diff --git a/lib/mix/lib/mix/scm/git.ex b/lib/mix/lib/mix/scm/git.ex index edc96c7ad3b..154df891263 100644 --- a/lib/mix/lib/mix/scm/git.ex +++ b/lib/mix/lib/mix/scm/git.ex @@ -9,7 +9,7 @@ defmodule Mix.SCM.Git do @impl true def format(opts) do - if rev = opts[:ref] || opts[:branch] || opts[:tag] do + if rev = get_opts_rev(opts) do "#{redact_uri(opts[:git])} - #{rev}" else redact_uri(opts[:git]) @@ -126,17 +126,17 @@ defmodule Mix.SCM.Git do update_origin(opts[:git]) # Fetch external data - branch_or_tag = opts[:branch] || opts[:tag] + rev = get_lock_rev(opts[:lock], opts) || get_opts_rev(opts) ["--git-dir=.git", "fetch", "--force", "--quiet"] |> Kernel.++(progress_switch(git_version())) |> Kernel.++(tags_switch(opts[:tag])) |> Kernel.++(depth_switch(opts[:depth])) - |> Kernel.++(if branch_or_tag, do: ["origin", branch_or_tag], else: []) + |> Kernel.++(refspec_switch(opts, rev)) |> git!() # Migrate the Git repo - rev = get_lock_rev(opts[:lock], opts) || opts[:ref] || branch_or_tag || default_branch() + rev = rev || default_branch() git!(["--git-dir=.git", "checkout", "--quiet", rev]) if opts[:submodules] do @@ -219,6 +219,15 @@ defmodule Mix.SCM.Git do ["--depth=#{n}"] end + defp refspec_switch(_opts, nil), do: [] + + defp refspec_switch(opts, rev) do + case Keyword.take(opts, [:depth, :branch, :tag]) do + [_ | _] -> ["origin", rev] + _ -> [] + end + end + ## Helpers defp validate_git_options(opts) do @@ -249,25 +258,30 @@ defmodule Mix.SCM.Git do end end + @sha1_size 40 + defp validate_depth(opts) do - case Keyword.take(opts, [:depth, :ref]) do - [_, _] -> - Mix.raise( - "Cannot specify :depth and :ref at the same time. " <> - "Error on Git dependency: #{redact_uri(opts[:git])}" - ) + case Keyword.take(opts, [:depth]) do + [] -> + opts + + [{:depth, depth}] when is_integer(depth) and depth > 0 -> + ref = opts[:ref] + + if ref && byte_size(ref) < @sha1_size do + Mix.raise( + "When :depth is used with :ref, a full commit hash is required. " <> + "Error on Git dependency: #{redact_uri(opts[:git])}" + ) + end - [depth: depth] when is_integer(depth) and depth > 0 -> opts - [depth: invalid_depth] -> + invalid_depth -> Mix.raise( "The depth must be a positive integer, and be specified only once, got: #{inspect(invalid_depth)}. " <> "Error on Git dependency: #{redact_uri(opts[:git])}" ) - - _ -> - opts end end @@ -296,6 +310,10 @@ defmodule Mix.SCM.Git do end end + defp get_opts_rev(opts) do + opts[:branch] || opts[:ref] || opts[:tag] + end + defp redact_uri(git) do case URI.parse(git) do %{userinfo: nil} -> git diff --git a/lib/mix/lib/mix/tasks/deps.ex b/lib/mix/lib/mix/tasks/deps.ex index c3a5f7faad2..bea69ac2f15 100644 --- a/lib/mix/lib/mix/tasks/deps.ex +++ b/lib/mix/lib/mix/tasks/deps.ex @@ -125,8 +125,8 @@ defmodule Mix.Tasks.Deps do * `:depth` *(since v1.17.0)* - creates a shallow clone of the Git repository, limiting the history to the specified number of commits. This can significantly improve clone speed for large repositories when full history is not needed. - The value must be a positive integer, typically `1`. Cannot be used with the - `:ref` option. + The value must be a positive integer, typically `1`. When using `:depth` with + `:ref`, a fully spelled hex object name (a 40-character SHA-1 hash) is required. If your Git repository requires authentication, such as basic username:password HTTP authentication via URLs, it can be achieved via Git configuration, keeping diff --git a/lib/mix/test/mix/tasks/deps.git_test.exs b/lib/mix/test/mix/tasks/deps.git_test.exs index cdaf7b337ca..0d8631a28e1 100644 --- a/lib/mix/test/mix/tasks/deps.git_test.exs +++ b/lib/mix/test/mix/tasks/deps.git_test.exs @@ -478,6 +478,22 @@ defmodule Mix.Tasks.DepsGitTest do purge([GitRepo, GitRepo.MixProject]) end + test "fetches with short ref" do + [<>, _ | _] = get_git_repo_revs("git_repo") + + Process.put(:git_repo_opts, ref: short_sha1) + + in_fixture("no_mixfile", fn -> + Mix.Project.push(GitApp) + + Mix.Tasks.Deps.Get.run([]) + message = "* Getting git_repo (#{fixture_path("git_repo")} - #{short_sha1})" + assert_received {:mix_shell, :info, [^message]} + + refute_received {:mix_shell, :error, _} + end) + end + describe "Git depth option" do @describetag :git_depth @@ -530,6 +546,66 @@ defmodule Mix.Tasks.DepsGitTest do end) end + test "with ref" do + [sha1, _ | _] = get_git_repo_revs("git_repo") + + Process.put(:git_repo_opts, depth: 1, ref: sha1) + + in_fixture("no_mixfile", fn -> + Mix.Project.push(GitApp) + + Mix.Tasks.Deps.Get.run([]) + message = "* Getting git_repo (#{fixture_path("git_repo")} - #{sha1})" + assert_received {:mix_shell, :info, [^message]} + assert_shallow("deps/git_repo", 1) + end) + end + + test "raises with short ref" do + # When fetching, Git requires a fully spelled hex object name. We prevent + # this failure mode by validating the ref length. + # + # If Git ever changes such that it can resolve a short ref in a shallow + # fetch, we can update our docs and this test to reflect that. + # + # https://git-scm.com/docs/git-fetch#Documentation/git-fetch.txt-ltrefspecgt + # https://stackoverflow.com/a/43136160 + + [<>, _ | _] = get_git_repo_revs("git_repo") + + Process.put(:git_repo_opts, depth: 1, ref: short_sha1) + + in_fixture("no_mixfile", fn -> + Mix.Project.push(GitApp) + + exception = assert_raise Mix.Error, fn -> Mix.Tasks.Deps.Get.run([]) end + + assert Exception.message(exception) =~ "a full commit hash is required" + end) + end + + test "changing refspec updates retaining depth" do + [last, first | _] = get_git_repo_revs("git_repo") + + Process.put(:git_repo_opts, ref: first, depth: 1) + + in_fixture("no_mixfile", fn -> + Mix.Project.push(GitApp) + + Mix.Tasks.Deps.Get.run([]) + message = "* Getting git_repo (#{fixture_path("git_repo")} - #{first})" + assert_received {:mix_shell, :info, [^message]} + assert_shallow("deps/git_repo", 1) + assert File.read!("mix.lock") =~ first + + # Change refspec + update_dep(ref: last, depth: 1) + Mix.Tasks.Deps.Get.run([]) + assert_shallow("deps/git_repo", 1) + assert File.read!("mix.lock") =~ last + end) + end + test "removing depth retains shallow repository" do # For compatibility and simplicity, we follow Git's behavior and do not # attempt to unshallow an existing repository. This should not be a