From d408f8236bc71c607a452ac91e27f552931566ba Mon Sep 17 00:00:00 2001 From: Antoine Lambert Date: Wed, 18 Mar 2026 19:29:02 +0100 Subject: [PATCH 1/2] client: Fix revision parameter parsing when it is set to None When the revision or peg_revision parameter from numerous client methods is set to None from Python it must be parsed on the rust side as subversion::Revision::Unspecified to avoid unexpected behaviors when calling related client methods. This was the previous behavior with subvertpy < 0.12 with the extension modules written in C. Also the logic to select the right revision when revision or peg_revision parameters are set as unspecified is already handled by the libsvn_subr C library so there is no need to do it from rust. --- client/src/context.rs | 157 +++++++++++++----------------------------- tests/test_client.py | 21 ++++++ 2 files changed, 67 insertions(+), 111 deletions(-) diff --git a/client/src/context.rs b/client/src/context.rs index 59cadcce..caa2b0b9 100644 --- a/client/src/context.rs +++ b/client/src/context.rs @@ -4,26 +4,30 @@ use pyo3::prelude::*; use std::sync::mpsc::{self, Receiver}; /// Parse a revision from Python - can be a string ("HEAD", "BASE", etc.) or integer -fn parse_revision(_py: Python, rev: &Bound) -> PyResult { - if let Ok(s) = rev.extract::() { - match s.to_uppercase().as_str() { - "HEAD" => Ok(subversion::Revision::Head), - "BASE" => Ok(subversion::Revision::Base), - "WORKING" => Ok(subversion::Revision::Working), - "COMMITTED" => Ok(subversion::Revision::Committed), - "PREV" => Ok(subversion::Revision::Previous), - _ => Err(PyErr::new::(format!( - "Invalid revision string: {}", - s - ))), +fn parse_revision(_py: Python, rev: &Option>) -> PyResult { + if let Some(r) = rev { + if let Ok(s) = r.extract::() { + match s.to_uppercase().as_str() { + "HEAD" => Ok(subversion::Revision::Head), + "BASE" => Ok(subversion::Revision::Base), + "WORKING" => Ok(subversion::Revision::Working), + "COMMITTED" => Ok(subversion::Revision::Committed), + "PREV" => Ok(subversion::Revision::Previous), + _ => Err(PyErr::new::(format!( + "Invalid revision string: {}", + s + ))), + } + } else if let Ok(n) = r.extract::() { + let revnum = subvertpy_util::to_revnum_or_head(n); + Ok(subversion::Revision::Number(revnum)) + } else { + Err(PyErr::new::( + "Revision must be a string or integer", + )) } - } else if let Ok(n) = rev.extract::() { - let revnum = subvertpy_util::to_revnum_or_head(n); - Ok(subversion::Revision::Number(revnum)) } else { - Err(PyErr::new::( - "Revision must be a string or integer", - )) + Ok(subversion::Revision::Unspecified) } } @@ -137,17 +141,9 @@ impl Client { ignore_externals: bool, allow_unver_obstructions: bool, ) -> PyResult { - let revision = if let Some(r) = rev { - parse_revision(py, &r)? - } else { - subversion::Revision::Head - }; + let revision = parse_revision(py, &rev)?; - let peg_revision = if let Some(r) = peg_rev { - parse_revision(py, &r)? - } else { - subversion::Revision::Head - }; + let peg_revision = parse_revision(py, &peg_rev)?; let depth = if recurse { subversion::Depth::Infinity @@ -195,11 +191,7 @@ impl Client { )); }; - let revision = if let Some(r) = rev { - parse_revision(py, &r)? - } else { - subversion::Revision::Head - }; + let revision = parse_revision(py, &rev)?; let depth = if recurse { subversion::Depth::Infinity @@ -480,16 +472,9 @@ impl Client { limit: i32, include_merged_revisions: bool, ) -> PyResult<()> { - let start_revision = if let Some(r) = start_rev { - parse_revision(py, &r)? - } else { - subversion::Revision::Head - }; - let end_revision = if let Some(r) = end_rev { - parse_revision(py, &r)? - } else { - subversion::Revision::Number(subvertpy_util::to_revnum(1).unwrap()) - }; + let start_revision = parse_revision(py, &start_rev)?; + + let end_revision = parse_revision(py, &end_rev)?; let revision_ranges = vec![subversion::RevisionRange { start: start_revision, @@ -587,16 +572,9 @@ impl Client { revprops: Option>, include_merged_revisions: bool, ) -> PyResult> { - let start_revision = if let Some(r) = start_rev { - parse_revision(py, &r)? - } else { - subversion::Revision::Head - }; - let end_revision = if let Some(r) = end_rev { - parse_revision(py, &r)? - } else { - subversion::Revision::Number(subvertpy_util::to_revnum(1).unwrap()) - }; + let start_revision = parse_revision(py, &start_rev)?; + + let end_revision = parse_revision(py, &end_rev)?; let revision_ranges = vec![subversion::RevisionRange { start: start_revision, @@ -880,17 +858,9 @@ impl Client { peg_revision: Option>, recurse: bool, ) -> PyResult> { - let rev = if let Some(r) = revision { - parse_revision(py, &r)? - } else { - subversion::Revision::Working - }; + let rev = parse_revision(py, &revision)?; - let peg_rev = if let Some(r) = peg_revision { - parse_revision(py, &r)? - } else { - subversion::Revision::Unspecified - }; + let peg_rev = parse_revision(py, &peg_revision)?; let depth = if recurse { subversion::Depth::Infinity @@ -927,11 +897,7 @@ impl Client { revision: Option>, depth: i32, ) -> PyResult>> { - let rev = if let Some(r) = revision { - parse_revision(py, &r)? - } else { - subversion::Revision::Working - }; + let rev = parse_revision(py, &revision)?; let svn_depth = parse_depth_int(depth); @@ -1008,11 +974,8 @@ impl Client { current_dir.join(&path_str).to_string_lossy().to_string() }; - let rev = if let Some(r) = revision { - parse_revision(py, &r)? - } else { - subversion::Revision::Unspecified - }; + let rev = parse_revision(py, &revision)?; + // Match the old C behavior: default unspecified revision to HEAD let rev = if matches!(rev, subversion::Revision::Unspecified) { subversion::Revision::Head @@ -1020,11 +983,7 @@ impl Client { rev }; - let peg_rev = if let Some(r) = peg_revision { - parse_revision(py, &r)? - } else { - subversion::Revision::Unspecified - }; + let peg_rev = parse_revision(py, &peg_revision)?; let svn_depth = if let Some(d) = depth { parse_depth_int(d) @@ -1129,17 +1088,9 @@ impl Client { current_dir.join(&to_path_str).to_string_lossy().to_string() }; - let revision = if let Some(r) = rev { - parse_revision(py, &r)? - } else { - subversion::Revision::Head - }; + let revision = parse_revision(py, &rev)?; - let peg_revision = if let Some(r) = peg_rev { - parse_revision(py, &r)? - } else { - subversion::Revision::Unspecified - }; + let peg_revision = parse_revision(py, &peg_rev)?; let depth = if recurse { subversion::Depth::Infinity @@ -1198,8 +1149,8 @@ impl Client { ignore_content_type: bool, encoding: Option<&str>, ) -> PyResult<(Py, Py)> { - let revision1 = parse_revision(py, &rev1)?; - let revision2 = parse_revision(py, &rev2)?; + let revision1 = parse_revision(py, &Some(rev1))?; + let revision2 = parse_revision(py, &Some(rev2))?; let options = subversion::client::DiffOptions { diff_options: diffopts.unwrap_or_default(), @@ -1257,17 +1208,9 @@ impl Client { peg_revision: Option>, expand_keywords: bool, ) -> PyResult<()> { - let rev = if let Some(r) = revision { - parse_revision(py, &r)? - } else { - subversion::Revision::Working - }; + let rev = parse_revision(py, &revision)?; - let peg_rev = if let Some(r) = peg_revision { - parse_revision(py, &r)? - } else { - rev - }; + let peg_rev = parse_revision(py, &peg_revision)?; let options = subversion::client::CatOptions { peg_revision: peg_rev, @@ -1328,11 +1271,7 @@ impl Client { pin_externals: bool, callback: Option>, ) -> PyResult<()> { - let src_revision = if let Some(r) = src_rev { - Some(parse_revision(py, &r)?) - } else { - None - }; + let src_revision = parse_revision(py, &src_rev)?; let callback_py = callback.map(|cb| cb.unbind()); let mut commit_callback_fn = @@ -1371,7 +1310,7 @@ impl Client { }; self.ctx - .copy(&[(src, src_revision)], dst, &mut options) + .copy(&[(src, Some(src_revision))], dst, &mut options) .map_err(|e| subvertpy_util::error::svn_err_to_py(e))?; Ok(()) @@ -1387,11 +1326,7 @@ impl Client { depth: i32, include_externals: bool, ) -> PyResult> { - let rev = if let Some(r) = revision { - parse_revision(py, &r)? - } else { - subversion::Revision::Head - }; + let rev = parse_revision(py, &revision)?; let svn_depth = parse_depth_int(depth); diff --git a/tests/test_client.py b/tests/test_client.py index c74fe540..f4effd52 100644 --- a/tests/test_client.py +++ b/tests/test_client.py @@ -92,6 +92,27 @@ def test_export(self): self.client.export(self.repos_url, "de") self.assertEqual(["foo"], os.listdir("de")) + def test_export_with_peg_rev(self): + self.build_tree({"dc/foo.sh": b"echo foo"}) + self.client.add("dc/foo.sh") + self.client.log_msg_func = lambda c: "Commit" + self.client.commit(["dc"]) + + self.client.delete(["dc/foo.sh"]) + self.client.commit(["dc"]) + + self.build_tree({"dc/foo.sh": b"echo bar"}) + self.client.add("dc/foo.sh") + self.client.commit(["dc"]) + + self.client.export(self.repos_url, "peg_rev_1", peg_rev=1) + with open("peg_rev_1/foo.sh", "r") as foo: + self.assertEqual("echo foo", foo.read()) + + self.client.export(self.repos_url, "peg_rev_3", peg_rev=3) + with open("peg_rev_3/foo.sh", "r") as foo: + self.assertEqual("echo bar", foo.read()) + def test_export_new_option(self): self.build_tree({"dc/foo": b"bla"}) self.client.add("dc/foo") From f5e3152e23ca4c6fcbed41e45bb17b055a811055 Mon Sep 17 00:00:00 2001 From: Antoine Lambert Date: Wed, 18 Mar 2026 20:15:01 +0100 Subject: [PATCH 2/2] client: Add some missing peg_revision parameters Client.list and Client.proplist were offering the peg_revision parameter in subvertpy < 0.12 so restore them. --- client/src/context.rs | 14 ++++++++++---- tests/test_client.py | 6 +++--- 2 files changed, 13 insertions(+), 7 deletions(-) diff --git a/client/src/context.rs b/client/src/context.rs index caa2b0b9..9bbf6be2 100644 --- a/client/src/context.rs +++ b/client/src/context.rs @@ -889,20 +889,23 @@ impl Client { } /// List properties on a path - #[pyo3(signature = (path, revision=None, depth=0))] + #[pyo3(signature = (path, revision=None, peg_revision=None, depth=0))] fn proplist<'py>( &mut self, py: Python<'py>, path: &str, revision: Option>, + peg_revision: Option>, depth: i32, ) -> PyResult>> { let rev = parse_revision(py, &revision)?; + let peg_rev = parse_revision(py, &peg_revision)?; + let svn_depth = parse_depth_int(depth); let options = subversion::client::ProplistOptions { - peg_revision: subversion::Revision::Unspecified, + peg_revision: peg_rev, revision: rev, depth: svn_depth, changelists: None, @@ -1317,21 +1320,24 @@ impl Client { } /// List directory contents - #[pyo3(signature = (path_or_url, revision=None, depth=0, include_externals=false))] + #[pyo3(signature = (path_or_url, revision=None, peg_revision=None, depth=0, include_externals=false))] fn list<'py>( &mut self, py: Python<'py>, path_or_url: &str, revision: Option>, + peg_revision: Option>, depth: i32, include_externals: bool, ) -> PyResult> { let rev = parse_revision(py, &revision)?; + let peg_rev = parse_revision(py, &peg_revision)?; + let svn_depth = parse_depth_int(depth); let options = subversion::client::ListOptions { - peg_revision: subversion::Revision::Unspecified, + peg_revision: peg_rev, revision: rev, patterns: None, depth: svn_depth, diff --git a/tests/test_client.py b/tests/test_client.py index f4effd52..43e2b107 100644 --- a/tests/test_client.py +++ b/tests/test_client.py @@ -373,7 +373,7 @@ def test_proplist(self): self.client.commit(["dc"]) self.client.propset("myprop", "myval", "dc/foo", False, True) self.client.commit(["dc"]) - result = self.client.proplist("dc/foo", "WORKING", 0) + result = self.client.proplist("dc/foo", "WORKING", depth=0) self.assertIsInstance(result, list) def test_update(self): @@ -388,7 +388,7 @@ def test_list(self): self.client.add("dc/foo") self.client.log_msg_func = lambda c: "Commit" self.client.commit(["dc"]) - entries = self.client.list(self.repos_url, "HEAD", 0) + entries = self.client.list(self.repos_url, "HEAD", depth=0) self.assertIsInstance(entries, dict) def test_config(self): @@ -649,7 +649,7 @@ def test_list_include_externals(self): self.client.add("dc/listfile") self.client.log_msg_func = lambda c: "Commit" self.client.commit(["dc"]) - entries = self.client.list(self.repos_url, "HEAD", 0, include_externals=False) + entries = self.client.list(self.repos_url, "HEAD", depth=0, include_externals=False) self.assertIsInstance(entries, dict) def test_delete_with_callback(self):