Skip to content

Cannot override "ui.diff.tool" to use builtin diff in repo config #3327

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.

Sign up for GitHub

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

Jump to bottom
Open
scott2000 opened this issue Mar 19, 2024 · 8 comments
Open

Cannot override "ui.diff.tool" to use builtin diff in repo config #3327

scott2000 opened this issue Mar 19, 2024 · 8 comments
Assignees
@tomafro

Comments

@scott2000
Copy link
Contributor

Description

I want to use Difftastic for most of my repos, so I have ui.diff.tool = "difft" in my user config file, but for a specific repo I want to use the builtin "color-words" diff, since Difftastic doesn't give good diffs for it. However, there seems to be no way to override it to use the builtin word diff on a repo level.

Steps to Reproduce the Problem

  1. jj config set --user ui.diff.tool "difft"
  2. jj config set --repo ui.diff.tool ":builtin"
  3. Run jj diff with some changes

Expected Behavior

I expected it to use the builtin diff from ui.diff.format (e.g. color-words).

Actual Behavior

I get the following error since it actually tries to run it as a program:

Error: Failed to generate diff
Caused by:
1: Error executing ':builtin' (run with --debug to see the exact invocation)
2: No such file or directory (os error 2)

A similar error occurs when set to "null" or "".

Specifications

  • Platform: macOS 12.7.1
  • Version: jj 0.15.1
@martinvonz
Copy link
Member

Makes sense. An alternative would be to allow ui.diff.tool = ":color-words" or something like that. Then we can remove the separate ui.diff.format config.

@tomafro
Copy link
Contributor

tomafro commented Mar 19, 2024

So ui.diff.tool could be set to :summary|:types|:git|:color-words|:stat|<tool>, builtin formats starting with a :. Then deprecate ui.diff.format?

I think that works, though technically an external tool could start with a :. If this is the way you want to go I'd be happy to take a crack at this.

@yuja
Copy link
Contributor

yuja commented Mar 19, 2024

So ui.diff.tool could be set to :summary|:types|:git|:color-words|:stat|<tool>, builtin formats starting with a :. Then deprecate ui.diff.format?

That's the idea. The config key might also be renamed to ui.diff-generator/diff-formatter, but I don't have a better name. We already have ui.diff-editor and ui.merge-editor having the same structure.

I think that works, though technically an external tool could start with a :.

There's an escape syntax: [":<name>", "<arg>", ..].

@tomafro tomafro self-assigned this Mar 19, 2024
@martinvonz
Copy link
Member

Oh, does this mean that one will also be able to do jj diff --tool :stat as an alternative to jj diff --stat? I suppose that should ideally work. --tool sounds a bit strange in that context. Is --format better, or does that instead sound too odd if it's an external tool? Whatever we call the flag, we should probably keep the current flags (--git, -s/--summary, etc.) as shorthands, or what do people think? We can possibly drop the shorthands from jj log/obslog.

@yuja
Copy link
Contributor

yuja commented Mar 20, 2024

one will also be able to do jj diff --tool :stat as an alternative to jj diff --stat?

Yes, but I think it can be implemented separately. --stat --summary is currently rejected at clap layer, but --tool :stat --summary wouldn't. We might need to clean up short/long format thingy.

