Skip to content
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

Support reword in log tab & refactor external editor calls #312

Closed
wants to merge 54 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
54 commits
Select commit Hold shift + click to select a range
5aef9c8
Initial attempt
WizardOhio24 Oct 7, 2020
823b065
Create reword_safe
WizardOhio24 Oct 7, 2020
4378ecc
Remove Rebase Struct
WizardOhio24 Oct 7, 2020
7e6cf14
reword text fill with previous commit message
WizardOhio24 Oct 7, 2020
c44b64b
Remove unnessessary comment
WizardOhio24 Oct 7, 2020
b4ee5bc
Fix comment typo
WizardOhio24 Oct 7, 2020
34854d8
Remove unused head
WizardOhio24 Oct 7, 2020
6232181
Fix clippy errors
WizardOhio24 Oct 7, 2020
1a981fd
reword_safe gets current branch
WizardOhio24 Oct 7, 2020
0c8dc2a
Fix vim test
WizardOhio24 Oct 7, 2020
5d7e0f6
Modify reword function
WizardOhio24 Oct 8, 2020
269297a
Make reword private
WizardOhio24 Oct 8, 2020
a79ca04
remove except in reword component open function
WizardOhio24 Oct 8, 2020
1b51822
Remove reword from sync mod
WizardOhio24 Oct 8, 2020
b2fcc30
Keep previous selection when rewording commit
WizardOhio24 Oct 10, 2020
7f5effb
fix clippy
Oct 17, 2020
2b27d20
Use Utf8Error for reword if invalid utf8
WizardOhio24 Oct 25, 2020
7bba5d5
Add reword_safe test
WizardOhio24 Oct 26, 2020
fbbb1ac
Allow opening external editor to edit reword
WizardOhio24 Oct 26, 2020
fc31ac9
Change Tag to Reword in Reword component
WizardOhio24 Oct 26, 2020
69a290d
Fix typo in rebase
WizardOhio24 Nov 5, 2020
683a1bd
Make function to find current branch and current branch ref
WizardOhio24 Nov 5, 2020
a884286
Find current branch through get_cur_branch
WizardOhio24 Nov 7, 2020
91c122e
Rename reword and reword_safe
WizardOhio24 Nov 7, 2020
2f7e7cb
Make get_cur_branch pub(crate)
WizardOhio24 Nov 16, 2020
9007bc9
Fix implemented comment typo
WizardOhio24 Nov 16, 2020
8404ec0
Make signature_allow... pub(crate)
WizardOhio24 Nov 16, 2020
9ef17cf
Merge branch 'master' into reword
WizardOhio24 Feb 2, 2021
e4b761d
Fix get_hint error
WizardOhio24 Feb 2, 2021
074ff6d
Set reword text input not to show character count
WizardOhio24 Feb 2, 2021
4274d08
Set show character count in the reword text input to true
WizardOhio24 Feb 2, 2021
9241576
Change tag to reword commit in reword_commit_confirm
WizardOhio24 Feb 4, 2021
8f0fde7
Change get_cur_branch_ref to get_head_refname
WizardOhio24 Feb 4, 2021
97edb8f
Change to use (Option<String>, EE) for External Editor
WizardOhio24 Nov 16, 2020
e551927
small changes in app to fix build
WizardOhio24 Nov 16, 2020
233355d
Remove unessessary comment
WizardOhio24 Feb 6, 2021
5318690
Use tempfile for external editor
WizardOhio24 Feb 6, 2021
4960fbb
Fix clippy and don't drop temp file
WizardOhio24 Feb 6, 2021
db6744c
Small formatting changes
WizardOhio24 Feb 6, 2021
90d0114
Refactor show_editor into externaleditor
WizardOhio24 Feb 6, 2021
3b5f165
Small comment change
WizardOhio24 Feb 6, 2021
b70a768
Merge branch 'master' into reword
WizardOhio24 Apr 29, 2021
5905942
Fix keys
WizardOhio24 Apr 29, 2021
9b78391
Remove mod select_branch
WizardOhio24 Apr 29, 2021
d82a6b3
Remove branch
WizardOhio24 Apr 29, 2021
5e18d25
Some Fixes
WizardOhio24 Apr 29, 2021
258cc76
Fix build
WizardOhio24 Apr 29, 2021
24bed95
Small Fixes
WizardOhio24 Apr 29, 2021
f3ecf33
Remove unnessessary comments
WizardOhio24 Apr 29, 2021
a8ca289
Fix clippy errors
WizardOhio24 Apr 29, 2021
5933353
Merge branch 'master' into reword
WizardOhio24 May 23, 2021
1107a72
Fix merge problems
WizardOhio24 May 23, 2021
e140e98
Merge branch 'master' into reword
WizardOhio24 May 25, 2021
ff4afb2
Fix merge problems
WizardOhio24 May 25, 2021
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Cargo.lock

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

