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

[isuffix]

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:

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

[isuffix]

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

[isuffix]

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

[isuffix]

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

[PhilipMetzger]

LG

[isuffix]

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

[martinvonz]

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.

[github-actions]

The following commits do not follow our format for subject lines:

• 76261c5: (draft) flaky test fixes

Commits should have a subject line following the format <topic>: <description>. Please review the commit guidelines for more information.

All commits are now correctly formatted. Thank you for your contribution!

[yuja]

Looks generally good to me, thanks for working this feature.

[yuja]

I thought this could be worked around by set_executable() or its caller, but I'm not against the current implementation either. The difference would only matter in reset() where we don't touch on-disk files at all.

fn set_executable(exec_bit: ExecBit, disk_path: &Path) -> Result<ExecBit, io::Error> {
    ....
    // Windows
    ExecBit(false)
}

let real_exec_bit = set_executable(desired_exec_bit, disk_path)?;
Ok(FileState::for_file(real_exec_bit, ...)

[isuffix]

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.

[yuja]

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.