Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add jj diffs --stat option #2106

Merged
merged 1 commit into from
Aug 20, 2023
Merged

Add jj diffs --stat option #2106

merged 1 commit into from
Aug 20, 2023

Conversation

ob
Copy link
Contributor

@ob ob commented Aug 18, 2023

This is a first cut at #2066. It needs a better way of getting the terminal width (hardcoded for now to 120) and maybe some tests... I did a few visual comparisons to git diff and it looks like it works...

jj ❯ COLUMNS=120 git diff --stat 2102de94 e414f3b7
 CHANGELOG.md                       |   4 +++
 cli/src/cli_util.rs                |  34 ++++++-----------------
 cli/src/commands/mod.rs            |  51 +++++++++++++++++-----------------
 cli/src/config-schema.json         |   5 ++++
 cli/src/config/colors.toml         |   1 +
 cli/src/diff_util.rs               |  20 ++++++++++----
 cli/src/merge_tools.rs             |  55 ++++++++++++++++++++-----------------
 cli/tests/test_describe_command.rs |  89 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 cli/tests/test_diff_command.rs     |  23 ++++++++++++++++
 docs/config.md                     |   4 +--
 docs/operation-log.md              |   2 +-
 lib/src/conflicts.rs               |  17 ++++--------
 lib/src/default_index_store.rs     | 115 +++++++++++++++++++++++------------------------------------------------------
 lib/src/gitignore.rs               |   7 ++---
 lib/src/local_backend.rs           |   4 +--
 lib/src/repo.rs                    |   7 +++--
 lib/src/working_copy.rs            |  76 ++++++++++++++++++++++++---------------------------
 lib/src/workspace.rs               |   7 ++---
 lib/tests/test_conflicts.rs        |   8 +-----
 19 files changed, 288 insertions(+), 241 deletions(-)
jj ❯ cargo build && ./target/debug/jj diff --stat --from 2102de94 --to e414f3b7
    Finished dev [unoptimized + debuginfo] target(s) in 0.15s
 CHANGELOG.md                       |    4 +++
 cli/src/cli_util.rs                |   34 +++++------------------
 cli/src/commands/mod.rs            |   51 +++++++++++++++++------------------
 cli/src/config/colors.toml         |    1 +
 cli/src/config-schema.json         |    5 +++
 cli/src/diff_util.rs               |   20 ++++++++++----
 cli/src/merge_tools.rs             |   55 +++++++++++++++++++++-----------------
 cli/tests/test_describe_command.rs |   89 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 cli/tests/test_diff_command.rs     |   23 ++++++++++++++++
 docs/config.md                     |    4 +-
 docs/operation-log.md              |    2 +-
 lib/src/conflicts.rs               |   17 ++++--------
 lib/src/default_index_store.rs     |  115 +++++++++++++++++++++++--------------------------------------------------------
 lib/src/gitignore.rs               |    7 +---
 lib/src/local_backend.rs           |    4 +--
 lib/src/repo.rs                    |    7 +++--
 lib/src/working_copy.rs            |   76 +++++++++++++++++++++++++---------------------------
 lib/src/workspace.rs               |    7 ++---
 lib/tests/test_conflicts.rs        |    8 +-----
 19 files changed, 288 insertions(+), 241 deletions(-)

Checklist

If applicable:

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

Copy link
Member

@martinvonz martinvonz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! Thank you!

and maybe some tests..

Yes, please :)

Also, please squash the second commit into the first and address comments by amending the commit. See https://github.com/martinvonz/jj/blob/main/docs/contributing.md#code-reviews.

cli/src/diff_util.rs Outdated Show resolved Hide resolved
cli/src/diff_util.rs Outdated Show resolved Hide resolved
cli/src/diff_util.rs Outdated Show resolved Hide resolved
cli/src/diff_util.rs Outdated Show resolved Hide resolved
cli/src/diff_util.rs Outdated Show resolved Hide resolved
cli/src/diff_util.rs Outdated Show resolved Hide resolved
cli/src/diff_util.rs Outdated Show resolved Hide resolved
cli/src/diff_util.rs Outdated Show resolved Hide resolved
cli/src/diff_util.rs Outdated Show resolved Hide resolved
cli/src/diff_util.rs Outdated Show resolved Hide resolved
cli/src/diff_util.rs Outdated Show resolved Hide resolved
cli/src/diff_util.rs Outdated Show resolved Hide resolved
@martinvonz
Copy link
Member

Thanks for fixing. Will there be a test or two as well? :) They would probably belong in https://github.com/martinvonz/jj/blob/main/cli/tests/test_diff_command.rs

@ob
Copy link
Contributor Author

ob commented Aug 20, 2023

Ah, thanks for the pointer to where the tests are. I'll figure out a way to add some tests...

@ob
Copy link
Contributor Author

ob commented Aug 20, 2023

Ok, added some tests (and of course found a new bugs)... if there are other corner cases you can think of let me know and I'll add more tests...

Copy link
Member

@martinvonz martinvonz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Thanks again. Feel free to merge once you've resolved the remaining comments.

cli/src/diff_util.rs Outdated Show resolved Hide resolved
cli/src/diff_util.rs Outdated Show resolved Hide resolved
cli/src/diff_util.rs Outdated Show resolved Hide resolved
cli/tests/test_diff_command.rs Outdated Show resolved Hide resolved
@ob ob force-pushed the ob/diffs-stat branch 2 times, most recently from 49c04c3 to 521f0cb Compare August 20, 2023 06:33
@ob ob enabled auto-merge (rebase) August 20, 2023 06:41
@ob ob merged commit 5bd726f into jj-vcs:main Aug 20, 2023
@martinvonz martinvonz mentioned this pull request Aug 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants