Skip to content
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.

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

Add :depth option to git deps #13128

Merged
merged 4 commits into from
Dec 4, 2023

Conversation

rhcarvalho
Copy link
Contributor

Based on conversation at https://elixirforum.com/t/mix-git-dependency-with-shallow-clone/59440.

This allows for faster clones that transfer less data over the network and take less space in disk, for cases when the full history is not needed.

As a particular example, and how I plan to use this, setting depth: 1 allows us to update the Phoenix Heroicons dependency pulling in as little data as possible (see phoenixframework/phoenix#5622 (comment)).

defmodule Hello.MixProject do
  use Mix.Project

  def project do
    [
      # ...
      deps: deps()
    ]
  end

  # ...

  defp deps do
    [
      # ...
      {:heroicons,
       github: "tailwindlabs/heroicons",
       tag: "v2.0.18",
       app: false,
       compile: false,
       sparse: "optimized",
       depth: 1},
      # ...
  end

Copy link
Contributor Author

@rhcarvalho rhcarvalho left a comment

Choose a reason for hiding this comment

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

I tried to make sure the new :depth option plays well with the existing options and can be extended later if needed, while keeping things simple. Recent Git versions support things like submodule depth, depth since date, etc.

Despite my testing efforts, I acknowledge the complexity of dealing with Git and the possibility of breaking users, so let's proceed with caution 😄

lib/mix/lib/mix/scm/git.ex Show resolved Hide resolved
lib/mix/lib/mix/scm/git.ex Show resolved Hide resolved
lib/mix/lib/mix/scm/git.ex Show resolved Hide resolved
@@ -282,6 +321,8 @@ defmodule Mix.SCM.Git do
end

defp default_branch() do
# Note: the `set-head -a` command requires the remote reference to be
# fetched first.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added this note because the most obvious code would be to "compute" rev in checkout/2 in a single expression as it used to be, but that doesn't work.

lib/mix/lib/mix/tasks/deps.ex Show resolved Hide resolved
Comment on lines +548 to +568
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
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is intended to cover José's concern in the original conversation.

The way to call git fetch can handle both changing the :depth and the :tag, :branch or :ref.

lib/mix/test/mix/tasks/deps.git_test.exs Outdated Show resolved Hide resolved
end

test "can be used with subdir" do
Process.put(:git_repo_opts, subdir: "sparse_dir", depth: 1)
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 noticed that the :subdir options forces the creation of the subdir, even if it doesn't exist in the repository. It was curious to me that the test fixtures don't create a subdir named "subdir" (what the existing subdir test uses), and so used the "sparse_dir" path here, and also asserting it contains what we expect (unlike the existing subdir test).

Copy link
Member

@josevalim josevalim left a comment

Choose a reason for hiding this comment

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

The tests are fantastic, I have done one initial pass, but the code changes generally look good to me. :)

@rhcarvalho rhcarvalho force-pushed the deps-git-shallow-clone branch from fcdf8c0 to 8cd49b3 Compare November 20, 2023 13:42
@josevalim
Copy link
Member

Tests are failing due to the changes in the format rendering. It should be a quick fix, can you please take a look?

@rhcarvalho
Copy link
Contributor Author

rhcarvalho commented Nov 26, 2023 via email

This allows for faster clones that transfer less data over the network
and take less space in disk, for cases when the full history is not
needed.
@rhcarvalho rhcarvalho force-pushed the deps-git-shallow-clone branch from 8cd49b3 to 849782c Compare December 4, 2023 10:36
@rhcarvalho
Copy link
Contributor Author

  • Rebased
  • Fixed format test ("b" instead of "origin/b")
  • Updated note re: not attempting to unshallow existing checkout

@josevalim josevalim merged commit 41690a3 into elixir-lang:main Dec 4, 2023
7 of 8 checks passed
@josevalim
Copy link
Member

💚 💙 💜 💛 ❤️

rhcarvalho added a commit to rhcarvalho/elixir that referenced this pull request Apr 5, 2024
Adding a note about a new option added in elixir-lang#13128.
whatyouhide pushed a commit that referenced this pull request Apr 5, 2024
Adding a note about a new option added in #13128.
gldubc pushed a commit to gldubc/elixir that referenced this pull request Apr 24, 2024
@rhcarvalho rhcarvalho deleted the deps-git-shallow-clone branch July 8, 2024 20:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants