-
-
Notifications
You must be signed in to change notification settings - Fork 575
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
Changes from all commits
5aef9c8
823b065
4378ecc
7e6cf14
c44b64b
b4ee5bc
34854d8
6232181
1a981fd
0c8dc2a
5d7e0f6
269297a
a79ca04
1b51822
b2fcc30
7f5effb
2b27d20
7bba5d5
fbbb1ac
fc31ac9
69a290d
683a1bd
a884286
91c122e
2f7e7cb
9007bc9
8404ec0
9ef17cf
e4b761d
074ff6d
4274d08
9241576
8f0fde7
97edb8f
e551927
233355d
5318690
4960fbb
db6744c
90d0114
3b5f165
b70a768
5905942
9b78391
d82a6b3
5e18d25
258cc76
24bed95
f3ecf33
a8ca289
5933353
1107a72
e140e98
ff4afb2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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>> { | ||
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(), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we have |
||
)?)); | ||
} | ||
Ok(None) | ||
} | ||
|
||
/// | ||
#[derive(Debug)] | ||
pub struct LocalBranch { | ||
|
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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. lets map this into the |
||
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<()> { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the new |
||
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(()) | ||
} | ||
} |
There was a problem hiding this comment.
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 insideasyncgit