Avoid name collision on fetching with appending the hash value of a dependency #2185

ikesyo · 2017-09-28T10:05:56Z

Should fix #2150. Should fix #2143.

This is inspired by https://github.com/apple/swift-package-manager/blob/98cfe1cb24573341488be985fb28d6fe9ecf8920/Sources/SourceControl/Repository.swift#L23-L33.

jdhealy · 2017-09-29T13:31:33Z

Two areas of thought on this:

Apple’s guidance from Hashable’s documentation:
Important
Hash values are not guaranteed to be equal across different executions of your program. Do not save hash values to use in a future execution.
Maybe our use of it (likewise with Swift Package Manager’s) is exceptional in regards to this, @ikesyo? Pulling in a hashing library might give us more dependable behavior, at the expense of having to write code for each property (so, some overhead).
Case-sensitivity (filesystems, GitHub request URLs, GitHub response URLs): what we are currently relying on, what this PR would rely on, what we can realistically support — something (given some free time) I hope to reexamine cross-project. Eventually and ideally, we would have documentation above Dependency regarding case-sensitivity.

Edit: grammar.

ikesyo · 2017-10-21T06:44:08Z

Maybe our use of it (likewise with Swift Package Manager’s) is exceptional in regards to this, @ikesyo? Pulling in a hashing library might give us more dependable behavior, at the expense of having to write code for each property (so, some overhead).

@jdhealy How does this look like?

dcefa9a

NachoSoto

Nice 👍

jdhealy

Still not sure about the case-sensitivity aspects of this. 🤔

ikesyo · 2017-10-23T23:39:53Z

Case-sensitivity (filesystems, GitHub request URLs, GitHub response URLs): what we are currently relying on, what this PR would rely on, what we can realistically support — something (given some free time) I hope to reexamine cross-project. Eventually and ideally, we would have documentation above Dependency regarding case-sensitivity.

@jdhealy I'm sorry but I assume you would do the job, isn't it? Please let me know your knowledge about this atm and I may succeed that.

ikesyo · 2017-11-10T04:42:59Z

Source/CarthageKit/Dependency.swift

 import Tentacle

 /// Uniquely identifies a project that can be used as a dependency.
 public enum Dependency {
 	/// A repository hosted on GitHub.com or GitHub Enterprise.
+	/// This is treated as case-insensitive.
 	case gitHub(Server, Repository)


Both Server and Repository are case-insensitive for equality:

https://github.com/mdiep/Tentacle/blob/90c57572d15c56d591e671bbe4fc37f7288df060/Sources/Tentacle/Server.swift#L54-L65

https://github.com/mdiep/Tentacle/blob/90c57572d15c56d591e671bbe4fc37f7288df060/Sources/Tentacle/Repository.swift#L28-L31

https://github.com/Carthage/Carthage/pull/2185/files#diff-d36e2d0823f4b902aeb9274469b117c0R120

ikesyo · 2017-11-10T04:45:00Z

Source/CarthageKit/Dependency.swift

 	case gitHub(Server, Repository)

 	/// An arbitrary Git repository.
+	/// This is treated as case-sensitive.
 	case git(GitURL)


GitURL's equality is based on normalizedURLString and there is no case-insensitive comparisons or case conversions in that implementation.

Carthage/Source/CarthageKit/GitURL.swift

Lines 8 to 46 in a3573d9

/// A normalized URL string, without protocol, authentication, or port

/// information. This is mostly useful for comparison, and not for any

/// actual Git operations.

internal var normalizedURLString: String {

if let parsedURL = URL(string: urlString), let host = parsedURL.host {

// Normal, valid URL.

let path = strippingGitSuffix(parsedURL.path)

return "\(host)\(path)"

} else if urlString.hasPrefix("/") // "/path/to/..."

|| urlString.hasPrefix(".") // "./path/to/...", "../path/to/..."

|| urlString.hasPrefix("~") // "~/path/to/..."

|| !urlString.contains(":") // "path/to/..." with avoiding "git@github.com:owner/name"

{

// Local path.

return strippingGitSuffix(urlString)

} else {

// scp syntax.

var strippedURLString = urlString

if let index = strippedURLString.index(of: "@") {

strippedURLString.removeSubrange(strippedURLString.startIndex...index)

}

var host = ""

if let index = strippedURLString.index(of: ":") {

host = String(strippedURLString[strippedURLString.startIndex..<index])

strippedURLString.removeSubrange(strippedURLString.startIndex...index)

}

var path = strippingGitSuffix(strippedURLString)

if !path.hasPrefix("/") {

// This probably isn't strictly legit, but we'll have a forward

// slash for other URL types.

path.insert("/", at: path.startIndex)

}

return "\(host)\(path)"

}

}

Carthage/Source/CarthageKit/GitURL.swift

Lines 64 to 66 in a3573d9

public static func == (_ lhs: GitURL, _ rhs: GitURL) -> Bool {

return lhs.normalizedURLString == rhs.normalizedURLString

}

https://github.com/Carthage/Carthage/pull/2185/files#diff-d36e2d0823f4b902aeb9274469b117c0R123

ikesyo · 2017-11-10T04:45:56Z

Source/CarthageKit/Dependency.swift

 	case git(GitURL)

 	/// A binary-only framework
+	/// This is treated as case-sensitive.
 	case binary(URL)


import Foundation URL(string: "https://google.com") == URL(string: "https://google.COM") // false

https://github.com/Carthage/Carthage/pull/2185/files#diff-d36e2d0823f4b902aeb9274469b117c0R126

mdiep · 2017-11-14T19:02:17Z

This would definitely fix the issue, but I don't think it's the best approach from an end-user perspective. Switching to a fork would require a full clone of the repository.

I think a better solution would be to always use the same repository for a given name, but to use a different git remote for each possible origin. For GitHub.com repositories, the name of the remote could just be the name of the owner. For other repositories, we could do essentially what you've done here.

The origin approach would share the git object database across all forks, leading to faster clones and saved disk space. It would also preserve everyones' caches when they upgrade to get this PR.

What do you think @ikesyo? Does that seem workable? If so, what would you think of pursuing that instead?

mdiep · 2017-11-14T19:03:15Z

Source/CarthageKit/Dependency.swift

@@ -27,6 +32,23 @@ public enum Dependency {
 		}
 	}

