Skip to content

Bring back support for Git :ref and :depth used together #13713

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jul 10, 2024
Merged

Bring back support for Git :ref and :depth used together #13713

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 33 additions & 15 deletions lib/mix/lib/mix/scm/git.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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])
Expand Down Expand Up @@ -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))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would merge refspec_switch and depth_switch logic into a single function. There is too much dependency between these two and unifying it in a single place would probably be cleaner.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other than moving the call to depth_switch into refspec_switch, I'm not seeing a clean path forward here:

  defp refspec_switch(_opts, nil), do: []

  defp refspec_switch(opts, rev) do
    depth_switch(opts[:depth]) ++
      case Keyword.take(opts, [:depth, :branch, :tag]) do
        [_ | _] -> ["origin", rev]
        _ -> []
      end
  end

  defp depth_switch(nil), do: []

  defp depth_switch(n) when is_integer(n) and n > 0 do
    check_depth_support(git_version())
    ["--depth=#{n}"]
  end

We only want to call check_depth_support with a valid :depth, and the other logic is about when to include the repository + refspec (e.g. origin main). Do you see a better way to organize this?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking this:

  defp fetch_args(opts, nil), do: []
  
  defp fetch_args(opts, rev) do
    cond do
      depth = opts[:depth] ->
        check_depth_support(git_version())
        ["--depth=#{n}", "origin", rev]

      # We can only fetch full refs, which :ref may not be one of
      opts[:ref] == rev ->
        []

      true ->
        ["origin", rev]
    end
  end

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm this misses the case of depth with no rev (tip of default branch, when neither of :ref, :branch, or :tag are specified)?

(I think a test case would go red)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn’t that rev equals to nil, handled in the first clause?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, my code above also falls into the same mistake. The current commit handles that case with the two independent functions.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So which arguments would reproduce the failure?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I ran the tests locally for code in #13713 (comment) and #13713 (comment) (with a small fix, s/{n}/{depth}/).

Both fail 5 tests (5 out of 10 in the describe "Git depth option"): all the tests where the mix spec doesn't include one of :ref, :branch, :tag (in the test file they are the first and the last four tests in the describe block).

The failure is the same, the produced repos are not shallow:

 Expected truthy, got false
 code: assert File.exists?(repo_path <> "/.git/shallow")

That confirms what I said in #13713 (comment) last night (I was on mobile and "ran the tests" on my head).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I got it. We need to run depth even if there is no 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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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

Expand Down Expand Up @@ -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
Expand Down
4 changes: 2 additions & 2 deletions lib/mix/lib/mix/tasks/deps.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
76 changes: 76 additions & 0 deletions lib/mix/test/mix/tasks/deps.git_test.exs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -478,6 +478,22 @@ defmodule Mix.Tasks.DepsGitTest do
purge([GitRepo, GitRepo.MixProject])
end

test "fetches with short ref" do
[<<short_sha1::binary-size(8), _::binary>>, _ | _] = 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

Expand Down Expand Up @@ -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

[<<short_sha1::binary-size(8), _::binary>>, _ | _] = 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
Expand Down