Add config for ignoring executable bit changes #4478

wrzian · 2024-09-15T16:41:15Z

Starting a PR to close #3949 . This is my first time contributing, so I would appreciate feedback on the code. I'm certain that there are some places that can be integrated better. I've left a few TODO markers scattered around that I would appreciate help with.

Also: I have not run these changes locally yet. They build and pass cargo test, but I'm still not certain how to run these myself.

I also haven't added any tests yet. I'd appreciate feedback on a direction to go for those and where it would be best to test these changes.

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 tests to cover my changes

martinvonz

Not a full review, just a few random comments

lib/src/local_working_copy.rs

martinvonz · 2024-09-15T17:54:00Z

lib/src/local_working_copy.rs

-            file_type: FileType::Normal {
-                executable: FileExecutableFlag::default(),
-            },
+            file_type: FileType::default(),


Now that the type doesn't vary at compile time between platform, we can use the same value on all platforms here and probably not need to implement FileType::default(). We should even be able to run this test case on Windows with a value for the the executable bit. Not that I think we need to do that; I'm just saying that there's no longer a need to avoid certain tests on Windows.

martinvonz · 2024-09-15T17:56:23Z

lib/src/settings.rs

+/// This is not a method on UserSettings since it needs to be callable even when
+/// only having the config itself (currently only used in `Ui::with_config`).
+pub fn report_executable_bit(config: &config::Config) -> Option<bool> {
+    config.get_bool("core.report-executable-bit").ok()


I feel like core would be better replaced by working-copy or local-working-copy, but I know we already use core so this is consistent for now. We can decide about renaming after this PR.

martinvonz · 2024-09-15T17:59:15Z

lib/src/settings.rs

+///
+/// This is not a method on UserSettings since it needs to be callable even when
+/// only having the config itself (currently only used in `Ui::with_config`).
+pub fn report_executable_bit(config: &config::Config) -> Option<bool> {


nit: Since this method is only used in local_working_copy.rs (right?), you can just define it there.

Nope 🙃 It's also used in cli/src/ui.rs. See the added exec_config arguments in the merge_tools/{mod,diff_working_copies}.rs files that originate from the ui.

Though I'm still not sure where to put this. If we choose local_working_copy.rs does that have any effect on other users of the library? I'm fine moving it there if you recommend

lib/src/settings.rs

martinvonz · 2024-09-15T18:05:36Z

Thanks, btw! It looks good overall

docs/config.md

martinvonz · 2024-09-16T03:53:01Z

docs/config.md

-executable bit and update the repository. Otherwise we will only update files if
-they are manually changed via `jj file chmod`.
+executable bits are supported. If they are, we will not ignore them and changing
+a files' executable bit will update the repository. If they are not supported we


nits:

files' -> file's

not ignore -> respect

On unix we -> On Unix, we

On unix setting -> On Unix, setting

Maybe it will be clearer add an introductory paragraph, making the whole thing something like this:

If you update the working copy to a commit, then the executable bit for the files in the file system will normally be updated to match the commit. Similarly, if you update a file's executable bit in the file system, then the subsequent snapshotting of the working copy will result in the executable bit getting updated in the working-copy commit.

This configuration option makes Jujutsu ignore the executable bit in the file system and instead preserve the executable bit in the commit. New files in the file system will be recorded as not executable (you can use jj file chmod to set it afterwards).

Since there is no executable bit on Windows, this option is effectively always on there. On Unix, Jujutsu will try to detect whether the file system supports executable bits. The configured value will override the detected value.

Just a thought. Take what you like from that text, or ignore it completely.

Thoughts on this?

Ignore the executable bit

Whether to ignore changes to the executable bit for files on Unix. This option
is unused on Windows.

core.ignore-executable-bit = true | false

On Unix systems, files have permission bits for whether they are executable as
scripts or binary code. Jujutsu stores this executable bit within each commit
and will update it for files as you operate on a repository. If you update your
working copy to a commit where a file is recorded as executable or not, jj
will adjust the permission of that file. If you change a file's executable bit
through the filesystem, jj will record that change in the working-copy commit
on the next snapshot.

Setting this to true will make Jujutsu ignore the executable bit on the
filesystem and only store the executable bit in the commit itself. jj will
also not attempt to modify file's executable bits and will add new files as "not
executable." This is the default behavior on Windows and can be useful for users
dual-booting Windows from Unix or experimenting with nonstandard filesystems.

On Unix, Jujutsu will try to detect whether the filesystem supports executable
bits to choose a default value, but a configured value will always override the
default. On Windows systems there is no executable bit, and this option is not
used.

You can use always use jj file chmod to update the recorded executable bit for
a file in the commit manually. If this option is false, jj will propagate that
change to the filesystem.

I think the short description at the top and the explicit line with the available options is really helpful to let a user who is just skimming the docs get what they need and be able to choose whether to read the rest or not.
I made your intro paragraph a bit more explicit about how the working-copy commit reads from the filesystem.

I prefer to use the word "ignore" less, since it has meaning already in a slightly different way.
I also think that an option saying "ignore=true" seems backward. It sounds much better to have "respect=false" since the positive action corresponds to the variable, so when you read the logic (or explain it), you don't have the negative verb and a false value confusing things. An example would be "show=false" versus "hide=true" where the logic is like if show then print versus if not hide then print.

I feel like ignore works fine, it's far enough from any gitignore files that I don't really care. However I'm fine with either "ignore" or "respect" as the main words.
Should I change it again or not?

lib/src/settings.rs

PhilipMetzger

Just some nits to conform to our code review style here: https://github.com/martinvonz/jj/blob/main/docs/contributing.md#code-reviews

lib/src/settings.rs

CHANGELOG.md

yuja · 2024-09-17T00:47:31Z

lib/src/local_working_copy.rs

+        store: Arc<Store>,
+        working_copy_path: PathBuf,
+        state_path: PathBuf,
+        exec_config: Option<bool>,


Perhaps, exec_config can be carried by SnapshotOptions? When materializing working-copy files, it's probably okay to try chmod() even if the filesystem doesn't support exec bit.

It's a bit odd that TreeState::empty() constructor needs it.

I can see how SnapshotOptions might work, but I'm not sure that it would preserve the correct behavior when doing a checkout/reset/recover and then calling LockedWorkingCopy::finish(), since those save to the repo. If we're ok with sending the permission changes regardless of if they might work then I could see it working, but it feels somewhat 'less correct'? Not sure.

(assuming fchmod() doesn't fail,) checkout is repo->working-copy operation, so it won't have to read exec bit from disk. reset() and recover() are similar?

Yes, but then its the call to finish() afterwards that tries to write to the repo. And finish doesn't currently take any other configuration. I think it makes sense to keep the config with the tree state.

Just to be celar, LockedLocalWorkingCopy::finish() doesn't write commits to repo, but writes working-copy/tree states to disk. It's more or less a serialization of in-memory data.

That said, it might be okay to keep a copy of filesystem capability options in TreeState. We already have symlink_support: bool (and store: Arc<Store> which isn't state data but a shared resource.) If we make symlink_support configurable, we'll have to resolve it somewhere in LocalWorkingCopy, and pass in to TreeState constructor (or methods.)

Ah, that makes more sense forfinish.

How would you feel about some sort of FileCapability or Platform trait/module? It seems like it effectively already exists for symlink and executable bits in file_util.rs, and it would be nice to add helpers like a from_config() function.

How would you feel about some sort of FileCapability or Platform trait/module? It seems like it effectively already exists for symlink and executable bits in file_util.rs, and it would be nice to add helpers like a from_config() function.

I don't think extensibility provided by trait would be needed. These parameters can be packed into struct FileCapability { some bools }, but I don't know if that makes sense. Config parameters would have to be Option<bool>s, and we'll need to explicitly control where/when the actual fs capability is tested. iirc, the error of check_symlink_support() isn't propagated right now, but it should be.

yuja · 2024-09-17T00:49:41Z

lib/src/local_working_copy.rs

@@ -1644,6 +1777,7 @@ impl LocalWorkingCopy {
        state_path: PathBuf,
        operation_id: OperationId,
        workspace_id: WorkspaceId,
+        config: &config::Config,


nit: we typically use &UserSeettings object.

I'm only using config because the Ui object only gets initialized with a config and it felt odd to pass user_settings into everything and then only use it by calling: ignore_exec_bit(user_settings.config()). I'm ok with changing it but I don't really see a reason to pass the full user settings?

I don't really see a reason to pass the full user settings?

I don't have a concrete plan, but we'll probably replace config::Config with our config module that supports layering better. Then, config::Config APIs might be rehosted on UserSettings.

Ok, then I will move to passing UserSettings

wrzian · 2024-09-21T13:59:26Z

Pushed some small fixes in the local_working_copy changes, also added a new commit that refactors the check_out functions in diff_working_copy.rs. Let me know if this needs any work.

I've also started using the update on my machine, and it seems to work correctly on my dual-boot repo with the config, but I haven't really tested all of the behavior.
(Still need to add actual tests for the new behavior and look at the failing executable bit test)

One other note is that I don't think my current changes work correctly if you update the config value. That is, if we alter the config value to stop respecting executable bits, the tree should refresh itself, but I think it still keeps the stored changes instead of refreshing even if I do jj status. Not sure what to do for that.

martinvonz · 2024-09-22T06:15:06Z

docs/config.md

+
+Setting this to `true` will make Jujutsu ignore the executable bit on the
+filesystem and only store the executable bit in the commit itself. `jj` will
+also not attempt to modify file's executable bits and will add new files as "not


nit: file's -> files' (or make it "a file's executable bit")

docs/config.md

lib/src/local_working_copy.rs

martinvonz · 2024-09-22T17:52:54Z

lib/src/local_working_copy.rs

+pub enum ExecFlag {
+    Exec(bool),
+    Ignore,
+}


Is it possible to always use a bool instead of this type and only change how we used that bool based on the IgnoreExec?

We could, the main difference is just whether the tree state stores that a file is not executable at a given moment. I assume this would affect users who modify the configuration between snapshots, but I don't really know what that would imply.

I can convert it to just a bool if you'd like.

Alternatively, it might be better to use a bool-equivalent enum, e.g. enum { Exec, NotExec }.

We could, the main difference is just whether the tree state stores that a file is not executable at a given moment. I assume this would affect users who modify the configuration between snapshots, but I don't really know what that would imply.

Good point. I think that means it's better to do our best to keep the stored flag up to date even the configuration currently says to ignore the executable bit in the file system. Otherwise we might clear the flag in the file and not notice if the user then sets the config to respect the config and also has cleared the executable bit on some file in the file system.

Alternatively, it might be better to use a bool-equivalent enum, e.g. enum { Exec, NotExec }.

I would go with just a bool until we find that it's hard to read or likely to cause bugs for some reason (e.g. because we confuse that boolean for some other boolean value).

lib/src/local_working_copy.rs

…Unix This is just the initial support for the feature. Threading the value into the working copy via user configuration is in the next commit.

…Side enum. I got annoyed by the checkout function above in my previous commit and tried to compress the code a bit, and I think the result is much nicer. I'm not certain that a closure is the right solution here, but I think it becomes easier to reason about the two temp filenames that get generated. I initially changed Option<DiffSide> to a bool, but found an enum more meaningful and more likely to be flexible in the future. I also moved the instruction writing to its own function. This makes the main `DiffEditWorkingCopies::check_out()` function is much nicer. J: This commit contains the following changes:

wrzian force-pushed the add-executable-config branch 4 times, most recently from ce8f245 to 76524dd Compare September 15, 2024 17:49

martinvonz reviewed Sep 15, 2024

View reviewed changes

wrzian force-pushed the add-executable-config branch from 5ed90ba to 83a0b0c Compare September 16, 2024 02:35

wrzian changed the title ~~Add config for reporting executable bit changes~~ Add config for ignoring executable bit changes Sep 16, 2024

wrzian force-pushed the add-executable-config branch from 83a0b0c to 685368d Compare September 16, 2024 03:04

martinvonz reviewed Sep 16, 2024

View reviewed changes

PhilipMetzger reviewed Sep 16, 2024

View reviewed changes

lib/src/settings.rs Show resolved Hide resolved

CHANGELOG.md Outdated Show resolved Hide resolved

CHANGELOG.md Outdated Show resolved Hide resolved

CHANGELOG.md Outdated Show resolved Hide resolved

yuja reviewed Sep 17, 2024

View reviewed changes

wrzian force-pushed the add-executable-config branch 3 times, most recently from eb5d4cb to e7df875 Compare September 21, 2024 13:52

wrzian force-pushed the add-executable-config branch 4 times, most recently from 9f2112b to ca6e105 Compare September 21, 2024 15:07

PhilipMetzger mentioned this pull request Sep 22, 2024

lib: Supporting building for wasm32-wasip1 #4519

Open

martinvonz reviewed Sep 22, 2024

View reviewed changes

martinvonz mentioned this pull request Sep 22, 2024

Add workspace config #3670

Open

3 tasks

wrzian force-pushed the add-executable-config branch 4 times, most recently from 98fe553 to 8c192a9 Compare September 24, 2024 01:08

wrzian added 3 commits September 23, 2024 21:14

local_working_copy: Initial option for reporting exec bit changes on …

51d7d8e

…Unix This is just the initial support for the feature. Threading the value into the working copy via user configuration is in the next commit.

local_working_copy: Allow user to configure exec bit behavior on Unix

660b01c

wrzian force-pushed the add-executable-config branch from 8c192a9 to 3982b04 Compare September 24, 2024 01:15

PhilipMetzger mentioned this pull request Dec 21, 2024

merge_tools: preserve executable bit on resolve #5169

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add config for ignoring executable bit changes #4478

Add config for ignoring executable bit changes #4478

wrzian commented Sep 15, 2024 •

edited

Loading

martinvonz left a comment

martinvonz Sep 15, 2024

martinvonz Sep 15, 2024

martinvonz Sep 15, 2024

wrzian Sep 16, 2024

wrzian Sep 16, 2024

martinvonz commented Sep 15, 2024

martinvonz Sep 16, 2024

wrzian Sep 16, 2024

wrzian Sep 16, 2024

joyously Sep 16, 2024

wrzian Sep 19, 2024

PhilipMetzger left a comment

yuja Sep 17, 2024

wrzian Sep 19, 2024

yuja Sep 19, 2024

wrzian Sep 21, 2024

yuja Sep 21, 2024

wrzian Sep 23, 2024

yuja Sep 24, 2024

yuja Sep 17, 2024

wrzian Sep 19, 2024

yuja Sep 19, 2024

wrzian Sep 23, 2024

wrzian commented Sep 21, 2024 •

edited

Loading

martinvonz Sep 22, 2024

martinvonz Sep 22, 2024

wrzian Sep 24, 2024

wrzian Sep 24, 2024

martinvonz Sep 24, 2024

Add config for ignoring executable bit changes #4478

Are you sure you want to change the base?

Add config for ignoring executable bit changes #4478

Conversation

wrzian commented Sep 15, 2024 • edited Loading

Checklist

martinvonz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martinvonz commented Sep 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ignore the executable bit

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PhilipMetzger left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wrzian commented Sep 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wrzian commented Sep 15, 2024 •

edited

Loading

wrzian commented Sep 21, 2024 •

edited

Loading