jj icon indicating copy to clipboard operation
jj copied to clipboard

Add executable bit configuration to allow `in_repo` and `on_disk` permissions to diverge

Open isuffix opened this issue 3 months ago • 7 comments

Let's try this again!

This is a remaking of #4478 that builds off of the many smaller PRs I've been making the past month.

One key difference from the approach in #4478 is how the third commit starts out by preserving the in_repo executable bit on all platforms, which makes the later commit adding on_disk for Unix easier to reason about.

Closes #3949

It took me a while to come to the final representation of the executable bit, but I think it's correct for what we want.

I find the symmetry between the from_repo and from_disk methods particularly convincing: if we're respecting the executable bit, then on_disk and in_repo always update to match each other. If we're ignoring the executable bit, then the two values are allowed to diverge, and instead we set them to their previous values (if known).

Checklist

If applicable:

  • [x] I have updated CHANGELOG.md
  • [x] I have updated the documentation (README.md, docs/, demos/)
  • [x] I have updated the config schema (cli/src/config-schema.json)
  • [x] I have added/updated tests to cover my changes

isuffix avatar Sep 11 '25 06:09 isuffix

Random: this has the exact same set of digits as the previous PR, 4478 vs. 7484.

isuffix avatar Sep 11 '25 07:09 isuffix

Rebased off main and added a new test in test_file_chmod_command.rs in the last commit

isuffix avatar Sep 25 '25 21:09 isuffix

I've removed the in-repo field from the ExecBit type and redone the last three commits. This is ready for another review.

isuffix avatar Oct 13 '25 21:10 isuffix

Rebased off main and fixed an out-of-date comment in the third commit.

isuffix avatar Oct 14 '25 19:10 isuffix

I haven't looked at this yet but it would be nice to have better descriptions for commits 3-5. They don't say much about the motivation or whether there are observable differences.

martinvonz avatar Oct 17 '25 19:10 martinvonz

I've found an issue with the PR design that needs a fix, and I'd like to get a confirmation on the direction before I commit to changing anything.

If we have the commands:

jj config set --repo working-copy.exec-bit-change ignore
touch file
jj file chmod x file
# on-disk file is not executable
jj config set --repo working-copy.exec-bit-change respect
jj file chmod x file
# on-disk file should be executable

The snapshot from the last command might not update the backend state, making the chmod not write anything because it thinks the file is already executable and there isn't anything to write.

I did have this pattern inside a larger test case (and have re-added it as a distinct test), but I didn't realize it was an issue sooner because it would only happen occasionally in CI and I couldn't reproduce it on my machine. So I changed the original test slightly (which fixed CI) and I thought it was either fine or an issue with CI to report later. But it turns out the changed test actually can reproduce the issue on my machine sometimes, I just got incredibly unlucky the first few times I tried.

Now that I can reproduce the issue on my machine, I've done a deep-dive and found the problem.

Why does this happen, and why only rarely?

The following file state cleanliness check should always set clean to false when the final command does its snapshot, but it only usually sets clean to false:

// lib/src/local_working_copy.rs:1600
let clean = new_file_state.is_clean(current_file_state)
    && current_file_state.mtime < self.tree_state.own_mtime;

I didn't think about this part of the code much because I don't change it in this PR, but I eventually realized it was the culprit. The first line here is always true (it should be false), so the reason clean is only usually false is because the modification time comparison on the second line will usually be false, but can be true based on chance (and is less likely on a machine that isn't overworked or running many things, which is why I've had difficulty reproducing it).

Since we want clean to always be false, the real issue is with the first line, so why is that true?

Because we only store the on-disk executable bit in the file states we're comparing, and the on-disk file never changed.

What's the fix?

When the configuration goes from ignore to respect, we need to be able to recognize a difference between the in-repo and on-disk executable bits when comparing file states.

One way would be to start reading the executable bit from the backend TreeValue before we compare file states. However, this would have to be read and checked for every file, and my understanding of the point of the file state cleanliness check is to avoid reading data from the backend in the first place.

The other way would be to store both the on-disk and in-repo bits in FileState (and in the file state cache) as I originally did in this PR in the ExecBit struct but was advised to change.

I would like to go back to the original design of the ExecBit struct, but I'd like confirmation first before I commit to changing anything.

isuffix avatar Oct 22 '25 21:10 isuffix

jj config set --repo working-copy.exec-bit-change ignore
touch file
jj file chmod x file
# on-disk file is not executable
jj config set --repo working-copy.exec-bit-change respect
jj file chmod x file
# on-disk file should be executable

This is common problem in EOL handling and watchman, and I personally don't think we need a special support. User should check out all files if they change exec-bit-change mode.

If we need a workaround, we can record the previous working-copy.exec-bit-change mode in tree_state file. If the mode changes, cached file states cannot be trusted, so all working-copy files will be snapshotted. In the example above, the second jj file chmod x file will notice that the working-copy file isn't an executable, so it will first snapshot the current state and update it to executable.

yuja avatar Oct 23 '25 08:10 yuja

It sounds like this is ready to merge. Anything left to do, @isuffix?

martinvonz avatar Nov 26 '25 14:11 martinvonz

Rebased on main and fixed the conflicts and broken tests.

It sounds like this is ready to merge. Anything left to do, @isuffix?

As you can see from the error in CI, this is not ready. We still need to resolve the issue in my previous comment.

For that, we can either go back to storing both the on-disk and in-repo bits in the FileState or start storing the working-copy.exec-bit-change mode in the working copy's tree_state file. I prefer the former, but I'm fine with either. What would you prefer?

isuffix avatar Nov 30 '25 22:11 isuffix

Do you mean the case quoted by @yuja above? I.e. this one:

jj config set --repo working-copy.exec-bit-change ignore
touch file
jj file chmod x file
# on-disk file is not executable
jj config set --repo working-copy.exec-bit-change respect
jj file chmod x file
# on-disk file should be executable

I actually think the jj file chmod x file there is expected to be a no-op because it doesn't have any effect on the working-copy commit, so it should have no effect on the files in the working copy either. So I think I agree with Yuya that we don't need special support for it. We can just document that changing the value of the config requires updating the files (e.g. touching them) for changing to be detected. Does that sound okay?

martinvonz avatar Dec 01 '25 06:12 martinvonz

I'm fine with that. I'll add to the documentation in config.md and rewrite the test case so it also updates the file content.

isuffix avatar Dec 01 '25 19:12 isuffix

I'll merge this tomorrow unless there are any new comments.

isuffix avatar Dec 03 '25 17:12 isuffix

I'll merge this tomorrow unless there are any new comments.

You'll need to rebase and move the changelog entry depending on when the release happens.

yuja avatar Dec 04 '25 01:12 yuja

Moved the changelog entry to unreleased (0.37)

isuffix avatar Dec 04 '25 16:12 isuffix

I realized the documentation in config.md should actually go under the Working copy settings header and moved it. I haven't updated it to fully match the style of the surrounding settings descriptions, but I'll leave that for a later PR if needed.

isuffix avatar Dec 04 '25 17:12 isuffix

Thanks a lot for working on this! When might it be included in a release?

ofek avatar Dec 04 '25 18:12 ofek

it's included if you build from HEAD or in January's release which is on 2026.1.7

PhilipMetzger avatar Dec 04 '25 18:12 PhilipMetzger

And to build it you would run: cargo install --path ./cli

isuffix avatar Dec 04 '25 18:12 isuffix