Skip to content

Commit

Permalink
config: remove check for relative path patterns, add doc about path e…
Browse files Browse the repository at this point in the history
…quivalence

I originally added the check so we would never canonicalize path relative to
random cwd, but Windows CI failed because "/" is a relative path. Suppose user
would want to share the same configuration file between native Windows and WSL,
this check would be too nitpicky.
  • Loading branch information
yuja committed Dec 20, 2024
1 parent 843e0ed commit 89d3a8b
Show file tree
Hide file tree
Showing 2 changed files with 12 additions and 16 deletions.
9 changes: 9 additions & 0 deletions docs/config.md
Original file line number Diff line number Diff line change
Expand Up @@ -1256,4 +1256,13 @@ Condition keys:

* `--when.repositories`: List of paths to match the repository path prefix.

Paths should be absolute. Each path component (directory or file name, drive
letter, etc.) is compared case-sensitively on all platforms. A path starting
with `~` is expanded to the home directory. On Windows, directory separator may
be either `\` or `/`. (Beware that `\` needs escape in double-quoted strings.)

Use `jj root` to see the workspace root directory. Note that the repository path
is in the main workspace if you're using multiple workspaces with `jj
workspace`.

If no conditions are specified, table is always enabled.
19 changes: 3 additions & 16 deletions lib/src/config_resolver.rs
Original file line number Diff line number Diff line change
Expand Up @@ -68,18 +68,13 @@ impl ScopeCondition {
}

fn expand_paths(mut self, context: &ConfigResolutionContext) -> Result<Self, &'static str> {
// It might make some sense to compare paths in canonicalized form, but
// be careful to not resolve relative path patterns against cwd, which
// wouldn't be what the user would expect.
for path in self.repositories.as_mut().into_iter().flatten() {
// TODO: might be better to compare paths in canonicalized form?
if let Some(new_path) = expand_home(path, context.home_dir)? {
*path = new_path;
}
// TODO: better to skip relative paths instead of an error? The rule
// differs across platforms. "/" is relative path on Windows.
// Another option is to add "platforms = [..]" predicate and do path
// validation lazily.
if path.is_relative() {
return Err("Relative path in --when.repositories");
}
}
Ok(self)
}
Expand Down Expand Up @@ -344,7 +339,6 @@ mod tests {
}

#[test]
#[cfg(not(windows))] // '/' is not absolute path on Windows
fn test_resolve_repo_path() {
let mut source_config = StackedConfig::empty();
source_config.add_layer(new_user_layer(indoc! {"
Expand Down Expand Up @@ -419,13 +413,6 @@ mod tests {
resolve(&new_config("--when.repositories = 0"), &context),
Err(ConfigGetError::Type { .. })
);
assert_matches!(
resolve(
&new_config("--when.repositories = ['relative/path']"),
&context
),
Err(ConfigGetError::Type { .. })
);
}

#[test]
Expand Down

0 comments on commit 89d3a8b

Please sign in to comment.