feat: hide .jj directory on Windows #6514
Conversation
On macOS and Linux, directories starting with a dot (.) are hidden by default. However, on Windows, we need to set additional attributes to achieve the same behavior. To maintain consistency across platforms, we now hide the .jj directory on Windows as well. Note: This change requires using unsafe code to call Windows FFI functions. Therefore, we had to relax the unsafe code restrictions in lib/src/lib.rs. Closes: jj-vcs#6513
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
|
Sounds reasonable, but do you know if Git, Mercurial, or other tools do this? Otherwise I'd be worried that this behavior will surprise users. |
I used to be a Windows git user(as for other tools, I have not used). For the
|
|
Looks like it's controlled by this Git config. (I don't think we need a config for it.) |
|
Git seems to have many 'quite peculiar' settings. For me, apart from:
|
lib/tests/test_init.rs
Outdated
There was a problem hiding this comment.
nit: please squash this into the parent commit
There was a problem hiding this comment.
Just for reference, Mercurial doesn't do, and there were some push back against the feature.
https://lists.mercurial-scm.org/pipermail/mercurial-devel/2014-March/205421.html
|
Can you also check if the |
lib/src/lib.rs
Outdated
There was a problem hiding this comment.
Nit: per contributing guidelines you can squash the third commit into this and probably should use workspace: <title> as the topic, since feat: just means feature which isn't something used by the project.
Yes, your concern is valid - there is indeed an issue where the This would be implemented around lines 108-109 in this code: jj/cli/src/commands/git/init.rs Lines 100 to 115 in ee9917e And also around line 147 in this code: jj/cli/src/commands/git/clone.rs Lines 126 to 149 in 9d1f799 I'm not very familiar with jj yet - are there any other possible scenarios/cases I should be aware of? |
|
We don't want to make Lines 233 to 238 in 9d1f799 That should cover both the |
|
I think ".git" directory should be (ideally) handled by gitoxide, and would have to respect the Git configuration. An easier (but adhoc) option might be to copy the hidden attribute from colocated https://doc.rust-lang.org/stable/std/fs/fn.set_permissions.html |
I don’t know much about gitoxide, but based on my search, it's a Rust implementation of Git. If possible, could you provide more details? :) https://git-scm.com/docs/git-config#Documentation/git-config.txt-corehideDotFiles Based on the current discussion, I’ve extracted the logic for hiding #[cfg(windows)]
pub fn hide_dotjj_and_dotgit(workspace_root: &Path) {
use std::ffi::CString;
use winapi::um::fileapi::SetFileAttributesA;
use winapi::um::winnt::FILE_ATTRIBUTE_HIDDEN;
let jj_dir = workspace_root.join(".jj");
let git_dir = workspace_root.join(".git");
let c_jj_dir = CString::new(jj_dir.as_os_str().as_encoded_bytes()).unwrap();
let c_git_dir = CString::new(git_dir.as_os_str().as_encoded_bytes()).unwrap();
#[allow(unsafe_code)]
unsafe {
if jj_dir.exists() {
let hide_jj_dir = SetFileAttributesA(c_jj_dir.as_ptr(), FILE_ATTRIBUTE_HIDDEN);
if hide_jj_dir == 0 {
println!("Failed to hide .jj directory");
}
}
if git_dir.exists() {
let hide_git_dir = SetFileAttributesA(c_git_dir.as_ptr(), FILE_ATTRIBUTE_HIDDEN);
if hide_git_dir == 0 {
println!("Failed to hide .git directory");
}
}
}
} It is applied at line 121 in the following function to cover Lines 118 to 127 in cb877a9 And at lines 244-245 in the following function to cover Lines 222 to 245 in cb877a9 Any further thoughts? |
https://github.com/search?q=repo%3AGitoxideLabs%2Fgitoxide%20hidedotfiles&type=code Appears that they have a plan.
It's unclear from the doc, but Git supports |
|
@Natural-selection1: Could you sign the Google CLA? We won't be able to accept your PR without it. |
I signed it before, but forgot to rescan as required. Sorry about that. : ) |
Respecting Git settings seems unlikely, as many tools bundle their own Git. Here's a demonstration of cargo's built-in Git behavior: However, after configuration,
Perhaps only respecting pre-existing
I wonder if this approach would be convincing? |
Well, I assume I don't know whether the
Maybe |
Remember the title of our PR? It's to ensure
Perhaps you could take a look at my current implementation (please ignore the rough eprintln for now). You can start reading from around #[cfg(windows)]
pub fn set_dotgit_and_dotjj_visibility(workspace_root: &Path) {
use std::ffi::CString;
use std::time::Duration;
use std::time::SystemTime;
use winapi::um::fileapi::GetFileAttributesA;
use winapi::um::fileapi::SetFileAttributesA;
use winapi::um::winnt::FILE_ATTRIBUTE_HIDDEN;
let dotjj_path = workspace_root.join(".jj");
let dotgit_path = workspace_root.join(".git");
let c_dotgit_path = CString::new(dotgit_path.as_os_str().as_encoded_bytes()).unwrap();
fn hide_folder(path: &Path) {
let dir_name = Path::new(path).file_name().unwrap().to_str().unwrap();
let c_path = CString::new(path.as_os_str().as_encoded_bytes()).unwrap();
#[allow(unsafe_code)]
unsafe {
if SetFileAttributesA(c_path.as_ptr(), FILE_ATTRIBUTE_HIDDEN) == 0 {
eprintln!("Failed to hide {dir_name}");
}
}
}
fn get_folder_age(path: &Path) -> Result<Duration, io::Error> {
SystemTime::now()
.duration_since(fs::metadata(path)?.created()?)
.map_err(|e| std::io::Error::new(std::io::ErrorKind::Other, e))
}
match dotgit_path.exists() {
// cover `jj git init`
// because there is no `.git` created by us just now or exists before,
// so we hide `.jj`, just like dot_folder on Mac or Linux
false => {
hide_folder(&dotjj_path);
}
// cover `jj git init --colocate` and `jj git clone [Source] --colocate`
true => {
// check if it's created by us just now or is old
let is_dotgit_exist_before = match get_folder_age(&dotgit_path) {
Ok(duration) => duration > Duration::from_secs(1),
Err(_) => true,
};
if is_dotgit_exist_before {
// if `.git` exists before, keep `.jj` the same visibility as `.git`
#[allow(unsafe_code)]
unsafe {
if GetFileAttributesA(c_dotgit_path.as_ptr()) & FILE_ATTRIBUTE_HIDDEN != 0 {
hide_folder(&dotjj_path);
}
}
} else {
// if `.git` is newly created, hide `.jj` and `.git` just like dot_folder on Mac or Linux
hide_folder(&dotjj_path);
hide_folder(&dotgit_path);
}
}
}
}I think the above approach might be the best solution without adding user configuration options.
I don't think we need to be overly cautious about unsafe here - we're just calling FFI, not playing with raw pointers.
I haven't found relevant doc, but judging from |
I know, but (Just for reference, Mercurial doesn't make
I believe we shouldn't do timestamp-based heuristics. (I suggested (2) just because it's easier to implement.) To make both BTW, you should use |
Appears that Microsoft's @thoughtpolice might have an opinion about the dependency. |
|
I've pushed a new version. Please ignore the suboptimal git commit messages for now (I'm currently having some difficulties using jj to manage the GitHub repository). If we agree this PR is mergeable, I'll reorganize the commits properly in Git : ) |
Oh, I noticed https://crates.io/crates/windows when I was looking up information about SetFileAttributeW. |
Closes: #6513
Checklist
If applicable:
CHANGELOG.mdThis is my first attempt to contribute to the jj project. I have read https://jj-vcs.github.io/jj/prerelease/contributing/, but it's still unclear, which caused me to encounter many issues (and I'm not sure how to resolve them) while trying to use jj to manage this contribution. If there's anything I've overlooked, any suggestions are welcome.