Add refs argument do diff command #160
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`
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.
| 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.
| 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.
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.
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)
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