ok/jj
1
0
Fork 0
forked from mirrors/jj
Code Pull requests Activity

cli: parse "git fetch --branch" parameter as string pattern

Browse source 
Even though "*" can't be used as a branch name to fetch, it should be better
to explicitly enable glob matching like the other commands.
This commit is contained in:
Yuya Nishihara 2023-10-24 08:29:42 +09:00
parent 09529b63e5
commit 560b63544a
5 changed files with 132 additions and 67 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

3
CHANGELOG.md
View file

@ -47,6 +47,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
* Pushing deleted/moved branches no longer abandons the local commits referenced
by the remote branches.
* `jj git fetch --branch` now requires `glob:` prefix to expand `*` in branch
name.
### New features
* `jj workspace add` now takes a `--revision` argument.

35
cli/src/commands/git.rs
View file

@ -28,9 +28,9 @@ use jj_lib::workspace::Workspace;
use maplit::hashset;
use crate::cli_util::{
print_failed_git_export, print_git_import_stats, resolve_multiple_nonempty_revsets,
short_change_hash, short_commit_hash, user_error, user_error_with_hint, CommandError,
CommandHelper, RevisionArg, WorkspaceCommandHelper,
parse_string_pattern, print_failed_git_export, print_git_import_stats,
resolve_multiple_nonempty_revsets, short_change_hash, short_commit_hash, user_error,
user_error_with_hint, CommandError, CommandHelper, RevisionArg, WorkspaceCommandHelper,
};
use crate::commands::make_branch_term;
use crate::progress::Progress;
@ -98,10 +98,10 @@ pub struct GitRemoteListArgs {}
pub struct GitFetchArgs {
/// Fetch only some of the branches
///
/// Any `*` in the argument is expanded as a glob. So, one `--branch` can
/// match several branches.
#[arg(long)]
branch: Vec<String>,
/// By default, the specified name matches exactly. Use `glob:` prefix to
/// expand `*` as a glob. The other wildcard characters aren't supported.
#[arg(long, value_parser = parse_string_pattern)]
branch: Vec<StringPattern>,
/// The remote to fetch from (only named remotes are supported, can be
/// repeated)
#[arg(long = "remote", value_name = "remote")]
@ -313,21 +313,32 @@ fn cmd_git_fetch(
"fetch from git remote(s) {}",
remotes.iter().join(",")
));
// TODO: maybe this should error out if the pattern contained meta
// characters and is not prefixed with "glob:".
let branches = args.branch.iter().map(|b| b.as_str()).collect_vec();
for remote in remotes {
let stats = with_remote_callbacks(ui, |cb| {
git::fetch(
tx.mut_repo(),
&git_repo,
&remote,
(!branches.is_empty()).then_some(&*branches),
(!args.branch.is_empty()).then_some(&args.branch),
cb,
&command.settings().git_settings(),
)
})
.map_err(|err| match err {
GitFetchError::InvalidBranchPattern => {
if args
.branch
.iter()
.any(|pattern| pattern.as_exact().map_or(false, |s| s.contains('*')))
{
user_error_with_hint(
err.to_string(),
"Prefix the pattern with `glob:` to expand `*` as a glob",
)
} else {
user_error(err.to_string())
}
}
GitFetchError::GitImportError(err) => err.into(),
GitFetchError::InternalGitError(err) => map_git_error(err),
_ => user_error(err.to_string()),
@ -535,7 +546,7 @@ fn do_git_clone(
}
GitFetchError::GitImportError(err) => CommandError::from(err),
GitFetchError::InternalGitError(err) => map_git_error(err),
GitFetchError::InvalidGlob => {
GitFetchError::InvalidBranchPattern => {
unreachable!("we didn't provide any globs")
}
})?;

75
cli/tests/test_git_fetch.rs
View file

@ -326,7 +326,7 @@ fn test_git_fetch_conflicting_branches() {
test_env.jj_cmd_ok(
&repo_path,
&["git", "fetch", "--remote", "rem1", "--branch", "*"],
&["git", "fetch", "--remote", "rem1", "--branch", "glob:*"],
);
// This should result in a CONFLICTED branch
insta::assert_snapshot!(get_branch_output(&test_env, &repo_path), @r###"
@ -569,10 +569,17 @@ fn test_git_fetch_some_of_many_branches() {
"###);
// Test an error message
let stderr =
test_env.jj_cmd_failure(&target_jj_repo_path, &["git", "fetch", "--branch", "^:a*"]);
let stderr = test_env.jj_cmd_failure(
&target_jj_repo_path,
&["git", "fetch", "--branch", "glob:^:a*"],
);
insta::assert_snapshot!(stderr, @r###"
Error: Invalid glob provided. Globs may not contain the characters `:` or `^`.
Error: Invalid branch pattern provided. Patterns may not contain the characters `:`, `^`, `?`, `[`, `]`
"###);
let stderr = test_env.jj_cmd_failure(&target_jj_repo_path, &["git", "fetch", "--branch", "a*"]);
insta::assert_snapshot!(stderr, @r###"
Error: Invalid branch pattern provided. Patterns may not contain the characters `:`, `^`, `?`, `[`, `]`
Hint: Prefix the pattern with `glob:` to expand `*` as a glob
"###);
// Nothing in our repo before the fetch
@ -598,8 +605,10 @@ fn test_git_fetch_some_of_many_branches() {
@origin: vpupmnsl c7d4bdcb descr_for_b
"###);
// ...then fetch two others with a glob.
let (stdout, stderr) =
test_env.jj_cmd_ok(&target_jj_repo_path, &["git", "fetch", "--branch", "a*"]);
let (stdout, stderr) = test_env.jj_cmd_ok(
&target_jj_repo_path,
&["git", "fetch", "--branch", "glob:a*"],
);
insta::assert_snapshot!(stdout, @"");
insta::assert_snapshot!(stderr, @"");
insta::assert_snapshot!(get_log_output(&test_env, &target_jj_repo_path), @r###"
@ -637,12 +646,12 @@ fn test_git_fetch_some_of_many_branches() {
let source_log = create_trunk2_and_rebase_branches(&test_env, &source_git_repo_path);
insta::assert_snapshot!(source_log, @r###"
===== Source git repo contents =====
◉ 13ac032802f1 descr_for_b b
│ ◉ 010977d69c5b descr_for_a2 a2
◉ 01d115196c39 descr_for_b b
│ ◉ 31c7d94b1f29 descr_for_a2 a2
├─╯
│ ◉ 6f4e1c4dfe29 descr_for_a1 a1
│ ◉ 6df2d34cf0da descr_for_a1 a1
├─╯
@ 09430ba04a82 descr_for_trunk2 trunk2
@ 2bb3ebd2bba3 descr_for_trunk2 trunk2
◉ ff36dc55760e descr_for_trunk1 trunk1
◉ 000000000000
"###);
@ -654,7 +663,7 @@ fn test_git_fetch_some_of_many_branches() {
// Our repo before and after fetch of two branches
insta::assert_snapshot!(get_log_output(&test_env, &target_jj_repo_path), @r###"
◉ 2be688d8c664 new_descr_for_b_to_create_conflict b*
◉ 6ebd41dc4f13 new_descr_for_b_to_create_conflict b*
│ ◉ decaa3966c83 descr_for_a2 a2
├─╯
│ ◉ 359a9a02457d descr_for_a1 a1
@ -673,11 +682,11 @@ fn test_git_fetch_some_of_many_branches() {
Abandoned 1 commits that are no longer reachable.
"###);
insta::assert_snapshot!(get_log_output(&test_env, &target_jj_repo_path), @r###"
◉ 13ac032802f1 descr_for_b b?? b@origin
│ ◉ 6f4e1c4dfe29 descr_for_a1 a1
◉ 01d115196c39 descr_for_b b?? b@origin
│ ◉ 6df2d34cf0da descr_for_a1 a1
├─╯
◉ 09430ba04a82 descr_for_trunk2
│ ◉ 2be688d8c664 new_descr_for_b_to_create_conflict b??
◉ 2bb3ebd2bba3 descr_for_trunk2
│ ◉ 6ebd41dc4f13 new_descr_for_b_to_create_conflict b??
├─╯
│ ◉ decaa3966c83 descr_for_a2 a2
├─╯
@ -689,34 +698,34 @@ fn test_git_fetch_some_of_many_branches() {
// We left a2 where it was before, let's see how `jj branch list` sees this.
insta::assert_snapshot!(get_branch_output(&test_env, &target_jj_repo_path), @r###"
a1: kmuktwqx 6f4e1c4d descr_for_a1
@origin: kmuktwqx 6f4e1c4d descr_for_a1
a1: ypowunwp 6df2d34c descr_for_a1
@origin: ypowunwp 6df2d34c descr_for_a1
a2: qkvnknrk decaa396 descr_for_a2
@origin: qkvnknrk decaa396 descr_for_a2
b (conflicted):
- vpupmnsl c7d4bdcb descr_for_b
+ vpupmnsl 2be688d8 new_descr_for_b_to_create_conflict
+ twmruqrv 13ac0328 descr_for_b
@origin (behind by 1 commits): twmruqrv 13ac0328 descr_for_b
+ vpupmnsl 6ebd41dc new_descr_for_b_to_create_conflict
+ nxrpswuq 01d11519 descr_for_b
@origin (behind by 1 commits): nxrpswuq 01d11519 descr_for_b
"###);
// Now, let's fetch a2 and double-check that fetching a1 and b again doesn't do
// anything.
let (stdout, stderr) = test_env.jj_cmd_ok(
&target_jj_repo_path,
&["git", "fetch", "--branch", "b", "--branch", "a*"],
&["git", "fetch", "--branch", "b", "--branch", "glob:a*"],
);
insta::assert_snapshot!(stdout, @"");
insta::assert_snapshot!(stderr, @r###"
Abandoned 1 commits that are no longer reachable.
"###);
insta::assert_snapshot!(get_log_output(&test_env, &target_jj_repo_path), @r###"
◉ 010977d69c5b descr_for_a2 a2
│ ◉ 13ac032802f1 descr_for_b b?? b@origin
◉ 31c7d94b1f29 descr_for_a2 a2
│ ◉ 01d115196c39 descr_for_b b?? b@origin
├─╯
│ ◉ 6f4e1c4dfe29 descr_for_a1 a1
│ ◉ 6df2d34cf0da descr_for_a1 a1
├─╯
◉ 09430ba04a82 descr_for_trunk2
│ ◉ 2be688d8c664 new_descr_for_b_to_create_conflict b??
◉ 2bb3ebd2bba3 descr_for_trunk2
│ ◉ 6ebd41dc4f13 new_descr_for_b_to_create_conflict b??
├─╯
◉ ff36dc55760e descr_for_trunk1
│ @ 230dd059e1b0
@ -724,15 +733,15 @@ fn test_git_fetch_some_of_many_branches() {
◉ 000000000000
"###);
insta::assert_snapshot!(get_branch_output(&test_env, &target_jj_repo_path), @r###"
a1: kmuktwqx 6f4e1c4d descr_for_a1
@origin: kmuktwqx 6f4e1c4d descr_for_a1
a2: xwxurvnt 010977d6 descr_for_a2
@origin: xwxurvnt 010977d6 descr_for_a2
a1: ypowunwp 6df2d34c descr_for_a1
@origin: ypowunwp 6df2d34c descr_for_a1
a2: qrmzolkr 31c7d94b descr_for_a2
@origin: qrmzolkr 31c7d94b descr_for_a2
b (conflicted):
- vpupmnsl c7d4bdcb descr_for_b
+ vpupmnsl 2be688d8 new_descr_for_b_to_create_conflict
+ twmruqrv 13ac0328 descr_for_b
@origin (behind by 1 commits): twmruqrv 13ac0328 descr_for_b
+ vpupmnsl 6ebd41dc new_descr_for_b_to_create_conflict
+ nxrpswuq 01d11519 descr_for_b
@origin (behind by 1 commits): nxrpswuq 01d11519 descr_for_b
"###);
}

46
lib/src/git.rs
View file

@ -33,6 +33,7 @@ use crate::refs::BranchPushUpdate;
use crate::repo::{MutableRepo, Repo};
use crate::revset::{self, RevsetExpression};
use crate::settings::GitSettings;
use crate::str_util::StringPattern;
use crate::view::View;
/// Reserved remote name for the backing Git repo.
@ -968,12 +969,17 @@ fn rename_remote_refs(mut_repo: &mut MutableRepo, old_remote_name: &str, new_rem
}
}
const INVALID_REFSPEC_CHARS: [char; 5] = [':', '^', '?', '[', ']'];
#[derive(Error, Debug)]
pub enum GitFetchError {
#[error("No git remote named '{0}'")]
NoSuchRemote(String),
#[error("Invalid glob provided. Globs may not contain the characters `:` or `^`.")]
InvalidGlob,
#[error(
"Invalid branch pattern provided. Patterns may not contain the characters `{chars}`",
chars = INVALID_REFSPEC_CHARS.iter().join("`, `")
)]
InvalidBranchPattern,
#[error("Failed to import Git refs: {0}")]
GitImportError(#[from] GitImportError),
// TODO: I'm sure there are other errors possible, such as transport-level errors.
@ -995,24 +1001,14 @@ pub fn fetch(
mut_repo: &mut MutableRepo,
git_repo: &git2::Repository,
remote_name: &str,
branch_name_globs: Option<&[&str]>,
branch_names: Option<&[StringPattern]>,
callbacks: RemoteCallbacks<'_>,
git_settings: &GitSettings,
) -> Result<GitFetchStats, GitFetchError> {
let branch_name_filter = {
let regex = if let Some(globs) = branch_name_globs {
let result = regex::RegexSet::new(
globs
.iter()
.map(|glob| format!("^{}$", glob.replace('*', ".*"))),
)
.map_err(|_| GitFetchError::InvalidGlob)?;
tracing::debug!(?globs, ?result, "globs as regex");
Some(result)
} else {
None
};
move |branch: &str| regex.as_ref().map(|r| r.is_match(branch)).unwrap_or(true)
let branch_name_filter = |branch: &str| {
branch_names.map_or(true, |patterns| {
patterns.iter().any(|pattern| pattern.matches(branch))
})
};
// Perform a `git fetch` on the local git repo, updating the remote-tracking
@ -1032,9 +1028,17 @@ pub fn fetch(
fetch_options.remote_callbacks(callbacks);
let refspecs = {
// If no globs have been given, import all branches
let globs = branch_name_globs.unwrap_or(&["*"]);
if globs.iter().any(|g| g.contains(|c| ":^".contains(c))) {
return Err(GitFetchError::InvalidGlob);
let globs = if let Some(patterns) = branch_names {
patterns
.iter()
.map(|pattern| pattern.to_glob())
.collect::<Option<Vec<_>>>()
.ok_or(GitFetchError::InvalidBranchPattern)?
} else {
vec!["*".into()]
};
if globs.iter().any(|g| g.contains(INVALID_REFSPEC_CHARS)) {
return Err(GitFetchError::InvalidBranchPattern);
}
// At this point, we are only updating Git's remote tracking branches, not the
// local branches.
@ -1071,7 +1075,7 @@ pub fn fetch(
tracing::debug!("import_refs");
let import_stats = import_some_refs(mut_repo, git_repo, git_settings, |ref_name| {
to_remote_branch(ref_name, remote_name)
.map(&branch_name_filter)
.map(branch_name_filter)
.unwrap_or_else(|| matches!(ref_name, RefName::Tag(_)))
})?;
let stats = GitFetchStats {

40
lib/src/str_util.rs
View file

@ -14,7 +14,7 @@
//! String helpers.
use std::borrow::Borrow;
use std::borrow::{Borrow, Cow};
use std::collections::BTreeMap;
use std::fmt;
@ -98,6 +98,20 @@ impl StringPattern {
}
}
/// Converts this pattern to a glob string. Returns `None` if the pattern
/// can't be represented as a glob.
pub fn to_glob(&self) -> Option<Cow<'_, str>> {
// TODO: If we add Regex pattern, it will return None.
match self {
StringPattern::Exact(literal) => Some(glob::Pattern::escape(literal).into()),
StringPattern::Glob(pattern) => Some(pattern.as_str().into()),
StringPattern::Substring(needle) if needle.is_empty() => Some("*".into()),
StringPattern::Substring(needle) => {
Some(format!("*{}*", glob::Pattern::escape(needle)).into())
}
}
}
/// Returns true if this pattern matches the `haystack`.
pub fn matches(&self, haystack: &str) -> bool {
match self {
@ -126,3 +140,27 @@ impl fmt::Display for StringPattern {
write!(f, "{}", self.as_str())
}
}
#[cfg(test)]
mod tests {
use super::*;
#[test]
fn test_string_pattern_to_glob() {
assert_eq!(StringPattern::everything().to_glob(), Some("*".into()));
assert_eq!(StringPattern::exact("a").to_glob(), Some("a".into()));
assert_eq!(StringPattern::exact("*").to_glob(), Some("[*]".into()));
assert_eq!(
StringPattern::glob("*").unwrap().to_glob(),
Some("*".into())
);
assert_eq!(
StringPattern::Substring("a".into()).to_glob(),
Some("*a*".into())
);
assert_eq!(
StringPattern::Substring("*".into()).to_glob(),
Some("*[*]*".into())
);
}
}