Refactor handle_repos_add

src/API.jl

@@ -41,7 +41,7 @@ function add_or_develop(ctx::Context, pkgs::Vector{PackageSpec}; mode::Symbol, s
     ctx.preview && preview_info()
     new_git = mode == :develop ?
         handle_repos_develop!(ctx, pkgs, shared) :
-        handle_repos_add!(ctx, pkgs; upgrade_or_add=true)
+        handle_repos_add!(ctx, pkgs)
 
     project_deps_resolve!(ctx.env, pkgs)
     registry_resolve!(ctx.env, pkgs)
@@ -463,7 +463,11 @@ function instantiate(ctx::Context; manifest::Union{Bool, Nothing}=nothing, kwarg
         append!(urls[uuid], url)
         urls[uuid] = unique(urls[uuid])
     end
-    new_git = handle_repos_add!(ctx, pkgs; upgrade_or_add=false)
+    new_git = UUID[]
+    for pkg in pkgs
+        pkg.repo !== nothing || continue
+        instantiate_pkg_repo!(pkg) && push!(new_git, pkg.uuid)
+    end
     new_apply = Operations.apply_versions(ctx, pkgs, hashes, urls)
     Operations.build_versions(ctx, union(new_apply, new_git))
 end

src/Operations.jl

@@ -1028,7 +1028,7 @@ function build_versions(ctx::Context, uuids::Vector{UUID}; might_need_to_resolve
                 path = project_rel_path(ctx, entry.path)
                 hash_or_path = path
             else
-                pkgerror("Could not find either `git-tree-sha1` or `path` for package $(pkg.name)")
+                pkgerror("Could not find either `git-tree-sha1` or `path` for package $name")
             end
             version = v"0.0"
         end
@@ -1188,9 +1188,8 @@ function up(ctx::Context, pkgs::Vector{PackageSpec})
             entry = manifest_info(ctx.env, pkg.uuid)
             if entry !== nothing && entry.repo.url !== nothing
                 pkg.repo = entry.repo
-                new = handle_repos_add!(ctx, [pkg]; credentials=creds,
-                                        upgrade_or_add = (level == UPLEVEL_MAJOR))
-                append!(new_git, new)
+                pkg.version = VersionNumber(entry.version)
+                new = instantiate_pkg_repo!(pkg) && push!(new_git, pkg.uuid)
             else
                 if entry !== nothing
                     pkg.uuid in keys(ctx.stdlibs) && continue

src/Types.jl

@@ -21,7 +21,7 @@ export UUID, pkgID, SHA1, VersionRange, VersionSpec, empty_versionspec,
     Requires, Fixed, merge_requires!, satisfies, ResolverError,
     PackageSpec, EnvCache, Context, Context!, get_deps,
     PkgError, pkgerror, has_name, has_uuid, write_env, parse_toml, find_registered!,
-    project_resolve!, project_deps_resolve!, manifest_resolve!, registry_resolve!, stdlib_resolve!, handle_repos_develop!, handle_repos_add!, ensure_resolved,
+    project_resolve!, project_deps_resolve!, manifest_resolve!, registry_resolve!, stdlib_resolve!, handle_repos_develop!, handle_repos_add!, ensure_resolved, instantiate_pkg_repo!,
     manifest_info, registered_uuids, registered_paths, registered_uuid, registered_name,
     read_project, read_package, read_manifest, pathrepr, registries,
     PackageMode, PKGMODE_MANIFEST, PKGMODE_PROJECT, PKGMODE_COMBINED,
@@ -691,7 +691,7 @@ end
 function handle_repos_develop!(ctx::Context, pkgs::AbstractVector{PackageSpec}, shared::Bool)
     for pkg in pkgs
         pkg.repo === nothing && (pkg.repo = Types.GitRepo())
-        pkg.repo.rev === nothing || pkgerror("git revision cannot be given to `develop`")
+        pkg.repo.rev !== nothing && pkgerror("git revision cannot be given to `develop`")
     end
 
     new_uuids = UUID[]
@@ -712,103 +712,133 @@ function handle_repos_develop!(ctx::Context, pkgs::AbstractVector{PackageSpec},
     return new_uuids
 end
 
-function handle_repos_add!(ctx::Context, pkgs::AbstractVector{PackageSpec};
-                           upgrade_or_add::Bool=true, credentials=nothing)
-    # Always update the registry when adding
-    UPDATED_REGISTRY_THIS_SESSION[] || update_registries(ctx)
-    creds = credentials !== nothing ? credentials : LibGit2.CachedCredentials()
-    try
-        env = ctx.env
-        new_uuids = UUID[]
-        for pkg in pkgs
-            pkg.repo == nothing && continue
-            pkg.special_action = PKGSPEC_REPO_ADDED
-            pkg.repo.url === nothing && set_repo_for_pkg!(env, pkg)
-            clones_dir = joinpath(depots1(), "clones")
-            mkpath(clones_dir)
-            repo_path = joinpath(clones_dir, string(hash(pkg.repo.url)))
-            repo = nothing
-            do_nothing_more = false
-            project_path = nothing
-            folder_already_downloaded = false
-            try
-                repo = GitTools.ensure_clone(repo_path, pkg.repo.url; isbare=true, credentials=creds)
-                entry = manifest_info(env, pkg.uuid)
-                pinned = (entry !== nothing && entry.pinned)
-                upgrading = upgrade_or_add && !pinned
-                if upgrading
-                    GitTools.fetch(repo; refspecs=refspecs, credentials=creds)
-                    rev = pkg.repo.rev
-                    # see if we can get rev as a branch
-                    if rev === nothing
-                        rev = LibGit2.isattached(repo) ?
-                            LibGit2.branch(repo) :
-                            string(LibGit2.GitHash(LibGit2.head(repo)))
-                    end
-                else
-                    # Not upgrading so the rev should be the current git-tree-sha
-                    rev = entry.repo.tree_sha
-                    pkg.version = VersionNumber(entry.version)
-                end
+clone_path(url) = joinpath(depots1(), "clones", string(hash(url)))
+function clone_path!(url)
+    clone = clone_path(url)
+    mkpath(dirname(clone))
+    Base.shred!(LibGit2.CachedCredentials()) do creds
+        LibGit2.with(GitTools.ensure_clone(clone, url; isbare=true, credentials=creds)) do repo
+            GitTools.fetch(repo; refspecs=refspecs, credentials=creds)
+        end
+    end
+    return clone
+end
 
-                gitobject, isbranch = get_object_branch(repo, rev, creds)
-                # If the user gave a shortened commit SHA, might as well update it to the full one
-                try
-                    if upgrading
-                        pkg.repo.rev = isbranch ? rev : string(LibGit2.GitHash(gitobject))
-                    end
-                    LibGit2.with(LibGit2.peel(LibGit2.GitTree, gitobject)) do git_tree
-                        @assert git_tree isa LibGit2.GitTree
-                        pkg.repo.tree_sha = SHA1(string(LibGit2.GitHash(git_tree)))
-                            version_path = nothing
-                            folder_already_downloaded = false
-                        if has_uuid(pkg) && has_name(pkg)
-                            version_path = Pkg.Operations.find_installed(pkg.name, pkg.uuid, pkg.repo.tree_sha)
-                            isdir(version_path) && (folder_already_downloaded = true)
-                            entry = manifest_info(env, pkg.uuid)
-                            if entry !== nothing &&
-                                entry.repo.tree_sha == pkg.repo.tree_sha && folder_already_downloaded
-                                # Same tree sha and this version already downloaded, nothing left to do
-                                do_nothing_more = true
-                            end
-                        end
-                        if folder_already_downloaded
-                            project_path = version_path
-                        else
-                            project_path = mktempdir()
-                            _project_path = project_path # https://github.com/JuliaLang/julia/issues/30048
-                            GC.@preserve _project_path begin
-                                opts = LibGit2.CheckoutOptions(
-                                    checkout_strategy = LibGit2.Consts.CHECKOUT_FORCE,
-                                    target_directory = Base.unsafe_convert(Cstring, _project_path),
-                                )
-                                LibGit2.checkout_tree(repo, git_tree, options=opts)
-                            end
-                        end
-                    end
-                finally
-                    close(gitobject)
-                end
-            finally
-                repo isa LibGit2.GitRepo && close(repo)
-            end
-            do_nothing_more && continue
-            parse_package!(ctx, pkg, project_path)
-            if !folder_already_downloaded
-                version_path = Pkg.Operations.find_installed(pkg.name, pkg.uuid, pkg.repo.tree_sha)
-                mkpath(version_path)
-                mv(project_path, version_path; force=true)
-                push!(new_uuids, pkg.uuid)
-                Base.rm(project_path; force = true, recursive = true)
+function guess_rev(repo_path)::String
+    rev = nothing
+    LibGit2.with(LibGit2.GitRepo(repo_path)) do repo
+        rev = LibGit2.isattached(repo) ?
+            LibGit2.branch(repo) :
+            string(LibGit2.GitHash(LibGit2.head(repo)))
+        gitobject, isbranch = nothing, nothing
+        Base.shred!(LibGit2.CachedCredentials()) do creds
+            gitobject, isbranch = get_object_branch(repo, rev, creds)
+        end
+        LibGit2.with(gitobject) do object
+            rev = isbranch ? rev : string(LibGit2.GitHash(gitobject))
+        end
+    end
+    return rev
+end
+
+function with_git_tree(fn, repo_path::String, rev::String)
+    gitobject = nothing
+    Base.shred!(LibGit2.CachedCredentials()) do creds
+        LibGit2.with(LibGit2.GitRepo(repo_path)) do repo
+            gitobject, isbranch = get_object_branch(repo, rev, creds)
+            LibGit2.with(LibGit2.peel(LibGit2.GitTree, gitobject)) do git_tree
+                @assert git_tree isa LibGit2.GitTree
+                return applicable(fn, repo, git_tree) ?
+                    fn(repo, git_tree) :
+                    fn(git_tree)
             end
-            @assert has_uuid(pkg)
         end
-        return new_uuids
-    finally
-        creds !== credentials && Base.shred!(creds)
     end
 end
 
+function repo_checkout(repo_path, rev)
+    project_path = mktempdir()
+    with_git_tree(repo_path, rev) do repo, git_tree
+        _project_path = project_path # https://github.com/JuliaLang/julia/issues/30048
+        GC.@preserve _project_path begin
+            opts = LibGit2.CheckoutOptions(
+                checkout_strategy = LibGit2.Consts.CHECKOUT_FORCE,
+                target_directory = Base.unsafe_convert(Cstring, _project_path),
+            )
+            LibGit2.checkout_tree(repo, git_tree, options=opts)
+        end
+    end
+    return project_path
+end
+
+function tree_hash(repo_path, rev)
+    with_git_tree(repo_path, rev) do git_tree
+        return SHA1(string(LibGit2.GitHash(git_tree)))
+    end
+end
+
+function instantiate_pkg_repo!(pkg::PackageSpec, repo_cache::Union{Nothing,String}=nothing)
+    pkg.special_action = PKGSPEC_REPO_ADDED
+    clone = clone_path!(pkg.repo.url)
+    pkg.repo.tree_sha = tree_hash(clone, pkg.repo.rev)
+    version_path = Pkg.Operations.find_installed(pkg.name, pkg.uuid, pkg.repo.tree_sha)
+    if repo_cache === nothing
+        repo_cache = repo_checkout(clone, string(pkg.repo.tree_sha))
+    end
+    isdir(version_path) && return false
+    mkpath(version_path)
+    mv(repo_cache, version_path; force=true)
+    return true
+end
+
+# partial PackageSpec -> PackageSpec with all the relevant fields filled out
+function resolve_repo_add!(ctx::Context, pkg::PackageSpec)
+    repo_cache = nothing
+    if pkg.repo.url !== nothing
+        clone_path = clone_path!(pkg.repo.url)
+        pkg.repo.rev = something(pkg.repo.rev, guess_rev(clone_path))
+        repo_cache = repo_checkout(clone_path, pkg.repo.rev)
+        package = parse_package!(ctx, pkg, repo_cache)
+    elseif pkg.name !== nothing || pkg.uuid !== nothing
+        registry_resolve!(ctx.env, [pkg])
+        ensure_resolved(ctx.env, [pkg]; registry=true)
+        _, pkg.repo.url = Types.registered_info(ctx.env, pkg.uuid, "repo")[1]
+        pkg.repo.rev === nothing && pkgerror("Rev must be specified")
+    else
+        pkgerror("Package must be specified by name, URL, or UUID")
+    end
+    return repo_cache
+end
+
+function handle_repo_add!(ctx::Context, pkg::PackageSpec)
+    pkg.repo === nothing && return
+    repo_cache = resolve_repo_add!(ctx, pkg)
+    # if pinned, return early
+    entry = manifest_info(ctx.env, pkg.uuid)
+    if (entry !== nothing && entry.pinned)
+        repo_cache !== nothing && rm(repo_cache; recursive=true, force=true)
+        pkg.repo.tree_sha = entry.repo.tree_sha
+        return false
+    end
+    # instantiate repo
+    return instantiate_pkg_repo!(pkg, repo_cache)
+end
+
+"""
+Ensure repo specified by `repo` exists at version path for package
+Set tree_sha
+"""
+function handle_repos_add!(ctx::Context, in_pkgs::AbstractVector{PackageSpec})
+    pkgs = filter(pkg -> pkg.repo !== nothing, in_pkgs)
+    # Always update the registry when adding
+    UPDATED_REGISTRY_THIS_SESSION[] || update_registries(ctx)
+    new_uuids = UUID[]
+    for pkg in pkgs
+        handle_repo_add!(ctx, pkg) && push!(new_uuids, pkg.uuid)
+    end
+    return new_uuids
+end
+
 function parse_package!(ctx, pkg, project_path)
     env = ctx.env
     project_file = projectfile_path(project_path)

test/api.jl

@@ -149,4 +149,36 @@ end
     end end
 end
 
+@testset "Pkg.add" begin
+    # Add by URL should not override pin
+    temp_pkg_dir() do project_path; with_temp_env() do env_path
+        Pkg.add(Pkg.PackageSpec(;name="Example", version="0.3.0"))
+        Pkg.pin(Pkg.PackageSpec(;name="Example"))
+        a = deepcopy(Pkg.Types.EnvCache().manifest)
+        Pkg.add(Pkg.PackageSpec(;url="https://github.com/JuliaLang/Example.jl"))
+        b = Pkg.Types.EnvCache().manifest
+        for (uuid, x) in a
+            y = b[uuid]
+            for property in propertynames(x)
+                @test getproperty(x, property) == getproperty(y, property)
+            end
+        end
+    end end
+    # Add by URL should not overwrite files
+    temp_pkg_dir() do project_path; with_temp_env() do env_path
+        Pkg.add(Pkg.PackageSpec(;url="https://github.com/JuliaLang/Example.jl"))
+        t1, t2 = nothing, nothing
+        for (uuid, entry) in Pkg.Types.EnvCache().manifest
+            entry.name == "Example" || continue
+            t1 = mtime(Pkg.Operations.find_installed(entry.name, uuid, entry.repo.tree_sha))
+        end
+        Pkg.add(Pkg.PackageSpec(;url="https://github.com/JuliaLang/Example.jl"))
+        for (uuid, entry) in Pkg.Types.EnvCache().manifest
+            entry.name == "Example" || continue
+            t2 = mtime(Pkg.Operations.find_installed(entry.name, uuid, entry.repo.tree_sha))
+        end
+        @test t1 == t2
+    end end
+end
+
 end # module APITests