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

[rhcarvalho]

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.

---

Related forum thread: https://elixirforum.com/t/elixir-v1-17-2-released/64794

[rhcarvalho]

An optimization to fetch only the about-to-be-used refs introduced when the :depth option was added

This was discussed in https://github.com/elixir-lang/elixir/pull/13128/files#r1399042619.

[rhcarvalho]

Ready for a second pass.

[josevalim]

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.

[rhcarvalho]

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?

[josevalim]

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

[rhcarvalho]

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)

[josevalim]

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

[rhcarvalho]

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

[josevalim]

So which arguments would reproduce the failure?

[rhcarvalho]

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).

[josevalim]

Thanks, I got it. We need to run depth even if there is no rev. :)

[josevalim]

💚 💙 💜 💛 ❤️