-
Notifications
You must be signed in to change notification settings - Fork 40
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
Add refs argument do diff command #160
base: master
Are you sure you want to change the base?
Conversation
It is often useful to only update the PR further up the stack. Eg if you have a stack of 4 but made changes to the second one only you could run: `spr diff -r HEAD~3`. Alternatively if you made changes to all but this one (for example because you haven't made a PR from it): `spr diff -r HEAD~4..HEAD~1`
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.
Good stuff!
I think it would be good to error if the user runs spr diff
with both the -r
and -a
option, because that's ambiguous.
And bonus points for adding this to the documentation in docs/
😛
spr/src/commands/diff.rs
Outdated
/// Submit this commit as if it was cherry-picked on master. Do not base it | ||
/// on any intermediate changes between the master branch and this commit. | ||
#[clap(long)] | ||
cherry_pick: bool, | ||
} | ||
|
||
fn get_oids(refs: &String, repo: &git2::Repository) -> Result<HashSet<Oid>> { |
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.
fn get_oids(refs: &String, repo: &git2::Repository) -> Result<HashSet<Oid>> { | |
fn get_oids(refs: &str, repo: &git2::Repository) -> Result<HashSet<Oid>> { |
Functions should always take &str
rather than &String
. (Equivalent to taking std::string_view
instead of std::string const&
in C++).
spr/src/commands/diff.rs
Outdated
return Err(Error::new("Unable to cope with merge_base --refs")); | ||
} else if revspec.mode().contains(git2::RevparseMode::SINGLE) { | ||
// simple case, just return the id | ||
return Ok(HashSet::from([revspec.from().unwrap().id()])); |
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.
return Ok(HashSet::from([revspec.from().unwrap().id()])); | |
return Ok(HashSet::from([revspec.from()?.id()])); |
unwrap()
is a bit frowned upon in production code. Given this function is returning a Result
anyway, using ?
to pass on the error to the caller should work.
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.
will figure it out - but note from() is an option, not a result
spr/src/commands/diff.rs
Outdated
// If it's a range we have to walk from `from` to `to` grabbing oids | ||
let mut to = revspec.to().ok_or(Error::new("Unexpectedly no to id in range"))?.id(); | ||
let from = revspec.from().ok_or(Error::new("Unexpectedly no from id in range"))?.id(); | ||
let mut ret = HashSet::new(); | ||
loop { | ||
ret.insert(to); | ||
if to == from { | ||
// finished the walk | ||
break; | ||
} | ||
let commit_to = repo.find_commit(to)?; | ||
let mut oid_iter = commit_to.parent_ids(); | ||
let next_oid = oid_iter.next(); | ||
if let Some(_) = oid_iter.next() { | ||
return Err(Error::new("Unexpectedly had multiple parents for commit")); | ||
} | ||
match next_oid { | ||
None => return Err(Error::new("Unexpectedly no parent id for commit")), | ||
Some(id) => to = id, | ||
} | ||
} | ||
return Ok(ret); | ||
} else { | ||
return Err(Error::new("Unable to cope with this type of revspec for --refs")); | ||
} |
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.
I don't think you have to implement all this yourself. git2
has a Revwalk
class to do this kind of thing.
Something along the lines of
let to = revspec.to().ok_or(Error::new("Unexpectedly no to id in range"))?.id();
let from = revspec.from().ok_or(Error::new("Unexpectedly no from id in range"))?.id();
let mut walk = repo.revwalk()?;
walk.push(to)?;
walk.hide(from)?;
walk.map(|r| Ok(r?)).collect()
The iterator stuff in Rust is super nice. The iterator that walk.map
returns yields Result<Oid, git2::Error>
items. The collect
method of the iterator trait is able to collect Result<T, E>
into Result<HashSet<T>, E>
. But since the error type that git2 gives us is different from the one we return here, we need to put in that map(|r| Ok(r?))
thing, where the ?
operator does the conversion. (Maybe there is a more idiomatic way to the error mapping.)
The slight difference to your implementation is that the resulting set won't include them from
commit, but I think that's more aligned with Git anyway. (E.g. git log HEAD~3..HEAD
will show 3 commits and won't include HEAD~3
)
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.
yeah - thats way neater
I did this
This I haven't done - but I might still depending on how much you want it I updated the PR as it had a bug when initially creating the PR on a commit up the tree. It would sort of leave things dangling which is not ideal. Fixed this by tracking the commits we want to run pull requests on in that loop, but keeping the commit information for the rewrite_commit_message stage |
It is often useful to only update the PR further up the stack.
Eg if you have a stack of 4 but made changes to the second one only you could run:
spr diff -r HEAD~3
.Alternatively if you made changes to all but this one (for example because you haven't made a PR from it):
spr diff -r HEAD~4..HEAD~1