ensure we do not end up in a situation where both tree hash and path is set (#4467)

Author: KristofferC

src/Operations.jl

@@ -252,9 +252,14 @@ function load_all_deps(
     for pkg in pkgs
         path, repo = get_path_repo(env.project, env.project_file, env.manifest_file, pkg.name)
         if path !== nothing
+            # Path from [sources] takes precedence - clear tree_hash and repo from manifest
+            pkg.tree_hash = nothing
+            pkg.repo = GitRepo()  # Clear any repo info
             pkg.path = path
         end
         if repo.source !== nothing
+            # Repo from [sources] takes precedence - clear path from manifest
+            pkg.path = nothing
             pkg.repo.source = repo.source
         end
         if repo.rev !== nothing
@@ -2224,7 +2229,7 @@ function up_load_versions!(ctx::Context, pkg::PackageSpec, entry::PackageEntry,
         if pkg.path === nothing
             pkg.tree_hash = entry.tree_hash
         end
-    elseif entry.repo.source !== nothing || source_repo.source !== nothing # repo packages have a version but are treated specially
+    elseif source_path === nothing && pkg.path === nothing && (entry.repo.source !== nothing || source_repo.source !== nothing) # repo packages have a version but are treated specially
         if source_repo.source !== nothing
             pkg.repo = source_repo
         else
@@ -2257,10 +2262,12 @@ end
 up_load_manifest_info!(pkg::PackageSpec, ::Nothing) = nothing
 function up_load_manifest_info!(pkg::PackageSpec, entry::PackageEntry)
     pkg.name = entry.name # TODO check name is same
-    if pkg.repo == GitRepo()
+    # Only restore repo from manifest if we don't already have a path set
+    if pkg.repo == GitRepo() && pkg.path === nothing
         pkg.repo = entry.repo # TODO check that repo is same
     end
-    if pkg.path === nothing
+    # Only set path if tree_hash is not already set (to avoid invalid state where both are set)
+    if pkg.path === nothing && pkg.repo == GitRepo() && pkg.tree_hash === nothing
         pkg.path = entry.path
     end
     return pkg.pinned = entry.pinned

src/Types.jl

@@ -1040,6 +1040,7 @@ function handle_repo_add!(ctx::Context, pkg::PackageSpec)
                     pkgerror("Did not find subdirectory `$(pkg.repo.subdir)`")
                 end
             end
+            @assert pkg.path === nothing
             pkg.tree_hash = SHA1(string(LibGit2.GitHash(tree_hash_object)))
 
             # If we already resolved a uuid, we can bail early if this package is already installed at the current tree_hash

test/sources.jl

@@ -3,6 +3,7 @@ module SourcesTest
 import ..Pkg # ensure we are using the correct Pkg
 using Test, Pkg
 using ..Utils
+using UUIDs
 
 temp_pkg_dir() do project_path
     @testset "test Project.toml [sources]" begin
@@ -152,6 +153,91 @@ temp_pkg_dir() do project_path
             end
         end
     end
+
+    # Regression test for https://github.com/JuliaLang/Pkg.jl/issues/4337
+    # Switching between path and repo sources should not cause assertion error
+    @testset "switching between path and repo sources (#4337)" begin
+        mktempdir() do tmp
+            cd(tmp) do
+                # Create a local package and initialize it as a git repo
+                local_pkg_uuid = UUID("00000000-0000-0000-0000-000000000001")
+                mkdir("LocalPkg")
+                write(
+                    joinpath("LocalPkg", "Project.toml"), """
+                    name = "LocalPkg"
+                    uuid = "$local_pkg_uuid"
+                    version = "0.1.0"
+                    """
+                )
+                mkdir(joinpath("LocalPkg", "src"))
+                write(joinpath("LocalPkg", "src", "LocalPkg.jl"), "module LocalPkg end")
+
+                # Initialize as a git repo
+                git_init_and_commit("LocalPkg")
+
+                # Get the absolute path for file:// URL
+                local_pkg_url = make_file_url(abspath("LocalPkg"))
+
+                # Create test project with path source
+                write(
+                    "Project.toml", """
+                    [deps]
+                    LocalPkg = "$local_pkg_uuid"
+
+                    [sources]
+                    LocalPkg = { path = "LocalPkg" }
+                    """
+                )
+
+                with_current_env() do
+                    # Initial resolve with path source
+                    Pkg.resolve()
+                    manifest = Pkg.Types.read_manifest("Manifest.toml")
+                    @test manifest[local_pkg_uuid].path !== nothing
+                    @test manifest[local_pkg_uuid].tree_hash === nothing
+                    @test manifest[local_pkg_uuid].repo.source === nothing
+                    # Update should work without error
+                    Pkg.update()
+
+                    # Switch to repo source using file:// protocol
+                    write(
+                        "Project.toml", """
+                        [deps]
+                        LocalPkg = "$local_pkg_uuid"
+
+                        [sources]
+                        LocalPkg = { url = "$local_pkg_url", rev = "HEAD" }
+                        """
+                    )
+
+                    # This should NOT cause an assertion error about tree_hash and path both being set
+                    Pkg.update()
+                    manifest = Pkg.Types.read_manifest("Manifest.toml")
+                    @test manifest[local_pkg_uuid].path === nothing
+                    @test manifest[local_pkg_uuid].tree_hash !== nothing
+                    @test manifest[local_pkg_uuid].repo.source !== nothing
+
+                    # Switch back to path source
+                    write(
+                        "Project.toml", """
+                        [deps]
+                        LocalPkg = "$local_pkg_uuid"
+
+                        [sources]
+                        LocalPkg = { path = "LocalPkg" }
+                        """
+                    )
+
+                    # This should work and restore the path source without assertion error
+                    Pkg.update()
+                    manifest = Pkg.Types.read_manifest("Manifest.toml")
+                    @test manifest[local_pkg_uuid].path !== nothing
+                    @test manifest[local_pkg_uuid].tree_hash === nothing
+                    @test manifest[local_pkg_uuid].repo.source === nothing
+                end
+            end
+        end
+    end
 end
 
 end # module

test/subdir.jl

@@ -65,16 +65,6 @@ function setup_packages_repository(dir)
     return package_tree_hash, dep_tree_hash
 end
 
-# Convert a path into a file URL.
-function make_file_url(path)
-    # Turn the slashes on Windows. In case the path starts with a
-    # drive letter, an extra slash will be needed in the file URL.
-    path = replace(path, "\\" => "/")
-    if !startswith(path, "/")
-        path = "/" * path
-    end
-    return "file://$(path)"
-end
 
 # Create a registry with the two packages `Package` and `Dep`.
 function setup_registry(dir, packages_dir_url, package_tree_hash, dep_tree_hash)

test/utils.jl

@@ -11,7 +11,7 @@ using UUIDs
 export temp_pkg_dir, cd_tempdir, isinstalled, write_build, with_current_env,
     with_temp_env, with_pkg_env, git_init_and_commit, copy_test_package,
     git_init_package, add_this_pkg, TEST_SIG, TEST_PKG, isolate, LOADED_DEPOT,
-    list_tarball_files, recursive_rm_cov_files, copy_this_pkg_cache
+    list_tarball_files, recursive_rm_cov_files, copy_this_pkg_cache, make_file_url
 
 const CACHE_DIRECTORY = realpath(mktempdir(; cleanup = true))
 
@@ -367,4 +367,15 @@ function recursive_rm_cov_files(rootdir::String)
     return
 end
 
+# Convert a path into a file URL.
+function make_file_url(path)
+    # Turn the slashes on Windows. In case the path starts with a
+    # drive letter, an extra slash will be needed in the file URL.
+    path = replace(path, "\\" => "/")
+    if !startswith(path, "/")
+        path = "/" * path
+    end
+    return "file://$(path)"
+end
+
 end