Skip to content

Move the 'GitRepositoryProvider' to compare urls using their canonical representation, correctly accepting results where repositories only differ by '.git' #7741

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.

Sign up for GitHub

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

Merged
merged 11 commits into from
Jul 22, 2024
Merged

Move the 'GitRepositoryProvider' to compare urls using their canonical representation, correctly accepting results where repositories only differ by '.git' #7741

Changes from 1 commit
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
76008c3
Move the 'GitRepositoryProvider' to compare urls using their canonica…
francescomikulis Jul 1, 2024
27a47a2
Added tests for the 'isValidDirectory' function of the RepositoryProv…
francescomikulis Jul 5, 2024
096755c
Removed 'repositoryExists(at:)' in favor of 'isValidDirectory(_:)'. R…
francescomikulis Jul 11, 2024
f588e25
Removed some unnecessary changes, restoring the original implementation.
francescomikulis Jul 13, 2024
b3bdde3
Removed more unnecessary changes.
francescomikulis Jul 15, 2024
363272c
Clarified and expanded the tests.
francescomikulis Jul 15, 2024
f5a8a65
Removed more unnecessary changes.
francescomikulis Jul 15, 2024
51592d7
Added tests for relative-path based origin.
francescomikulis Jul 15, 2024
8e2691e
Feedback - added back the validation on the matching path extension.
francescomikulis Jul 15, 2024
0ead74d
Revert "Feedback - added back the validation on the matching path ext…
francescomikulis Jul 18, 2024
e1c678f
Updated the test InMemoryRepositoryProvider to explicitly handle repo…
Jul 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 14 additions & 2 deletions Sources/SourceControl/GitRepository.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,9 @@ import Basics
import Dispatch
import class Foundation.NSLock

import struct PackageModel.CanonicalPackageLocation
import struct PackageModel.CanonicalPackageURL

import struct TSCBasic.ByteString
import protocol TSCBasic.DiagnosticLocation
import struct TSCBasic.FileInfo
Expand Down Expand Up @@ -207,12 +210,21 @@ public struct GitRepositoryProvider: RepositoryProvider, Cancellable {

public func isValidDirectory(_ directory: Basics.AbsolutePath) throws -> Bool {
let result = try self.git.run(["-C", directory.pathString, "rev-parse", "--git-dir"])
return result == ".git" || result == "." || result == directory.pathString
return result == ".git" || result == "."
// Compare the canonical representation, which will drop any suffix
|| CanonicalPackageLocation(result) == CanonicalPackageLocation(directory.pathString)
}

public func isValidDirectory(_ directory: Basics.AbsolutePath, for repository: RepositorySpecifier) throws -> Bool {
let remoteURL = try self.git.run(["-C", directory.pathString, "config", "--get", "remote.origin.url"])
return remoteURL == repository.url
switch repository.location {
case .url(let url):
// Compare the canonical representation, which will drop any suffix and canonicalize scp-style urls
return CanonicalPackageURL(remoteURL) == CanonicalPackageURL(url.absoluteString)
case .path(let absolutePath):
// Compare the canonical representation, which will drop any suffix
return CanonicalPackageLocation(remoteURL) == CanonicalPackageLocation(absolutePath.pathString)
Copy link
Contributor

Choose a reason for hiding this comment

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

I doubt this matters in practice, but the caching logic always uses URL and it's probably better to do that here as well. That wouldn't fix the case mentioned in the description, but to fix that you could update computeCanonicalLocation to return a file scheme if one wasn't given (which seems somewhat reasonable given we're already inferring ssh if the user is git). Does that seem fine to you @neonichu?

}
}

public func copy(from sourcePath: Basics.AbsolutePath, to destinationPath: Basics.AbsolutePath) throws {
Expand Down