Panic in built-in diff editor when replacing directory with file

[arxanas]

I'm not sure if I should make a new issue, but I got a consistent unreachable panic corner case type of thing that seems related to this (there is a possibility this is precisely what you guys are trying to fix :sweat_smile:)

Basically if you replace a folder with a file (in my case it was a symlink) and then try to split you get a panic.

minimal repro:

λ jj git init test
Initialized repo in "test"
λ cd test
λ mkdir folder
λ touch folder/.keep
λ jj ci -m 'added folder'
Working copy now at: (x) c76fc22 (empty) (no description set)
Parent commit      : (u) 9092cba added folder
λ rm -r folder
λ touch folder
λ jj split
Hint: Using default editor ':builtin'; run `jj config set --user ui.diff-editor :builtin` to disabl
e this message.
thread 'main' panicked at cli/src/merge_tools/builtin.rs:205:13:
internal error: entered unreachable code: list of changed files included a tree: TreeId("29a422c192
51aeaeb907175e9b3219a9bed6c616")
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

Originally posted by @necauqua in https://github.com/jj-vcs/jj/issues/3702#issuecomment-2561200255

---

On this topic, I've successfully used property-based testing for incremental file changes in a similar context. It would be good to try it here to fuzz for panics, at least.

[jgreitemann]

As of #6411, this no longer produces a panic. However, the behavior is still faulty: Following the above repro and not selecting any changes in the diff editor produces a split-off commit with an empty tree (containing neither folder/.keep, nor the file folder). I've already written a test which produces both the panic (when branched off of v0.28.2) and the empty tree (when based on the current main). I'm working on a PR for a possible fix, too.

Is there still any interest in approaching this with property-based testing to fuzz for additional edge cases? I'd fancy giving that a try. That would require adding proptest as a dev dependency. I've used proptest before, but never in anger.

[martinvonz]

Thanks! I don't have any object to property-based testing in general. Which properties are you thinking of testing here? Things like "for any two trees, selecting everything in the diff editor results in the right tree" and "for any two trees and any selection in the diff editor, some new tree is created (no panic)"? SGTM

[jgreitemann]

Exactly right. The existing test_edit_diff_builtin* tests already merely check that the left/right tree is recovered when selecting no/all changes in the diff. Fuzzing for this same property would just generate successively more complex diffs.

Another property to check might be that you can split the diff randomly into two parts and then apply these first successively to go from left to right. That would also include the case that any subset of the diff can be applied without panicking, as you mentioned.

[arxanas]

I'm a big fan of using property-based testing for this purpose. Feel free to ping me on Discord for additional chat/discussion.

• In the system I mentioned in my original comment:
  • I originally generated two random directory structures, but it was pretty bad at exploring "interesting" transitions, such as converting a file into a directory, or moving an entire directory from one location to another.
  • This suggests preferring monadic (prop_flat_map) over applicative generation (prop_recursive), because generation wants to "look back" at the thus-far-generated test case. This is also relevant for generating symlinks that point to extant files, particular those outside of their containing directory.
  • I switched to generating a sequence of filesystem operations instead, including higher-level primitives like "move this file/directory to this other (extant) directory", which was a lot more useful.
  • I think proptest has a state machine testing system, which might also be relevant for this exact use-case, but I haven't tried it out personally. I'd recommend at least looking into it, since it sounds like the right conceptual approach, now that I have the benefit of hindsight.
  • For testing the incremental part, IIRC I would generate an initial sequence of operations and a subsequent one starting from that point, and ensure that the incremental computation result matched the result of applying the second operation sequence.
    • You could conceivably just generate one sequence of operations and split it somewhere (but maybe that would have worse shrinking behavior?)
    • Unfortunately, I didn't get Hypothesis (for Python) to minimize the size of the difference between the start and end values, so shrinking was sometimes suboptimal, but not by much.
    • In particular, it can sometimes be more intuitive to examine a test case where the first working copy state actually has more entries, when it's closer to the ultimate working copy state. I would occasionally need to investigate whether the larger difference was meaningful for the actual issue at hand.
    • Come to think of it, the state machine tooling might actually help in this case? It would probably try to minimize the number of transitions from a good to bad state?
• It might be nice to generate actual working copy states as part of this test setup, not just in-memory objects, to enable future tests.
  • There was another bug somewhere about failing to update as expected past a commit which changed the casing of a directory. It would be nice to reuse the test case generation for that case as well.
  • You could also imagine generating/updating .gitignore and sparse profiles, in order to suss out bugs there. Probably submodules at some point as well.