Skip to content
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.

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

Add support for parsing subdirectories out of git URLs #9

Closed
wants to merge 5 commits into from
Closed
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
44 changes: 44 additions & 0 deletions src/v2/context.in.rs
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,21 @@ impl Context {
Context::Dir(Path::new(&s_ref).to_owned())
}
}

/// Determines if two Contexts are equivalent. This is only true
/// if they are the same directory path, or if they are same git
/// repository and branch. Subdirectories of a repository may differ
pub fn is_equivalent_to(&self, other: &Context) -> bool {
Copy link
Owner

Choose a reason for hiding this comment

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

I think we need a better name than equivalent here. It's hard for me to give a sensible name to this function, which makes me suspicious about the design here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I totally agree. Really, the whole purpose of this function is to figure out whether two sources conflict, i.e. this check. So it's very driven by Cage's requirements, which is to know whether or not two contexts with the same alias are compatible.

Given that it's so Cage-specific, I do wonder if it belongs in cage/src/ext/context.rs? Then it could have a more revealing name, like conflicts_with? The parsing of the git URL could stay in compose_yml, as that is a bit more general-purpose.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Failing that, a better name might be is_compatible_with? It feels a little bit better, though you'd probably still need to read its docs to know what exactly "compatible" means.

match (self, other) {
(&Context::Dir(_), &Context::GitUrl(_)) => false,
(&Context::GitUrl(_), &Context::Dir(_)) => false,
(&Context::Dir(ref dir_1), &Context::Dir(ref dir_2)) => dir_1 == dir_2,
(&Context::GitUrl(ref git_url_1), &Context::GitUrl(ref git_url_2)) => {
git_url_1.repository() == git_url_2.repository() &&
git_url_1.branch() == git_url_2.branch()
Copy link
Owner

Choose a reason for hiding this comment

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

The GitUrl-specific portion of this function should probably be moved to GitUrl, with a name like can_share_checkout_with.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

},
}
}
}

impl_interpolatable_value!(Context);
Expand Down Expand Up @@ -80,3 +95,32 @@ fn context_may_contain_dir_paths() {
assert_eq!(context.to_string(), path);
}
}

#[test]
fn is_equivalent_if_they_are_the_same_dir_or_the_same_repo_and_branch() {
let dir_1: Context = FromStr::from_str("./foo").unwrap();
let dir_2: Context = FromStr::from_str("./bar").unwrap();

let plain_repo: Context = FromStr::from_str("[email protected]:docker/docker.git").unwrap();
let repo_with_branch: Context = FromStr::from_str("[email protected]:docker/docker.git#somebranch").unwrap();
let repo_with_subdir: Context = FromStr::from_str("[email protected]:docker/docker.git#:somedir").unwrap();
let repo_with_branch_and_subdir: Context = FromStr::from_str("[email protected]:docker/docker.git#somebranch:somedir").unwrap();

let different_repo: Context = FromStr::from_str("[email protected]:docker/compose.git").unwrap();

assert!(dir_1.is_equivalent_to(&dir_1));
assert!(!dir_1.is_equivalent_to(&dir_2));
assert!(!dir_1.is_equivalent_to(&plain_repo));

assert!(plain_repo.is_equivalent_to(&plain_repo));
assert!(plain_repo.is_equivalent_to(&repo_with_subdir));

assert!(!plain_repo.is_equivalent_to(&dir_1));
assert!(!plain_repo.is_equivalent_to(&different_repo));
assert!(!plain_repo.is_equivalent_to(&repo_with_branch));
assert!(!plain_repo.is_equivalent_to(&repo_with_branch_and_subdir));

assert!(repo_with_branch.is_equivalent_to(&repo_with_branch));
assert!(repo_with_branch.is_equivalent_to(&repo_with_branch_and_subdir));
assert!(!repo_with_branch.is_equivalent_to(&plain_repo));
}
64 changes: 64 additions & 0 deletions src/v2/git_url.rs
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,34 @@ impl GitUrl {
}
}
}

/// Extract the repository part of the URL
pub fn repository(&self) -> String {
lazy_static! {
static ref REPO_PARSE: Regex = Regex::new(r#"([^#]*)"#).unwrap();
Copy link
Owner

Choose a reason for hiding this comment

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

It might be a good idea to anchor the leading edge of this regex like ^([^#]*) (or include a comment saying why it's unnecessary.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

}
REPO_PARSE.captures(&self.url).unwrap().at(1).unwrap().to_string()
}

/// Extract the optional branch part of the git URL
pub fn branch(&self) -> Option<String> {
lazy_static! {
static ref BRANCH_PARSE: Regex = Regex::new(r#".*#([^:]+)"#).unwrap();
Copy link
Owner

Choose a reason for hiding this comment

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

Now that I think about it, I'm uneasy about having two different regular expressions that try to parse the URL. It would be better to have a single function that handles all URL parsing, probably with a single regex. Also, the right hand side should probably be anchored with $.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair call, I'll figure out the one regex to rule them all.

}
BRANCH_PARSE.captures(&self.url).map(
|captures| captures.at(1).unwrap().to_string()
)
}

/// Extract the optional subdirectory part of the git URL
pub fn subdirectory(&self) -> Option<String> {
lazy_static! {
static ref SUBDIR_PARSE: Regex = Regex::new(r#".*#.*:(.+)"#).unwrap();
}
SUBDIR_PARSE.captures(&self.url).map(
|captures| captures.at(1).unwrap().to_string()
)
}
}

impl fmt::Display for GitUrl {
Expand Down Expand Up @@ -134,3 +162,39 @@ fn to_url_converts_git_urls_to_real_ones() {
assert!(GitUrl::new(url).is_err());
}
}

#[test]
fn it_can_extract_its_repo_branch_and_subdir_parts() {
let urls = &[
"git://github.com/docker/docker",
"https://github.com/docker/docker.git",
"http://github.com/docker/docker.git",
"[email protected]:docker/docker.git",
"[email protected]:atlassianlabs/atlassian-docker.git",
"github.com/docker/docker.git",
];

// Refs/folders specified as per:
// https://docs.docker.com/engine/reference/commandline/build/#git-repositories
for &url in urls {
let plain = GitUrl::new(format!("{}{}", url, "")).unwrap();
assert_eq!(plain.repository(), url);
assert_eq!(plain.branch(), None);
assert_eq!(plain.subdirectory(), None);

let with_ref = GitUrl::new(format!("{}{}", url, "#mybranch")).unwrap();
assert_eq!(with_ref.repository(), url);
assert_eq!(with_ref.branch(), Some("mybranch".to_string()));
assert_eq!(with_ref.subdirectory(), None);

let with_subdir = GitUrl::new(format!("{}{}", url, "#:myfolder")).unwrap();
assert_eq!(with_subdir.repository(), url);
assert_eq!(with_subdir.branch(), None);
assert_eq!(with_subdir.subdirectory(), Some("myfolder".to_string()));

let with_ref_and_subdir = GitUrl::new(format!("{}{}", url, "#mybranch:myfolder")).unwrap();
Copy link
Owner

Choose a reason for hiding this comment

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

I'm glad to see this test case.

assert_eq!(with_ref_and_subdir.repository(), url);
assert_eq!(with_ref_and_subdir.branch(), Some("mybranch".to_string()));
assert_eq!(with_ref_and_subdir.subdirectory(), Some("myfolder".to_string()));
}
}