cli: add jj operation show and jj operation diff commands

[bnjmnt4n]

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

[bnjmnt4n]

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]

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]

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]

@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]

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

[bnjmnt4n]

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]

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.

[bnjmnt4n]

Hmm, I'm depending on WorkspaceCommandTransaction for write_commit_summary though. Would it be possible to reuse the existing code in some way?

[yuja]

I don't remember exactly, but there would be a context repo/view where templates should be evaluated? I think WorkspaceCommandHelper can be instantiated against this repo/view, and template can be obtained from there.

[yuja]

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

[martinvonz]

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).

[yuja]

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]

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]

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]

(Feel free to just leave a TODO instead of implementing this if it's easier to postpone these)

This looked slightly confusing when I used it. I would format it a little differently: (Update: This is for jj op diff --operation, it's fine for --from/--to. After thinking about this a bit more, perhaps jj op diff --operation could just be replaced jj op show instead. jj op diff could then require --from or --to)

"Changes in operation abcd: squash commits
Parent operation xyzw: ..."

Also, if you have the energy for it, colors would be nice here. I guess the description should just get op_log description label, and the id should bet op_log id according to

jj/cli/src/config/colors.toml

Line 86 in 4cca845

"op_log id" = "blue"