Merge #970

970: Fix repo handling r=KristofferC a=00vareladavid

I started off just trying to break down `handle_repos_add` (it was a beast!) but the refactoring allowed two bugs to be fixed along the way.

Fixes #951. In the first stage (`resolve_repo_add!`) we make sure certain fields of the `PackageSpec` are filled out (`name`, `uuid`, `repo.url`, `repo.rev`). This means we can have more consistent logic for the rest of the operation. In particular, once we have the UUID, it is simple to check if the package is pinned.

Fixes #614. I added a test which simply makes sure that the `mtime` for the source is the same after an `add` by URL.

In terms of refactoring, moving all git-heavy operations into their own functions helps to clarify the `add`-specific logic. Also, `handle_repos_add!` was being called from many places. It turns out those calls just needed some more limited functionality which is now in `instantiate_pkg_repo!`.

Co-authored-by: David Varela <00.varela.david@gmail.com>

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,
@@ -560,7 +560,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[]
@@ -581,103 +581,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