cli: add `jj operation show` and `jj operation diff` commands #3617

bnjmnt4n · 2024-05-03T11:43:56Z

Checklist

If applicable:

I have updated CHANGELOG.md
I have updated the documentation (README.md, docs/, demos/)
I have updated the config schema (cli/src/config-schema.json)
I have added tests to cover my changes

lib/src/revset_graph.rs

bnjmnt4n · 2024-05-06T11:27:08Z

I'm not particularly sure what to test, any suggestions on test cases to add? Maybe some common operations like squash, rebase, git fetch and push?

bnjmnt4n · 2024-05-06T14:54:11Z

I'm also thinking that templates are a good idea, maybe something like ModifiedChange nodes and ModifiedBranch nodes if that makes sense? Although the commit summary is a good starting point, seeing things like hidden repeatedly on removed commits isn't particularly useful. I'm not sure if the branch status is useful as well since, it will also be shown in the modified branches section.

joyously · 2024-05-06T15:51:23Z

I'm not particularly sure what to test, any suggestions on test cases to add? Maybe some common operations like squash, rebase, git fetch and push?

I think you need to exercise the code for each option that the command handles.
If you expect edge cases for different operations, test that. I expect undo to work the same as other commands, but a test should prove it. Maybe abandon or branch create would be good to test that not just the commits but the meta data of the operation is shown or diffed correctly.

martinvonz · 2024-05-18T18:26:18Z

@bnjmnt4n bnjmnt4n marked this pull request as ready for review 2 weeks ago

Sorry, I missed this event. GitHub doesn't send an email for it. I'll try to remember to review this later today.

martinvonz · 2024-05-18T21:36:19Z

I think this really should have some tests. I'll review the code now.

bnjmnt4n · 2024-05-18T22:28:22Z

I'll try to add some testcases when I have the time. Here's the list of operations I think we should test (feel free to suggest more):

new
abandon
squash
rebase
branch create
git fetch
git push
op undo
op restore

I'm not sure how to structure it though: I guess I should add a single test beginning from an empty repo for each of these operations?

yuja

Seems useful, but I haven't reviewed the last patch thoroughly. It's a bit hard to comment on the features without snapshot tests.

yuja · 2024-05-19T09:10:58Z

lib/src/graph.rs

    Missing,
    Direct,
    Indirect,
 }

