-
Notifications
You must be signed in to change notification settings - Fork 27
Conversation
2b6b66d
to
e20f157
Compare
None, | ||
None, | ||
)?; | ||
working.find_commit(patch_head.into())? |
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.
Maybe move this code into a private method? That way the condition and resulting actions are clear.
Otherwise 👍 |
e20f157
to
115e339
Compare
The description is not actually optional, so don't return an `Option`.
Since the patch may not have been pulled into the working copy, we shouldn't rely on it for displaying patch information.
Before this change, `rad patch checkout` would fail if the patch commits were not in the working copy.
115e339
to
5ccf380
Compare
|
||
pub fn run( | ||
storage: &Repository, | ||
git_workdir: &git::raw::Repository, | ||
stored: &Repository, |
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.
Why the rename, out of interest?
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.
storage
is confusing when it doesn't refer to Storage
, no?
I think a nice convention if there are two repositories is stored
for the stored copy and working
for the working copy. That was the idea.
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.
Ya, that makes sense! I only noticed the working copy param after I sent off the comment, but you confirmed my hunch 😄
See commits.