file deletion ignored by jj split (when using the built-in diff editor)

[Zoybean]

Description

jj split and jj squash -i both appear to ignore selected file deletions. This appears to be limited to the default editor (could not reproduce with --tool meld). They are shown in the diff editor but do not end up in the chosen commit.

Steps to Reproduce the Problem

1. mv foo bar
2. jj split
3. select all changes
4. jj show @-
5. jj show @

Expected Behavior

the first commit contains a file addition (bar) and a file deletion (foo)
the second commit is empty

Actual Behavior

the first commit contains a file addition (bar)
the second commit contains a file deletion (foo)

Specifications

• Platform: Windows 10
• Version: jj 0.17.1-c34f35fffe1b8e5932ba27ac4da8ba5a97d417a5

[hroi]

I'm able to reproduce this with :builtin, but not when running scm-diff-editor as an external tool:

# config.toml
# Install scm-diff-editor binary:
# cargo install --git https://github.com/arxanas/scm-record.git --features scm-diff-editor

[ui]
# diff-editor = ":builtin"
diff-editor = "scm-diff-editor"

[merge-tools.scm-diff-editor]
program = "/path/to/scm-diff-editor"
edit-args = ["--dir-diff", "$left", "$right"]

[martinvonz]

That's good to know. That means the problem is probably somewhere in cli/src/merge_tools/builtin.rs, in case anyone has time to take a look.

[Zoybean]

the code at apply_diff_builtin's first match arm seems suspect to me, but I haven't got enough familiarity with the codebase to know for sure one way or the other.

[30350n]

Possibly related to #3526.

I missunderstood that one and am actually experiencing the issue describe in here. I could imagine that they are related though.

[arxanas]

I think the most straightforward near-term solution would be as discussed here:

What the caller was "supposed" to do is always have a FileMode section for newly-created (or newly-deleted) files. The user could choose to include the file mode change in their selection or not.

• This would also handle the current issue of not being able to select empty files because they have no items inside of them; instead, there would be a FileMode section.
• The main problem is that you want logical implication in certain cases:
  • If you select some lines to add without selecting the "newly-added" file mode, then you would somehow be requesting to add some of the file contents without adding the file itself.
  • If you select the "newly-deleted" file mode without also selecting all the changes that would delete the lines of that file, then you would somehow be requesting that the file be deleted but also keep some of its contents.

Maybe scm_record should support certain implication rules, or maybe there's a better design that I haven't thought of. One possibility is to not allow line-wise selection for newly-added or newly-deleted files (which might be what hg crecord does?).

I think disabling line-wise selection and using only file-mode selection for newly-added/-deleted files should be quite tractable without any changes to the data model. (You can still render the file contents in the editor by using Section::Unchanged, to avoid regressing the workflow too much.)

If anyone wants to pick it up, let me know and I can provide more guidance.

[30350n]

Thanks a lot for that explanation! I think if I understand correctly, the main caveat here is that there's no way to differentiate between deleting a file and deleting all the lines out of a file (while keeping the empty file) in scm_record.

One possibility is to not allow line-wise selection for newly-added or newly-deleted files.

This solution would be very poor imo (newly-added files should also not be causing any issues with file mode anyways, right?).

For deleted files, I think the solution would to be to simply use the "before" file mode for any partial splits and to use the "new" file mode, only if all changes are selected.

Regarding the linked issue (i.e. files being marked as executable), couldn't you simply add a line to the scm_record diff, which enables selection of the file mode change?

[Zoybean]

I agree that I would rather not disallow linewise selection for file creation/deletion, and @30350n's interim solution is how I would expect the diff editor to behave anyway (short of a full solution).

With @arxanas' interim proposal, we're missing a few key options that I think are hard to express without the linewise diff editor:

• created non-empty file:
  1. we don't want to add it
  2. we want to add it, but leave it empty (unavailable)
  3. we want to add it and some of its lines (unavailable)
  4. we want to add it and all of its lines
• deleted non-empty file:
  1. we don't want to delete any of it
  2. we want to delete all of its lines but leave the empty file (unavailable)
  3. we want to delete some of its lines (unavailable)
  4. we want to delete the file and all of its lines

@30350n's interim proposal, we have all but the tricky-seeming options:

• created non-empty file:
  1. we don't want to add it
  2. we want to add it, but leave it empty (unavailable)
  3. we want to add it and some of its lines
  4. we want to add it and all of its lines
• deleted non-empty file:
  1. we don't want to delete any of it
  2. we want to delete all of its lines but leave the empty file (unavailable)
  3. we want to delete some of its lines
  4. we want to delete the file and all of its lines

This has me thinking, could we get some unstable CLI commands to cover the remaining 2 cases in the interim, which I think can be expressed as, "record the presence of empty file $NAME (with mode $MODE)". maybe a jj file record-empty $NAME --mode $MODE? Mark it as unstable, and remove it once the editor can handle it. As I see it, this has 2 main downsides (besides the work to implement them):

• people may rely on it despite it being marked as unstable, and will complain when it is removed.
• it being implemented at all reduces pressure on fixing the builtin diff editor, so it may end up being a permanent temporary solution.

[30350n]

I can't really think of any realsitic use cases for the two remaining cases tbh and it's not like you can't work around them already. So additional CLI options seem kind of unnecessary to me.

[Zoybean]

Yeah I suppose you can just tools like touch to the same effect.

[30350n]

Okay, just took a look at the code and it seems like @emesterhazy already investigated into this a while ago: https://github.com/martinvonz/jj/blob/415c831e306a4259167cf103a7c89a9ec883935d/cli/src/merge_tools/builtin.rs#L435-L461

I just stepped through this code with a debugger and the problem seems to be that old_mode always equals new_mode, because file.file_mode and file.get_file_mode() return the same thing.

[30350n]

Regarding the linked issue (i.e. files being marked as executable), couldn't you simply add a line to the scm_record diff, which enables selection of the file mode change?

Also it seems like this is already implemented.

[30350n]

I think I managed to fix #3846 and this issue now, but I'm running into some side effects which also need to be resolved. I'll share some draft PRs later.

[martinvonz]

I think I managed to fix #3846 and this issue now, but I'm running into some side effects which also need to be resolved. I'll share some draft PRs later.

Thanks! Can you also keep #3846 in mind when doing that? Hopefully your changes end up fixing that bug too. I think there may also be other related bugs.

[30350n]

Yep, like I said I already fixed that one ^^. Draft PR is here

[necauqua]

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

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

[arxanas]

@necauqua Thanks for reporting. I filed #5189 since I believe the underlying cause should be considered different.