Skip to content

Commit

Permalink
Remove footgun with add.branch_replacements (#92)
Browse files Browse the repository at this point in the history
If you defined `add.branch_replacements` in your `config.toml`, but the
result of applying the replacements included a `/`, `git-prole` would
naively use that as the basename of a directory, causing problems (like
the parent directory not existing).

Now, the last component of the result is used, to match the behavior
when `add.branch_replacements` is not set.

Fixes #91
  • Loading branch information
9999years authored Oct 22, 2024
1 parent 9235337 commit f649fbf
Show file tree
Hide file tree
Showing 5 changed files with 86 additions and 5 deletions.
3 changes: 3 additions & 0 deletions config.toml
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,9 @@ commands = [
# `git prole add -b ...` will create a directory named `some-ticket-title`.
# (The branch name will still be unchanged and way too long.)
#
# Note that if the result of applying your regex substitutions includes a `/`,
# the last component of the result will be used.
#
# For completeness, you can also specify how many replacements are performed:
#
# [[add.branch_replacements]]
Expand Down
16 changes: 12 additions & 4 deletions src/add.rs
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,6 @@ impl<'a> WorktreePlan<'a> {
};

self.git.worktree().add_command(
// TODO: What if the destination already exists?
&self.destination,
&AddWorktreeOpts {
force_branch,
Expand Down Expand Up @@ -170,6 +169,14 @@ impl<'a> WorktreePlan<'a> {
pub fn execute(&self) -> miette::Result<()> {
let mut command = self.command();

// Test: `add_destination_exists`
if self.destination.exists() {
return Err(miette!(
"Worktree destination {} already exists",
self.destination.display_path_cwd()
));
}

tracing::info!("{self}");
tracing::debug!("{self:#?}");

Expand All @@ -179,10 +186,11 @@ impl<'a> WorktreePlan<'a> {
'$'.if_supports_color(Stream::Stdout, |text| text.green()),
Utf8ProgramAndArgs::from(&command)
);
} else {
command.status_checked()?;
self.copy_untracked()?;
return Ok(());
}

command.status_checked()?;
self.copy_untracked()?;
self.run_commands()?;
Ok(())
}
Expand Down
15 changes: 14 additions & 1 deletion src/git/worktree/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -272,7 +272,20 @@ where
}
.into_owned();
}
dirname.into()

if dirname.contains(std::path::MAIN_SEPARATOR_STR) {
let final_component = final_component(&dirname);
tracing::warn!(
%branch,
after_replacements=%dirname,
using=%final_component,
"Applying `add.branch_replacements` substitutions resulted in a directory name which includes a `{}`",
std::path::MAIN_SEPARATOR_STR,
);
final_component.to_owned().into()
} else {
dirname.into()
}
}
}

Expand Down
21 changes: 21 additions & 0 deletions tests/add_destination_exists.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
use command_error::CommandExt;
use test_harness::GitProle;

#[test]
fn add_destination_exists() -> miette::Result<()> {
let prole = GitProle::new().unwrap();
prole.setup_worktree_repo("my-repo").unwrap();

prole.sh(r#"
cd my-repo || exit
mkdir puppy
"#)?;

prole
.cd_cmd("my-repo/main")
.args(["add", "puppy"])
.status_checked()
.unwrap_err();

Ok(())
}
36 changes: 36 additions & 0 deletions tests/config_add_branch_replacements_path_separator.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
use command_error::CommandExt;
use test_harness::GitProle;
use test_harness::WorktreeState;

#[test]
fn config_add_branch_replacements_path_separator() -> miette::Result<()> {
let prole = GitProle::new()?;
prole.setup_worktree_repo("my-repo")?;
prole.write_config(
r#"
[[add.branch_replacements]]
find = "doggy"
replace = "silly"
"#,
)?;

prole
.cd_cmd("my-repo/main")
.args(["add", "-b", "puppy/doggy"])
.status_checked()
.unwrap();

prole
.repo_state("my-repo")
.worktrees([
WorktreeState::new_bare(),
WorktreeState::new("main").branch("main"),
// Last component of the result of the replacements is used:
WorktreeState::new("silly")
.branch("puppy/doggy")
.upstream("main"),
])
.assert();

Ok(())
}

0 comments on commit f649fbf

Please sign in to comment.