From 8fb21d7dbac06b518943813d99388c8dbf58830e Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Wed, 30 Nov 2022 15:50:21 +0100 Subject: [PATCH 1/7] refactor - remove unwrap() in favor of returning an error - use expect() instead of unwrap() --- git-packetline/src/read/sidebands/async_io.rs | 27 ++++++++++++++----- .../src/read/sidebands/blocking_io.rs | 4 +-- 2 files changed, 22 insertions(+), 9 deletions(-) diff --git a/git-packetline/src/read/sidebands/async_io.rs b/git-packetline/src/read/sidebands/async_io.rs index 5c67c16fa78..af9918cd19a 100644 --- a/git-packetline/src/read/sidebands/async_io.rs +++ b/git-packetline/src/read/sidebands/async_io.rs @@ -29,7 +29,10 @@ where { fn drop(&mut self) { if let State::Idle { ref mut parent } = self.state { - parent.as_mut().unwrap().reset(); + parent + .as_mut() + .expect("parent is always available if we are idle") + .reset(); } } } @@ -118,14 +121,22 @@ where /// Forwards to the parent [StreamingPeekableIter::reset_with()] pub fn reset_with(&mut self, delimiters: &'static [PacketLineRef<'static>]) { if let State::Idle { ref mut parent } = self.state { - parent.as_mut().unwrap().reset_with(delimiters) + parent + .as_mut() + .expect("parent is always available if we are idle") + .reset_with(delimiters) } } /// Forwards to the parent [StreamingPeekableIter::stopped_at()] pub fn stopped_at(&self) -> Option> { match self.state { - State::Idle { ref parent } => parent.as_ref().unwrap().stopped_at, + State::Idle { ref parent } => { + parent + .as_ref() + .expect("parent is always available if we are idle") + .stopped_at + } _ => None, } } @@ -139,7 +150,12 @@ where /// next on a call to [`read_line()`][io::BufRead::read_line()]. pub async fn peek_data_line(&mut self) -> Option>> { match self.state { - State::Idle { ref mut parent } => match parent.as_mut().unwrap().peek_line().await { + State::Idle { ref mut parent } => match parent + .as_mut() + .expect("parent is always available if we are idle") + .peek_line() + .await + { Some(Ok(Ok(crate::PacketLineRef::Data(line)))) => Some(Ok(Ok(line))), Some(Ok(Err(err))) => Some(Ok(Err(err))), Some(Err(err)) => Some(Err(err)), @@ -174,8 +190,7 @@ where ); let Self { buf, parent } = &mut *self; let line = std::str::from_utf8(ready!(Pin::new(parent).poll_fill_buf(cx))?) - .map_err(|err| std::io::Error::new(std::io::ErrorKind::Other, err)) - .unwrap(); + .map_err(|err| std::io::Error::new(std::io::ErrorKind::Other, err))?; buf.clear(); buf.push_str(line); let bytes = line.len(); diff --git a/git-packetline/src/read/sidebands/blocking_io.rs b/git-packetline/src/read/sidebands/blocking_io.rs index e8a83466841..30b1ae1ea71 100644 --- a/git-packetline/src/read/sidebands/blocking_io.rs +++ b/git-packetline/src/read/sidebands/blocking_io.rs @@ -157,9 +157,7 @@ where self.cap, 0, "we don't support partial buffers right now - read-line must be used consistently" ); - let line = std::str::from_utf8(self.fill_buf()?) - .map_err(|err| io::Error::new(io::ErrorKind::Other, err)) - .unwrap(); + let line = std::str::from_utf8(self.fill_buf()?).map_err(|err| io::Error::new(io::ErrorKind::Other, err))?; buf.push_str(line); let bytes = line.len(); self.cap = 0; From 41fdb84717b825399bfaefb58e98a84a8b373cb5 Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Wed, 30 Nov 2022 17:28:47 +0100 Subject: [PATCH 2/7] feat: `WithSidebands` now offers a `read_data_line(byte_buf)` method. That way one won't have to assume UTF8 encoding in the returned buffer. Note that the reason for it not returning a reference to its internal buffer is due to the async implementation requiring it. Its future-based architecture can't really express the lifetimes associated with it (yet). --- git-packetline/src/line/mod.rs | 18 +++--- git-packetline/src/read/sidebands/async_io.rs | 55 ++++++++++++++++++- .../src/read/sidebands/blocking_io.rs | 19 ++++++- git-packetline/tests/read/sideband.rs | 46 ++++++++++++++++ 4 files changed, 125 insertions(+), 13 deletions(-) diff --git a/git-packetline/src/line/mod.rs b/git-packetline/src/line/mod.rs index e3777812807..538630ecc7a 100644 --- a/git-packetline/src/line/mod.rs +++ b/git-packetline/src/line/mod.rs @@ -4,14 +4,14 @@ use crate::{decode, BandRef, Channel, ErrorRef, PacketLineRef, TextRef, ERR_PREF impl<'a> PacketLineRef<'a> { /// Return this instance as slice if it's [`Data`][PacketLineRef::Data]. - pub fn as_slice(&self) -> Option<&[u8]> { + pub fn as_slice(&self) -> Option<&'a [u8]> { match self { PacketLineRef::Data(d) => Some(d), PacketLineRef::Flush | PacketLineRef::Delimiter | PacketLineRef::ResponseEnd => None, } } /// Return this instance's [`as_slice()`][PacketLineRef::as_slice()] as [`BStr`]. - pub fn as_bstr(&self) -> Option<&BStr> { + pub fn as_bstr(&self) -> Option<&'a BStr> { self.as_slice().map(Into::into) } /// Interpret this instance's [`as_slice()`][PacketLineRef::as_slice()] as [`ErrorRef`]. @@ -20,13 +20,13 @@ impl<'a> PacketLineRef<'a> { /// /// Note that this creates an unchecked error using the slice verbatim, which is useful to [serialize it][ErrorRef::write_to()]. /// See [`check_error()`][PacketLineRef::check_error()] for a version that assures the error information is in the expected format. - pub fn as_error(&self) -> Option> { + pub fn as_error(&self) -> Option> { self.as_slice().map(ErrorRef) } /// Check this instance's [`as_slice()`][PacketLineRef::as_slice()] is a valid [`ErrorRef`] and return it. /// /// This works for any data received in an error [channel][crate::Channel]. - pub fn check_error(&self) -> Option> { + pub fn check_error(&self) -> Option> { self.as_slice().and_then(|data| { if data.len() >= ERR_PREFIX.len() && &data[..ERR_PREFIX.len()] == ERR_PREFIX { Some(ErrorRef(&data[ERR_PREFIX.len()..])) @@ -36,7 +36,7 @@ impl<'a> PacketLineRef<'a> { }) } /// Return this instance as text, with the trailing newline truncated if present. - pub fn as_text(&self) -> Option> { + pub fn as_text(&self) -> Option> { self.as_slice().map(Into::into) } @@ -44,7 +44,7 @@ impl<'a> PacketLineRef<'a> { /// /// Note that this is only relevant in a side-band channel. /// See [`decode_band()`][PacketLineRef::decode_band()] in case `kind` is unknown. - pub fn as_band(&self, kind: Channel) -> Option> { + pub fn as_band(&self, kind: Channel) -> Option> { self.as_slice().map(|d| match kind { Channel::Data => BandRef::Data(d), Channel::Progress => BandRef::Progress(d), @@ -53,7 +53,7 @@ impl<'a> PacketLineRef<'a> { } /// Decode the band of this [`slice`][PacketLineRef::as_slice()] - pub fn decode_band(&self) -> Result, decode::band::Error> { + pub fn decode_band(&self) -> Result, decode::band::Error> { let d = self.as_slice().ok_or(decode::band::Error::NonDataLine)?; Ok(match d[0] { 1 => BandRef::Data(&d[1..]), @@ -73,11 +73,11 @@ impl<'a> From<&'a [u8]> for TextRef<'a> { impl<'a> TextRef<'a> { /// Return this instance's data. - pub fn as_slice(&self) -> &[u8] { + pub fn as_slice(&self) -> &'a [u8] { self.0 } /// Return this instance's data as [`BStr`]. - pub fn as_bstr(&self) -> &BStr { + pub fn as_bstr(&self) -> &'a BStr { self.0.into() } } diff --git a/git-packetline/src/read/sidebands/async_io.rs b/git-packetline/src/read/sidebands/async_io.rs index af9918cd19a..c8d009ec4a1 100644 --- a/git-packetline/src/read/sidebands/async_io.rs +++ b/git-packetline/src/read/sidebands/async_io.rs @@ -148,7 +148,11 @@ where /// Effectively forwards to the parent [StreamingPeekableIter::peek_line()], allowing to see what would be returned /// next on a call to [`read_line()`][io::BufRead::read_line()]. - pub async fn peek_data_line(&mut self) -> Option>> { + /// + /// # Warning + /// + /// This skips all sideband handling and may return an unprocessed line with sidebands still contained in it. + pub async fn peek_data_line(&mut self) -> Option>> { match self.state { State::Idle { ref mut parent } => match parent .as_mut() @@ -156,7 +160,7 @@ where .peek_line() .await { - Some(Ok(Ok(crate::PacketLineRef::Data(line)))) => Some(Ok(Ok(line))), + Some(Ok(Ok(PacketLineRef::Data(line)))) => Some(Ok(Ok(line))), Some(Ok(Err(err))) => Some(Ok(Err(err))), Some(Err(err)) => Some(Err(err)), _ => None, @@ -165,10 +169,55 @@ where } } - /// Read a packet line as line. + /// Read a packet line as string line. pub fn read_line<'b>(&'b mut self, buf: &'b mut String) -> ReadLineFuture<'a, 'b, T, F> { ReadLineFuture { parent: self, buf } } + + /// Read a packet line from the underlying packet reader, returning empty lines if a stop-packetline was reached. + /// + /// # Warning + /// + /// This skips all sideband handling and may return an unprocessed line with sidebands still contained in it. + pub async fn read_data_line(&mut self) -> Option, decode::Error>>> { + match &mut self.state { + State::Idle { parent: Some(parent) } => { + assert_eq!( + self.cap, 0, + "we don't support partial buffers right now - read-line must be used consistently" + ); + parent.read_line().await + } + _ => None, + } + } +} + +pub struct ReadDataLineFuture<'a, 'b, T: AsyncRead, F> { + parent: &'b mut WithSidebands<'a, T, F>, + buf: &'b mut Vec, +} + +impl<'a, 'b, T, F> Future for ReadDataLineFuture<'a, 'b, T, F> +where + T: AsyncRead + Unpin, + F: FnMut(bool, &[u8]) + Unpin, +{ + type Output = std::io::Result; + + fn poll(mut self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll { + assert_eq!( + self.parent.cap, 0, + "we don't support partial buffers right now - read-line must be used consistently" + ); + let Self { buf, parent } = &mut *self; + let line = ready!(Pin::new(parent).poll_fill_buf(cx))?; + buf.clear(); + buf.extend_from_slice(line); + let bytes = line.len(); + self.parent.cap = 0; + Poll::Ready(Ok(bytes)) + } } pub struct ReadLineFuture<'a, 'b, T: AsyncRead, F> { diff --git a/git-packetline/src/read/sidebands/blocking_io.rs b/git-packetline/src/read/sidebands/blocking_io.rs index 30b1ae1ea71..ea67dc09cef 100644 --- a/git-packetline/src/read/sidebands/blocking_io.rs +++ b/git-packetline/src/read/sidebands/blocking_io.rs @@ -84,14 +84,31 @@ where /// Effectively forwards to the parent [StreamingPeekableIter::peek_line()], allowing to see what would be returned /// next on a call to [`read_line()`][io::BufRead::read_line()]. + /// + /// # Warning + /// + /// This skips all sideband handling and may return an unprocessed line with sidebands still contained in it. pub fn peek_data_line(&mut self) -> Option>> { match self.parent.peek_line() { - Some(Ok(Ok(crate::PacketLineRef::Data(line)))) => Some(Ok(Ok(line))), + Some(Ok(Ok(PacketLineRef::Data(line)))) => Some(Ok(Ok(line))), Some(Ok(Err(err))) => Some(Ok(Err(err))), Some(Err(err)) => Some(Err(err)), _ => None, } } + + /// Read a whole packetline from the underlying reader, with empty lines indicating a stop packetline. + /// + /// # Warning + /// + /// This skips all sideband handling and may return an unprocessed line with sidebands still contained in it. + pub fn read_data_line(&mut self) -> Option, crate::decode::Error>>> { + assert_eq!( + self.cap, 0, + "we don't support partial buffers right now - read-line must be used consistently" + ); + self.parent.read_line() + } } impl<'a, T, F> BufRead for WithSidebands<'a, T, F> diff --git a/git-packetline/tests/read/sideband.rs b/git-packetline/tests/read/sideband.rs index 5484b598216..6762ced03d2 100644 --- a/git-packetline/tests/read/sideband.rs +++ b/git-packetline/tests/read/sideband.rs @@ -144,6 +144,52 @@ async fn read_line_trait_method_reads_one_packet_line_at_a_time() -> crate::Resu Ok(()) } +#[maybe_async::test(feature = "blocking-io", async(feature = "async-io", async_std::test))] +async fn readline_reads_one_packet_line_at_a_time() -> crate::Result { + let buf = fixture_bytes("v1/01-clone.combined-output-no-binary"); + + let mut rd = git_packetline::StreamingPeekableIter::new(&buf[..], &[PacketLineRef::Flush]); + + let mut r = rd.as_read(); + let line = r.read_data_line().await.unwrap()??.as_bstr().unwrap(); + assert_eq!(line, "808e50d724f604f69ab93c6da2919c014667bedb HEAD\0multi_ack thin-pack side-band side-band-64k ofs-delta shallow deepen-since deepen-not deepen-relative no-progress include-tag multi_ack_detailed symref=HEAD:refs/heads/master object-format=sha1 agent=git/2.28.0\n"); + let line = r.read_data_line().await.unwrap()??.as_bstr().unwrap(); + assert_eq!(line, "808e50d724f604f69ab93c6da2919c014667bedb refs/heads/master\n"); + let line = r.read_data_line().await; + assert!(line.is_none(), "flush means `None`"); + let line = r.read_data_line().await; + assert!(line.is_none(), "…which can't be overcome unless the reader is reset"); + assert_eq!( + r.stopped_at(), + Some(PacketLineRef::Flush), + "it knows what stopped the reader" + ); + + drop(r); + rd.reset(); + + let mut r = rd.as_read(); + let line = r.read_data_line().await.unwrap()??.as_bstr().unwrap(); + assert_eq!(line.as_bstr(), "NAK\n"); + + drop(r); + + let mut r = rd.as_read_with_sidebands(|_, _| ()); + let line = r.read_data_line().await.unwrap()??.as_bstr().unwrap(); + assert_eq!( + line.as_bstr(), + "\x02Enumerating objects: 3, done.\n", + "sidebands are ignored entirely here" + ); + for _ in 0..6 { + let _discard_more_progress = r.read_data_line().await.unwrap()??.as_bstr().unwrap(); + } + let line = r.read_data_line().await; + assert!(line.is_none(), "and we have reached the end"); + + Ok(()) +} + #[maybe_async::test(feature = "blocking-io", async(feature = "async-io", async_std::test))] async fn peek_past_an_actual_eof_is_an_error() -> crate::Result { let input = b"0009ERR e"; From 08dcda254bc942dcc36d432cf7130c2ce4c6d54e Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Wed, 30 Nov 2022 11:10:54 +0100 Subject: [PATCH 3/7] fix!: `Capabiltiies::from_lines()` takes a single buffer. That way it doesn't have to convert to `String` as intermediary which may fail as illformed UTF8 might be present. Performance wise, reading all lines ahead of time, it is the same as it was before as it would collect all lines beforehand anyway. We are also seeing only a few lines in V2, and in V1 it was fully streaming already. --- git-transport/Cargo.toml | 2 +- git-transport/src/client/capabilities.rs | 54 +++++++++++------------- 2 files changed, 26 insertions(+), 30 deletions(-) diff --git a/git-transport/Cargo.toml b/git-transport/Cargo.toml index 9b2f86e4d27..8847012fc81 100644 --- a/git-transport/Cargo.toml +++ b/git-transport/Cargo.toml @@ -59,7 +59,7 @@ git-packetline = { version = "^0.14.0", path = "../git-packetline" } git-credentials = { version = "^0.7.0", path = "../git-credentials", optional = true } serde = { version = "1.0.114", optional = true, default-features = false, features = ["std", "derive"]} -bstr = { version = "1.0.1", default-features = false, features = ["std"] } +bstr = { version = "1.0.1", default-features = false, features = ["std", "unicode"] } thiserror = "1.0.26" # for async-client diff --git a/git-transport/src/client/capabilities.rs b/git-transport/src/client/capabilities.rs index 3f1c2e93922..f4abad4588a 100644 --- a/git-transport/src/client/capabilities.rs +++ b/git-transport/src/client/capabilities.rs @@ -15,9 +15,9 @@ pub enum Error { #[error("a version line was expected, but none was retrieved")] MissingVersionLine, #[error("expected 'version X', got {0:?}")] - MalformattedVersionLine(String), + MalformattedVersionLine(BString), #[error("Got unsupported version '{}', expected {actual:?}", *desired as u8)] - UnsupportedVersion { desired: Protocol, actual: String }, + UnsupportedVersion { desired: Protocol, actual: BString }, #[error("An IO error occurred while reading V2 lines")] Io(#[from] std::io::Error), } @@ -81,34 +81,32 @@ impl Capabilities { )) } - /// Parse capabilities from the given a `first_line` and the rest of the lines as single newline - /// separated string via `remaining_lines`. + /// Parse capabilities from the given a `lines_buf` which is expected to be all newline separated lines + /// from the server. /// /// Useful for parsing capabilities from a data sent from a server, and to avoid having to deal with /// blocking and async traits for as long as possible. There is no value in parsing a few bytes /// in a non-blocking fashion. - pub fn from_lines( - first_line: Option>>, - remaining_lines: impl Into, - ) -> Result { - let version_line = first_line.map(Into::into).ok_or(Error::MissingVersionLine)??; + pub fn from_lines(lines_buf: BString) -> Result { + let mut lines = <_ as bstr::ByteSlice>::lines(lines_buf.as_slice().trim()); + let version_line = lines.next().ok_or(Error::MissingVersionLine)?; let (name, value) = version_line.split_at( version_line - .find(' ') - .ok_or_else(|| Error::MalformattedVersionLine(version_line.clone()))?, + .find(b" ") + .ok_or_else(|| Error::MalformattedVersionLine(version_line.to_owned().into()))?, ); - if name != "version" { - return Err(Error::MalformattedVersionLine(version_line)); + if name != b"version" { + return Err(Error::MalformattedVersionLine(version_line.to_owned().into())); } - if value != " 2" { + if value != b" 2" { return Err(Error::UnsupportedVersion { desired: Protocol::V2, - actual: value.to_owned(), + actual: value.to_owned().into(), }); } Ok(Capabilities { value_sep: b'\n', - data: remaining_lines.into().into(), + data: lines.as_bytes().into(), }) } @@ -154,7 +152,8 @@ impl Capabilities { #[cfg(feature = "blocking-client")] /// pub mod recv { - use std::{io, io::BufRead}; + use std::io; + use std::io::Read; use crate::{client, client::Capabilities, Protocol}; @@ -203,9 +202,10 @@ pub mod recv { } Protocol::V2 => Ok(Outcome { capabilities: { - let rd = rd.as_read(); - let mut lines = rd.lines(); - Capabilities::from_lines(lines.next(), lines.collect::, _>>()?.join("\n"))? + let mut rd = rd.as_read(); + let mut buf = Vec::new(); + rd.read_to_end(&mut buf)?; + Capabilities::from_lines(buf.into())? }, refs: None, protocol: Protocol::V2, @@ -220,7 +220,7 @@ pub mod recv { /// pub mod recv { use futures_io::{AsyncBufRead, AsyncRead}; - use futures_lite::{AsyncBufReadExt, StreamExt}; + use futures_lite::AsyncReadExt; use crate::{client, client::Capabilities, Protocol}; @@ -270,14 +270,10 @@ pub mod recv { } Protocol::V2 => Ok(Outcome { capabilities: { - let rd = rd.as_read(); - let mut lines_with_err = rd.lines(); - let mut lines = Vec::new(); - while let Some(line) = lines_with_err.next().await { - lines.push(line?); - } - let mut lines = lines.into_iter(); - Capabilities::from_lines(lines.next().map(Ok), lines.collect::>().join("\n"))? + let mut rd = rd.as_read(); + let mut buf = Vec::new(); + rd.read_to_end(&mut buf).await?; + Capabilities::from_lines(buf.into())? }, refs: None, protocol: Protocol::V2, From 49461b1ca92e301734c090220936941fca57db0e Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Wed, 30 Nov 2022 11:13:24 +0100 Subject: [PATCH 4/7] adapt to changes in `git-transport` --- git-protocol/src/command/tests.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/git-protocol/src/command/tests.rs b/git-protocol/src/command/tests.rs index b921fda2eac..68a93afb7e9 100644 --- a/git-protocol/src/command/tests.rs +++ b/git-protocol/src/command/tests.rs @@ -53,7 +53,7 @@ mod v2 { use git_transport::client::Capabilities; fn capabilities(command: &str, input: &str) -> Capabilities { - Capabilities::from_lines(Some(Ok("version 2".into())), format!("{}={}", command, input)) + Capabilities::from_lines(format!("version 2\n{}={}", command, input).into()) .expect("valid input for V2 capabilities") } From 1204bfcaadd31ed198b923df05f19115da3754a4 Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Wed, 30 Nov 2022 15:28:13 +0100 Subject: [PATCH 5/7] feat!: Provide support for reading packetlines directly. The handshake response itself now provides a `ref` read implementation with direct readline support. That way one can avoid having to go through `String`. --- .../src/client/async_io/bufread_ext.rs | 47 +++++++++++++++++-- git-transport/src/client/async_io/mod.rs | 2 +- git-transport/src/client/async_io/traits.rs | 2 +- .../src/client/blocking_io/bufread_ext.rs | 39 ++++++++++++++- .../src/client/blocking_io/http/mod.rs | 11 ++++- git-transport/src/client/blocking_io/mod.rs | 2 +- .../src/client/blocking_io/traits.rs | 4 +- git-transport/src/client/capabilities.rs | 6 +-- git-transport/src/client/mod.rs | 6 ++- 9 files changed, 101 insertions(+), 18 deletions(-) diff --git a/git-transport/src/client/async_io/bufread_ext.rs b/git-transport/src/client/async_io/bufread_ext.rs index b681be8a564..12993fe776c 100644 --- a/git-transport/src/client/async_io/bufread_ext.rs +++ b/git-transport/src/client/async_io/bufread_ext.rs @@ -5,6 +5,7 @@ use std::{ use async_trait::async_trait; use futures_io::{AsyncBufRead, AsyncRead}; +use git_packetline::PacketLineRef; use crate::{ client::{Error, MessageKind}, @@ -16,11 +17,28 @@ use crate::{ /// it onto an executor. pub type HandleProgress = Box; -/// This trait exists to get a version of a `git_packetline::Provider` without type parameters. -/// For the sake of usability, it also implements [`std::io::BufRead`] making it trivial to (eventually) -/// read pack files while keeping the possibility to read individual lines with low overhead. +/// This trait exists to get a version of a `git_packetline::Provider` without type parameters, +/// but leave support for reading lines directly without forcing them through `String`. +/// +/// For the sake of usability, it also implements [`std::io::BufRead`] making it trivial to +/// read pack files while keeping open the option to read individual lines with low overhead. #[async_trait(?Send)] -pub trait ExtendedBufRead: AsyncBufRead { +pub trait ReadlineBufRead: AsyncBufRead { + /// Read a packet line into the internal buffer and return it. + /// + /// Returns `None` if the end of iteration is reached because of one of the following: + /// + /// * natural EOF + /// * ERR packet line encountered + /// * A `delimiter` packet line encountered + async fn readline( + &mut self, + ) -> Option, git_packetline::decode::Error>>>; +} + +/// Provide even more access to the underlying packet reader. +#[async_trait(?Send)] +pub trait ExtendedBufRead: ReadlineBufRead { /// Set the handler to which progress will be delivered. /// /// Note that this is only possible if packet lines are sent in side band mode. @@ -35,6 +53,13 @@ pub trait ExtendedBufRead: AsyncBufRead { fn stopped_at(&self) -> Option; } +#[async_trait(?Send)] +impl<'a, T: ReadlineBufRead + ?Sized + 'a + Unpin> ReadlineBufRead for Box { + async fn readline(&mut self) -> Option, git_packetline::decode::Error>>> { + self.deref_mut().readline().await + } +} + #[async_trait(?Send)] impl<'a, T: ExtendedBufRead + ?Sized + 'a + Unpin> ExtendedBufRead for Box { fn set_progress_handler(&mut self, handle_progress: Option) { @@ -54,6 +79,20 @@ impl<'a, T: ExtendedBufRead + ?Sized + 'a + Unpin> ExtendedBufRead for Box { } } +#[async_trait(?Send)] +impl ReadlineBufRead for git_packetline::read::WithSidebands<'_, T, for<'b> fn(bool, &'b [u8])> { + async fn readline(&mut self) -> Option, git_packetline::decode::Error>>> { + self.readline().await + } +} + +#[async_trait(?Send)] +impl<'a, T: AsyncRead + Unpin> ReadlineBufRead for git_packetline::read::WithSidebands<'a, T, HandleProgress> { + async fn readline(&mut self) -> Option, git_packetline::decode::Error>>> { + self.readline().await + } +} + #[async_trait(?Send)] impl<'a, T: AsyncRead + Unpin> ExtendedBufRead for git_packetline::read::WithSidebands<'a, T, HandleProgress> { fn set_progress_handler(&mut self, handle_progress: Option) { diff --git a/git-transport/src/client/async_io/mod.rs b/git-transport/src/client/async_io/mod.rs index 2b12f2da802..6cb1a500e18 100644 --- a/git-transport/src/client/async_io/mod.rs +++ b/git-transport/src/client/async_io/mod.rs @@ -1,5 +1,5 @@ mod bufread_ext; -pub use bufread_ext::{ExtendedBufRead, HandleProgress}; +pub use bufread_ext::{ExtendedBufRead, HandleProgress, ReadlineBufRead}; mod request; pub use request::RequestWriter; diff --git a/git-transport/src/client/async_io/traits.rs b/git-transport/src/client/async_io/traits.rs index a4e818412c1..e64ab0f4920 100644 --- a/git-transport/src/client/async_io/traits.rs +++ b/git-transport/src/client/async_io/traits.rs @@ -16,7 +16,7 @@ pub struct SetServiceResponse<'a> { /// The capabilities parsed from the server response. pub capabilities: Capabilities, /// In protocol version one, this is set to a list of refs and their peeled counterparts. - pub refs: Option>, + pub refs: Option>, } /// All methods provided here must be called in the correct order according to the [communication protocol][Protocol] diff --git a/git-transport/src/client/blocking_io/bufread_ext.rs b/git-transport/src/client/blocking_io/bufread_ext.rs index e894d51374f..1e6386cb2af 100644 --- a/git-transport/src/client/blocking_io/bufread_ext.rs +++ b/git-transport/src/client/blocking_io/bufread_ext.rs @@ -1,3 +1,4 @@ +use git_packetline::PacketLineRef; use std::{ io, ops::{Deref, DerefMut}, @@ -10,10 +11,26 @@ use crate::{ /// A function `f(is_error, text)` receiving progress or error information. pub type HandleProgress = Box; -/// This trait exists to get a version of a `git_packetline::Provider` without type parameters. +/// This trait exists to get a version of a `git_packetline::Provider` without type parameters, +/// but leave support for reading lines directly without forcing them through `String`. +/// /// For the sake of usability, it also implements [`std::io::BufRead`] making it trivial to /// read pack files while keeping open the option to read individual lines with low overhead. -pub trait ExtendedBufRead: io::BufRead { +pub trait ReadlineBufRead: io::BufRead { + /// Read a packet line into the internal buffer and return it. + /// + /// Returns `None` if the end of iteration is reached because of one of the following: + /// + /// * natural EOF + /// * ERR packet line encountered + /// * A `delimiter` packet line encountered + fn readline( + &mut self, + ) -> Option, git_packetline::decode::Error>>>; +} + +/// Provide even more access to the underlying packet reader. +pub trait ExtendedBufRead: ReadlineBufRead { /// Set the handler to which progress will be delivered. /// /// Note that this is only possible if packet lines are sent in side band mode. @@ -28,6 +45,12 @@ pub trait ExtendedBufRead: io::BufRead { fn stopped_at(&self) -> Option; } +impl<'a, T: ReadlineBufRead + ?Sized + 'a> ReadlineBufRead for Box { + fn readline(&mut self) -> Option, git_packetline::decode::Error>>> { + ReadlineBufRead::readline(self.deref_mut()) + } +} + impl<'a, T: ExtendedBufRead + ?Sized + 'a> ExtendedBufRead for Box { fn set_progress_handler(&mut self, handle_progress: Option) { self.deref_mut().set_progress_handler(handle_progress) @@ -46,6 +69,18 @@ impl<'a, T: ExtendedBufRead + ?Sized + 'a> ExtendedBufRead for Box { } } +impl ReadlineBufRead for git_packetline::read::WithSidebands<'_, T, fn(bool, &[u8])> { + fn readline(&mut self) -> Option, git_packetline::decode::Error>>> { + self.read_data_line() + } +} + +impl<'a, T: io::Read> ReadlineBufRead for git_packetline::read::WithSidebands<'a, T, HandleProgress> { + fn readline(&mut self) -> Option, git_packetline::decode::Error>>> { + self.read_data_line() + } +} + impl<'a, T: io::Read> ExtendedBufRead for git_packetline::read::WithSidebands<'a, T, HandleProgress> { fn set_progress_handler(&mut self, handle_progress: Option) { self.set_progress_handler(handle_progress) diff --git a/git-transport/src/client/blocking_io/http/mod.rs b/git-transport/src/client/blocking_io/http/mod.rs index a75dcccc50c..b4a8e0b449d 100644 --- a/git-transport/src/client/blocking_io/http/mod.rs +++ b/git-transport/src/client/blocking_io/http/mod.rs @@ -9,6 +9,7 @@ use bstr::BStr; use git_packetline::PacketLineRef; pub use traits::{Error, GetResponse, Http, PostResponse}; +use crate::client::blocking_io::bufread_ext::ReadlineBufRead; use crate::{ client::{self, capabilities, Capabilities, ExtendedBufRead, HandleProgress, MessageKind, RequestWriter}, Protocol, Service, @@ -387,14 +388,14 @@ impl HeadersThenBody { } } -impl Read for HeadersThenBody { +impl Read for HeadersThenBody { fn read(&mut self, buf: &mut [u8]) -> std::io::Result { self.handle_headers()?; self.body.read(buf) } } -impl BufRead for HeadersThenBody { +impl BufRead for HeadersThenBody { fn fill_buf(&mut self) -> std::io::Result<&[u8]> { self.handle_headers()?; self.body.fill_buf() @@ -405,6 +406,12 @@ impl BufRead for HeadersThenBody { } } +impl ReadlineBufRead for HeadersThenBody { + fn readline(&mut self) -> Option, git_packetline::decode::Error>>> { + self.body.readline() + } +} + impl ExtendedBufRead for HeadersThenBody { fn set_progress_handler(&mut self, handle_progress: Option) { self.body.set_progress_handler(handle_progress) diff --git a/git-transport/src/client/blocking_io/mod.rs b/git-transport/src/client/blocking_io/mod.rs index 48f4073cf3a..dfb3752af95 100644 --- a/git-transport/src/client/blocking_io/mod.rs +++ b/git-transport/src/client/blocking_io/mod.rs @@ -8,7 +8,7 @@ pub mod file; pub mod http; mod bufread_ext; -pub use bufread_ext::{ExtendedBufRead, HandleProgress}; +pub use bufread_ext::{ExtendedBufRead, HandleProgress, ReadlineBufRead}; mod request; pub use request::RequestWriter; diff --git a/git-transport/src/client/blocking_io/traits.rs b/git-transport/src/client/blocking_io/traits.rs index 0b1bae1cc1b..d0c91175b61 100644 --- a/git-transport/src/client/blocking_io/traits.rs +++ b/git-transport/src/client/blocking_io/traits.rs @@ -1,4 +1,4 @@ -use std::{io, io::Write, ops::DerefMut}; +use std::{io::Write, ops::DerefMut}; use bstr::BString; @@ -14,7 +14,7 @@ pub struct SetServiceResponse<'a> { /// The capabilities parsed from the server response. pub capabilities: Capabilities, /// In protocol version one, this is set to a list of refs and their peeled counterparts. - pub refs: Option>, + pub refs: Option>, } /// All methods provided here must be called in the correct order according to the [communication protocol][Protocol] diff --git a/git-transport/src/client/capabilities.rs b/git-transport/src/client/capabilities.rs index f4abad4588a..89b60a96dbb 100644 --- a/git-transport/src/client/capabilities.rs +++ b/git-transport/src/client/capabilities.rs @@ -165,7 +165,7 @@ pub mod recv { /// /// This is `Some` only when protocol v1 is used. The [`io::BufRead`] must be exhausted by /// the caller. - pub refs: Option>, + pub refs: Option>, /// The [`Protocol`] the remote advertised. pub protocol: Protocol, } @@ -219,7 +219,7 @@ pub mod recv { #[allow(missing_docs)] /// pub mod recv { - use futures_io::{AsyncBufRead, AsyncRead}; + use futures_io::AsyncRead; use futures_lite::AsyncReadExt; use crate::{client, client::Capabilities, Protocol}; @@ -232,7 +232,7 @@ pub mod recv { /// /// This is `Some` only when protocol v1 is used. The [`AsyncBufRead`] must be exhausted by /// the caller. - pub refs: Option>, + pub refs: Option>, /// The [`Protocol`] the remote advertised. pub protocol: Protocol, } diff --git a/git-transport/src/client/mod.rs b/git-transport/src/client/mod.rs index 9285167387c..0eeb6f145d6 100644 --- a/git-transport/src/client/mod.rs +++ b/git-transport/src/client/mod.rs @@ -2,7 +2,8 @@ mod async_io; #[cfg(feature = "async-client")] pub use async_io::{ - connect, ExtendedBufRead, HandleProgress, RequestWriter, SetServiceResponse, Transport, TransportV2Ext, + connect, ExtendedBufRead, HandleProgress, ReadlineBufRead, RequestWriter, SetServiceResponse, Transport, + TransportV2Ext, }; mod traits; @@ -14,7 +15,8 @@ mod blocking_io; pub use blocking_io::http; #[cfg(feature = "blocking-client")] pub use blocking_io::{ - connect, file, ssh, ExtendedBufRead, HandleProgress, RequestWriter, SetServiceResponse, Transport, TransportV2Ext, + connect, file, ssh, ExtendedBufRead, HandleProgress, ReadlineBufRead, RequestWriter, SetServiceResponse, Transport, + TransportV2Ext, }; #[cfg(feature = "blocking-client")] #[doc(inline)] From 806b8c2ef392137f3a6ebd0f28da2a3a07a9f3eb Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Wed, 30 Nov 2022 11:16:34 +0100 Subject: [PATCH 6/7] fix: Parse refs from bytes, not from String. The latter can cause issues around illformed UTF-8 which wouldn't bother git either. This comes at the expense of not parsing line by line anymore, but instead reading as fast as possible and parsing afterwards. Performance wise I think it doesn't matter, but it will cause more memory to be used. If this ever becomes a problem, for example during pushes where we are stuck with V1, we can consider implementing our own streaming appreach that works with packet lines instead - they are just not exposed here even though they could. --- git-protocol/Cargo.toml | 2 +- git-protocol/src/fetch/tests.rs | 344 ++++++++++++++++++ git-protocol/src/fetch/tests/arguments.rs | 333 ----------------- git-protocol/src/fetch/tests/mod.rs | 4 - git-protocol/src/handshake/refs/async_io.rs | 29 +- .../src/handshake/refs/blocking_io.rs | 27 +- git-protocol/src/handshake/refs/mod.rs | 11 +- git-protocol/src/handshake/refs/shared.rs | 42 +-- .../tests/refs.rs => handshake/refs/tests.rs} | 42 ++- 9 files changed, 434 insertions(+), 400 deletions(-) create mode 100644 git-protocol/src/fetch/tests.rs delete mode 100644 git-protocol/src/fetch/tests/arguments.rs delete mode 100644 git-protocol/src/fetch/tests/mod.rs rename git-protocol/src/{fetch/tests/refs.rs => handshake/refs/tests.rs} (80%) diff --git a/git-protocol/Cargo.toml b/git-protocol/Cargo.toml index 295ead89660..445da74e67f 100644 --- a/git-protocol/Cargo.toml +++ b/git-protocol/Cargo.toml @@ -46,7 +46,7 @@ git-credentials = { version = "^0.7.0", path = "../git-credentials" } thiserror = "1.0.32" serde = { version = "1.0.114", optional = true, default-features = false, features = ["derive"]} -bstr = { version = "1.0.1", default-features = false, features = ["std"] } +bstr = { version = "1.0.1", default-features = false, features = ["std", "unicode"] } nom = { version = "7", default-features = false, features = ["std"]} btoi = "0.4.2" diff --git a/git-protocol/src/fetch/tests.rs b/git-protocol/src/fetch/tests.rs new file mode 100644 index 00000000000..bc142bbfdd8 --- /dev/null +++ b/git-protocol/src/fetch/tests.rs @@ -0,0 +1,344 @@ +#[cfg(any(feature = "async-client", feature = "blocking-client"))] +mod arguments { + use bstr::ByteSlice; + use git_transport::Protocol; + + use crate::fetch; + + fn arguments_v1(features: impl IntoIterator) -> fetch::Arguments { + fetch::Arguments::new(Protocol::V1, features.into_iter().map(|n| (n, None)).collect()) + } + + fn arguments_v2(features: impl IntoIterator) -> fetch::Arguments { + fetch::Arguments::new(Protocol::V2, features.into_iter().map(|n| (n, None)).collect()) + } + + struct Transport { + inner: T, + stateful: bool, + } + + #[cfg(feature = "blocking-client")] + mod impls { + use std::borrow::Cow; + + use bstr::BStr; + use git_transport::{ + client, + client::{Error, MessageKind, RequestWriter, SetServiceResponse, WriteMode}, + Protocol, Service, + }; + + use crate::fetch::tests::arguments::Transport; + + impl client::TransportWithoutIO for Transport { + fn set_identity(&mut self, identity: client::Account) -> Result<(), Error> { + self.inner.set_identity(identity) + } + + fn request( + &mut self, + write_mode: WriteMode, + on_into_read: MessageKind, + ) -> Result, Error> { + self.inner.request(write_mode, on_into_read) + } + + fn to_url(&self) -> Cow<'_, BStr> { + self.inner.to_url() + } + + fn supported_protocol_versions(&self) -> &[Protocol] { + self.inner.supported_protocol_versions() + } + + fn connection_persists_across_multiple_requests(&self) -> bool { + self.stateful + } + + fn configure( + &mut self, + config: &dyn std::any::Any, + ) -> Result<(), Box> { + self.inner.configure(config) + } + } + + impl client::Transport for Transport { + fn handshake<'a>( + &mut self, + service: Service, + extra_parameters: &'a [(&'a str, Option<&'a str>)], + ) -> Result, Error> { + self.inner.handshake(service, extra_parameters) + } + } + } + + #[cfg(feature = "async-client")] + mod impls { + use std::borrow::Cow; + + use async_trait::async_trait; + use bstr::BStr; + use git_transport::{ + client, + client::{Error, MessageKind, RequestWriter, SetServiceResponse, WriteMode}, + Protocol, Service, + }; + + use crate::fetch::tests::arguments::Transport; + impl client::TransportWithoutIO for Transport { + fn set_identity(&mut self, identity: client::Account) -> Result<(), Error> { + self.inner.set_identity(identity) + } + + fn request( + &mut self, + write_mode: WriteMode, + on_into_read: MessageKind, + ) -> Result, Error> { + self.inner.request(write_mode, on_into_read) + } + + fn to_url(&self) -> Cow<'_, BStr> { + self.inner.to_url() + } + + fn supported_protocol_versions(&self) -> &[Protocol] { + self.inner.supported_protocol_versions() + } + + fn connection_persists_across_multiple_requests(&self) -> bool { + self.stateful + } + + fn configure( + &mut self, + config: &dyn std::any::Any, + ) -> Result<(), Box> { + self.inner.configure(config) + } + } + + #[async_trait(?Send)] + impl client::Transport for Transport { + async fn handshake<'a>( + &mut self, + service: Service, + extra_parameters: &'a [(&'a str, Option<&'a str>)], + ) -> Result, Error> { + self.inner.handshake(service, extra_parameters).await + } + } + } + + fn transport( + out: &mut Vec, + stateful: bool, + ) -> Transport>> { + Transport { + inner: git_transport::client::git::Connection::new( + &[], + out, + Protocol::V1, // does not matter + b"does/not/matter".as_bstr().to_owned(), + None::<(&str, _)>, + git_transport::client::git::ConnectMode::Process, // avoid header to be sent + ), + stateful, + } + } + + fn id(hex: &str) -> git_hash::ObjectId { + git_hash::ObjectId::from_hex(hex.as_bytes()).expect("expect valid hex id") + } + + mod v1 { + use bstr::ByteSlice; + + use crate::fetch::tests::arguments::{arguments_v1, id, transport}; + + #[maybe_async::test(feature = "blocking-client", async(feature = "async-client", async_std::test))] + async fn haves_and_wants_for_clone() { + let mut out = Vec::new(); + let mut t = transport(&mut out, true); + let mut arguments = arguments_v1(["feature-a", "feature-b"].iter().cloned()); + + arguments.want(id("7b333369de1221f9bfbbe03a3a13e9a09bc1c907")); + arguments.want(id("ff333369de1221f9bfbbe03a3a13e9a09bc1ffff")); + arguments.send(&mut t, true).await.expect("sending to buffer to work"); + assert_eq!( + out.as_bstr(), + b"0046want 7b333369de1221f9bfbbe03a3a13e9a09bc1c907 feature-a feature-b +0032want ff333369de1221f9bfbbe03a3a13e9a09bc1ffff +00000009done +" + .as_bstr() + ); + } + + #[maybe_async::test(feature = "blocking-client", async(feature = "async-client", async_std::test))] + async fn haves_and_wants_for_fetch_stateless() { + let mut out = Vec::new(); + let mut t = transport(&mut out, false); + let mut arguments = arguments_v1(["feature-a", "shallow", "deepen-since", "deepen-not"].iter().copied()); + + arguments.deepen(1); + arguments.shallow(id("7b333369de1221f9bfbbe03a3a13e9a09bc1c9ff")); + arguments.want(id("7b333369de1221f9bfbbe03a3a13e9a09bc1c907")); + arguments.deepen_since(12345); + arguments.deepen_not("refs/heads/main".into()); + arguments.have(id("0000000000000000000000000000000000000000")); + arguments.send(&mut t, false).await.expect("sending to buffer to work"); + + arguments.have(id("1111111111111111111111111111111111111111")); + arguments.send(&mut t, true).await.expect("sending to buffer to work"); + assert_eq!( + out.as_bstr(), + b"005cwant 7b333369de1221f9bfbbe03a3a13e9a09bc1c907 feature-a shallow deepen-since deepen-not +0035shallow 7b333369de1221f9bfbbe03a3a13e9a09bc1c9ff +000ddeepen 1 +0017deepen-since 12345 +001fdeepen-not refs/heads/main +00000032have 0000000000000000000000000000000000000000 +0000005cwant 7b333369de1221f9bfbbe03a3a13e9a09bc1c907 feature-a shallow deepen-since deepen-not +0035shallow 7b333369de1221f9bfbbe03a3a13e9a09bc1c9ff +000ddeepen 1 +0017deepen-since 12345 +001fdeepen-not refs/heads/main +00000032have 1111111111111111111111111111111111111111 +0009done +" + .as_bstr() + ); + } + + #[maybe_async::test(feature = "blocking-client", async(feature = "async-client", async_std::test))] + async fn haves_and_wants_for_fetch_stateful() { + let mut out = Vec::new(); + let mut t = transport(&mut out, true); + let mut arguments = arguments_v1(["feature-a", "shallow"].iter().copied()); + + arguments.deepen(1); + arguments.want(id("7b333369de1221f9bfbbe03a3a13e9a09bc1c907")); + arguments.have(id("0000000000000000000000000000000000000000")); + arguments.send(&mut t, false).await.expect("sending to buffer to work"); + + arguments.have(id("1111111111111111111111111111111111111111")); + arguments.send(&mut t, true).await.expect("sending to buffer to work"); + assert_eq!( + out.as_bstr(), + b"0044want 7b333369de1221f9bfbbe03a3a13e9a09bc1c907 feature-a shallow +000ddeepen 1 +00000032have 0000000000000000000000000000000000000000 +00000032have 1111111111111111111111111111111111111111 +0009done +" + .as_bstr() + ); + } + } + + mod v2 { + use bstr::ByteSlice; + + use crate::fetch::tests::arguments::{arguments_v2, id, transport}; + + #[maybe_async::test(feature = "blocking-client", async(feature = "async-client", async_std::test))] + async fn haves_and_wants_for_clone_stateful() { + let mut out = Vec::new(); + let mut t = transport(&mut out, true); + let mut arguments = arguments_v2(["feature-a", "shallow"].iter().copied()); + + arguments.deepen(1); + arguments.deepen_relative(); + arguments.want(id("7b333369de1221f9bfbbe03a3a13e9a09bc1c907")); + arguments.want(id("ff333369de1221f9bfbbe03a3a13e9a09bc1ffff")); + arguments.send(&mut t, true).await.expect("sending to buffer to work"); + assert_eq!( + out.as_bstr(), + b"0012command=fetch +0001000ethin-pack +0010include-tag +000eofs-delta +000ddeepen 1 +0014deepen-relative +0032want 7b333369de1221f9bfbbe03a3a13e9a09bc1c907 +0032want ff333369de1221f9bfbbe03a3a13e9a09bc1ffff +0009done +0000" + .as_bstr(), + "we filter features/capabilities without value as these apparently shouldn't be listed (remote dies otherwise)" + ); + } + + #[maybe_async::test(feature = "blocking-client", async(feature = "async-client", async_std::test))] + async fn haves_and_wants_for_fetch_stateless_and_stateful() { + for is_stateful in &[false, true] { + let mut out = Vec::new(); + let mut t = transport(&mut out, *is_stateful); + let mut arguments = arguments_v2(Some("shallow")); + + arguments.deepen(1); + arguments.deepen_since(12345); + arguments.shallow(id("7b333369de1221f9bfbbe03a3a13e9a09bc1c9ff")); + arguments.want(id("7b333369de1221f9bfbbe03a3a13e9a09bc1c907")); + arguments.deepen_not("refs/heads/main".into()); + arguments.have(id("0000000000000000000000000000000000000000")); + arguments.send(&mut t, false).await.expect("sending to buffer to work"); + + arguments.have(id("1111111111111111111111111111111111111111")); + arguments.send(&mut t, true).await.expect("sending to buffer to work"); + assert_eq!( + out.as_bstr(), + b"0012command=fetch +0001000ethin-pack +0010include-tag +000eofs-delta +000ddeepen 1 +0017deepen-since 12345 +0035shallow 7b333369de1221f9bfbbe03a3a13e9a09bc1c9ff +0032want 7b333369de1221f9bfbbe03a3a13e9a09bc1c907 +001fdeepen-not refs/heads/main +0032have 0000000000000000000000000000000000000000 +00000012command=fetch +0001000ethin-pack +0010include-tag +000eofs-delta +000ddeepen 1 +0017deepen-since 12345 +0035shallow 7b333369de1221f9bfbbe03a3a13e9a09bc1c9ff +0032want 7b333369de1221f9bfbbe03a3a13e9a09bc1c907 +001fdeepen-not refs/heads/main +0032have 1111111111111111111111111111111111111111 +0009done +0000" + .as_bstr(), + "V2 is stateless by default, so it repeats all but 'haves' in each request" + ); + } + } + + #[maybe_async::test(feature = "blocking-client", async(feature = "async-client", async_std::test))] + async fn ref_in_want() { + let mut out = Vec::new(); + let mut t = transport(&mut out, false); + let mut arguments = arguments_v2(["ref-in-want"].iter().copied()); + + arguments.want_ref(b"refs/heads/main".as_bstr()); + arguments.send(&mut t, true).await.expect("sending to buffer to work"); + assert_eq!( + out.as_bstr(), + b"0012command=fetch +0001000ethin-pack +0010include-tag +000eofs-delta +001dwant-ref refs/heads/main +0009done +0000" + .as_bstr() + ) + } + } +} diff --git a/git-protocol/src/fetch/tests/arguments.rs b/git-protocol/src/fetch/tests/arguments.rs deleted file mode 100644 index 334e571ab15..00000000000 --- a/git-protocol/src/fetch/tests/arguments.rs +++ /dev/null @@ -1,333 +0,0 @@ -use bstr::ByteSlice; -use git_transport::Protocol; - -use crate::fetch; - -fn arguments_v1(features: impl IntoIterator) -> fetch::Arguments { - fetch::Arguments::new(Protocol::V1, features.into_iter().map(|n| (n, None)).collect()) -} - -fn arguments_v2(features: impl IntoIterator) -> fetch::Arguments { - fetch::Arguments::new(Protocol::V2, features.into_iter().map(|n| (n, None)).collect()) -} - -struct Transport { - inner: T, - stateful: bool, -} - -#[cfg(feature = "blocking-client")] -mod impls { - use std::borrow::Cow; - - use bstr::BStr; - use git_transport::{ - client, - client::{Error, MessageKind, RequestWriter, SetServiceResponse, WriteMode}, - Protocol, Service, - }; - - use crate::fetch::tests::arguments::Transport; - - impl client::TransportWithoutIO for Transport { - fn set_identity(&mut self, identity: client::Account) -> Result<(), Error> { - self.inner.set_identity(identity) - } - - fn request(&mut self, write_mode: WriteMode, on_into_read: MessageKind) -> Result, Error> { - self.inner.request(write_mode, on_into_read) - } - - fn to_url(&self) -> Cow<'_, BStr> { - self.inner.to_url() - } - - fn supported_protocol_versions(&self) -> &[Protocol] { - self.inner.supported_protocol_versions() - } - - fn connection_persists_across_multiple_requests(&self) -> bool { - self.stateful - } - - fn configure( - &mut self, - config: &dyn std::any::Any, - ) -> Result<(), Box> { - self.inner.configure(config) - } - } - - impl client::Transport for Transport { - fn handshake<'a>( - &mut self, - service: Service, - extra_parameters: &'a [(&'a str, Option<&'a str>)], - ) -> Result, Error> { - self.inner.handshake(service, extra_parameters) - } - } -} - -#[cfg(feature = "async-client")] -mod impls { - use std::borrow::Cow; - - use async_trait::async_trait; - use bstr::BStr; - use git_transport::{ - client, - client::{Error, MessageKind, RequestWriter, SetServiceResponse, WriteMode}, - Protocol, Service, - }; - - use crate::fetch::tests::arguments::Transport; - impl client::TransportWithoutIO for Transport { - fn set_identity(&mut self, identity: client::Account) -> Result<(), Error> { - self.inner.set_identity(identity) - } - - fn request(&mut self, write_mode: WriteMode, on_into_read: MessageKind) -> Result, Error> { - self.inner.request(write_mode, on_into_read) - } - - fn to_url(&self) -> Cow<'_, BStr> { - self.inner.to_url() - } - - fn supported_protocol_versions(&self) -> &[Protocol] { - self.inner.supported_protocol_versions() - } - - fn connection_persists_across_multiple_requests(&self) -> bool { - self.stateful - } - - fn configure( - &mut self, - config: &dyn std::any::Any, - ) -> Result<(), Box> { - self.inner.configure(config) - } - } - - #[async_trait(?Send)] - impl client::Transport for Transport { - async fn handshake<'a>( - &mut self, - service: Service, - extra_parameters: &'a [(&'a str, Option<&'a str>)], - ) -> Result, Error> { - self.inner.handshake(service, extra_parameters).await - } - } -} - -fn transport( - out: &mut Vec, - stateful: bool, -) -> Transport>> { - Transport { - inner: git_transport::client::git::Connection::new( - &[], - out, - Protocol::V1, // does not matter - b"does/not/matter".as_bstr().to_owned(), - None::<(&str, _)>, - git_transport::client::git::ConnectMode::Process, // avoid header to be sent - ), - stateful, - } -} - -fn id(hex: &str) -> git_hash::ObjectId { - git_hash::ObjectId::from_hex(hex.as_bytes()).expect("expect valid hex id") -} - -mod v1 { - use bstr::ByteSlice; - - use crate::fetch::tests::arguments::{arguments_v1, id, transport}; - - #[maybe_async::test(feature = "blocking-client", async(feature = "async-client", async_std::test))] - async fn haves_and_wants_for_clone() { - let mut out = Vec::new(); - let mut t = transport(&mut out, true); - let mut arguments = arguments_v1(["feature-a", "feature-b"].iter().cloned()); - - arguments.want(id("7b333369de1221f9bfbbe03a3a13e9a09bc1c907")); - arguments.want(id("ff333369de1221f9bfbbe03a3a13e9a09bc1ffff")); - arguments.send(&mut t, true).await.expect("sending to buffer to work"); - assert_eq!( - out.as_bstr(), - b"0046want 7b333369de1221f9bfbbe03a3a13e9a09bc1c907 feature-a feature-b -0032want ff333369de1221f9bfbbe03a3a13e9a09bc1ffff -00000009done -" - .as_bstr() - ); - } - - #[maybe_async::test(feature = "blocking-client", async(feature = "async-client", async_std::test))] - async fn haves_and_wants_for_fetch_stateless() { - let mut out = Vec::new(); - let mut t = transport(&mut out, false); - let mut arguments = arguments_v1(["feature-a", "shallow", "deepen-since", "deepen-not"].iter().copied()); - - arguments.deepen(1); - arguments.shallow(id("7b333369de1221f9bfbbe03a3a13e9a09bc1c9ff")); - arguments.want(id("7b333369de1221f9bfbbe03a3a13e9a09bc1c907")); - arguments.deepen_since(12345); - arguments.deepen_not("refs/heads/main".into()); - arguments.have(id("0000000000000000000000000000000000000000")); - arguments.send(&mut t, false).await.expect("sending to buffer to work"); - - arguments.have(id("1111111111111111111111111111111111111111")); - arguments.send(&mut t, true).await.expect("sending to buffer to work"); - assert_eq!( - out.as_bstr(), - b"005cwant 7b333369de1221f9bfbbe03a3a13e9a09bc1c907 feature-a shallow deepen-since deepen-not -0035shallow 7b333369de1221f9bfbbe03a3a13e9a09bc1c9ff -000ddeepen 1 -0017deepen-since 12345 -001fdeepen-not refs/heads/main -00000032have 0000000000000000000000000000000000000000 -0000005cwant 7b333369de1221f9bfbbe03a3a13e9a09bc1c907 feature-a shallow deepen-since deepen-not -0035shallow 7b333369de1221f9bfbbe03a3a13e9a09bc1c9ff -000ddeepen 1 -0017deepen-since 12345 -001fdeepen-not refs/heads/main -00000032have 1111111111111111111111111111111111111111 -0009done -" - .as_bstr() - ); - } - - #[maybe_async::test(feature = "blocking-client", async(feature = "async-client", async_std::test))] - async fn haves_and_wants_for_fetch_stateful() { - let mut out = Vec::new(); - let mut t = transport(&mut out, true); - let mut arguments = arguments_v1(["feature-a", "shallow"].iter().copied()); - - arguments.deepen(1); - arguments.want(id("7b333369de1221f9bfbbe03a3a13e9a09bc1c907")); - arguments.have(id("0000000000000000000000000000000000000000")); - arguments.send(&mut t, false).await.expect("sending to buffer to work"); - - arguments.have(id("1111111111111111111111111111111111111111")); - arguments.send(&mut t, true).await.expect("sending to buffer to work"); - assert_eq!( - out.as_bstr(), - b"0044want 7b333369de1221f9bfbbe03a3a13e9a09bc1c907 feature-a shallow -000ddeepen 1 -00000032have 0000000000000000000000000000000000000000 -00000032have 1111111111111111111111111111111111111111 -0009done -" - .as_bstr() - ); - } -} - -mod v2 { - use bstr::ByteSlice; - - use crate::fetch::tests::arguments::{arguments_v2, id, transport}; - - #[maybe_async::test(feature = "blocking-client", async(feature = "async-client", async_std::test))] - async fn haves_and_wants_for_clone_stateful() { - let mut out = Vec::new(); - let mut t = transport(&mut out, true); - let mut arguments = arguments_v2(["feature-a", "shallow"].iter().copied()); - - arguments.deepen(1); - arguments.deepen_relative(); - arguments.want(id("7b333369de1221f9bfbbe03a3a13e9a09bc1c907")); - arguments.want(id("ff333369de1221f9bfbbe03a3a13e9a09bc1ffff")); - arguments.send(&mut t, true).await.expect("sending to buffer to work"); - assert_eq!( - out.as_bstr(), - b"0012command=fetch -0001000ethin-pack -0010include-tag -000eofs-delta -000ddeepen 1 -0014deepen-relative -0032want 7b333369de1221f9bfbbe03a3a13e9a09bc1c907 -0032want ff333369de1221f9bfbbe03a3a13e9a09bc1ffff -0009done -0000" - .as_bstr(), - "we filter features/capabilities without value as these apparently shouldn't be listed (remote dies otherwise)" - ); - } - - #[maybe_async::test(feature = "blocking-client", async(feature = "async-client", async_std::test))] - async fn haves_and_wants_for_fetch_stateless_and_stateful() { - for is_stateful in &[false, true] { - let mut out = Vec::new(); - let mut t = transport(&mut out, *is_stateful); - let mut arguments = arguments_v2(Some("shallow")); - - arguments.deepen(1); - arguments.deepen_since(12345); - arguments.shallow(id("7b333369de1221f9bfbbe03a3a13e9a09bc1c9ff")); - arguments.want(id("7b333369de1221f9bfbbe03a3a13e9a09bc1c907")); - arguments.deepen_not("refs/heads/main".into()); - arguments.have(id("0000000000000000000000000000000000000000")); - arguments.send(&mut t, false).await.expect("sending to buffer to work"); - - arguments.have(id("1111111111111111111111111111111111111111")); - arguments.send(&mut t, true).await.expect("sending to buffer to work"); - assert_eq!( - out.as_bstr(), - b"0012command=fetch -0001000ethin-pack -0010include-tag -000eofs-delta -000ddeepen 1 -0017deepen-since 12345 -0035shallow 7b333369de1221f9bfbbe03a3a13e9a09bc1c9ff -0032want 7b333369de1221f9bfbbe03a3a13e9a09bc1c907 -001fdeepen-not refs/heads/main -0032have 0000000000000000000000000000000000000000 -00000012command=fetch -0001000ethin-pack -0010include-tag -000eofs-delta -000ddeepen 1 -0017deepen-since 12345 -0035shallow 7b333369de1221f9bfbbe03a3a13e9a09bc1c9ff -0032want 7b333369de1221f9bfbbe03a3a13e9a09bc1c907 -001fdeepen-not refs/heads/main -0032have 1111111111111111111111111111111111111111 -0009done -0000" - .as_bstr(), - "V2 is stateless by default, so it repeats all but 'haves' in each request" - ); - } - } - - #[maybe_async::test(feature = "blocking-client", async(feature = "async-client", async_std::test))] - async fn ref_in_want() { - let mut out = Vec::new(); - let mut t = transport(&mut out, false); - let mut arguments = arguments_v2(["ref-in-want"].iter().copied()); - - arguments.want_ref(b"refs/heads/main".as_bstr()); - arguments.send(&mut t, true).await.expect("sending to buffer to work"); - assert_eq!( - out.as_bstr(), - b"0012command=fetch -0001000ethin-pack -0010include-tag -000eofs-delta -001dwant-ref refs/heads/main -0009done -0000" - .as_bstr() - ) - } -} diff --git a/git-protocol/src/fetch/tests/mod.rs b/git-protocol/src/fetch/tests/mod.rs deleted file mode 100644 index 465ac0dc320..00000000000 --- a/git-protocol/src/fetch/tests/mod.rs +++ /dev/null @@ -1,4 +0,0 @@ -#[cfg(any(feature = "async-client", feature = "blocking-client"))] -mod arguments; -#[cfg(any(feature = "blocking-client", feature = "async-client"))] -mod refs; diff --git a/git-protocol/src/handshake/refs/async_io.rs b/git-protocol/src/handshake/refs/async_io.rs index 474b893ef49..f98e1011cf6 100644 --- a/git-protocol/src/handshake/refs/async_io.rs +++ b/git-protocol/src/handshake/refs/async_io.rs @@ -1,19 +1,17 @@ use futures_io::AsyncBufRead; -use futures_lite::AsyncBufReadExt; +use futures_lite::AsyncReadExt; use crate::handshake::{refs, refs::parse::Error, Ref}; +use bstr::ByteSlice; /// Parse refs from the given input line by line. Protocol V2 is required for this to succeed. pub async fn from_v2_refs(in_refs: &mut (dyn AsyncBufRead + Unpin)) -> Result, Error> { let mut out_refs = Vec::new(); - let mut line = String::new(); - loop { - line.clear(); - let bytes_read = in_refs.read_line(&mut line).await?; - if bytes_read == 0 { - break; - } - out_refs.push(refs::shared::parse_v2(&line)?); + let mut buf = Vec::new(); + + in_refs.read_to_end(&mut buf).await?; + for line in ByteSlice::lines(buf.as_slice()) { + out_refs.push(refs::shared::parse_v2(line.into())?); } Ok(out_refs) } @@ -32,14 +30,11 @@ pub async fn from_v1_refs_received_as_part_of_handshake_and_capabilities<'a>( ) -> Result, refs::parse::Error> { let mut out_refs = refs::shared::from_capabilities(capabilities)?; let number_of_possible_symbolic_refs_for_lookup = out_refs.len(); - let mut line = String::new(); - loop { - line.clear(); - let bytes_read = in_refs.read_line(&mut line).await?; - if bytes_read == 0 { - break; - } - refs::shared::parse_v1(number_of_possible_symbolic_refs_for_lookup, &mut out_refs, &line)?; + + let mut buf = Vec::new(); + in_refs.read_to_end(&mut buf).await?; + for line in buf.as_slice().lines() { + refs::shared::parse_v1(number_of_possible_symbolic_refs_for_lookup, &mut out_refs, line.into())?; } Ok(out_refs.into_iter().map(Into::into).collect()) } diff --git a/git-protocol/src/handshake/refs/blocking_io.rs b/git-protocol/src/handshake/refs/blocking_io.rs index 5d55663bf85..7001bf3760b 100644 --- a/git-protocol/src/handshake/refs/blocking_io.rs +++ b/git-protocol/src/handshake/refs/blocking_io.rs @@ -1,18 +1,10 @@ -use std::io; - use crate::handshake::{refs, refs::parse::Error, Ref}; /// Parse refs from the given input line by line. Protocol V2 is required for this to succeed. -pub fn from_v2_refs(in_refs: &mut dyn io::BufRead) -> Result, Error> { +pub fn from_v2_refs(in_refs: &mut dyn git_transport::client::ReadlineBufRead) -> Result, Error> { let mut out_refs = Vec::new(); - let mut line = String::new(); - loop { - line.clear(); - let bytes_read = in_refs.read_line(&mut line)?; - if bytes_read == 0 { - break; - } - out_refs.push(refs::shared::parse_v2(&line)?); + while let Some(line) = in_refs.readline().transpose()?.transpose()?.and_then(|l| l.as_bstr()) { + out_refs.push(refs::shared::parse_v2(line)?); } Ok(out_refs) } @@ -26,19 +18,14 @@ pub fn from_v2_refs(in_refs: &mut dyn io::BufRead) -> Result, Error> { /// Symbolic refs are shoe-horned into server capabilities whereas refs (without symbolic ones) are sent automatically as /// part of the handshake. Both symbolic and peeled refs need to be combined to fit into the [`Ref`] type provided here. pub fn from_v1_refs_received_as_part_of_handshake_and_capabilities<'a>( - in_refs: &mut dyn io::BufRead, + in_refs: &mut dyn git_transport::client::ReadlineBufRead, capabilities: impl Iterator>, ) -> Result, Error> { let mut out_refs = refs::shared::from_capabilities(capabilities)?; let number_of_possible_symbolic_refs_for_lookup = out_refs.len(); - let mut line = String::new(); - loop { - line.clear(); - let bytes_read = in_refs.read_line(&mut line)?; - if bytes_read == 0 { - break; - } - refs::shared::parse_v1(number_of_possible_symbolic_refs_for_lookup, &mut out_refs, &line)?; + + while let Some(line) = in_refs.readline().transpose()?.transpose()?.and_then(|l| l.as_bstr()) { + refs::shared::parse_v1(number_of_possible_symbolic_refs_for_lookup, &mut out_refs, line)?; } Ok(out_refs.into_iter().map(Into::into).collect()) } diff --git a/git-protocol/src/handshake/refs/mod.rs b/git-protocol/src/handshake/refs/mod.rs index 5eeabf6d3d5..209fc90d7d8 100644 --- a/git-protocol/src/handshake/refs/mod.rs +++ b/git-protocol/src/handshake/refs/mod.rs @@ -13,17 +13,19 @@ pub mod parse { #[error(transparent)] Io(#[from] std::io::Error), #[error(transparent)] + DecodePacketline(#[from] git_transport::packetline::decode::Error), + #[error(transparent)] Id(#[from] git_hash::decode::Error), #[error("{symref:?} could not be parsed. A symref is expected to look like :.")] MalformedSymref { symref: BString }, #[error("{0:?} could not be parsed. A V1 ref line should be ' '.")] - MalformedV1RefLine(String), + MalformedV1RefLine(BString), #[error( "{0:?} could not be parsed. A V2 ref line should be ' [ (peeled|symref-target):'." )] - MalformedV2RefLine(String), + MalformedV2RefLine(BString), #[error("The ref attribute {attribute:?} is unknown. Found in line {line:?}")] - UnkownAttribute { attribute: String, line: String }, + UnkownAttribute { attribute: BString, line: BString }, #[error("{message}")] InvariantViolation { message: &'static str }, } @@ -65,3 +67,6 @@ pub use async_io::{from_v1_refs_received_as_part_of_handshake_and_capabilities, mod blocking_io; #[cfg(feature = "blocking-client")] pub use blocking_io::{from_v1_refs_received_as_part_of_handshake_and_capabilities, from_v2_refs}; + +#[cfg(test)] +mod tests; diff --git a/git-protocol/src/handshake/refs/shared.rs b/git-protocol/src/handshake/refs/shared.rs index 4ba356465b4..5e0d6c75aff 100644 --- a/git-protocol/src/handshake/refs/shared.rs +++ b/git-protocol/src/handshake/refs/shared.rs @@ -1,4 +1,4 @@ -use bstr::{BString, ByteSlice}; +use bstr::{BStr, BString, ByteSlice}; use crate::handshake::{refs::parse::Error, Ref}; @@ -70,7 +70,7 @@ impl InternalRef { _ => None, } } - fn lookup_symbol_has_path(&self, predicate_path: &str) -> bool { + fn lookup_symbol_has_path(&self, predicate_path: &BStr) -> bool { matches!(self, InternalRef::SymbolicForLookup { path, .. } if path == predicate_path) } } @@ -109,19 +109,19 @@ pub(crate) fn from_capabilities<'a>( pub(in crate::handshake::refs) fn parse_v1( num_initial_out_refs: usize, out_refs: &mut Vec, - line: &str, + line: &BStr, ) -> Result<(), Error> { let trimmed = line.trim_end(); let (hex_hash, path) = trimmed.split_at( trimmed - .find(' ') - .ok_or_else(|| Error::MalformedV1RefLine(trimmed.to_owned()))?, + .find(b" ") + .ok_or_else(|| Error::MalformedV1RefLine(trimmed.to_owned().into()))?, ); let path = &path[1..]; if path.is_empty() { - return Err(Error::MalformedV1RefLine(trimmed.to_owned())); + return Err(Error::MalformedV1RefLine(trimmed.to_owned().into())); } - match path.strip_suffix("^{}") { + match path.strip_suffix(b"^{}") { Some(stripped) => { let (previous_path, tag) = out_refs @@ -146,7 +146,7 @@ pub(in crate::handshake::refs) fn parse_v1( match out_refs .iter() .take(num_initial_out_refs) - .position(|r| r.lookup_symbol_has_path(path)) + .position(|r| r.lookup_symbol_has_path(path.into())) { Some(position) => match out_refs.swap_remove(position) { InternalRef::SymbolicForLookup { path: _, target } => out_refs.push(InternalRef::Symbolic { @@ -166,36 +166,36 @@ pub(in crate::handshake::refs) fn parse_v1( Ok(()) } -pub(in crate::handshake::refs) fn parse_v2(line: &str) -> Result { +pub(in crate::handshake::refs) fn parse_v2(line: &BStr) -> Result { let trimmed = line.trim_end(); - let mut tokens = trimmed.splitn(3, ' '); + let mut tokens = trimmed.splitn(3, |b| *b == b' '); match (tokens.next(), tokens.next()) { (Some(hex_hash), Some(path)) => { - let id = if hex_hash == "unborn" { + let id = if hex_hash == b"unborn" { None } else { Some(git_hash::ObjectId::from_hex(hex_hash.as_bytes())?) }; if path.is_empty() { - return Err(Error::MalformedV2RefLine(trimmed.to_owned())); + return Err(Error::MalformedV2RefLine(trimmed.to_owned().into())); } Ok(if let Some(attribute) = tokens.next() { - let mut tokens = attribute.splitn(2, ':'); + let mut tokens = attribute.splitn(2, |b| *b == b':'); match (tokens.next(), tokens.next()) { (Some(attribute), Some(value)) => { if value.is_empty() { - return Err(Error::MalformedV2RefLine(trimmed.to_owned())); + return Err(Error::MalformedV2RefLine(trimmed.to_owned().into())); } match attribute { - "peeled" => Ref::Peeled { + b"peeled" => Ref::Peeled { full_ref_name: path.into(), object: git_hash::ObjectId::from_hex(value.as_bytes())?, tag: id.ok_or(Error::InvariantViolation { message: "got 'unborn' as tag target", })?, }, - "symref-target" => match value { - "(null)" => Ref::Direct { + b"symref-target" => match value { + b"(null)" => Ref::Direct { full_ref_name: path.into(), object: id.ok_or(Error::InvariantViolation { message: "got 'unborn' while (null) was a symref target", @@ -215,13 +215,13 @@ pub(in crate::handshake::refs) fn parse_v2(line: &str) -> Result { }, _ => { return Err(Error::UnkownAttribute { - attribute: attribute.to_owned(), - line: trimmed.to_owned(), + attribute: attribute.to_owned().into(), + line: trimmed.to_owned().into(), }) } } } - _ => return Err(Error::MalformedV2RefLine(trimmed.to_owned())), + _ => return Err(Error::MalformedV2RefLine(trimmed.to_owned().into())), } } else { Ref::Direct { @@ -232,6 +232,6 @@ pub(in crate::handshake::refs) fn parse_v2(line: &str) -> Result { } }) } - _ => Err(Error::MalformedV2RefLine(trimmed.to_owned())), + _ => Err(Error::MalformedV2RefLine(trimmed.to_owned().into())), } } diff --git a/git-protocol/src/fetch/tests/refs.rs b/git-protocol/src/handshake/refs/tests.rs similarity index 80% rename from git-protocol/src/fetch/tests/refs.rs rename to git-protocol/src/handshake/refs/tests.rs index 35424af270e..fc76cb85a91 100644 --- a/git-protocol/src/fetch/tests/refs.rs +++ b/git-protocol/src/handshake/refs/tests.rs @@ -15,6 +15,8 @@ unborn refs/heads/symbolic symref-target:refs/heads/target " .as_bytes(); + #[cfg(feature = "blocking-client")] + let input = &mut Fixture(input); let out = refs::from_v2_refs(input).await.expect("no failure on valid input"); assert_eq!( @@ -56,6 +58,7 @@ unborn refs/heads/symbolic symref-target:refs/heads/target #[maybe_async::test(feature = "blocking-client", async(feature = "async-client", async_std::test))] async fn extract_references_from_v1_refs() { + #[cfg_attr(feature = "blocking-client", allow(unused_mut))] let input = &mut "73a6868963993a3328e7d8fe94e5a6ac5078a944 HEAD 21c9b7500cb144b3169a6537961ec2b9e865be81 MISSING_NAMESPACE_TARGET 73a6868963993a3328e7d8fe94e5a6ac5078a944 refs/heads/main @@ -63,6 +66,8 @@ async fn extract_references_from_v1_refs() { dce0ea858eef7ff61ad345cc5cdac62203fb3c10 refs/tags/git-commitgraph-v0.0.0 21c9b7500cb144b3169a6537961ec2b9e865be81 refs/tags/git-commitgraph-v0.0.0^{}" .as_bytes(); + #[cfg(feature = "blocking-client")] + let input = &mut Fixture(input); let out = refs::from_v1_refs_received_as_part_of_handshake_and_capabilities( input, Capabilities::from_bytes(b"\0symref=HEAD:refs/heads/main symref=MISSING_NAMESPACE_TARGET:(null)") @@ -106,7 +111,7 @@ fn extract_symbolic_references_from_capabilities() -> Result<(), client::Error> let caps = client::Capabilities::from_bytes( b"\0unrelated symref=HEAD:refs/heads/main symref=ANOTHER:refs/heads/foo symref=MISSING_NAMESPACE_TARGET:(null) agent=git/2.28.0", )? - .0; + .0; let out = refs::shared::from_capabilities(caps.iter()).expect("a working example"); assert_eq!( @@ -128,3 +133,38 @@ fn extract_symbolic_references_from_capabilities() -> Result<(), client::Error> ); Ok(()) } + +#[cfg(feature = "blocking-client")] +struct Fixture<'a>(&'a [u8]); + +#[cfg(feature = "blocking-client")] +impl<'a> std::io::Read for Fixture<'a> { + fn read(&mut self, buf: &mut [u8]) -> std::io::Result { + self.0.read(buf) + } +} + +#[cfg(feature = "blocking-client")] +impl<'a> std::io::BufRead for Fixture<'a> { + fn fill_buf(&mut self) -> std::io::Result<&[u8]> { + self.0.fill_buf() + } + + fn consume(&mut self, amt: usize) { + self.0.consume(amt) + } +} + +#[cfg(feature = "blocking-client")] +impl<'a> git_transport::client::ReadlineBufRead for Fixture<'a> { + fn readline( + &mut self, + ) -> Option, git_packetline::decode::Error>>> { + use bstr::{BStr, ByteSlice}; + let bytes: &BStr = self.0.into(); + let mut lines = bytes.lines(); + let res = lines.next()?; + self.0 = lines.as_bytes(); + Some(Ok(Ok(git_packetline::PacketLineRef::Data(res)))) + } +} From 5fe6aa3f3f2f81d84f0d96e874e68a8bf4de1db1 Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Wed, 30 Nov 2022 20:12:30 +0100 Subject: [PATCH 7/7] change!: environment variable permissions are per topic. Now `Permissions` for environment variables are so that they are by topic instead of by prefix, by default. That way it's easier to allow or deny particular sets of related environment variables. The catch-all permissions by prefix are still present for all other variables that aren't contained in one particular topic. --- git-repository/src/config/cache/init.rs | 52 ++++++++++---------- git-repository/src/repository/permissions.rs | 39 ++++++++------- git-repository/tests/repository/open.rs | 2 +- git-repository/tests/util/mod.rs | 1 + 4 files changed, 49 insertions(+), 45 deletions(-) diff --git a/git-repository/src/config/cache/init.rs b/git-repository/src/config/cache/init.rs index cd1bf05fbba..368c8075a1e 100644 --- a/git-repository/src/config/cache/init.rs +++ b/git-repository/src/config/cache/init.rs @@ -34,7 +34,7 @@ impl Cache { ssh_prefix: _, http_transport, identity, - gitoxide_prefix, + objects, }: repository::permissions::Environment, repository::permissions::Config { git_binary: use_installation, @@ -136,7 +136,7 @@ impl Cache { source: git_config::Source::Api, })?; } - apply_environment_overrides(&mut globals, *git_prefix, http_transport, identity, gitoxide_prefix)?; + apply_environment_overrides(&mut globals, *git_prefix, http_transport, identity, objects)?; globals }; @@ -253,7 +253,7 @@ fn apply_environment_overrides( git_prefix: Permission, http_transport: Permission, identity: Permission, - gitoxide_prefix: Permission, + objects: Permission, ) -> Result<(), Error> { fn var_as_bstring(var: &str, perm: Permission) -> Option { perm.check_opt(var) @@ -266,15 +266,15 @@ fn apply_environment_overrides( let mut section = env_override .new_section("http", None) .expect("statically known valid section name"); - for (var, key, permission) in [ - ("GIT_HTTP_LOW_SPEED_LIMIT", "lowSpeedLimit", git_prefix), - ("GIT_HTTP_LOW_SPEED_TIME", "lowSpeedTime", git_prefix), - ("GIT_HTTP_USER_AGENT", "userAgent", git_prefix), - ("GIT_HTTP_PROXY_AUTHMETHOD", "proxyAuthMethod", git_prefix), - ("all_proxy", "all-proxy-lower", http_transport), - ("ALL_PROXY", "all-proxy", http_transport), + for (var, key) in [ + ("GIT_HTTP_LOW_SPEED_LIMIT", "lowSpeedLimit"), + ("GIT_HTTP_LOW_SPEED_TIME", "lowSpeedTime"), + ("GIT_HTTP_USER_AGENT", "userAgent"), + ("GIT_HTTP_PROXY_AUTHMETHOD", "proxyAuthMethod"), + ("all_proxy", "all-proxy-lower"), + ("ALL_PROXY", "all-proxy"), ] { - if let Some(value) = var_as_bstring(var, permission) { + if let Some(value) = var_as_bstring(var, http_transport) { section.push_with_comment( key.try_into().expect("statically known to be valid"), Some(value.as_ref()), @@ -318,7 +318,7 @@ fn apply_environment_overrides( ("GIT_COMMITTER_NAME", "nameFallback"), ("GIT_COMMITTER_EMAIL", "emailFallback"), ] { - if let Some(value) = var_as_bstring(var, git_prefix) { + if let Some(value) = var_as_bstring(var, identity) { section.push_with_comment( key.try_into().expect("statically known to be valid"), Some(value.as_ref()), @@ -342,7 +342,7 @@ fn apply_environment_overrides( ("GIT_AUTHOR_NAME", "nameFallback"), ("GIT_AUTHOR_EMAIL", "emailFallback"), ] { - if let Some(value) = var_as_bstring(var, git_prefix) { + if let Some(value) = var_as_bstring(var, identity) { section.push_with_comment( key.try_into().expect("statically known to be valid"), Some(value.as_ref()), @@ -387,7 +387,7 @@ fn apply_environment_overrides( .expect("statically known valid section name"); for (var, key) in [("GIT_PROTOCOL_FROM_USER", "protocolFromUser")] { - if let Some(value) = var_as_bstring(var, git_prefix) { + if let Some(value) = var_as_bstring(var, http_transport) { section.push_with_comment( key.try_into().expect("statically known to be valid"), Some(value.as_ref()), @@ -429,9 +429,9 @@ fn apply_environment_overrides( .expect("statically known valid section name"); for (var, key, permission) in [ - ("GIT_NO_REPLACE_OBJECTS", "noReplace", git_prefix), - ("GIT_REPLACE_REF_BASE", "replaceRefBase", git_prefix), - ("GITOXIDE_OBJECT_CACHE_MEMORY", "cacheLimit", gitoxide_prefix), + ("GIT_NO_REPLACE_OBJECTS", "noReplace", objects), + ("GIT_REPLACE_REF_BASE", "replaceRefBase", objects), + ("GITOXIDE_OBJECT_CACHE_MEMORY", "cacheLimit", objects), ] { if let Some(value) = var_as_bstring(var, permission) { section.push_with_comment( @@ -454,7 +454,7 @@ fn apply_environment_overrides( .expect("statically known valid section name"); for (var, key) in [("GITOXIDE_PACK_CACHE_MEMORY", "deltaBaseCacheLimit")] { - if let Some(value) = var_as_bstring(var, gitoxide_prefix) { + if let Some(value) = var_as_bstring(var, objects) { section.push_with_comment( key.try_into().expect("statically known to be valid"), Some(value.as_ref()), @@ -474,15 +474,15 @@ fn apply_environment_overrides( .new_section("gitoxide", Some(Cow::Borrowed("http".into()))) .expect("statically known valid section name"); - for (var, key, permission) in [ - ("ALL_PROXY", "allProxy", http_transport), - ("all_proxy", "allProxy", http_transport), - ("NO_PROXY", "noProxy", http_transport), - ("no_proxy", "noProxy", http_transport), - ("http_proxy", "proxy", http_transport), - ("GIT_CURL_VERBOSE", "verbose", git_prefix), + for (var, key) in [ + ("ALL_PROXY", "allProxy"), + ("all_proxy", "allProxy"), + ("NO_PROXY", "noProxy"), + ("no_proxy", "noProxy"), + ("http_proxy", "proxy"), + ("GIT_CURL_VERBOSE", "verbose"), ] { - if let Some(value) = var_as_bstring(var, permission) { + if let Some(value) = var_as_bstring(var, http_transport) { section.push_with_comment( key.try_into().expect("statically known to be valid"), Some(value.as_ref()), diff --git a/git-repository/src/repository/permissions.rs b/git-repository/src/repository/permissions.rs index 45dfa046e6d..aab98c6799b 100644 --- a/git-repository/src/repository/permissions.rs +++ b/git-repository/src/repository/permissions.rs @@ -68,35 +68,38 @@ pub struct Environment { pub xdg_config_home: git_sec::Permission, /// Control the way resources pointed to by the home directory (similar to `xdg_config_home`) may be used. pub home: git_sec::Permission, - /// Control if resources pointed to by `GIT_*` prefixed environment variables can be used. - pub git_prefix: git_sec::Permission, - /// Control if resources pointed to by `SSH_*` prefixed environment variables can be used (like `SSH_ASKPASS`) - pub ssh_prefix: git_sec::Permission, /// Control if environment variables to configure the HTTP transport, like `http_proxy` may be used. /// - /// Note that http-transport related environment variables prefixed with `GIT_` are falling under the - /// `git_prefix` permission, like `GIT_HTTP_USER_AGENT`. + /// Note that http-transport related environment variables prefixed with `GIT_` may also be included here + /// if they match this category like `GIT_HTTP_USER_AGENT`. pub http_transport: git_sec::Permission, /// Control if the `EMAIL` environment variables may be read. /// - /// Note that identity related environment variables prefixed with `GIT_` are falling under the - /// `git_prefix` permission, like `GIT_AUTHOR_NAME`. + /// Note that identity related environment variables prefixed with `GIT_` may also be included here + /// if they match this category. pub identity: git_sec::Permission, - /// Decide if `gitoxide` specific variables may be read, prefixed with `GITOXIDE_`. - pub gitoxide_prefix: git_sec::Permission, + /// Control if environment variables related to the object database are handled. This includes features and performance + /// options alike. + pub objects: git_sec::Permission, + /// Control if resources pointed to by `GIT_*` prefixed environment variables can be used, **but only** if they + /// are not contained in any other category. This is a catch-all section. + pub git_prefix: git_sec::Permission, + /// Control if resources pointed to by `SSH_*` prefixed environment variables can be used (like `SSH_ASKPASS`) + pub ssh_prefix: git_sec::Permission, } impl Environment { /// Allow access to the entire environment. pub fn all() -> Self { + let allow = git_sec::Permission::Allow; Environment { - xdg_config_home: git_sec::Permission::Allow, - home: git_sec::Permission::Allow, - git_prefix: git_sec::Permission::Allow, - ssh_prefix: git_sec::Permission::Allow, - http_transport: git_sec::Permission::Allow, - identity: git_sec::Permission::Allow, - gitoxide_prefix: git_sec::Permission::Allow, + xdg_config_home: allow, + home: allow, + git_prefix: allow, + ssh_prefix: allow, + http_transport: allow, + identity: allow, + objects: allow, } } } @@ -143,7 +146,7 @@ impl Permissions { git_prefix: deny, http_transport: deny, identity: deny, - gitoxide_prefix: deny, + objects: deny, } }, } diff --git a/git-repository/tests/repository/open.rs b/git-repository/tests/repository/open.rs index 4cbd04bc333..a0fdc013027 100644 --- a/git-repository/tests/repository/open.rs +++ b/git-repository/tests/repository/open.rs @@ -123,7 +123,7 @@ mod with_overrides { opts.permissions.env.git_prefix = Permission::Allow; opts.permissions.env.http_transport = Permission::Allow; opts.permissions.env.identity = Permission::Allow; - opts.permissions.env.gitoxide_prefix = Permission::Allow; + opts.permissions.env.objects = Permission::Allow; let repo = named_subrepo_opts("make_config_repos.sh", "http-config", opts)?; let config = repo.config_snapshot(); assert_eq!( diff --git a/git-repository/tests/util/mod.rs b/git-repository/tests/util/mod.rs index f2aac4176de..3518ed5926e 100644 --- a/git-repository/tests/util/mod.rs +++ b/git-repository/tests/util/mod.rs @@ -35,6 +35,7 @@ pub fn restricted() -> open::Options { pub fn restricted_and_git() -> open::Options { let mut opts = open::Options::isolated(); opts.permissions.env.git_prefix = git_sec::Permission::Allow; + opts.permissions.env.identity = git_sec::Permission::Allow; opts }