-fn reachable_targets(edges: &[RevsetGraphEdge]) -> impl DoubleEndedIterator<Item = &CommitId> {
+fn reachable_targets<N: Clone>(edges: &[GraphEdge<N>]) -> impl DoubleEndedIterator<Item = &N> {


nit: N: Clone isn't needed here

yuja · 2024-05-19T09:11:58Z

lib/src/graph.rs

-    pub fn new(input: impl IntoIterator<Item = (CommitId, Vec<RevsetGraphEdge>)>) -> Self {
+impl<N> ReverseGraphIterator<N>
+where
+    N: Hash + Eq + PartialEq + Clone,


nit: PartialEq is implied by Eq
(two more places that can be simplified)

yuja · 2024-05-19T09:15:33Z

cli/src/commands/operation.rs

+        _ => None,
+    };
+    let op = op_walk::resolve_op_for_load(repo_loader, &args.operation)?;
+    let parent_op = merge_operations(repo_loader, command.settings(), op.parents())?;


Maybe we can simply error out if the operation is a merge?

Since operation can't be merged manually by user, auto-merge result would be identical to the merge operation.

Here was the quick question I asked on Discord:

bnjmnt4n (04/05/2024 19:43)

What should jj op show with an operation with multiple parents be? Should it merge the parents and attempt to diff with the child? afaik the result of this diff would always be empty, but perhaps that might be different if there are operations which are manually merged?

banana (04/05/2024 20:06)

the operation log isn't really a general purpose tree, i think it would be ok to tailor the output to the assumption that the merges are ones created by jj merging op heads

Martin von Zweigbergk (04/05/2024 20:14)

What should jj op show with an operation with multiple parents be? Should it merge the parents and attempt to diff with the child? afaik the result of this diff would always be empty, but perhaps that might be different if there are operations which are manually merged?

I think so. That would be consistent with what we do for the commit log

I guess it's also possible for a user to manually merge operations using jj_lib, which is why it might be useful to diff with the merged operation parents.

Yeah, that's technically correct. I was just wondering how we can reduce the amount of the custom code. Maybe we can make RepoLoader::_resolve_op_heads() public in some form?

yuja · 2024-05-19T09:18:48Z

cli/src/commands/operation.rs

+    // Create a new transaction starting from `to_repo`.
+    let mut workspace_command =
+        command.for_loaded_repo(ui, command.load_workspace()?, to_repo.clone())?;
+    let mut tx = workspace_command.start_transaction();


It might be better to use low-level repo.start_transaction(). WorkspaceCommandHelper assumes that the loaded repo is synchronized with the working copy, and things might get weird if we committed the transaction by mistake.

yuja · 2024-05-19T09:20:02Z

cli/src/commands/operation.rs

+    let mut tx = workspace_command.start_transaction();
+    // Merge index from `from_repo` to `to_repo`, so commits in `from_repo` are
+    // accessible.
+    tx.mut_repo().merge_index(from_repo);


Perhaps, we can use the current (head) repo.index()? It should contain all historical commits.

OTOH, doing it the current way means it works even if --from or --to point to an operation that was never "published" (made an op head).

doing it the current way means it works even if --from or --to point to an operation that was never "published" (made an op head).

I don't know if such functionality is needed, but there might be (current_repo, from_repo, to_repo) tuple instead of (from_repo, to_repo). The current_repo provides a context (or visible heads + index) where repo-level templates will be evaluated.

I'm not sure if current_repo should be selected by --at-op or --to, but jj op show doesn't need to merge indexes. jj op diff will have to merge if visible heads are controlled by --to, not by --at-op.

bnjmnt4n · 2024-05-19T14:55:28Z

Thanks for taking a look @yuja. I'll try to work on adding some tests to make it easier to see what the expected output is like, perhaps further reviews can be put on hold if you'd like.

I'm also thinking that templates are a good idea, maybe something like ModifiedChange nodes and ModifiedBranch nodes if that makes sense? Although the commit summary is a good starting point, seeing things like hidden repeatedly on removed commits isn't particularly useful. I'm not sure if the branch status is useful as well since, it will also be shown in the modified branches section.

I'd also like to see if there's any desire to change the output displayed.

martinvonz · 2024-05-19T17:46:01Z

I'll try to add some testcases when I have the time. Here's the list of operations I think we should test (feel free to suggest more):

new

abandon

squash

rebase

branch create

git fetch

git push

As @joyously said, it's probably best to instead focus on what the code does and the different cases it handles. For example, there's some logic for displaying how refs moved, so there should be some tests demonstrating that, and ideally some including conflicted refs. It doesn't matter which commands you use to create the refs, so use whatever is easiest for that (for creating conflicted branches, it's probably easiest to use jj branch set --at-op=...).

op undo

op restore

I don't think these two need to be tested. They don't do much you can't test by making the same changes explicitly.

I'm not sure how to structure it though: I guess I should add a single test beginning from an empty repo for each of these operations?

SGTM.

ilyagr · 2024-05-22T00:03:39Z

cli/src/commands/log.rs

(If this makes your life easier): I would consider splitting this commit into a separate PR, finishing it up, and merging it, so that you don't have to worry about merge conflicts if somebody else edits the renamed files. At least, that's what I tend to do. (I'm assuming this is a good change on its own, @yuja can correct me if I'm wrong)

ilyagr · 2024-05-24T03:30:19Z

cli/src/commands/operation.rs

+pub struct OperationShowArgs {
+    /// Show repository changes in this operation, compared to its parent(s)
+    #[arg(default_value = "@")]
+    operation: String,


I've been using these (thank you!), but I always want to use --op instead of --operation. I think it'd be good to add an alias visible_alias="op" for --operation. (Also for op diff)

bnjmnt4n force-pushed the bnjmnt4n/push-uzynoulwtlvz branch 3 times, most recently from 7e11943 to eb59a88 Compare May 4, 2024 20:56

bnjmnt4n changed the title ~~cli: add jj operation diff command~~ cli: add jj operation show and jj operation diff commands May 4, 2024

bnjmnt4n commented May 4, 2024

View reviewed changes

lib/src/revset_graph.rs Outdated Show resolved Hide resolved

bnjmnt4n force-pushed the bnjmnt4n/push-uzynoulwtlvz branch from eb59a88 to c82584f Compare May 4, 2024 21:02

martinvonz reviewed May 4, 2024

View reviewed changes

lib/src/revset_graph.rs Show resolved Hide resolved

lib/src/revset_graph.rs Outdated Show resolved Hide resolved

lib/src/revset_graph.rs Outdated Show resolved Hide resolved

bnjmnt4n force-pushed the bnjmnt4n/push-uzynoulwtlvz branch 2 times, most recently from 7ae89e3 to c3343eb Compare May 6, 2024 11:24

bnjmnt4n marked this pull request as ready for review May 6, 2024 11:25

bnjmnt4n force-pushed the bnjmnt4n/push-uzynoulwtlvz branch from c3343eb to dbb9cec Compare May 15, 2024 01:44

bnjmnt4n added 3 commits May 19, 2024 02:28

revset_graph: rename to graph and make generic over graph node type

1f6c1a9

repo: add MutableRepo::merge_index

ae92af0

cli: add jj operation show and jj operation diff commands

33ee8dd

bnjmnt4n force-pushed the bnjmnt4n/push-uzynoulwtlvz branch from dbb9cec to 33ee8dd Compare May 18, 2024 18:29

yuja reviewed May 19, 2024

View reviewed changes

ilyagr reviewed May 22, 2024

View reviewed changes

ilyagr reviewed May 24, 2024

View reviewed changes

ilyagr mentioned this pull request May 24, 2024

Concurrency bug with import git refs #3747

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cli: add `jj operation show` and `jj operation diff` commands #3617

cli: add `jj operation show` and `jj operation diff` commands #3617

bnjmnt4n commented May 3, 2024 •

edited

bnjmnt4n commented May 6, 2024

bnjmnt4n commented May 6, 2024 •

edited

joyously commented May 6, 2024

martinvonz commented May 18, 2024

martinvonz commented May 18, 2024

bnjmnt4n commented May 18, 2024

yuja left a comment

yuja May 19, 2024

yuja May 19, 2024

yuja May 19, 2024

bnjmnt4n May 19, 2024

yuja May 20, 2024

yuja May 19, 2024

yuja May 19, 2024

martinvonz May 19, 2024

yuja May 20, 2024

bnjmnt4n commented May 19, 2024

martinvonz commented May 19, 2024

ilyagr May 22, 2024

ilyagr May 24, 2024

cli: add jj operation show and jj operation diff commands #3617

Are you sure you want to change the base?

cli: add jj operation show and jj operation diff commands #3617

Conversation

bnjmnt4n commented May 3, 2024 • edited

Checklist

bnjmnt4n commented May 6, 2024

bnjmnt4n commented May 6, 2024 • edited

joyously commented May 6, 2024

martinvonz commented May 18, 2024

martinvonz commented May 18, 2024

bnjmnt4n commented May 18, 2024

yuja left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bnjmnt4n commented May 19, 2024

martinvonz commented May 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cli: add `jj operation show` and `jj operation diff` commands #3617

cli: add `jj operation show` and `jj operation diff` commands #3617

bnjmnt4n commented May 3, 2024 •

edited

bnjmnt4n commented May 6, 2024 •

edited