+	/// The unique identifier for this project which is useful as a file system path.
+	public var fileSystemIdentifier: SignalProducer<String, CarthageError> {
+		let task = Task("/usr/bin/shasum", arguments: ["-a", "256"])


I hate shelling out for things like this. But I guess there's probably not an easy Swift option. 😞

ikesyo · 2017-11-14T23:47:04Z

Hmm so let’s revert #2125 for now.

…ependency

ikesyo · 2017-12-25T04:38:11Z

Reopened and rebased on current master.

mdiep · 2018-01-02T19:15:06Z

@ikesyo What do you think of the idea I described in #2286 (comment)?

I think that the best solution here is to move the cache to ~/Library/Caches/org.carthage.CarthageKit/repositories/{ProjectName}/{OwnerOrURL}. That cleans things up quite a bit and makes the cache more navigable and debuggable.

I haven't found Xcode's derived data folder, with hash-appended directories, to be very user-friendly. And although I'm sure the Xcode team would say that you don't need to navigate there, I've found that I often do need too. In other words, there's prior-art for using the hash-appended directory name, but I don't think it's particularly good art.

stale · 2018-06-30T05:27:18Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

ikesyo · 2018-07-04T12:03:46Z

#2185 (comment)

I'd like to revisit that direction.

ikesyo requested a review from a team September 28, 2017 11:18

jdhealy self-assigned this Sep 29, 2017

ikesyo changed the title ~~Avoid name collision on fetching with appending the hash value of a dependency~~ [WIP] Avoid name collision on fetching with appending the hash value of a dependency Oct 5, 2017

ikesyo force-pushed the avoid-repo-name-collision branch from 60af54e to d40fcec Compare October 6, 2017 13:09

ikesyo force-pushed the avoid-repo-name-collision branch from d40fcec to 9ac70d4 Compare October 21, 2017 06:08

ikesyo force-pushed the avoid-repo-name-collision branch from cce9143 to dcefa9a Compare October 21, 2017 06:52

ikesyo changed the title ~~[WIP] Avoid name collision on fetching with appending the hash value of a dependency~~ Avoid name collision on fetching with appending the hash value of a dependency Oct 21, 2017

NachoSoto approved these changes Oct 23, 2017

View reviewed changes

jdhealy requested changes Oct 23, 2017

View reviewed changes

ikesyo force-pushed the avoid-repo-name-collision branch from dcefa9a to d23f94a Compare November 7, 2017 05:01

ikesyo commented Nov 10, 2017

View reviewed changes

ikesyo requested review from mdiep, NachoSoto and jdhealy November 10, 2017 04:47

ikesyo mentioned this pull request Nov 13, 2017

Update readme to mention the new resolver flag #2253

Merged

mdiep reviewed Nov 14, 2017

View reviewed changes

ikesyo closed this Nov 14, 2017

ikesyo mentioned this pull request Nov 14, 2017

Revert "Fix fetching a local repository with relative path" #2260

Merged

ikesyo mentioned this pull request Dec 16, 2017

Fix cache failing to resolve dependencies with name collision #2286

Open

ikesyo reopened this Dec 25, 2017

ikesyo added 3 commits December 25, 2017 13:35

Avoid name collision on fetching with appending the hash value of a d…

35ddaee

…ependency

Use shasum command

6088c99

Add case sensitivity info on Dependency cases

a463a6e

ikesyo force-pushed the avoid-repo-name-collision branch from f21932a to a463a6e Compare December 25, 2017 04:36

stale bot added the stale label Jun 30, 2018

stale bot removed the stale label Jul 4, 2018

ikesyo added the enhancement label Jul 4, 2018

ikesyo closed this Jun 14, 2021

ikesyo deleted the avoid-repo-name-collision branch June 14, 2021 13:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Avoid name collision on fetching with appending the hash value of a dependency #2185

Avoid name collision on fetching with appending the hash value of a dependency #2185

ikesyo commented Sep 28, 2017 •

edited

jdhealy commented Sep 29, 2017 •

edited

ikesyo commented Oct 21, 2017 •

edited

NachoSoto left a comment

jdhealy left a comment

ikesyo commented Oct 23, 2017

ikesyo Nov 10, 2017

ikesyo Nov 10, 2017

ikesyo Nov 10, 2017 •

edited

ikesyo Nov 10, 2017

ikesyo Nov 10, 2017

ikesyo Nov 10, 2017

mdiep commented Nov 14, 2017

mdiep Nov 14, 2017

ikesyo commented Nov 14, 2017 •

edited

ikesyo commented Dec 25, 2017

mdiep commented Jan 2, 2018

stale bot commented Jun 30, 2018

ikesyo commented Jul 4, 2018

	/// A normalized URL string, without protocol, authentication, or port
	/// information. This is mostly useful for comparison, and not for any
	/// actual Git operations.
	internal var normalizedURLString: String {
	if let parsedURL = URL(string: urlString), let host = parsedURL.host {
	// Normal, valid URL.
	let path = strippingGitSuffix(parsedURL.path)
	return "\(host)\(path)"
	} else if urlString.hasPrefix("/") // "/path/to/..."
	\|\| urlString.hasPrefix(".") // "./path/to/...", "../path/to/..."
	\|\| urlString.hasPrefix("~") // "~/path/to/..."
	\|\| !urlString.contains(":") // "path/to/..." with avoiding "git@github.com:owner/name"
	{
	// Local path.
	return strippingGitSuffix(urlString)
	} else {
	// scp syntax.
	var strippedURLString = urlString

	if let index = strippedURLString.index(of: "@") {
	strippedURLString.removeSubrange(strippedURLString.startIndex...index)
	}

	var host = ""
	if let index = strippedURLString.index(of: ":") {
	host = String(strippedURLString[strippedURLString.startIndex..<index])
	strippedURLString.removeSubrange(strippedURLString.startIndex...index)
	}

	var path = strippingGitSuffix(strippedURLString)
	if !path.hasPrefix("/") {
	// This probably isn't strictly legit, but we'll have a forward
	// slash for other URL types.
	path.insert("/", at: path.startIndex)
	}

	return "\(host)\(path)"
	}
	}

	public static func == (_ lhs: GitURL, _ rhs: GitURL) -> Bool {
	return lhs.normalizedURLString == rhs.normalizedURLString
	}

Avoid name collision on fetching with appending the hash value of a dependency #2185

Avoid name collision on fetching with appending the hash value of a dependency #2185

Conversation

ikesyo commented Sep 28, 2017 • edited

jdhealy commented Sep 29, 2017 • edited

ikesyo commented Oct 21, 2017 • edited

NachoSoto left a comment

Choose a reason for hiding this comment

jdhealy left a comment

Choose a reason for hiding this comment

ikesyo commented Oct 23, 2017

ikesyo Nov 10, 2017

Choose a reason for hiding this comment

ikesyo Nov 10, 2017

Choose a reason for hiding this comment

ikesyo Nov 10, 2017 • edited

Choose a reason for hiding this comment

ikesyo Nov 10, 2017

Choose a reason for hiding this comment

ikesyo Nov 10, 2017

Choose a reason for hiding this comment

ikesyo Nov 10, 2017

Choose a reason for hiding this comment

mdiep commented Nov 14, 2017

mdiep Nov 14, 2017

Choose a reason for hiding this comment

ikesyo commented Nov 14, 2017 • edited

ikesyo commented Dec 25, 2017

mdiep commented Jan 2, 2018

stale bot commented Jun 30, 2018

ikesyo commented Jul 4, 2018

ikesyo commented Sep 28, 2017 •

edited

jdhealy commented Sep 29, 2017 •

edited

ikesyo commented Oct 21, 2017 •

edited

ikesyo Nov 10, 2017 •

edited

ikesyo commented Nov 14, 2017 •

edited