diff: fix inconsistent handling of "short" format arguments

All "short" formats should be able to be combined with -p/--patch. It was also
weird that "short" diff stats followed "long" color-words/git diffs.

Fixes #5986

CHANGELOG.md

@@ -53,6 +53,8 @@ to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
 
 ### Fixed bugs
 
+* `jj log -p --stat` now shows diff stats as well as the default color-words/git
+  diff output. [#5986](https://github.com/jj-vcs/jj/issues/5986)
 
 ## [0.27.0] - 2025-03-05
 

cli/src/diff_util.rs

@@ -143,6 +143,18 @@ pub enum DiffFormat {
     Tool(Box<ExternalMergeTool>),
 }
 
+impl DiffFormat {
+    fn is_short(&self) -> bool {
+        match self {
+            DiffFormat::Summary
+            | DiffFormat::Stat(_)
+            | DiffFormat::Types
+            | DiffFormat::NameOnly => true,
+            DiffFormat::Git(_) | DiffFormat::ColorWords(_) | DiffFormat::Tool(_) => false,
+        }
+    }
+}
+
 /// Returns a list of requested diff formats, which will never be empty.
 pub fn diff_formats_for(
     settings: &UserSettings,
@@ -164,8 +176,10 @@ pub fn diff_formats_for_log(
     patch: bool,
 ) -> Result<Vec<DiffFormat>, ConfigGetError> {
     let mut formats = diff_formats_from_args(settings, args)?;
-    // --patch implies default if no format other than --summary is specified
-    if patch && matches!(formats.as_slice(), [] | [DiffFormat::Summary]) {
+    // --patch implies default if no "long" format is specified
+    if patch && formats.iter().all(DiffFormat::is_short) {
+        // TODO: maybe better to error out if the configured default isn't a
+        // "long" format?
         formats.push(default_diff_format(settings, args)?);
         formats.dedup();
     }
@@ -177,15 +191,22 @@ fn diff_formats_from_args(
     args: &DiffFormatArgs,
 ) -> Result<Vec<DiffFormat>, ConfigGetError> {
     let mut formats = Vec::new();
+    // "short" format first:
     if args.summary {
         formats.push(DiffFormat::Summary);
     }
+    if args.stat {
+        let mut options = DiffStatOptions::default();
+        options.merge_args(args);
+        formats.push(DiffFormat::Stat(Box::new(options)));
+    }
     if args.types {
         formats.push(DiffFormat::Types);
     }
     if args.name_only {
         formats.push(DiffFormat::NameOnly);
     }
+    // "long" format follows:
     if args.git {
         let mut options = UnifiedDiffOptions::from_settings(settings)?;
         options.merge_args(args);
@@ -196,11 +217,6 @@ fn diff_formats_from_args(
         options.merge_args(args);
         formats.push(DiffFormat::ColorWords(Box::new(options)));
     }
-    if args.stat {
-        let mut options = DiffStatOptions::default();
-        options.merge_args(args);
-        formats.push(DiffFormat::Stat(Box::new(options)));
-    }
     if let Some(name) = &args.tool {
         let tool = merge_tools::get_external_tool_config(settings, name)?
             .unwrap_or_else(|| ExternalMergeTool::with_program(name));
@@ -232,6 +248,11 @@ fn default_diff_format(
     };
     match name.as_ref() {
         "summary" => Ok(DiffFormat::Summary),
+        "stat" => {
+            let mut options = DiffStatOptions::default();
+            options.merge_args(args);
+            Ok(DiffFormat::Stat(Box::new(options)))
+        }
         "types" => Ok(DiffFormat::Types),
         "name-only" => Ok(DiffFormat::NameOnly),
         "git" => {
@@ -244,11 +265,6 @@ fn default_diff_format(
             options.merge_args(args);
             Ok(DiffFormat::ColorWords(Box::new(options)))
         }
-        "stat" => {
-            let mut options = DiffStatOptions::default();
-            options.merge_args(args);
-            Ok(DiffFormat::Stat(Box::new(options)))
-        }
         _ => Err(ConfigGetError::Type {
             name: "ui.diff.format".to_owned(),
             error: format!("Invalid diff format: {name}").into(),

cli/tests/test_log_command.rs

@@ -150,6 +150,53 @@ fn test_log_with_or_without_diff() {
     [EOF]
     ");
 
+    // `-p` for default diff output, `--stat` for diff-stat
+    let output = test_env.run_jj_in(&repo_path, ["log", "-T", "description", "-p", "--stat"]);
+    insta::assert_snapshot!(output, @r"
+    @  a new commit
+    │  file1 | 1 +
+    │  1 file changed, 1 insertion(+), 0 deletions(-)
+    │  Modified regular file file1:
+    │     1    1: foo
+    │          2: bar
+    ○  add a file
+    │  file1 | 1 +
+    │  1 file changed, 1 insertion(+), 0 deletions(-)
+    │  Added regular file file1:
+    │          1: foo
+    ◆
+       0 files changed, 0 insertions(+), 0 deletions(-)
+    [EOF]
+    ");
+
+    // `--stat` is short format, which should be printed first
+    let output = test_env.run_jj_in(&repo_path, ["log", "-T", "description", "--git", "--stat"]);
+    insta::assert_snapshot!(output, @r"
+    @  a new commit
+    │  file1 | 1 +
+    │  1 file changed, 1 insertion(+), 0 deletions(-)
+    │  diff --git a/file1 b/file1
+    │  index 257cc5642c..3bd1f0e297 100644
+    │  --- a/file1
+    │  +++ b/file1
+    │  @@ -1,1 +1,2 @@
+    │   foo
+    │  +bar
+    ○  add a file
+    │  file1 | 1 +
+    │  1 file changed, 1 insertion(+), 0 deletions(-)
+    │  diff --git a/file1 b/file1
+    │  new file mode 100644
+    │  index 0000000000..257cc5642c
+    │  --- /dev/null
+    │  +++ b/file1
+    │  @@ -0,0 +1,1 @@
+    │  +foo
+    ◆
+       0 files changed, 0 insertions(+), 0 deletions(-)
+    [EOF]
+    ");
+
     // `-p` enables default "summary" output, so `-s` is noop
     let output = test_env.run_jj_in(
         &repo_path,