1 change: 1 addition & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ serde = "1.0"
anyhow = "1.0"
unicode-width = "0.1"
textwrap = "0.13"
tempfile = "3.2"
unicode-truncate = "0.2"
unicode-segmentation = "1.7"
easy-cast = "0.4"
Expand Down
6 changes: 6 additions & 0 deletions asyncgit/src/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,12 @@ pub enum Error {
#[error("git: work dir error")]
NoWorkDir,

#[error("git: no parent of commit found")]
NoParent,

#[error("git: not on a branch")]
NoBranch,

#[error("git: uncommitted changes")]
UncommittedChanges,

Expand Down
26 changes: 26 additions & 0 deletions asyncgit/src/sync/branch/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,32 @@ pub(crate) fn get_branch_name_repo(
Err(Error::NoHead)
}

/// Gets the current branch the user is on.
/// Returns none if they are not on a branch
/// and Err if there was a problem finding the branch
pub(crate) fn get_cur_branch(
repo: &Repository,
) -> Result<Option<git2::Branch>> {
for b in repo.branches(None)? {
let branch = b?.0;
if branch.is_head() {
return Ok(Some(branch));
}
}
Ok(None)
}

/// Convenience function to get the current branch reference
pub fn get_head_refname(repo_path: &str) -> Result<Option<String>> {
Copy link
Owner

Choose a reason for hiding this comment

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

pub(crate) this seems to only be needed inside asyncgit

let repo = utils::repo(repo_path)?;
if let Ok(Some(b)) = get_cur_branch(&repo) {
return Ok(Some(String::from_utf8(
b.get().name_bytes().to_vec(),
Copy link
Owner

Choose a reason for hiding this comment

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

we have bytes2string for this

)?));
}
Ok(None)
}

///
#[derive(Debug)]
pub struct LocalBranch {
Expand Down
2 changes: 2 additions & 0 deletions asyncgit/src/sync/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ mod ignore;
mod logwalker;
mod merge;
mod patches;
mod rebase;
pub mod remotes;
mod reset;
mod staging;
Expand Down Expand Up @@ -54,6 +55,7 @@ pub use logwalker::LogWalker;
pub use merge::{
abort_merge, merge_branch, merge_commit, merge_msg, mergehead_ids,
};
pub use rebase::reword;
pub use remotes::{
get_default_remote, get_remotes, push::AsyncProgress,
tags::PushTagsProgress,
Expand Down
163 changes: 163 additions & 0 deletions asyncgit/src/sync/rebase.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,163 @@
//!

use super::branch::get_head_refname;
use super::commit::signature_allow_undefined_name;
use crate::{
error::Error,
error::Result,
sync::{
branch::get_cur_branch,
utils::{self, bytes2string},
},
};
use git2::{Oid, RebaseOptions};

/// This is the same as reword, but will abort and fix the repo if something goes wrong
pub fn reword(
repo_path: &str,
commit_oid: Oid,
message: &str,
) -> Result<()> {
let repo = utils::repo(repo_path)?;
let cur_branch_ref = get_head_refname(repo_path)?;

match reword_internal(repo_path, commit_oid, message) {
Ok(()) => Ok(()),
// Something went wrong, checkout the previous branch then error
Err(e) => {
if let Ok(mut rebase) = repo.open_rebase(None) {
match cur_branch_ref {
Some(cur_branch) => {
rebase.abort()?;
repo.set_head(&cur_branch)?;
repo.checkout_head(None)?;
}
None => return Err(Error::NoBranch),
}
}
Err(e)
}
}
}

/// Changes the commit message of a commit with a specified oid
///
/// While this function is most commonly associated with doing a
/// reword opperation in an interactive rebase, that is not how it
/// is implemented in git2rs
Comment on lines +45 to +47
Copy link
Owner

Choose a reason for hiding this comment

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

what does that mean? what does it imply?

///
/// This is dangerous if it errors, as the head will be detached so this should
/// always be wrapped by another function which aborts the rebase if something goes wrong
fn reword_internal(
repo_path: &str,
commit_oid: Oid,
message: &str,
) -> Result<()> {
let repo = utils::repo(repo_path)?;
let sig = signature_allow_undefined_name(&repo)?;

let parent_commit_oid = if let Ok(parent_commit) =
repo.find_commit(commit_oid)?.parent(0)
Copy link
Owner

Choose a reason for hiding this comment

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

what if the commit is a merge commit with multiple parents? please add a unittest for that case

{
Some(parent_commit.id())
} else {
None
};

let commit_to_change = if let Some(pc_oid) = parent_commit_oid {
// Need to start at one previous to the commit, so
// first rebase.next() points to the actual commit we want to change
repo.find_annotated_commit(pc_oid)?
} else {
return Err(Error::NoParent);
};

// If we are on a branch
if let Ok(Some(branch)) = get_cur_branch(&repo) {
Copy link
Owner

Choose a reason for hiding this comment

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

lets map this into the Err we want and ? it here to early out if not on a branch, this makes the rest indent one less and easier to read

let cur_branch_ref = bytes2string(branch.get().name_bytes())?;
let cur_branch_name = bytes2string(branch.name_bytes()?)?;
let top_branch_commit = repo.find_annotated_commit(
branch.get().peel_to_commit()?.id(),
)?;

let mut rebase = repo.rebase(
Some(&top_branch_commit),
Some(&commit_to_change),
None,
Some(&mut RebaseOptions::default()),
)?;

let mut target;

rebase.next();
if parent_commit_oid.is_none() {
return Err(Error::NoParent);
}
target = rebase.commit(None, &sig, Some(message))?;

// Set target to top commit, don't know when the rebase will end
// so have to loop till end
while rebase.next().is_some() {
target = rebase.commit(None, &sig, None)?;
}
rebase.finish(None)?;

// Now override the previous branch
repo.branch(
&cur_branch_name,
&repo.find_commit(target)?,
true,
)?;

// Reset the head back to the branch then checkout head
repo.set_head(&cur_branch_ref)?;
repo.checkout_head(None)?;
return Ok(());
}
// Repo is not on a branch, possibly detached head
WizardOhio24 marked this conversation as resolved.
Show resolved Hide resolved
Err(Error::NoBranch)
}

#[cfg(test)]
mod tests {
use super::*;
use crate::sync::{
commit, stage_add_file, tests::repo_init_empty,
};
use std::{fs::File, io::Write, path::Path};

#[test]
fn test_reword() -> Result<()> {
Copy link
Owner

Choose a reason for hiding this comment

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

we currently only test the happy path, what if the rename does not go through and the rebase has to be rolled back. we need a test for that aswell

let file_path = Path::new("foo");
let (_td, repo) = repo_init_empty().unwrap();
let root = repo.path().parent().unwrap();
let repo_path = root.as_os_str().to_str().unwrap();

File::create(&root.join(file_path))?.write_all(b"a")?;
stage_add_file(repo_path, file_path).unwrap();
commit(repo_path, "commit1").unwrap();
Comment on lines +136 to +138
Copy link
Owner

Choose a reason for hiding this comment

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

the new write_commit_file makes creating/writing/committing test files more convenient/easier to read now

File::create(&root.join(file_path))?.write_all(b"ab")?;
stage_add_file(repo_path, file_path).unwrap();
let oid2 = commit(repo_path, "commit2").unwrap();

let branch =
repo.branches(None).unwrap().next().unwrap().unwrap().0;
let branch_ref = branch.get();
let commit_ref = branch_ref.peel_to_commit().unwrap();
let message = commit_ref.message().unwrap();

assert_eq!(message, "commit2");

reword(repo_path, oid2.into(), "NewCommitMessage").unwrap();

// Need to get the branch again as top oid has changed
let branch =
repo.branches(None).unwrap().next().unwrap().unwrap().0;
let branch_ref = branch.get();
let commit_ref_new = branch_ref.peel_to_commit().unwrap();
let message_new = commit_ref_new.message().unwrap();
assert_eq!(message_new, "NewCommitMessage");

Ok(())
}
}
Loading