From 22a78364022e0b6f8bb5099675c0c421909866d5 Mon Sep 17 00:00:00 2001 From: Luc Perkins Date: Wed, 19 Jun 2024 10:20:52 -0700 Subject: [PATCH 01/16] Enable auto-formatting subgroups in flake names --- .editorconfig | 2 + src/push_context.rs | 136 ++++++++++++++++++++++++++++++++++++++++---- 2 files changed, 127 insertions(+), 11 deletions(-) diff --git a/.editorconfig b/.editorconfig index f92d298b..107273a5 100644 --- a/.editorconfig +++ b/.editorconfig @@ -9,3 +9,5 @@ charset = utf-8 trim_trailing_whitespace = true insert_final_newline = true +[*.rs] +indent_size = 4 diff --git a/src/push_context.rs b/src/push_context.rs index 05a45fa0..8fdd1d06 100644 --- a/src/push_context.rs +++ b/src/push_context.rs @@ -103,17 +103,9 @@ impl PushContext { } else { repository.clone() }; - let mut repository_split = repository.split('/'); - let project_owner = repository_split - .next() - .ok_or_else(|| eyre!("Could not determine owner, pass `--repository` formatted like `determinatesystems/flakehub-push`"))? - .to_string(); - let project_name = repository_split.next() - .ok_or_else(|| eyre!("Could not determine project, pass `--repository` formatted like `determinatesystems/flakehub-push`"))? - .to_string(); - if repository_split.next().is_some() { - Err(eyre!("Could not determine the owner/project, pass `--repository` formatted like `determinatesystems/flakehub-push`. The passed value has too many slashes (/) to be a valid repository"))?; - } + + let (project_owner, project_name) = + get_project_owner_and_name(repository, exec_env.clone())?; let maybe_git_root = match &cli.git_root.0 { Some(gr) => Ok(gr.to_owned()), @@ -393,3 +385,125 @@ impl PushContext { Ok(ctx) } } + +fn get_project_owner_and_name( + repository: &String, + exec_env: ExecutionEnvironment, +) -> Result<(String, String)> { + let mut repository_split = repository.split('/'); + + Ok(match repository_split.clone().count() { + // Gitlab supports subgroups (repos of the form `org/subgroup/repo`). In that environment, we + // automatically "flatten" this to `org/subgroup-repo` + 3 => match exec_env { + ExecutionEnvironment::GitLab => ( + String::from( + repository_split + .next() + .expect("failed to get expected first string segment; this should never happen"), + ), + format!("{}-{}", repository_split.next().expect( + "failed to get expected second string segment; this should never happen", + ), repository_split.next().expect( + "failed to get expected third string segment; this should never happen", + ),) + ), + _ => return Err(eyre!("Could not determine project owner and name; pass `--repository` formatted like `determinatesystems/flakehub-push`")), + }, + // Outside of Gitlab subgroups we require `owner/repo` in all cases + 2 => ( + String::from( + repository_split + .next() + .expect("failed to get expected first string segment; this should never happen"), + ), + String::from( + repository_split + .next() + .expect("failed to get expected second string segment; this should never happen"), + ), + ), + // Anything that isn't of the form `owner/repo` is malformed and we tailor the suggestion to + // the environment + _ => return Err(eyre!(match exec_env { + ExecutionEnvironment::GitLab => "Could not determine project owner and name; pass `--repository` formatted like `determinatesystems/flakehub-push` or `determinatesystems/my-subgroup/flakehub-push`", + _ => "Could not determine project owner and name; pass `--repository` formatted like `determinatesystems/flakehub-push`" + })), + }) +} + +#[cfg(test)] +mod tests { + use crate::push_context::get_project_owner_and_name; + + use super::ExecutionEnvironment; + + #[test] + fn project_owner_and_name() { + let success_cases: Vec<(&str, ExecutionEnvironment, (&str, &str))> = vec![ + ( + "DeterminateSystems/flakehub", + ExecutionEnvironment::GitHub, + ("DeterminateSystems", "flakehub"), + ), + ( + "DeterminateSystems/flakehub", + ExecutionEnvironment::Local, + ("DeterminateSystems", "flakehub"), + ), + ("a/b/c", ExecutionEnvironment::GitLab, ("a", "b-c")), + ( + "DeterminateSystems/subgroup/flakehub", + ExecutionEnvironment::GitLab, + ("DeterminateSystems", "subgroup-flakehub"), + ), + ]; + + for (repo, env, (expected_owner, expected_name)) in success_cases { + assert_eq!( + (String::from(expected_owner), String::from(expected_name)), + get_project_owner_and_name(&String::from(repo), env).unwrap() + ); + } + + let failure_cases: Vec<(&str, ExecutionEnvironment, &str)> = vec![ + ( + "just-an-owner", + ExecutionEnvironment::GitHub, + "Could not determine project owner and name; pass `--repository` formatted like `determinatesystems/flakehub-push`", + ), + ( + "just-an-owner", + ExecutionEnvironment::Local, + "Could not determine project owner and name; pass `--repository` formatted like `determinatesystems/flakehub-push`", + ), + ( + "just-an-owner", + ExecutionEnvironment::GitLab, + "Could not determine project owner and name; pass `--repository` formatted like `determinatesystems/flakehub-push` or `determinatesystems/my-subgroup/flakehub-push`", + ), + ( + "DeterminateSystems/subgroup/flakehub", + ExecutionEnvironment::GitHub, + "Could not determine project owner and name; pass `--repository` formatted like `determinatesystems/flakehub-push`", + ), + ("a/b/c/d", ExecutionEnvironment::GitHub, "Could not determine project owner and name; pass `--repository` formatted like `determinatesystems/flakehub-push`"), + ( + "DeterminateSystems/subgroup/subsubgroup/flakehub", + ExecutionEnvironment::GitLab, + "Could not determine project owner and name; pass `--repository` formatted like `determinatesystems/flakehub-push` or `determinatesystems/my-subgroup/flakehub-push`", + ), + ("a/b/c/d", ExecutionEnvironment::GitLab, "Could not determine project owner and name; pass `--repository` formatted like `determinatesystems/flakehub-push` or `determinatesystems/my-subgroup/flakehub-push`"), + ]; + + for (repo, env, expected_error) in failure_cases { + assert_eq!( + String::from(expected_error), + get_project_owner_and_name(&String::from(repo), env) + .err() + .unwrap() + .to_string(), + ); + } + } +} From 7f695738929f35d852e24ff3400ae8005389e3df Mon Sep 17 00:00:00 2001 From: Luc Perkins Date: Wed, 19 Jun 2024 10:22:46 -0700 Subject: [PATCH 02/16] Fix wording in inline comment --- src/push_context.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/push_context.rs b/src/push_context.rs index 8fdd1d06..83b6d5fe 100644 --- a/src/push_context.rs +++ b/src/push_context.rs @@ -393,8 +393,8 @@ fn get_project_owner_and_name( let mut repository_split = repository.split('/'); Ok(match repository_split.clone().count() { - // Gitlab supports subgroups (repos of the form `org/subgroup/repo`). In that environment, we - // automatically "flatten" this to `org/subgroup-repo` + // Gitlab supports subgroups (repos of the form `owner/subgroup/repo`). In that environment, we + // automatically "flatten" this to `owner/subgroup-repo` 3 => match exec_env { ExecutionEnvironment::GitLab => ( String::from( From 1bb9f909791cb55a90547abe046e364c9e721e5f Mon Sep 17 00:00:00 2001 From: Luc Perkins Date: Wed, 19 Jun 2024 10:40:02 -0700 Subject: [PATCH 03/16] Streamline pattern match --- src/push_context.rs | 60 +++++++++++++++++---------------------------- 1 file changed, 22 insertions(+), 38 deletions(-) diff --git a/src/push_context.rs b/src/push_context.rs index 83b6d5fe..fa328c4b 100644 --- a/src/push_context.rs +++ b/src/push_context.rs @@ -392,44 +392,28 @@ fn get_project_owner_and_name( ) -> Result<(String, String)> { let mut repository_split = repository.split('/'); - Ok(match repository_split.clone().count() { - // Gitlab supports subgroups (repos of the form `owner/subgroup/repo`). In that environment, we - // automatically "flatten" this to `owner/subgroup-repo` - 3 => match exec_env { - ExecutionEnvironment::GitLab => ( - String::from( - repository_split - .next() - .expect("failed to get expected first string segment; this should never happen"), - ), - format!("{}-{}", repository_split.next().expect( - "failed to get expected second string segment; this should never happen", - ), repository_split.next().expect( - "failed to get expected third string segment; this should never happen", - ),) - ), - _ => return Err(eyre!("Could not determine project owner and name; pass `--repository` formatted like `determinatesystems/flakehub-push`")), - }, - // Outside of Gitlab subgroups we require `owner/repo` in all cases - 2 => ( - String::from( - repository_split - .next() - .expect("failed to get expected first string segment; this should never happen"), - ), - String::from( - repository_split - .next() - .expect("failed to get expected second string segment; this should never happen"), - ), - ), - // Anything that isn't of the form `owner/repo` is malformed and we tailor the suggestion to - // the environment - _ => return Err(eyre!(match exec_env { - ExecutionEnvironment::GitLab => "Could not determine project owner and name; pass `--repository` formatted like `determinatesystems/flakehub-push` or `determinatesystems/my-subgroup/flakehub-push`", - _ => "Could not determine project owner and name; pass `--repository` formatted like `determinatesystems/flakehub-push`" - })), - }) + let error_msg = match exec_env { + ExecutionEnvironment::GitLab => "Could not determine project owner and name; pass `--repository` formatted like `determinatesystems/flakehub-push` or `determinatesystems/my-subgroup/flakehub-push`", + _ => "Could not determine project owner and name; pass `--repository` formatted like `determinatesystems/flakehub-push`" + }; + + if repository_split.clone().count() > 3 { + return Err(eyre!(error_msg)); + }; + + match ( + repository_split.next(), + repository_split.next(), + repository_split.next(), + ) { + (Some(owner), Some(subgroup), Some(name)) + if matches!(exec_env, ExecutionEnvironment::GitLab) => + { + Ok((String::from(owner), format!("{}-{}", subgroup, name))) + } + (Some(owner), Some(name), None) => Ok((String::from(owner), String::from(name))), + _ => Err(eyre!(error_msg)), + } } #[cfg(test)] From fe3f6c16d05cc356e55a9dfc618545ff27cdbc65 Mon Sep 17 00:00:00 2001 From: Luc Perkins Date: Wed, 19 Jun 2024 10:41:06 -0700 Subject: [PATCH 04/16] Address Clippy warning --- src/push_context.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/push_context.rs b/src/push_context.rs index fa328c4b..a40fee75 100644 --- a/src/push_context.rs +++ b/src/push_context.rs @@ -387,7 +387,7 @@ impl PushContext { } fn get_project_owner_and_name( - repository: &String, + repository: &str, exec_env: ExecutionEnvironment, ) -> Result<(String, String)> { let mut repository_split = repository.split('/'); From 4c5bb311363a214a19e8e27d0cfe531db4f17f8a Mon Sep 17 00:00:00 2001 From: Luc Perkins Date: Wed, 19 Jun 2024 10:57:44 -0700 Subject: [PATCH 05/16] Support an indefinite number of subgroup segments --- src/push_context.rs | 46 ++++++++++++++++++++++++++++++++------------- 1 file changed, 33 insertions(+), 13 deletions(-) diff --git a/src/push_context.rs b/src/push_context.rs index a40fee75..0b999a93 100644 --- a/src/push_context.rs +++ b/src/push_context.rs @@ -397,7 +397,7 @@ fn get_project_owner_and_name( _ => "Could not determine project owner and name; pass `--repository` formatted like `determinatesystems/flakehub-push`" }; - if repository_split.clone().count() > 3 { + if !matches!(exec_env, ExecutionEnvironment::GitLab) && repository_split.clone().count() > 3 { return Err(eyre!(error_msg)); }; @@ -409,7 +409,18 @@ fn get_project_owner_and_name( (Some(owner), Some(subgroup), Some(name)) if matches!(exec_env, ExecutionEnvironment::GitLab) => { - Ok((String::from(owner), format!("{}-{}", subgroup, name))) + Ok((String::from(owner), { + // Gitlab subgroups can be nested quite deeply. This logic supports any level of nesting + // by appending `-{segment}` for each additional segment. + let mut s = String::from(subgroup); + s.push('-'); + s.push_str(name); + while let Some(segment) = repository_split.next() { + s.push('-'); + s.push_str(segment); + } + s + })) } (Some(owner), Some(name), None) => Ok((String::from(owner), String::from(name))), _ => Err(eyre!(error_msg)), @@ -436,17 +447,34 @@ mod tests { ("DeterminateSystems", "flakehub"), ), ("a/b/c", ExecutionEnvironment::GitLab, ("a", "b-c")), + ( + "a/b/c/d/e/f/g/h", + ExecutionEnvironment::GitLab, + ("a", "b-c-d-e-f-g-h"), + ), + ( + "a/b/c/d/e/f/g/h/i/j/k/l", + ExecutionEnvironment::GitLab, + ("a", "b-c-d-e-f-g-h-i-j-k-l"), + ), ( "DeterminateSystems/subgroup/flakehub", ExecutionEnvironment::GitLab, ("DeterminateSystems", "subgroup-flakehub"), ), + ( + "DeterminateSystems/subgroup/subsubgroup/flakehub", + ExecutionEnvironment::GitLab, + ("DeterminateSystems", "subgroup-subsubgroup-flakehub"), + ), ]; for (repo, env, (expected_owner, expected_name)) in success_cases { + let (owner, name) = get_project_owner_and_name(&String::from(repo), env).unwrap(); assert_eq!( (String::from(expected_owner), String::from(expected_name)), - get_project_owner_and_name(&String::from(repo), env).unwrap() + (owner.clone(), name.clone()), + "expected {expected_owner}/{expected_name} from repository {repo} but got {owner}/{name}" ); } @@ -472,21 +500,13 @@ mod tests { "Could not determine project owner and name; pass `--repository` formatted like `determinatesystems/flakehub-push`", ), ("a/b/c/d", ExecutionEnvironment::GitHub, "Could not determine project owner and name; pass `--repository` formatted like `determinatesystems/flakehub-push`"), - ( - "DeterminateSystems/subgroup/subsubgroup/flakehub", - ExecutionEnvironment::GitLab, - "Could not determine project owner and name; pass `--repository` formatted like `determinatesystems/flakehub-push` or `determinatesystems/my-subgroup/flakehub-push`", - ), - ("a/b/c/d", ExecutionEnvironment::GitLab, "Could not determine project owner and name; pass `--repository` formatted like `determinatesystems/flakehub-push` or `determinatesystems/my-subgroup/flakehub-push`"), ]; for (repo, env, expected_error) in failure_cases { + let result = get_project_owner_and_name(&String::from(repo), env); assert_eq!( String::from(expected_error), - get_project_owner_and_name(&String::from(repo), env) - .err() - .unwrap() - .to_string(), + result.err().unwrap().to_string(), ); } } From df88aae56902ae82180b7a6b6c1c8e8d6820de39 Mon Sep 17 00:00:00 2001 From: Luc Perkins Date: Wed, 19 Jun 2024 11:01:50 -0700 Subject: [PATCH 06/16] Fix Clippy warning about while-let construction --- src/push_context.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/push_context.rs b/src/push_context.rs index 0b999a93..3917ad00 100644 --- a/src/push_context.rs +++ b/src/push_context.rs @@ -415,7 +415,7 @@ fn get_project_owner_and_name( let mut s = String::from(subgroup); s.push('-'); s.push_str(name); - while let Some(segment) = repository_split.next() { + for segment in repository_split { s.push('-'); s.push_str(segment); } From a38ae3d95aae7e24ad118956ac86755b362eee90 Mon Sep 17 00:00:00 2001 From: Luc Perkins Date: Wed, 19 Jun 2024 11:06:35 -0700 Subject: [PATCH 07/16] Replace iterator with fold function --- src/push_context.rs | 23 +++++++++++------------ 1 file changed, 11 insertions(+), 12 deletions(-) diff --git a/src/push_context.rs b/src/push_context.rs index 3917ad00..89d8a1b9 100644 --- a/src/push_context.rs +++ b/src/push_context.rs @@ -409,18 +409,17 @@ fn get_project_owner_and_name( (Some(owner), Some(subgroup), Some(name)) if matches!(exec_env, ExecutionEnvironment::GitLab) => { - Ok((String::from(owner), { - // Gitlab subgroups can be nested quite deeply. This logic supports any level of nesting - // by appending `-{segment}` for each additional segment. - let mut s = String::from(subgroup); - s.push('-'); - s.push_str(name); - for segment in repository_split { - s.push('-'); - s.push_str(segment); - } - s - })) + // Gitlab subgroups can be nested quite deeply. This logic supports any level of nesting + // by appending `-{segment}` for each additional segment. + let name_from_segments = + repository_split.fold(String::from(name), |mut acc, segment| { + acc.push_str(&format!("-{segment}")); + acc + }); + Ok(( + String::from(owner), + format!("{}-{}", subgroup, name_from_segments), + )) } (Some(owner), Some(name), None) => Ok((String::from(owner), String::from(name))), _ => Err(eyre!(error_msg)), From 8406f3af78b6cc77b7532279ec08019234ad6d20 Mon Sep 17 00:00:00 2001 From: Luc Perkins Date: Wed, 19 Jun 2024 11:29:43 -0700 Subject: [PATCH 08/16] Provide flag/environment variable to disable subgroups --- src/cli/mod.rs | 6 ++++++ src/push_context.rs | 24 +++++++++++++++++------- 2 files changed, 23 insertions(+), 7 deletions(-) diff --git a/src/cli/mod.rs b/src/cli/mod.rs index 4da5ef33..8b41ac71 100644 --- a/src/cli/mod.rs +++ b/src/cli/mod.rs @@ -108,6 +108,12 @@ pub(crate) struct FlakeHubPushCli { #[clap(long, env = "FLAKEHUB_PUSH_INCLUDE_OUTPUT_PATHS", value_parser = EmptyBoolParser, default_value_t = false)] pub(crate) include_output_paths: bool, + + // Gitlab has a concept of subgroups, which enables repo names like https://gitlab.com/a/b/c/d/e/f/g. By default, + // flakehub-push would parse that to flake name `a/b-c-d-e-f-g`. This flag/environment variable provides a + // mechanism to disable this behavior. + #[clap(long, env = "FLAKEHUB_PUSH_DISABLE_SUBGROUPS", default_value_t = false)] + pub(crate) disable_subgroups: bool, } #[derive(Clone, Debug)] diff --git a/src/push_context.rs b/src/push_context.rs index 89d8a1b9..7e41bbe9 100644 --- a/src/push_context.rs +++ b/src/push_context.rs @@ -105,7 +105,7 @@ impl PushContext { }; let (project_owner, project_name) = - get_project_owner_and_name(repository, exec_env.clone())?; + get_project_owner_and_name(repository, exec_env.clone(), cli.disable_subgroups)?; let maybe_git_root = match &cli.git_root.0 { Some(gr) => Ok(gr.to_owned()), @@ -389,6 +389,7 @@ impl PushContext { fn get_project_owner_and_name( repository: &str, exec_env: ExecutionEnvironment, + disable_subgroups: bool, ) -> Result<(String, String)> { let mut repository_split = repository.split('/'); @@ -407,7 +408,7 @@ fn get_project_owner_and_name( repository_split.next(), ) { (Some(owner), Some(subgroup), Some(name)) - if matches!(exec_env, ExecutionEnvironment::GitLab) => + if !disable_subgroups && matches!(exec_env, ExecutionEnvironment::GitLab) => { // Gitlab subgroups can be nested quite deeply. This logic supports any level of nesting // by appending `-{segment}` for each additional segment. @@ -469,7 +470,8 @@ mod tests { ]; for (repo, env, (expected_owner, expected_name)) in success_cases { - let (owner, name) = get_project_owner_and_name(&String::from(repo), env).unwrap(); + let (owner, name) = + get_project_owner_and_name(&String::from(repo), env, false).unwrap(); assert_eq!( (String::from(expected_owner), String::from(expected_name)), (owner.clone(), name.clone()), @@ -477,32 +479,40 @@ mod tests { ); } - let failure_cases: Vec<(&str, ExecutionEnvironment, &str)> = vec![ + let failure_cases: Vec<(&str, ExecutionEnvironment, &str, bool)> = vec![ ( "just-an-owner", ExecutionEnvironment::GitHub, "Could not determine project owner and name; pass `--repository` formatted like `determinatesystems/flakehub-push`", + false, ), ( "just-an-owner", ExecutionEnvironment::Local, "Could not determine project owner and name; pass `--repository` formatted like `determinatesystems/flakehub-push`", + false, ), ( "just-an-owner", ExecutionEnvironment::GitLab, "Could not determine project owner and name; pass `--repository` formatted like `determinatesystems/flakehub-push` or `determinatesystems/my-subgroup/flakehub-push`", + false, ), ( "DeterminateSystems/subgroup/flakehub", ExecutionEnvironment::GitHub, "Could not determine project owner and name; pass `--repository` formatted like `determinatesystems/flakehub-push`", + false, ), - ("a/b/c/d", ExecutionEnvironment::GitHub, "Could not determine project owner and name; pass `--repository` formatted like `determinatesystems/flakehub-push`"), + ("a/b/c/d", ExecutionEnvironment::GitHub, "Could not determine project owner and name; pass `--repository` formatted like `determinatesystems/flakehub-push`", false), + // In these cases, --disable-subgroups is set, which makes this Gitlab repo name work like GitHub and local, + // that is, names of the form `owner/name` are required. + ("a/b/c/d/e/f/g", ExecutionEnvironment::GitLab, "Could not determine project owner and name; pass `--repository` formatted like `determinatesystems/flakehub-push` or `determinatesystems/my-subgroup/flakehub-push`", true), + ("a/b/c", ExecutionEnvironment::GitLab, "Could not determine project owner and name; pass `--repository` formatted like `determinatesystems/flakehub-push` or `determinatesystems/my-subgroup/flakehub-push`", true), ]; - for (repo, env, expected_error) in failure_cases { - let result = get_project_owner_and_name(&String::from(repo), env); + for (repo, env, expected_error, disable_subgroups) in failure_cases { + let result = get_project_owner_and_name(&String::from(repo), env, disable_subgroups); assert_eq!( String::from(expected_error), result.err().unwrap().to_string(), From 1f8a5c469a9dbdb03613299cebe3ab4cb5ef05ca Mon Sep 17 00:00:00 2001 From: Luc Perkins Date: Wed, 19 Jun 2024 11:40:25 -0700 Subject: [PATCH 09/16] Streamline subgroups allowed/disallowed --- src/push_context.rs | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/src/push_context.rs b/src/push_context.rs index 7e41bbe9..53ae95c0 100644 --- a/src/push_context.rs +++ b/src/push_context.rs @@ -389,16 +389,20 @@ impl PushContext { fn get_project_owner_and_name( repository: &str, exec_env: ExecutionEnvironment, - disable_subgroups: bool, + subgroups_explicitly_disabled: bool, ) -> Result<(String, String)> { + let subgroups_enabled = + !subgroups_explicitly_disabled && matches!(exec_env, ExecutionEnvironment::GitLab); + let mut repository_split = repository.split('/'); - let error_msg = match exec_env { - ExecutionEnvironment::GitLab => "Could not determine project owner and name; pass `--repository` formatted like `determinatesystems/flakehub-push` or `determinatesystems/my-subgroup/flakehub-push`", - _ => "Could not determine project owner and name; pass `--repository` formatted like `determinatesystems/flakehub-push`" + let error_msg = if subgroups_enabled { + "Could not determine project owner and name; pass `--repository` formatted like `determinatesystems/flakehub-push` or `determinatesystems/my-subgroup/flakehub-push`" + } else { + "Could not determine project owner and name; pass `--repository` formatted like `determinatesystems/flakehub-push`" }; - if !matches!(exec_env, ExecutionEnvironment::GitLab) && repository_split.clone().count() > 3 { + if !subgroups_enabled && repository_split.clone().count() > 3 { return Err(eyre!(error_msg)); }; @@ -407,9 +411,7 @@ fn get_project_owner_and_name( repository_split.next(), repository_split.next(), ) { - (Some(owner), Some(subgroup), Some(name)) - if !disable_subgroups && matches!(exec_env, ExecutionEnvironment::GitLab) => - { + (Some(owner), Some(subgroup), Some(name)) if subgroups_enabled => { // Gitlab subgroups can be nested quite deeply. This logic supports any level of nesting // by appending `-{segment}` for each additional segment. let name_from_segments = @@ -507,8 +509,8 @@ mod tests { ("a/b/c/d", ExecutionEnvironment::GitHub, "Could not determine project owner and name; pass `--repository` formatted like `determinatesystems/flakehub-push`", false), // In these cases, --disable-subgroups is set, which makes this Gitlab repo name work like GitHub and local, // that is, names of the form `owner/name` are required. - ("a/b/c/d/e/f/g", ExecutionEnvironment::GitLab, "Could not determine project owner and name; pass `--repository` formatted like `determinatesystems/flakehub-push` or `determinatesystems/my-subgroup/flakehub-push`", true), - ("a/b/c", ExecutionEnvironment::GitLab, "Could not determine project owner and name; pass `--repository` formatted like `determinatesystems/flakehub-push` or `determinatesystems/my-subgroup/flakehub-push`", true), + ("a/b/c/d/e/f/g", ExecutionEnvironment::GitLab, "Could not determine project owner and name; pass `--repository` formatted like `determinatesystems/flakehub-push`", true), + ("a/b/c", ExecutionEnvironment::GitLab, "Could not determine project owner and name; pass `--repository` formatted like `determinatesystems/flakehub-push`", true), ]; for (repo, env, expected_error, disable_subgroups) in failure_cases { From b37716de4e081b858d566959910b3543e068bf67 Mon Sep 17 00:00:00 2001 From: Luc Perkins Date: Wed, 19 Jun 2024 11:44:00 -0700 Subject: [PATCH 10/16] Change flag and env var name --- src/cli/mod.rs | 8 ++++++-- src/push_context.rs | 6 +++--- 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/src/cli/mod.rs b/src/cli/mod.rs index 8b41ac71..b20ec8c4 100644 --- a/src/cli/mod.rs +++ b/src/cli/mod.rs @@ -112,8 +112,12 @@ pub(crate) struct FlakeHubPushCli { // Gitlab has a concept of subgroups, which enables repo names like https://gitlab.com/a/b/c/d/e/f/g. By default, // flakehub-push would parse that to flake name `a/b-c-d-e-f-g`. This flag/environment variable provides a // mechanism to disable this behavior. - #[clap(long, env = "FLAKEHUB_PUSH_DISABLE_SUBGROUPS", default_value_t = false)] - pub(crate) disable_subgroups: bool, + #[clap( + long, + env = "FLAKEHUB_PUSH_DISABLE_RENAME_SUBGROUPS", + default_value_t = true + )] + pub(crate) disable_rename_subgroups: bool, } #[derive(Clone, Debug)] diff --git a/src/push_context.rs b/src/push_context.rs index 53ae95c0..fe27f583 100644 --- a/src/push_context.rs +++ b/src/push_context.rs @@ -105,7 +105,7 @@ impl PushContext { }; let (project_owner, project_name) = - get_project_owner_and_name(repository, exec_env.clone(), cli.disable_subgroups)?; + get_project_owner_and_name(repository, exec_env.clone(), cli.disable_rename_subgroups)?; let maybe_git_root = match &cli.git_root.0 { Some(gr) => Ok(gr.to_owned()), @@ -389,10 +389,10 @@ impl PushContext { fn get_project_owner_and_name( repository: &str, exec_env: ExecutionEnvironment, - subgroups_explicitly_disabled: bool, + renaming_subgroups_explicitly_disabled: bool, ) -> Result<(String, String)> { let subgroups_enabled = - !subgroups_explicitly_disabled && matches!(exec_env, ExecutionEnvironment::GitLab); + !renaming_subgroups_explicitly_disabled && matches!(exec_env, ExecutionEnvironment::GitLab); let mut repository_split = repository.split('/'); From 7ef3f6e357473383ddc4b61283ae311d14275f10 Mon Sep 17 00:00:00 2001 From: Luc Perkins Date: Wed, 19 Jun 2024 12:04:42 -0700 Subject: [PATCH 11/16] Streamline folding operation again --- src/push_context.rs | 27 ++++++++++++++++++++++++--- 1 file changed, 24 insertions(+), 3 deletions(-) diff --git a/src/push_context.rs b/src/push_context.rs index fe27f583..883c4065 100644 --- a/src/push_context.rs +++ b/src/push_context.rs @@ -391,21 +391,41 @@ fn get_project_owner_and_name( exec_env: ExecutionEnvironment, renaming_subgroups_explicitly_disabled: bool, ) -> Result<(String, String)> { - let subgroups_enabled = + let subgroup_renaming_enabled = !renaming_subgroups_explicitly_disabled && matches!(exec_env, ExecutionEnvironment::GitLab); let mut repository_split = repository.split('/'); - let error_msg = if subgroups_enabled { + let error_msg = if subgroup_renaming_enabled { "Could not determine project owner and name; pass `--repository` formatted like `determinatesystems/flakehub-push` or `determinatesystems/my-subgroup/flakehub-push`" } else { "Could not determine project owner and name; pass `--repository` formatted like `determinatesystems/flakehub-push`" }; - if !subgroups_enabled && repository_split.clone().count() > 3 { + if !subgroup_renaming_enabled && repository_split.clone().count() > 2 { return Err(eyre!(error_msg)); }; + match (repository_split.next(), repository_split.next()) { + (Some(owner), Some(name)) => { + if subgroup_renaming_enabled { + Ok(( + String::from(owner), + // If subgroup renaming is enabled, all segments past the first are treated + // equally and joined by dashes + repository_split.fold(String::from(name), |mut acc, segment| { + acc.push_str(&format!("-{segment}")); + acc + }), + )) + } else { + Ok((String::from(owner), String::from(name))) + } + } + _ => Err(eyre!(error_msg)), + } + + /* match ( repository_split.next(), repository_split.next(), @@ -427,6 +447,7 @@ fn get_project_owner_and_name( (Some(owner), Some(name), None) => Ok((String::from(owner), String::from(name))), _ => Err(eyre!(error_msg)), } + */ } #[cfg(test)] From 1cbcf4cfcd9902f4b06d70ca0650c021ff47139a Mon Sep 17 00:00:00 2001 From: Luc Perkins Date: Wed, 19 Jun 2024 12:07:33 -0700 Subject: [PATCH 12/16] Remove commented-out prior solution --- src/push_context.rs | 24 ------------------------ 1 file changed, 24 deletions(-) diff --git a/src/push_context.rs b/src/push_context.rs index 883c4065..2333d3de 100644 --- a/src/push_context.rs +++ b/src/push_context.rs @@ -424,30 +424,6 @@ fn get_project_owner_and_name( } _ => Err(eyre!(error_msg)), } - - /* - match ( - repository_split.next(), - repository_split.next(), - repository_split.next(), - ) { - (Some(owner), Some(subgroup), Some(name)) if subgroups_enabled => { - // Gitlab subgroups can be nested quite deeply. This logic supports any level of nesting - // by appending `-{segment}` for each additional segment. - let name_from_segments = - repository_split.fold(String::from(name), |mut acc, segment| { - acc.push_str(&format!("-{segment}")); - acc - }); - Ok(( - String::from(owner), - format!("{}-{}", subgroup, name_from_segments), - )) - } - (Some(owner), Some(name), None) => Ok((String::from(owner), String::from(name))), - _ => Err(eyre!(error_msg)), - } - */ } #[cfg(test)] From f686e928b1daac7ef9968bf9d32f9dffb24ac11e Mon Sep 17 00:00:00 2001 From: Luc Perkins Date: Wed, 19 Jun 2024 12:31:23 -0700 Subject: [PATCH 13/16] Streamline folding logic a bit further --- src/push_context.rs | 24 +++++++++++------------- 1 file changed, 11 insertions(+), 13 deletions(-) diff --git a/src/push_context.rs b/src/push_context.rs index 2333d3de..357e8967 100644 --- a/src/push_context.rs +++ b/src/push_context.rs @@ -407,21 +407,19 @@ fn get_project_owner_and_name( }; match (repository_split.next(), repository_split.next()) { - (Some(owner), Some(name)) => { + (Some(owner), Some(name)) => Ok(( + String::from(owner), + // If subgroup renaming is enabled, all segments past the first are treated + // equally and joined by dashes if subgroup_renaming_enabled { - Ok(( - String::from(owner), - // If subgroup renaming is enabled, all segments past the first are treated - // equally and joined by dashes - repository_split.fold(String::from(name), |mut acc, segment| { - acc.push_str(&format!("-{segment}")); - acc - }), - )) + repository_split.fold(String::from(name), |mut acc, segment| { + acc.push_str(&format!("-{segment}")); + acc + }) } else { - Ok((String::from(owner), String::from(name))) - } - } + String::from(name) + }, + )), _ => Err(eyre!(error_msg)), } } From 2b9b1073138cd866f7f00328f65575a9fa1584ef Mon Sep 17 00:00:00 2001 From: Luc Perkins Date: Wed, 19 Jun 2024 13:55:01 -0700 Subject: [PATCH 14/16] Tighten up test cases and fix Clippy warning --- src/push_context.rs | 269 +++++++++++++++++++++++--------------------- 1 file changed, 142 insertions(+), 127 deletions(-) diff --git a/src/push_context.rs b/src/push_context.rs index 357e8967..f6aea276 100644 --- a/src/push_context.rs +++ b/src/push_context.rs @@ -86,26 +86,8 @@ impl PushContext { return Err(eyre!("Could not determine repository name, pass `--repository` formatted like `determinatesystems/flakehub-push`")); }; - // If the upload name is supplied by the user, ensure that it contains exactly - // one slash and no whitespace. Default to the repository name. - let upload_name = if let Some(ref name) = cli.name.0 { - let num_slashes = name.matches('/').count(); - - if num_slashes == 0 - || num_slashes > 1 - || !name.is_ascii() - || name.contains(char::is_whitespace) - { - return Err(eyre!("The argument `--name` must be in the format of `owner-name/repo-name` and cannot contain whitespace or other special characters")); - } else { - name.to_string() - } - } else { - repository.clone() - }; - - let (project_owner, project_name) = - get_project_owner_and_name(repository, exec_env.clone(), cli.disable_rename_subgroups)?; + let (upload_name, project_owner, project_name) = + determine_names(&cli.name.0, repository, cli.disable_rename_subgroups)?; let maybe_git_root = match &cli.git_root.0 { Some(gr) => Ok(gr.to_owned()), @@ -386,133 +368,166 @@ impl PushContext { } } -fn get_project_owner_and_name( +fn determine_names( + explicitly_provided_name: &Option, repository: &str, - exec_env: ExecutionEnvironment, - renaming_subgroups_explicitly_disabled: bool, -) -> Result<(String, String)> { - let subgroup_renaming_enabled = - !renaming_subgroups_explicitly_disabled && matches!(exec_env, ExecutionEnvironment::GitLab); - - let mut repository_split = repository.split('/'); - - let error_msg = if subgroup_renaming_enabled { - "Could not determine project owner and name; pass `--repository` formatted like `determinatesystems/flakehub-push` or `determinatesystems/my-subgroup/flakehub-push`" + subgroup_renaming_explicitly_disabled: bool, +) -> Result<(String, String, String)> { + // If a flake name is explicitly provided, validate that name, otherwise use the + // inferred repository name + let upload_name = if let Some(name) = explicitly_provided_name { + let num_slashes = name.matches('/').count(); + + if num_slashes == 0 + || !name.is_ascii() + || name.contains(char::is_whitespace) + // Prohibit more than one slash only if subgroup renaming is disabled + || (!subgroup_renaming_explicitly_disabled && num_slashes > 1) + { + let error_msg = if subgroup_renaming_explicitly_disabled { + "The argument `--name` must be in the format of `owner-name/repo-name` and cannot contain whitespace or other special characters" + } else { + "The argument `--name` must be in the format of `owner-name/subgroup/repo-name` and cannot contain whitespace or other special characters" + }; + return Err(eyre!(error_msg)); + } else { + name.to_string() + } } else { - "Could not determine project owner and name; pass `--repository` formatted like `determinatesystems/flakehub-push`" + String::from(repository) }; - if !subgroup_renaming_enabled && repository_split.clone().count() > 2 { - return Err(eyre!(error_msg)); + let error_msg = if subgroup_renaming_explicitly_disabled { + "Could not determine project owner and name; pass `--repository` formatted like `determinatesystems/flakehub-push`" + } else { + "Could not determine project owner and name; pass `--repository` formatted like `determinatesystems/flakehub-push` or `determinatesystems/subgroup-segments.../flakehub-push`)" }; - match (repository_split.next(), repository_split.next()) { - (Some(owner), Some(name)) => Ok(( - String::from(owner), - // If subgroup renaming is enabled, all segments past the first are treated - // equally and joined by dashes - if subgroup_renaming_enabled { - repository_split.fold(String::from(name), |mut acc, segment| { - acc.push_str(&format!("-{segment}")); - acc - }) - } else { - String::from(name) - }, - )), - _ => Err(eyre!(error_msg)), + let mut repository_split = repository.split('/'); + let project_owner = repository_split + .next() + .ok_or_else(|| eyre!(error_msg))? + .to_string(); + let project_name = repository_split + .next() + .ok_or_else(|| eyre!(error_msg))? + .to_string(); + if subgroup_renaming_explicitly_disabled && repository_split.next().is_some() { + Err(eyre!(error_msg))?; } + let project_name = if subgroup_renaming_explicitly_disabled { + project_name + } else { + repository_split.fold(project_name, |mut acc, segment| { + acc.push_str(&format!("-{segment}")); + acc + }) + }; + + Ok((upload_name, project_owner, project_name)) } #[cfg(test)] mod tests { - use crate::push_context::get_project_owner_and_name; - - use super::ExecutionEnvironment; + use crate::push_context::determine_names; #[test] fn project_owner_and_name() { - let success_cases: Vec<(&str, ExecutionEnvironment, (&str, &str))> = vec![ - ( - "DeterminateSystems/flakehub", - ExecutionEnvironment::GitHub, - ("DeterminateSystems", "flakehub"), - ), - ( - "DeterminateSystems/flakehub", - ExecutionEnvironment::Local, - ("DeterminateSystems", "flakehub"), - ), - ("a/b/c", ExecutionEnvironment::GitLab, ("a", "b-c")), - ( - "a/b/c/d/e/f/g/h", - ExecutionEnvironment::GitLab, - ("a", "b-c-d-e-f-g-h"), - ), - ( - "a/b/c/d/e/f/g/h/i/j/k/l", - ExecutionEnvironment::GitLab, - ("a", "b-c-d-e-f-g-h-i-j-k-l"), - ), - ( - "DeterminateSystems/subgroup/flakehub", - ExecutionEnvironment::GitLab, - ("DeterminateSystems", "subgroup-flakehub"), - ), - ( - "DeterminateSystems/subgroup/subsubgroup/flakehub", - ExecutionEnvironment::GitLab, - ("DeterminateSystems", "subgroup-subsubgroup-flakehub"), - ), - ]; + struct Expected { + upload_name: &'static str, + project_owner: &'static str, + project_name: &'static str, + } - for (repo, env, (expected_owner, expected_name)) in success_cases { - let (owner, name) = - get_project_owner_and_name(&String::from(repo), env, false).unwrap(); - assert_eq!( - (String::from(expected_owner), String::from(expected_name)), - (owner.clone(), name.clone()), - "expected {expected_owner}/{expected_name} from repository {repo} but got {owner}/{name}" - ); + struct TestCase { + explicit_name: Option<&'static str>, + repository: &'static str, + expected: Expected, } - let failure_cases: Vec<(&str, ExecutionEnvironment, &str, bool)> = vec![ - ( - "just-an-owner", - ExecutionEnvironment::GitHub, - "Could not determine project owner and name; pass `--repository` formatted like `determinatesystems/flakehub-push`", - false, - ), - ( - "just-an-owner", - ExecutionEnvironment::Local, - "Could not determine project owner and name; pass `--repository` formatted like `determinatesystems/flakehub-push`", - false, - ), - ( - "just-an-owner", - ExecutionEnvironment::GitLab, - "Could not determine project owner and name; pass `--repository` formatted like `determinatesystems/flakehub-push` or `determinatesystems/my-subgroup/flakehub-push`", - false, - ), - ( - "DeterminateSystems/subgroup/flakehub", - ExecutionEnvironment::GitHub, - "Could not determine project owner and name; pass `--repository` formatted like `determinatesystems/flakehub-push`", - false, - ), - ("a/b/c/d", ExecutionEnvironment::GitHub, "Could not determine project owner and name; pass `--repository` formatted like `determinatesystems/flakehub-push`", false), - // In these cases, --disable-subgroups is set, which makes this Gitlab repo name work like GitHub and local, - // that is, names of the form `owner/name` are required. - ("a/b/c/d/e/f/g", ExecutionEnvironment::GitLab, "Could not determine project owner and name; pass `--repository` formatted like `determinatesystems/flakehub-push`", true), - ("a/b/c", ExecutionEnvironment::GitLab, "Could not determine project owner and name; pass `--repository` formatted like `determinatesystems/flakehub-push`", true), + let success_cases: Vec = vec![ + TestCase { + explicit_name: Some("DeterminateSystems/flakehub-test"), + repository: "DeterminateSystems/flakehub", + expected: Expected { + upload_name: "DeterminateSystems/flakehub-test", + project_owner: "DeterminateSystems", + project_name: "flakehub", + }, + }, + TestCase { + explicit_name: None, + repository: "DeterminateSystems/flakehub", + expected: Expected { + upload_name: "DeterminateSystems/flakehub", + project_owner: "DeterminateSystems", + project_name: "flakehub", + }, + }, + TestCase { + explicit_name: Some("a/my-flake"), + repository: "a/b/c", + expected: Expected { + upload_name: "a/my-flake", + project_owner: "a", + project_name: "b-c", + }, + }, + TestCase { + explicit_name: None, + repository: "a/b/c/d/e/f/g/h", + expected: Expected { + upload_name: "a/b/c/d/e/f/g/h", + project_owner: "a", + project_name: "b-c-d-e-f-g-h", + }, + }, + TestCase { + explicit_name: None, + repository: "a/b/c/d/e/f/g/h/i/j/k/l", + expected: Expected { + upload_name: "a/b/c/d/e/f/g/h/i/j/k/l", + project_owner: "a", + project_name: "b-c-d-e-f-g-h-i-j-k-l", + }, + }, + TestCase { + explicit_name: None, + repository: "DeterminateSystems/subgroup/flakehub", + expected: Expected { + upload_name: "DeterminateSystems/subgroup/flakehub", + project_owner: "DeterminateSystems", + project_name: "subgroup-flakehub", + }, + }, + TestCase { + explicit_name: None, + repository: "DeterminateSystems/subgroup/subsubgroup/flakehub", + expected: Expected { + upload_name: "DeterminateSystems/subgroup/subsubgroup/flakehub", + project_owner: "DeterminateSystems", + project_name: "subgroup-subsubgroup-flakehub", + }, + }, ]; - for (repo, env, expected_error, disable_subgroups) in failure_cases { - let result = get_project_owner_and_name(&String::from(repo), env, disable_subgroups); + for TestCase { + explicit_name, + repository, + expected: + Expected { + upload_name: expected_upload_name, + project_owner: expected_project_owner, + project_name: expected_project_name, + }, + } in success_cases + { + let (upload_name, owner, name) = + determine_names(&explicit_name.map(String::from), repository, false).unwrap(); assert_eq!( - String::from(expected_error), - result.err().unwrap().to_string(), + (String::from(expected_upload_name), String::from(expected_project_owner), String::from(expected_project_name)), + (upload_name.clone(), owner.clone(), name.clone()), + "expected {expected_project_owner}/{expected_project_name} from repository {repository} but got {owner}/{name} instead" ); } } From 8648f6b6093b6b2f6c1063fb08fa1082044aaea0 Mon Sep 17 00:00:00 2001 From: Luc Perkins Date: Wed, 19 Jun 2024 14:14:18 -0700 Subject: [PATCH 15/16] Add failure cases --- src/push_context.rs | 125 ++++++++++++++++++++++++++++++++++++-------- 1 file changed, 103 insertions(+), 22 deletions(-) diff --git a/src/push_context.rs b/src/push_context.rs index f6aea276..c16ca2c3 100644 --- a/src/push_context.rs +++ b/src/push_context.rs @@ -382,7 +382,7 @@ fn determine_names( || !name.is_ascii() || name.contains(char::is_whitespace) // Prohibit more than one slash only if subgroup renaming is disabled - || (!subgroup_renaming_explicitly_disabled && num_slashes > 1) + || (subgroup_renaming_explicitly_disabled && num_slashes > 1) { let error_msg = if subgroup_renaming_explicitly_disabled { "The argument `--name` must be in the format of `owner-name/repo-name` and cannot contain whitespace or other special characters" @@ -439,33 +439,44 @@ mod tests { project_name: &'static str, } - struct TestCase { - explicit_name: Option<&'static str>, + struct SuccessTestCase { + explicit_upload_name: Option<&'static str>, repository: &'static str, + disable_subgroup_renaming: bool, expected: Expected, } - let success_cases: Vec = vec![ - TestCase { - explicit_name: Some("DeterminateSystems/flakehub-test"), + struct FailureTestCase { + explicit_upload_name: Option<&'static str>, + repository: &'static str, + disable_subgroup_renaming: bool, + error_msg: &'static str, + } + + let success_cases: Vec = vec![ + SuccessTestCase { + explicit_upload_name: Some("DeterminateSystems/flakehub-test"), repository: "DeterminateSystems/flakehub", + disable_subgroup_renaming: false, expected: Expected { upload_name: "DeterminateSystems/flakehub-test", project_owner: "DeterminateSystems", project_name: "flakehub", }, }, - TestCase { - explicit_name: None, + SuccessTestCase { + explicit_upload_name: None, repository: "DeterminateSystems/flakehub", + disable_subgroup_renaming: false, expected: Expected { upload_name: "DeterminateSystems/flakehub", project_owner: "DeterminateSystems", project_name: "flakehub", }, }, - TestCase { - explicit_name: Some("a/my-flake"), + SuccessTestCase { + explicit_upload_name: Some("a/my-flake"), + disable_subgroup_renaming: false, repository: "a/b/c", expected: Expected { upload_name: "a/my-flake", @@ -473,36 +484,40 @@ mod tests { project_name: "b-c", }, }, - TestCase { - explicit_name: None, + SuccessTestCase { + explicit_upload_name: None, repository: "a/b/c/d/e/f/g/h", + disable_subgroup_renaming: false, expected: Expected { upload_name: "a/b/c/d/e/f/g/h", project_owner: "a", project_name: "b-c-d-e-f-g-h", }, }, - TestCase { - explicit_name: None, + SuccessTestCase { + explicit_upload_name: None, repository: "a/b/c/d/e/f/g/h/i/j/k/l", + disable_subgroup_renaming: false, expected: Expected { upload_name: "a/b/c/d/e/f/g/h/i/j/k/l", project_owner: "a", project_name: "b-c-d-e-f-g-h-i-j-k-l", }, }, - TestCase { - explicit_name: None, + SuccessTestCase { + explicit_upload_name: None, repository: "DeterminateSystems/subgroup/flakehub", + disable_subgroup_renaming: false, expected: Expected { upload_name: "DeterminateSystems/subgroup/flakehub", project_owner: "DeterminateSystems", project_name: "subgroup-flakehub", }, }, - TestCase { - explicit_name: None, + SuccessTestCase { + explicit_upload_name: None, repository: "DeterminateSystems/subgroup/subsubgroup/flakehub", + disable_subgroup_renaming: false, expected: Expected { upload_name: "DeterminateSystems/subgroup/subsubgroup/flakehub", project_owner: "DeterminateSystems", @@ -511,9 +526,10 @@ mod tests { }, ]; - for TestCase { - explicit_name, + for SuccessTestCase { + explicit_upload_name, repository, + disable_subgroup_renaming, expected: Expected { upload_name: expected_upload_name, @@ -522,13 +538,78 @@ mod tests { }, } in success_cases { - let (upload_name, owner, name) = - determine_names(&explicit_name.map(String::from), repository, false).unwrap(); + let (upload_name, owner, name) = determine_names( + &explicit_upload_name.map(String::from), + repository, + disable_subgroup_renaming, + ) + .unwrap(); assert_eq!( (String::from(expected_upload_name), String::from(expected_project_owner), String::from(expected_project_name)), (upload_name.clone(), owner.clone(), name.clone()), "expected {expected_project_owner}/{expected_project_name} from repository {repository} but got {owner}/{name} instead" ); } + + let failure_cases: Vec = vec![ + FailureTestCase { + explicit_upload_name: None, + // Two slashes in repository with subgroup renaming disabled + repository: "a/b/c", + disable_subgroup_renaming: true, + error_msg: "Could not determine project owner and name; pass `--repository` formatted like `determinatesystems/flakehub-push`", + }, + FailureTestCase { + explicit_upload_name: None, + // No slashes in repository + repository: "a", + disable_subgroup_renaming: false, + error_msg: "Could not determine project owner and name; pass `--repository` formatted like `determinatesystems/flakehub-push` or `determinatesystems/subgroup-segments.../flakehub-push`)", + }, + FailureTestCase { + // No slashes in explicit name + explicit_upload_name: Some("zero-slashes"), + repository: "doesnt-matter", + disable_subgroup_renaming: true, + error_msg: "The argument `--name` must be in the format of `owner-name/repo-name` and cannot contain whitespace or other special characters", + }, + FailureTestCase { + // Two slashes in explicit name wit subgroup renaming disabled + explicit_upload_name: Some("a/b/c"), + repository: "a/b", + disable_subgroup_renaming: true, + error_msg: "The argument `--name` must be in the format of `owner-name/repo-name` and cannot contain whitespace or other special characters", + }, + FailureTestCase { + // Five slashes in explicit name wit subgroup renaming disabled + explicit_upload_name: Some("a/b/c/d/e/f"), + repository: "doesnt-matter", + disable_subgroup_renaming: true, + error_msg: "The argument `--name` must be in the format of `owner-name/repo-name` and cannot contain whitespace or other special characters", + }, + ]; + + for FailureTestCase { + explicit_upload_name, + repository, + disable_subgroup_renaming, + error_msg: expected_error_msg, + } in failure_cases + { + let error_msg = determine_names( + &explicit_upload_name.map(String::from), + repository, + disable_subgroup_renaming, + ) + .err() + .unwrap() + .to_string(); + + assert_eq!( + error_msg, + String::from(expected_error_msg), + "expected {} and `{repository}` to produce error message `{expected_error_msg}` but produced message `{error_msg}` instead", if let Some(ref explicit_upload_name) = &explicit_upload_name { format!("explicit upload name `{}`", explicit_upload_name) } else { String::from("no explicit upload name") }, + ); + } } } From 225cf514d73b47881e9104f1b43bf0a4e29f4614 Mon Sep 17 00:00:00 2001 From: Luc Perkins Date: Wed, 19 Jun 2024 14:16:28 -0700 Subject: [PATCH 16/16] Add inline documentation --- src/push_context.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/push_context.rs b/src/push_context.rs index c16ca2c3..2dc377b1 100644 --- a/src/push_context.rs +++ b/src/push_context.rs @@ -415,6 +415,10 @@ fn determine_names( if subgroup_renaming_explicitly_disabled && repository_split.next().is_some() { Err(eyre!(error_msg))?; } + // If subgroup renaming is disabled, the project name is just the originally provided + // name (and we've already determined that the name is of the form `{owner}/{project}`. + // But if subgroup renaming is disabled, then a repo name like `a/b/c/d/e` is converted + // to `a/b-c-d-e`. let project_name = if subgroup_renaming_explicitly_disabled { project_name } else {