Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

windows test robsustness #1444

Merged
merged 4 commits into from
Jul 5, 2024
Merged

windows test robsustness #1444

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0899c2e
feat!: on Windows, also instruct msys to create real symlinks (#1443)
Byron Jul 5, 2024
93e088a
fix gix-archive tests for when symlinks are allowed
Byron Jul 5, 2024
b3973e4
add more standard traits to `Change` type.
Byron Jul 5, 2024
f48ed0c
Allow tests to work with symlinks on Windows
Byron Jul 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 10 additions & 40 deletions gix-archive/tests/archive.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ mod from_tree {
#[test]
fn basic_usage_internal() -> gix_testtools::Result {
basic_usage(gix_archive::Format::InternalTransientNonPersistable, |buf| {
assert_eq!(buf.len(), if cfg!(windows) { 565 } else { 551 });
assert_eq!(buf.len(), 551);

let mut stream = gix_worktree_stream::Stream::from_read(std::io::Cursor::new(buf));
let mut paths_and_modes = Vec::new();
Expand All @@ -27,11 +27,7 @@ mod from_tree {
entry.read_to_end(&mut buf).expect("stream can always be read");
}

let expected_link_mode = if cfg!(windows) {
EntryKind::Blob
} else {
EntryKind::Link
};
let expected_link_mode = EntryKind::Link;
let expected_exe_mode = if cfg!(windows) {
EntryKind::Blob
} else {
Expand All @@ -53,11 +49,7 @@ mod from_tree {
(
"symlink-to-a".into(),
expected_link_mode,
hex_to_id(if cfg!(windows) {
"45b983be36b73c0788dc9cbcb76cbb80fc7bb057"
} else {
"2e65efe2a145dda7ee51d1741299f848e5bf752e"
})
hex_to_id("2e65efe2a145dda7ee51d1741299f848e5bf752e")
),
(
"dir/b".into(),
Expand Down Expand Up @@ -119,34 +111,20 @@ mod from_tree {
header.mode()?,
));
}
let expected_symlink_type = if cfg!(windows) {
EntryType::Regular
} else {
EntryType::Symlink
};
let expected_symlink_type = EntryType::Symlink;
let expected_exe_mode = if cfg!(windows) { 420 } else { 493 };
assert_eq!(
out,
[
("prefix/.gitattributes", EntryType::Regular, 56, 420),
("prefix/a", EntryType::Regular, 3, 420),
(
"prefix/symlink-to-a",
expected_symlink_type,
if cfg!(windows) { 3 } else { 0 },
420
),
("prefix/symlink-to-a", expected_symlink_type, 0, 420),
("prefix/dir/b", EntryType::Regular, 3, 420),
("prefix/dir/subdir/exe", EntryType::Regular, 0, expected_exe_mode),
("prefix/extra-file", EntryType::Regular, 21, 420),
("prefix/extra-exe", EntryType::Regular, 0, expected_exe_mode),
("prefix/extra-dir-empty", EntryType::Directory, 0, 420),
(
"prefix/extra-dir/symlink-to-extra",
expected_symlink_type,
if cfg!(windows) { 21 } else { 0 },
420
)
("prefix/extra-dir/symlink-to-extra", expected_symlink_type, 0, 420)
]
.into_iter()
.map(|(path, b, c, d)| (bstr::BStr::new(path).to_owned(), b, c, d))
Expand Down Expand Up @@ -183,7 +161,7 @@ mod from_tree {
},
|buf| {
assert!(
buf.len() < 1270,
buf.len() < 1280,
"much bigger than uncompressed for some reason (565): {} < 1270",
buf.len()
);
Expand All @@ -208,19 +186,11 @@ mod from_tree {
);
let mut link = ar.by_name("prefix/symlink-to-a")?;
assert!(!link.is_dir());
assert_eq!(
link.is_symlink(),
cfg!(not(windows)),
"symlinks are supported as well, but only on Unix"
);
assert_eq!(
link.unix_mode(),
Some(if cfg!(windows) { 0o100644 } else { 0o120644 }),
"the mode specifies what it should be"
);
assert!(link.is_symlink(), "symlinks are supported as well, but only on Unix");
assert_eq!(link.unix_mode(), Some(0o120644), "the mode specifies what it should be");
let mut buf = Vec::new();
link.read_to_end(&mut buf)?;
assert_eq!(buf.as_bstr(), if cfg!(windows) { "hi\n" } else { "a" });
assert_eq!(buf.as_bstr(), "a");
Ok(())
},
)
Expand Down
1 change: 1 addition & 0 deletions gix-index/src/entry/mode.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,7 @@ impl From<gix_object::tree::EntryMode> for Mode {
}

/// A change of a [`Mode`].
#[derive(Debug, Copy, Clone, PartialEq, Eq)]
pub enum Change {
/// The type of mode changed, like symlink => file.
Type {
Expand Down
14 changes: 11 additions & 3 deletions gix-status/src/index_as_worktree/function.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -424,12 +424,19 @@ impl<'index> State<'_, 'index> {

self.buf.clear();
self.buf2.clear();
let file_size_bytes = if cfg!(windows) && metadata.is_symlink() {
// symlinks on Windows seem to have a length of zero, so just pretend
// they have the correct length to avoid short-cutting, and enforce a full buffer check.
entry.stat.size as u64
} else {
metadata.len()
};
let fetch_data = ReadDataImpl {
buf: &mut self.buf,
path: worktree_path,
rela_path,
entry,
file_len: metadata.len(),
file_len: file_size_bytes,
filter: &mut self.filter,
attr_stack: &mut self.attr_stack,
options: self.options,
Expand All @@ -440,7 +447,7 @@ impl<'index> State<'_, 'index> {
odb_reads: self.odb_reads,
odb_bytes: self.odb_bytes,
};
let content_change = diff.compare_blobs(entry, metadata.len(), fetch_data, &mut self.buf2)?;
let content_change = diff.compare_blobs(entry, file_size_bytes, fetch_data, &mut self.buf2)?;
// This file is racy clean! Set the size to 0 so we keep detecting this as the file is updated.
if content_change.is_some() || executable_bit_changed {
let set_entry_stat_size_zero = content_change.is_some() && racy_clean;
Expand Down Expand Up @@ -531,7 +538,8 @@ where
let out = if is_symlink && self.options.fs.symlink {
// conversion to bstr can never fail because symlinks are only used
// on unix (by git) so no reason to use the try version here
let symlink_path = gix_path::into_bstr(std::fs::read_link(self.path)?);
let symlink_path =
gix_path::to_unix_separators_on_windows(gix_path::into_bstr(std::fs::read_link(self.path)?));
self.buf.extend_from_slice(&symlink_path);
self.worktree_bytes.fetch_add(self.buf.len() as u64, Ordering::Relaxed);
Stream {
Expand Down
14 changes: 2 additions & 12 deletions gix-status/tests/stack/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,21 +83,11 @@ fn paths_leading_through_symlinks_are_rejected() {
}

fn is_symlink(m: &std::fs::Metadata) -> bool {
if cfg!(windows) {
// On windows, symlinks can't be seen, at least not through std
m.is_file()
} else {
m.is_symlink()
}
m.is_symlink()
}

fn is_symlinked_dir(m: &std::fs::Metadata) -> bool {
if cfg!(windows) {
// On windows, symlinks can't be seen, at least not through std
m.is_dir()
} else {
m.is_symlink()
}
m.is_symlink()
}
fn is_file(m: &std::fs::Metadata) -> bool {
m.is_file()
Expand Down
22 changes: 2 additions & 20 deletions gix-status/tests/status/index_as_worktree.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -469,16 +469,7 @@ fn refresh() {
}
.into(),
),
(
BStr::new("empty"),
3,
Change::Modification {
executable_bit_changed: false,
content_change: Some(()),
set_entry_stat_size_zero: false
}
.into(),
),
(BStr::new("empty"), 3, Change::Type.into()),
(
BStr::new("executable"),
4,
Expand Down Expand Up @@ -551,16 +542,7 @@ fn modified() {
}
.into(),
),
(
BStr::new("empty"),
3,
Change::Modification {
executable_bit_changed: false,
content_change: Some(()),
set_entry_stat_size_zero: false,
}
.into(),
),
(BStr::new("empty"), 3, Change::Type.into()),
(
BStr::new("executable"),
4,
Expand Down
16 changes: 12 additions & 4 deletions gix-worktree-state/src/checkout/entry.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
use std::borrow::Cow;
use std::{
fs::OpenOptions,
io::Write,
Expand Down Expand Up @@ -145,12 +146,19 @@ where
err,
path: dest.to_path_buf(),
})?;
let symlink_destination = gix_path::try_from_byte_slice(obj.data)
.map_err(|_| crate::checkout::Error::IllformedUtf8 { path: obj.data.into() })?;

if symlink {
#[cfg_attr(not(windows), allow(unused_mut))]
let mut symlink_destination = Cow::Borrowed(
gix_path::try_from_byte_slice(obj.data)
.map_err(|_| crate::checkout::Error::IllformedUtf8 { path: obj.data.into() })?,
);
#[cfg(windows)]
{
symlink_destination = gix_path::to_native_path_on_windows(gix_path::into_bstr(symlink_destination))
}

try_op_or_unlink(dest, overwrite_existing, |p| {
gix_fs::symlink::create(symlink_destination, p)
gix_fs::symlink::create(symlink_destination.as_ref(), p)
})?;
} else {
let mut file = try_op_or_unlink(dest, overwrite_existing, |p| {
Expand Down
12 changes: 2 additions & 10 deletions gix-worktree-stream/tests/stream.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,11 +115,7 @@ mod from_tree {
} else {
EntryKind::BlobExecutable
};
let expected_link_mode = if cfg!(windows) {
EntryKind::Blob
} else {
EntryKind::Link
};
let expected_link_mode = EntryKind::Link;
assert_eq!(
paths_and_modes,
&[
Expand All @@ -141,11 +137,7 @@ mod from_tree {
(
"symlink-to-a".into(),
expected_link_mode,
hex_to_id(if cfg!(windows) {
"45b983be36b73c0788dc9cbcb76cbb80fc7bb057"
} else {
"2e65efe2a145dda7ee51d1741299f848e5bf752e"
})
hex_to_id("2e65efe2a145dda7ee51d1741299f848e5bf752e")
),
(
"dir/.gitattributes".into(),
Expand Down
4 changes: 3 additions & 1 deletion tests/tools/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -590,13 +590,16 @@ fn configure_command<'a>(
script_result_directory: &Path,
) -> &'a mut std::process::Command {
let never_path = if cfg!(windows) { "-" } else { ":" };
let mut msys_for_git_bash_on_windows = std::env::var("MSYS").unwrap_or_default();
msys_for_git_bash_on_windows.push_str(" winsymlinks:nativestrict");
Comment on lines +593 to +594
Copy link
Member

@EliahKagan EliahKagan Jul 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I decided to check to make sure this behaves correctly even when another winsymlinks option (rather than other possibly symlink-unrelated options) is already listed in $MSYS.

This code is correct! 😸 I say that based on this experiment:

ek@Glub MINGW64 ~/tmp
$ touch target

ek@Glub MINGW64 ~/tmp
$ MSYS=winsymlinks:lnk ln -s target lnk-symlink

ek@Glub MINGW64 ~/tmp
$ MSYS=winsymlinks:nativestrict ln -s target nativestrict-symlink

ek@Glub MINGW64 ~/tmp
$ MSYS='winsymlinks:lnk winsymlinks:nativestrict' ln -s target lnk-nativestrict-symlink

ek@Glub MINGW64 ~/tmp
$ MSYS='winsymlinks:nativestrict winsymlinks:lnk' ln -s target nativestrict-lnk-symlink

ek@Glub MINGW64 ~/tmp
$ cmd //c dir
 Volume in drive C is OS
 Volume Serial Number is B203-10FB

 Directory of C:\Users\ek\tmp

07/06/2024  07:22 PM    <DIR>          .
07/06/2024  07:22 PM    <DIR>          ..
07/06/2024  07:21 PM    <SYMLINK>      lnk-nativestrict-symlink [target]
07/06/2024  07:21 PM               525 lnk-symlink.lnk
07/06/2024  07:22 PM               525 nativestrict-lnk-symlink.lnk
07/06/2024  07:21 PM    <SYMLINK>      nativestrict-symlink [target]
07/06/2024  07:19 PM                 0 target
               5 File(s)          1,050 bytes
               2 Dir(s)  157,514,899,456 bytes free

The later option overrides the earlier one, as one would expect. This doesn't seem to be documented for CYGWIN (in Cygwin) or MSYS, but I think it's reasonable to rely on. The alternative of removing prior winsymlinks options seems much less good to me, because that effect could be unintuitive when inspecting the environment and could also create the false impression that other options such as error_start would be removed, even though they would not be.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for trying it out! Just this morning I was thinking about this, wondering if it's working as expected :).

cmd.args(args)
.stdout(std::process::Stdio::piped())
.stderr(std::process::Stdio::piped())
.current_dir(script_result_directory)
.env_remove("GIT_DIR")
.env_remove("GIT_ASKPASS")
.env_remove("SSH_ASKPASS")
.env("MSYS", msys_for_git_bash_on_windows)
.env("GIT_CONFIG_SYSTEM", never_path)
.env("GIT_CONFIG_GLOBAL", never_path)
.env("GIT_TERMINAL_PROMPT", "false")
Expand Down Expand Up @@ -751,7 +754,6 @@ fn extract_archive(
}
#[cfg(not(feature = "xz"))]
{
use std::io::Read;
input_archive.read_to_end(&mut buf)?;
}
buf
Expand Down
Loading