jj split of a newly added executable file does not commit file as executable

[spectral54]

Description

Steps to Reproduce the Problem

$ jj git init
$ echo hi > executable_file && chmod +x executable_file
$ echo hi > other_file
$ jj split        # pick only executable_file
$ jj status
Working copy changes:
M executable_file
A some_other_file
Working copy : ysmvmwqx edefcfd6 (no description set)
Parent commit: zosvqltr 1fca1963 Commit executable file
$ jj diff
Non-executable file became executable at executable_file:
Added regular file some_other_file:
        1: hi

Expected Behavior

Files committed via jj split should also commit their metadata changes (such as +x).

Actual Behavior

The file contents are committed, but not the metadata

Specifications

• Platform: Linux
• Version: jj google-0.17.1-jj-client_20240528_00_RC00

[martinvonz]

@arxanas: What are we supposed to pass into scm-record in this case? We currently don't add a mode section if the file was absent on one side because we intended for the mode from the other side to be preserved, but that does happen here. Is File::file_mode supposed to indicate the file mode on the "before" side of the diff (i.e. absent in this case)? In that case we'll have to keep track of the new mode in the integration in jj-cli. Or are we supposed to set File::file_mode to the "after" state (i.e. executable in this case)?

[arxanas]

Here is the documentation for File::file_mode:

The Unix file mode of the file (before any changes), if available. This may be rendered by the UI.

This value is not directly modified by the UI; instead, construct a Section::FileMode and use the [FileState::get_file_mode] function to read a user-provided updated to the file mode function to read a user-provided updated to the file mode.

@martinvonz I think it's the "before" case you're describing, so the file mode doesn't appear in the "after" version.

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?).

Also, maybe scm_record trying really hard to preserve/maintain the "before" file mode is misguided anyways 🙃. It might be better to include only opaque caller-supplied metadata, or just force the caller to handle it out-of-band.

[lgarron]

I just ran into this issue, and it took me three attempts to realize I was expecting this to be the behaviour of jj when it is not.

I can't speak for everyone, but I cannot think of a situation where I create a new file with executable permissions that I would not want jj split to commit as executable.

Although I expect to commit the entire contents of the script virtually every time, I think this makes sense even if I commit a subset. (Otherwise, I would have waited to put the executable flag on it.)

Now that I know what's going on I can work around it, but I think it would be great to fix for future jj users.

[lgarron]

I can confirm that #3846 is working well for me with brew install --HEAD jj! 🥳