Merge branch 'sh-on-windows'

Byron · Byron · commit 2b80d8424196 · 2023-11-16T17:05:20.000+01:00
diff --git a/Cargo.lock b/Cargo.lock
diff --git a/gitoxide-core/src/pack/multi_index.rs b/gitoxide-core/src/pack/multi_index.rs
@@ -82,7 +82,7 @@ pub fn entries(multi_index_path: PathBuf, format: OutputFormat, mut out: impl st
     if format != OutputFormat::Human {
         bail!("Only human format is supported right now");
     }
-    let file = gix::odb::pack::multi_index::File::at(&multi_index_path)?;
+    let file = gix::odb::pack::multi_index::File::at(multi_index_path)?;
     for entry in file.iter() {
         writeln!(out, "{} {} {}", entry.oid, entry.pack_index, entry.pack_offset)?;
     }
diff --git a/gitoxide-core/src/repository/exclude.rs b/gitoxide-core/src/repository/exclude.rs
@@ -38,7 +38,7 @@ pub fn query(
     let index = repo.index()?;
     let mut cache = repo.excludes(
         &index,
-        Some(gix::ignore::Search::from_overrides(&mut overrides.into_iter())),
+        Some(gix::ignore::Search::from_overrides(overrides.into_iter())),
         Default::default(),
     )?;
 
diff --git a/gitoxide-core/src/repository/index/mod.rs b/gitoxide-core/src/repository/index/mod.rs
@@ -49,7 +49,7 @@ pub fn from_list(
     let object_hash = gix::hash::Kind::Sha1;
 
     let mut index = gix::index::State::new(object_hash);
-    for path in std::io::BufReader::new(std::fs::File::open(&entries_file)?).lines() {
+    for path in std::io::BufReader::new(std::fs::File::open(entries_file)?).lines() {
         let path: PathBuf = path?.into();
         if !path.is_relative() {
             bail!("Input paths need to be relative, but {path:?} is not.")
diff --git a/gix-attributes/tests/state/mod.rs b/gix-attributes/tests/state/mod.rs
@@ -15,6 +15,7 @@ mod value {
 }
 
 #[test]
+#[allow(invalid_from_utf8)]
 fn from_value() {
     assert!(std::str::from_utf8(ILLFORMED_UTF8).is_err());
     assert!(
diff --git a/gix-command/Cargo.toml b/gix-command/Cargo.toml
@@ -14,8 +14,10 @@ doctest = false
 
 [dependencies]
 gix-trace = { version = "^0.1.3", path = "../gix-trace" }
+gix-path = { version = "^0.10.0", path = "../gix-path" }
 
 bstr = { version = "1.5.0", default-features = false, features = ["std"] }
+shell-words = "1.0"
 
 [dev-dependencies]
 gix-testtools = { path = "../tests/tools" }
diff --git a/gix-command/src/lib.rs b/gix-command/src/lib.rs
@@ -20,6 +20,19 @@ pub struct Prepare {
     pub env: Vec<(OsString, OsString)>,
     /// If `true`, we will use `sh` to execute the `command`.
     pub use_shell: bool,
+    /// If `true` (default `true` on windows and `false` everywhere else)
+    /// we will see if it's safe to manually invoke `command` after splitting
+    /// its arguments as a shell would do.
+    /// Note that outside of windows, it's generally not advisable as this
+    /// removes support for literal shell scripts with shell-builtins.
+    ///
+    /// This mimics the behaviour we see with `git` on windows, which also
+    /// won't invoke the shell there at all.
+    ///
+    /// Only effective if `use_shell` is `true` as well, as the shell will
+    /// be used as a fallback if it's not possible to split arguments as
+    /// the command-line contains 'scripting'.
+    pub allow_manual_arg_splitting: bool,
 }
 
 mod prepare {
@@ -54,6 +67,14 @@ mod prepare {
             self
         }
 
+        /// Use a shell, but try to split arguments by hand if this be safely done without a shell.
+        ///
+        /// If that's not the case, use a shell instead.
+        pub fn with_shell_allow_argument_splitting(mut self) -> Self {
+            self.allow_manual_arg_splitting = true;
+            self.with_shell()
+        }
+
         /// Configure the process to use `stdio` for _stdin.
         pub fn stdin(mut self, stdio: Stdio) -> Self {
             self.stdin = stdio;
@@ -103,14 +124,38 @@ mod prepare {
     impl From<Prepare> for Command {
         fn from(mut prep: Prepare) -> Command {
             let mut cmd = if prep.use_shell {
-                let mut cmd = Command::new(if cfg!(windows) { "sh" } else { "/bin/sh" });
-                cmd.arg("-c");
-                if !prep.args.is_empty() {
-                    prep.command.push(" \"$@\"")
+                let split_args = prep
+                    .allow_manual_arg_splitting
+                    .then(|| {
+                        if gix_path::into_bstr(std::borrow::Cow::Borrowed(prep.command.as_ref()))
+                            .find_byteset(b"\\|&;<>()$`\n*?[#~%")
+                            .is_none()
+                        {
+                            prep.command
+                                .to_str()
+                                .and_then(|args| shell_words::split(args).ok().map(Vec::into_iter))
+                        } else {
+                            None
+                        }
+                    })
+                    .flatten();
+                match split_args {
+                    Some(mut args) => {
+                        let mut cmd = Command::new(args.next().expect("non-empty input"));
+                        cmd.args(args);
+                        cmd
+                    }
+                    None => {
+                        let mut cmd = Command::new(if cfg!(windows) { "sh" } else { "/bin/sh" });
+                        cmd.arg("-c");
+                        if !prep.args.is_empty() {
+                            prep.command.push(" \"$@\"")
+                        }
+                        cmd.arg(prep.command);
+                        cmd.arg("--");
+                        cmd
+                    }
                 }
-                cmd.arg(prep.command);
-                cmd.arg("--");
-                cmd
             } else {
                 Command::new(prep.command)
             };
@@ -140,5 +185,6 @@ pub fn prepare(cmd: impl Into<OsString>) -> Prepare {
         args: Vec::new(),
         env: Vec::new(),
         use_shell: false,
+        allow_manual_arg_splitting: cfg!(windows),
     }
 }
diff --git a/gix-command/tests/command.rs b/gix-command/tests/command.rs
@@ -15,12 +15,25 @@ mod prepare {
         let cmd = std::process::Command::from(gix_command::prepare(""));
         assert_eq!(format!("{cmd:?}"), "\"\"");
     }
+
     #[test]
     fn single_and_multiple_arguments() {
         let cmd = std::process::Command::from(gix_command::prepare("ls").arg("first").args(["second", "third"]));
         assert_eq!(format!("{cmd:?}"), quoted(&["ls", "first", "second", "third"]));
     }
 
+    #[test]
+    fn multiple_arguments_in_one_line_with_auto_split() {
+        let cmd = std::process::Command::from(
+            gix_command::prepare("echo first second third").with_shell_allow_argument_splitting(),
+        );
+        assert_eq!(
+            format!("{cmd:?}"),
+            quoted(&["echo", "first", "second", "third"]),
+            "we split by hand which works unless one tries to rely on shell-builtins (which we can't detect)"
+        );
+    }
+
     #[test]
     fn single_and_multiple_arguments_as_part_of_command() {
         let cmd = std::process::Command::from(gix_command::prepare("ls first second third"));
@@ -36,7 +49,11 @@ mod prepare {
         let cmd = std::process::Command::from(gix_command::prepare("ls first second third").with_shell());
         assert_eq!(
             format!("{cmd:?}"),
-            quoted(&[SH, "-c", "ls first second third", "--"]),
+            if cfg!(windows) {
+                quoted(&["ls", "first", "second", "third"])
+            } else {
+                quoted(&[SH, "-c", "ls first second third", "--"])
+            },
             "with shell, this works as it performs word splitting"
         );
     }
@@ -46,17 +63,43 @@ mod prepare {
         let cmd = std::process::Command::from(gix_command::prepare("ls --foo \"a b\"").arg("additional").with_shell());
         assert_eq!(
             format!("{cmd:?}"),
-            format!("\"{SH}\" \"-c\" \"ls --foo \\\"a b\\\" \\\"$@\\\"\" \"--\" \"additional\""),
+            if cfg!(windows) {
+                quoted(&["ls", "--foo", "a b", "additional"])
+            } else {
+                format!(r#""{SH}" "-c" "ls --foo \"a b\" \"$@\"" "--" "additional""#)
+            },
             "with shell, this works as it performs word splitting"
         );
     }
 
+    #[test]
+    fn single_and_complex_arguments_with_auto_split() {
+        let cmd =
+            std::process::Command::from(gix_command::prepare("ls --foo=\"a b\"").with_shell_allow_argument_splitting());
+        assert_eq!(
+            format!("{cmd:?}"),
+            format!(r#""ls" "--foo=a b""#),
+            "splitting can also handle quotes"
+        );
+    }
+
+    #[test]
+    fn single_and_complex_arguments_will_not_auto_split_on_special_characters() {
+        let cmd =
+            std::process::Command::from(gix_command::prepare("ls --foo=~/path").with_shell_allow_argument_splitting());
+        assert_eq!(
+            format!("{cmd:?}"),
+            format!(r#""{SH}" "-c" "ls --foo=~/path" "--""#),
+            "splitting can also handle quotes"
+        );
+    }
+
     #[test]
     fn tilde_path_and_multiple_arguments_as_part_of_command_with_shell() {
         let cmd = std::process::Command::from(gix_command::prepare("~/bin/exe --foo \"a b\"").with_shell());
         assert_eq!(
             format!("{cmd:?}"),
-            format!("\"{SH}\" \"-c\" \"~/bin/exe --foo \\\"a b\\\"\" \"--\""),
+            format!(r#""{SH}" "-c" "~/bin/exe --foo \"a b\"" "--""#),
             "this always needs a shell as we need tilde expansion"
         );
     }
diff --git a/gix-credentials/Cargo.toml b/gix-credentials/Cargo.toml
@@ -28,7 +28,6 @@ gix-trace = { version = "^0.1.3", path = "../gix-trace" }
 thiserror = "1.0.32"
 serde = { version = "1.0.114", optional = true, default-features = false, features = ["derive"] }
 bstr = { version = "1.3.0", default-features = false, features = ["std"]}
-shell-words = "1.0"
 
 
 
diff --git a/gix-credentials/src/program/mod.rs b/gix-credentials/src/program/mod.rs
@@ -80,24 +80,10 @@ impl Program {
                 args.insert_str(0, "credential-");
                 args.insert_str(0, " ");
                 args.insert_str(0, git_program);
-                let split_args = if args.find_byteset(b"\\|&;<>()$`\n*?[#~%").is_none() {
-                    args.to_str()
-                        .ok()
-                        .and_then(|args| shell_words::split(args).ok().map(Vec::into_iter))
-                } else {
-                    None
-                };
-                match split_args {
-                    Some(mut args) => {
-                        let mut cmd = Command::new(args.next().expect("non-empty input"));
-                        cmd.args(args).arg(action.as_arg(true));
-                        cmd
-                    }
-                    None => gix_command::prepare(gix_path::from_bstr(args.as_ref()).into_owned())
-                        .arg(action.as_arg(true))
-                        .with_shell()
-                        .into(),
-                }
+                gix_command::prepare(gix_path::from_bstr(args.as_ref()).into_owned())
+                    .arg(action.as_arg(true))
+                    .with_shell_allow_argument_splitting()
+                    .into()
             }
             Kind::ExternalShellScript(for_shell)
             | Kind::ExternalPath {
diff --git a/gix-credentials/tests/program/from_custom_definition.rs b/gix-credentials/tests/program/from_custom_definition.rs
@@ -73,7 +73,11 @@ fn path_with_args_that_definitely_need_shell() {
     assert!(matches!(&prog.kind, Kind::ExternalPath{path_and_args} if path_and_args == input));
     assert_eq!(
         format!("{:?}", prog.to_command(&helper::Action::Store("egal".into()))),
-        format!(r#""{SH}" "-c" "/abs/name --arg --bar=\"a b\" \"$@\"" "--" "store""#)
+        if cfg!(windows) {
+            r#""/abs/name" "--arg" "--bar=a b" "store""#.to_owned()
+        } else {
+            format!(r#""{SH}" "-c" "/abs/name --arg --bar=\"a b\" \"$@\"" "--" "store""#)
+        }
     );
 }
 
@@ -96,7 +100,11 @@ fn path_with_simple_args() {
     assert!(matches!(&prog.kind, Kind::ExternalPath{path_and_args} if path_and_args == input));
     assert_eq!(
         format!("{:?}", prog.to_command(&helper::Action::Store("egal".into()))),
-        format!(r#""{SH}" "-c" "/abs/name a b \"$@\"" "--" "store""#),
-        "a shell is used as well because there are arguments, and we don't do splitting ourselves. On windows, this can be a problem."
+        if cfg!(windows) {
+            r#""/abs/name" "a" "b" "store""#.to_owned()
+        } else {
+            format!(r#""{SH}" "-c" "/abs/name a b \"$@\"" "--" "store""#)
+        },
+        "a shell is used as there are arguments, and it's generally more flexible, but on windows we split ourselves"
     );
 }
diff --git a/gix-index/src/extension/link.rs b/gix-index/src/extension/link.rs
@@ -81,7 +81,7 @@ impl Link {
             .expect("split index file in .git folder")
             .join(format!("sharedindex.{}", self.shared_index_checksum));
         let mut shared_index = crate::File::at(
-            &shared_index_path,
+            shared_index_path,
             object_hash,
             skip_hash,
             crate::decode::Options {
diff --git a/gix-pack/tests/pack/index.rs b/gix-pack/tests/pack/index.rs
@@ -19,7 +19,7 @@ mod version {
         #[test]
         fn lookup() -> Result<(), Box<dyn std::error::Error>> {
             let object_hash = gix_hash::Kind::Sha1;
-            let file = index::File::at(&fixture_path(INDEX_V1), object_hash)?;
+            let file = index::File::at(fixture_path(INDEX_V1), object_hash)?;
             for (id, desired_index, assertion) in &[
                 (&b"036bd66fe9b6591e959e6df51160e636ab1a682e"[..], Some(0), "first"),
                 (b"f7f791d96b9a34ef0f08db4b007c5309b9adc3d6", Some(65), "close to last"),
@@ -63,7 +63,7 @@ mod version {
         #[test]
         fn lookup() -> Result<(), Box<dyn std::error::Error>> {
             let object_hash = gix_hash::Kind::Sha1;
-            let file = index::File::at(&fixture_path(INDEX_V2), object_hash)?;
+            let file = index::File::at(fixture_path(INDEX_V2), object_hash)?;
             for (id, expected, assertion_message, hex_len) in [
                 (&b"0ead45fc727edcf5cadca25ef922284f32bb6fc1"[..], Some(0), "first", 4),
                 (b"e800b9c207e17f9b11e321cc1fba5dfe08af4222", Some(29), "last", 40),
diff --git a/gix-packetline/src/read/async_io.rs b/gix-packetline/src/read/async_io.rs
@@ -138,7 +138,8 @@ where
         }
     }
 
-    /// Peek the next packet line without consuming it.
+    /// Peek the next packet line without consuming it. Returns `None` if a stop-packet or an error
+    /// was encountered.
     ///
     /// Multiple calls to peek will return the same packet line, if there is one.
     pub async fn peek_line(&mut self) -> Option<io::Result<Result<PacketLineRef<'_>, decode::Error>>> {
diff --git a/gix-packetline/src/read/blocking_io.rs b/gix-packetline/src/read/blocking_io.rs
@@ -132,7 +132,8 @@ where
         }
     }
 
-    /// Peek the next packet line without consuming it.
+    /// Peek the next packet line without consuming it. Returns `None` if a stop-packet or an error
+    /// was encountered.
     ///
     /// Multiple calls to peek will return the same packet line, if there is one.
     pub fn peek_line(&mut self) -> Option<io::Result<Result<PacketLineRef<'_>, decode::Error>>> {
diff --git a/gix-packetline/tests/read/mod.rs b/gix-packetline/tests/read/mod.rs
@@ -72,6 +72,28 @@ pub mod streaming_peek_iter {
         Ok(())
     }
 
+    #[maybe_async::test(feature = "blocking-io", async(feature = "async-io", async_std::test))]
+    async fn peek_eof_is_none() -> crate::Result {
+        let mut rd =
+            gix_packetline::StreamingPeekableIter::new(&b"0005a0009ERR e0000"[..], &[PacketLineRef::Flush], false);
+        rd.fail_on_err_lines(false);
+        let res = rd.peek_line().await;
+        assert_eq!(res.expect("line")??, PacketLineRef::Data(b"a"));
+        rd.read_line().await;
+        let res = rd.peek_line().await;
+        assert_eq!(
+            res.expect("line")??,
+            PacketLineRef::Data(b"ERR e"),
+            "we read the ERR but it's not interpreted as such"
+        );
+        rd.read_line().await;
+
+        let res = rd.peek_line().await;
+        assert!(res.is_none(), "we peek into the flush packet, which is EOF");
+        assert_eq!(rd.stopped_at(), Some(PacketLineRef::Flush));
+        Ok(())
+    }
+
     #[maybe_async::test(feature = "blocking-io", async(feature = "async-io", async_std::test))]
     async fn peek_non_data() -> crate::Result {
         let mut rd =
diff --git a/gix/src/remote/connection/fetch/update_refs/tests.rs b/gix/src/remote/connection/fetch/update_refs/tests.rs
@@ -191,7 +191,7 @@ mod update {
 
     #[test]
     fn checked_out_branches_in_worktrees_are_rejected_with_additional_information() -> Result {
-        let root = gix_path::realpath(&gix_testtools::scripted_fixture_read_only_with_args(
+        let root = gix_path::realpath(gix_testtools::scripted_fixture_read_only_with_args(
             "make_fetch_repos.sh",
             [base_repo_path()],
         )?)?;
diff --git a/gix/tests/repository/shallow.rs b/gix/tests/repository/shallow.rs
@@ -71,8 +71,7 @@ mod traverse {
     #[test]
     #[parallel]
     fn complex_graphs_can_be_iterated_despite_multiple_shallow_boundaries() -> crate::Result {
-        let base =
-            gix_path::realpath(&gix_testtools::scripted_fixture_read_only("make_remote_repos.sh")?.join("base"))?;
+        let base = gix_path::realpath(gix_testtools::scripted_fixture_read_only("make_remote_repos.sh")?.join("base"))?;
         let shallow_base = gix_testtools::scripted_fixture_read_only_with_args(
             "make_complex_shallow_repo.sh",
             Some(base.to_string_lossy()),

Original file line number	Diff line number	Diff line change
`@@ -82,7 +82,7 @@ pub fn entries(multi_index_path: PathBuf, format: OutputFormat, mut out: impl st`
`82`	`82`	`if format != OutputFormat::Human {`
`83`	`83`	`bail!("Only human format is supported right now");`
`84`	`84`	`}`
`85`		`- let file = gix::odb::pack::multi_index::File::at(&multi_index_path)?;`
	`85`	`+ let file = gix::odb::pack::multi_index::File::at(multi_index_path)?;`
`86`	`86`	`for entry in file.iter() {`
`87`	`87`	`writeln!(out, "{} {} {}", entry.oid, entry.pack_index, entry.pack_offset)?;`
`88`	`88`	`}`
Original file line number	Diff line number	Diff line change
`@@ -15,6 +15,7 @@ mod value {`
`15`	`15`	`}`
`16`	`16`
`17`	`17`	`#[test]`
	`18`	`+#[allow(invalid_from_utf8)]`
`18`	`19`	`fn from_value() {`
`19`	`20`	`assert!(std::str::from_utf8(ILLFORMED_UTF8).is_err());`
`20`	`21`	`assert!(`