rebase -s: add support for --insert-after and --insert-before
#3607
Conversation
bnjmnt4n
force-pushed
the
bnjmnt4n/push-nptwtmlksnsm
branch
from
cac8365 to
21ee85d
Compare
bnjmnt4n
changed the title
--insert-after and --insert-before--insert-after and --insert-before
bnjmnt4n
force-pushed
the
bnjmnt4n/push-nptwtmlksnsm
branch
from
21ee85d to
26380e1
Compare
lib/src/rewrite.rs
Outdated
| // Compute the roots of `target_commits` if not provided. | ||
| let target_roots: HashSet<_> = if target_roots.is_empty() { | ||
| connected_target_commits_internal_parents | ||
| .iter() | ||
| .filter(|(commit_id, parents)| { | ||
| target_commit_ids.contains(commit_id) && parents.is_empty() | ||
| }) | ||
| .map(|(commit_id, _)| commit_id.clone()) | ||
| .collect() | ||
| } else { | ||
| target_roots.iter().cloned().collect() | ||
| }; |
lib/src/rewrite.rs
Outdated
| .map(|(commit_id, _)| commit_id.clone()) | ||
| .collect(); | ||
| // Compute the roots of `target_commits` if not provided. | ||
| let target_roots: HashSet<_> = if target_roots.is_empty() { |
There was a problem hiding this comment.
nit: make target_roots an Option instead so it's consistent (but useless, iiuc) with an empty list
There was a problem hiding this comment.
Alternatively, target_commits and target_roots could be enum Revisions(_)|Roots(_)? I assume some connectivity checks can be skipped if Roots(_).
There was a problem hiding this comment.
Alternatively,
target_commitsandtarget_rootscould be enumRevisions(_)|Roots(_)? I assume some connectivity checks can be skipped ifRoots(_).
That's definitely an option. I was thinking that since this was a library function, we might want to allow customizing consumers to specify the target set of commits and the new roots of the set anyway. WDYT? Otherwise we can also change this to an enum like you suggested.
There was a problem hiding this comment.
I don't think library function should support weird setup such as roots out of the target commits. If we can optimize for common configuration or guard against invalid parameters, I would choose restricted enum than free parameters. (They can be separate function entry points if that makes sense.)
There was a problem hiding this comment.
I'm wondering if it's fine to revisit this tweak in a separate PR? I think it would be easier for me to follow up after these commits are merged, then have to individually go back and edit each commit to update the API.
There was a problem hiding this comment.
Sorry, I didn't mean the optimization for Root(..) should be added in this PR. I just meant the argument can be changed to Option or Revisions(_)|Roots(_).
bnjmnt4n
force-pushed
the
bnjmnt4n/push-nptwtmlksnsm
branch
2 times, most recently
from
80c684f to
4e23829
Compare
bnjmnt4n
force-pushed
the
bnjmnt4n/push-nptwtmlksnsm
branch
4 times, most recently
from
df412a6 to
5c8ab13
Compare
bnjmnt4n
force-pushed
the
bnjmnt4n/push-nptwtmlksnsm
branch
from
5c8ab13 to
1a8f649
Compare
|
Oops, I accidentally force-pushed the wrong change to this branch, then deleted and recreated the branch but looks like this PR can no longer be reopened. Will recreate this. |
Checklist
If applicable:
CHANGELOG.md