Skip to content

[wip] check validity of branch name #759

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

Closed
wants to merge 10 commits into from
Closed
6 changes: 2 additions & 4 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 3 additions & 2 deletions asyncgit/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,9 @@ keywords = ["git"]

[dependencies]
scopetime = { path = "../scopetime", version = "0.1" }
git2 = { version = "0.13", features = ["vendored-openssl"] }
# git2 = { version = "0.13", features = ["vendored-openssl"] }
# git2 = { path = "../../github/git2-rs", features = ["vendored-openssl"]}
git2 = { git="https://github.com/rust-lang/git2-rs.git", rev="0454efb", features = ["vendored-openssl"]}
# git2 = { git="https://github.com/extrawurst/git2-rs.git", rev="513a8c9", features = ["vendored-openssl"]}
rayon-core = "1.9"
crossbeam-channel = "0.5"
Expand All @@ -28,4 +29,4 @@ easy-cast = "0.4"
tempfile = "3.2"
invalidstring = { path = "../invalidstring", version = "0.1" }
serial_test = "0.5.1"
pretty_assertions = "0.7"
pretty_assertions = "0.7"
5 changes: 5 additions & 0 deletions asyncgit/src/sync/branch/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -324,6 +324,11 @@ pub fn delete_branch(
Ok(())
}

/// checks that the branch name is valid
pub fn branch_name_is_valid(name: &str) -> Result<bool> {
Ok(git2::Branch::name_is_valid(name)?)
}

/// creates a new branch pointing to current HEAD commit and updating HEAD to new branch
pub fn create_branch(repo_path: &str, name: &str) -> Result<()> {
scope_time!("create_branch");
Expand Down
7 changes: 4 additions & 3 deletions asyncgit/src/sync/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -29,9 +29,10 @@ pub mod utils;

pub use blame::{blame_file, BlameHunk, FileBlame};
pub use branch::{
branch_compare_upstream, checkout_branch, config_is_pull_rebase,
create_branch, delete_branch, get_branch_remote,
get_branches_info, merge_commit::merge_upstream_commit,
branch_compare_upstream, branch_name_is_valid, checkout_branch,
config_is_pull_rebase, create_branch, delete_branch,
get_branch_remote, get_branches_info,
merge_commit::merge_upstream_commit,
merge_ff::branch_merge_upstream_fastforward,
merge_rebase::merge_upstream_rebase, rename::rename_branch,
BranchCompare, BranchInfo,
Expand Down
65 changes: 62 additions & 3 deletions src/components/create_branch.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,12 +12,18 @@ use crate::{
use anyhow::Result;
use asyncgit::{sync, CWD};
use crossterm::event::Event;
use tui::{backend::Backend, layout::Rect, Frame};
use easy_cast::Cast;
use tui::{
backend::Backend, layout::Rect, widgets::Paragraph, Frame,
};

pub struct CreateBranchComponent {
input: TextInputComponent,
queue: Queue,
key_config: SharedKeyConfig,
theme: SharedTheme,
invalid_name: bool,
already_exists: bool,
}

impl DrawableComponent for CreateBranchComponent {
Expand All @@ -27,6 +33,7 @@ impl DrawableComponent for CreateBranchComponent {
rect: Rect,
) -> Result<()> {
self.input.draw(f, rect)?;
self.draw_warnings(f);

Ok(())
}
Expand Down Expand Up @@ -60,8 +67,9 @@ impl Component for CreateBranchComponent {
}

if let Event::Key(e) = ev {
self.validate_input()?;
Copy link
Collaborator

Choose a reason for hiding this comment

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

does this even work? IMO every key consumed by self.input will not even go here

if e == self.key_config.enter {
self.create_branch();
self.create_branch()
}

return Ok(EventState::Consumed);
Expand Down Expand Up @@ -95,13 +103,16 @@ impl CreateBranchComponent {
Self {
queue,
input: TextInputComponent::new(
theme,
theme.clone(),
key_config.clone(),
&strings::create_branch_popup_title(&key_config),
&strings::create_branch_popup_msg(&key_config),
true,
),
key_config,
theme,
invalid_name: false,
already_exists: false,
}
}

Expand Down Expand Up @@ -137,4 +148,52 @@ impl CreateBranchComponent {
}
}
}

fn validate_input(&mut self) -> Result<()> {
let branch_name = self.input.get_text().as_str();

self.already_exists = false;
Copy link
Collaborator

Choose a reason for hiding this comment

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

since we only show either invalid_name or already exists (which is ok) we also only need to store the validity in an enum. this also means we do not need to test the for the other if the first check already fails.

Copy link
Author

Choose a reason for hiding this comment

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

ah, makes sense! will do. thanks!

self.invalid_name = false;
Copy link
Collaborator

Choose a reason for hiding this comment

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

this assignment is useless


let branches = sync::get_branches_info(CWD, true)?;
for branch in &branches {
if branch.name == self.input.get_text().as_str() {
self.already_exists = true;
}
}
self.invalid_name = sync::branch_name_is_valid(branch_name)?;
Copy link
Collaborator

Choose a reason for hiding this comment

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

this cannot work, this is inverted


Ok(())
}

fn draw_warnings<B: Backend>(&self, f: &mut Frame<B>) {
let branch_name = self.input.get_text().as_str();

let msg;
if branch_name.is_empty() {
return;
} else if self.invalid_name {
msg = strings::branch_invalid_name_warning();
} else if self.already_exists {
msg = strings::branch_already_exists();
} else {
return;
}

let msg_length: u16 = msg.len().cast();
let w = Paragraph::new(msg).style(self.theme.text_danger());

let rect = {
let mut rect = self.input.get_area();
rect.y += rect.height.saturating_sub(1);
rect.height = 1;
let offset = rect.width.saturating_sub(msg_length + 1);
rect.width = rect.width.saturating_sub(offset + 1);
rect.x += offset;

rect
};

f.render_widget(w, rect);
}
}
2 changes: 1 addition & 1 deletion src/components/rename_branch.rs
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,7 @@ impl RenameBranchComponent {
.push_back(InternalEvent::SelectBranch);
}
Err(e) => {
log::error!("create branch: {}", e,);
log::error!("rename branch: {}", e,);
self.queue.borrow_mut().push_back(
InternalEvent::ShowErrorMsg(format!(
"rename branch error:\n{}",
Expand Down
6 changes: 6 additions & 0 deletions src/strings.rs
Original file line number Diff line number Diff line change
Expand Up @@ -237,6 +237,12 @@ pub fn create_branch_popup_msg(
) -> String {
"type branch name".to_string()
}
pub fn branch_already_exists() -> String {
"[branch with this name already exists]".to_string()
}
pub fn branch_invalid_name_warning() -> String {
"[invalid branch name]".to_string()
}
pub fn username_popup_title(_key_config: &SharedKeyConfig) -> String {
"Username".to_string()
}
Expand Down