@tomafro tomafro mentioned this issue Mar 25, 2024
Draft
4 tasks
tomafro added a commit to tomafro/jj that referenced this issue Mar 26, 2024
@tomafro
cli: introduce 'ui.diff-viewer' setting
f1ef217 
'ui.diff-viewer' combines 'ui.diff.tool' and 'ui.diff.format' into a single 
setting, which in turn allows repository level configuration to override 
global settings (see jj-vcs#3327).
yuja added a commit to yuja/jj that referenced this issue Aug 18, 2024
@yuja
diff: add option to display complex color-words diffs without inlining
c95e869 
In this patch, I use the number of adds<->removes alternation as a threshold,
which approximates the visual complexity of diff hunks. I don't think user can
choose the threshold intuitively, but we need a config knob to try out some.
I set `max-inline-alternation = 3` locally. 0 and 1 mean "disable inlining"
and "inline adds-only/removes-only lines" respectively.

I've added "diff.<format>" config namespace assuming "ui.diff" will be
reorganized as "ui.diff-formatter" or something. jj-vcs#3327

Some other metrics I've tried:
```
// Per-line alternation. This also works well, but can't measure complexity of
// changes across lines.
fn count_max_diff_alternation_per_line(diff_lines: &[DiffLine]) -> usize {
    diff_lines
        .iter()
        .map(|line| {
            let sides = line.hunks.iter().map(|&(side, _)| side);
            sides
                .filter(|&side| side != DiffLineHunkSide::Both)
                .dedup() // omit e.g. left->both->left
                .count()
        })
        .max()
        .unwrap_or(0)
}

// Per-line occupancy of changes. Large diffs don't always look complex.
fn max_diff_token_ratio_per_line(diff_lines: &[DiffLine]) -> f32 {
    diff_lines
        .iter()
        .filter_map(|line| {
            let [both_len, left_len, right_len] =
                line.hunks.iter().fold([0, 0, 0], |mut acc, (side, data)| {
                    let index = match side {
                        DiffLineHunkSide::Both => 0,
                        DiffLineHunkSide::Left => 1,
                        DiffLineHunkSide::Right => 2,
                    };
                    acc[index] += data.len();
                    acc
                });
            // left/right-only change is readable
            (left_len != 0 && right_len != 0).then(|| {
                let diff_len = left_len + right_len;
                let total_len = both_len + left_len + right_len;
                (diff_len as f32) / (total_len as f32)
            })
        })
        .reduce(f32::max)
        .unwrap_or(0.0)
}

// Total occupancy of changes. Large diffs don't always look complex.
fn total_change_ratio(diff_lines: &[DiffLine]) -> f32 {
    let (diff_len, total_len) = diff_lines
        .iter()
        .flat_map(|line| &line.hunks)
        .fold((0, 0), |(diff_len, total_len), (side, data)| {
            let l = data.len();
            match side {
                DiffLineHunkSide::Both => (diff_len, total_len + l),
                DiffLineHunkSide::Left => (diff_len + l, total_len + l),
                DiffLineHunkSide::Right => (diff_len + l, total_len + l),
            }
        });
    (diff_len as f32) / (total_len as f32)
}
```
yuja added a commit to yuja/jj that referenced this issue Aug 19, 2024
@yuja
diff: add option to display complex color-words diffs without inlining
f6675b4 
In this patch, I use the number of adds<->removes alternation as a threshold,
which approximates the visual complexity of diff hunks. I don't think user can
choose the threshold intuitively, but we need a config knob to try out some.
I set `max-inline-alternation = 3` locally. 0 and 1 mean "disable inlining"
and "inline adds-only/removes-only lines" respectively.

I've added "diff.<format>" config namespace assuming "ui.diff" will be
reorganized as "ui.diff-formatter" or something. jj-vcs#3327

Some other metrics I've tried:
```
// Per-line alternation. This also works well, but can't measure complexity of
// changes across lines.
fn count_max_diff_alternation_per_line(diff_lines: &[DiffLine]) -> usize {
    diff_lines
        .iter()
        .map(|line| {
            let sides = line.hunks.iter().map(|&(side, _)| side);
            sides
                .filter(|&side| side != DiffLineHunkSide::Both)
                .dedup() // omit e.g. left->both->left
                .count()
        })
        .max()
        .unwrap_or(0)
}

// Per-line occupancy of changes. Large diffs don't always look complex.
fn max_diff_token_ratio_per_line(diff_lines: &[DiffLine]) -> f32 {
    diff_lines
        .iter()
        .filter_map(|line| {
            let [both_len, left_len, right_len] =
                line.hunks.iter().fold([0, 0, 0], |mut acc, (side, data)| {
                    let index = match side {
                        DiffLineHunkSide::Both => 0,
                        DiffLineHunkSide::Left => 1,
                        DiffLineHunkSide::Right => 2,
                    };
                    acc[index] += data.len();
                    acc
                });
            // left/right-only change is readable
            (left_len != 0 && right_len != 0).then(|| {
                let diff_len = left_len + right_len;
                let total_len = both_len + left_len + right_len;
                (diff_len as f32) / (total_len as f32)
            })
        })
        .reduce(f32::max)
        .unwrap_or(0.0)
}

// Total occupancy of changes. Large diffs don't always look complex.
fn total_change_ratio(diff_lines: &[DiffLine]) -> f32 {
    let (diff_len, total_len) = diff_lines
        .iter()
        .flat_map(|line| &line.hunks)
        .fold((0, 0), |(diff_len, total_len), (side, data)| {
            let l = data.len();
            match side {
                DiffLineHunkSide::Both => (diff_len, total_len + l),
                DiffLineHunkSide::Left => (diff_len + l, total_len + l),
                DiffLineHunkSide::Right => (diff_len + l, total_len + l),
            }
        });
    (diff_len as f32) / (total_len as f32)
}
```
yuja added a commit to yuja/jj that referenced this issue Aug 19, 2024
@yuja
diff: add option to display complex color-words diffs without inlining
9a9cefe 
In this patch, I use the number of adds<->removes alternation as a threshold,
which approximates the visual complexity of diff hunks. I don't think user can
choose the threshold intuitively, but we need a config knob to try out some.
I set `max-inline-alternation = 3` locally. 0 and 1 mean "disable inlining"
and "inline adds-only/removes-only lines" respectively.

I've added "diff.<format>" config namespace assuming "ui.diff" will be
reorganized as "ui.diff-formatter" or something. jj-vcs#3327

Some other metrics I've tried:
```
// Per-line alternation. This also works well, but can't measure complexity of
// changes across lines.
fn count_max_diff_alternation_per_line(diff_lines: &[DiffLine]) -> usize {
    diff_lines
        .iter()
        .map(|line| {
            let sides = line.hunks.iter().map(|&(side, _)| side);
            sides
                .filter(|&side| side != DiffLineHunkSide::Both)
                .dedup() // omit e.g. left->both->left
                .count()
        })
        .max()
        .unwrap_or(0)
}

// Per-line occupancy of changes. Large diffs don't always look complex.
fn max_diff_token_ratio_per_line(diff_lines: &[DiffLine]) -> f32 {
    diff_lines
        .iter()
        .filter_map(|line| {
            let [both_len, left_len, right_len] =
                line.hunks.iter().fold([0, 0, 0], |mut acc, (side, data)| {
                    let index = match side {
                        DiffLineHunkSide::Both => 0,
                        DiffLineHunkSide::Left => 1,
                        DiffLineHunkSide::Right => 2,
                    };
                    acc[index] += data.len();
                    acc
                });
            // left/right-only change is readable
            (left_len != 0 && right_len != 0).then(|| {
                let diff_len = left_len + right_len;
                let total_len = both_len + left_len + right_len;
                (diff_len as f32) / (total_len as f32)
            })
        })
        .reduce(f32::max)
        .unwrap_or(0.0)
}

// Total occupancy of changes. Large diffs don't always look complex.
fn total_change_ratio(diff_lines: &[DiffLine]) -> f32 {
    let (diff_len, total_len) = diff_lines
        .iter()
        .flat_map(|line| &line.hunks)
        .fold((0, 0), |(diff_len, total_len), (side, data)| {
            let l = data.len();
            match side {
                DiffLineHunkSide::Both => (diff_len, total_len + l),
                DiffLineHunkSide::Left => (diff_len + l, total_len + l),
                DiffLineHunkSide::Right => (diff_len + l, total_len + l),
            }
        });
    (diff_len as f32) / (total_len as f32)
}
```
yuja added a commit to yuja/jj that referenced this issue Aug 21, 2024
@yuja
diff: add option to display complex color-words diffs without inlining
91ac8e1 
In this patch, I use the number of adds<->removes alternation as a threshold,
which approximates the visual complexity of diff hunks. I don't think user can
choose the threshold intuitively, but we need a config knob to try out some.
I set `max-inline-alternation = 3` locally. 0 and 1 mean "disable inlining"
and "inline adds-only/removes-only lines" respectively.

I've added "diff.<format>" config namespace assuming "ui.diff" will be
reorganized as "ui.diff-formatter" or something. jj-vcs#3327

Some other metrics I've tried:
```
// Per-line alternation. This also works well, but can't measure complexity of
// changes across lines.
fn count_max_diff_alternation_per_line(diff_lines: &[DiffLine]) -> usize {
    diff_lines
        .iter()
        .map(|line| {
            let sides = line.hunks.iter().map(|&(side, _)| side);
            sides
                .filter(|&side| side != DiffLineHunkSide::Both)
                .dedup() // omit e.g. left->both->left
                .count()
        })
        .max()
        .unwrap_or(0)
}

// Per-line occupancy of changes. Large diffs don't always look complex.
fn max_diff_token_ratio_per_line(diff_lines: &[DiffLine]) -> f32 {
    diff_lines
        .iter()
        .filter_map(|line| {
            let [both_len, left_len, right_len] =
                line.hunks.iter().fold([0, 0, 0], |mut acc, (side, data)| {
                    let index = match side {
                        DiffLineHunkSide::Both => 0,
                        DiffLineHunkSide::Left => 1,
                        DiffLineHunkSide::Right => 2,
                    };
                    acc[index] += data.len();
                    acc
                });
            // left/right-only change is readable
            (left_len != 0 && right_len != 0).then(|| {
                let diff_len = left_len + right_len;
                let total_len = both_len + left_len + right_len;
                (diff_len as f32) / (total_len as f32)
            })
        })
        .reduce(f32::max)
        .unwrap_or(0.0)
}

// Total occupancy of changes. Large diffs don't always look complex.
fn total_change_ratio(diff_lines: &[DiffLine]) -> f32 {
    let (diff_len, total_len) = diff_lines
        .iter()
        .flat_map(|line| &line.hunks)
        .fold((0, 0), |(diff_len, total_len), (side, data)| {
            let l = data.len();
            match side {
                DiffLineHunkSide::Both => (diff_len, total_len + l),
                DiffLineHunkSide::Left => (diff_len + l, total_len + l),
                DiffLineHunkSide::Right => (diff_len + l, total_len + l),
            }
        });
    (diff_len as f32) / (total_len as f32)
}
```
yuja added a commit to yuja/jj that referenced this issue Aug 21, 2024
@yuja
diff: add option to display complex color-words diffs without inlining
e02e55b 
In this patch, I use the number of adds<->removes alternation as a threshold,
which approximates the visual complexity of diff hunks. I don't think user can
choose the threshold intuitively, but we need a config knob to try out some.
I set `max-inline-alternation = 3` locally. 0 and 1 mean "disable inlining"
and "inline adds-only/removes-only lines" respectively.

I've added "diff.<format>" config namespace assuming "ui.diff" will be
reorganized as "ui.diff-formatter" or something. jj-vcs#3327

Some other metrics I've tried:
```
// Per-line alternation. This also works well, but can't measure complexity of
// changes across lines.
fn count_max_diff_alternation_per_line(diff_lines: &[DiffLine]) -> usize {
    diff_lines
        .iter()
        .map(|line| {
            let sides = line.hunks.iter().map(|&(side, _)| side);
            sides
                .filter(|&side| side != DiffLineHunkSide::Both)
                .dedup() // omit e.g. left->both->left
                .count()
        })
        .max()
        .unwrap_or(0)
}

// Per-line occupancy of changes. Large diffs don't always look complex.
fn max_diff_token_ratio_per_line(diff_lines: &[DiffLine]) -> f32 {
    diff_lines
        .iter()
        .filter_map(|line| {
            let [both_len, left_len, right_len] =
                line.hunks.iter().fold([0, 0, 0], |mut acc, (side, data)| {
                    let index = match side {
                        DiffLineHunkSide::Both => 0,
                        DiffLineHunkSide::Left => 1,
                        DiffLineHunkSide::Right => 2,
                    };
                    acc[index] += data.len();
                    acc
                });
            // left/right-only change is readable
            (left_len != 0 && right_len != 0).then(|| {
                let diff_len = left_len + right_len;
                let total_len = both_len + left_len + right_len;
                (diff_len as f32) / (total_len as f32)
            })
        })
        .reduce(f32::max)
        .unwrap_or(0.0)
}

// Total occupancy of changes. Large diffs don't always look complex.
fn total_change_ratio(diff_lines: &[DiffLine]) -> f32 {
    let (diff_len, total_len) = diff_lines
        .iter()
        .flat_map(|line| &line.hunks)
        .fold((0, 0), |(diff_len, total_len), (side, data)| {
            let l = data.len();
            match side {
                DiffLineHunkSide::Both => (diff_len, total_len + l),
                DiffLineHunkSide::Left => (diff_len + l, total_len + l),
                DiffLineHunkSide::Right => (diff_len + l, total_len + l),
            }
        });
    (diff_len as f32) / (total_len as f32)
}
```
yuja added a commit that referenced this issue Aug 21, 2024
@yuja
diff: add option to display complex color-words diffs without inlining
a83dadd 
In this patch, I use the number of adds<->removes alternation as a threshold,
which approximates the visual complexity of diff hunks. I don't think user can
choose the threshold intuitively, but we need a config knob to try out some.
I set `max-inline-alternation = 3` locally. 0 and 1 mean "disable inlining"
and "inline adds-only/removes-only lines" respectively.

I've added "diff.<format>" config namespace assuming "ui.diff" will be
reorganized as "ui.diff-formatter" or something. #3327

Some other metrics I've tried:
```
// Per-line alternation. This also works well, but can't measure complexity of
// changes across lines.
fn count_max_diff_alternation_per_line(diff_lines: &[DiffLine]) -> usize {
    diff_lines
        .iter()
        .map(|line| {
            let sides = line.hunks.iter().map(|&(side, _)| side);
            sides
                .filter(|&side| side != DiffLineHunkSide::Both)
                .dedup() // omit e.g. left->both->left
                .count()
        })
        .max()
        .unwrap_or(0)
}

// Per-line occupancy of changes. Large diffs don't always look complex.
fn max_diff_token_ratio_per_line(diff_lines: &[DiffLine]) -> f32 {
    diff_lines
        .iter()
        .filter_map(|line| {
            let [both_len, left_len, right_len] =
                line.hunks.iter().fold([0, 0, 0], |mut acc, (side, data)| {
                    let index = match side {
                        DiffLineHunkSide::Both => 0,
                        DiffLineHunkSide::Left => 1,
                        DiffLineHunkSide::Right => 2,
                    };
                    acc[index] += data.len();
                    acc
                });
            // left/right-only change is readable
            (left_len != 0 && right_len != 0).then(|| {
                let diff_len = left_len + right_len;
                let total_len = both_len + left_len + right_len;
                (diff_len as f32) / (total_len as f32)
            })
        })
        .reduce(f32::max)
        .unwrap_or(0.0)
}

// Total occupancy of changes. Large diffs don't always look complex.
fn total_change_ratio(diff_lines: &[DiffLine]) -> f32 {
    let (diff_len, total_len) = diff_lines
        .iter()
        .flat_map(|line| &line.hunks)
        .fold((0, 0), |(diff_len, total_len), (side, data)| {
            let l = data.len();
            match side {
                DiffLineHunkSide::Both => (diff_len, total_len + l),
                DiffLineHunkSide::Left => (diff_len + l, total_len + l),
                DiffLineHunkSide::Right => (diff_len + l, total_len + l),
            }
        });
    (diff_len as f32) / (total_len as f32)
}
```
@jjt
Copy link

jjt commented Mar 18, 2025 •
edited
Loading

As of v0.27.0, running jj diff --color-words seems to do the trick here. It's somewhat confusing to me that --color-words would override ui.diff.tool, but the result is that I get an explicit command to use the builtin color-words diff.

Relevant config:

ui.diff.tool = "difft"
merge-tools.difft.diff-args = ["--color=always", "$left", "$right"]

@scott2000
Copy link
Contributor Author

Yeah, this issue was more for configuration, since I wanted the default to be color-words for certain repos even though my overall default was difft. However I've actually stopped using difftastic since jj's git style is actually really nice in most cases, as long as you change the formatting to use background highlighting instead of underlines for the changed sections.

@martinvonz
Copy link
Member

FYI, the difft config is shipped by default as of v0.27.0 (#5660), so you can remove that from your personal config now.

@yuja yuja mentioned this issue Apr 6, 2025
Open
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@tomafro tomafro

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@tomafro @yuja @jjt @martinvonz @scott2000